[Internal] Add retries to Authentication Repository #2414
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
polqf
force-pushed
the
bugfix/fix-async-connect
branch
from
5c5735a
to
922f710
Compare
polqf
force-pushed
the
bugfix/fix-async-connect
branch
from
922f710
to
c558462
Compare
polqf
changed the title
polqf
force-pushed
the
bugfix/fix-async-connect
branch
from
c558462
to
22abb42
Compare
martinmitrevski
approved these changes
Dec 16, 2022
There was a problem hiding this comment.
LGTM ✅ I've also manually tested and it recovers after an initial token failure. Would be nice if @alteral does more tests.
Before merging:
- we should update the CHANGELOG, since it can be considered as a new feature
- add more tests (and fix the existing ones if broken)
|
JWT tests just fly! @polqf, you rock! |
martinmitrevski
approved these changes
Dec 16, 2022
| XCTAssertNotNil(repository.tokenProvider) | ||
| waitForExpectations(timeout: 0.1) | ||
| waitForExpectations(timeout: 10000) |
| @@ -549,6 +590,9 @@ final class AuthenticationRepository_Tests: XCTestCase { | |||
| let testError = TestError() | |||
| connectionRepository.connectResult = .failure(testError) | |||
|
|
|||
| // API Result | |||
| apiClient.test_mockResponseResult(Result<GuestUserTokenPayload, Error>.success(GuestUserTokenPayload(user: CurrentUserPayload.dummy(userId: "", role: .user), token: apiToken))) | |||
There was a problem hiding this comment.
We should format this one for better readability. Wonder why this was not done on the pre-commit hook?
| @@ -580,6 +621,9 @@ final class AuthenticationRepository_Tests: XCTestCase { | |||
| // Simulate Success on Connection Repository | |||
| connectionRepository.connectResult = .success(()) | |||
|
|
|||
| // API Result | |||
| apiClient.test_mockResponseResult(Result<GuestUserTokenPayload, Error>.success(GuestUserTokenPayload(user: CurrentUserPayload.dummy(userId: "", role: .user), token: apiToken))) | |||
There was a problem hiding this comment.
Same here (formatting)
|
Kudos, SonarCloud Quality Gate passed! |
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.
🔗 Issue Links
🎯 Goal
Fix E2E tests which were failing due to missing retries after a token fetch failure
🛠 Implementation
This PR adds retries when a token fetch process fails. It does it under the following circumstances:
In order to do that, it also removes state tracking of token attempts refresh from the APIClient.
Now, whenever the Authentication Repository is fetching a token, it notifies the APIClient so it can temporarily suspend the queue. In the case where requests are already in the queue and are being executed, those will be requeued as soon as an expired token is received, to then be relaunched when the queue is resumed.
To be quicker detecting expired tokens, we also try to refresh tokens when the connection changes to
.disconnecting, as well as when it does to.disconnected, and the server error reflects an expired token. Before we were only doing so when the state was.disconnected.🎨 Showcase
☑️ Contributor Checklist
🎁 Meme