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 Rack::Request headers usage. #1880
base: main
Are you sure you want to change the base?
Conversation
There are some small details which I think we need to flesh out too. Specifically, I noticed that the linter does not do any validation on incoming request I suspect it might be a good idea to add this. Specifically, I've added (and should probably revert, but left it in for this specific discussion):
Generally speaking there is only one response header whihc does not allow comma separated values: Regardless of what we do here, we should update |
c3a646a
to
f4683c1
Compare
f4683c1
to
f837b42
Compare
I think this breaks backwards compatibility too much. I have submitted an alternative backwards compatible proposal that I think offers a nicer API: #1881 |
@jeremyevans what part of this PR is not compatible with existing code? Basically, all the specs pass with no changes (except one specific point mentioned above). |
The plan that this PR sets forth (removing the warnings and breaking code in Rack 3.1) is not backwards compatible. I think if the goal of the PR is to break backwards compatibility later, the PR should be considered as not backwards compatible, even if it technically retains immediate compatibility via code that warns that it will be removed in a later version. Considering you like the interface in #1881, and that approach is backwards compatible, I think we should use that approach. We can warn in the documentation against using the existing In terms of implementation in this PR, |
I essentially see this as a bug fix PR where as your proposal is a new feature. Rails is already working around this behaviour. The implementation here should be safely composable with existing Rails releases and also allow them with minimal changes to remove their patch over
With that interpretation, what about all the other deprecated functionality that is slated to be removed in 3.1? Quite a bit of of that is not backwards compatible.
I'm fine with that.
Define "correctly"...? From https://www.rfc-editor.org/rfc/rfc3875.html#section-4.1.18
Based on this interpretation from the CGI RFC, these are no longer headers: "remove any header fields... Content-Length... Content-Type...". Not sure what the ideal mechanism is here, but after the point they are extracted, they should no longer be present in the "headers" according to the RFC.
It doesn't solve the problem being addressed here. I like the general idea though. I think we should discuss it for Rack 3.1 Perfectly happy to punt this PR to a later Rack release too. It seems like something we should try to fix though. |
I do not see this as a bug fix API. The existing
We've agreed to make many backwards incompatible changes. We knew when doing so that removal was not backwards compatible. It's not like we accepted deprecation with a plan to remove and considered it a backwards compatible solution.
To be clear, you are fine with merging #1881, closing this, and deprecating the existing
My reading of this is that I think almost all rack users would assume that
The problem being addressed here is that users don't have a good way to do |
Okay, either (1) change the name to match the function or (2) fix the function to match the name. To me, it's both bug. Since (1) introduce major compatibility issues, a careful implementation of (2) seems like the only way forward. I think it's clear from the comment made 7 (!) years ago that there is confusion around what these methods are supposed to do, with the original documentation giving examples using raw HTTP header names which seems to imply the name is correct and the implementation is wrong. Since generally, CGI variables, headers and metadata keys can be distinct (with some very minor/obscure overlap) we can definitely fix these methods to be both compatible and do the right thing.
I find
Your solution does not fix the original source of the problem, it just introduce a new interface without fixing the original issue. In order to deal with that, you'd then need to introduce blanket deprecations on I think we all agree that |
I prefer (3), realize that backwards compatibility is important, don't break it, and instead introduce a better API for solving the same problem.
It's clear from when these methods were first introduced in ef5546c that they were not intended to deal with HTTP specific headers, but with
You can 100% not fix these methods to be backwards compatible and "do the right thing". Either the methods mangle, or they don't. If they mangle, they are not backwards compatible.
99% of users are using HTTP/1 and would expect that The
I disagree. My solution fixes the problem of allowing users to easily deal with HTTP headers. You want to modify You are fixated on the fact that We don't need to introduce blanket deprecations at all. We can keep the existing methods, and just indicate via documentation that it is a bad idea to use them in new code. Old code stays working, new code uses
No, we don't all agree about that. We probably all agree that absent backwards compatibility concerns, it would make more sense for
This approach is backwards incompatible if you change the behavior that currently warns to always treating as an HTTP header. Until you do that, the semantics of the |
Thanks for the great investigation. I don't think consistency is a foolish desire. Being inconsistent leads to real world bugs, hard to use code, and security issues. By choosing (3) we don't fix any existing code paths and end up with confusing/inconsistent interfaces - so I think (3) needs to be combined with warnings & a deprecation strategy.
Yes.
The semantics are already questionable and will remain questionable unless changed. This is an incremental step towards being consistent with other methods of the same name in Not sure if there are any alternatives to fixing this except doing nothing at all to the implementation and adding documentation saying it's questionable, but that does not address my root concern.
|
Ok, never mind about |
de76b8c
to
af48fdb
Compare
As discussed previously there is a mismatch between
Rack::Response#get/set_headers
andRack::Request#get/set_headers
.Specifically,
Rack::Response
headers are actual HTTP headers, whileRack::Request
headers are env key pairs.This PR is one option for changing
Rack::Request#get/set_headers
to be consistent with the semantic of HTTP headers. I think this is the best option. The proposed methods are backwards compatible but emit warnings when used with things which are likely to be env keys.The biggest limitation is that not all HTTP header keys are allowed, specifically headers with periods (
.
) are considered env keys, even though periods are considered validtoken
characters and thus valid header keys. This allowsrack.internal.keys
to pass correctly as non-headers retaining compatibility (following the spec which specifies one period must be included in application specific keys).I believe this PR is 100% backwards compatible and I've split it into two parts: (commit 1) is just the change itself and emits lots of warnings, and (commit 2) is reworking all Rack code to use
env
directly where it makes sense.I believe this PR will allow Rails to remove their "patch" https://github.com/rails/rails/blob/f95c0b7e96eb36bc3efc0c5beffbb9e84ea664e4/actionpack/lib/action_dispatch/http/headers.rb#L121-L129 althought in theory it remains fully compatible.