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

fix(core): headers set by the sdk must respect case insensitive keys #627

Merged
merged 7 commits into from
Aug 10, 2021

Conversation

andersfylling
Copy link
Contributor

I noticed that the "Accept" field is added when it is not already present in the request header. However, the check is case sensitive, while the http RFC states that the http header are case insensitive.

This causes the application/json to be added when a user specify accept and not Accept:

    nock(mockBaseUrl)
      .get(new RegExp('/documents/preview'))
      .matchHeader('Accept', 'image/png')
      .query({ documentId: 1 })
      .once()
      .reply(200);

await this.get<ResponseType>(uri, {
      responseType: HttpResponseType.ArrayBuffer,
      headers: {
        accept: 'image/png',
      },
    });

The above test will fail as the accept header contains both image/png and application/json.

@codecov
Copy link

codecov bot commented Aug 2, 2021

Codecov Report

Merging #627 (36e84f8) into master (29b3a17) will decrease coverage by 1.38%.
The diff coverage is 70.62%.

❗ Current head 36e84f8 differs from pull request most recent head b0b8a33. Consider uploading reports for the commit b0b8a33 to get more accurate results

@@            Coverage Diff             @@
##           master     #627      +/-   ##
==========================================
- Coverage   77.15%   75.76%   -1.39%     
==========================================
  Files          87       90       +3     
  Lines        2779     3202     +423     
  Branches      390      443      +53     
==========================================
+ Hits         2144     2426     +282     
- Misses        599      776     +177     
+ Partials       36        0      -36     
Impacted Files Coverage Δ
packages/stable/src/__tests__/testUtils.ts 100.00% <ø> (ø)
packages/stable/src/types.ts 100.00% <ø> (ø)
.../beta/src/api/templateGroups/templateGraphQlApi.ts 40.00% <14.28%> (-26.67%) ⬇️
packages/stable/src/api/3d/nodes3DApi.ts 81.81% <20.00%> (-18.19%) ⬇️
packages/stable/src/api/projects/projectsApi.ts 78.94% <20.00%> (-21.06%) ⬇️
packages/beta/src/types.ts 44.11% <33.33%> (-3.89%) ⬇️
packages/stable/src/api/3d/revisions3DApi.ts 95.65% <33.33%> (-4.35%) ⬇️
packages/core/src/baseCogniteClient.ts 74.54% <40.35%> (-17.46%) ⬇️
...eta/src/api/templateGroups/templateInstancesApi.ts 82.60% <50.00%> (-1.71%) ⬇️
packages/beta/src/cogniteClient.ts 96.96% <66.66%> (+0.09%) ⬆️
... and 83 more

@tommythorsen
Copy link
Contributor

tommythorsen commented Aug 10, 2021

Does this change cause all header names to be lowercased, regardless of what case the users provide them in? If so, I'm not sure it's a good idea.

The ideal implementation would be case-insensitive, but case-preserving at the same time. Meaning that comparisons would be case-insensitive, but the original casing that the user provided the header names in, would be preserved.

@andersfylling
Copy link
Contributor Author

I do see that debugging may be more difficult if the user input is transformed. I updated the code 👍

@andersfylling andersfylling requested a review from a team August 10, 2021 12:26
@andersfylling andersfylling changed the title fix(core): make headers lowercase to avoid case sensitive bugs fix(core): header fields set by the sdk does not respect existing case insensitive keys Aug 10, 2021
@andersfylling andersfylling changed the title fix(core): header fields set by the sdk does not respect existing case insensitive keys fix(core): header fields set by the sdk must respect case insensitive keys Aug 10, 2021
@andersfylling andersfylling changed the title fix(core): header fields set by the sdk must respect case insensitive keys fix(core): headers set by the sdk must respect case insensitive keys Aug 10, 2021
@andersfylling andersfylling merged commit 02703c5 into master Aug 10, 2021
@andersfylling andersfylling deleted the patch/default-headers branch August 10, 2021 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants