fix(aws-amplify/auth): debouncing get user session calls

[AllanZhengYP]

Description of changes

Short version

This change adds back changes made in #10553, which was reverted in #10630. It was reverted because of DataStore auth integration test failures.
The DataStore issue causes every authenticated query/mutation request to make extra Cognito requests.

The additional commit 1fe6949 resolves the DataStore issue.

Integration test passed here

Long version

In general this change is to reduce concurrent API requests to CognitoIdentityProvider.GetUser. Compared to previously reverted PR, the additional changes are to resolve the issue that DataStore makes more Cognito requests before each query. Here's 2 screenshots of the network tab of an App sign-in a user and querySubscribe to a DataStore model. The 2 screenshots shows the difference before and after the change:

Network tab before the change

Network tab after the change

From the screenshots above, you can see not only did the DataStore make more Cognito requests, but also the Authenticator UI is make more Cognito requests. This is likely due to unintended removal localstorage.

Here's an analogue of the workflow of the DataStore request authentication:

1. For any query/mutation in DataStore module, and the model is marked as cognito user pool authentication, _headerBasedAuth() is called:

   amplify-js/packages/api-graphql/src/GraphQLAPI.ts

   Lines 170 to 178 in 0fa40e6

   	case 'AMAZON_COGNITO_USER_POOLS':
   	try {
   	const session = await this.Auth.currentSession();
   	headers = {
   	Authorization: session.getAccessToken().getJwtToken(),
   	};
   	} catch (e) {
   	throw new Error(GraphQLAuthError.NO_CURRENT_USER);
   	}

2. Fetch the current user pool user in Auth module:

   amplify-js/packages/auth/src/Auth.ts

   Lines 1573 to 1575 in 1fe6949

   	public currentUserPoolUser(
   	params?: CurrentUserOpts
   	): Promise<CognitoUser | any> {

   1. First it loads the user session with user.getSession(). This function is debounced and only first of concurrent calls are made. The other concurrent invocations will resolve the promise of the 1st invocation:

      amplify-js/packages/auth/src/Auth.ts

      Line 1791 in bbe44ee

      private async _userSession(user?: CognitoUser): Promise<CognitoUserSession> {

   2. With the user session, load the rest of the user attributes with user.getUserData(). This function is not debounced:

      amplify-js/packages/auth/src/Auth.ts

      Line 1639 in bbe44ee

      user.getUserData(

What caused more Cognito calls?
For every DataStore query/mutation invocation, it will reach to the code where we set user session to CognitoUser instance:

amplify-js/packages/auth/src/Auth.ts

Line 1835 in bbe44ee

user.setSignInUserSession(userSession!);

This is necessary because we debounce the concurrent call the fetch the user session. But we still need to set the fetched user session to each of the concurrent CognitoUser instances. If we look deeper to the user.setSignInUserSession implementation:

amplify-js/packages/amazon-cognito-identity-js/src/CognitoUser.js

Lines 95 to 99 in a238031

	setSignInUserSession(signInUserSession) {
	this.clearCachedUserData();
	this.signInUserSession = signInUserSession;
	this.cacheTokens();
	}

. We will find it will remove the user data(including attributes) besides setting the private signInUserSession property. As a result, when we call user.getUserData(), we always find the user data is disappears from the local storage. Hense, extra Cognito requests need to be made to fetch the user data repeatedly.

What is the solution?
The solution is simple, instead of calling user.setSignInUserSession(), we can set the private member directly. So the userdata localstorage won't be removed. When user.getUserData() is called, it won't make API request, but load from localstorage.

What is really causing the integration test to fail?
It's caused by interference between the tests. The each integ test is ended by the ready event. The event change will trigger new graphql query. Because now each graphql query causes GetUser API call and hence refreshes localstorage. By the time previous test ended and 2nd test started, the localstorage is reloaded by the async requests from the 1st test.

Checklist

• PR description included
• yarn test passes
• Tests are changed or added
• Relevant documentation is changed or added (and PR referenced)

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

[codecov-commenter]

Codecov Report

Merging #10654 (ac8f5e9) into main (623374d) will increase coverage by 0.00%.
The diff coverage is 96.42%.

@@           Coverage Diff           @@
##             main   #10654   +/-   ##
=======================================
  Coverage   85.59%   85.59%           
=======================================
  Files         196      196           
  Lines       18236    18237    +1     
  Branches     3887     3886    -1     
=======================================
+ Hits        15609    15610    +1     
  Misses       2549     2549           
  Partials       78       78           

Impacted Files	Coverage Δ
packages/auth/src/Auth.ts	87.14% <96.42%> (+0.17%)	⬆️
...kages/amazon-cognito-identity-js/src/BigInteger.js	89.31% <0.00%> (-0.41%)	⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[jimblanc]

Looks good, small question about de-bounce logic. Appreciate the detailed PR write-up!

[jimblanc]

I realize original PR had this also, but is this a safe assumption to make? E.g. if user.getSession takes longer than normal and in the mean time a different user logs in, would this method return the session from the original user?

Also, small typo asumes

[AllanZhengYP]

Confirmed with @elorzafe this is low risk, but I'd like him to chime-in.

[elorzafe]

is a good concern @jimblanc this class could potenially have a RC on this, do you think we could add in a separate PR a mechanism to use this debouncer for other operations as well?

[elorzafe]

Just minor comments on tests but overall looks good!

Thanks @AllanZhengYP 🌮

[elorzafe]

is a good concern @jimblanc this class could potenially have a RC on this, do you think we could add in a separate PR a mechanism to use this debouncer for other operations as well?

[elorzafe]

Looks good to me

Thanks @AllanZhengYP 🚢