-
Notifications
You must be signed in to change notification settings - Fork 791
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix header casing compatibility with Rails 7 #790
Conversation
629e1e6
to
37f4776
Compare
Tests should be fixed by #791 |
37f4776
to
de25bf2
Compare
Wait, what? Doesn't rack >= 2.2.4 don't care about the casing of the headers? #758 (comment) |
For any library that aims to support Rack 2 and Rack 3, the casing of headers matters unless those libraries return So Rails returns So as an alternative to this PR, we could apply def bad_request_response(env)
if head_request?(env)
[ 400, Headers[{ "content-type" => "text/plain", Rack::CONTENT_LENGTH => "0" }], [] ]
else
[ 400, Headers[{ "content-type" => "text/plain", Rack::CONTENT_LENGTH => "11" }], [ "Bad Request" ] ]
end
end where I wrote some thoughts on using |
X_CASCADE = "X-Cascade" | ||
VARY = "Vary" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
X_CASCADE = "X-Cascade" | |
VARY = "Vary" | |
X_CASCADE = "X-Cascade" # :nodoc: | |
VARY = "Vary" # :nodoc: |
I don't want to maintain constants for strings just because Rack 3 is incompatible with Rack 2
To support Rack 3, response headers in `Sprockets::Server` were all downcased. However, this has led to issues with Rack 2 applications (ex. Rails 7) since they still expect mixed case (ex. `Content-Type`) headers. To ensure compatibility with both Rack 2 and Rack 3 applications, this commit makes the casing of the headers conditional on the Rack version. Rack itself provides constants to do this easily for most of the headers used (`Content-Type`, `Content-Length`, `Cache-Control`, and `ETag`) and the rest are added as constants under `Rack::Server`. As an alternative to this, the responses could instead be wrapped using `Rack::Headers` (and `Rack::Utils::HeaderHash` in Rack 2), but making the header casing conditional seems better to me because it is relatively easier to implement and there will be less churn if/when Rack 2 support is eventually removed.
de25bf2
to
5d795a7
Compare
Ref rails/rails#48874
Fixes rails/rails#47096 for Rails < 7.1
To support Rack 3, response headers in
Sprockets::Server
were all downcased. However, this has led to issues with Rack 2 applications (ex. Rails 7) since they still expect mixed case (ex.Content-Type
) headers.To ensure compatibility with both Rack 2 and Rack 3 applications, this commit makes the casing of the headers conditional on the Rack version. Rack itself provides constants to do this easily for most of the headers used (
Content-Type
,Content-Length
,Cache-Control
, andETag
) and the rest are added as constants underRack::Server
.As an alternative to this, the responses could instead be wrapped using
Rack::Headers
(andRack::Utils::HeaderHash
in Rack 2), but making the header casing conditional seems better to me because it is relatively easier to implement and there will be less churn if/when Rack 2 support is eventually removed.