From c5038e591abcbb8c06ab88e7e36b6164c3a9bca8 Mon Sep 17 00:00:00 2001 From: Dylan Thacker-Smith Date: Fri, 11 Jun 2021 10:56:21 -0400 Subject: [PATCH 1/9] Rename methods to avoid confusion for new manual expiration methods --- lib/identity_cache/cached/attribute.rb | 10 +++++----- lib/identity_cache/query_api.rb | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/lib/identity_cache/cached/attribute.rb b/lib/identity_cache/cached/attribute.rb index 96e83e92..9e222b49 100644 --- a/lib/identity_cache/cached/attribute.rb +++ b/lib/identity_cache/cached/attribute.rb @@ -34,13 +34,13 @@ def fetch(db_key) end end - def expire(record) + def expire_for_save(record) unless record.send(:was_new_record?) - old_key = old_cache_key(record) + old_key = old_cache_key_for_record(record) IdentityCache.cache.delete(old_key) end unless record.destroyed? - new_key = new_cache_key(record) + new_key = new_cache_key_for_record(record) if new_key != old_key IdentityCache.cache.delete(new_key) end @@ -95,12 +95,12 @@ def cache_key_prefix end end - def new_cache_key(record) + def new_cache_key_for_record(record) new_key_values = key_fields.map { |field| record.send(field) } cache_key_from_key_values(new_key_values) end - def old_cache_key(record) + def old_cache_key_for_record(record) old_key_values = key_fields.map do |field| field_string = field.to_s changes = record.transaction_changed_attributes diff --git a/lib/identity_cache/query_api.rb b/lib/identity_cache/query_api.rb index c64f0b44..8bd3734f 100644 --- a/lib/identity_cache/query_api.rb +++ b/lib/identity_cache/query_api.rb @@ -184,7 +184,7 @@ def was_new_record? # :nodoc: def expire_attribute_indexes # :nodoc: cache_indexes.each do |cached_attribute| - cached_attribute.expire(self) + cached_attribute.expire_for_save(self) end end end From 4f7e0da0b310385bb48cc745aeaed5df55efcdc8 Mon Sep 17 00:00:00 2001 From: Dylan Thacker-Smith Date: Tue, 15 Jun 2021 17:36:51 -0400 Subject: [PATCH 2/9] Automatically install parent expiration hooks in parent_expiration_entries --- lib/identity_cache/parent_model_expiration.rb | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/lib/identity_cache/parent_model_expiration.rb b/lib/identity_cache/parent_model_expiration.rb index 25fbb956..fcd9e0e0 100644 --- a/lib/identity_cache/parent_model_expiration.rb +++ b/lib/identity_cache/parent_model_expiration.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true module IdentityCache - module ParentModelExpiration # :nodoc: + # @api private + module ParentModelExpiration extend ActiveSupport::Concern include ArTransactionChanges @@ -35,9 +36,16 @@ def lazy_hooks end end + module ClassMethods + def parent_expiration_entries + ParentModelExpiration.install_pending_parent_expiry_hooks(cached_model) + _parent_expiration_entries + end + end + included do - class_attribute(:parent_expiration_entries) - self.parent_expiration_entries = Hash.new { |hash, key| hash[key] = [] } + class_attribute(:_parent_expiration_entries) + self._parent_expiration_entries = Hash.new { |hash, key| hash[key] = [] } end def expire_parent_caches @@ -49,7 +57,6 @@ def expire_parent_caches end def add_parents_to_cache_expiry_set(parents_to_expire) - ParentModelExpiration.install_pending_parent_expiry_hooks(cached_model) self.class.parent_expiration_entries.each do |association_name, cached_associations| parents_to_expire_on_changes(parents_to_expire, association_name, cached_associations) end From 312f7c646b07a36fe56dc90025a38837083994bd Mon Sep 17 00:00:00 2001 From: Dylan Thacker-Smith Date: Fri, 11 Jun 2021 11:07:44 -0400 Subject: [PATCH 3/9] [Untested] Add expire_cache_for_{insert,update,delete} methods --- lib/identity_cache/cached/attribute.rb | 14 ++++++ lib/identity_cache/configuration_dsl.rb | 1 + lib/identity_cache/parent_model_expiration.rb | 11 +++++ lib/identity_cache/without_primary_index.rb | 49 +++++++++++++++++++ 4 files changed, 75 insertions(+) diff --git a/lib/identity_cache/cached/attribute.rb b/lib/identity_cache/cached/attribute.rb index 9e222b49..3e72b49c 100644 --- a/lib/identity_cache/cached/attribute.rb +++ b/lib/identity_cache/cached/attribute.rb @@ -47,6 +47,20 @@ def expire_for_save(record) end end + def expire_for_values(values_hash) + key_values = key_fields.map { |name| values_hash.fetch(name) } + IdentityCache.cache.delete(cache_key_from_key_values(key_values)) + end + + def expire_for_update(old_values_hash, changes) + expire_for_values(old_values_hash) + + if key_fields.any? { |name| changes.key?(name) } + key_values = key_fields.map { |name| changes.fetch(name) { old_values_hash.fetch(name) } } + IdentityCache.cache.delete(cache_key_from_key_values(key_values)) + end + end + def cache_key(index_key) values_hash = IdentityCache.memcache_hash(unhashed_values_cache_key_string(index_key)) "#{model.rails_cache_key_namespace}#{cache_key_prefix}#{values_hash}" diff --git a/lib/identity_cache/configuration_dsl.rb b/lib/identity_cache/configuration_dsl.rb index bb5e65fc..f7450800 100644 --- a/lib/identity_cache/configuration_dsl.rb +++ b/lib/identity_cache/configuration_dsl.rb @@ -129,6 +129,7 @@ def cache_attribute_by_alias(attribute_or_proc, alias_name:, by:, unique:) cached_attribute = klass.new(self, attribute_or_proc, alias_name, fields, unique) cached_attribute.build cache_indexes.push(cached_attribute) + @cache_indexed_columns = nil end def ensure_base_model diff --git a/lib/identity_cache/parent_model_expiration.rb b/lib/identity_cache/parent_model_expiration.rb index fcd9e0e0..a8e290ee 100644 --- a/lib/identity_cache/parent_model_expiration.rb +++ b/lib/identity_cache/parent_model_expiration.rb @@ -41,6 +41,17 @@ def parent_expiration_entries ParentModelExpiration.install_pending_parent_expiry_hooks(cached_model) _parent_expiration_entries end + + def check_for_unsupported_parent_expiration_entries + return unless parent_expiration_entries.any? + msg = +"Unsupported manual expiration of #{name} record that is embedded in parent associations:\n" + parent_expiration_entries.each do |association_name, cached_associations| + cached_associations.each do |parent_class, _only_on_foreign_key_change| + msg << "- #{association_name}" + end + end + raise msg + end end included do diff --git a/lib/identity_cache/without_primary_index.rb b/lib/identity_cache/without_primary_index.rb index 5b4ed22c..3c0af040 100644 --- a/lib/identity_cache/without_primary_index.rb +++ b/lib/identity_cache/without_primary_index.rb @@ -26,6 +26,55 @@ module ClassMethods def primary_cache_index_enabled false end + + # Get only the columns whose values are needed to manually expire caches + # after updating or deleting rows without triggering after_commit callbacks. + # + # 1. Pass the returned columns into Active Record's `select` or `pluck` query + # method on the scope that will be used to modify the database in order to + # query original for these rows that will be modified. + # 2. Update or delete the rows + # 3. Use {expire_cache_for_update} or {expire_cache_for_delete} to expires the + # caches, passing in the values from the query in step 1 as the indexed_values. + # + # @return [Array] the array of column names + def cache_indexed_columns + @cache_indexed_columns ||= begin + check_for_unsupported_parent_expiration_entries + columns = Set.new + columns << primary_key.to_sym if primary_cache_index_enabled + cache_indexes.each do |cached_attribute| + columns.merge(cached_attribute.key_fields) + end + columns.to_a.freeze + end + end + + def expire_cache_for_update(old_indexed_values, changes) + if primary_cache_index_enabled + id = old_indexed_values.fetch(primary_key.to_sym) + expire_primary_key_cache_index(id) + end + cache_indexes.each do |cached_attribute| + cached_attribute.expire_for_update(old_indexed_values, changes) + end + check_for_unsupported_parent_expiration_entries + end + + private def expire_cache_for_insert_or_delete(indexed_values) + if primary_cache_index_enabled + id = indexed_values.fetch(primary_key.to_sym) + expire_primary_key_cache_index(id) + end + cache_indexes.each do |cached_attribute| + cached_attribute.expire_for_values(indexed_values) + end + check_for_unsupported_parent_expiration_entries + end + + alias_method :expire_cache_for_insert, :expire_cache_for_insert_or_delete + + alias_method :expire_cache_for_delete, :expire_cache_for_insert_or_delete end end end From 27dac82d25a91a6fa45b50d00664f11cd925da1f Mon Sep 17 00:00:00 2001 From: Dylan Thacker-Smith Date: Tue, 15 Jun 2021 17:34:16 -0400 Subject: [PATCH 4/9] Start adding tests --- test/parent_model_expiration_test.rb | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/test/parent_model_expiration_test.rb b/test/parent_model_expiration_test.rb index 97cec203..9863fc54 100644 --- a/test/parent_model_expiration_test.rb +++ b/test/parent_model_expiration_test.rb @@ -32,4 +32,17 @@ def test_recursively_expire_parent_caches fetched_name = Item.fetch(item.id).fetch_associated_records.first.fetch_deeply_associated_records.first.name assert_equal("updated child", fetched_name) end + + def test_check_for_unsupported_parent_expiration_entries + Item.cache_has_many(:associated_records, embed: true) + + Item.check_for_unsupported_parent_expiration_entries + exc = assert_raises do + AssociatedRecord.check_for_unsupported_parent_expiration_entries + end + assert_equal( + "Unsupported manual expiration of AssociatedRecord record that is embedded in parent associations:\n- item", + exc.message + ) + end end From d96fc9d709be5eefdea0255567e82701b3660ccf Mon Sep 17 00:00:00 2001 From: Dylan Thacker-Smith Date: Tue, 15 Jun 2021 17:41:35 -0400 Subject: [PATCH 5/9] fixup! [Untested] Add expire_cache_for_{insert,update,delete} methods --- lib/identity_cache/parent_model_expiration.rb | 11 ----------- lib/identity_cache/without_primary_index.rb | 13 +++++++++++++ 2 files changed, 13 insertions(+), 11 deletions(-) diff --git a/lib/identity_cache/parent_model_expiration.rb b/lib/identity_cache/parent_model_expiration.rb index a8e290ee..fcd9e0e0 100644 --- a/lib/identity_cache/parent_model_expiration.rb +++ b/lib/identity_cache/parent_model_expiration.rb @@ -41,17 +41,6 @@ def parent_expiration_entries ParentModelExpiration.install_pending_parent_expiry_hooks(cached_model) _parent_expiration_entries end - - def check_for_unsupported_parent_expiration_entries - return unless parent_expiration_entries.any? - msg = +"Unsupported manual expiration of #{name} record that is embedded in parent associations:\n" - parent_expiration_entries.each do |association_name, cached_associations| - cached_associations.each do |parent_class, _only_on_foreign_key_change| - msg << "- #{association_name}" - end - end - raise msg - end end included do diff --git a/lib/identity_cache/without_primary_index.rb b/lib/identity_cache/without_primary_index.rb index 3c0af040..05702dc0 100644 --- a/lib/identity_cache/without_primary_index.rb +++ b/lib/identity_cache/without_primary_index.rb @@ -75,6 +75,19 @@ def expire_cache_for_update(old_indexed_values, changes) alias_method :expire_cache_for_insert, :expire_cache_for_insert_or_delete alias_method :expire_cache_for_delete, :expire_cache_for_insert_or_delete + + private + + def check_for_unsupported_parent_expiration_entries + return unless parent_expiration_entries.any? + msg = +"Unsupported manual expiration of #{name} record that is embedded in parent associations:\n" + parent_expiration_entries.each do |association_name, cached_associations| + cached_associations.each do |parent_class, _only_on_foreign_key_change| + msg << "- #{association_name}" + end + end + raise msg + end end end end From 5465c74fc44805bfd22c2f7c7f1ca7b92cfd0e6c Mon Sep 17 00:00:00 2001 From: Dylan Thacker-Smith Date: Mon, 21 Jun 2021 16:37:56 -0400 Subject: [PATCH 6/9] fixup! Start adding tests --- test/parent_model_expiration_test.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/parent_model_expiration_test.rb b/test/parent_model_expiration_test.rb index 9863fc54..0f0a722e 100644 --- a/test/parent_model_expiration_test.rb +++ b/test/parent_model_expiration_test.rb @@ -36,9 +36,9 @@ def test_recursively_expire_parent_caches def test_check_for_unsupported_parent_expiration_entries Item.cache_has_many(:associated_records, embed: true) - Item.check_for_unsupported_parent_expiration_entries + Item.send(:check_for_unsupported_parent_expiration_entries) exc = assert_raises do - AssociatedRecord.check_for_unsupported_parent_expiration_entries + AssociatedRecord.send(:check_for_unsupported_parent_expiration_entries) end assert_equal( "Unsupported manual expiration of AssociatedRecord record that is embedded in parent associations:\n- item", From 136c43a4ad4e4fee0b56da5dc71e7392c2d927f8 Mon Sep 17 00:00:00 2001 From: Hector Mendoza Jacobo Date: Mon, 28 Jun 2021 15:51:32 -0400 Subject: [PATCH 7/9] Add testing for update, delete and insert cache expirations --- lib/identity_cache/without_primary_index.rb | 6 +- test/cache_manual_expire_test.rb | 89 +++++++++++++++++++++ 2 files changed, 93 insertions(+), 2 deletions(-) create mode 100644 test/cache_manual_expire_test.rb diff --git a/lib/identity_cache/without_primary_index.rb b/lib/identity_cache/without_primary_index.rb index 05702dc0..d01ba42e 100644 --- a/lib/identity_cache/without_primary_index.rb +++ b/lib/identity_cache/without_primary_index.rb @@ -61,7 +61,7 @@ def expire_cache_for_update(old_indexed_values, changes) check_for_unsupported_parent_expiration_entries end - private def expire_cache_for_insert_or_delete(indexed_values) + def expire_cache_for_insert_or_delete(indexed_values) if primary_cache_index_enabled id = indexed_values.fetch(primary_key.to_sym) expire_primary_key_cache_index(id) @@ -76,13 +76,15 @@ def expire_cache_for_update(old_indexed_values, changes) alias_method :expire_cache_for_delete, :expire_cache_for_insert_or_delete + private :expire_cache_for_insert_or_delete + private def check_for_unsupported_parent_expiration_entries return unless parent_expiration_entries.any? msg = +"Unsupported manual expiration of #{name} record that is embedded in parent associations:\n" parent_expiration_entries.each do |association_name, cached_associations| - cached_associations.each do |parent_class, _only_on_foreign_key_change| + cached_associations.each do |_parent_class, _only_on_foreign_key_change| msg << "- #{association_name}" end end diff --git a/test/cache_manual_expire_test.rb b/test/cache_manual_expire_test.rb new file mode 100644 index 00000000..87855b18 --- /dev/null +++ b/test/cache_manual_expire_test.rb @@ -0,0 +1,89 @@ +# frozen_string_literal: true +require "test_helper" + +module IdentityCache + class CacheManualExpireTest < IdentityCache::TestCase + def setup + super + AssociatedRecord.cache_attribute(:name) + + @parent = Item.create!(title: "bob") + @record = @parent.associated_records.create!(name: "foo") + IdentityCache.cache.clear + end + + def test_cache_indexed_columns_returns_the_correct_columns_for_expiration + AssociatedRecord.cache_attribute(:name, by: :item_id) + expected_result = [:id, :item_id] + assert_equal(expected_result, AssociatedRecord.cache_indexed_columns) + end + + def test_expire_cache_for_udate + id = 1 + item_id = 1 + AssociatedRecord.cache_attribute(:item_id, by: :name) + + assert_queries(1) do + assert_equal(item_id, AssociatedRecord.fetch_item_id_by_name("foo")) + end + + AssociatedRecord.where(id: 1).update_all(name: "bar") + old_values = { + name: "foo", + id: id, + } + new_values = { + name: "bar", + id: id, + } + + AssociatedRecord.expire_cache_for_update(old_values, new_values) + assert_queries(2) do + assert_equal(item_id, AssociatedRecord.fetch_item_id_by_name("bar")) + assert_nil(AssociatedRecord.fetch_item_id_by_name("foo")) + end + end + + def test_expire_cache_for_udate_raises_when_a_hash_is_missing_an_index_key + expected_error_message = "key not found: :id" + old_values = { + name: "foo", + } + new_values = { + name: "bar", + } + + error = assert_raises do + AssociatedRecord.expire_cache_for_update(old_values, new_values) + end + + assert_equal(expected_error_message, error.message) + end + + def test_expire_cache_for_insert + id = 1 + AssociatedRecord.fetch_name_by_id(id) + expire_hash_keys = { + id: id, + } + + AssociatedRecord.expire_cache_for_insert(expire_hash_keys) + assert_queries(1) do + assert_equal("foo", AssociatedRecord.fetch_name_by_id(id)) + end + end + + def test_expire_cache_for_delete + id = 1 + AssociatedRecord.fetch_name_by_id(1) + expire_hash_keys = { + id: id, + } + + AssociatedRecord.expire_cache_for_delete(expire_hash_keys) + assert_queries(1) do + assert_equal("foo", AssociatedRecord.fetch_name_by_id(id)) + end + end + end +end From dfe099223ec6e67a0f06f22b1a98c9fec0724016 Mon Sep 17 00:00:00 2001 From: Hector Mendoza Jacobo Date: Mon, 12 Jul 2021 16:03:56 -0400 Subject: [PATCH 8/9] Update tests --- ..._test.rb => without_primary_index_test.rb} | 34 +++++++++++-------- 1 file changed, 19 insertions(+), 15 deletions(-) rename test/{cache_manual_expire_test.rb => without_primary_index_test.rb} (67%) diff --git a/test/cache_manual_expire_test.rb b/test/without_primary_index_test.rb similarity index 67% rename from test/cache_manual_expire_test.rb rename to test/without_primary_index_test.rb index 87855b18..4989ea16 100644 --- a/test/cache_manual_expire_test.rb +++ b/test/without_primary_index_test.rb @@ -1,15 +1,15 @@ # frozen_string_literal: true require "test_helper" +require "byebug" module IdentityCache - class CacheManualExpireTest < IdentityCache::TestCase + class WithoutPrimaryIndexTest < IdentityCache::TestCase def setup super AssociatedRecord.cache_attribute(:name) @parent = Item.create!(title: "bob") @record = @parent.associated_records.create!(name: "foo") - IdentityCache.cache.clear end def test_cache_indexed_columns_returns_the_correct_columns_for_expiration @@ -18,7 +18,7 @@ def test_cache_indexed_columns_returns_the_correct_columns_for_expiration assert_equal(expected_result, AssociatedRecord.cache_indexed_columns) end - def test_expire_cache_for_udate + def test_expire_cache_for_update id = 1 item_id = 1 AssociatedRecord.cache_attribute(:item_id, by: :name) @@ -44,7 +44,7 @@ def test_expire_cache_for_udate end end - def test_expire_cache_for_udate_raises_when_a_hash_is_missing_an_index_key + def test_expire_cache_for_update_raises_when_a_hash_is_missing_an_index_key expected_error_message = "key not found: :id" old_values = { name: "foo", @@ -53,7 +53,7 @@ def test_expire_cache_for_udate_raises_when_a_hash_is_missing_an_index_key name: "bar", } - error = assert_raises do + error = assert_raises(KeyError) do AssociatedRecord.expire_cache_for_update(old_values, new_values) end @@ -61,29 +61,33 @@ def test_expire_cache_for_udate_raises_when_a_hash_is_missing_an_index_key end def test_expire_cache_for_insert - id = 1 - AssociatedRecord.fetch_name_by_id(id) + test_record_name = "Test Record" + AssociatedRecord.insert_all([{name: test_record_name}]) + test_record = AssociatedRecord.find_by(name: test_record_name) expire_hash_keys = { - id: id, + id: test_record.id, } - + + assert_equal(test_record_name, AssociatedRecord.fetch_name_by_id(test_record.id)) AssociatedRecord.expire_cache_for_insert(expire_hash_keys) assert_queries(1) do - assert_equal("foo", AssociatedRecord.fetch_name_by_id(id)) + assert_equal(test_record_name, AssociatedRecord.fetch_name_by_id(test_record.id)) end end def test_expire_cache_for_delete - id = 1 - AssociatedRecord.fetch_name_by_id(1) + assert_equal("foo", AssociatedRecord.fetch_name_by_id(@record.id)) expire_hash_keys = { - id: id, + id: @record.id, } + AssociatedRecord.delete(@record.id) + assert_equal("foo", AssociatedRecord.fetch_name_by_id(@record.id)) + AssociatedRecord.expire_cache_for_delete(expire_hash_keys) assert_queries(1) do - assert_equal("foo", AssociatedRecord.fetch_name_by_id(id)) + assert_nil(AssociatedRecord.fetch_name_by_id(@record.id)) end end end -end +end \ No newline at end of file From f78fed90160c3c659e46107f1d59964185ca54fa Mon Sep 17 00:00:00 2001 From: Hector Mendoza Jacobo Date: Thu, 15 Jul 2021 18:15:34 -0400 Subject: [PATCH 9/9] Remove unused import --- test/without_primary_index_test.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/test/without_primary_index_test.rb b/test/without_primary_index_test.rb index 4989ea16..02c4e68f 100644 --- a/test/without_primary_index_test.rb +++ b/test/without_primary_index_test.rb @@ -1,6 +1,5 @@ # frozen_string_literal: true require "test_helper" -require "byebug" module IdentityCache class WithoutPrimaryIndexTest < IdentityCache::TestCase