-
Notifications
You must be signed in to change notification settings - Fork 4
[PB-5850] Add interceptor to the axios client to handle 429 #236
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
Conversation
| afterEach(() => { | ||
| vi.restoreAllMocks(); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Due to our configuration, it is no longer necessary to use this, as it is done automatically.
| } as any; | ||
|
|
||
| beforeEach(() => { | ||
| vi.clearAllMocks(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same
| it('should create a request interceptor with a fresh delay state', () => { | ||
| attachRateLimiterInterceptors(instance); | ||
|
|
||
| expect(createRequestInterceptor).toHaveBeenCalledWith({ pending: null }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some new helpers called call, calls that we can use to validate function calls. Call is used together with toMatchObject to validate the parameters passed to the function.
They are useful for improving the readability of tests and are specialized for this purpose. They are useful for changing the expect that use toHaveBeenCallWith.
| const delayStatePassedToRequest = (createRequestInterceptor as Mock).mock.calls[0][0]; | ||
| const delayStatePassedToResponse = (createResponseInterceptor as Mock).mock.calls[0][2]; | ||
|
|
||
| expect(delayStatePassedToRequest).toBe(delayStatePassedToResponse); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This validation is a bit unusual. Here, you could also use the helper call.
| let retryResponse: AxiosResponse; | ||
|
|
||
| beforeEach(() => { | ||
| vi.clearAllMocks(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this line
|
|
||
| const result = await onRejected(error); | ||
|
|
||
| expect(addJitter).toHaveBeenCalledWith(3000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use new call helper
|
This issue does not seem to be important,we are using
|
|
|
||
| await onRejected(error); | ||
|
|
||
| call(addJitter).toEqual(5000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change toEqual to toStrictEqual
| await expect(onRejected(error)).rejects.toBe(error); | ||
| expect(updateStateFromHeaders).not.toHaveBeenCalled(); | ||
| expect(addJitter).not.toHaveBeenCalled(); | ||
| expect(waitForDelay).not.toHaveBeenCalled(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change toHaveBeenCalled() to toBeCalled()
|


What is Changed / Added
An interceptor has been added to the
drive-server-client(Which underneath uses axios) to handle the 429 'Too many request' issue.Now, uppon recieving a 429 error, it will add a wait mechanism so that concurrent calls can also wait for the limit to reset, having a "jitter" time so that all of the awaited requests dont just retry all at the same time.
It has been added a retry mechanism of max 3 times.
Why
Currently this scenario wasnt being handled and the current client was just throwing the error up