Rails 5.2 #37098
Merged
Rails 5.2 #37098
Conversation
This file contains 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
Hamms
added
the
Rails Upgrade
Hamms
force-pushed
the
rails-5.2
branch
2 times, most recently
from
5e49cd3
to
1a9a3b8
Compare
Part of the ongoing work to upgrade us to Rails 6.
This was updated in rails/rails#28800
To reflect the new rails functionality. Specifically, in rails/rails#30367 the cache control headers were normalized more aggressively, which results in the headers we examine to determine whether or not caching is enabled to have slightly different content. I'm not 100% clear on exactly which headers here are or are not important to us, but according to [the relevant documentation](https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Cache-Control) the removed headers seem unnecessary: > `**no-store**` > > The response may **not** be stored in _any_ cache. Although other directives may be set, this alone is _the only directive you need in preventing cached responses_ on modern browsers. `max-age=0` **is already implied. Setting** `must-revalidate` **does not make sense** because in order to go through revalidation you need the response to be stored in a cache, which `no-store` prevents.
Update the logic which detects if caching is disabled
Follow-up to #37741 It looks like the Rails caching logic doesn't always include the `private` header. This makes sense; again referenving [the relevant documentation](https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Cache-Control): > **`private`** > > The response may be stored only by a _browser's_ cache, even if the response is normally non-cacheable. **If you mean to not store the response in any cache, use `no-store` instead.** _This directive is not effective in preventing caches from storing your response._
The method we're patching to override this behavior was updated a couple of times since our previous version of rack; first in rack/rack#1435, then presumably a couple more times because the relevant logic ends up looking like this in 2.2.3: https://github.com/rack/rack/blob/1741c580d71cfca8e541e96cc372305c8892ee74/lib/rack/request.rb#L222-L229 We therefore in this PR update the logic of our patch to better match the current internal structure of Rack.
wjordan
approved these changes
Nov 13, 2020
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.
Update us to Rails 5.2, the very latest 5.x version.
Like the 5.1 upgrade, this PR is kept pretty minimal; most of the required work has been done in other PRs. Some of that work includes:
The changes included in this pull request are those that cannot be applied in advance, and the individual commits for each change should explain in some detail why they are necessary.
Next steps
Next up is 6.0! I plan to start by working through the official "Upgrading from Rails 5.2 to Rails 6.0" guide, as well as dealing with the deprecation warnings we can already see in this PR. First among those is the change to start enforcing safer SQL queries in ActiveRecord, work which has already been started in #37654
Testing story
I've done quite a bit of manual testing over the course of this upgrade, but I am mostly relying on our existing tests
Reviewer Checklist: