From 61b6bd1c56574b2c459f01d2401572d31bd7869e Mon Sep 17 00:00:00 2001 From: ashika112 Date: Mon, 1 Jul 2024 16:13:52 -0700 Subject: [PATCH 01/13] update parseAmplifyOutput --- .../__tests__/parseAmplifyOutputs.test.ts | 42 +++++++++++++++++++ packages/core/src/parseAmplifyOutputs.ts | 24 ++++++++++- .../src/singleton/AmplifyOutputs/types.ts | 5 +++ packages/core/src/singleton/Storage/types.ts | 9 ++++ packages/core/src/singleton/types.ts | 2 + 5 files changed, 80 insertions(+), 2 deletions(-) diff --git a/packages/core/__tests__/parseAmplifyOutputs.test.ts b/packages/core/__tests__/parseAmplifyOutputs.test.ts index 7d15f7b1f52..144c9ee475f 100644 --- a/packages/core/__tests__/parseAmplifyOutputs.test.ts +++ b/packages/core/__tests__/parseAmplifyOutputs.test.ts @@ -229,6 +229,48 @@ describe('parseAmplifyOutputs tests', () => { }, }); }); + it('should parse storage multi bucket', () => { + const amplifyOutputs: AmplifyOutputs = { + version: '1', + storage: { + aws_region: 'us-west-2', + bucket_name: 'storage-bucket-test', + buckets: [ + { + name: 'default-bucket', + bucket_name: 'storage-bucket-test', + aws_region: 'us-west-2', + }, + { + name: 'bucket-2', + bucket_name: 'storage-bucket-test-2', + aws_region: 'us-west-2', + }, + ], + }, + }; + + const result = parseAmplifyOutputs(amplifyOutputs); + + expect(result).toEqual({ + Storage: { + S3: { + bucket: 'storage-bucket-test', + region: 'us-west-2', + buckets: { + 'bucket-2': { + bucketName: 'storage-bucket-test-2', + region: 'us-west-2', + }, + 'default-bucket': { + bucketName: 'storage-bucket-test', + region: 'us-west-2', + }, + }, + }, + }, + }); + }); }); describe('analytics tests', () => { diff --git a/packages/core/src/parseAmplifyOutputs.ts b/packages/core/src/parseAmplifyOutputs.ts index ed742189266..2298cb28d4c 100644 --- a/packages/core/src/parseAmplifyOutputs.ts +++ b/packages/core/src/parseAmplifyOutputs.ts @@ -4,7 +4,7 @@ /* This is because JSON schema contains keys with snake_case */ /* eslint-disable camelcase */ -/* Does not like exahaustive checks */ +/* Does not like exhaustive checks */ /* eslint-disable no-case-declarations */ import { @@ -30,6 +30,7 @@ import { import { AnalyticsConfig, AuthConfig, + BucketInfo, GeoConfig, LegacyConfig, ResourcesConfig, @@ -56,12 +57,13 @@ function parseStorage( return undefined; } - const { bucket_name, aws_region } = amplifyOutputsStorageProperties; + const { bucket_name, aws_region, buckets } = amplifyOutputsStorageProperties; return { S3: { bucket: bucket_name, region: aws_region, + buckets: buckets && mapNameToBucketInfo(buckets), }, }; } @@ -333,3 +335,21 @@ function getMfaStatus( return 'off'; } + +function mapNameToBucketInfo( + buckets: { + name: string; + bucket_name: string; + aws_region: string; + }[], +) { + const mappedBuckets: Record = {}; + buckets.forEach(({ name, bucket_name: bucketName, aws_region: region }) => { + mappedBuckets[name] = { + bucketName, + region, + }; + }); + + return mappedBuckets; +} diff --git a/packages/core/src/singleton/AmplifyOutputs/types.ts b/packages/core/src/singleton/AmplifyOutputs/types.ts index 9f03f49a7fb..344cb552642 100644 --- a/packages/core/src/singleton/AmplifyOutputs/types.ts +++ b/packages/core/src/singleton/AmplifyOutputs/types.ts @@ -46,6 +46,11 @@ export interface AmplifyOutputsAuthProperties { export interface AmplifyOutputsStorageProperties { aws_region: string; bucket_name: string; + buckets?: { + name: string; + bucket_name: string; + aws_region: string; + }[]; } export interface AmplifyOutputsGeoProperties { diff --git a/packages/core/src/singleton/Storage/types.ts b/packages/core/src/singleton/Storage/types.ts index b21413a797a..5bca120c9b3 100644 --- a/packages/core/src/singleton/Storage/types.ts +++ b/packages/core/src/singleton/Storage/types.ts @@ -6,6 +6,13 @@ import { AtLeastOne } from '../types'; /** @deprecated This may be removed in the next major version. */ export type StorageAccessLevel = 'guest' | 'protected' | 'private'; +/** Information on bucket used to store files/objects */ +export interface BucketInfo { + /** Actual bucket name */ + bucketName: string; + /** Region of the bucket */ + region: string; +} export interface S3ProviderConfig { S3: { bucket?: string; @@ -16,6 +23,8 @@ export interface S3ProviderConfig { * @internal */ dangerouslyConnectToHttpEndpointForTesting?: string; + /** Map of friendly name for bucket to its information */ + buckets?: Record; }; } diff --git a/packages/core/src/singleton/types.ts b/packages/core/src/singleton/types.ts index e2acbeb6611..3419be9a1ec 100644 --- a/packages/core/src/singleton/types.ts +++ b/packages/core/src/singleton/types.ts @@ -19,6 +19,7 @@ import { import { GeoConfig } from './Geo/types'; import { PredictionsConfig } from './Predictions/types'; import { + BucketInfo, LibraryStorageOptions, StorageAccessLevel, StorageConfig, @@ -77,6 +78,7 @@ export { PredictionsConfig, StorageAccessLevel, StorageConfig, + BucketInfo, AnalyticsConfig, CognitoIdentityPoolConfig, GeoConfig, From 26cea414a8955ec5bd5927947b297f44c7b27f2d Mon Sep 17 00:00:00 2001 From: ashika112 Date: Mon, 1 Jul 2024 16:52:28 -0700 Subject: [PATCH 02/13] update bundle size --- packages/aws-amplify/package.json | 50 +++++++++++++++--------------- packages/interactions/package.json | 6 ++-- 2 files changed, 28 insertions(+), 28 deletions(-) diff --git a/packages/aws-amplify/package.json b/packages/aws-amplify/package.json index 31564a92ec3..0f8d0da824c 100644 --- a/packages/aws-amplify/package.json +++ b/packages/aws-amplify/package.json @@ -293,31 +293,31 @@ "name": "[Analytics] record (Pinpoint)", "path": "./dist/esm/analytics/index.mjs", "import": "{ record }", - "limit": "17.09 kB" + "limit": "17.13 kB" }, { "name": "[Analytics] record (Kinesis)", "path": "./dist/esm/analytics/kinesis/index.mjs", "import": "{ record }", - "limit": "48.56 kB" + "limit": "48.57 kB" }, { "name": "[Analytics] record (Kinesis Firehose)", "path": "./dist/esm/analytics/kinesis-firehose/index.mjs", "import": "{ record }", - "limit": "45.68 kB" + "limit": "45.71 kB" }, { "name": "[Analytics] record (Personalize)", "path": "./dist/esm/analytics/personalize/index.mjs", "import": "{ record }", - "limit": "49.50 kB" + "limit": "49.53 kB" }, { "name": "[Analytics] identifyUser (Pinpoint)", "path": "./dist/esm/analytics/index.mjs", "import": "{ identifyUser }", - "limit": "15.59 kB" + "limit": "15.65 kB" }, { "name": "[Analytics] enable", @@ -353,13 +353,13 @@ "name": "[Auth] resetPassword (Cognito)", "path": "./dist/esm/auth/index.mjs", "import": "{ resetPassword }", - "limit": "12.44 kB" + "limit": "12.48 kB" }, { "name": "[Auth] confirmResetPassword (Cognito)", "path": "./dist/esm/auth/index.mjs", "import": "{ confirmResetPassword }", - "limit": "12.39 kB" + "limit": "12.42 kB" }, { "name": "[Auth] signIn (Cognito)", @@ -371,7 +371,7 @@ "name": "[Auth] resendSignUpCode (Cognito)", "path": "./dist/esm/auth/index.mjs", "import": "{ resendSignUpCode }", - "limit": "12.40 kB" + "limit": "12.44 kB" }, { "name": "[Auth] confirmSignUp (Cognito)", @@ -383,31 +383,31 @@ "name": "[Auth] confirmSignIn (Cognito)", "path": "./dist/esm/auth/index.mjs", "import": "{ confirmSignIn }", - "limit": "28.27 kB" + "limit": "28.32 kB" }, { "name": "[Auth] updateMFAPreference (Cognito)", "path": "./dist/esm/auth/index.mjs", "import": "{ updateMFAPreference }", - "limit": "11.74 kB" + "limit": "11.77 kB" }, { "name": "[Auth] fetchMFAPreference (Cognito)", "path": "./dist/esm/auth/index.mjs", "import": "{ fetchMFAPreference }", - "limit": "11.78 kB" + "limit": "11.83 kB" }, { "name": "[Auth] verifyTOTPSetup (Cognito)", "path": "./dist/esm/auth/index.mjs", "import": "{ verifyTOTPSetup }", - "limit": "12.6 kB" + "limit": "12.66 kB" }, { "name": "[Auth] updatePassword (Cognito)", "path": "./dist/esm/auth/index.mjs", "import": "{ updatePassword }", - "limit": "12.63 kB" + "limit": "12.67 kB" }, { "name": "[Auth] setUpTOTP (Cognito)", @@ -419,19 +419,19 @@ "name": "[Auth] updateUserAttributes (Cognito)", "path": "./dist/esm/auth/index.mjs", "import": "{ updateUserAttributes }", - "limit": "11.87 kB" + "limit": "11.90 kB" }, { "name": "[Auth] getCurrentUser (Cognito)", "path": "./dist/esm/auth/index.mjs", "import": "{ getCurrentUser }", - "limit": "7.75 kB" + "limit": "7.78 kB" }, { "name": "[Auth] confirmUserAttribute (Cognito)", "path": "./dist/esm/auth/index.mjs", "import": "{ confirmUserAttribute }", - "limit": "12.61 kB" + "limit": "12.65 kB" }, { "name": "[Auth] signInWithRedirect (Cognito)", @@ -443,55 +443,55 @@ "name": "[Auth] fetchUserAttributes (Cognito)", "path": "./dist/esm/auth/index.mjs", "import": "{ fetchUserAttributes }", - "limit": "11.69 kB" + "limit": "11.73 kB" }, { "name": "[Auth] Basic Auth Flow (Cognito)", "path": "./dist/esm/auth/index.mjs", "import": "{ signIn, signOut, fetchAuthSession, confirmSignIn }", - "limit": "30.06 kB" + "limit": "30.10 kB" }, { "name": "[Auth] OAuth Auth Flow (Cognito)", "path": "./dist/esm/auth/index.mjs", "import": "{ signInWithRedirect, signOut, fetchAuthSession }", - "limit": "21.47 kB" + "limit": "21.52 kB" }, { "name": "[Storage] copy (S3)", "path": "./dist/esm/storage/index.mjs", "import": "{ copy }", - "limit": "14.54 kB" + "limit": "14.57 kB" }, { "name": "[Storage] downloadData (S3)", "path": "./dist/esm/storage/index.mjs", "import": "{ downloadData }", - "limit": "15.17 kB" + "limit": "15.20 kB" }, { "name": "[Storage] getProperties (S3)", "path": "./dist/esm/storage/index.mjs", "import": "{ getProperties }", - "limit": "14.43 kB" + "limit": "14.45 kB" }, { "name": "[Storage] getUrl (S3)", "path": "./dist/esm/storage/index.mjs", "import": "{ getUrl }", - "limit": "15.51 kB" + "limit": "15.55 kB" }, { "name": "[Storage] list (S3)", "path": "./dist/esm/storage/index.mjs", "import": "{ list }", - "limit": "14.94 kB" + "limit": "14.98 kB" }, { "name": "[Storage] remove (S3)", "path": "./dist/esm/storage/index.mjs", "import": "{ remove }", - "limit": "14.29 kB" + "limit": "14.32 kB" }, { "name": "[Storage] uploadData (S3)", diff --git a/packages/interactions/package.json b/packages/interactions/package.json index f111dbb0fd3..08050a39280 100644 --- a/packages/interactions/package.json +++ b/packages/interactions/package.json @@ -89,19 +89,19 @@ "name": "Interactions (default to Lex v2)", "path": "./dist/esm/index.mjs", "import": "{ Interactions }", - "limit": "52.52 kB" + "limit": "52.56 kB" }, { "name": "Interactions (Lex v2)", "path": "./dist/esm/lex-v2/index.mjs", "import": "{ Interactions }", - "limit": "52.52 kB" + "limit": "52.56 kB" }, { "name": "Interactions (Lex v1)", "path": "./dist/esm/lex-v1/index.mjs", "import": "{ Interactions }", - "limit": "47.33 kB" + "limit": "47.36 kB" } ] } From 98ce4508f7a0f0f6089583a59289f21a977ed75b Mon Sep 17 00:00:00 2001 From: ashika112 Date: Tue, 2 Jul 2024 14:07:10 -0700 Subject: [PATCH 03/13] address comments --- packages/core/src/parseAmplifyOutputs.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/core/src/parseAmplifyOutputs.ts b/packages/core/src/parseAmplifyOutputs.ts index 2298cb28d4c..c0ee5f1f55d 100644 --- a/packages/core/src/parseAmplifyOutputs.ts +++ b/packages/core/src/parseAmplifyOutputs.ts @@ -63,7 +63,7 @@ function parseStorage( S3: { bucket: bucket_name, region: aws_region, - buckets: buckets && mapNameToBucketInfo(buckets), + buckets: buckets && mapBucketInfoToName(buckets), }, }; } @@ -336,13 +336,13 @@ function getMfaStatus( return 'off'; } -function mapNameToBucketInfo( +function mapBucketInfoToName( buckets: { name: string; bucket_name: string; aws_region: string; }[], -) { +): Record { const mappedBuckets: Record = {}; buckets.forEach(({ name, bucket_name: bucketName, aws_region: region }) => { mappedBuckets[name] = { From 469e7b591b361a7c5493226ef1cd7e64a8136f93 Mon Sep 17 00:00:00 2001 From: ashika112 Date: Tue, 2 Jul 2024 14:34:03 -0700 Subject: [PATCH 04/13] update options and test --- .../s3/apis/uploadData/putObjectJob.test.ts | 39 +++++++++++++++++++ .../storage/src/providers/s3/types/options.ts | 7 ++++ 2 files changed, 46 insertions(+) diff --git a/packages/storage/__tests__/providers/s3/apis/uploadData/putObjectJob.test.ts b/packages/storage/__tests__/providers/s3/apis/uploadData/putObjectJob.test.ts index 335e804c0ea..d580f1cf557 100644 --- a/packages/storage/__tests__/providers/s3/apis/uploadData/putObjectJob.test.ts +++ b/packages/storage/__tests__/providers/s3/apis/uploadData/putObjectJob.test.ts @@ -233,4 +233,43 @@ describe('putObjectJob with path', () => { await job(); expect(calculateContentMd5).toHaveBeenCalledWith('data'); }); + + it('should override bucket in putObject call when bucket is passed in option', async () => { + const abortController = new AbortController(); + const data = 'data'; + const bucketName = 'bucket-1'; + const region = 'region-1'; + + const job = putObjectJob( + { + path: 'path/', + data, + options: { + bucket: { + bucketName, + region, + }, + }, + }, + new AbortController().signal, + ); + await job(); + + await expect(mockPutObject).toBeLastCalledWithConfigAndInput( + { + credentials, + region, + abortSignal: abortController.signal, + onUploadProgress: expect.any(Function), + useAccelerateEndpoint: true, + userAgentValue: expect.any(String), + }, + { + Bucket: bucketName, + Key: 'path/', + Body: data, + ContentMD5: undefined, + }, + ); + }); }); diff --git a/packages/storage/src/providers/s3/types/options.ts b/packages/storage/src/providers/s3/types/options.ts index b2b7dfd0ddc..1dacf46781f 100644 --- a/packages/storage/src/providers/s3/types/options.ts +++ b/packages/storage/src/providers/s3/types/options.ts @@ -10,12 +10,19 @@ import { StorageListPaginateOptions, } from '../../../types/options'; +interface BucketInfo { + bucketName: string; + region: string; +} + +type StorageBucket = string | BucketInfo; interface CommonOptions { /** * Whether to use accelerate endpoint. * @default false */ useAccelerateEndpoint?: boolean; + bucket?: StorageBucket; } /** @deprecated This may be removed in the next major version. */ From 6259cdc4c234d02a6d0efe43e3e04c832dc057ef Mon Sep 17 00:00:00 2001 From: ashika112 Date: Tue, 2 Jul 2024 16:07:13 -0700 Subject: [PATCH 05/13] update assertion error & download test --- jest.setup.js | 2 +- .../providers/s3/apis/downloadData.test.ts | 34 +++++++++++++++++++ .../s3/apis/uploadData/putObjectJob.test.ts | 4 +-- .../storage/src/errors/types/validation.ts | 4 +++ .../storage/src/providers/s3/types/options.ts | 4 +-- .../s3/utils/resolveS3ConfigAndInput.ts | 27 +++++++++++++-- 6 files changed, 67 insertions(+), 8 deletions(-) diff --git a/jest.setup.js b/jest.setup.js index 05fbee97db1..bce7630e105 100644 --- a/jest.setup.js +++ b/jest.setup.js @@ -2,7 +2,7 @@ // Comment out log level as necessary (e.g. while debugging tests) global.console = { ...console, - log: jest.fn(), + // log: jest.fn(), debug: jest.fn(), info: jest.fn(), warn: jest.fn(), diff --git a/packages/storage/__tests__/providers/s3/apis/downloadData.test.ts b/packages/storage/__tests__/providers/s3/apis/downloadData.test.ts index 57d402b1f24..14e74e77439 100644 --- a/packages/storage/__tests__/providers/s3/apis/downloadData.test.ts +++ b/packages/storage/__tests__/providers/s3/apis/downloadData.test.ts @@ -24,6 +24,7 @@ import { ItemWithPath, } from '../../../../src/providers/s3/types/outputs'; import './testUtils'; +import { BucketInfo } from '../../../../src/providers/s3/types/options'; jest.mock('../../../../src/providers/s3/utils/client'); jest.mock('../../../../src/providers/s3/utils'); @@ -366,4 +367,37 @@ describe('downloadData with path', () => { }), ); }); + + it('should override bucket in putObject call when bucket is passed in option', async () => { + (getObject as jest.Mock).mockResolvedValueOnce({ Body: 'body' }); + const abortController = new AbortController(); + const bucketInfo: BucketInfo = { + bucketName: 'bucket-1', + region: 'region-1', + }; + + downloadData({ + path: inputPath, + options: { + bucket: bucketInfo, + }, + }); + + const { job } = mockCreateDownloadTask.mock.calls[0][0]; + await job(); + + expect(getObject).toHaveBeenCalledTimes(1); + await expect(getObject).toBeLastCalledWithConfigAndInput( + { + credentials, + region: bucketInfo.region, + abortSignal: abortController.signal, + userAgentValue: expect.any(String), + }, + { + Bucket: bucketInfo.bucketName, + Key: inputPath, + }, + ); + }); }); diff --git a/packages/storage/__tests__/providers/s3/apis/uploadData/putObjectJob.test.ts b/packages/storage/__tests__/providers/s3/apis/uploadData/putObjectJob.test.ts index d580f1cf557..828c0bc0cdd 100644 --- a/packages/storage/__tests__/providers/s3/apis/uploadData/putObjectJob.test.ts +++ b/packages/storage/__tests__/providers/s3/apis/uploadData/putObjectJob.test.ts @@ -260,15 +260,13 @@ describe('putObjectJob with path', () => { credentials, region, abortSignal: abortController.signal, - onUploadProgress: expect.any(Function), - useAccelerateEndpoint: true, userAgentValue: expect.any(String), }, { Bucket: bucketName, Key: 'path/', Body: data, - ContentMD5: undefined, + ContentType: 'application/octet-stream', }, ); }); diff --git a/packages/storage/src/errors/types/validation.ts b/packages/storage/src/errors/types/validation.ts index d72b9852162..129752d38ba 100644 --- a/packages/storage/src/errors/types/validation.ts +++ b/packages/storage/src/errors/types/validation.ts @@ -13,6 +13,7 @@ export enum StorageValidationErrorCode { NoDestinationPath = 'NoDestinationPath', NoBucket = 'NoBucket', NoRegion = 'NoRegion', + InvalidStorageBucket = 'InvalidStorageBucket', InvalidStorageOperationPrefixInput = 'InvalidStorageOperationPrefixInput', InvalidStorageOperationInput = 'InvalidStorageOperationInput', InvalidStoragePathInput = 'InvalidStoragePathInput', @@ -70,4 +71,7 @@ export const validationErrorMap: AmplifyErrorMap = { [StorageValidationErrorCode.InvalidStoragePathInput]: { message: 'Input `path` does not allow a leading slash (/).', }, + [StorageValidationErrorCode.InvalidStorageBucket]: { + message: 'Unable to find bucket from name in Amplify Configure', + }, }; diff --git a/packages/storage/src/providers/s3/types/options.ts b/packages/storage/src/providers/s3/types/options.ts index 1dacf46781f..0642d943ced 100644 --- a/packages/storage/src/providers/s3/types/options.ts +++ b/packages/storage/src/providers/s3/types/options.ts @@ -10,12 +10,12 @@ import { StorageListPaginateOptions, } from '../../../types/options'; -interface BucketInfo { +export interface BucketInfo { bucketName: string; region: string; } -type StorageBucket = string | BucketInfo; +export type StorageBucket = string | BucketInfo; interface CommonOptions { /** * Whether to use accelerate endpoint. diff --git a/packages/storage/src/providers/s3/utils/resolveS3ConfigAndInput.ts b/packages/storage/src/providers/s3/utils/resolveS3ConfigAndInput.ts index ae7a185c93c..ecaafa45d76 100644 --- a/packages/storage/src/providers/s3/utils/resolveS3ConfigAndInput.ts +++ b/packages/storage/src/providers/s3/utils/resolveS3ConfigAndInput.ts @@ -6,7 +6,7 @@ import { AmplifyClassV6, StorageAccessLevel } from '@aws-amplify/core'; import { assertValidationError } from '../../../errors/utils/assertValidationError'; import { StorageValidationErrorCode } from '../../../errors/types/validation'; import { resolvePrefix as defaultPrefixResolver } from '../../../utils/resolvePrefix'; -import { ResolvedS3Config } from '../types/options'; +import { ResolvedS3Config, StorageBucket } from '../types/options'; import { DEFAULT_ACCESS_LEVEL, LOCAL_TESTING_S3_ENDPOINT } from './constants'; @@ -14,6 +14,7 @@ interface S3ApiOptions { accessLevel?: StorageAccessLevel; targetIdentityId?: string; useAccelerateEndpoint?: boolean; + bucket?: StorageBucket; } interface ResolvedS3ConfigAndInput { @@ -62,8 +63,30 @@ export const resolveS3ConfigAndInput = async ( return credentials; }; - const { bucket, region, dangerouslyConnectToHttpEndpointForTesting } = + let { bucket, region, dangerouslyConnectToHttpEndpointForTesting, buckets } = amplify.getConfig()?.Storage?.S3 ?? {}; + + if (apiOptions?.bucket) { + if (typeof apiOptions.bucket === 'string') { + const name = apiOptions.bucket; + if (buckets && buckets[name]) { + bucket = buckets[name].bucketName; + // eslint-disable-next-line @typescript-eslint/prefer-destructuring + region = buckets[name].region; + } else { + assertValidationError( + !!(buckets && buckets[name]), + StorageValidationErrorCode.InvalidStorageBucket, + ); + } + } + if (typeof apiOptions.bucket === 'object') { + bucket = apiOptions.bucket.bucketName; + // eslint-disable-next-line @typescript-eslint/prefer-destructuring + region = apiOptions.bucket.region; + } + } + assertValidationError(!!bucket, StorageValidationErrorCode.NoBucket); assertValidationError(!!region, StorageValidationErrorCode.NoRegion); From 569a1ecdbdbfb86899e3ae87621434f77c289957 Mon Sep 17 00:00:00 2001 From: ashika112 Date: Tue, 2 Jul 2024 16:32:58 -0700 Subject: [PATCH 06/13] update getUrl & getProp test --- .../providers/s3/apis/downloadData.test.ts | 2 +- .../providers/s3/apis/getProperties.test.ts | 29 +++++++++++++++++++ .../providers/s3/apis/getUrl.test.ts | 28 ++++++++++++++++++ 3 files changed, 58 insertions(+), 1 deletion(-) diff --git a/packages/storage/__tests__/providers/s3/apis/downloadData.test.ts b/packages/storage/__tests__/providers/s3/apis/downloadData.test.ts index 14e74e77439..711189fc974 100644 --- a/packages/storage/__tests__/providers/s3/apis/downloadData.test.ts +++ b/packages/storage/__tests__/providers/s3/apis/downloadData.test.ts @@ -368,7 +368,7 @@ describe('downloadData with path', () => { ); }); - it('should override bucket in putObject call when bucket is passed in option', async () => { + it('should override bucket in getObject call when bucket is passed in option', async () => { (getObject as jest.Mock).mockResolvedValueOnce({ Body: 'body' }); const abortController = new AbortController(); const bucketInfo: BucketInfo = { diff --git a/packages/storage/__tests__/providers/s3/apis/getProperties.test.ts b/packages/storage/__tests__/providers/s3/apis/getProperties.test.ts index bb5a5b957a7..dddb9a802eb 100644 --- a/packages/storage/__tests__/providers/s3/apis/getProperties.test.ts +++ b/packages/storage/__tests__/providers/s3/apis/getProperties.test.ts @@ -13,6 +13,7 @@ import { GetPropertiesWithPathOutput, } from '../../../../src/providers/s3/types'; import './testUtils'; +import { BucketInfo } from '../../../../src/providers/s3/types/options'; jest.mock('../../../../src/providers/s3/utils/client'); jest.mock('@aws-amplify/core', () => ({ @@ -275,6 +276,34 @@ describe('Happy cases: With path', () => { ); }, ); + it('should override bucket in headObject call when bucket is passed in option', async () => { + const bucketInfo: BucketInfo = { + bucketName: 'bucket-1', + region: 'region-1', + }; + const headObjectOptions = { + Bucket: bucketInfo.bucketName, + Key: inputPath, + }; + + await getPropertiesWrapper({ + path: inputPath, + options: { + bucket: bucketInfo, + useAccelerateEndpoint: true, + }, + }); + expect(headObject).toHaveBeenCalledTimes(1); + await expect(headObject).toBeLastCalledWithConfigAndInput( + { + credentials, + region: bucketInfo.region, + useAccelerateEndpoint: true, + userAgentValue: expect.any(String), + }, + headObjectOptions, + ); + }); }); describe('Error cases : With path', () => { diff --git a/packages/storage/__tests__/providers/s3/apis/getUrl.test.ts b/packages/storage/__tests__/providers/s3/apis/getUrl.test.ts index 994f4a0b648..72baecc5c1e 100644 --- a/packages/storage/__tests__/providers/s3/apis/getUrl.test.ts +++ b/packages/storage/__tests__/providers/s3/apis/getUrl.test.ts @@ -16,6 +16,7 @@ import { GetUrlWithPathOutput, } from '../../../../src/providers/s3/types'; import './testUtils'; +import { BucketInfo } from '../../../../src/providers/s3/types/options'; jest.mock('../../../../src/providers/s3/utils/client'); jest.mock('@aws-amplify/core', () => ({ @@ -235,7 +236,34 @@ describe('getUrl test with path', () => { }); }, ); + + it('should override bucket in getPresignedGetObjectUrl call when bucket is passed in option', async () => { + const inputPath = 'path/'; + const bucketInfo: BucketInfo = { + bucketName: 'bucket-1', + region: 'region-1', + }; + await getUrlWrapper({ + path: inputPath, + options: { + bucket: bucketInfo, + }, + }); + expect(getPresignedGetObjectUrl).toHaveBeenCalledTimes(1); + await expect(getPresignedGetObjectUrl).toBeLastCalledWithConfigAndInput( + { + credentials, + region: bucketInfo.region, + expiration: expect.any(Number), + }, + { + Bucket: bucketInfo.bucketName, + Key: inputPath, + }, + ); + }); }); + describe('Error cases : With path', () => { afterAll(() => { jest.clearAllMocks(); From 385e3f5f3f11a93b9296c4de57dafb8fef38fb25 Mon Sep 17 00:00:00 2001 From: ashika112 Date: Mon, 8 Jul 2024 13:23:53 -0700 Subject: [PATCH 07/13] update remove and list tests --- .../__tests__/providers/s3/apis/list.test.ts | 33 +++++++++++++++++++ .../providers/s3/apis/remove.test.ts | 23 +++++++++++++ 2 files changed, 56 insertions(+) diff --git a/packages/storage/__tests__/providers/s3/apis/list.test.ts b/packages/storage/__tests__/providers/s3/apis/list.test.ts index 9629129d7a2..0766eff322c 100644 --- a/packages/storage/__tests__/providers/s3/apis/list.test.ts +++ b/packages/storage/__tests__/providers/s3/apis/list.test.ts @@ -482,6 +482,39 @@ describe('list API', () => { ); }, ); + + it('should override bucket in listObject call when bucket is passed in option', async () => { + mockListObject.mockImplementationOnce(() => { + return { + Contents: [ + { + ...listObjectClientBaseResultItem, + Key: 'path/', + }, + ], + NextContinuationToken: nextToken, + }; + }); + const mockBucketName = 'bucket-1'; + const mockRegion = 'region-1'; + await listPaginatedWrapper({ + path: 'path/', + options: { bucket: { bucketName: mockBucketName, region: mockRegion } }, + }); + expect(listObjectsV2).toHaveBeenCalledTimes(1); + await expect(listObjectsV2).toBeLastCalledWithConfigAndInput( + { + credentials, + region: mockRegion, + userAgentValue: expect.any(String), + }, + { + Bucket: mockBucketName, + MaxKeys: 1000, + Prefix: 'path/', + }, + ); + }); }); describe('Error Cases:', () => { diff --git a/packages/storage/__tests__/providers/s3/apis/remove.test.ts b/packages/storage/__tests__/providers/s3/apis/remove.test.ts index ca1107f0912..4dfd1b03a88 100644 --- a/packages/storage/__tests__/providers/s3/apis/remove.test.ts +++ b/packages/storage/__tests__/providers/s3/apis/remove.test.ts @@ -157,6 +157,29 @@ describe('remove API', () => { ); }); }); + + it('should override bucket in deleteObject call when bucket is passed in option', async () => { + const mockBucketName = 'bucket-1'; + const mockRegion = 'region-1'; + await removeWrapper({ + path: 'path/', + options: { + bucket: { bucketName: mockBucketName, region: mockRegion }, + }, + }); + expect(deleteObject).toHaveBeenCalledTimes(1); + await expect(deleteObject).toBeLastCalledWithConfigAndInput( + { + credentials, + region: mockRegion, + userAgentValue: expect.any(String), + }, + { + Bucket: mockBucketName, + Key: 'path/', + }, + ); + }); }); }); From b0b53e3554fa0f1de689fc9b46412fc877bfeba1 Mon Sep 17 00:00:00 2001 From: ashika112 Date: Thu, 11 Jul 2024 15:41:17 -0700 Subject: [PATCH 08/13] update multi-part tests --- .../src/singleton/AmplifyOutputs/types.ts | 6 + .../apis/uploadData/multipartHandlers.test.ts | 127 ++++++++++++ .../s3/apis/uploadData/putObjectJob.test.ts | 180 ++++++++++++++---- 3 files changed, 276 insertions(+), 37 deletions(-) diff --git a/packages/core/src/singleton/AmplifyOutputs/types.ts b/packages/core/src/singleton/AmplifyOutputs/types.ts index 4d0dfebdada..90602c65df2 100644 --- a/packages/core/src/singleton/AmplifyOutputs/types.ts +++ b/packages/core/src/singleton/AmplifyOutputs/types.ts @@ -44,13 +44,19 @@ export interface AmplifyOutputsAuthProperties { } export interface AmplifyOutputsStorageBucketProperties { + /** Friendly bucket name provided in Amplify Outputs */ name: string; + /** Actual s3 bucket name given */ bucket_name: string; + /** Region for the bucket */ aws_region: string; } export interface AmplifyOutputsStorageProperties { + /** Default region for Storage */ aws_region: string; + /** Default bucket for Storage */ bucket_name: string; + /** List of buckets for Storage */ buckets?: AmplifyOutputsStorageBucketProperties[]; } diff --git a/packages/storage/__tests__/providers/s3/apis/uploadData/multipartHandlers.test.ts b/packages/storage/__tests__/providers/s3/apis/uploadData/multipartHandlers.test.ts index c40e5c83de6..f7515c04cf2 100644 --- a/packages/storage/__tests__/providers/s3/apis/uploadData/multipartHandlers.test.ts +++ b/packages/storage/__tests__/providers/s3/apis/uploadData/multipartHandlers.test.ts @@ -143,6 +143,7 @@ describe('getMultipartUploadHandlers with key', () => { S3: { bucket, region, + buckets: { 'default-bucket': { bucketName: bucket, region } }, }, }, }); @@ -347,6 +348,69 @@ describe('getMultipartUploadHandlers with key', () => { expect(mockUploadPart).toHaveBeenCalledTimes(2); expect(mockCompleteMultipartUpload).not.toHaveBeenCalled(); }); + + describe('bucket passed in options', () => { + const mockData = 'Ü'.repeat(4 * MB); + it('should override bucket in putObject call when bucket as object', async () => { + const mockBucket = 'bucket-1'; + const mockRegion = 'region-1'; + mockMultipartUploadSuccess(); + const { multipartUploadJob } = getMultipartUploadHandlers({ + key: 'key', + data: mockData, + options: { + bucket: { bucketName: mockBucket, region: mockRegion }, + }, + }); + await multipartUploadJob(); + await expect( + mockCreateMultipartUpload, + ).toBeLastCalledWithConfigAndInput( + expect.objectContaining({ + credentials, + region: mockRegion, + abortSignal: expect.any(AbortSignal), + }), + expect.objectContaining({ + Bucket: mockBucket, + Key: 'public/key', + ContentType: defaultContentType, + }), + ); + expect(mockCreateMultipartUpload).toHaveBeenCalledTimes(1); + expect(mockUploadPart).toHaveBeenCalledTimes(2); + expect(mockCompleteMultipartUpload).toHaveBeenCalledTimes(1); + }); + + it('should override bucket in putObject call when bucket as string', async () => { + mockMultipartUploadSuccess(); + const { multipartUploadJob } = getMultipartUploadHandlers({ + key: 'key', + data: mockData, + options: { + bucket: 'default-bucket', + }, + }); + await multipartUploadJob(); + await expect( + mockCreateMultipartUpload, + ).toBeLastCalledWithConfigAndInput( + expect.objectContaining({ + credentials, + region, + abortSignal: expect.any(AbortSignal), + }), + expect.objectContaining({ + Bucket: bucket, + Key: 'public/key', + ContentType: defaultContentType, + }), + ); + expect(mockCreateMultipartUpload).toHaveBeenCalledTimes(1); + expect(mockUploadPart).toHaveBeenCalledTimes(2); + expect(mockCompleteMultipartUpload).toHaveBeenCalledTimes(1); + }); + }); }); describe('upload caching', () => { @@ -665,6 +729,7 @@ describe('getMultipartUploadHandlers with path', () => { S3: { bucket, region, + buckets: { 'default-bucket': { bucketName: bucket, region } }, }, }, }); @@ -861,6 +926,68 @@ describe('getMultipartUploadHandlers with path', () => { expect(mockUploadPart).toHaveBeenCalledTimes(2); expect(mockCompleteMultipartUpload).not.toHaveBeenCalled(); }); + + describe('bucket passed in options', () => { + const mockData = 'Ü'.repeat(4 * MB); + it('should override bucket in putObject call when bucket as object', async () => { + const mockBucket = 'bucket-1'; + const mockRegion = 'region-1'; + mockMultipartUploadSuccess(); + const { multipartUploadJob } = getMultipartUploadHandlers({ + path: 'path/', + data: mockData, + options: { + bucket: { bucketName: mockBucket, region: mockRegion }, + }, + }); + await multipartUploadJob(); + await expect( + mockCreateMultipartUpload, + ).toBeLastCalledWithConfigAndInput( + expect.objectContaining({ + credentials, + region: mockRegion, + abortSignal: expect.any(AbortSignal), + }), + expect.objectContaining({ + Bucket: mockBucket, + Key: 'path/', + ContentType: defaultContentType, + }), + ); + expect(mockCreateMultipartUpload).toHaveBeenCalledTimes(1); + expect(mockUploadPart).toHaveBeenCalledTimes(2); + expect(mockCompleteMultipartUpload).toHaveBeenCalledTimes(1); + }); + it('should override bucket in putObject call when bucket as string', async () => { + mockMultipartUploadSuccess(); + const { multipartUploadJob } = getMultipartUploadHandlers({ + path: 'path/', + data: mockData, + options: { + bucket: 'default-bucket', + }, + }); + await multipartUploadJob(); + await expect( + mockCreateMultipartUpload, + ).toBeLastCalledWithConfigAndInput( + expect.objectContaining({ + credentials, + region, + abortSignal: expect.any(AbortSignal), + }), + expect.objectContaining({ + Bucket: bucket, + Key: 'path/', + ContentType: defaultContentType, + }), + ); + expect(mockCreateMultipartUpload).toHaveBeenCalledTimes(1); + expect(mockUploadPart).toHaveBeenCalledTimes(2); + expect(mockCompleteMultipartUpload).toHaveBeenCalledTimes(1); + }); + }); }); describe('upload caching', () => { diff --git a/packages/storage/__tests__/providers/s3/apis/uploadData/putObjectJob.test.ts b/packages/storage/__tests__/providers/s3/apis/uploadData/putObjectJob.test.ts index 828c0bc0cdd..aa9cf2ff8cd 100644 --- a/packages/storage/__tests__/providers/s3/apis/uploadData/putObjectJob.test.ts +++ b/packages/storage/__tests__/providers/s3/apis/uploadData/putObjectJob.test.ts @@ -38,6 +38,8 @@ const credentials: AWSCredentials = { const identityId = 'identityId'; const mockFetchAuthSession = jest.mocked(Amplify.Auth.fetchAuthSession); const mockPutObject = jest.mocked(putObject); +const bucket = 'bucket'; +const region = 'region'; mockFetchAuthSession.mockResolvedValue({ credentials, @@ -46,8 +48,9 @@ mockFetchAuthSession.mockResolvedValue({ jest.mocked(Amplify.getConfig).mockReturnValue({ Storage: { S3: { - bucket: 'bucket', - region: 'region', + bucket, + region, + buckets: { 'default-bucket': { bucketName: bucket, region } }, }, }, }); @@ -102,14 +105,14 @@ describe('putObjectJob with key', () => { await expect(mockPutObject).toBeLastCalledWithConfigAndInput( { credentials, - region: 'region', + region, abortSignal: abortController.signal, onUploadProgress: expect.any(Function), useAccelerateEndpoint: true, userAgentValue: expect.any(String), }, { - Bucket: 'bucket', + Bucket: bucket, Key: `public/${inputKey}`, Body: data, ContentType: mockContentType, @@ -139,6 +142,76 @@ describe('putObjectJob with key', () => { await job(); expect(calculateContentMd5).toHaveBeenCalledWith('data'); }); + + describe('bucket passed in options', () => { + it('should override bucket in putObject call when bucket as object', async () => { + const abortController = new AbortController(); + const data = 'data'; + const bucketName = 'bucket-1'; + const mockRegion = 'region-1'; + + const job = putObjectJob( + { + key: 'key', + data, + options: { + bucket: { + bucketName, + region: mockRegion, + }, + }, + }, + new AbortController().signal, + ); + await job(); + + await expect(mockPutObject).toBeLastCalledWithConfigAndInput( + { + credentials, + region: mockRegion, + abortSignal: abortController.signal, + userAgentValue: expect.any(String), + }, + { + Bucket: bucketName, + Key: 'public/key', + Body: data, + ContentType: 'application/octet-stream', + }, + ); + }); + + it('should override bucket in putObject call when bucket as string', async () => { + const abortController = new AbortController(); + const data = 'data'; + const job = putObjectJob( + { + key: 'key', + data, + options: { + bucket: 'default-bucket', + }, + }, + new AbortController().signal, + ); + await job(); + + await expect(mockPutObject).toBeLastCalledWithConfigAndInput( + { + credentials, + region, + abortSignal: abortController.signal, + userAgentValue: expect.any(String), + }, + { + Bucket: bucket, + Key: 'public/key', + Body: data, + ContentType: 'application/octet-stream', + }, + ); + }); + }); }); describe('putObjectJob with path', () => { @@ -195,14 +268,14 @@ describe('putObjectJob with path', () => { await expect(mockPutObject).toBeLastCalledWithConfigAndInput( { credentials, - region: 'region', + region, abortSignal: abortController.signal, onUploadProgress: expect.any(Function), useAccelerateEndpoint: true, userAgentValue: expect.any(String), }, { - Bucket: 'bucket', + Bucket: bucket, Key: expectedKey, Body: data, ContentType: mockContentType, @@ -234,40 +307,73 @@ describe('putObjectJob with path', () => { expect(calculateContentMd5).toHaveBeenCalledWith('data'); }); - it('should override bucket in putObject call when bucket is passed in option', async () => { - const abortController = new AbortController(); - const data = 'data'; - const bucketName = 'bucket-1'; - const region = 'region-1'; + describe('bucket passed in options', () => { + it('should override bucket in putObject call when bucket as object', async () => { + const abortController = new AbortController(); + const data = 'data'; + const bucketName = 'bucket-1'; + const mockRegion = 'region-1'; - const job = putObjectJob( - { - path: 'path/', - data, - options: { - bucket: { - bucketName, - region, + const job = putObjectJob( + { + path: 'path/', + data, + options: { + bucket: { + bucketName, + region: mockRegion, + }, }, }, - }, - new AbortController().signal, - ); - await job(); + new AbortController().signal, + ); + await job(); - await expect(mockPutObject).toBeLastCalledWithConfigAndInput( - { - credentials, - region, - abortSignal: abortController.signal, - userAgentValue: expect.any(String), - }, - { - Bucket: bucketName, - Key: 'path/', - Body: data, - ContentType: 'application/octet-stream', - }, - ); + await expect(mockPutObject).toBeLastCalledWithConfigAndInput( + { + credentials, + region: mockRegion, + abortSignal: abortController.signal, + userAgentValue: expect.any(String), + }, + { + Bucket: bucketName, + Key: 'path/', + Body: data, + ContentType: 'application/octet-stream', + }, + ); + }); + + it('should override bucket in putObject call when bucket as string', async () => { + const abortController = new AbortController(); + const data = 'data'; + const job = putObjectJob( + { + path: 'path/', + data, + options: { + bucket: 'default-bucket', + }, + }, + new AbortController().signal, + ); + await job(); + + await expect(mockPutObject).toBeLastCalledWithConfigAndInput( + { + credentials, + region, + abortSignal: abortController.signal, + userAgentValue: expect.any(String), + }, + { + Bucket: bucket, + Key: 'path/', + Body: data, + ContentType: 'application/octet-stream', + }, + ); + }); }); }); From 773e90b41f482d3e8cb8562bc683f91eb3151c58 Mon Sep 17 00:00:00 2001 From: ashika112 Date: Thu, 11 Jul 2024 15:56:49 -0700 Subject: [PATCH 09/13] update list & download test --- .../providers/s3/apis/downloadData.test.ts | 153 +++++++++++++--- .../__tests__/providers/s3/apis/list.test.ts | 168 ++++++++++++++---- 2 files changed, 263 insertions(+), 58 deletions(-) diff --git a/packages/storage/__tests__/providers/s3/apis/downloadData.test.ts b/packages/storage/__tests__/providers/s3/apis/downloadData.test.ts index 711189fc974..35b790366bc 100644 --- a/packages/storage/__tests__/providers/s3/apis/downloadData.test.ts +++ b/packages/storage/__tests__/providers/s3/apis/downloadData.test.ts @@ -63,7 +63,7 @@ const mockDownloadResultBase = { const mockFetchAuthSession = Amplify.Auth.fetchAuthSession as jest.Mock; const mockCreateDownloadTask = createDownloadTask as jest.Mock; const mockValidateStorageInput = validateStorageOperationInput as jest.Mock; -const mockGetConfig = Amplify.getConfig as jest.Mock; +const mockGetConfig = jest.mocked(Amplify.getConfig); describe('downloadData with key', () => { beforeAll(() => { @@ -76,6 +76,7 @@ describe('downloadData with key', () => { S3: { bucket, region, + buckets: { 'default-bucket': { bucketName: bucket, region } }, }, }, }); @@ -221,6 +222,70 @@ describe('downloadData with key', () => { }), ); }); + + describe('bucket passed in options', () => { + it('should override bucket in getObject call when bucket is object', async () => { + (getObject as jest.Mock).mockResolvedValueOnce({ Body: 'body' }); + const abortController = new AbortController(); + const bucketInfo: BucketInfo = { + bucketName: 'bucket-1', + region: 'region-1', + }; + + downloadData({ + key: inputKey, + options: { + bucket: bucketInfo, + }, + }); + + const { job } = mockCreateDownloadTask.mock.calls[0][0]; + await job(); + + expect(getObject).toHaveBeenCalledTimes(1); + await expect(getObject).toBeLastCalledWithConfigAndInput( + { + credentials, + region: bucketInfo.region, + abortSignal: abortController.signal, + userAgentValue: expect.any(String), + }, + { + Bucket: bucketInfo.bucketName, + Key: `public/${inputKey}`, + }, + ); + }); + + it('should override bucket in getObject call when bucket is string', async () => { + (getObject as jest.Mock).mockResolvedValueOnce({ Body: 'body' }); + const abortController = new AbortController(); + + downloadData({ + key: inputKey, + options: { + bucket: 'default-bucket', + }, + }); + + const { job } = mockCreateDownloadTask.mock.calls[0][0]; + await job(); + + expect(getObject).toHaveBeenCalledTimes(1); + await expect(getObject).toBeLastCalledWithConfigAndInput( + { + credentials, + region, + abortSignal: abortController.signal, + userAgentValue: expect.any(String), + }, + { + Bucket: bucket, + Key: `public/${inputKey}`, + }, + ); + }); + }); }); describe('downloadData with path', () => { @@ -234,6 +299,7 @@ describe('downloadData with path', () => { S3: { bucket, region, + buckets: { 'default-bucket': { bucketName: bucket, region } }, }, }, }); @@ -368,36 +434,67 @@ describe('downloadData with path', () => { ); }); - it('should override bucket in getObject call when bucket is passed in option', async () => { - (getObject as jest.Mock).mockResolvedValueOnce({ Body: 'body' }); - const abortController = new AbortController(); - const bucketInfo: BucketInfo = { - bucketName: 'bucket-1', - region: 'region-1', - }; + describe('bucket passed in options', () => { + it('should override bucket in getObject call when bucket is object', async () => { + (getObject as jest.Mock).mockResolvedValueOnce({ Body: 'body' }); + const abortController = new AbortController(); + const bucketInfo: BucketInfo = { + bucketName: 'bucket-1', + region: 'region-1', + }; - downloadData({ - path: inputPath, - options: { - bucket: bucketInfo, - }, + downloadData({ + path: inputPath, + options: { + bucket: bucketInfo, + }, + }); + + const { job } = mockCreateDownloadTask.mock.calls[0][0]; + await job(); + + expect(getObject).toHaveBeenCalledTimes(1); + await expect(getObject).toBeLastCalledWithConfigAndInput( + { + credentials, + region: bucketInfo.region, + abortSignal: abortController.signal, + userAgentValue: expect.any(String), + }, + { + Bucket: bucketInfo.bucketName, + Key: inputPath, + }, + ); }); - const { job } = mockCreateDownloadTask.mock.calls[0][0]; - await job(); + it('should override bucket in getObject call when bucket is string', async () => { + (getObject as jest.Mock).mockResolvedValueOnce({ Body: 'body' }); + const abortController = new AbortController(); - expect(getObject).toHaveBeenCalledTimes(1); - await expect(getObject).toBeLastCalledWithConfigAndInput( - { - credentials, - region: bucketInfo.region, - abortSignal: abortController.signal, - userAgentValue: expect.any(String), - }, - { - Bucket: bucketInfo.bucketName, - Key: inputPath, - }, - ); + downloadData({ + path: inputPath, + options: { + bucket: 'default-bucket', + }, + }); + + const { job } = mockCreateDownloadTask.mock.calls[0][0]; + await job(); + + expect(getObject).toHaveBeenCalledTimes(1); + await expect(getObject).toBeLastCalledWithConfigAndInput( + { + credentials, + region, + abortSignal: abortController.signal, + userAgentValue: expect.any(String), + }, + { + Bucket: bucket, + Key: inputPath, + }, + ); + }); }); }); diff --git a/packages/storage/__tests__/providers/s3/apis/list.test.ts b/packages/storage/__tests__/providers/s3/apis/list.test.ts index 0766eff322c..20ccc32b366 100644 --- a/packages/storage/__tests__/providers/s3/apis/list.test.ts +++ b/packages/storage/__tests__/providers/s3/apis/list.test.ts @@ -31,7 +31,7 @@ jest.mock('@aws-amplify/core', () => ({ }, })); const mockFetchAuthSession = Amplify.Auth.fetchAuthSession as jest.Mock; -const mockGetConfig = Amplify.getConfig as jest.Mock; +const mockGetConfig = jest.mocked(Amplify.getConfig); const mockListObject = listObjectsV2 as jest.Mock; const inputKey = 'path/itemsKey'; const bucket = 'bucket'; @@ -93,6 +93,7 @@ describe('list API', () => { S3: { bucket, region, + buckets: { 'default-bucket': { bucketName: bucket, region } }, }, }, }); @@ -304,6 +305,76 @@ describe('list API', () => { }); }, ); + + describe('bucket passed in options', () => { + it('should override bucket in listObject call when bucket is object', async () => { + mockListObject.mockImplementationOnce(() => { + return { + Contents: [ + { + ...listObjectClientBaseResultItem, + Key: inputKey, + }, + ], + NextContinuationToken: nextToken, + }; + }); + const mockBucketName = 'bucket-1'; + const mockRegion = 'region-1'; + await listPaginatedWrapper({ + prefix: inputKey, + options: { + bucket: { bucketName: mockBucketName, region: mockRegion }, + }, + }); + expect(listObjectsV2).toHaveBeenCalledTimes(1); + await expect(listObjectsV2).toBeLastCalledWithConfigAndInput( + { + credentials, + region: mockRegion, + userAgentValue: expect.any(String), + }, + { + Bucket: mockBucketName, + MaxKeys: 1000, + Prefix: `public/${inputKey}`, + }, + ); + }); + + it('should override bucket in listObject call when bucket is string', async () => { + mockListObject.mockImplementationOnce(() => { + return { + Contents: [ + { + ...listObjectClientBaseResultItem, + Key: inputKey, + }, + ], + NextContinuationToken: nextToken, + }; + }); + await listPaginatedWrapper({ + prefix: inputKey, + options: { + bucket: 'default-bucket', + }, + }); + expect(listObjectsV2).toHaveBeenCalledTimes(1); + await expect(listObjectsV2).toBeLastCalledWithConfigAndInput( + { + credentials, + region, + userAgentValue: expect.any(String), + }, + { + Bucket: bucket, + MaxKeys: 1000, + Prefix: `public/${inputKey}`, + }, + ); + }); + }); }); describe('Path: Happy Cases:', () => { @@ -483,37 +554,74 @@ describe('list API', () => { }, ); - it('should override bucket in listObject call when bucket is passed in option', async () => { - mockListObject.mockImplementationOnce(() => { - return { - Contents: [ - { - ...listObjectClientBaseResultItem, - Key: 'path/', - }, - ], - NextContinuationToken: nextToken, - }; + describe('bucket passed in options', () => { + it('should override bucket in listObject call when bucket is object', async () => { + mockListObject.mockImplementationOnce(() => { + return { + Contents: [ + { + ...listObjectClientBaseResultItem, + Key: 'path/', + }, + ], + NextContinuationToken: nextToken, + }; + }); + const mockBucketName = 'bucket-1'; + const mockRegion = 'region-1'; + await listPaginatedWrapper({ + path: 'path/', + options: { + bucket: { bucketName: mockBucketName, region: mockRegion }, + }, + }); + expect(listObjectsV2).toHaveBeenCalledTimes(1); + await expect(listObjectsV2).toBeLastCalledWithConfigAndInput( + { + credentials, + region: mockRegion, + userAgentValue: expect.any(String), + }, + { + Bucket: mockBucketName, + MaxKeys: 1000, + Prefix: 'path/', + }, + ); }); - const mockBucketName = 'bucket-1'; - const mockRegion = 'region-1'; - await listPaginatedWrapper({ - path: 'path/', - options: { bucket: { bucketName: mockBucketName, region: mockRegion } }, + + it('should override bucket in listObject call when bucket is string', async () => { + mockListObject.mockImplementationOnce(() => { + return { + Contents: [ + { + ...listObjectClientBaseResultItem, + Key: 'path/', + }, + ], + NextContinuationToken: nextToken, + }; + }); + await listPaginatedWrapper({ + path: 'path/', + options: { + bucket: 'default-bucket', + }, + }); + expect(listObjectsV2).toHaveBeenCalledTimes(1); + await expect(listObjectsV2).toBeLastCalledWithConfigAndInput( + { + credentials, + region, + userAgentValue: expect.any(String), + }, + { + Bucket: bucket, + MaxKeys: 1000, + Prefix: 'path/', + }, + ); }); - expect(listObjectsV2).toHaveBeenCalledTimes(1); - await expect(listObjectsV2).toBeLastCalledWithConfigAndInput( - { - credentials, - region: mockRegion, - userAgentValue: expect.any(String), - }, - { - Bucket: mockBucketName, - MaxKeys: 1000, - Prefix: 'path/', - }, - ); }); }); From 6a26594a4c9e5162aa2a3db7ba6af4e9c11b3734 Mon Sep 17 00:00:00 2001 From: ashika112 Date: Fri, 12 Jul 2024 10:51:36 -0700 Subject: [PATCH 10/13] update more storage tests --- .../providers/s3/apis/getProperties.test.ts | 128 ++++++++++++++---- .../providers/s3/apis/getUrl.test.ts | 117 ++++++++++++---- .../providers/s3/apis/remove.test.ts | 110 ++++++++++++--- 3 files changed, 284 insertions(+), 71 deletions(-) diff --git a/packages/storage/__tests__/providers/s3/apis/getProperties.test.ts b/packages/storage/__tests__/providers/s3/apis/getProperties.test.ts index dddb9a802eb..0fcd989453e 100644 --- a/packages/storage/__tests__/providers/s3/apis/getProperties.test.ts +++ b/packages/storage/__tests__/providers/s3/apis/getProperties.test.ts @@ -29,7 +29,7 @@ jest.mock('@aws-amplify/core', () => ({ })); const mockHeadObject = headObject as jest.MockedFunction; const mockFetchAuthSession = Amplify.Auth.fetchAuthSession as jest.Mock; -const mockGetConfig = Amplify.getConfig as jest.Mock; +const mockGetConfig = jest.mocked(Amplify.getConfig); const bucket = 'bucket'; const region = 'region'; @@ -66,14 +66,16 @@ describe('getProperties with key', () => { S3: { bucket, region, + buckets: { 'default-bucket': { bucketName: bucket, region } }, }, }, }); }); + describe('Happy cases: With key', () => { const config = { credentials, - region: 'region', + region, userAgentValue: expect.any(String), }; beforeEach(() => { @@ -153,6 +155,56 @@ describe('getProperties with key', () => { ); }, ); + + describe('bucket passed in options', () => { + it('should override bucket in headObject call when bucket is object', async () => { + const bucketInfo: BucketInfo = { + bucketName: 'bucket-1', + region: 'region-1', + }; + const headObjectOptions = { + Bucket: bucketInfo.bucketName, + Key: `public/${inputKey}`, + }; + + await getPropertiesWrapper({ + key: inputKey, + options: { + bucket: bucketInfo, + }, + }); + expect(headObject).toHaveBeenCalledTimes(1); + await expect(headObject).toBeLastCalledWithConfigAndInput( + { + credentials, + region: bucketInfo.region, + + userAgentValue: expect.any(String), + }, + headObjectOptions, + ); + }); + it('should override bucket in headObject call when bucket is string', async () => { + await getPropertiesWrapper({ + key: inputKey, + options: { + bucket: 'default-bucket', + }, + }); + expect(headObject).toHaveBeenCalledTimes(1); + await expect(headObject).toBeLastCalledWithConfigAndInput( + { + credentials, + region, + userAgentValue: expect.any(String), + }, + { + Bucket: bucket, + Key: `public/${inputKey}`, + }, + ); + }); + }); }); describe('Error cases : With key', () => { @@ -202,6 +254,7 @@ describe('Happy cases: With path', () => { S3: { bucket, region, + buckets: { 'default-bucket': { bucketName: bucket, region } }, }, }, }); @@ -276,33 +329,54 @@ describe('Happy cases: With path', () => { ); }, ); - it('should override bucket in headObject call when bucket is passed in option', async () => { - const bucketInfo: BucketInfo = { - bucketName: 'bucket-1', - region: 'region-1', - }; - const headObjectOptions = { - Bucket: bucketInfo.bucketName, - Key: inputPath, - }; + describe('bucket passed in options', () => { + it('should override bucket in headObject call when bucket is object', async () => { + const bucketInfo: BucketInfo = { + bucketName: 'bucket-1', + region: 'region-1', + }; + const headObjectOptions = { + Bucket: bucketInfo.bucketName, + Key: inputPath, + }; - await getPropertiesWrapper({ - path: inputPath, - options: { - bucket: bucketInfo, - useAccelerateEndpoint: true, - }, + await getPropertiesWrapper({ + path: inputPath, + options: { + bucket: bucketInfo, + }, + }); + expect(headObject).toHaveBeenCalledTimes(1); + await expect(headObject).toBeLastCalledWithConfigAndInput( + { + credentials, + region: bucketInfo.region, + + userAgentValue: expect.any(String), + }, + headObjectOptions, + ); + }); + it('should override bucket in headObject call when bucket is string', async () => { + await getPropertiesWrapper({ + path: inputPath, + options: { + bucket: 'default-bucket', + }, + }); + expect(headObject).toHaveBeenCalledTimes(1); + await expect(headObject).toBeLastCalledWithConfigAndInput( + { + credentials, + region, + userAgentValue: expect.any(String), + }, + { + Bucket: bucket, + Key: inputPath, + }, + ); }); - expect(headObject).toHaveBeenCalledTimes(1); - await expect(headObject).toBeLastCalledWithConfigAndInput( - { - credentials, - region: bucketInfo.region, - useAccelerateEndpoint: true, - userAgentValue: expect.any(String), - }, - headObjectOptions, - ); }); }); diff --git a/packages/storage/__tests__/providers/s3/apis/getUrl.test.ts b/packages/storage/__tests__/providers/s3/apis/getUrl.test.ts index 72baecc5c1e..47136f139d1 100644 --- a/packages/storage/__tests__/providers/s3/apis/getUrl.test.ts +++ b/packages/storage/__tests__/providers/s3/apis/getUrl.test.ts @@ -57,6 +57,7 @@ describe('getUrl test with key', () => { S3: { bucket, region, + buckets: { 'default-bucket': { bucketName: bucket, region } }, }, }, }); @@ -136,6 +137,52 @@ describe('getUrl test with key', () => { expect({ url, expiresAt }).toEqual(expectedResult); }, ); + describe('bucket passed in options', () => { + it('should override bucket in getPresignedGetObjectUrl call when bucket is object', async () => { + const bucketInfo: BucketInfo = { + bucketName: 'bucket-1', + region: 'region-1', + }; + await getUrlWrapper({ + key: 'key', + options: { + bucket: bucketInfo, + }, + }); + expect(getPresignedGetObjectUrl).toHaveBeenCalledTimes(1); + await expect(getPresignedGetObjectUrl).toBeLastCalledWithConfigAndInput( + { + credentials, + region: bucketInfo.region, + expiration: expect.any(Number), + }, + { + Bucket: bucketInfo.bucketName, + Key: 'public/key', + }, + ); + }); + it('should override bucket in getPresignedGetObjectUrl call when bucket is string', async () => { + await getUrlWrapper({ + key: 'key', + options: { + bucket: 'default-bucket', + }, + }); + expect(getPresignedGetObjectUrl).toHaveBeenCalledTimes(1); + await expect(getPresignedGetObjectUrl).toBeLastCalledWithConfigAndInput( + { + credentials, + region, + expiration: expect.any(Number), + }, + { + Bucket: bucket, + Key: 'public/key', + }, + ); + }); + }); }); describe('Error cases : With key', () => { afterAll(() => { @@ -176,6 +223,7 @@ describe('getUrl test with path', () => { S3: { bucket, region, + buckets: { 'default-bucket': { bucketName: bucket, region } }, }, }, }); @@ -237,30 +285,53 @@ describe('getUrl test with path', () => { }, ); - it('should override bucket in getPresignedGetObjectUrl call when bucket is passed in option', async () => { - const inputPath = 'path/'; - const bucketInfo: BucketInfo = { - bucketName: 'bucket-1', - region: 'region-1', - }; - await getUrlWrapper({ - path: inputPath, - options: { - bucket: bucketInfo, - }, + describe('bucket passed in options', () => { + it('should override bucket in getPresignedGetObjectUrl call when bucket is object', async () => { + const inputPath = 'path/'; + const bucketInfo: BucketInfo = { + bucketName: 'bucket-1', + region: 'region-1', + }; + await getUrlWrapper({ + path: inputPath, + options: { + bucket: bucketInfo, + }, + }); + expect(getPresignedGetObjectUrl).toHaveBeenCalledTimes(1); + await expect(getPresignedGetObjectUrl).toBeLastCalledWithConfigAndInput( + { + credentials, + region: bucketInfo.region, + expiration: expect.any(Number), + }, + { + Bucket: bucketInfo.bucketName, + Key: inputPath, + }, + ); + }); + it('should override bucket in getPresignedGetObjectUrl call when bucket is string', async () => { + const inputPath = 'path/'; + await getUrlWrapper({ + path: inputPath, + options: { + bucket: 'default-bucket', + }, + }); + expect(getPresignedGetObjectUrl).toHaveBeenCalledTimes(1); + await expect(getPresignedGetObjectUrl).toBeLastCalledWithConfigAndInput( + { + credentials, + region, + expiration: expect.any(Number), + }, + { + Bucket: bucket, + Key: inputPath, + }, + ); }); - expect(getPresignedGetObjectUrl).toHaveBeenCalledTimes(1); - await expect(getPresignedGetObjectUrl).toBeLastCalledWithConfigAndInput( - { - credentials, - region: bucketInfo.region, - expiration: expect.any(Number), - }, - { - Bucket: bucketInfo.bucketName, - Key: inputPath, - }, - ); }); }); diff --git a/packages/storage/__tests__/providers/s3/apis/remove.test.ts b/packages/storage/__tests__/providers/s3/apis/remove.test.ts index 4dfd1b03a88..eb3407eb610 100644 --- a/packages/storage/__tests__/providers/s3/apis/remove.test.ts +++ b/packages/storage/__tests__/providers/s3/apis/remove.test.ts @@ -29,7 +29,7 @@ jest.mock('@aws-amplify/core', () => ({ })); const mockDeleteObject = deleteObject as jest.Mock; const mockFetchAuthSession = Amplify.Auth.fetchAuthSession as jest.Mock; -const mockGetConfig = Amplify.getConfig as jest.Mock; +const mockGetConfig = jest.mocked(Amplify.getConfig); const inputKey = 'key'; const bucket = 'bucket'; const region = 'region'; @@ -56,6 +56,7 @@ describe('remove API', () => { S3: { bucket, region, + buckets: { 'default-bucket': { bucketName: bucket, region } }, }, }, }); @@ -115,6 +116,51 @@ describe('remove API', () => { ); }); }); + + describe('bucket passed in options', () => { + it('should override bucket in deleteObject call when bucket is object', async () => { + const mockBucketName = 'bucket-1'; + const mockRegion = 'region-1'; + await removeWrapper({ + key: inputKey, + options: { + bucket: { bucketName: mockBucketName, region: mockRegion }, + }, + }); + expect(deleteObject).toHaveBeenCalledTimes(1); + await expect(deleteObject).toBeLastCalledWithConfigAndInput( + { + credentials, + region: mockRegion, + userAgentValue: expect.any(String), + }, + { + Bucket: mockBucketName, + Key: `public/${inputKey}`, + }, + ); + }); + it('should override bucket in deleteObject call when bucket is string', async () => { + await removeWrapper({ + key: inputKey, + options: { + bucket: 'default-bucket', + }, + }); + expect(deleteObject).toHaveBeenCalledTimes(1); + await expect(deleteObject).toBeLastCalledWithConfigAndInput( + { + credentials, + region, + userAgentValue: expect.any(String), + }, + { + Bucket: bucket, + Key: `public/${inputKey}`, + }, + ); + }); + }); }); describe('With Path', () => { const removeWrapper = ( @@ -158,27 +204,49 @@ describe('remove API', () => { }); }); - it('should override bucket in deleteObject call when bucket is passed in option', async () => { - const mockBucketName = 'bucket-1'; - const mockRegion = 'region-1'; - await removeWrapper({ - path: 'path/', - options: { - bucket: { bucketName: mockBucketName, region: mockRegion }, - }, + describe('bucket passed in options', () => { + it('should override bucket in deleteObject call when bucket is object', async () => { + const mockBucketName = 'bucket-1'; + const mockRegion = 'region-1'; + await removeWrapper({ + path: 'path/', + options: { + bucket: { bucketName: mockBucketName, region: mockRegion }, + }, + }); + expect(deleteObject).toHaveBeenCalledTimes(1); + await expect(deleteObject).toBeLastCalledWithConfigAndInput( + { + credentials, + region: mockRegion, + userAgentValue: expect.any(String), + }, + { + Bucket: mockBucketName, + Key: 'path/', + }, + ); + }); + it('should override bucket in deleteObject call when bucket is string', async () => { + await removeWrapper({ + path: 'path/', + options: { + bucket: 'default-bucket', + }, + }); + expect(deleteObject).toHaveBeenCalledTimes(1); + await expect(deleteObject).toBeLastCalledWithConfigAndInput( + { + credentials, + region, + userAgentValue: expect.any(String), + }, + { + Bucket: bucket, + Key: 'path/', + }, + ); }); - expect(deleteObject).toHaveBeenCalledTimes(1); - await expect(deleteObject).toBeLastCalledWithConfigAndInput( - { - credentials, - region: mockRegion, - userAgentValue: expect.any(String), - }, - { - Bucket: mockBucketName, - Key: 'path/', - }, - ); }); }); }); From 0445ab3e0c635b014e21de8a06f96ce2f9cc6ddb Mon Sep 17 00:00:00 2001 From: ashika112 Date: Mon, 15 Jul 2024 09:50:30 -0700 Subject: [PATCH 11/13] refactor resolveBucketConfig --- .../storage/src/errors/types/validation.ts | 3 +- .../s3/utils/resolveS3ConfigAndInput.ts | 65 ++++++++++--------- 2 files changed, 38 insertions(+), 30 deletions(-) diff --git a/packages/storage/src/errors/types/validation.ts b/packages/storage/src/errors/types/validation.ts index 129752d38ba..d8de32f6c95 100644 --- a/packages/storage/src/errors/types/validation.ts +++ b/packages/storage/src/errors/types/validation.ts @@ -72,6 +72,7 @@ export const validationErrorMap: AmplifyErrorMap = { message: 'Input `path` does not allow a leading slash (/).', }, [StorageValidationErrorCode.InvalidStorageBucket]: { - message: 'Unable to find bucket from name in Amplify Configure', + message: + 'Unable to lookup bucket from provided name in Amplify configuration', }, }; diff --git a/packages/storage/src/providers/s3/utils/resolveS3ConfigAndInput.ts b/packages/storage/src/providers/s3/utils/resolveS3ConfigAndInput.ts index ecaafa45d76..52a60919cec 100644 --- a/packages/storage/src/providers/s3/utils/resolveS3ConfigAndInput.ts +++ b/packages/storage/src/providers/s3/utils/resolveS3ConfigAndInput.ts @@ -6,7 +6,7 @@ import { AmplifyClassV6, StorageAccessLevel } from '@aws-amplify/core'; import { assertValidationError } from '../../../errors/utils/assertValidationError'; import { StorageValidationErrorCode } from '../../../errors/types/validation'; import { resolvePrefix as defaultPrefixResolver } from '../../../utils/resolvePrefix'; -import { ResolvedS3Config, StorageBucket } from '../types/options'; +import { BucketInfo, ResolvedS3Config, StorageBucket } from '../types/options'; import { DEFAULT_ACCESS_LEVEL, LOCAL_TESTING_S3_ENDPOINT } from './constants'; @@ -66,25 +66,11 @@ export const resolveS3ConfigAndInput = async ( let { bucket, region, dangerouslyConnectToHttpEndpointForTesting, buckets } = amplify.getConfig()?.Storage?.S3 ?? {}; - if (apiOptions?.bucket) { - if (typeof apiOptions.bucket === 'string') { - const name = apiOptions.bucket; - if (buckets && buckets[name]) { - bucket = buckets[name].bucketName; - // eslint-disable-next-line @typescript-eslint/prefer-destructuring - region = buckets[name].region; - } else { - assertValidationError( - !!(buckets && buckets[name]), - StorageValidationErrorCode.InvalidStorageBucket, - ); - } - } - if (typeof apiOptions.bucket === 'object') { - bucket = apiOptions.bucket.bucketName; - // eslint-disable-next-line @typescript-eslint/prefer-destructuring - region = apiOptions.bucket.region; - } + const resolvedBucketConfig = + apiOptions?.bucket && resolveBucketConfig(apiOptions, buckets); + + if (resolvedBucketConfig) { + ({ bucket, region } = resolvedBucketConfig); } assertValidationError(!!bucket, StorageValidationErrorCode.NoBucket); @@ -96,15 +82,14 @@ export const resolveS3ConfigAndInput = async ( isObjectLockEnabled, } = amplify.libraryOptions?.Storage?.S3 ?? {}; - const keyPrefix = await prefixResolver({ - accessLevel: - apiOptions?.accessLevel ?? defaultAccessLevel ?? DEFAULT_ACCESS_LEVEL, - // use conditional assign to make tsc happy because StorageOptions is a union type that may not have targetIdentityId - targetIdentityId: - apiOptions?.accessLevel === 'protected' - ? apiOptions?.targetIdentityId ?? identityId - : identityId, - }); + const accessLevel = + apiOptions?.accessLevel ?? defaultAccessLevel ?? DEFAULT_ACCESS_LEVEL; + const targetIdentityId = + accessLevel === 'protected' + ? apiOptions?.targetIdentityId ?? identityId + : identityId; + + const keyPrefix = await prefixResolver({ accessLevel, targetIdentityId }); return { s3Config: { @@ -124,3 +109,25 @@ export const resolveS3ConfigAndInput = async ( isObjectLockEnabled, }; }; + +const resolveBucketConfig = ( + apiOptions: S3ApiOptions, + buckets: Record | undefined, +): { bucket: string; region: string } | undefined => { + if (typeof apiOptions.bucket === 'string') { + const bucketConfig = buckets?.[apiOptions.bucket]; + assertValidationError( + !!bucketConfig, + StorageValidationErrorCode.InvalidStorageBucket, + ); + + return { bucket: bucketConfig.bucketName, region: bucketConfig.region }; + } + + if (typeof apiOptions.bucket === 'object') { + return { + bucket: apiOptions.bucket.bucketName, + region: apiOptions.bucket.region, + }; + } +}; From bf31d7472fa8a5de24cf949d75e3cba8833b0012 Mon Sep 17 00:00:00 2001 From: ashika112 Date: Mon, 15 Jul 2024 10:19:56 -0700 Subject: [PATCH 12/13] update bundle size --- jest.setup.js | 2 +- packages/aws-amplify/package.json | 14 +++++++------- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/jest.setup.js b/jest.setup.js index bce7630e105..05fbee97db1 100644 --- a/jest.setup.js +++ b/jest.setup.js @@ -2,7 +2,7 @@ // Comment out log level as necessary (e.g. while debugging tests) global.console = { ...console, - // log: jest.fn(), + log: jest.fn(), debug: jest.fn(), info: jest.fn(), warn: jest.fn(), diff --git a/packages/aws-amplify/package.json b/packages/aws-amplify/package.json index d0e9d94f1d2..6a464af7441 100644 --- a/packages/aws-amplify/package.json +++ b/packages/aws-amplify/package.json @@ -461,43 +461,43 @@ "name": "[Storage] copy (S3)", "path": "./dist/esm/storage/index.mjs", "import": "{ copy }", - "limit": "14.64 kB" + "limit": "14.76 kB" }, { "name": "[Storage] downloadData (S3)", "path": "./dist/esm/storage/index.mjs", "import": "{ downloadData }", - "limit": "15.25 kB" + "limit": "15.38 kB" }, { "name": "[Storage] getProperties (S3)", "path": "./dist/esm/storage/index.mjs", "import": "{ getProperties }", - "limit": "14.51 kB" + "limit": "14.64 kB" }, { "name": "[Storage] getUrl (S3)", "path": "./dist/esm/storage/index.mjs", "import": "{ getUrl }", - "limit": "15.60 kB" + "limit": "15.74 kB" }, { "name": "[Storage] list (S3)", "path": "./dist/esm/storage/index.mjs", "import": "{ list }", - "limit": "15.02 kB" + "limit": "15.15 kB" }, { "name": "[Storage] remove (S3)", "path": "./dist/esm/storage/index.mjs", "import": "{ remove }", - "limit": "14.37 kB" + "limit": "14.50 kB" }, { "name": "[Storage] uploadData (S3)", "path": "./dist/esm/storage/index.mjs", "import": "{ uploadData }", - "limit": "19.68 kB" + "limit": "19.79 kB" } ] } From 38fb333d2fe7fc6d1cd5958ef768e1537ff993af Mon Sep 17 00:00:00 2001 From: ashika112 Date: Tue, 16 Jul 2024 16:34:32 -0700 Subject: [PATCH 13/13] address comments --- .../src/singleton/AmplifyOutputs/types.ts | 2 +- .../utils/resolveS3ConfigAndInput.test.ts | 36 +++++++++++++++++-- .../storage/src/errors/types/validation.ts | 2 +- .../s3/utils/resolveS3ConfigAndInput.ts | 18 +++++----- 4 files changed, 45 insertions(+), 13 deletions(-) diff --git a/packages/core/src/singleton/AmplifyOutputs/types.ts b/packages/core/src/singleton/AmplifyOutputs/types.ts index 90602c65df2..2968955158f 100644 --- a/packages/core/src/singleton/AmplifyOutputs/types.ts +++ b/packages/core/src/singleton/AmplifyOutputs/types.ts @@ -46,7 +46,7 @@ export interface AmplifyOutputsAuthProperties { export interface AmplifyOutputsStorageBucketProperties { /** Friendly bucket name provided in Amplify Outputs */ name: string; - /** Actual s3 bucket name given */ + /** Actual S3 bucket name given */ bucket_name: string; /** Region for the bucket */ aws_region: string; diff --git a/packages/storage/__tests__/providers/s3/apis/utils/resolveS3ConfigAndInput.test.ts b/packages/storage/__tests__/providers/s3/apis/utils/resolveS3ConfigAndInput.test.ts index e26cb63b6c7..022c2f0c1fb 100644 --- a/packages/storage/__tests__/providers/s3/apis/utils/resolveS3ConfigAndInput.test.ts +++ b/packages/storage/__tests__/providers/s3/apis/utils/resolveS3ConfigAndInput.test.ts @@ -9,6 +9,8 @@ import { StorageValidationErrorCode, validationErrorMap, } from '../../../../../src/errors/types/validation'; +import { BucketInfo } from '../../../../../src/providers/s3/types/options'; +import { StorageError } from '../../../../../src/errors/StorageError'; jest.mock('@aws-amplify/core', () => ({ ConsoleLogger: jest.fn(), @@ -21,7 +23,7 @@ jest.mock('@aws-amplify/core', () => ({ })); jest.mock('../../../../../src/utils/resolvePrefix'); -const mockGetConfig = Amplify.getConfig as jest.Mock; +const mockGetConfig = jest.mocked(Amplify.getConfig); const mockDefaultResolvePrefix = resolvePrefix as jest.Mock; const mockFetchAuthSession = Amplify.Auth.fetchAuthSession as jest.Mock; @@ -49,6 +51,7 @@ describe('resolveS3ConfigAndInput', () => { S3: { bucket, region, + buckets: { 'bucket-1': { bucketName: bucket, region } }, }, }, }); @@ -132,7 +135,7 @@ describe('resolveS3ConfigAndInput', () => { S3: { bucket, region, - dangerouslyConnectToHttpEndpointForTesting: true, + dangerouslyConnectToHttpEndpointForTesting: 'true', }, }, }); @@ -214,4 +217,33 @@ describe('resolveS3ConfigAndInput', () => { }); expect(keyPrefix).toEqual('prefix'); }); + + it('should resolve bucket and region with overrides when bucket API option is passed', async () => { + const bucketInfo: BucketInfo = { + bucketName: 'bucket-2', + region: 'region-2', + }; + + const { + bucket: resolvedBucket, + s3Config: { region: resolvedRegion }, + } = await resolveS3ConfigAndInput(Amplify, { + bucket: bucketInfo, + }); + + expect(mockGetConfig).toHaveBeenCalled(); + expect(resolvedBucket).toEqual(bucketInfo.bucketName); + expect(resolvedRegion).toEqual(bucketInfo.region); + }); + + it('should throw when unable to lookup bucket from the config when bucket API option is passed', async () => { + try { + await resolveS3ConfigAndInput(Amplify, { + bucket: 'error-bucket', + }); + } catch (error: any) { + expect(error).toBeInstanceOf(StorageError); + expect(error.name).toBe(StorageValidationErrorCode.InvalidStorageBucket); + } + }); }); diff --git a/packages/storage/src/errors/types/validation.ts b/packages/storage/src/errors/types/validation.ts index d8de32f6c95..b84443ad78f 100644 --- a/packages/storage/src/errors/types/validation.ts +++ b/packages/storage/src/errors/types/validation.ts @@ -73,6 +73,6 @@ export const validationErrorMap: AmplifyErrorMap = { }, [StorageValidationErrorCode.InvalidStorageBucket]: { message: - 'Unable to lookup bucket from provided name in Amplify configuration', + 'Unable to lookup bucket from provided name in Amplify configuration.', }, }; diff --git a/packages/storage/src/providers/s3/utils/resolveS3ConfigAndInput.ts b/packages/storage/src/providers/s3/utils/resolveS3ConfigAndInput.ts index 52a60919cec..928d1876251 100644 --- a/packages/storage/src/providers/s3/utils/resolveS3ConfigAndInput.ts +++ b/packages/storage/src/providers/s3/utils/resolveS3ConfigAndInput.ts @@ -63,15 +63,15 @@ export const resolveS3ConfigAndInput = async ( return credentials; }; - let { bucket, region, dangerouslyConnectToHttpEndpointForTesting, buckets } = - amplify.getConfig()?.Storage?.S3 ?? {}; - - const resolvedBucketConfig = - apiOptions?.bucket && resolveBucketConfig(apiOptions, buckets); - - if (resolvedBucketConfig) { - ({ bucket, region } = resolvedBucketConfig); - } + const { + bucket: defaultBucket, + region: defaultRegion, + dangerouslyConnectToHttpEndpointForTesting, + buckets, + } = amplify.getConfig()?.Storage?.S3 ?? {}; + + const { bucket = defaultBucket, region = defaultRegion } = + (apiOptions?.bucket && resolveBucketConfig(apiOptions, buckets)) || {}; assertValidationError(!!bucket, StorageValidationErrorCode.NoBucket); assertValidationError(!!region, StorageValidationErrorCode.NoRegion);