Cache busting doesn't bubble up in some cases without eagerloading

[jbourassa]

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
end

when 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?

[dylanahsmith]

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

[jbourassa]

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!

[dylanahsmith]

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]

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 :/

[jbourassa]

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]

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]

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.