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(DataStore): retry on session expired requests #3477

Merged
merged 3 commits into from
Jan 22, 2024

Conversation

lawmicha
Copy link
Contributor

Issue #

#3259
More specifically #3259 (comment)

Case 1: Expired Cognito Token

Description

An expired token will be refreshed automatically when Amplify.API retrieves the token from the token provider, via Amplify.Auth. At this point, it should refresh the token if it is expired. When the token cannot be refreshed because the Refresh token is expired, then API receives an AuthError, which is wrapped in an APIError.operationError. The user needs to sign in again.

DataStore's SyncMutationToCloudOperation uses Amplify.API to make the user pool auth request and receives API.opeationError(,,AuthError.sessionExpired) error. When the session is expired, DataStore shouldn't delete mutation event because the request never made it to the service to perform the authorization check. The PR adds a check for session expired errors and goes into the exponential retry state.

Once the user signs in, and the mutation event gets sent on the next retry, then the response will be handled as normally. Mutation events should only be deleted if the response is Unauthorized, by calling errorHandler to notify the developer that we're dropping the event, then deleting it.

General Checklist

  • Added new tests to cover change, if needed
  • Build succeeds with all target using Swift Package Manager
  • All unit tests pass
  • All integration tests pass
  • Security oriented best practices and standards are followed (e.g. using input sanitization, principle of least privilege, etc)
  • Documentation update for the change if required
  • PR title conforms to conventional commit style
  • New or updated tests include Given When Then inline code documentation and are named accordingly testThing_condition_expectation()
  • If breaking change, documentation/changelog update with migration instructions

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@@ -40,7 +40,8 @@ class RequestRetryablePolicy: RequestRetryable {
.timedOut,
.dataNotAllowed,
.cannotParseResponse,
.networkConnectionLost:
.networkConnectionLost,
.userAuthenticationRequired:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we have a unit test for this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added!

@codecov-commenter
Copy link

codecov-commenter commented Jan 19, 2024

Codecov Report

Attention: 10 lines in your changes are missing coverage. Please review.

Comparison is base (f9b02de) 67.89% compared to head (570d556) 68.03%.
Report is 3 commits behind head on main.

Files Patch % Lines
...ngMutationQueue/SyncMutationToCloudOperation.swift 0.00% 10 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3477      +/-   ##
==========================================
+ Coverage   67.89%   68.03%   +0.13%     
==========================================
  Files        1079     1069      -10     
  Lines       36359    36010     -349     
==========================================
- Hits        24687    24498     -189     
+ Misses      11672    11512     -160     
Flag Coverage Δ
API_plugin_unit_test 67.07% <ø> (ø)
AWSPluginsCore 64.13% <ø> (ø)
Amplify 48.12% <ø> (+0.01%) ⬆️
Analytics_plugin_unit_test 81.16% <ø> (ø)
Auth_plugin_unit_test 78.99% <ø> (ø)
CoreMLPredictions_plugin_unit_test 59.44% <ø> (ø)
DataStore_plugin_unit_test 81.23% <16.66%> (+0.83%) ⬆️
Geo_plugin_unit_test ?
Logging_plugin_unit_test 63.34% <ø> (ø)
Predictions_plugin_unit_test 37.16% <ø> (ø)
PushNotifications_plugin_unit_test 87.13% <ø> (ø)
Storage_plugin_unit_test 77.51% <ø> (ø)
unit_tests 68.03% <16.66%> (+0.13%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@lawmicha lawmicha changed the base branch from main to fix/gh-3259 January 22, 2024 18:03
@lawmicha lawmicha merged commit 11b7d5e into fix/gh-3259 Jan 22, 2024
30 checks passed
@lawmicha lawmicha deleted the lawmicha.ds-session-expired branch January 22, 2024 18:04
thisisabhash added a commit that referenced this pull request Jan 22, 2024
… update retry mechanism for error codes (#3479)

* fix(DataStore): retry on session expired requests

* fix(datastore): Adding secureConnectionFailed as a retryable error (#3475)

* fix(datastore): decode optimistically with paginated list response type (#3474)

* fix(datastore): decode optimistically with paginated list response type

* add unit test cases

* resolve comments

* Update PaginatedListTests.swift

* fix(DataStore): retry on session expired requests (#3477)

* fix(DataStore): retry on session expired requests

* add test

---------

Co-authored-by: Michael Law <[email protected]>
Co-authored-by: Harsh <[email protected]>
Co-authored-by: Di Wu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants