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.

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

HTTP::Request: store common header names in a normalized way #8061

Merged
merged 1 commit into from Aug 10, 2019

Conversation

@asterite
Copy link
Member

commented Aug 8, 2019

Fixes #8060

Doesn't actually fix it because we aren't storing exactly what was sent, but since headers are case insensitive this shouldn't be a problem. I think the "normalized" way is pretty common and used everywhere so in most cases this way will match what was sent.

If this is a real problem, though, we can revert this optimization.

Show resolved Hide resolved spec/std/http/request_spec.cr Outdated

@asterite asterite force-pushed the asterite:http-headers-normalization branch from 5033080 to 3e9f396 Aug 8, 2019

@asterite asterite force-pushed the asterite:http-headers-normalization branch from 3e9f396 to 3755dc4 Aug 8, 2019

@bcardiff bcardiff added this to the 0.30.1 milestone Aug 8, 2019

@ysbaddaden

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

From HTTP/2 and onwards the normalized form of HTTP headers is... downcased. And HTTP/2 is much more common that it would seem. We should embrace the future (that started yesterday) and just store headers lowercased, we're only going to receive them more and more downcased, even in HTTP/1.

@bcardiff bcardiff removed this from the 0.30.1 milestone Aug 8, 2019

@asterite asterite force-pushed the asterite:http-headers-normalization branch from 3755dc4 to 1b0fcfb Aug 9, 2019

@asterite

This comment has been minimized.

Copy link
Member Author

commented Aug 9, 2019

Updated!

I think we could later use a binary search to optimize this but doing it with slices proved to be a bit hard for me when I tried to timebox it.

@asterite asterite force-pushed the asterite:http-headers-normalization branch from 1b0fcfb to f0c6cd6 Aug 9, 2019

@asterite

This comment has been minimized.

Copy link
Member Author

commented Aug 9, 2019

Done with binary search :-)

(I think I got it right)

@asterite asterite force-pushed the asterite:http-headers-normalization branch from f0c6cd6 to ce97f87 Aug 9, 2019

@j8r

This comment has been minimized.

Copy link
Contributor

commented Aug 9, 2019

Ho, @ysbaddaden is right. For example in GitHub response headers are all downcased:

HTTP/2.0 200 OK
access-control-allow-methods: GET
access-control-max-age: 3600
last-modified: Fri, 09 Aug 2019 18:56:47 GMT
etag: "d6756a52bf468deae45841b35756e6d8"
content-type: text/css
server: AmazonS3
content-encoding: gzip
via: 1.1 varnish
accept-ranges: bytes
date: Fri, 09 Aug 2019 20:46:48 GMT
via: 1.1 varnish
age: 2890
x-served-by: cache-iad2126-IAD, cache-cdg20764-CDG
x-cache: HIT, HIT
x-cache-hits: 1, 340
x-timer: S1565383608.050355,VS0,VE0
vary: Origin, Access-Control-Request-Headers, Access-Control-Request-Method, Accept-Encoding
access-control-allow-origin: *
x-fastly-request-id: b7cdd43b9564b5ec584b54bfde1b8233512b2697
content-length: 75635
X-Firefox-Spdy: h2

But my request headers are in camelcase with dashes.

@straight-shoota

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

@j8r Yes, this is likely the way we're going to move forward. But it's not the point here. We don't want to introduce a breaking change at this point. On the contrary, this PR is supposed to be a bugfix in 0.30.1 for the behaviour that was unintentionally broken in 0.30.0.

The idea to normalize all headers downcased should be discussed in a separate issue and has a broader scope than just parse_header.

@RX14

RX14 approved these changes Aug 10, 2019

@asterite asterite added this to the 0.30.1 milestone Aug 10, 2019

@asterite asterite merged commit 55b664c into crystal-lang:master Aug 10, 2019

4 of 5 checks passed

ci/circleci: test_linux32 Your tests failed on CircleCI
Details
ci/circleci: check_format Your tests passed on CircleCI!
Details
ci/circleci: test_darwin Your tests passed on CircleCI!
Details
ci/circleci: test_linux Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.