Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for manual expiration for after direct DB manipulation #500

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from
6 changes: 4 additions & 2 deletions lib/identity_cache/without_primary_index.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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
Expand Down
89 changes: 89 additions & 0 deletions test/cache_manual_expire_test.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
# frozen_string_literal: true
require "test_helper"

module IdentityCache
class CacheManualExpireTest < IdentityCache::TestCase
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test files should be organized around the code it tests as explained in #498

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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def test_expire_cache_for_udate
def test_expire_cache_for_update

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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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",
}
new_values = {
name: "bar",
}

error = assert_raises do
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
error = assert_raises do
error = assert_raises(KeyError) 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)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is there no assertion for what value is returned by this? Also, why is this making an assertion for a hard code id?

Shouldn't this test be making the insertion after this using something like insert_all that skips callbacks? It looks like this is loading a record that already exists

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)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There isn't a corresponding deletion in this test, so this isn't really testing deletion.

assert_queries(1) do
assert_equal("foo", AssociatedRecord.fetch_name_by_id(id))
end
end
end
end