Bug fixes - #9
Open
bdurand wants to merge 8 commits into
Open
Conversation
Greptile SummaryThis PR is a broad correctness fix for
Confidence Score: 4/5The PR fixes real bugs and is safe to merge; the changes are well-tested and the logic is sound throughout. All changes correct previously broken behavior and each fix is backed by a new test. The two main areas to watch are: (1) support_table_cacheable_scope? uses where_clause.send(:predicates) to access a private ActiveRecord method—if this API moves between Rails versions the guard silently stops working, causing cache bypasses rather than returning wrong data; (2) open_transaction? calls connection.current_transaction which is also an internal Rails method and could raise if its behavior changes. Neither is a correctness regression relative to the old code, but both create a future-compatibility dependency worth tracking. lib/support_table_cache/relation_override.rb (private-API predicate count) and lib/support_table_cache.rb (open_transaction? internals and where matching without type casting) are the places most likely to need revisiting on a Rails version upgrade. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Caller
participant FindByOverride
participant RelationOverride
participant SupportTableCache
participant Cache
Caller->>FindByOverride: Model.find_by(name: "One")
FindByOverride->>FindByOverride: args Hash? scoped? in transaction?
alt scoped (default scope / scoping block)
FindByOverride->>RelationOverride: super → find_by
RelationOverride->>RelationOverride: "support_table_cacheable_scope?<br/>(predicates == where_values_hash size,<br/>no joins/group/having/from/offset/lock)"
RelationOverride->>SupportTableCache: open_transaction?(klass)
alt inside joinable transaction
SupportTableCache-->>RelationOverride: true → bypass cache
RelationOverride->>Caller: DB result (not cached)
else
RelationOverride->>SupportTableCache: cache_key_for_query(klass, attrs)
SupportTableCache-->>RelationOverride: cache key or nil
RelationOverride->>Cache: "fetch(key) { DB query }"
Cache-->>Caller: record
end
else no scope
FindByOverride->>SupportTableCache: open_transaction?(self)
alt inside joinable transaction
SupportTableCache-->>FindByOverride: true → bypass cache
FindByOverride->>Caller: DB result (not cached)
else
FindByOverride->>SupportTableCache: cache_key_for_query(klass, attrs)
SupportTableCache-->>FindByOverride: cache key or nil
FindByOverride->>Cache: "fetch(key) { DB query }"
Cache-->>Caller: record
end
end
Note over Cache: Invalidation always runs<br/>(support_table_cache_for_invalidation<br/>ignores disabled flag)
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant Caller
participant FindByOverride
participant RelationOverride
participant SupportTableCache
participant Cache
Caller->>FindByOverride: Model.find_by(name: "One")
FindByOverride->>FindByOverride: args Hash? scoped? in transaction?
alt scoped (default scope / scoping block)
FindByOverride->>RelationOverride: super → find_by
RelationOverride->>RelationOverride: "support_table_cacheable_scope?<br/>(predicates == where_values_hash size,<br/>no joins/group/having/from/offset/lock)"
RelationOverride->>SupportTableCache: open_transaction?(klass)
alt inside joinable transaction
SupportTableCache-->>RelationOverride: true → bypass cache
RelationOverride->>Caller: DB result (not cached)
else
RelationOverride->>SupportTableCache: cache_key_for_query(klass, attrs)
SupportTableCache-->>RelationOverride: cache key or nil
RelationOverride->>Cache: "fetch(key) { DB query }"
Cache-->>Caller: record
end
else no scope
FindByOverride->>SupportTableCache: open_transaction?(self)
alt inside joinable transaction
SupportTableCache-->>FindByOverride: true → bypass cache
FindByOverride->>Caller: DB result (not cached)
else
FindByOverride->>SupportTableCache: cache_key_for_query(klass, attrs)
SupportTableCache-->>FindByOverride: cache key or nil
FindByOverride->>Cache: "fetch(key) { DB query }"
Cache-->>Caller: record
end
end
Note over Cache: Invalidation always runs<br/>(support_table_cache_for_invalidation<br/>ignores disabled flag)
Reviews (1): Last reviewed commit: "Bug fixes" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Owner
Author
|
@CodeRabbit full review |
✅ Action performedFull review finished. |
📝 WalkthroughWalkthroughSupportTableCache now restricts cache lookups to safe query shapes, normalizes typed cache keys, preserves invalidation while disabled, filters cache loading, and improves fiber-local, memory-cache, and association behavior. Tests and documentation cover the updated release 1.1.6 semantics. ChangesCache correctness and concurrency
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant RelationOverride
participant SupportTableCache
participant MemoryCache
participant Database
Caller->>RelationOverride: find_by query
RelationOverride->>SupportTableCache: validate query and build key
SupportTableCache->>MemoryCache: fetch cache key
MemoryCache-->>RelationOverride: cached record or miss
RelationOverride->>Database: execute query on miss
Database-->>RelationOverride: return record
RelationOverride->>MemoryCache: cache record
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/support_table_cache/memory_cache.rb`:
- Around line 26-39: Update the cache-miss fill flow around the lookup method
and its write call so a value fetched outside `@mutex` cannot be written after a
concurrent delete or clear. Coordinate fills with the existing invalidation
methods in SupportTableCache, either by holding the cache synchronization
through the fill or by tracking per-key generations and rejecting writes from
invalidated generations; preserve expiration and nil-value behavior.
In `@lib/support_table_cache/relation_override.rb`:
- Around line 89-102: Replace the internal where_clause.send(:predicates) call
in support_table_cacheable_scope? with the stable, supported where-clause
attribute API, preserving the existing comparison that rejects non-simple
conditions. Verify the chosen API across the project’s supported Rails version
range and add a focused compatibility spec if needed so future Rails changes
fail explicitly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 89109ee4-a762-47d8-a513-6bc1ba814363
📒 Files selected for processing (13)
CHANGELOG.mdREADME.mdVERSIONlib/support_table_cache.rblib/support_table_cache/associations.rblib/support_table_cache/fiber_locals.rblib/support_table_cache/find_by_override.rblib/support_table_cache/memory_cache.rblib/support_table_cache/relation_override.rbspec/spec_helper.rbspec/support_table_cache/associations_spec.rbspec/support_table_cache/fiber_locals_spec.rbspec/support_table_cache_spec.rb
- MemoryCache#fetch no longer stores a value generated concurrently with a delete or clear, which could resurrect a stale record after invalidation. Fills are coordinated with invalidation via a generation counter. - Cast values through the attribute type when matching cache_by where clauses in cache_key_for_query and load_cache so equivalent values (e.g. 1 and "1") match the same way cache keys are built. - Guard the internal Rails APIs used by support_table_cacheable_scope? (WhereClause#predicates) and open_transaction? (current_transaction and joinable?) so a future Rails change degrades to bypassing the cache instead of raising, document the dependencies, and add specs pinning the APIs so an incompatible Rails upgrade fails explicitly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/support_table_cache/relation_override.rb (1)
32-36: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not let explicit
find_byargs overwrite scoped predicateswhere(name: "Two").find_by(name: "One")must still behave likename = "Two" AND name = "One"and returnnil; merging here replaces the scoped value and can return a cached record the relation excludes. Bypass the cache when relation andfind_bykeys overlap.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/support_table_cache/relation_override.rb` around lines 32 - 36, Update the relation override logic around scope_conditions and attributes so overlapping keys between scoped predicates and explicit find_by arguments bypass the cache instead of merging values. Preserve normal cache lookup when the key sets do not overlap, while ensuring overlapping conditions are evaluated by the database with both predicates so scoped exclusions cannot return a cached record.lib/support_table_cache.rb (1)
127-130: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake
fetch_byreject queries that miss configuredwhereclauses
RelationOverride#fetch_bystill checks only attribute names, soTypedWhereModel.fetch_by(name: "typed-one")is accepted even though no cache entry can satisfywhere: {value: 1}and it falls back to the database. Include the configuredwherescope in the safety check sofetch_byraises when the query cannot hit the cache.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/support_table_cache.rb` around lines 127 - 130, The cache safety check used by RelationOverride#fetch_by must validate configured where clauses in addition to attribute names. Update the matching logic to compare the query’s where scope against each entry’s configured where value, preserving matches only when both attributes and where conditions are satisfied; otherwise raise instead of falling back to the database.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/support_table_cache/memory_cache.rb`:
- Around line 44-51: Update the cache write logic in the method containing
serialized_value and expire_at so replacing an expired entry without expires_in
clears the prior expiration timestamp. Ensure the stored entry uses nil or a
newly computed expiry for the replacement, rather than retaining the old
expire_at value.
---
Outside diff comments:
In `@lib/support_table_cache.rb`:
- Around line 127-130: The cache safety check used by RelationOverride#fetch_by
must validate configured where clauses in addition to attribute names. Update
the matching logic to compare the query’s where scope against each entry’s
configured where value, preserving matches only when both attributes and where
conditions are satisfied; otherwise raise instead of falling back to the
database.
In `@lib/support_table_cache/relation_override.rb`:
- Around line 32-36: Update the relation override logic around scope_conditions
and attributes so overlapping keys between scoped predicates and explicit
find_by arguments bypass the cache instead of merging values. Preserve normal
cache lookup when the key sets do not overlap, while ensuring overlapping
conditions are evaluated by the database with both predicates so scoped
exclusions cannot return a cached record.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7abfd83a-6e41-4765-a94d-8eec46f3b864
📒 Files selected for processing (7)
CHANGELOG.mdlib/support_table_cache.rblib/support_table_cache/memory_cache.rblib/support_table_cache/relation_override.rbspec/spec_helper.rbspec/support_table_cache/memory_cache_spec.rbspec/support_table_cache_spec.rb
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixed
not,or, joins,group,having,from,offset, locking, or non-hashfind_byarguments) now bypass the cache instead of silently ignoring those conditions and returning or caching the wrong record.create_withvalues are no longer included in cache keys, so queries usingcreate_withcan now be cached correctly.joinable: false(such as Rails transactional test fixtures) still use the cache.disableblock (or while caching was disabled globally) left stale entries behind for when caching was re-enabled.:oneand"one", or"5"and5) produce the same cache key. Previously such entries could be written under keys that the invalidation callbacks could never delete. This also fixes cache keys forfalseattribute values, which were previously indistinguishable fromnil.load_cacheno longer raises an error when caching is disabled and now honorswhereconditions oncache_byconfigurations instead of caching records that do not match them. It also refreshes existing cache entries instead of skipping them.cache_byconfiguration whosewhereclause does not match a query no longer prevents later configurations from matching, and no longer mutates the query attributes while matching.cache_byin a subclass no longer mutates the superclass's cache configuration.SupportTableCachewithout callingcache_byno longer raise an error onfind_by.cache_belongs_tomore than once for the same association no longer causes infinite recursion when reading the association.SupportTableCache::MemoryCachenow synchronizes all access to the underlying hash (previously reads, deletes, and clears were unsynchronized), purges expired entries, and no longer serializes values twice on a cache miss.SupportTableCache::FiberLocalsnow stores state in the fiber's native local storage so that state cannot leak from fibers that are garbage collected while suspended inside a block.Summary by CodeRabbit
whereconstraints, and ignoringcreate_withwhen building keys.cache_byinheritance/matching,find_byedge cases, repeated cache associations, and fixed race safety in memory cache and fiber-local state isolation.