Open jbourassa opened 5 years ago
Your example in the ancestor-invalidation-failing branch uses Item.cache_has_many(:associated_records)
but the default is embed: :ids
for cache_has_many
. Of course, this is confusing given that the default for cache_has_one
is embed: true
You're absolutely right, turns out that wasn't the issue, it was around some hooks not being installed (ie B.parent_expiration_entries
not being populated). Looks like I can't get the behaviour to work as expected without explicitly loading with eager_load!
, but at least that works.
Thanks for the quick feedback!
Oh, that sounds like it could be an issue with the lazy expiration hook installation. Maybe that isn't working properly for deeply embedded associations
Yeah, I ended up debugging quite a bit more and that's what it is. The fix shouldn't be too bad but I need to figure out how to test and not impact performance – not something I can realistically dedicate to in the next couple days :/
I took some time to put together what I think is a real failing test: Commit: https://github.com/Shopify/identity_cache/commit/28ac7a128115c44b764d61de3d6b04dc00e2d6ad CI: https://travis-ci.org/Shopify/identity_cache/builds/633337374?utm_source=github_status&utm_medium=notification
Been thinking about it some more and the only solution I can come up with is to recursively load all associated models so their hooks are loaded, which means loading everything. This will likely have a significant development perf impact for big codebases.
And even thant is only guaranteed to work when relationships are defined on both sides.
I don't think we need to load everything, since parent expiration is only needed for has_one and has_many connections, which requires a corresponding belongs_to association. So only the belongs_to associations need to be loaded on a call to expire_parent_caches
and that would only need to be done recursively if the inverse of that belongs to association has a corresponding cached association (i.e. that embeds the model being expired).
Unfortunately, that would still end up loading more than needed, so would have a performance impact on development. If that becomes a concern, it would be possible to make it more precise by being more explicit about which models embed that model to reduce what is loaded. However, I would prefer to avoid adding that complexity until we know it is needed, since it isn't clear to me that it would be significant enough to be a problem.
Given the following classes:
when saving an instance of
C
,B
gets removed from the cache, but notA
. Thus,B#fetch_c
returns the latest version butA#fetch_bs
return a stale collection.I could reproduce this scenario as a failing test: https://github.com/Shopify/identity_cache/compare/ancestor-invalidation-failing
Am I missing something or is this a bug?