Skip to content
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.

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

feat: preventOverwrite with if-none-match header #13954

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
7d2cff9
feat: opt in checksum
wuuxigh Sep 16, 2024
8689812
fix: revert local prettier suggestion
wuuxigh Sep 16, 2024
01b3bd5
fix: up size limit for storage upload data
wuuxigh Sep 16, 2024
92bbe33
feat: react native crc32
wuuxigh Sep 20, 2024
bc6ce6b
fix: up bundle size limit and fix typo
wuuxigh Sep 23, 2024
3cae42c
feat: add documentation for checksumAlgorithm
wuuxigh Sep 24, 2024
e75e313
Merge branch 'staging' into opt-in-checksum
wuuxigh Sep 24, 2024
0786145
Merge branch 'staging' into opt-in-checksum
wuuxigh Sep 24, 2024
ebd6109
fix: update bundle size limit
wuuxigh Sep 24, 2024
8d721e2
fix: update bundle size limit
wuuxigh Sep 25, 2024
f951902
fix: address pr feedbacks
wuuxigh Oct 3, 2024
b51a94d
fix: bundle-size limit
wuuxigh Oct 3, 2024
1c4034d
Merge branch 'storage-browser/integrity' into opt-in-checksum
AllanZhengYP Oct 9, 2024
a54b99a
fix: ract native 0.70.0 support for crc32
wuuxigh Oct 18, 2024
d96db71
chore: move comment to the new readFile
wuuxigh Oct 18, 2024
a55f76f
Merge branch 'storage-browser/integrity' into opt-in-checksum
wuuxigh Oct 18, 2024
2773a2c
fix: update optionsHash values for tests
wuuxigh Oct 18, 2024
e55f583
chore: remove scripts folder from ts config
wuuxigh Oct 18, 2024
9af2b55
chore: update bundle size
wuuxigh Oct 18, 2024
5cbba83
feat: preventOverwrite with If-None-Match header
wuuxigh Oct 19, 2024
31fac2a
fix: address pr comments
wuuxigh Oct 22, 2024
4fbaeaf
fix: update bundle size
wuuxigh Oct 23, 2024
b6dfc0b
Merge branch 'storage-browser/integrity' into opt-in-checksum
wuuxigh Oct 24, 2024
4ab3fae
Merge branch 'opt-in-checksum' into if-none-match
wuuxigh Oct 24, 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
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
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
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<
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 this!

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,
];
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
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

This file was deleted.

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
Loading