BREAKING: Overhaul HTTP headers (make case-insensitive, allow for multiples) #34
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
By converting used header names to lowercase, they can get compared in a case-insensitive way. Coincidentally, they also MUST be lowercased when connecting with HTTP/2 (not implemented yet, but maybe in the future?)
The amusing thing about `HeaderName` being an `enum` is that its values get suggested in one's IDE autocomplete when necessary, so it's harder for users to make typos. However, thanks to `:from` we can still implicitly convert any arbitrary string and use it as a header key.
This unifies handling headers between `Request` and `Response`.
Old server code that may have expected a single header value may be surprised to see multiple of them now, thanks to splitting known headers when parsing a `Request`. Old client code that may have expected repeated header names may be surprised to see a single value, comma- or semicolon-delimited, thanks to merging known headers when sending a `Response`. NOTE: As of this commit, Weblink does not parse quoted " " strings nor CRLF folds in header values correctly. It never used to, but it still does not, too.
Frixuu
changed the title
|
This PR has ballooned more than I expected, so it's time for me to give you a quick status update. What should work (as I understand it):
What's not done yet, but I'm not sure if I want to cram it into the same patch:
|
I'd say split it up, you can group it, back I think we can merge this in, is there any other things you'd like to include in it? |
|
Nah; if there was something I desperately needed to include, I'd set the PR as draft 😅 How would you like to proceed? |
|
I'll take a look today and most likely merge it. |
PXshadow
reviewed
Oct 3, 2024
| return arr; | ||
| } | ||
|
|
||
| public static function populate():Array<Field> { |
There was a problem hiding this comment.
I like the build macro approach well done.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
According to spec, HTTP header field names should be treated in a case-insensitive way.
However, so far this was not the case with Weblink. This meant a bunch of things were silently very broken. For example, a Node.js fetch() client could not send POST request bodies: the server was expecting a
Content-Lengthheader, but gotcontent-lengthinstead.This implementation introduces a new abstract
HeaderName(and a companionHeaderValue). When implicitly assigning aStringto aHeaderName, it simply gets converted to lowercase (plus some ASCII validation). This way it can be used as a key even in default standard library collections and the comparisons would be case-insensitive.Of course, this means the original string casing is lost – but given that HTTP/2 (not supported currently) forces sent header names to be lowercase, I deemed it an acceptable loss.
Also according to spec, multiple header values with the same name can coexist. They can be either set as separate key-value pairs (i.e.
Set-Cookie) or as a comma-separated list (e.g.Accept-Language,If-Match).Requestdid not support that usecase.Responsedid, but made it difficult to query. Both classes now use a specialized collection, aHeaderMap, that allows for multiple values to be associated with one key.