Fix regression, enforce fresh ETag header after collection contents change#38119
Conversation
bd56147 to
e05cc0c
Compare
|
Note that this commit adds a few integration tests to If my test infringes on any Rails conventions, any feedback on how to better meet said conventions is appreciated! |
e05cc0c to
b0f3d42
Compare
|
Thanks @alipman88 Should you add fix for collection |
|
I submitted the PR eliminating the unnecessary query separately - #38099. |
There was a problem hiding this comment.
Action Pack tests can't depend on Active Record. If you really need to test this behavior in a controller you will need to write a test in the railtie folder.
There was a problem hiding this comment.
Thanks for the clarification. I've removed the Action Pack tests. (I'm ambivalent on whether there really needs to be a test for this. Now that cache_key and cache_version have been split into separate methods, I expect this interface will remain stable.)
There was a problem hiding this comment.
I prefer to copy this method to relation than adding a new module just for this method.
There was a problem hiding this comment.
Reasonable, rebased accordingly.
activerecord/CHANGELOG.md
Outdated
There was a problem hiding this comment.
You name needs to be in a different line
There was a problem hiding this comment.
Rebased accordingly - but the Rails guide seems to indicate that a single paragraph is appropriate:
Your name can be added directly after the last word if there are no code examples or multiple paragraphs. Otherwise, it's best to make a new paragraph.
https://edgeguides.rubyonrails.org/contributing_to_ruby_on_rails.html#updating-the-changelog
b0f3d42 to
f6c82e0
Compare
Add ActiveRecord::Relation#cache_key_with_version. This method will be used by ActionController::ConditionalGet to ensure that when collection cache versioning is enabled, requests using ConditionalGet don't return the same ETag header after a collection is modified. Prior to the introduction of collection cache versioning in 4f2ac80, all collection cache keys included a version. However, with cache versioning enabled, collection cache keys remain constant. In turn, ETag headers remain constant, rendering them ineffective. This commit takes the cache_key_with_version method used for individual Active Record objects (from aa8749e), and adds it to collections.
f6c82e0 to
58b0409
Compare
…lection_changes Fix regression, enforce fresh ETag header after collection contents change
Add
ActiveRecord::Relation#cache_key_with_version. This method will be used by ActionController::ConditionalGet to ensure that when collection cache versioning is enabled, requests using ConditionalGet don't return the same ETag header after a collection is modified.Prior to the introduction of collection cache versioning in 4f2ac80,
ActiveRecord::Relation#cache_keywas suitable for the purposes of ConditionalGet as all cache keys included a version. However, with collection cache versioning enabled, keys remain constant. In turn, ETag headers remain constant, rendering them ineffective.This commit takes the
cache_key_with_versionlogic used for individual Active Record objects (from aa8749e), abstracts it into a module, and applies it to both individual Active Record objects and collections.Fixes #38078