fix: override auth header while retrying #896
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cemreyavuz
requested review from
BugGambit,
andersfylling and
polomani
as code owners
cemreyavuz
force-pushed
the
fix-401-second-try
branch
from
8a9e38c
to
d346322
Compare
cemreyavuz
commented
Sep 26, 2022
| @@ -215,7 +216,7 @@ export class BasicHttpClient { | |||
| protected async request<ResponseType>(request: HttpRequest) { | |||
| const mutatedRequest = await this.preRequest(request); | |||
| const rawResponse = await this.rawRequest<ResponseType>(mutatedRequest); | |||
| return this.postRequest(rawResponse, mutatedRequest); | |||
| return this.postRequest(rawResponse, request, mutatedRequest); | |||
There was a problem hiding this comment.
We need a way to differentiate between default headers and original headers in the request. Filtering out the default headers will not work because they can be overridden in the original request.
We need the mutated request to compare the authorization header used in the request, so maybe we can pass that instead of the whole request.
Codecov Report
@@ Coverage Diff @@
## master #896 +/- ##
==========================================
- Coverage 84.06% 79.52% -4.55%
==========================================
Files 70 105 +35
Lines 1952 3389 +1437
Branches 249 437 +188
==========================================
+ Hits 1641 2695 +1054
- Misses 301 692 +391
+ Partials 10 2 -8
|
BugGambit
approved these changes
Sep 26, 2022
|
@cemreyavuz Did the PR fix the problem? |
|
@BugGambit yes the problem is fixed with this version - also I have used it for the whole day and i didn't see any problem: 401s, retrys, etc. Everything looked fine 👍🏻 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Latest change to this line caused a regression:
cognite-sdk-js/packages/core/src/httpClient/basicHttpClient.ts
Line 218 in 9797d78
Updated tokens are not used on retries for 401s now.
Because now we are passing
mutatedRequesttopostRequest, not all the default headers are applied properly. For us, the important one is the authorization header. Here we keep using the old authorization header on retries of because of this ordering:cognite-sdk-js/packages/core/src/httpClient/basicHttpClient.ts
Lines 172 to 177 in 9797d78
I'm not sure if we should change this to other way - then we wouldn't be able to override default headers. So I went with adding another parameter to
postRequestfor keeping both the original request and the mutated version. Open to suggestions to clean it up.