Skip to content

fix: don't convert header names to lowercase - #178

Merged
develohpanda merged 2 commits into
Kong:masterfrom
jgiovaresco:fix/74-dont-lowercase-header-keys
Sep 17, 2020
Merged

fix: don't convert header names to lowercase#178
develohpanda merged 2 commits into
Kong:masterfrom
jgiovaresco:fix/74-dont-lowercase-header-keys

Conversation

@jgiovaresco

@jgiovarescojgiovaresco commented Sep 16, 2020

Copy link
Copy Markdown
Contributor

I've completed @darrenjennings branch to remove the .toLowerCase for header names.

Closes#74

@jgiovaresco
jgiovarescoforce-pushed the fix/74-dont-lowercase-header-keys branch from bfab905 to b5e1039CompareSeptember 16, 2020 16:30
@develohpanda
develohpanda self-requested a review September 16, 2020 19:11

@develohpandadevelohpanda left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for finishing this off!

Do you have any thoughts about whether handling headers with HTTP/1.1 (case-insensitive) vs HTTP/2 (case-sensitive) is the responsibility of this library, or responsibility of the consumer? (cc: @darrenjennings)

I am impartial, but since we have access to the version on the HAR object, we might as well follow the RFC.

Comment threadsrc/index.js Outdated
@jgiovaresco
jgiovarescoforce-pushed the fix/74-dont-lowercase-header-keys branch from b5e1039 to 4076e96CompareSeptember 17, 2020 07:06

@develohpandadevelohpanda left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good now, nice to see an additional unit test! 👍

Just a minor comment on the use of regex

Comment threadsrc/index.js Outdated
@jgiovaresco
jgiovarescoforce-pushed the fix/74-dont-lowercase-header-keys branch from 4076e96 to 958b1e0CompareSeptember 17, 2020 20:15

@develohpandadevelohpanda left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nicely done 🙌

@develohpanda
develohpanda merged commit 6afa231 into Kong:masterSep 17, 2020
@jgiovaresco
jgiovaresco deleted the fix/74-dont-lowercase-header-keys branch September 18, 2020 11:08
@reynolek

Copy link
Copy Markdown
Contributor

released in 1.23.0

@erunion

Copy link
Copy Markdown
Contributor

This change has unfortunately introduced some header duping in some cases of commonly used headers like Content-Type where if it's as Content-Type in the HAR, but a target is looking for content-type or adds its own content-type header (like in multipart/form-data where it is overridden) we end up with double headers containing potentially differing values.

I'm working on a PR right now though.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Don't convert header names to lowercase

5 participants

@jgiovaresco@reynolek@erunion@develohpanda@darrenjennings