From 9f4bed3be41ada2feb42f426e187720a7961d8c9 Mon Sep 17 00:00:00 2001 From: Jianbin Chen Date: Wed, 9 Oct 2024 14:35:45 -0700 Subject: [PATCH] Merge the two methods into one --- lib/identity_cache.rb | 56 +++++++------------ lib/identity_cache/cached/attribute.rb | 4 +- lib/identity_cache/cached/primary_index.rb | 4 +- lib/identity_cache/parent_model_expiration.rb | 4 +- test/index_cache_test.rb | 44 +++++++-------- test/save_test.rb | 12 +--- test/test_helper.rb | 10 ++++ 7 files changed, 58 insertions(+), 76 deletions(-) diff --git a/lib/identity_cache.rb b/lib/identity_cache.rb index 026cc321..78e750bb 100644 --- a/lib/identity_cache.rb +++ b/lib/identity_cache.rb @@ -60,9 +60,7 @@ class AssociationError < StandardError; end class InverseAssociationError < StandardError; end - class NestedDeferredParentBlockError < StandardError; end - - class NestedDeferredAttributeExpirationBlockError < StandardError; end + class NestedDeferredCacheExpirationBlockError < StandardError; end class UnsupportedScopeError < StandardError; end @@ -204,59 +202,45 @@ def fetch_multi(*keys) result end - # Executes a block with deferred parent expiration, ensuring that the parent - # records' cache expiration is deferred until the block completes. When the block - # completes, it triggers expiration of the primary index for the parent records. - # Raises a NestedDeferredParentBlockError if a deferred parent expiration block - # is already active on the current thread. + # Executes a block with deferred cache expiration, ensuring that the records' (parent, + # children and attributes) cache expiration is deferred until the block completes. When + # the block completes, it issues delete_multi calls for all the records and attributes + # that were marked for expiration. # # == Parameters: # No parameters. # # == Raises: - # NestedDeferredParentBlockError if a deferred parent expiration block is already active. + # NestedDeferredCacheExpirationBlockError if a deferred cache expiration block is already active. # # == Yield: - # Runs the provided block with deferred parent expiration. + # Runs the provided block with deferred cache expiration. # # == Returns: # The result of executing the provided block. # # == Ensures: - # Cleans up thread-local variables related to deferred parent expiration regardless + # Cleans up thread-local variables related to deferred cache expiration regardless # of whether the block raises an exception. - def with_deferred_parent_expiration - raise NestedDeferredParentBlockError if Thread.current[:idc_deferred_parent_expiration] - - Thread.current[:idc_deferred_parent_expiration] = true - Thread.current[:idc_parent_records_for_cache_expiry] = Set.new - - result = yield - - Thread.current[:idc_deferred_parent_expiration] = nil - Thread.current[:idc_parent_records_for_cache_expiry].each(&:expire_primary_index) - - result - ensure - Thread.current[:idc_deferred_parent_expiration] = nil - Thread.current[:idc_parent_records_for_cache_expiry].clear - end - - def with_deferred_attribute_expiration - raise NestedDeferredAttributeExpirationBlockError if Thread.current[:identity_cache_deferred_attribute_expiration] + def with_deferred_expiration + raise NestedDeferredCacheExpirationBlockError if Thread.current[:idc_deferred_expiration] - Thread.current[:idc_deferred_attribute_expiration] = true - Thread.current[:idc_records_to_expire] = Set.new + Thread.current[:idc_deferred_expiration] = true + Thread.current[:idc_parent_records_to_expire] = Set.new + Thread.current[:idc_child_records_to_expire] = Set.new Thread.current[:idc_attributes_to_expire] = Set.new result = yield - Thread.current[:idc_deferred_attribute_expiration] = nil - IdentityCache.cache.delete_multi(Thread.current[:idc_records_to_expire]) - IdentityCache.cache.delete_multi(Thread.current[:idc_attributes_to_expire]) + Thread.current[:idc_deferred_expiration] = nil + IdentityCache.cache.delete_multi(Thread.current[:idc_parent_records_to_expire]) if Thread.current[:idc_parent_records_to_expire].any? + IdentityCache.cache.delete_multi(Thread.current[:idc_child_records_to_expire]) if Thread.current[:idc_child_records_to_expire].any? + IdentityCache.cache.delete_multi(Thread.current[:idc_attributes_to_expire]) if Thread.current[:idc_attributes_to_expire].any? result ensure - Thread.current[:idc_deferred_attribute_expiration] = nil + Thread.current[:idc_deferred_expiration] = nil + Thread.current[:idc_parent_records_to_expire].clear + Thread.current[:idc_child_records_to_expire].clear Thread.current[:idc_attributes_to_expire].clear end diff --git a/lib/identity_cache/cached/attribute.rb b/lib/identity_cache/cached/attribute.rb index 9ffa0aa0..bbaec914 100644 --- a/lib/identity_cache/cached/attribute.rb +++ b/lib/identity_cache/cached/attribute.rb @@ -40,7 +40,7 @@ def expire(record) unless record.send(:was_new_record?) old_key = old_cache_key(record) - if Thread.current[:idc_deferred_attribute_expiration] + if Thread.current[:idc_deferred_expiration] Thread.current[:idc_attributes_to_expire] << old_key # defer the deletion, and don't block the following deletion all_deleted = true @@ -51,7 +51,7 @@ def expire(record) unless record.destroyed? new_key = new_cache_key(record) if new_key != old_key - if Thread.current[:idc_deferred_attribute_expiration] + if Thread.current[:idc_deferred_expiration] Thread.current[:idc_attributes_to_expire] << new_key all_deleted = true else diff --git a/lib/identity_cache/cached/primary_index.rb b/lib/identity_cache/cached/primary_index.rb index 538c95e0..3abd04a5 100644 --- a/lib/identity_cache/cached/primary_index.rb +++ b/lib/identity_cache/cached/primary_index.rb @@ -45,8 +45,8 @@ def fetch_multi(ids) def expire(id) id = cast_id(id) - if Thread.current[:idc_deferred_attribute_expiration] - Thread.current[:idc_records_to_expire] << cache_key(id) + if Thread.current[:idc_deferred_expiration] + Thread.current[:idc_child_records_to_expire] << cache_key(id) else IdentityCache.cache.delete(cache_key(id)) end diff --git a/lib/identity_cache/parent_model_expiration.rb b/lib/identity_cache/parent_model_expiration.rb index 499e6d0b..a26fff44 100644 --- a/lib/identity_cache/parent_model_expiration.rb +++ b/lib/identity_cache/parent_model_expiration.rb @@ -47,8 +47,8 @@ def expire_parent_caches add_parents_to_cache_expiry_set(parents_to_expire) parents_to_expire.select! { |parent| parent.class.primary_cache_index_enabled } parents_to_expire.reduce(true) do |all_expired, parent| - if Thread.current[:idc_deferred_parent_expiration] - Thread.current[:idc_parent_records_for_cache_expiry] << parent + if Thread.current[:idc_deferred_expiration] + Thread.current[:idc_parent_records_to_expire] << parent.cache_key next parent end parent.expire_primary_index && all_expired diff --git a/test/index_cache_test.rb b/test/index_cache_test.rb index bb3a48ea..1fbf3f17 100644 --- a/test/index_cache_test.rb +++ b/test/index_cache_test.rb @@ -166,38 +166,36 @@ def test_unique_cache_index_with_non_id_primary_key assert_equal(123, KeyedRecord.fetch_by_value("a").id) end - def test_with_deferred_parent_expiration_expires_parent_index_once + def test_with_deferred_expiration_for_parent_records_expires_parent_index_once Item.send(:cache_has_many, :associated_records, embed: true) @parent = Item.create!(title: "bob") @records = @parent.associated_records.create!([{ name: "foo" }, { name: "bar" }, { name: "baz" }]) - @memcached_spy = Spy.on(backend, :write).and_call_through - + @memcached_spy_write_multi = Spy.on(backend, :write_multi).and_call_through expected_item_expiration_count = Array(@parent).count expected_associated_record_expiration_count = @records.count expected_return_value = "Some text that we expect to see returned from the block" - result = IdentityCache.with_deferred_parent_expiration do + result = IdentityCache.with_deferred_expiration do @parent.transaction do @parent.associated_records.destroy_all end - assert_equal(expected_associated_record_expiration_count, @memcached_spy.calls.count) expected_return_value end - expired_cache_keys = @memcached_spy.calls.map(&:args).map(&:first) - item_expiration_count = expired_cache_keys.count { _1.include?("Item") } - associated_record_expiration_count = expired_cache_keys.count { _1.include?("AssociatedRecord") } + all_keys = @memcached_spy_write_multi.calls.flat_map { |call| call.args.first.keys } + item_expiration_count = all_keys.count { _1.start_with?("items/") } + associated_record_expiration_count = all_keys.count { _1.include?(":AssociatedRecord:") } - assert_operator(@memcached_spy.calls.count, :>, 0) + assert_operator(@memcached_spy_write_multi.calls.count, :>, 0) assert_equal(expected_item_expiration_count, item_expiration_count) assert_equal(expected_associated_record_expiration_count, associated_record_expiration_count) assert_equal(expected_return_value, result) end - def test_double_nested_deferred_parent_expiration_will_raise_error + def test_double_nested_deferred_expiration_for_parent_records_will_raise_error Item.send(:cache_has_many, :associated_records, embed: true) @parent = Item.create!(title: "bob") @@ -205,9 +203,9 @@ def test_double_nested_deferred_parent_expiration_will_raise_error @memcached_spy = Spy.on(backend, :write).and_call_through - assert_raises(IdentityCache::NestedDeferredParentBlockError) do - IdentityCache.with_deferred_parent_expiration do - IdentityCache.with_deferred_parent_expiration do + assert_raises(IdentityCache::NestedDeferredCacheExpirationBlockError) do + IdentityCache.with_deferred_expiration do + IdentityCache.with_deferred_expiration do @parent.transaction do @parent.associated_records.destroy_all end @@ -218,7 +216,7 @@ def test_double_nested_deferred_parent_expiration_will_raise_error assert_equal(0, @memcached_spy.calls.count) end - def test_deep_association_with_deferred_parent_expiration_expires_parent_once + def test_deep_association_with_deferred_expiration_expires_parent_once AssociatedRecord.send(:has_many, :deeply_associated_records, dependent: :destroy) Item.send(:cache_has_many, :associated_records, embed: true) @@ -232,27 +230,27 @@ def test_deep_association_with_deferred_parent_expiration_expires_parent_once ]) end - @memcached_spy = Spy.on(backend, :write).and_call_through + @memcached_spy_write_multi = Spy.on(backend, :write_multi).and_call_through expected_item_expiration_count = Array(@parent).count expected_associated_record_expiration_count = @records.count expected_deeply_associated_record_expiration_count = @records.flat_map(&:deeply_associated_records).count - IdentityCache.with_deferred_parent_expiration do + IdentityCache.with_deferred_expiration do @parent.transaction do @parent.associated_records.destroy_all end end - expired_cache_keys = @memcached_spy.calls.map(&:args).map(&:first) - item_expiration_count = expired_cache_keys.count { _1.include?("Item") } - associated_record_expiration_count = expired_cache_keys.count { _1.include?(":AssociatedRecord:") } - deeply_associated_record_expiration_count = expired_cache_keys.count { _1.include?("DeeplyAssociatedRecord") } + all_keys = @memcached_spy_write_multi.calls.flat_map { |call| call.args.first.keys } + item_expiration_count = all_keys.count { |key| key.start_with?("items/") } + associated_record_keys = all_keys.count { |key| key.include?(":AssociatedRecord:") } + deeply_associated_record_keys = all_keys.count { |key| key.include?(":DeeplyAssociatedRecord:") } - assert_operator(@memcached_spy.calls.count, :>, 0) + assert_operator(@memcached_spy_write_multi.calls.count, :>, 0) assert_equal(expected_item_expiration_count, item_expiration_count) - assert_equal(expected_associated_record_expiration_count, associated_record_expiration_count) - assert_equal(expected_deeply_associated_record_expiration_count, deeply_associated_record_expiration_count) + assert_equal(expected_associated_record_expiration_count, associated_record_keys) + assert_equal(expected_deeply_associated_record_expiration_count, deeply_associated_record_keys) end private diff --git a/test/save_test.rb b/test/save_test.rb index dc954ee7..2aa16253 100644 --- a/test/save_test.rb +++ b/test/save_test.rb @@ -111,7 +111,7 @@ def test_touch_with_batched_calls ]) expect_cache_deletes([@record1.primary_cache_index_key, @record2.primary_cache_index_key]) - IdentityCache.with_deferred_attribute_expiration do + IdentityCache.with_deferred_expiration do ActiveRecord::Base.transaction do @record1.touch @record2.touch @@ -119,14 +119,4 @@ def test_touch_with_batched_calls end end - private - - def expect_cache_delete(key) - @backend.expects(:write).with(key, IdentityCache::DELETED, anything) - end - - def expect_cache_deletes(keys) - key_values = keys.map { |key| [key, IdentityCache::DELETED] } - @backend.expects(:write_multi).with(key_values, anything) - end end diff --git a/test/test_helper.rb b/test/test_helper.rb index ed5df3b8..a37c7947 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -9,6 +9,7 @@ require "helpers/active_record_objects" require "helpers/mocked_cache_backend" require "spy/integration" +require "byebug" require File.dirname(__FILE__) + "/../lib/identity_cache" @@ -128,6 +129,15 @@ def assert_memcache_operations(num, &block) ret end + def expect_cache_delete(key) + @backend.expects(:write).with(key, IdentityCache::DELETED, anything) + end + + def expect_cache_deletes(keys) + key_values = keys.map { |key| [key, IdentityCache::DELETED] }.to_h + @backend.expects(:write_multi).with(key_values, anything) + end + def assert_no_queries(**subscribe_opts, &block) subscribe_to_sql_queries(->(sql) { raise "Unexpected SQL query: #{sql}" }, **subscribe_opts, &block) end