Skip to content

Commit

Permalink
feat: preventOverwrite with if-none-match header (#13954)
Browse files Browse the repository at this point in the history 
Co-authored-by: AllanZhengYP <[email protected]>
  • Loading branch information
@wuuxigh @AllanZhengYP
wuuxigh and AllanZhengYP authored Oct 24, 2024
1 parent 873a446 commit bb4e0a3
Show file tree
Hide file tree
Showing 9 changed files with 80 additions and 189 deletions.
63 changes: 4 additions & 59 deletions packages/storage/__tests__/providers/s3/apis/uploadData/multipartHandlers.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1153,16 +1153,8 @@ describe('getMultipartUploadHandlers with path', () => {
});

describe('overwrite prevention', () => {
beforeEach(() => {
mockHeadObject.mockReset();
mockUploadPart.mockReset();
});

it('should upload if target key is not found', async () => {
expect.assertions(7);
const notFoundError = new Error('mock message');
notFoundError.name = 'NotFound';
mockHeadObject.mockRejectedValueOnce(notFoundError);
it('should include if-none-match header in complete request', async () => {
expect.assertions(3);
mockMultipartUploadSuccess();

const { multipartUploadJob } = getMultipartUploadHandlers({
Expand All @@ -1171,62 +1163,15 @@ describe('getMultipartUploadHandlers with path', () => {
options: { preventOverwrite: true },
});
await multipartUploadJob();

expect(mockCreateMultipartUpload).toHaveBeenCalledTimes(1);
expect(mockUploadPart).toHaveBeenCalledTimes(2);
expect(mockHeadObject).toHaveBeenCalledTimes(1);
await expect(mockHeadObject).toBeLastCalledWithConfigAndInput(
expect(mockCompleteMultipartUpload).toBeLastCalledWithConfigAndInput(
expect.objectContaining({
credentials,
region,
}),
expect.objectContaining({
Bucket: bucket,
Key: testPath,
IfNoneMatch: '*',
}),
);
expect(mockCompleteMultipartUpload).toHaveBeenCalledTimes(1);
});

it('should not upload if target key already exists', async () => {
expect.assertions(6);
mockHeadObject.mockResolvedValueOnce({
ContentLength: 0,
$metadata: {},
});
mockMultipartUploadSuccess();

const { multipartUploadJob } = getMultipartUploadHandlers({
path: testPath,
data: new ArrayBuffer(8 * MB),
options: { preventOverwrite: true },
});

await expect(multipartUploadJob()).rejects.toThrow(
'At least one of the pre-conditions you specified did not hold',
);
expect(mockCreateMultipartUpload).toHaveBeenCalledTimes(1);
expect(mockUploadPart).toHaveBeenCalledTimes(2);
expect(mockCompleteMultipartUpload).not.toHaveBeenCalled();
});

it('should not upload if HeadObject fails with other error', async () => {
expect.assertions(6);
const accessDeniedError = new Error('mock error');
accessDeniedError.name = 'AccessDenied';
mockHeadObject.mockRejectedValueOnce(accessDeniedError);
mockMultipartUploadSuccess();

const { multipartUploadJob } = getMultipartUploadHandlers({
path: testPath,
data: new ArrayBuffer(8 * MB),
options: { preventOverwrite: true },
});

await expect(multipartUploadJob()).rejects.toThrow('mock error');
expect(mockCreateMultipartUpload).toHaveBeenCalledTimes(1);
expect(mockUploadPart).toHaveBeenCalledTimes(2);
expect(mockCompleteMultipartUpload).not.toHaveBeenCalled();
});
});

Expand Down
72 changes: 7 additions & 65 deletions packages/storage/__tests__/providers/s3/apis/uploadData/putObjectJob.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,7 @@
import { AWSCredentials } from '@aws-amplify/core/internals/utils';
import { Amplify } from '@aws-amplify/core';

import {
headObject,
putObject,
} from '../../../../../src/providers/s3/utils/client/s3data';
import { putObject } from '../../../../../src/providers/s3/utils/client/s3data';
import { calculateContentMd5 } from '../../../../../src/providers/s3/utils';
import * as CRC32 from '../../../../../src/providers/s3/utils/crc32';
import { putObjectJob } from '../../../../../src/providers/s3/apis/internal/uploadData/putObjectJob';
Expand Down Expand Up @@ -44,7 +41,6 @@ const credentials: AWSCredentials = {
const identityId = 'identityId';
const mockFetchAuthSession = jest.mocked(Amplify.Auth.fetchAuthSession);
const mockPutObject = jest.mocked(putObject);
const mockHeadObject = jest.mocked(headObject);
const bucket = 'bucket';
const region = 'region';

Expand Down Expand Up @@ -364,16 +360,7 @@ describe('putObjectJob with path', () => {
});

describe('overwrite prevention', () => {
beforeEach(() => {
mockHeadObject.mockClear();
});

it('should upload if target key is not found', async () => {
expect.assertions(3);
const notFoundError = new Error('mock message');
notFoundError.name = 'NotFound';
mockHeadObject.mockRejectedValueOnce(notFoundError);

it('should include if-none-match header', async () => {
const job = putObjectJob(
{
path: testPath,
Expand All @@ -384,57 +371,12 @@ describe('putObjectJob with path', () => {
);
await job();

await expect(mockHeadObject).toBeLastCalledWithConfigAndInput(
{
credentials,
region: 'region',
},
{
Bucket: 'bucket',
Key: testPath,
},
);
expect(mockHeadObject).toHaveBeenCalledTimes(1);
expect(mockPutObject).toHaveBeenCalledTimes(1);
});

it('should not upload if target key already exists', async () => {
expect.assertions(3);
mockHeadObject.mockResolvedValueOnce({
ContentLength: 0,
$metadata: {},
});
const job = putObjectJob(
{
path: testPath,
data: 'data',
options: { preventOverwrite: true },
},
new AbortController().signal,
);
await expect(job()).rejects.toThrow(
'At least one of the pre-conditions you specified did not hold',
);
expect(mockHeadObject).toHaveBeenCalledTimes(1);
expect(mockPutObject).not.toHaveBeenCalled();
});

it('should not upload if HeadObject fails with other error', async () => {
expect.assertions(3);
const accessDeniedError = new Error('mock error');
accessDeniedError.name = 'AccessDenied';
mockHeadObject.mockRejectedValueOnce(accessDeniedError);
const job = putObjectJob(
{
path: testPath,
data: 'data',
options: { preventOverwrite: true },
},
new AbortController().signal,
await expect(mockPutObject).toBeLastCalledWithConfigAndInput(
expect.objectContaining({ credentials, region }),
expect.objectContaining({
IfNoneMatch: '*',
}),
);
await expect(job()).rejects.toThrow('mock error');
expect(mockHeadObject).toHaveBeenCalledTimes(1);
expect(mockPutObject).not.toHaveBeenCalled();
});
});

Expand Down
72 changes: 49 additions & 23 deletions packages/storage/__tests__/providers/s3/utils/client/S3/cases/completeMultipartUpload.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,30 @@ import {
expectedMetadata,
} from './shared';

const defaultExpectedRequest = {
url: expect.objectContaining({
href: 'https://bucket.s3.us-east-1.amazonaws.com/key?uploadId=uploadId',
}),
method: 'POST',
headers: expect.objectContaining({
'content-type': 'application/xml',
}),
body:
'<?xml version="1.0" encoding="UTF-8"?>' +
'<CompleteMultipartUpload xmlns="http://s3.amazonaws.com/doc/2006-03-01/">' +
'<Part>' +
'<ETag>etag1</ETag>' +
'<PartNumber>1</PartNumber>' +
'<ChecksumCRC32>test-checksum-1</ChecksumCRC32>' +
'</Part>' +
'<Part>' +
'<ETag>etag2</ETag>' +
'<PartNumber>2</PartNumber>' +
'<ChecksumCRC32>test-checksum-2</ChecksumCRC32>' +
'</Part>' +
'</CompleteMultipartUpload>',
};

// API reference: https://docs.aws.amazon.com/AmazonS3/latest/API/API_CompleteMultipartUpload.html
const completeMultipartUploadHappyCase: ApiFunctionalTestCase<
typeof completeMultipartUpload
Expand Down Expand Up @@ -37,29 +61,7 @@ const completeMultipartUploadHappyCase: ApiFunctionalTestCase<
},
UploadId: 'uploadId',
},
expect.objectContaining({
url: expect.objectContaining({
href: 'https://bucket.s3.us-east-1.amazonaws.com/key?uploadId=uploadId',
}),
method: 'POST',
headers: expect.objectContaining({
'content-type': 'application/xml',
}),
body:
'<?xml version="1.0" encoding="UTF-8"?>' +
'<CompleteMultipartUpload xmlns="http://s3.amazonaws.com/doc/2006-03-01/">' +
'<Part>' +
'<ETag>etag1</ETag>' +
'<PartNumber>1</PartNumber>' +
'<ChecksumCRC32>test-checksum-1</ChecksumCRC32>' +
'</Part>' +
'<Part>' +
'<ETag>etag2</ETag>' +
'<PartNumber>2</PartNumber>' +
'<ChecksumCRC32>test-checksum-2</ChecksumCRC32>' +
'</Part>' +
'</CompleteMultipartUpload>',
}),
expect.objectContaining(defaultExpectedRequest),
{
status: 200,
headers: { ...DEFAULT_RESPONSE_HEADERS },
Expand All @@ -79,6 +81,29 @@ const completeMultipartUploadHappyCase: ApiFunctionalTestCase<
},
];

// API reference: https://docs.aws.amazon.com/AmazonS3/latest/API/API_CompleteMultipartUpload.html
const completeMultipartUploadHappyCaseIfNoneMatch: ApiFunctionalTestCase<
typeof completeMultipartUpload
> = [
'happy case',
'completeMultipartUpload - if-none-match',
completeMultipartUpload,
defaultConfig,
{
...completeMultipartUploadHappyCase[4],
IfNoneMatch: 'mock-if-none-match',
},
expect.objectContaining({
...defaultExpectedRequest,
headers: {
'content-type': 'application/xml',
'If-None-Match': 'mock-if-none-match',
},
}),
completeMultipartUploadHappyCase[6],
completeMultipartUploadHappyCase[7],
];

// API reference: https://docs.aws.amazon.com/AmazonS3/latest/API/API_CompleteMultipartUpload.html
const completeMultipartUploadErrorCase: ApiFunctionalTestCase<
typeof completeMultipartUpload
Expand Down Expand Up @@ -141,6 +166,7 @@ const completeMultipartUploadErrorWith200CodeCase: ApiFunctionalTestCase<

export default [
completeMultipartUploadHappyCase,
completeMultipartUploadHappyCaseIfNoneMatch,
completeMultipartUploadErrorCase,
completeMultipartUploadErrorWith200CodeCase,
];
9 changes: 1 addition & 8 deletions packages/storage/src/providers/s3/apis/internal/uploadData/multipart/uploadHandlers.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ import {
} from '../../../../utils/client/s3data';
import { getStorageUserAgentValue } from '../../../../utils/userAgent';
import { logger } from '../../../../../../utils';
import { validateObjectNotExists } from '../validateObjectNotExists';
import { calculateContentCRC32 } from '../../../../utils/crc32';
import { StorageOperationOptionsInput } from '../../../../../../types/inputs';

Expand Down Expand Up @@ -237,13 +236,6 @@ export const getMultipartUploadHandlers = (

await Promise.all(concurrentUploadPartExecutors);

if (preventOverwrite) {
await validateObjectNotExists(resolvedS3Config, {
Bucket: resolvedBucket,
Key: finalKey,
});
}

const { ETag: eTag } = await completeMultipartUpload(
{
...resolvedS3Config,
Expand All @@ -255,6 +247,7 @@ export const getMultipartUploadHandlers = (
Key: finalKey,
UploadId: inProgressUpload.uploadId,
ChecksumCRC32: inProgressUpload.finalCrc32,
IfNoneMatch: preventOverwrite ? '*' : undefined,
MultipartUpload: {
Parts: inProgressUpload.completedParts.sort(
(partA, partB) => partA.PartNumber! - partB.PartNumber!,
Expand Down
10 changes: 1 addition & 9 deletions packages/storage/src/providers/s3/apis/internal/uploadData/putObjectJob.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,6 @@ import {
import { calculateContentCRC32 } from '../../../utils/crc32';
import { constructContentDisposition } from '../../../utils/constructContentDisposition';

import { validateObjectNotExists } from './validateObjectNotExists';

/**
* The input interface for UploadData API with only the options needed for single part upload.
* It supports both legacy Gen 1 input with key and Gen2 input with path. It also support additional
Expand Down Expand Up @@ -81,13 +79,6 @@ export const putObjectJob =
? await calculateContentMd5(data)
: undefined;

if (preventOverwrite) {
await validateObjectNotExists(s3Config, {
Bucket: bucket,
Key: finalKey,
});
}

const { ETag: eTag, VersionId: versionId } = await putObject(
{
...s3Config,
Expand All @@ -106,6 +97,7 @@ export const putObjectJob =
ContentMD5: contentMD5,
ChecksumCRC32: checksumCRC32?.checksum,
ExpectedBucketOwner: expectedBucketOwner,
IfNoneMatch: preventOverwrite ? '*' : undefined,
},
);

Expand Down
25 changes: 0 additions & 25 deletions packages/storage/src/providers/s3/apis/internal/uploadData/validateObjectNotExists.ts
View file

This file was deleted.

2 changes: 2 additions & 0 deletions packages/storage/src/providers/s3/utils/client/s3data/completeMultipartUpload.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ export type CompleteMultipartUploadInput = Pick<
| 'MultipartUpload'
| 'ChecksumCRC32'
| 'ExpectedBucketOwner'
| 'IfNoneMatch'
>;

export type CompleteMultipartUploadOutput = Pick<
Expand All @@ -62,6 +63,7 @@ const completeMultipartUploadSerializer = async (
...assignStringVariables({
'x-amz-checksum-crc32': input.ChecksumCRC32,
'x-amz-expected-bucket-owner': input.ExpectedBucketOwner,
'If-None-Match': input.IfNoneMatch,
}),
};
const url = new AmplifyUrl(endpoint.url.toString());
Expand Down
Loading

0 comments on commit bb4e0a3

Please sign in to comment.