Skip to content

Commit

Permalink
feat(storage): move forceRefresh logics to storage clients instead of…
Browse files Browse the repository at this point in the history 
… global
  • Loading branch information
@AllanZhengYP
AllanZhengYP committed Jul 30, 2024
1 parent 6a938df commit e5a3226
Show file tree
Hide file tree
Showing 12 changed files with 195 additions and 138 deletions.
55 changes: 0 additions & 55 deletions packages/core/__tests__/clients/middleware/retry/defaultRetryDecider.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,10 @@
import { HttpResponse } from '../../../../src/clients';
import { getRetryDecider } from '../../../../src/clients/middleware/retry';
import { isClockSkewError } from '../../../../src/clients/middleware/retry/isClockSkewError';
import { isCredentialsExpiredError as isCredentialsExpiredErrorValidator } from '../../../../src/clients/middleware/retry/isCredentialsExpiredError';

jest.mock('../../../../src/clients/middleware/retry/isClockSkewError');
jest.mock('../../../../src/clients/middleware/retry/isCredentialsExpiredError');

const mockIsClockSkewError = jest.mocked(isClockSkewError);
const mockIsCredentialsExpiredError = jest.mocked(
isCredentialsExpiredErrorValidator,
);

describe('getRetryDecider', () => {
const mockErrorParser = jest.fn();
Expand All @@ -35,7 +30,6 @@ describe('getRetryDecider', () => {
const { retryable, isCredentialsExpiredError } = await retryDecider(
mockHttpResponse,
connectionError,
{},
);
expect(retryable).toBe(true);
expect(isCredentialsExpiredError).toBeFalsy();
Expand Down Expand Up @@ -65,7 +59,6 @@ describe('getRetryDecider', () => {
const { retryable, isCredentialsExpiredError } = await retryDecider(
mockHttpResponse,
undefined,
{},
);
expect(retryable).toBe(true);
expect(isCredentialsExpiredError).toBeFalsy();
Expand All @@ -83,7 +76,6 @@ describe('getRetryDecider', () => {
statusCode: 429,
},
undefined,
{},
);
expect(retryable).toBe(true);
expect(isInvalidCredentialsError).toBeFalsy();
Expand All @@ -101,7 +93,6 @@ describe('getRetryDecider', () => {
const { retryable, isCredentialsExpiredError } = await retryDecider(
mockHttpResponse,
undefined,
{},
);
expect(retryable).toBe(true);
expect(isCredentialsExpiredError).toBeFalsy();
Expand All @@ -122,7 +113,6 @@ describe('getRetryDecider', () => {
statusCode,
},
undefined,
{},
);
expect(retryable).toBe(true);
expect(isCredentialsExpiredError).toBeFalsy();
Expand All @@ -138,54 +128,9 @@ describe('getRetryDecider', () => {
const { retryable, isCredentialsExpiredError } = await retryDecider(
mockHttpResponse,
undefined,
{},
);
expect(retryable).toBe(true);
expect(isCredentialsExpiredError).toBeFalsy();
},
);

describe('handling expired token errors', () => {
const mockErrorName = 'ExpiredToken';
const mockErrorMessage = 'Token expired';
it.each([
{
code: mockErrorName,
message: mockErrorMessage,
},
{
name: mockErrorName,
message: mockErrorMessage,
},
])('should retry if expired credentials error %o', async parsedError => {
expect.assertions(3);
mockErrorParser.mockResolvedValue(parsedError);
mockIsCredentialsExpiredError.mockReturnValue(true);
const retryDecider = getRetryDecider(mockErrorParser);
const { retryable, isCredentialsExpiredError } = await retryDecider(
mockHttpResponse,
undefined,
{},
);
expect(retryable).toBe(true);
expect(isCredentialsExpiredError).toBe(true);
expect(mockIsCredentialsExpiredError).toHaveBeenCalledWith(
mockErrorName,
mockErrorMessage,
);
});

it('should not retry if invalid credentials error has been retried previously', async () => {
expect.assertions(2);
mockIsCredentialsExpiredError.mockReturnValue(true);
const retryDecider = getRetryDecider(mockErrorParser);
const { retryable, isCredentialsExpiredError } = await retryDecider(
mockHttpResponse,
undefined,
{ isCredentialsExpired: true },
);
expect(retryable).toBe(false);
expect(isCredentialsExpiredError).toBe(true);
});
});
});
25 changes: 0 additions & 25 deletions packages/core/__tests__/clients/middleware/retry/isCredentialsExpiredError.test.ts
View file

This file was deleted.

14 changes: 1 addition & 13 deletions packages/core/src/clients/middleware/retry/defaultRetryDecider.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,8 @@
// SPDX-License-Identifier: Apache-2.0

import { ErrorParser, HttpResponse } from '../../types';
import { MiddlewareContext } from '../../types/core';

import { isClockSkewError } from './isClockSkewError';
import { isCredentialsExpiredError } from './isCredentialsExpiredError';
import { RetryDeciderOutput } from './types';

/**
Expand All @@ -17,32 +15,22 @@ export const getRetryDecider =
async (
response?: HttpResponse,
error?: unknown,
middlewareContext?: MiddlewareContext,
): Promise<RetryDeciderOutput> => {
const parsedError =
(error as Error & { code: string }) ??
(await errorParser(response)) ??
undefined;
const errorCode = parsedError?.code || parsedError?.name;
const errorMessage = parsedError?.message;
const statusCode = response?.statusCode;

const isCredentialsExpired = isCredentialsExpiredError(
errorCode,
errorMessage,
);
const isRetryable =
isConnectionError(error) ||
isThrottlingError(statusCode, errorCode) ||
isClockSkewError(errorCode) ||
isServerSideError(statusCode, errorCode) ||
// If we know the previous retry attempt sets isCredentialsInvalid in the
// middleware context, we don't want to retry anymore.
(isCredentialsExpired && !middlewareContext?.isCredentialsExpired);
isServerSideError(statusCode, errorCode);

return {
retryable: isRetryable,
isCredentialsExpiredError: isCredentialsExpired,
};
};

Expand Down
34 changes: 0 additions & 34 deletions packages/core/src/clients/middleware/retry/isCredentialsExpiredError.ts
View file

This file was deleted.

1 change: 1 addition & 0 deletions packages/core/src/clients/types/index.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
export {
Middleware,
MiddlewareHandler,
MiddlewareContext,
Request,
Response,
TransferHandler,
Expand Down
2 changes: 0 additions & 2 deletions packages/storage/__tests__/providers/s3/apis/copy.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -384,7 +384,6 @@ describe('copy API', () => {
},
});
} catch (error: any) {
console.log(error);
expect(error).toBeInstanceOf(StorageError);
expect(error.name).toBe(
StorageValidationErrorCode.InvalidCopyOperationStorageBucket,
Expand All @@ -403,7 +402,6 @@ describe('copy API', () => {
},
});
} catch (error: any) {
console.log(error);
expect(error).toBeInstanceOf(StorageError);
expect(error.name).toBe(
StorageValidationErrorCode.InvalidCopyOperationStorageBucket,
Expand Down
103 changes: 103 additions & 0 deletions packages/storage/__tests__/providers/s3/utils/client/S3/utils/retryDecider.test.ts
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
// SPDX-License-Identifier: Apache-2.0
import {
HttpResponse,
getRetryDecider as getDefaultRetryDecider,
} from '@aws-amplify/core/internals/aws-client-utils';

import { retryDecider } from '../../../../../../../src/providers/s3/utils/client/utils';
import { parseXmlError } from '../../../../../../../src/providers/s3/utils/client/utils/parsePayload';

jest.mock(
'../../../../../../../src/providers/s3/utils/client/utils/parsePayload',
);
jest.mock('@aws-amplify/core/internals/aws-client-utils');

const mockErrorParser = jest.mocked(parseXmlError);

describe('retryDecider', () => {
const mockHttpResponse: HttpResponse = {
statusCode: 200,
headers: {},
body: 'body' as any,
};

beforeEach(() => {
jest.mocked(getDefaultRetryDecider).mockReturnValue(async () => {
return { retryable: false };
});
});

afterEach(() => {
jest.resetAllMocks();
});

it('should invoke the default retry decider', async () => {
expect.assertions(3);
const { retryable, isCredentialsExpiredError } = await retryDecider(
mockHttpResponse,
undefined,
{},
);
expect(getDefaultRetryDecider).toHaveBeenCalledWith(mockErrorParser);
expect(retryable).toBe(false);
expect(isCredentialsExpiredError).toBeFalsy();
});

describe('handling expired token errors', () => {
const mockErrorMessage = 'Token expired';
it.each(['RequestExpired', 'ExpiredTokenException', 'ExpiredToken'])(
'should retry if expired credentials error name %s',
async errorName => {
expect.assertions(2);
const parsedError = {
name: errorName,
message: mockErrorMessage,
$metadata: {},
};
mockErrorParser.mockResolvedValue(parsedError);
const { retryable, isCredentialsExpiredError } = await retryDecider(
{ ...mockHttpResponse, statusCode: 400 },
undefined,
{},
);
expect(retryable).toBe(true);
expect(isCredentialsExpiredError).toBe(true);
},
);

it('should retry if error message indicates invalid credentials', async () => {
expect.assertions(2);
const parsedError = {
name: 'InvalidSignature',
message: 'Auth token in request is expired.',
$metadata: {},
};
mockErrorParser.mockResolvedValue(parsedError);
const { retryable, isCredentialsExpiredError } = await retryDecider(
{ ...mockHttpResponse, statusCode: 400 },
undefined,
{},
);
expect(retryable).toBe(true);
expect(isCredentialsExpiredError).toBe(true);
});

it('should not retry if invalid credentials error has been retried previously', async () => {
expect.assertions(2);
const parsedError = {
name: 'RequestExpired',
message: mockErrorMessage,
$metadata: {},
};
mockErrorParser.mockResolvedValue(parsedError);
const { retryable, isCredentialsExpiredError } = await retryDecider(
{ ...mockHttpResponse, statusCode: 400 },
undefined,
{ isCredentialsExpired: true },
);
expect(retryable).toBe(false);
expect(isCredentialsExpiredError).toBe(true);
});
});
});
5 changes: 2 additions & 3 deletions packages/storage/src/providers/s3/utils/client/s3control/base.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,10 @@ import {
import {
EndpointResolverOptions,
getDnsSuffix,
getRetryDecider,
jitteredBackoff,
} from '@aws-amplify/core/internals/aws-client-utils';

import { parseXmlError } from '../utils';
import { retryDecider } from '../utils';

/**
* The service name used to sign requests if the API requires authentication.
Expand Down Expand Up @@ -64,7 +63,7 @@ const endpointResolver = (
export const defaultConfig = {
service: SERVICE_NAME,
endpointResolver,
retryDecider: getRetryDecider(parseXmlError),
retryDecider,
computeDelay: jitteredBackoff,
userAgentValue: getAmplifyUserAgent(),
uriEscapePath: false, // Required by S3. See https://github.com/aws/aws-sdk-js-v3/blob/9ba012dfa3a3429aa2db0f90b3b0b3a7a31f9bc3/packages/signature-v4/src/SignatureV4.ts#L76-L83
Expand Down
5 changes: 2 additions & 3 deletions packages/storage/src/providers/s3/utils/client/s3data/base.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,10 @@ import {
import {
EndpointResolverOptions,
getDnsSuffix,
getRetryDecider,
jitteredBackoff,
} from '@aws-amplify/core/internals/aws-client-utils';

import { parseXmlError } from '../utils';
import { retryDecider } from '../utils';

const DOMAIN_PATTERN = /^[a-z0-9][a-z0-9.-]{1,61}[a-z0-9]$/;
const IP_ADDRESS_PATTERN = /(\d+\.){3}\d+/;
Expand Down Expand Up @@ -106,7 +105,7 @@ export const isDnsCompatibleBucketName = (bucketName: string): boolean =>
export const defaultConfig = {
service: SERVICE_NAME,
endpointResolver,
retryDecider: getRetryDecider(parseXmlError),
retryDecider,
computeDelay: jitteredBackoff,
userAgentValue: getAmplifyUserAgent(),
useAccelerateEndpoint: false,
Expand Down
Loading

0 comments on commit e5a3226

Please sign in to comment.