Skip to content

[Multi-bucket] Update options to include bucket #13574

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 14 commits into from
Jul 18, 2024
Merged

[Multi-bucket] Update options to include bucket #13574

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
61b6bd1
update parseAmplifyOutput
ashika112 Jul 1, 2024
26cea41
update bundle size
ashika112 Jul 1, 2024
98ce450
address comments
ashika112 Jul 2, 2024
469e7b5
update options and test
ashika112 Jul 2, 2024
6259cdc
update assertion error & download test
ashika112 Jul 2, 2024
569a1ec
update getUrl & getProp test
ashika112 Jul 2, 2024
385e3f5
update remove and list tests
ashika112 Jul 8, 2024
0cbeda0
Merge branch 'feat/multi-bucket' of github.com:aws-amplify/amplify-js…
ashika112 Jul 11, 2024
b0b53e3
update multi-part tests
ashika112 Jul 11, 2024
773e90b
update list & download test
ashika112 Jul 11, 2024
6a26594
update more storage tests
ashika112 Jul 12, 2024
0445ab3
refactor resolveBucketConfig
ashika112 Jul 15, 2024
bf31d74
update bundle size
ashika112 Jul 15, 2024
38fb333
address comments
ashika112 Jul 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions packages/aws-amplify/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
}
]
}
6 changes: 6 additions & 0 deletions packages/core/src/singleton/AmplifyOutputs/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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[];
}

Expand Down
133 changes: 132 additions & 1 deletion packages/storage/__tests__/providers/s3/apis/downloadData.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down Expand Up @@ -62,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(() => {
Expand All @@ -75,6 +76,7 @@ describe('downloadData with key', () => {
S3: {
bucket,
region,
buckets: { 'default-bucket': { bucketName: bucket, region } },
},
},
});
Expand Down Expand Up @@ -220,6 +222,70 @@ describe('downloadData with key', () => {
}),
);
});

describe('bucket passed in options', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for adding additional tests.

One thought here is that the bucket info overriding is carried by the function resolveS3ConfigAndInput.ts should we primarily test this function, and verify whether other consumer functions are invoking this function correctly, so we don't need to repeat the same tests in every test suite (as they are basically testing the resolveS3ConfigAndInput function)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So couple of pointers here, i think what called out makes sense.

  1. I still have not made changes to COPY api which needs resolution in both source and destination. Per design we might end up extracting resolveBucketConfig logic out and make it per API level if something pops out from Copy implement so for now i have it top level since that makes most sense. So i wrote the test per APi level so if we make the change it can catch any changes.

  2. We will still need test APi level here but rather in that case will check if API options is passed right. I think both of these achieve the same result in my mind.

That said, with next PR on copy API if this still stays here i can add in test specific to that function to check high level. But i think it is still beneficial per API level as well. wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not a big deal, but is there a way to compose this such that the same logic can be re-used between suites?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah i thought about it but we kinda do this in all test suites. Also, i see value in testing this API level like we are doing other places. let me take another look at this. All of these are testing their input on actual s3 call , so i think even if we compose we still need to pass that part in

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', () => {
Expand All @@ -233,6 +299,7 @@ describe('downloadData with path', () => {
S3: {
bucket,
region,
buckets: { 'default-bucket': { bucketName: bucket, region } },
},
},
});
Expand Down Expand Up @@ -366,4 +433,68 @@ describe('downloadData with path', () => {
}),
);
});

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,
},
});

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,
},
);
});

it('should override bucket in getObject call when bucket is string', async () => {
(getObject as jest.Mock).mockResolvedValueOnce({ Body: 'body' });
const abortController = new AbortController();

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,
},
);
});
});
});
107 changes: 105 additions & 2 deletions packages/storage/__tests__/providers/s3/apis/getProperties.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => ({
Expand All @@ -28,7 +29,7 @@ jest.mock('@aws-amplify/core', () => ({
}));
const mockHeadObject = headObject as jest.MockedFunction<typeof headObject>;
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';
Expand Down Expand Up @@ -65,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(() => {
Expand Down Expand Up @@ -152,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', () => {
Expand Down Expand Up @@ -201,6 +254,7 @@ describe('Happy cases: With path', () => {
S3: {
bucket,
region,
buckets: { 'default-bucket': { bucketName: bucket, region } },
},
},
});
Expand Down Expand Up @@ -275,6 +329,55 @@ describe('Happy cases: With path', () => {
);
},
);
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,
},
});
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,
},
);
});
});
});

describe('Error cases : With path', () => {
Expand Down
Loading