From 130deed6677160516300469447a3a1dc126827fb Mon Sep 17 00:00:00 2001 From: Brian Durand Date: Sat, 4 Jul 2026 22:48:24 -0700 Subject: [PATCH] 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