Skip to content

Conversation

@jgiovaresco
Copy link
Contributor

@jgiovaresco jgiovaresco commented Sep 16, 2020

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

Closes #74

@jgiovaresco jgiovaresco force-pushed the fix/74-dont-lowercase-header-keys branch from bfab905 to b5e1039 Compare September 16, 2020 16:30
@develohpanda develohpanda self-requested a review September 16, 2020 19:11
Copy link
Contributor

@develohpanda develohpanda left a comment

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.

@jgiovaresco jgiovaresco force-pushed the fix/74-dont-lowercase-header-keys branch from b5e1039 to 4076e96 Compare September 17, 2020 07:06
Copy link
Contributor

@develohpanda develohpanda left a comment

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

@jgiovaresco jgiovaresco force-pushed the fix/74-dont-lowercase-header-keys branch from 4076e96 to 958b1e0 Compare September 17, 2020 20:15
Copy link
Contributor

@develohpanda develohpanda left a comment

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:master Sep 17, 2020
@jgiovaresco jgiovaresco deleted the fix/74-dont-lowercase-header-keys branch September 18, 2020 11:08
@reynolek
Copy link
Contributor

released in 1.23.0

@erunion
Copy link
Contributor

erunion commented Oct 15, 2020

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 free to 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