Caseless header comparing in HTTP adapter. #2880
Conversation
It was adding User-Agent and removing Authorization, but only when existing headers had the exact right casing. Node uses caseless logic when managing headers. This was causing some requests to have `User-Agent` appended to the headers object and overriding provided agent strings. Also included is an update to not override the `Content-Length` if it was already defined in the options. It can be desirable to manually specify a content length that does not match the data on hand. Especially for testing.
|
@mastermatt What do you think about using lib/helpers/normalizeHeaderName.js, and there are 2 places merging for headers,
Then both adapters, xhr and http, can be unchanged. |
|
I'd be happy to look at alternative ways of doing this so the adapters don't drift. I didn't want to touch the XHR since I don't know the inner workings as well. While HTTP headers are case-insensitive, not every client and server is perfect about following this (take this PR for example). As such if a user of this client explicitly passes a header, that casing should be maintained across the wire. It's a subtle, but important distinction that my change does not modify the provided headers, just updates the internal logic around adding defaults. That being said, looking at the callers of This project really does probably need a proper header helper/utility class, like in #1291, but that PR has been sitting for two years. |
|
Please can you fix the test in this branch, update it to the latest master, and let me know so I can review it fully. |
Play nice with axios#3703
…checking-http-adapter
|
@jasonsaayman updated and ready to review. I had to merge a conflict with #3703. Tests from both PRs now pass. |
* Caseless header comparing in HTTP adapter. It was adding User-Agent and removing Authorization, but only when existing headers had the exact right casing. Node uses caseless logic when managing headers. This was causing some requests to have `User-Agent` appended to the headers object and overriding provided agent strings. Also included is an update to not override the `Content-Length` if it was already defined in the options. It can be desirable to manually specify a content length that does not match the data on hand. Especially for testing. * Fix eslint error * fixup: update state UA logic Play nice with axios#3703 Co-authored-by: Jay <jasonsaayman@gmail.com>
It was adding User-Agent and removing Authorization, but only when
existing headers had the exact right casing. Node uses caseless logic
when managing headers.
This was causing some requests to have
User-Agentappended to theheaders object and overriding provided agent strings.
Also included is an update to not override the
Content-Lengthif itwas already defined in the options. It can be desirable to manually
specify a content length that does not match the data on hand.
Especially for testing.
Ref: #1237 (dup #2876) (stale PR #1291) nock/nock#1969 nock/nock#1845 https://github.com/sendgrid/sendgrid-nodejs/issues/1083