feat(clients): use custom cognito-identity and pinpoint clients #11427

AllanZhengYP · 2023-05-29T21:54:55Z

Description of changes

Custom clients for Cognito Identity and Pinpoint to reduce the library bundle size. No interface change or compatibility change.

Bundle size reduction from custom clients:

Category	Current size	new size
analytics	56.5 kB	29.7 kB
api-graphql	104.5 kB	86.1 kB
api-rest	47.5 kB	28.7 kB
api	106 kB	87.4 kB
auth	73 kB	53.3 kB
datastore	155 kB	135.5 kB
geo	65 kB	50.2 kB
interactions	87 kB	74 kB
notifications	71 kB	29 kB
pubsub(IoT)	97.5 kB	78.5 kB
pubsub(Mqtt)	97.5 kB	78.4 kB

Description of how you validated changes

Unit test; Integrations test; Amplify UI canary; Amplify Studio build

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.

…eware (#11188) * feat(clients): middleware interface and retry middleware * test(clients): retry middleare unit test * fix(clients): middleware type to include options * feat(clients): add retry middleware unit test and update interface * chore(clients): update bundle size limit for fetch and retry * chore(clients): add retry docs; fix format * fix(clients): address feedbacks

* fix(retry): add metadata to returns from retry middldeware; fix minor bugs * feat(clients): address retry middleware feedbacks Co-authored-by: Chris F <5827964+cshfang@users.noreply.github.com>

…11216)

* feat(clients): implement service API composer * chore(clients): rename middleware interface * feat(clients): middleware handler interface can be non request/response interface * chore(clients): move fetch handler to handlers folder * feat(clients): implement user agent middleware * feat(clients): implement unauth aws transfer handler * feat(clients): implement api handler composer * feat(clients): implement cognito-identity client * chore(clients): update bundle size limit * feat(clients): add serde utils; remove retry in api handler composer * feat(clients): integrate cognito-identity client to Credentials class * fix(clients): make retryDecider interface async * chore: move cognito client to dev dep * chore(clients): use Pascale path name * fix(clients): handle fetch response.body undefined in RN * chore: publish under v5-custom-clients dist-tag * fix(clients): read body once for errors

* test(clients): add cognito-identity functional test * chore(clients): prefer destructuring parameters at top

* feat(clients): Add custom signature v4 signer * Updated docstrings * Extracted common code * Use == null instead of isNil * Add unit tests * Add missing licensing headers * Use test case options in presign tests * Fixed comment * Add test for data hashing with SourceData keys * Remove buffer dependency * Remove internal sdk dependency

* feat(clients): Add signing middleware * Use closure for clock offset * Add bundle size test entry * Address comments

…1311) * feat(clients): support CN partition by adding DNS suffix resolver * chore(clients): update bundle size test limit * fix(clients): address feedbacks * chore: update bundle size limit

* feat(clients) Add updateEndpoint API * Rename handler

* feat(clients) Add putEvents API * Add additional verification for expected date format * Mark API as internal

* feat(clients) Add getInAppMessages API * Update unit test

Co-authored-by: Allan Zheng <zheallan@amazon.com>

…11374)

.circleci/config.yml

stocaaro

I'm only half way through, submitting comments now and will return to this in a couple hours.

packages/analytics/src/Providers/EventBuffer.ts

packages/api-graphql/package.json

packages/core/src/clients/handlers/fetch.ts

packages/core/__tests__/AwsClients/Pinpoint/updateEndpoint-test.ts

packages/core/__tests__/clients/utils/isClockSkewedError-test.ts

package.json

packages/core/src/AwsClients/CognitoIdentity/getCredentialsForIdentity.ts

packages/core/src/AwsClients/CognitoIdentity/types.ts

packages/core/src/Credentials.ts

packages/core/src/clients/internal/composeServiceApi.ts

packages/notifications/src/InAppMessaging/Providers/AWSPinpointProvider/utils.ts

package.json

stocaaro

LGTM

svidgen

Lots of comments for you! Feel free to take most as signaling my preferences for future PR's. And on that note, let's try strategize future PR's to be more focused, please.

Four things I'd want touched on in this PR before approving:

The potential race in the response stream/body reader/cache promise.
Confirmation that error cases are tested to the extent they need to be.
A link to the encodeURIComponent RFC gapfill.
I had a comment somewhere in here about something that sounds like a latent bomb. Can't remember where. But, something there to settle my nerves, please!

In general, a few things were initially hard to follow. But, nothing that should block the PR. And, it seems like some of the harder-to-follow bits are in support of some pretty easy-to-read client/"service api" generation. So, net good, IMO. Good work!

I think most of my comments are about half-baked docstrings. If you have time, I'd love to see followup PR (at least eventually) with meaningful docstrings. It looks like you started in some areas and produced some really helpful docs and maybe just lost steam.

And my disclosure: I'm not an expert on the auth exchanges, and I didn't look to closely at the pinpoint stuff. I assume others are reviewing these in depth.

packages/core/src/clients/handlers/fetch.ts

svidgen · 2023-05-31T22:42:16Z

packages/core/src/clients/handlers/fetch.ts

+	let cached: T;
+	return async () => {
+		if (!cached) {
+			cached = await payloadAccessor();
+		}
+		return cached;
+	};


This formulation contains an inherent race. If this is invoked repeatedly in rapid succession, multiple calls to payloadAccessor() can be spawned concurrently. The race can be eliminated by storing and branching on the source promise directly instead of branching on the awaited result.

Suggested change

let cached: T;

return async () => {

if (!cached) {

cached = await payloadAccessor();

}

return cached;

};

let cached: Promise<T>;

return async () => {

if (!cached) {

// Explicitly not awaiting. Intermediate await would add overhead and

// introduce a possible race in the event that this wrapper is called

// again before the first `payloadAccessor` call resolves.

cached = payloadAccessor();

}

return cached;

};

I've tried to demo what I mean in this playground.

The inherent race might be mitigated due to calls being internal and fairly well-controlled. But, I don't have my head around this all yet to be confident. So, at the moment, I'd be more comfortable if we could eliminate the possibility.

This race will only happen if the withPayloadCaching is called concurrently. However payload accessor is only called once per http response

svidgen · 2023-06-01T14:45:53Z

packages/core/src/clients/handlers/fetch.ts

+	const responseHeaders = {};
+	resp.headers?.forEach((value, key) => {
+		responseHeaders[key.toLowerCase()] = value;
+	});


I didn't expect to see this here. That doesn't mean it's wrong, but perhaps it warrants a comment. Why is lower-casing header keys a responsibility of this handler?

packages/core/__tests__/AwsClients/CognitoIdentity/getCredentialsForIdentity-test.ts

packages/core/__tests__/AwsClients/CognitoIdentity/getId-test.ts

packages/core/src/clients/middleware/retry/jitteredBackoff.ts

svidgen · 2023-06-01T18:48:37Z

packages/core/src/clients/middleware/retry/jitteredBackoff.ts

+export const jitteredBackoff: RetryOptions['computeDelay'] = attempt => {
+	const delayFunction = jitteredBackoffUtil();
+	const delay = delayFunction(attempt);
+	return delay === false ? MAX_DELAY_MS : delay;


Is delay ever > MAX_DELAY_MS?

packages/core/src/AwsClients/CognitoIdentity/base.ts

svidgen · 2023-06-01T19:59:36Z

packages/core/src/clients/middleware/retry/middleware.ts

+	maxAttempts = DEFAULT_RETRY_ATTEMPTS,
+	retryDecider,
+	computeDelay,
+	abortSignal,


Interesting ... I must have read right past this before or it didn't "click" for me. But, having this here means that we build the middleware for each request, correct?

Not a blocker. But, that seems odd to me.

Yes, the composing of pure functional middleware happens for each request. It's the same for AWS SDK too.

svidgen · 2023-06-01T20:06:41Z

packages/core/src/clients/internal/composeServiceApi.ts

+	serializer: (input: Input, endpoint: Endpoint) => HttpRequest,
+	deserializer: (output: HttpResponse) => Promise<Output>,


FWIW: Here and throughout, I find it a little odd that these functions are nouns instead of verbs.

elorzafe

This is awesome!!

Thanks for the hard work and putting this out.
I have some nit comments, but feel free to update later.

packages/amazon-cognito-identity-js/CHANGELOG.md

.github/workflows/issue-pending-response.yml

.github/workflows/snyk-security.yml

packages/amazon-cognito-identity-js/README.md

packages/amazon-cognito-identity-js/index.d.ts

packages/core/src/Credentials.ts

elorzafe · 2023-06-01T20:14:49Z

packages/core/src/Credentials.ts

-		let credentials = undefined;
-		if (identity_id) {
-			const cognitoIdentityParams: FromCognitoIdentityParameters = {
+		const credentialsProvider = async () => {


nit: the name could be authenticatedCredentialsProvider

elorzafe · 2023-06-01T20:17:01Z

packages/core/src/Credentials.ts

-						IdentityPoolId: identityPoolId,
-					})
-				);
+		const credentialsProvider = async () => {


nit: I would name it guestCredentialsProvider

semi-nit: This code is very similar to a code below

packages/core/src/AwsClients/Pinpoint/types.ts

elorzafe · 2023-06-01T20:20:35Z

packages/core/src/AwsClients/CognitoIdentity/base.ts

+ * The endpoint resolver function that returns the endpoint URL for a given region.
+ */
+const endpointResolver = ({ region }: EndpointResolverOptions) => ({
+	url: new URL(`https://cognito-identity.${region}.${getDnsSuffix(region)}`),


nit: reuse SERVICE_NAME to save a couple of bytes

elorzafe

Great work

wlee221

Approving for the devops portion (which got removed anyway 😛)

svidgen

Thanks for the rev!

svidgen · 2023-06-02T14:04:06Z

packages/core/src/clients/middleware/retry/jitteredBackoff.ts

-const MAX_DELAY_MS = 5 * 60 * 1000;
+const DEFAULT_MAX_DELAY_MS = 5 * 60 * 1000;

 export const jitteredBackoff: RetryOptions['computeDelay'] = attempt => {
-	const delayFunction = jitteredBackoffUtil();
+	const delayFunction = jitteredBackoffUtil(DEFAULT_MAX_DELAY_MS);
 	const delay = delayFunction(attempt);
-	return delay === false ? MAX_DELAY_MS : delay;
+	// The delayFunction returns false when the delay is greater than the max delay(5 mins).
+	// In this case, the retry middleware will delay 5 mins instead, as a ceiling of the delay.
+	return delay === false ? DEFAULT_MAX_DELAY_MS : delay;
 };


It's a bit odd having both the delay function and this function doing conditional things around the DEFAULT_MAX_DELAY_MS. But, I think I can see how it makes sense. And, the change addresses the larger concern I had, so I appreciate it. Thanks!

svidgen · 2023-06-02T14:05:41Z

packages/core/src/clients/middleware/signing/utils/extendedEncodeURIComponent.ts

+// SPDX-License-Identifier: Apache-2.0
+
+/**
+ * Wraps encodeURIComponent to encode additional characters to fully adhere to RFC 3986.


Not the kind of reference I was hoping for. But, it does demonstrates that this copies another established library that does AWS stuff. So, I'll take it! Thanks!

svidgen · 2023-06-02T14:11:35Z

packages/core/src/clients/middleware/signing/utils/isClockSkewed.ts

@@ -3,16 +3,16 @@

 import { getSkewCorrectedDate } from './getSkewCorrectedDate';

-const SKEW_WINDOW = 5 * 60 * 1000; // 5 minutes
+// 5 mins in milliseconds. Ref: https://github.com/aws/aws-sdk-js-v3/blob/6c0f44fab30a1bb2134af47362a31332abc3666b/packages/middleware-signing/src/utils/isClockSkewed.ts#L10


Not quite the explanation I thought we'd have, but it works for me!

svidgen

Thanks for the rev!

AllanZhengYP and others added 30 commits March 22, 2023 14:01

chore(core): bump to ts 5.0 (#11077)

75e80e3

feat(clients): basic types and fetch handler (#11120)

79f8ae8

fix(retry): add metadata to returns from retry middldeware (#11212)

7fc0540

* fix(retry): add metadata to returns from retry middldeware; fix minor bugs * feat(clients): address retry middleware feedbacks Co-authored-by: Chris F <5827964+cshfang@users.noreply.github.com>

Merge remote-tracking branch 'upstream/main' into v5/custom-clients

addfe0e

chore(core): update size limit

080edee

Merge remote-tracking branch 'upstream/main' into v5/custom-clients (#…

e39314f

…11216)

chore(clients): enable tagged release of custom clients (#11267)

bc79b21

test(clients): add functional test to cognito identity client (#11266)

b593887

* test(clients): add cognito-identity functional test * chore(clients): prefer destructuring parameters at top

feat(clients): add useragent to cognito identity (#11269)

475f26b

chore(clients): add license header to files (#11292)

0cebaf5

chore(clients): Make signing functions synchronous (#11307)

56dc9e0

chore(clients): Replace existing Signer implementation (#11310)

1755800

chore(clients): Add @internal annotation (#11320)

3d607ff

feat(clients): Add signing middleware (#11323)

de1ce7b

* feat(clients): Add signing middleware * Use closure for clock offset * Add bundle size test entry * Address comments

feat(clients): support CN partition by adding DNS suffix resolver (#1…

9de2975

…1311) * feat(clients): support CN partition by adding DNS suffix resolver * chore(clients): update bundle size test limit * fix(clients): address feedbacks * chore: update bundle size limit

chore(clients): Add context to some regex (#11334)

497a105

feat(clients) Add updateEndpoint API (#11330)

36ca0bd

* feat(clients) Add updateEndpoint API * Rename handler

chore(clients): Use DNS suffix util in Pinpoint client (#11340)

40dbb3a

chore(clients): Annotate custom client APIs with @internal (#11347)

de56f9e

feat(clients) Add putEvents API (#11342)

a188ae7

* feat(clients) Add putEvents API * Add additional verification for expected date format * Mark API as internal

feat(clients) Add getInAppMessages API (#11348)

48162c1

* feat(clients) Add getInAppMessages API * Update unit test

chore(clients): Replace SDK Pinpoint Client (#11359)

bd7883d

Co-authored-by: Allan Zheng <zheallan@amazon.com>

Merge remote-tracking branch 'upstream/main' into v5/custom-clients

926d1ef

chore: update size limit

a2c3f49

Merge remote-tracking branch 'upstream/main' into v5/custom-clients (#…

d81af13

…11374)

chore(clients): export pinpoint client from internal subpath (#11369)

b9b5c47

feat(clients): allow fetch handler to read body multiple times

868fb4a

AllanZhengYP requested review from a team as code owners May 31, 2023 18:20

AllanZhengYP commented May 31, 2023

View reviewed changes

.circleci/config.yml Outdated Show resolved Hide resolved

stocaaro reviewed May 31, 2023

View reviewed changes

AllanZhengYP commented May 31, 2023

View reviewed changes

package.json Outdated Show resolved Hide resolved

AllanZhengYP requested review from ioanabrooks and Jshhhh May 31, 2023 22:58

stocaaro reviewed Jun 1, 2023

View reviewed changes

Jshhhh previously approved these changes Jun 1, 2023

View reviewed changes

stocaaro previously approved these changes Jun 1, 2023

View reviewed changes

svidgen reviewed Jun 1, 2023

View reviewed changes

elorzafe previously approved these changes Jun 1, 2023

View reviewed changes

chore: prepare for release; split out data packages diffs

47ff2e1

AllanZhengYP dismissed stale reviews from elorzafe, stocaaro, and Jshhhh via 9beb16b June 1, 2023 22:31

AllanZhengYP force-pushed the v5/custom-clients branch from 9beb16b to c67f268 Compare June 1, 2023 22:40

fix: address feedbacks

2d6eecf

AllanZhengYP force-pushed the v5/custom-clients branch from c67f268 to 2d6eecf Compare June 1, 2023 23:20

AllanZhengYP requested a review from wlee221 June 1, 2023 23:26

elorzafe approved these changes Jun 1, 2023

View reviewed changes

wlee221 approved these changes Jun 2, 2023

View reviewed changes

stocaaro approved these changes Jun 2, 2023

View reviewed changes

svidgen approved these changes Jun 2, 2023

View reviewed changes

Merge branch 'main' into v5/custom-clients

e402fa7

AllanZhengYP merged commit 2d934a3 into main Jun 2, 2023

This was referenced Jun 6, 2023

chore(geo): update size limit #11462

Merged

chore: sync with main #11463

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(clients): use custom cognito-identity and pinpoint clients #11427

feat(clients): use custom cognito-identity and pinpoint clients #11427

AllanZhengYP commented May 29, 2023 •

edited

Loading

stocaaro left a comment

stocaaro left a comment

svidgen left a comment

svidgen May 31, 2023

AllanZhengYP Jun 1, 2023

svidgen Jun 1, 2023

svidgen Jun 1, 2023

svidgen Jun 1, 2023

AllanZhengYP Jun 1, 2023

svidgen Jun 1, 2023

elorzafe left a comment

elorzafe Jun 1, 2023

elorzafe Jun 1, 2023

elorzafe Jun 1, 2023

elorzafe left a comment

wlee221 left a comment

svidgen left a comment

svidgen Jun 2, 2023

svidgen Jun 2, 2023

svidgen Jun 2, 2023

svidgen left a comment

		serializer: (input: Input, endpoint: Endpoint) => HttpRequest,
		deserializer: (output: HttpResponse) => Promise<Output>,

feat(clients): use custom cognito-identity and pinpoint clients #11427

feat(clients): use custom cognito-identity and pinpoint clients #11427

Conversation

AllanZhengYP commented May 29, 2023 • edited Loading

Description of changes

Description of how you validated changes

Checklist

stocaaro left a comment

Choose a reason for hiding this comment

stocaaro left a comment

Choose a reason for hiding this comment

svidgen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elorzafe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elorzafe left a comment

Choose a reason for hiding this comment

wlee221 left a comment

Choose a reason for hiding this comment

svidgen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

svidgen left a comment

Choose a reason for hiding this comment

AllanZhengYP commented May 29, 2023 •

edited

Loading