Fix Action View collection caching to store fragments as bare strings #48645
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
casperisfine
commented
Jul 4, 2023
| if content = cached_partials[cache_key] | ||
| build_rendered_template(content, template) | ||
| else | ||
| yield.tap do |rendered_partial| | ||
| collection_cache.write(cache_key, rendered_partial.body) |
There was a problem hiding this comment.
While I was at it, I converted this write loop into a single write_multi.
matthewd
reviewed
Jul 4, 2023
| @@ -9,7 +9,7 @@ module CollectionCaching # :nodoc: | |||
| included do | |||
| # Fallback cache store if Action View is used without Rails. | |||
| # Otherwise overridden in Railtie to use Rails.cache. | |||
| mattr_accessor :collection_cache, default: ActiveSupport::Cache::MemoryStore.new | |||
| mattr_accessor :collection_cache, default: ActiveSupport::Cache::NullStore.new | |||
There was a problem hiding this comment.
This definitely doesn't sound backportable
There was a problem hiding this comment.
It's not strictly necessary, just seemed weird to me. I'll revert it.
matthewd
reviewed
Jul 4, 2023
| collection_cache.write(cache_key, rendered_partial.body) | ||
| end | ||
| rendered_partial = yield | ||
| entries_to_write[cache_key] = rendered_partial.body.to_str |
There was a problem hiding this comment.
Is body absolutely definitely 150% guaranteed to be an OutputBuffer (or String) here, even given a particularly pathological template renderer etc?
(This is both "can we assume there's a to_str?", and "are we allowed to force the value to a string?")
Sorry this is a lazy "I haven't read the context" question, but I imagine the answer is likely "yes I have already read and checked that", in which case it's easier for me to just ask. 😅
There was a problem hiding this comment.
Nah, it was a good question. It appears that EmptyCollection#body returns nil for instance.
However I do think we can enforce nil or respond_to?(:to_str).
casperisfine
force-pushed
the
action-view-bare-cache-fragments
branch
from
9015335 to
5e916e7
Compare
Ref: rails#48611 Individual fragments have been cached as bare string since forever, but somehow fragment cached via collection caching were stored as `ActionView::OutputBuffer` instances. This is both bad for performance, but also can cause issues on Rails upgrades if the internal representation of `AV::OutputBuffer` changes.
casperisfine
force-pushed
the
action-view-bare-cache-fragments
branch
from
5e916e7 to
6842e76
Compare
casperisfine
pushed a commit
to Shopify/rails
that referenced
this pull request
Jul 5, 2023
…agments Fix Action View collection caching to store fragments as bare strings
|
Backported as 64bd0ac |
|
@casperisfine Hi, it broke our application when caching is used with Jbuilder gem https://github.com/rails/jbuilder. We are getting: |
|
@edariedl, could you share a longer backtrace please?
|
|
@byroot yeah sure, here is the full stack trace: Thank you for looking at it 🙏. Is there an alternative to |
casperisfine
pushed a commit
to Shopify/rails
that referenced
this pull request
Aug 14, 2023
Followup: rails#48645 Some template engines such as `jbuilder` use these Action View primitives with types other than strings, which breaks a bunch of assumptions. I wish I could add a test for this, but this is deep in private methods I don't see a way to cover this.
casperisfine
pushed a commit
to Shopify/rails
that referenced
this pull request
Aug 14, 2023
Followup: rails#48645 Some template engines such as `jbuilder` use these Action View primitives with types other than strings, which breaks a bunch of assumptions. I wish I could add a test for this, but this is deep in private methods I don't see a way to cover this.
|
I think the switch to write_multi also breaks Rails cache when using Redis::Distributed - eg with config.cache_store = :redis_cache_store, {
url: [url1, url2],
...
}
}I'm seeing Want me to open it as a separate issue? |
Yes. IMO this is a limitation of It should either be documented as a limitation, or fixed in |
This was referenced Aug 14, 2023
paulreece
pushed a commit
to paulreece/rails-paulreece
that referenced
this pull request
Aug 26, 2023
Followup: rails#48645 Some template engines such as `jbuilder` use these Action View primitives with types other than strings, which breaks a bunch of assumptions. I wish I could add a test for this, but this is deep in private methods I don't see a way to cover this.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Ref: #48611
Individual fragments have been cached as bare string since forever, but somehow fragment cached via collection caching were stored as
ActionView::OutputBufferinstances.This is both bad for performance, but also can cause issues on Rails upgrades if the internal representation of
AV::OutputBufferchanges.I think we should consider this PR as a bug fix, and backport it accordingly.
cc @matthewd, @matthutchinson, @skipkayhil