From 130deed6677160516300469447a3a1dc126827fb Mon Sep 17 00:00:00 2001 From: Brian Durand Date: Sat, 4 Jul 2026 22:48:24 -0700 Subject: [PATCH 1/4] Bug fixes --- CHANGELOG.md | 17 +++ README.md | 10 ++ VERSION | 2 +- lib/support_table_cache.rb | 72 +++++++++-- lib/support_table_cache/associations.rb | 4 +- lib/support_table_cache/fiber_locals.rb | 53 ++++---- lib/support_table_cache/find_by_override.rb | 28 ++--- lib/support_table_cache/memory_cache.rb | 28 ++++- lib/support_table_cache/relation_override.rb | 48 ++++--- spec/spec_helper.rb | 15 +++ spec/support_table_cache/associations_spec.rb | 5 + spec/support_table_cache/fiber_locals_spec.rb | 23 +++- spec/support_table_cache_spec.rb | 119 ++++++++++++++++++ 13 files changed, 334 insertions(+), 90 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a78b15a..058f810 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,23 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## 1.1.6 + +### Fixed + +- Queries on relations with conditions that cannot be represented in the cache key (SQL string conditions, ranges, `not`, `or`, joins, `group`, `having`, `from`, `offset`, locking, or non-hash `find_by` arguments) now bypass the cache instead of silently ignoring those conditions and returning or caching the wrong record. +- `create_with` values are no longer included in cache keys, so queries using `create_with` can now be cached correctly. +- Queries inside an open transaction now bypass the cache so that uncommitted data can never be cached. Previously a rolled back transaction could permanently poison the cache with data that was never committed. Transactions created with `joinable: false` (such as Rails transactional test fixtures) still use the cache. +- Cache invalidation now runs even when caching is disabled. Previously records changed inside a `disable` block (or while caching was disabled globally) left stale entries behind for when caching was re-enabled. +- Cache key values are now cast through the attribute type, so equivalent values (e.g. `:one` and `"one"`, or `"5"` and `5`) 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 for `false` attribute values, which were previously indistinguishable from `nil`. +- `load_cache` no longer raises an error when caching is disabled and now honors `where` conditions on `cache_by` configurations instead of caching records that do not match them. It also refreshes existing cache entries instead of skipping them. +- A `cache_by` configuration whose `where` clause does not match a query no longer prevents later configurations from matching, and no longer mutates the query attributes while matching. +- Calling `cache_by` in a subclass no longer mutates the superclass's cache configuration. +- Models that include `SupportTableCache` without calling `cache_by` no longer raise an error on `find_by`. +- Calling `cache_belongs_to` more than once for the same association no longer causes infinite recursion when reading the association. +- `SupportTableCache::MemoryCache` now 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::FiberLocals` now 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. + ## 1.1.5 ### Fixed diff --git a/README.md b/README.md index 3b17652..443c1f7 100644 --- a/README.md +++ b/README.md @@ -89,6 +89,12 @@ You can also set a cache per class. For instance, you can set an in-memory cache Note that in-memory caches exist separately within each process and will not be cleared when records are changed in the database. The only way to refresh elements in an in-memory cache is to restart the process or set the `support_table_cache_ttl` value so that the entries will expire. +It is a good idea to always set `support_table_cache_ttl`, even when using a shared cache store. Cache invalidation happens when a record's changes are committed, but a process reading the old row at the same moment can still write the stale value back to the cache just after it was invalidated. A TTL puts an upper bound on how long such a stale entry can survive. + +The global cache and disabled settings (`SupportTableCache.cache=` and `SupportTableCache.disable` without a block) are intended to be set during application initialization and are not synchronized for concurrent modification at runtime. + +Queries made inside an open database transaction always bypass the cache. This prevents uncommitted data from being cached (which would never be cleared if the transaction were rolled back). Transactions created with `joinable: false`, such as the ones wrapping Rails transactional test fixtures, do not bypass the cache. + ### Disabling Caching You can disable the cache within a block either globally or only for a specific class. If the cache is disabled, then all queries will pass through to the database. @@ -108,6 +114,10 @@ SupportTableCache.enable do end ``` +Cache entries are still cleared when records are changed while caching is disabled, so re-enabling the cache will not expose stale data. + +Note that the class-level `disable_cache` and `enable_cache` settings apply only to the class they are called on; they do not apply to subclasses in a single table inheritance hierarchy. + ### Caching Belongs to Associations You can also cache belongs to associations to cacheable models. diff --git a/VERSION b/VERSION index e25d8d9..0664a8f 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -1.1.5 +1.1.6 diff --git a/lib/support_table_cache.rb b/lib/support_table_cache.rb index d419b1a..91daca8 100644 --- a/lib/support_table_cache.rb +++ b/lib/support_table_cache.rb @@ -28,6 +28,8 @@ module SupportTableCache # @api private class_attribute :support_table_cache_impl, instance_accessor: false + self.support_table_cache_by_attributes = [] + unless ActiveRecord::Relation.include?(RelationOverride) ActiveRecord::Relation.prepend(RelationOverride) end @@ -68,13 +70,15 @@ def enable_cache(&block) # @return [void] def load_cache cache = current_support_table_cache - return super if cache.nil? + return if cache.nil? find_each do |record| - support_table_cache_by_attributes.each do |attribute_names, case_sensitive| + support_table_cache_by_attributes.each do |attribute_names, case_sensitive, where| + next unless where.nil? || where.all? { |name, value| record[name] == value } + attributes = record.attributes.slice(*attribute_names) cache_key = SupportTableCache.cache_key(self, attributes, attribute_names, case_sensitive) - cache.fetch(cache_key, expires_in: support_table_cache_ttl) { record } + cache.write(cache_key, record, expires_in: support_table_cache_ttl) end end end @@ -120,9 +124,10 @@ def cache_by(attributes, case_sensitive: true, where: nil) where = where.stringify_keys end - self.support_table_cache_by_attributes ||= [] - support_table_cache_by_attributes.delete_if { |data| data.first == attributes } - self.support_table_cache_by_attributes += [[attributes, case_sensitive, where]] + # Build a new array rather than mutating the existing one since the class attribute + # value can be shared with the superclass. + existing = (support_table_cache_by_attributes || []).reject { |data| data.first == attributes } + self.support_table_cache_by_attributes = existing + [[attributes, case_sensitive, where]] end private @@ -138,6 +143,13 @@ def support_table_cache_disabled? def current_support_table_cache return nil if support_table_cache_disabled? + support_table_cache_for_invalidation + end + + # The cache used when removing entries. Invalidation must ignore the disabled flag; + # otherwise records changed while caching is disabled would leave stale entries behind + # for when caching is re-enabled. + def support_table_cache_for_invalidation SupportTableCache.testing_cache || support_table_cache_impl || SupportTableCache.cache end end @@ -241,7 +253,11 @@ def cache_key(klass, attributes, key_attribute_names, case_sensitive) sorted_attributes = {} sorted_names.each do |attribute_name| - value = (attributes[attribute_name] || attributes[attribute_name.to_sym]) + value = (attributes.key?(attribute_name) ? attributes[attribute_name] : attributes[attribute_name.to_sym]) + # Cast the value through the attribute type so that equivalent values (e.g. a symbol + # and a string, or "5" and 5) always produce the same cache key. Otherwise entries + # could be written under keys that the invalidation callbacks can never delete. + value = klass.type_for_attribute(attribute_name).cast(value) if !case_sensitive && (value.is_a?(String) || value.is_a?(Symbol)) value = value.to_s.downcase end @@ -251,6 +267,44 @@ def cache_key(klass, attributes, key_attribute_names, case_sensitive) [klass.name, sorted_attributes] end + # Find the cache key for a query on a set of attributes by matching the attributes + # against the cacheable attribute configuration for a class. Returns nil if the + # query cannot be cached. + # + # @param klass [Class] The class that is being queried. + # @param attributes [Hash] The query attributes with stringified keys. + # @return [Array(String, Hash), nil] The cache key or nil if the query is not cacheable. + # @api private + def cache_key_for_query(klass, attributes) + return nil if attributes.blank? + + Array(klass.support_table_cache_by_attributes).each do |attribute_names, case_sensitive, where| + where_matched = where.nil? || where.all? { |name, value| attributes.include?(name) && attributes[name] == value } + next unless where_matched + + key_attributes = (where ? attributes.except(*where.keys) : attributes) + key = cache_key(klass, key_attributes, attribute_names, case_sensitive) + return key if key + end + + nil + end + + # Return true if there is an open transaction on the class' connection. Queries should + # not be cached inside a transaction since they could return uncommitted data that would + # be invalid if the transaction is rolled back. Transactions opened with joinable: false + # (i.e. Rails transactional test fixtures) are ignored. + # + # @param klass [Class] The model class being queried. + # @return [Boolean] + # @api private + def open_transaction?(klass) + return false unless klass.connection_pool.active_connection? + + connection = klass.connection + connection.transaction_open? && connection.current_transaction.joinable? + end + def fiber_local_value(varname) @fiber_locals[varname] end @@ -267,7 +321,7 @@ def uncache cache_by_attributes = self.class.support_table_cache_by_attributes return if cache_by_attributes.blank? - cache = self.class.send(:current_support_table_cache) + cache = self.class.send(:support_table_cache_for_invalidation) return if cache.nil? cache_by_attributes.each do |attribute_names, case_sensitive| @@ -289,7 +343,7 @@ def support_table_clear_cache_entries cache_by_attributes = self.class.support_table_cache_by_attributes return if cache_by_attributes.blank? - cache = self.class.send(:current_support_table_cache) + cache = self.class.send(:support_table_cache_for_invalidation) return if cache.nil? cache_by_attributes.each do |attribute_names, case_sensitive| diff --git a/lib/support_table_cache/associations.rb b/lib/support_table_cache/associations.rb index 46bbada..d05bce2 100644 --- a/lib/support_table_cache/associations.rb +++ b/lib/support_table_cache/associations.rb @@ -50,7 +50,9 @@ def #{association_name}_with_cache end end - alias_method :#{association_name}_without_cache, :#{association_name} + unless method_defined?(:#{association_name}_without_cache) || private_method_defined?(:#{association_name}_without_cache) + alias_method :#{association_name}_without_cache, :#{association_name} + end alias_method :#{association_name}, :#{association_name}_with_cache RUBY end diff --git a/lib/support_table_cache/fiber_locals.rb b/lib/support_table_cache/fiber_locals.rb index 7884ea3..e460634 100644 --- a/lib/support_table_cache/fiber_locals.rb +++ b/lib/support_table_cache/fiber_locals.rb @@ -1,51 +1,40 @@ # frozen_string_literal: true module SupportTableCache - # Utility class for managing fiber-local variables. This implementation - # does not pollute the global namespace. + # Utility class for managing fiber-local variables. All values are stored in a single + # hash inside the fiber's native local storage (Thread.current[], which is fiber-local + # in Ruby) so the fiber-local namespace is not polluted with individual keys. Because + # the state lives on the fiber itself, it is garbage collected along with the fiber + # and cannot leak or be picked up by another fiber. class FiberLocals def initialize - @mutex = Mutex.new - @locals = {} + @locals_key = :"support_table_cache_locals_#{object_id}" end def [](key) - fiber_locals = nil - @mutex.synchronize do - fiber_locals = @locals[Fiber.current.object_id] - end - return nil if fiber_locals.nil? - - fiber_locals[key] + locals = Thread.current[@locals_key] + locals[key] if locals end def with(key, value) - fiber_id = Fiber.current.object_id - fiber_locals = nil - previous_value = nil - inited_vars = false - - begin - @mutex.synchronize do - fiber_locals = @locals[fiber_id] - if fiber_locals.nil? - fiber_locals = {} - @locals[fiber_id] = fiber_locals - inited_vars = true - end - end + locals = Thread.current[@locals_key] + if locals.nil? + locals = {} + Thread.current[@locals_key] = locals + end - previous_value = fiber_locals[key] - fiber_locals[key] = value + exists = locals.key?(key) + previous_value = locals[key] + locals[key] = value + begin yield ensure - if inited_vars - @mutex.synchronize do - @locals.delete(fiber_id) - end + if exists + locals[key] = previous_value else - fiber_locals[key] = previous_value + locals.delete(key) + Thread.current[@locals_key] = nil if locals.empty? end end end diff --git a/lib/support_table_cache/find_by_override.rb b/lib/support_table_cache/find_by_override.rb index 31fc027..9ad1f83 100644 --- a/lib/support_table_cache/find_by_override.rb +++ b/lib/support_table_cache/find_by_override.rb @@ -8,26 +8,18 @@ def find_by(*args) cache = current_support_table_cache return super unless cache - cache_key = nil - attributes = ((args.size == 1 && args.first.is_a?(Hash)) ? args.first.stringify_keys : {}) + # Only queries by simple attribute equality can be matched against cache keys. + return super unless args.size == 1 && args.first.is_a?(Hash) - if respond_to?(:scope_attributes) && scope_attributes.present? - attributes = scope_attributes.stringify_keys.merge(attributes) - end + # If the class has any scope applied (a default scope or a scoping block), defer to + # the relation override, which checks whether the scoped query can be cached. + return super if all.values.present? - if attributes.present? - support_table_cache_by_attributes.each do |attribute_names, case_sensitive, where| - where&.each do |name, value| - if attributes.include?(name) && attributes[name] == value - attributes.delete(name) - else - return super - end - end - cache_key = SupportTableCache.cache_key(self, attributes, attribute_names, case_sensitive) - break if cache_key - end - end + # Queries inside a transaction could see uncommitted data that would be invalid + # if the transaction is rolled back, so they cannot be cached. + return super if SupportTableCache.open_transaction?(self) + + cache_key = SupportTableCache.cache_key_for_query(self, args.first.stringify_keys) if cache_key cache.fetch(cache_key, expires_in: support_table_cache_ttl) { super } diff --git a/lib/support_table_cache/memory_cache.rb b/lib/support_table_cache/memory_cache.rb index 44405fb..87b7420 100644 --- a/lib/support_table_cache/memory_cache.rb +++ b/lib/support_table_cache/memory_cache.rb @@ -23,13 +23,21 @@ def initialize # @yield Block to execute to get a new value if the key is not cached. # @return [Object, nil] The cached value or the result of the block, or nil if no value is found. def fetch(key, expires_in: nil) - serialized_value, expire_at = @cache[key] - if serialized_value.nil? || (expire_at && expire_at < Process.clock_gettime(Process::CLOCK_MONOTONIC)) + serialized_value = nil + @mutex.synchronize do + serialized_value, expire_at = @cache[key] + if expire_at && expire_at < Process.clock_gettime(Process::CLOCK_MONOTONIC) + @cache.delete(key) + serialized_value = nil + end + end + + if serialized_value.nil? value = yield if block_given? return nil if value.nil? - write(key, value, expires_in: expires_in) - serialized_value = Marshal.dump(value) + serialized_value = write(key, value, expires_in: expires_in) end + Marshal.load(serialized_value) end @@ -59,6 +67,8 @@ def write(key, value, expires_in: nil) @mutex.synchronize do @cache[key] = [serialized_value, expire_at] end + + serialized_value end # Delete a value from the cache. @@ -66,14 +76,20 @@ def write(key, value, expires_in: nil) # @param key [Object] The cache key. # @return [void] def delete(key) - @cache.delete(key) + @mutex.synchronize do + @cache.delete(key) + end + nil end # Clear all values from the cache. # # @return [void] def clear - @cache.clear + @mutex.synchronize do + @cache.clear + end + nil end end end diff --git a/lib/support_table_cache/relation_override.rb b/lib/support_table_cache/relation_override.rb index 6970562..bdf8207 100644 --- a/lib/support_table_cache/relation_override.rb +++ b/lib/support_table_cache/relation_override.rb @@ -19,27 +19,23 @@ def find_by(*args) return super if select_values.present? - cache_key = nil - attributes = ((args.size == 1 && args.first.is_a?(Hash)) ? args.first.stringify_keys : {}) + # Only queries by simple attribute equality can be matched against cache keys. + simple_attribute_query = args.empty? || (args.size == 1 && args.first.is_a?(Hash)) + return super unless simple_attribute_query - # Apply any attributes from the current relation chain - if scope_attributes.present? - attributes = scope_attributes.stringify_keys.merge(attributes) - end + return super unless support_table_cacheable_scope? - if attributes.present? - support_table_cache_by_attributes.each do |attribute_names, case_sensitive, where| - where&.each do |name, value| - if attributes.include?(name) && attributes[name] == value - attributes.delete(name) - else - return super - end - end - cache_key = SupportTableCache.cache_key(klass, attributes, attribute_names, case_sensitive) - break if cache_key - end - end + # Queries inside a transaction could see uncommitted data that would be invalid + # if the transaction is rolled back, so they cannot be cached. + return super if SupportTableCache.open_transaction?(klass) + + attributes = (args.first || {}).stringify_keys + + # Apply any conditions from the current relation chain + scope_conditions = where_values_hash.stringify_keys + attributes = scope_conditions.merge(attributes) if scope_conditions.present? + + cache_key = SupportTableCache.cache_key_for_query(klass, attributes) if cache_key cache.fetch(cache_key, expires_in: support_table_cache_ttl) { super } @@ -90,6 +86,20 @@ def fetch_by!(attributes) private + # A relation can only be cached if all of its conditions are simple equality conditions + # on the model's own attributes that can be represented in a cache key. Anything else + # (SQL string conditions, ranges, OR clauses, joins, etc.) is not visible in + # where_values_hash and could silently change which record the query returns. + def support_table_cacheable_scope? + return false unless where_clause.send(:predicates).size == where_values_hash.size + return false if joins_values.present? || left_outer_joins_values.present? + return false if group_values.present? || !having_clause.empty? + return false if !from_clause.empty? || offset_value.present? || lock_value + return false if eager_loading? + + true + end + def support_table_find_by_attribute_names(attributes) attributes ||= {} if scope_attributes.present? diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index c81e8a7..51a6308 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -70,6 +70,21 @@ class DefaultScopeModel < ActiveRecord::Base default_scope { where(deleted_at: nil) } end +class WhereConditionModel < ActiveRecord::Base + include SupportTableCache + + self.table_name = "default_scope_models" + + cache_by :name, where: {deleted_at: nil} + cache_by :label +end + +class NoCacheByModel < ActiveRecord::Base + include SupportTableCache + + self.table_name = "things" +end + class Thing < ActiveRecord::Base has_many :other_things has_many :test_models, through: :other_things diff --git a/spec/support_table_cache/associations_spec.rb b/spec/support_table_cache/associations_spec.rb index a690dca..4470e08 100644 --- a/spec/support_table_cache/associations_spec.rb +++ b/spec/support_table_cache/associations_spec.rb @@ -14,6 +14,11 @@ expect(parent.test_model).to eq record end + it "does not break the association when cache_belongs_to is called more than once" do + ParentModel.cache_belongs_to :test_model + expect(parent.test_model).to eq record + end + it "does not cache if the cache is set to nil" do cache = SupportTableCache.cache begin diff --git a/spec/support_table_cache/fiber_locals_spec.rb b/spec/support_table_cache/fiber_locals_spec.rb index 05bd6ba..695f592 100644 --- a/spec/support_table_cache/fiber_locals_spec.rb +++ b/spec/support_table_cache/fiber_locals_spec.rb @@ -180,7 +180,7 @@ end it "does not leak memory across fibers" do - initial_locals_count = fiber_locals.instance_variable_get(:@locals).size + locals_key = fiber_locals.instance_variable_get(:@locals_key) 100.times do Fiber.new do @@ -190,9 +190,24 @@ end.resume end - # Locals should be cleaned up for completed fibers - final_locals_count = fiber_locals.instance_variable_get(:@locals).size - expect(final_locals_count).to be <= initial_locals_count + 1 + # State is stored on each fiber, so nothing is retained by the FiberLocals instance + # and the current fiber's storage is cleaned up when the block exits. + expect(Thread.current[locals_key]).to be_nil + end + + it "does not retain state for a fiber that dies inside a with block" do + locals_key = fiber_locals.instance_variable_get(:@locals_key) + + fiber = Fiber.new do + fiber_locals.with(:key, :value) do + Fiber.yield + end + end + fiber.resume + # The fiber is suspended inside the block and never resumed; its state lives only + # on the fiber itself, so other fibers are unaffected. + expect(fiber_locals[:key]).to be_nil + expect(Thread.current[locals_key]).to be_nil end end diff --git a/spec/support_table_cache_spec.rb b/spec/support_table_cache_spec.rb index bed6480..6674c60 100644 --- a/spec/support_table_cache_spec.rb +++ b/spec/support_table_cache_spec.rb @@ -26,6 +26,13 @@ key = SupportTableCache.cache_key(TestModel, {group: "first", code: "one"}, ["code", "name"], false) expect(key).to eq nil end + + it "normalizes equivalent attribute values to the same cache key" do + expect(SupportTableCache.cache_key(TestModel, {name: :One}, ["name"], true)) + .to eq SupportTableCache.cache_key(TestModel, {name: "One"}, ["name"], true) + expect(SupportTableCache.cache_key(TestModel, {value: "1"}, ["value"], true)) + .to eq SupportTableCache.cache_key(TestModel, {value: 1}, ["value"], true) + end end describe "cache_by" do @@ -33,6 +40,14 @@ expect(TestModel.support_table_cache_by_attributes.size).to eq 2 expect(Subclass.support_table_cache_by_attributes.size).to eq 1 end + + it "does not modify the parent class configuration when a subclass overrides it" do + parent_config = TestModel.support_table_cache_by_attributes.dup + Class.new(TestModel) do + cache_by :name, case_sensitive: false + end + expect(TestModel.support_table_cache_by_attributes).to eq parent_config + end end describe "finding" do @@ -86,6 +101,28 @@ expect(TestModel.find_by(value: 1)).to eq record_1 expect(TestModel.find_by(name: "One", value: 1)).to eq record_1 end + + it "does not use the cache when find_by is called with non-hash arguments" do + expect(TestModel.find_by(name: "One")).to eq record_1 # prime the cache + expect(TestModel.find_by("value > 100")).to be_nil + end + + it "does not error on a model that includes the concern without any cache_by" do + thing = NoCacheByModel.create!(name: "no cache by") + expect(NoCacheByModel.find_by(name: "no cache by")).to eq thing + end + + it "invalidates entries created with equivalent attribute values" do + TestModel.support_table_cache = :memory + begin + expect(TestModel.find_by(name: :One).value).to eq 1 + record_1.update!(value: 42) + expect(TestModel.find_by(name: :One).value).to eq 42 + expect(TestModel.find_by(name: "One").value).to eq 42 + ensure + TestModel.support_table_cache = nil + end + end end describe "finding on a relation" do @@ -132,6 +169,61 @@ expect(TestModel.select(:id, :name).find_by(name: "One")).to eq record_1 expect(SupportTableCache.cache.read(SupportTableCache.cache_key(TestModel, {name: "One"}, ["name"], true))).to eq nil end + + it "does not use the cache when the relation has conditions that cannot be represented in the cache key" do + expect(TestModel.find_by(name: "One")).to eq record_1 # prime the cache + expect(TestModel.where("value > 100").find_by(name: "One")).to be_nil + expect(TestModel.where(value: 100..200).find_by(name: "One")).to be_nil + expect(TestModel.where.not(value: 1).find_by(name: "One")).to be_nil + expect(TestModel.where(name: "One").find_by("value > 100")).to be_nil + end + + it "does not populate the cache from a relation with conditions not in the cache key" do + cache_key = SupportTableCache.cache_key(TestModel, {name: "One"}, ["name"], true) + expect(TestModel.where("value > 100").find_by(name: "One")).to be_nil + expect(SupportTableCache.cache.read(cache_key)).to be_nil + end + + it "ignores create_with values when building the cache key" do + expect(TestModel.create_with(value: 100).where(group: "First").find_by(code: "one")).to eq record_1 + expect(SupportTableCache.cache.read(SupportTableCache.cache_key(TestModel, {code: "one", group: "First"}, ["code", "group"], false))).to eq record_1 + end + end + + describe "finding inside a transaction" do + it "does not use or populate the cache inside a transaction" do + cache_key = SupportTableCache.cache_key(TestModel, {name: "One"}, ["name"], true) + TestModel.transaction do + expect(TestModel.find_by(name: "One")).to eq record_1 + end + expect(SupportTableCache.cache.read(cache_key)).to be_nil + end + + it "does not cache uncommitted data from a rolled back transaction" do + TestModel.transaction do + record_1.update!(value: 500) + expect(TestModel.find_by(name: "One").value).to eq 500 + raise ActiveRecord::Rollback + end + expect(record_1.reload.value).to eq 1 + expect(TestModel.find_by(name: "One").value).to eq 1 + end + + it "uses the cache inside a non-joinable transaction like transactional test fixtures" do + cache_key = SupportTableCache.cache_key(TestModel, {name: "One"}, ["name"], true) + TestModel.connection.transaction(joinable: false) do + expect(TestModel.find_by(name: "One")).to eq record_1 + end + expect(SupportTableCache.cache.read(cache_key)).to eq record_1 + end + end + + describe "finding with a where condition on the cache configuration" do + it "uses the cache for a config declared after a config with a non-matching where clause" do + record = WhereConditionModel.create!(name: "n1", label: "l1") + expect(WhereConditionModel.find_by(label: "l1")).to eq record + expect(SupportTableCache.cache.read(SupportTableCache.cache_key(WhereConditionModel, {label: "l1"}, ["label"], true))).to eq record + end end describe "finding with a default scope" do @@ -284,6 +376,14 @@ expect(blocks_executed).to eq true end + + it "still clears cache entries when a record is changed while caching is disabled" do + expect(TestModel.find_by(name: "One").value).to eq 1 + SupportTableCache.disable do + record_1.update!(value: 42) + end + expect(TestModel.find_by(name: "One").value).to eq 42 + end end describe "setting the cache" do @@ -359,6 +459,25 @@ expect(cache.read(cache_key)).to eq record end end + + it "does nothing when caching is disabled" do + expect { SupportTableCache.disable { TestModel.load_cache } }.to_not raise_error + end + + it "does not cache records that do not match a where condition" do + active = WhereConditionModel.create!(name: "active-one") + deleted = WhereConditionModel.create!(name: "deleted-one", deleted_at: Time.now) + cache = ActiveSupport::Cache::MemoryStore.new + WhereConditionModel.support_table_cache = cache + begin + WhereConditionModel.load_cache + expect(cache.read(SupportTableCache.cache_key(WhereConditionModel, {name: "active-one"}, ["name"], true))).to eq active + expect(cache.read(SupportTableCache.cache_key(WhereConditionModel, {name: "deleted-one"}, ["name"], true))).to be_nil + expect(deleted.reload.name).to eq "deleted-one" + ensure + WhereConditionModel.support_table_cache = nil + end + end end describe "testing!" do From 03d2513865c6bd5a396c605068c0580c9893589f Mon Sep 17 00:00:00 2001 From: Brian Durand Date: Sun, 19 Jul 2026 12:02:30 -0700 Subject: [PATCH 2/4] Address PR review feedback - 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 --- CHANGELOG.md | 3 +- lib/support_table_cache.rb | 18 ++++++-- lib/support_table_cache/memory_cache.rb | 20 ++++++++- lib/support_table_cache/relation_override.rb | 6 +++ spec/spec_helper.rb | 9 ++++ spec/support_table_cache/memory_cache_spec.rb | 18 ++++++++ spec/support_table_cache_spec.rb | 42 +++++++++++++++++++ 7 files changed, 110 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 058f810..87184af 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,7 +18,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Calling `cache_by` in a subclass no longer mutates the superclass's cache configuration. - Models that include `SupportTableCache` without calling `cache_by` no longer raise an error on `find_by`. - Calling `cache_belongs_to` more than once for the same association no longer causes infinite recursion when reading the association. -- `SupportTableCache::MemoryCache` now 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::MemoryCache` now 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. A `fetch` that races with a concurrent `delete` or `clear` no longer stores its stale value back in the cache. +- The `where` clause on a `cache_by` configuration is now matched against query attributes using values cast through the attribute type, so equivalent values (e.g. `1` and `"1"`) match the same way they do when building cache keys. - `SupportTableCache::FiberLocals` now 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. ## 1.1.5 diff --git a/lib/support_table_cache.rb b/lib/support_table_cache.rb index 91daca8..e8031c8 100644 --- a/lib/support_table_cache.rb +++ b/lib/support_table_cache.rb @@ -74,7 +74,7 @@ def load_cache find_each do |record| support_table_cache_by_attributes.each do |attribute_names, case_sensitive, where| - next unless where.nil? || where.all? { |name, value| record[name] == value } + next unless where.nil? || where.all? { |name, value| record[name] == type_for_attribute(name).cast(value) } attributes = record.attributes.slice(*attribute_names) cache_key = SupportTableCache.cache_key(self, attributes, attribute_names, case_sensitive) @@ -279,7 +279,12 @@ def cache_key_for_query(klass, attributes) return nil if attributes.blank? Array(klass.support_table_cache_by_attributes).each do |attribute_names, case_sensitive, where| - where_matched = where.nil? || where.all? { |name, value| attributes.include?(name) && attributes[name] == value } + # Cast both sides through the attribute type so that equivalent values (e.g. 1 and "1") + # match the where clause the same way they are matched when building cache keys. + where_matched = where.nil? || where.all? do |name, value| + type = klass.type_for_attribute(name) + attributes.include?(name) && type.cast(attributes[name]) == type.cast(value) + end next unless where_matched key_attributes = (where ? attributes.except(*where.keys) : attributes) @@ -302,7 +307,14 @@ def open_transaction?(klass) return false unless klass.connection_pool.active_connection? connection = klass.connection - connection.transaction_open? && connection.current_transaction.joinable? + return false unless connection.transaction_open? + + # current_transaction and joinable? are internal Rails APIs. If a future Rails version + # changes them, fail safe by treating the transaction as open (bypassing the cache) + # rather than raising. There is a spec asserting the API exists so an incompatible + # Rails upgrade fails explicitly. + transaction = connection.current_transaction + !transaction.respond_to?(:joinable?) || transaction.joinable? end def fiber_local_value(varname) diff --git a/lib/support_table_cache/memory_cache.rb b/lib/support_table_cache/memory_cache.rb index 87b7420..30764cd 100644 --- a/lib/support_table_cache/memory_cache.rb +++ b/lib/support_table_cache/memory_cache.rb @@ -14,6 +14,9 @@ class MemoryCache def initialize @cache = {} @mutex = Mutex.new + # Incremented on every delete or clear so that a fetch that was generating a value + # while the cache was invalidated will not store a stale value. + @generation = 0 end # Fetch a value from the cache. If the key is not found or has expired, yields to get a new value. @@ -24,7 +27,9 @@ def initialize # @return [Object, nil] The cached value or the result of the block, or nil if no value is found. def fetch(key, expires_in: nil) serialized_value = nil + generation = nil @mutex.synchronize do + generation = @generation serialized_value, expire_at = @cache[key] if expire_at && expire_at < Process.clock_gettime(Process::CLOCK_MONOTONIC) @cache.delete(key) @@ -35,7 +40,16 @@ def fetch(key, expires_in: nil) if serialized_value.nil? value = yield if block_given? return nil if value.nil? - serialized_value = write(key, value, expires_in: expires_in) + + serialized_value = Marshal.dump(value) + expire_at = Process.clock_gettime(Process::CLOCK_MONOTONIC) + expires_in if expires_in + + @mutex.synchronize do + # Only store the value if the cache was not invalidated while the value was being + # generated. Otherwise a record deleted by a concurrent invalidation could be + # resurrected in the cache with stale data. + @cache[key] = [serialized_value, expire_at] if @generation == generation + end end Marshal.load(serialized_value) @@ -68,7 +82,7 @@ def write(key, value, expires_in: nil) @cache[key] = [serialized_value, expire_at] end - serialized_value + nil end # Delete a value from the cache. @@ -77,6 +91,7 @@ def write(key, value, expires_in: nil) # @return [void] def delete(key) @mutex.synchronize do + @generation += 1 @cache.delete(key) end nil @@ -87,6 +102,7 @@ def delete(key) # @return [void] def clear @mutex.synchronize do + @generation += 1 @cache.clear end nil diff --git a/lib/support_table_cache/relation_override.rb b/lib/support_table_cache/relation_override.rb index bdf8207..00f0d92 100644 --- a/lib/support_table_cache/relation_override.rb +++ b/lib/support_table_cache/relation_override.rb @@ -91,6 +91,12 @@ def fetch_by!(attributes) # (SQL string conditions, ranges, OR clauses, joins, etc.) is not visible in # where_values_hash and could silently change which record the query returns. def support_table_cacheable_scope? + # WhereClause#predicates is not part of the public Rails API (its visibility has varied + # across Rails versions), but there is no public way to detect conditions that cannot + # be represented in where_values_hash. If the method is ever removed in a future Rails + # version, fail safe by bypassing the cache rather than raising. There is a spec + # asserting the method exists so an incompatible Rails upgrade fails explicitly. + return false unless where_clause.respond_to?(:predicates, true) return false unless where_clause.send(:predicates).size == where_values_hash.size return false if joins_values.present? || left_outer_joins_values.present? return false if group_values.present? || !having_clause.empty? diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 51a6308..fa90469 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -79,6 +79,15 @@ class WhereConditionModel < ActiveRecord::Base cache_by :label end +class TypedWhereModel < ActiveRecord::Base + include SupportTableCache + + self.table_name = "test_models" + + cache_by :name, where: {value: 1} + cache_by :code, where: {value: "1"} +end + class NoCacheByModel < ActiveRecord::Base include SupportTableCache diff --git a/spec/support_table_cache/memory_cache_spec.rb b/spec/support_table_cache/memory_cache_spec.rb index 15606e5..1b15c10 100644 --- a/spec/support_table_cache/memory_cache_spec.rb +++ b/spec/support_table_cache/memory_cache_spec.rb @@ -50,5 +50,23 @@ expect(value).to eq :baz end + it "does not store a value if the key is deleted while the value is being generated" do + value = cache.fetch("foo") do + cache.delete("foo") + :bar + end + expect(value).to eq :bar + expect(cache.read("foo")).to eq nil + end + + it "does not store a value if the cache is cleared while the value is being generated" do + value = cache.fetch("foo") do + cache.clear + :bar + end + expect(value).to eq :bar + expect(cache.read("foo")).to eq nil + end + # rubocop:enable Style/RedundantFetchBlock end diff --git a/spec/support_table_cache_spec.rb b/spec/support_table_cache_spec.rb index 6674c60..f2e85ad 100644 --- a/spec/support_table_cache_spec.rb +++ b/spec/support_table_cache_spec.rb @@ -224,6 +224,12 @@ expect(WhereConditionModel.find_by(label: "l1")).to eq record expect(SupportTableCache.cache.read(SupportTableCache.cache_key(WhereConditionModel, {label: "l1"}, ["label"], true))).to eq record end + + it "casts values through the attribute type when matching the where clause" do + record = TypedWhereModel.create!(name: "typed-one", value: 1) + expect(TypedWhereModel.find_by(name: "typed-one", value: "1")).to eq record + expect(SupportTableCache.cache.read(SupportTableCache.cache_key(TypedWhereModel, {name: "typed-one"}, ["name"], true))).to eq record + end end describe "finding with a default scope" do @@ -478,6 +484,19 @@ WhereConditionModel.support_table_cache = nil end end + + it "casts where condition values through the attribute type when matching records" do + record = TypedWhereModel.create!(name: "typed-one", code: "typed-code", value: 1) + cache = ActiveSupport::Cache::MemoryStore.new + TypedWhereModel.support_table_cache = cache + begin + TypedWhereModel.load_cache + expect(cache.read(SupportTableCache.cache_key(TypedWhereModel, {name: "typed-one"}, ["name"], true))).to eq record + expect(cache.read(SupportTableCache.cache_key(TypedWhereModel, {code: "typed-code"}, ["code"], true))).to eq record + ensure + TypedWhereModel.support_table_cache = nil + end + end end describe "testing!" do @@ -497,4 +516,27 @@ expect(SupportTableCache.cache).to eq normal_cache end end + + # These specs pin the internal Rails APIs that the gem depends on. The code fails safe by + # bypassing the cache if these APIs disappear, so these specs exist to make an incompatible + # Rails upgrade fail explicitly instead of silently degrading performance. + describe "internal Rails API dependencies" do + it "can count the predicates on a relation's where clause" do + where_clause = TestModel.where(name: "One", value: 1..10).send(:where_clause) + expect(where_clause.respond_to?(:predicates, true)).to be true + expect(where_clause.send(:predicates).size).to eq 2 + end + + it "can detect whether the current transaction is joinable" do + TestModel.connection.transaction(joinable: false) do + transaction = TestModel.connection.current_transaction + expect(transaction).to respond_to(:joinable?) + expect(transaction.joinable?).to be false + end + + TestModel.transaction do + expect(TestModel.connection.current_transaction.joinable?).to be true + end + end + end end From 95a972a71902f73365c03e088fc1218cf1d044b7 Mon Sep 17 00:00:00 2001 From: Brian Durand Date: Wed, 22 Jul 2026 09:11:18 -0700 Subject: [PATCH 3/4] update gem setup --- .github/workflows/continuous_integration.yml | 50 ++++++-------------- .gitignore | 36 ++++++++++---- .standard.yml | 9 +--- Gemfile | 23 +++++---- Rakefile | 29 +++++++++++- gemfiles/activerecord_5.gemfile | 8 +--- gemfiles/activerecord_6.gemfile | 8 +--- gemfiles/activerecord_7.gemfile | 8 +--- gemfiles/activerecord_8.gemfile | 8 +--- support_table_cache.gemspec | 4 +- 10 files changed, 95 insertions(+), 88 deletions(-) diff --git a/.github/workflows/continuous_integration.yml b/.github/workflows/continuous_integration.yml index 091d8cf..657f4cc 100644 --- a/.github/workflows/continuous_integration.yml +++ b/.github/workflows/continuous_integration.yml @@ -8,12 +8,7 @@ on: pull_request: branches-ignore: - actions-* - -env: - BUNDLE_CLEAN: "true" - BUNDLE_PATH: vendor/bundle - BUNDLE_JOBS: 3 - BUNDLE_RETRY: 3 + workflow_dispatch: jobs: build: @@ -34,33 +29,16 @@ jobs: - ruby: "2.5" appraisal: "activerecord_5" steps: - - uses: actions/checkout@v4 - - name: Set up Ruby ${{ matrix.ruby }} - uses: ruby/setup-ruby@v1 - with: - ruby-version: "${{ matrix.ruby }}" - - name: Install packages - run: | - sudo apt-get update - sudo apt-get install libsqlite3-dev - - name: Setup bundler - if: matrix.bundler != '' - run: | - gem uninstall bundler --all - gem install bundler --no-document --version ${{ matrix.bundler }} - - name: Set Appraisal bundle - if: matrix.appraisal != '' - run: | - echo "using gemfile gemfiles/${{ matrix.appraisal }}.gemfile" - bundle config set gemfile "gemfiles/${{ matrix.appraisal }}.gemfile" - - name: Install gems - run: | - bundle install - - name: Run Tests - run: bundle exec rake - - name: standardrb - if: matrix.standardrb == true - run: bundle exec standardrb - - name: yard - if: matrix.yard == true - run: bundle exec yard doc --fail-on-warning + - name: Checkout + uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 + with: + persist-credentials: false + - name: Ruby CI + uses: bdurand/github-actions/ruby-ci@b7560883d5f8557f5d69fe8bd82721bbb5ed76df # v1.0.4 + with: + ruby: ${{ matrix.ruby }} + appraisal: ${{ matrix.appraisal }} + bundler: ${{ matrix.bundler }} + standardrb: ${{ matrix.standardrb }} + yard: ${{ matrix.yard }} + frozen_strings: ${{ matrix.frozen_strings }} diff --git a/.gitignore b/.gitignore index 90f9586..72dc7e3 100644 --- a/.gitignore +++ b/.gitignore @@ -1,13 +1,33 @@ .DS_Store -.bundle/ + +/.bundle/ +/vendor/bundle +/lib/bundler/man/ + .rspec -.ruby-version -.yardoc/ .env +*.gem +*.rbc + +/spec/reports/ +/spec/examples.txt +/.yardoc/ +/_yardoc/ +/doc/ +/rdoc/ +/coverage/ +/log/*.log +/pkg/ +/tmp/ + +.ruby-version +.ruby-gemset Gemfile.lock -coverage/ gemfiles/*.gemfile.lock -log/*.log -pkg/ -rdoc/ -doc/ + +.byebug_history + +.claude/ +.conductor/ +.cursor/ +/.config diff --git a/.standard.yml b/.standard.yml index 36e2e7a..1699115 100644 --- a/.standard.yml +++ b/.standard.yml @@ -1,10 +1,3 @@ -ruby_version: 2.5 +ruby_version: 2.6 format: progress - -ignore: - - "**/*": - - Style/RedundantParentheses - - "spec/**/*": - - Lint/ConstantDefinitionInBlock - - Lint/UselessAssignment diff --git a/Gemfile b/Gemfile index cf50c4f..71eede4 100644 --- a/Gemfile +++ b/Gemfile @@ -2,11 +2,18 @@ source "https://rubygems.org" gemspec -gem "rspec", "~> 3.0" -gem "rake" -gem "irb" -gem "sqlite3" -gem "appraisal" -gem "standard", "~>1.0" -gem "pry-byebug" -gem "yard" +# Exclude development-only gems from dependabot. +unless ENV["DEPENDABOT"] + gem "sqlite3" + + gem "rake" + gem "rspec", "~> 3.11" + + # Exclude development-only gems from the gemfiles generated by appraisal. + unless defined?(::Appraisal::Gemfile) && is_a?(::Appraisal::Gemfile) + gem "appraisal", require: false + gem "standard", require: false + gem "simplecov", require: false + gem "yard", require: false + end +end diff --git a/Rakefile b/Rakefile index 3deea70..48f3eb5 100644 --- a/Rakefile +++ b/Rakefile @@ -1,3 +1,5 @@ +# frozen_string_literal: true + begin require "bundler/setup" rescue LoadError @@ -13,10 +15,33 @@ task :verify_release_branch do end end -Rake::Task[:release].enhance([:verify_release_branch]) +Rake::Task[:release].prerequisites.prepend("verify_release_branch") require "rspec/core/rake_task" RSpec::Core::RakeTask.new(:spec) -task default: :spec +task default: [:spec] + +namespace :appraisal do + desc "Update the appraisal gemfiles" + task :update do + Dir.glob("gemfiles/*.gemfile*") do |file| + File.delete(file) if File.file?(file) + end + + system "bundle exec appraisal generate" || abort("appraisal generate failed") + + Dir.glob("gemfiles/*.gemfile") do |file| + puts "Locking #{file}" + Bundler.with_unbundled_env do + system( + { + "BUNDLE_GEMFILE" => file + }, + "bundle", "lock", "--update" + ) || abort("appraisal lock failed on #{file}") + end + end + end +end diff --git a/gemfiles/activerecord_5.gemfile b/gemfiles/activerecord_5.gemfile index 336d1d3..82934d8 100644 --- a/gemfiles/activerecord_5.gemfile +++ b/gemfiles/activerecord_5.gemfile @@ -2,13 +2,9 @@ source "https://rubygems.org" -gem "rspec", "~> 3.0" -gem "rake" gem "sqlite3", "~> 1.3.0" -gem "appraisal" -gem "standard", "~>1.0" -gem "pry-byebug" -gem "yard" +gem "rake" +gem "rspec", "~> 3.11" gem "activerecord", "~> 5.0" gemspec path: "../" diff --git a/gemfiles/activerecord_6.gemfile b/gemfiles/activerecord_6.gemfile index ccf4270..ceaf9ce 100644 --- a/gemfiles/activerecord_6.gemfile +++ b/gemfiles/activerecord_6.gemfile @@ -2,13 +2,9 @@ source "https://rubygems.org" -gem "rspec", "~> 3.0" -gem "rake" gem "sqlite3", "~> 1.4.0" -gem "appraisal" -gem "standard", "~>1.0" -gem "pry-byebug" -gem "yard" +gem "rake" +gem "rspec", "~> 3.11" gem "activerecord", "~> 6.0" gem "concurrent-ruby", "1.3.4" diff --git a/gemfiles/activerecord_7.gemfile b/gemfiles/activerecord_7.gemfile index 7555ae1..bb90af0 100644 --- a/gemfiles/activerecord_7.gemfile +++ b/gemfiles/activerecord_7.gemfile @@ -2,13 +2,9 @@ source "https://rubygems.org" -gem "rspec", "~> 3.0" -gem "rake" gem "sqlite3", "~> 1.4.0" -gem "appraisal" -gem "standard", "~>1.0" -gem "pry-byebug" -gem "yard" +gem "rake" +gem "rspec", "~> 3.11" gem "activerecord", "~> 7.0" gemspec path: "../" diff --git a/gemfiles/activerecord_8.gemfile b/gemfiles/activerecord_8.gemfile index 6a073c3..2b08377 100644 --- a/gemfiles/activerecord_8.gemfile +++ b/gemfiles/activerecord_8.gemfile @@ -2,13 +2,9 @@ source "https://rubygems.org" -gem "rspec", "~> 3.0" -gem "rake" gem "sqlite3", "~> 2.5.0" -gem "appraisal" -gem "standard", "~>1.0" -gem "pry-byebug" -gem "yard" +gem "rake" +gem "rspec", "~> 3.11" gem "activerecord", "~> 8.0" gemspec path: "../" diff --git a/support_table_cache.gemspec b/support_table_cache.gemspec index cabb4f0..90a42ca 100644 --- a/support_table_cache.gemspec +++ b/support_table_cache.gemspec @@ -28,7 +28,7 @@ Gem::Specification.new do |spec| spec.require_paths = ["lib"] - spec.add_dependency "activerecord" + spec.required_ruby_version = ">= 2.6" - spec.add_development_dependency "bundler" + spec.add_dependency "activerecord" end From d285f841164b9450c1565b416e801107ac5aed9c Mon Sep 17 00:00:00 2001 From: Brian Durand Date: Wed, 22 Jul 2026 09:14:31 -0700 Subject: [PATCH 4/4] update .gitignore --- .gitignore | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/.gitignore b/.gitignore index 72dc7e3..a876bb2 100644 --- a/.gitignore +++ b/.gitignore @@ -1,6 +1,6 @@ .DS_Store -/.bundle/ +.bundle/ /vendor/bundle /lib/bundler/man/ @@ -11,8 +11,8 @@ /spec/reports/ /spec/examples.txt -/.yardoc/ -/_yardoc/ +.yardoc/ +_yardoc/ /doc/ /rdoc/ /coverage/ @@ -30,4 +30,4 @@ gemfiles/*.gemfile.lock .claude/ .conductor/ .cursor/ -/.config +.config