Add support for manual expiration for after direct DB manipulation #500
Conversation
|
|
||
| def check_for_unsupported_parent_expiration_entries | ||
| return unless parent_expiration_entries.any? | ||
| msg = "Unsupported manual expiration of record embedded in parent associations:\n" |
There was a problem hiding this comment.
@Hectorhammett will this be needed for your use case?
There was a problem hiding this comment.
This should not cause any problems with our use.
dylanahsmith
changed the base branch from
master
to
skip-rubocop-version-with-bug
dylanahsmith
force-pushed
the
manual-expire
branch
from
c7b1418
to
1e59ad2
Compare
| end | ||
|
|
||
| def expire_for_update(old_values_hash, changes) | ||
| expire_for_values(old_values_hash, changes) |
There was a problem hiding this comment.
expire_for_values expects only one parameter, isn't it?
There was a problem hiding this comment.
Oh yeah, this should be expire_for_values(old_values_hash)
dylanahsmith
force-pushed
the
manual-expire
branch
from
1e59ad2
to
d96fc9d
Compare
Hectorhammett
force-pushed
the
manual-expire
branch
from
7a1265a
to
136c43a
Compare
test/cache_manual_expire_test.rb
Outdated
|
|
||
| def test_expire_cache_for_insert | ||
| id = 1 | ||
| AssociatedRecord.fetch_name_by_id(id) |
There was a problem hiding this comment.
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
test/cache_manual_expire_test.rb
Outdated
| id: id, | ||
| } | ||
|
|
||
| AssociatedRecord.expire_cache_for_delete(expire_hash_keys) |
There was a problem hiding this comment.
There isn't a corresponding deletion in this test, so this isn't really testing deletion.
test/cache_manual_expire_test.rb
Outdated
| require "test_helper" | ||
|
|
||
| module IdentityCache | ||
| class CacheManualExpireTest < IdentityCache::TestCase |
There was a problem hiding this comment.
The test files should be organized around the code it tests as explained in #498
test/cache_manual_expire_test.rb
Outdated
| assert_equal(expected_result, AssociatedRecord.cache_indexed_columns) | ||
| end | ||
|
|
||
| def test_expire_cache_for_udate |
There was a problem hiding this comment.
| def test_expire_cache_for_udate | |
| def test_expire_cache_for_update |
test/cache_manual_expire_test.rb
Outdated
| end | ||
| end | ||
|
|
||
| def test_expire_cache_for_udate_raises_when_a_hash_is_missing_an_index_key |
There was a problem hiding this comment.
| 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 |
test/cache_manual_expire_test.rb
Outdated
| name: "bar", | ||
| } | ||
|
|
||
| error = assert_raises do |
There was a problem hiding this comment.
| error = assert_raises do | |
| error = assert_raises(KeyError) do |
cc @Larochelle & @Hectorhammett
I've started an alternative to #495 (and the existing
expire_primary_key_cache_indexclass method) since I think we need a way to expire caches that doesn't make assumptions about what caches need to be expired. Otherwise, it is easy to miss caches that the developer using this manual expiration didn't think about (e.g. from embedded associations) or ones added in the future.This PR adds
cache_indexed_columnsclass method that can be used to get the columns that are needed for cache expiration (see its documentation for details). After that data is queried and put into the form of a hash, it can then be used withexpire_cache_for_deleteorexpire_cache_for_updateclass methods. I've also addedexpire_cache_for_insert, which could just be used with the data to insert.Note that I haven't yet tested or fully documented this code (e.g. we could use a brief mention of this in the REAMDE and link to a new docs/manual_expire.md file with details).