Ensure compatibility with Rack 2.x and 3.x#653
Ensure compatibility with Rack 2.x and 3.x#653SamSaffron merged 1 commit intoMiniProfiler:masterfrom
Conversation
Flink
force-pushed
the
headers-lower-case
branch
from
24217c7 to
02b3cb1
Compare
| name: Ruby ${{ matrix.ruby }} | ||
| name: Ruby ${{ matrix.ruby }} (Rack ~> ${{ matrix.rack }}) | ||
| services: | ||
| redis: |
There was a problem hiding this comment.
Since there was only one version of Redis tested, let’s use the official Redis image as a service.
| ruby: ["3.4", "3.3", "3.2", "3.1"] | ||
| redis: ["5.x"] | ||
| ruby: ["3.4", "3.3", "3.2"] | ||
| rack: ["2.0", "3.0"] |
There was a problem hiding this comment.
Here we’re ensuring everything is tested against Rack 2 and 3.
There was a problem hiding this comment.
I'm not sure what these versions are referring to but you should really be testing on 2.2.x and 3.2.x (and perhaps 3.1.x).
There was a problem hiding this comment.
In the Gemfile we’re using ~>, so with ~> 2.0 and ~> 3.0 it will take the latest available versions for Rack 2 and 3.
| headers.delete('ETag') | ||
| headers.delete('Date') | ||
| headers.delete(Rack::ETAG) | ||
| headers.delete('date') || headers.delete('Date') |
There was a problem hiding this comment.
There’s no Rack::DATE constant defined.
Some parts of the code are changing the headers from the Rack request, but are using capitalized headers when doing so. If the underlying object is not a bare hash but either a `Rack::Utils::HeaderHash` or a `Rack::Headers` object, then everything will work as expected. But sometimes the headers object can be a bare hash (that can be the case when Rails is serving static files) and if Rack 3 is in use, things can break. This patch ensures the headers names are compliant with both versions of Rack. (mainly by using things like `Rack::CONTENT_TYPE` instead of writing directly `Content-Type`/`content-type`)
Flink
force-pushed
the
headers-lower-case
branch
from
02b3cb1 to
4e69dd4
Compare
|
@jeremyevans / @ioquatix - any thoughts on this one, it looks like cross compatibility is somewhat tricky We have: https://github.com/rack/rack/blob/main/UPGRADE-GUIDE.md But I wonder if it should have a section for "how to make something cross compatible" |
|
Looks like most of this is response header casing. On Rack 3, you can assume lower case keys. On Rack 2, the code already was not working for all cases. To correctly delete a header by key on Rack 2, you need to iterate over the headers and delete any keys where the key is a case insensitive match to what you want to delete, or convert the headers to use case insensitive or lowercase keys, and then delete the key. Obviously, this is inefficient, and the primary reason we enforced lower case response header keys in Rack 3. |
|
thanks heaps for having a look @jeremyevans / @ioquatix |
the PR is MiniProfiler#653
the PR is MiniProfiler#653 and github.com link is broken
the PR is #653 and github.com link is broken
4 participants
Some parts of the code are changing the headers from the Rack request, but are using capitalized headers when doing so. If the underlying object is not a bare hash but either a
Rack::Utils::HeaderHashor aRack::Headersobject, then everything will work as expected. But sometimes the headers object can be a bare hash (that can be the case when Rails is serving static files) and if Rack 3 is in use, things can break.This PR ensures the headers names are compliant with both versions of Rack. (mainly by using things like
Rack::CONTENT_TYPEinstead of writing directlyContent-Type/content-type)