Repository navigation
Cache busting doesn't bubble up in some cases without eagerloading #389
Description
Given the following classes:
class A < AR::Base
has_many :bs, inverse: :a
cache_has_many(:bs, embed: true)
end
class B < AR::Base
belongs_to :a, inverse: :bs
has_one :c
cache_has_one(:c, embed: true)
end
class C < AR::Base
has_many :b, inverse: :c
endwhen saving an instance of C, B gets removed from the cache, but not A. Thus, B#fetch_c returns the latest version but A#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?
Activity
dylanahsmith commentedon Sep 9, 2019
Your example in the ancestor-invalidation-failing branch uses
Item.cache_has_many(:associated_records)but the default isembed: :idsforcache_has_many. Of course, this is confusing given that the default forcache_has_oneisembed: truejbourassa commentedon Sep 10, 2019
You're absolutely right, turns out that wasn't the issue, it was around some hooks not being installed (ie
B.parent_expiration_entriesnot being populated). Looks like I can't get the behaviour to work as expected without explicitly loading witheager_load!, but at least that works.Thanks for the quick feedback!
dylanahsmith commentedon Sep 10, 2019
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
jbourassa commentedon Sep 10, 2019
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 :/
[-]Cache busting doesn't bubble up in some cases[/-][+]Cache busting doesn't bubble up in some cases without eagerloading[/+]jbourassa commentedon Jan 6, 2020
I took some time to put together what I think is a real failing test:
Commit: 28ac7a1
CI: https://travis-ci.org/Shopify/identity_cache/builds/633337374?utm_source=github_status&utm_medium=notification
jbourassa commentedon Jan 6, 2020
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.
dylanahsmith commentedon Jan 7, 2020
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_cachesand 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.