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

Support header name capitalization #9891

Merged

Conversation

MrDoomBringer
Copy link
Contributor

Closes #6741

Header names will now preserve their capitalization when passed into setContext(). This allows for non-http-spec-compliant servers to use capitalized header names, etc.

Originally the headersToLowerCase function was created to prevent unintentionally duplicating a header instead of overwriting it (see #8447 and #8449). This PR adds an additional loop to that function which, after removing duplicates, returns header names to their original capitalization, hopefully preventing regression on #8447 while still closing #6741.

Because some (non-http-spec-compliant) servers might be expecting header names from us to be lowercase, I'm PRing against release-3.7 instead of main.

Checklist:

  • If this PR is a new feature, please reference an issue where a consensus about the design was reached (not necessary for small changes)
  • Make sure all of the significant new logic is covered by tests

Updates the headersToLowerCase function to allow for intentionally
capitalized header names, hopefully without breaking servers
that expect our header names to be all-lowercase
and hopefully still preventing people from
accidentally duplicating headers
Copy link
Member

@hwillson hwillson left a comment

Choose a reason for hiding this comment

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

Thanks very much for working on this @MrDoomBringer! You rightfully called this out in your PR description:

Because some (non-http-spec-compliant) servers might be expecting header names from us to be lowercase, I'm PRing against release-3.7 instead of main.

I think we would need to go further here however, as this change could be breaking. If people currently have headers defined in their application using upper case notation of some kind, we're converting those headers to all lower case and firing them off to their server of choice. If we ship a minor/patch version of Apollo Client that retains their mixed/upper case however, there is a chance their server can't handle non-standard header case, and fail. This means we have 2 options:

  1. Retarget these changes against Apollo Client 4.0. Or ...
  2. We make the conversion of headers to lower case configurable via an HttpLink option. This conversion would remain on by default, but people that really need header case preserved could set a preserveHeaderCase option to true in the HttpLink constructor, and we'll adjust the conversion internally based on this.

Option 2 is probably our best bet if we want to get this resolved sooner than later (as we could ship this in 3.7), but that means adding/supporting another HttpLink option.

@benjamn @alessbell I'd love to hear your thoughts as well. Thanks!

src/link/http/selectHttpOptionsAndBody.ts Outdated Show resolved Hide resolved
src/link/http/selectHttpOptionsAndBody.ts Outdated Show resolved Hide resolved
src/link/http/selectHttpOptionsAndBody.ts Outdated Show resolved Hide resolved
@MrDoomBringer
Copy link
Contributor Author

MrDoomBringer commented Jul 13, 2022

Thanks a ton for the review @hwillson !

I went ahead and made a feature commit (76655a3) adding the preserveHeaderCase config option following your suggestion. It's toggleable in the HttpLink/BatchHttpLink constructor, and in the http option of setContext (where it will override httplink), following the pattern set by includeExtensions.

Y'all let me know if we think this is a good route forward and I'll add additional testing & documentation :-)

@MrDoomBringer MrDoomBringer merged commit 552b0c7 into apollographql:release-3.7 Sep 13, 2022
@MrDoomBringer MrDoomBringer self-assigned this Sep 13, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

setContext converts all headers to lower case
3 participants