r? @eileencodes

(@rails-bot has picked a reviewer for you, use r? to override)

👍 for this feature. I accidentally created a similar PR. Just curious, but why define collection_cache_versioning instead of relying on a single cache_versioning config? I guess it's more configurable, but I would imagine that if you want cache versioning, then you want it across the board.

@gingerlime From what I understand - cache_versioning = true is going to become the default in Rails 6. Having these behind a feature config that is enable by default would seem to defeat the point of the feature config.

If people have overwritten collection_cache_key, they will need to change it to collection_cache_version to enable the recycling of cache keys - and this will give them a change to do so while there app is running in Rails 6.

If cache_versioning was unreleased at this point then I would agree that they should be behind the same flag.

Thanks @lsylvester. I see it as an omission from the original feature, but maybe it was intended this way? I'm not sure.

Besides that, this PR should hopefully resolve (what looks to me like) a performance regression bug that happens in some cases even if you don't switch cache_versioning on (and already with Rails 5.2).

In any case, keen to see this merged one way or another :)

@gingerlime I wasn't ware of that performance regression. That is clearly a bug, and while I don't think that we can enable full relation cache versioning without some time to update collection_cache_key methods - I think we can avoid this performance regression.

In my opinion - if an object response to cache_key but not cache_version then its cache_version should be nil even it it response to to_a.

Let me know if you want to have a go at that otherwise I will make a PR when I have time.

Thanks @lsylvester. I didn't get any response regarding this, so it's good that I'm not the only one who thinks this is a bug :)

I guess the simplest fix would be to implement cache_version (and return nil) on the AR relation. In theory, if someone relied on the relation expanding (using to_a) and then each element returning their own cache_version, then this can break this assumption though... But I somehow doubt there's any legitimate use case for this. I'll try to find some time to create another PR for this. No clue about the timelines for bug fixes and features like this one though, but hope someone can push this forward :)

EDIT: I guess this simplest fix would conflict with your PR however... So another option is to implement this check inside the normalize_version.

@rafaelfranca referencing my comment

This bug can cause quite severe performance regression from my experience, and it largely goes unnoticed, unless you actively measure performance. Hope that #34378 gets merged quickly and also backported, to fix existing versions of rails affected by this regression.

Is there any timeline for moving this forward? Happy to help if I can.

Will this make it into Rails 6 ?

Hey @lsylvester, sorry for never getting to review this! After a pretty huge refactoring in 10919bf I've just gone ahead and merged this in 1da9a7e. I opted not to add the collection_cache_version override because the collection_cache_key in integration.rb was marked as nodoc, so I don't think we need that feature. cc @kamipo

Thanks for working on this, I'm looking forward to this in 6.0! ❤️

@kaspth Any chance of this being backported into v5.2.x? I would reason that collection caching being a fully working feature in v5.1 and regressing in v5.2 makes this a worthy candidate.

It’s not broken in 5.2, collections just weren’t supported. They will be in 6.0.

@kaspth

class FooController < ApplicationController
  def bar
    @xyz = Xyz.all
  end
end

<% cache(@xyz) do %>
  <!-- -->
<% end %>

In 5.1.x the code as outlined above does cache collection and on non-initial hits only queries for cache_key.
In 5.2.x the caching also works, but causes extra to_a unwrapping of relation due mandatory #normalize_version call.

Would you like me building a sample test case to demonstrate further?

Gotcha, feel free to send a fix then, don’t think we want to back port since it’s a lot of code.

@kaspth Just to be completely sure on the meaning of "fix" & spare wasted time - are you proposing I create a backport of this PR to 5-2-stable? Because #34503 already proposed "a fix" that wasn't OK'ed by @rafaelfranca.

@kaspth Kasper, I'm using collection.cache_key in 5.2 inside Rails.cache.fetch calls, where I'm expected to manually build my cache keys for a collection.

When cache_key evolved to cache versioning for individual records, returning a stable key, a new method cache_key_with_version was provided, that returned the old volatile string with timestamp. So it was a 'breaking change', but at least we had a way to update our code to avoid breakage.

However, after this PR was merged in 1da9a7e, cache_key changed behavior and there's no cache_key_with_version to fallback on, meaning not only it's a breaking change (cache_key is a public method), but also we don't have any way to update our code to avoid breakage inside Rails.cache.fetch calls, where we have to build the key ourselves.

• Shouldn't we have collection.cache_key_with_version to keep consistency with the record instance methods? (that provide both cache_key and cache_key_with_version?
• If not, what should I use inside a Rails.cache.fetch where I'm building a key manually for a collection?

Pinging @lsylvester as well

Has anything happend with @feliperaul's comment above about relations not actually having a cache_key_with_version method? It seems strange that turning on versioning makes it so cache_key no longer includes the version but then not have the cache_key_with_version method on relations to fall back on.

It also seems like this would not "just work" with Rails.cache.fetch(association) since it looks like the code here tries cache_key_with_version which associations do not have and then drops down to cache_key which no longer includes a version if you turn on versioning.

If anyone else ends up here from a web search looking into why cache_key_with_version isn't on collections, it was added in 6.1 from this PR: #38119

If you have an Rails version >= 6.0 and < 6.1, you can put this backport in an initializer:

if Rails.version < "6.1.0"
  class ActiveRecord::Relation
    def cache_key_with_version
      if version = cache_version
        "#{cache_key}-#{version}"
      else
        cache_key
      end
    end
  end
else
  raise "ActiveRecord::Relation cache_key_with_version backport no longer required in Rails >= 6.1.0."
end