Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Rack::Mock#parse_cookies_from_header does not follow SPEC #1629

Closed
mpalmer opened this issue Mar 29, 2020 · 7 comments · Fixed by #1687
Closed

Rack::Mock#parse_cookies_from_header does not follow SPEC #1629

mpalmer opened this issue Mar 29, 2020 · 7 comments · Fixed by #1687

Comments

@mpalmer
Copy link
Contributor

mpalmer commented Mar 29, 2020

From SPEC:

The header must respond to each, and yield values of key and value.

However, Rack::Mock#parse_cookies_from_header calls has_key? on the headers. As such, this breaks any Rack application which takes the spec at its word, and provides another object which responds to each and yields values of key and value (such as, for instance, an array of two-element arrays representing header names and values).
@ioquatix
Copy link
Member

It should use Rack::Utils::HeaderHash probably.

@ioquatix
Copy link
Member

@mpalmer do you want to submit a PR? Try wrapping the headers in Rack::Utils::HeaderHash[headers].

@mpalmer
Copy link
Contributor Author

mpalmer commented Mar 30, 2020

Yes, I can whip up a PR. I was thinking of just iterating the original_headers looking for the cookie ones; that seems like it'd be less overhead than converting to a HeaderHash just to pluck one (known) key out. Since it's for tests, though, I'm assuming that any difference would be negligible.

mpalmer added a commit to mpalmer/rack that referenced this issue Mar 30, 2020
@mpalmer
Bring Rack::Mock cookie parsing into SPEC (fixes rack#1629)
97c9c24 
#has_key? isn't required to be supported by the object in the
`headers` element of the response coming back out of the app.
This replaces that call with a transmogrification into a
HeaderHash so we can use an index as normal.  The odd
`hash[] || ""` construction is necessary because HeaderHash#fetch
doesn't respect header name case insensitivity.
@mpalmer
Copy link
Contributor Author

mpalmer commented Mar 30, 2020

A wild PR appears! It strikes!

@ioquatix
Copy link
Member

HeaderHash is memoized can you please check if in the typical case it can be almost a no-op.

@mpalmer
Copy link
Contributor Author

mpalmer commented Mar 30, 2020

If I'm understanding you correctly, then yes, if what gets passed into the MockResponse is already a HeaderHash, then there's no overhead of re-parsing, so it should be practically a no-op. My PR uses HeaderHash[], so it will benefit from this.

@ioquatix
Copy link
Member

Sorry, was putting kids to bed, that makes sense!

mpalmer added a commit to mpalmer/rack that referenced this issue Mar 30, 2020
@mpalmer
Bring Rack::Mock cookie parsing into SPEC (fixes rack#1629)
59b7dad 
`#has_key?` isn't required to be supported by the object in the `headers`
element of the response coming back out of the app.  This replaces that call
with a transmogrification into a `HeaderHash` so we can use an index as
normal.  The odd `hash[] || ""` construction is necessary because
`HeaderHash#fetch` doesn't respect header name case insensitivity.
mpalmer added a commit to mpalmer/rack that referenced this issue Mar 30, 2020
@mpalmer
Bring Rack::Mock cookie parsing into SPEC (fixes rack#1629)
d445f04 
`#has_key?` isn't required to be supported by the object in the `headers`
element of the response coming back out of the app.  This replaces that call
with a transmogrification into a `HeaderHash` so we can use an index as
normal.  The odd `hash[] || ""` construction is necessary because
`HeaderHash#fetch` doesn't respect header name case insensitivity.
mpalmer added a commit to mpalmer/rack that referenced this issue Mar 31, 2020
@mpalmer
Allow HeaderHash#fetch to respect case insensitivity
4aeda5c 
It just makes good sense in general, but it specifically makes the
proximate fix for rack#1629 a lot cleaner.
mpalmer added a commit to mpalmer/rack that referenced this issue Mar 31, 2020
@mpalmer
Bring Rack::Mock cookie parsing into SPEC (fixes rack#1629)
41a5e9f 
`#has_key?` isn't required to be supported by the object in the `headers`
element of the response coming back out of the app.  This replaces that call
with a transmogrification into a `HeaderHash` so we can use an index as
normal.  The odd `hash[] || ""` construction is necessary because
`HeaderHash#fetch` doesn't respect header name case insensitivity.
jeremyevans added a commit to jeremyevans/rack that referenced this issue Jul 14, 2020
@jeremyevans
Make Rack::MockResponse handle non-hash headers
46302ae 
Rack::MockResponse inherits from Rack::Response, which already
uses a HeaderHash for the headers.  The original_headers were
only used for cookie parsing, which for some reason was happening
before the call to super in initialize (so the headers weren't
available yet). There seems to be no reason why the cookie parsing
can't happen after the call to super, in which case we can use the
headers directly.

Co-Authored-By: Matt Palmer <mpalmer@hezmatt.org>

Fixes rack#1629
Fixes rack#1630
jeremyevans added a commit to jeremyevans/rack that referenced this issue Jul 14, 2020
@jeremyevans @mpalmer
Make Rack::MockResponse handle non-hash headers
30ecc6c 
Rack::MockResponse inherits from Rack::Response, which already
uses a HeaderHash for the headers.  The original_headers were
only used for cookie parsing, which for some reason was happening
before the call to super in initialize (so the headers weren't
available yet). There seems to be no reason why the cookie parsing
can't happen after the call to super, in which case we can use the
headers directly.

Fixes rack#1629
Fixes rack#1630

Co-authored-by: Matt Palmer <mpalmer@hezmatt.org>
@jeremyevans jeremyevans mentioned this issue Jul 14, 2020
Merged
jeremyevans added a commit that referenced this issue Dec 1, 2020
@jeremyevans @mpalmer
Make Rack::MockResponse handle non-hash headers
d3225f7 
Rack::MockResponse inherits from Rack::Response, which already
uses a HeaderHash for the headers.  The original_headers were
only used for cookie parsing, which for some reason was happening
before the call to super in initialize (so the headers weren't
available yet). There seems to be no reason why the cookie parsing
can't happen after the call to super, in which case we can use the
headers directly.

Fixes #1629
Fixes #1630

Co-authored-by: Matt Palmer <mpalmer@hezmatt.org>
This was referenced Mar 8, 2021
Open
Open
Open
Open
Open
Open
Open
Open
Merged
Open
Open
Open
@dependabot dependabot bot mentioned this issue Mar 15, 2021
Open
This was referenced Mar 15, 2021
Open
Open
Closed
Open
Open
Open
Open
Open
Open
Open
Open
Open
Closed
Open
Open
Open
Open
Open
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Make Rack::MockResponse handle non-hash headers jeremyevans/rack
2 participants
@mpalmer @ioquatix