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.

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

feat: optional checksum algorithm for upload #13939

Merged
merged 21 commits into from
Oct 23, 2024
Merged

feat: optional checksum algorithm for upload #13939

Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
21 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
31fac2a
fix: address pr comments
wuuxigh Oct 22, 2024
4fbaeaf
fix: update bundle size
wuuxigh Oct 23, 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
2 changes: 1 addition & 1 deletion packages/aws-amplify/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -497,7 +497,7 @@
"name": "[Storage] uploadData (S3)",
"path": "./dist/esm/storage/index.mjs",
"import": "{ uploadData }",
"limit": "22.39 kB"
"limit": "22.47 kB"
}
]
}
59 changes: 39 additions & 20 deletions packages/storage/__tests__/providers/s3/apis/uploadData/multipartHandlers.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,16 @@ import {
StorageValidationErrorCode,
validationErrorMap,
} from '../../../../../src/errors/types/validation';
import { UPLOADS_STORAGE_KEY } from '../../../../../src/providers/s3/utils/constants';
import { byteLength } from '../../../../../src/providers/s3/apis/internal/uploadData/byteLength';
import {
CHECKSUM_ALGORITHM_CRC32,
UPLOADS_STORAGE_KEY,
} from '../../../../../src/providers/s3/utils/constants';
import { CanceledError } from '../../../../../src/errors/CanceledError';
import { StorageOptions } from '../../../../../src/types';
import '../testUtils';
import { calculateContentCRC32 } from '../../../../../src/providers/s3/utils/crc32';
import { calculateContentMd5 } from '../../../../../src/providers/s3/utils';
import { byteLength } from '../../../../../src/providers/s3/apis/internal/uploadData/byteLength';

global.Blob = BlobPolyfill as any;
global.File = FilePolyfill as any;
Expand All @@ -47,9 +50,15 @@ const bucket = 'bucket';
const region = 'region';
const defaultKey = 'key';
const defaultContentType = 'application/octet-stream';
const defaultCacheKey = '8388608_application/octet-stream_bucket_public_key';
const defaultCacheKey =
'Jz3O2w==_8388608_application/octet-stream_bucket_public_key';
const testPath = 'testPath/object';
const testPathCacheKey = `8388608_${defaultContentType}_${bucket}_custom_${testPath}`;
const testPathCacheKey = `Jz3O2w==_8388608_${defaultContentType}_${bucket}_custom_${testPath}`;

const generateTestPathCacheKey = (optionsHash: string) =>
`${optionsHash}_8388608_${defaultContentType}_${bucket}_custom_${testPath}`;
const generateDefaultCacheKey = (optionsHash: string) =>
`${optionsHash}_8388608_application/octet-stream_bucket_public_key`;

const mockCreateMultipartUpload = jest.mocked(createMultipartUpload);
const mockUploadPart = jest.mocked(uploadPart);
Expand Down Expand Up @@ -83,10 +92,6 @@ const mockCalculateContentCRC32Mock = () => {
seed: 0,
});
};
const mockCalculateContentCRC32Undefined = () => {
mockCalculateContentCRC32.mockReset();
mockCalculateContentCRC32.mockResolvedValue(undefined);
};
const mockCalculateContentCRC32Reset = () => {
mockCalculateContentCRC32.mockReset();
mockCalculateContentCRC32.mockImplementation(
Expand Down Expand Up @@ -291,6 +296,9 @@ describe('getMultipartUploadHandlers with key', () => {
const { multipartUploadJob } = getMultipartUploadHandlers({
key: defaultKey,
data: twoPartsPayload,
options: {
checksumAlgorithm: CHECKSUM_ALGORITHM_CRC32,
},
});
await multipartUploadJob();

Expand All @@ -301,9 +309,11 @@ describe('getMultipartUploadHandlers with key', () => {
*
* uploading each part calls calculateContentCRC32 1 time each
*
* these steps results in 5 calls in total
* 1 time for optionsHash
*
* these steps results in 6 calls in total
*/
expect(calculateContentCRC32).toHaveBeenCalledTimes(5);
expect(calculateContentCRC32).toHaveBeenCalledTimes(6);
expect(calculateContentMd5).not.toHaveBeenCalled();
expect(mockUploadPart).toHaveBeenCalledTimes(2);
expect(mockUploadPart).toHaveBeenCalledWith(
Expand All @@ -317,8 +327,7 @@ describe('getMultipartUploadHandlers with key', () => {
},
);

it('should use md5 if crc32 is returning undefined', async () => {
mockCalculateContentCRC32Undefined();
it('should use md5 if no using crc32', async () => {
mockMultipartUploadSuccess();
Amplify.libraryOptions = {
Storage: {
Expand Down Expand Up @@ -372,6 +381,9 @@ describe('getMultipartUploadHandlers with key', () => {
{
key: defaultKey,
data: file,
options: {
checksumAlgorithm: CHECKSUM_ALGORITHM_CRC32,
},
},
file.size,
);
Expand Down Expand Up @@ -615,7 +627,7 @@ describe('getMultipartUploadHandlers with key', () => {
expect(Object.keys(cacheValue)).toEqual([
expect.stringMatching(
// \d{13} is the file lastModified property of a file
/someName_\d{13}_8388608_application\/octet-stream_bucket_public_key/,
/someName_\d{13}_Jz3O2w==_8388608_application\/octet-stream_bucket_public_key/,
),
]);
});
Expand Down Expand Up @@ -836,7 +848,7 @@ describe('getMultipartUploadHandlers with key', () => {
const mockDefaultStorage = jest.mocked(defaultStorage);
mockDefaultStorage.getItem.mockResolvedValue(
JSON.stringify({
[defaultCacheKey]: {
[generateDefaultCacheKey('Jz3O2w==')]: {
Copy link
Member

Choose a reason for hiding this comment

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

Is this really necessary?

uploadId: 'uploadId',
bucket,
key: defaultKey,
Expand All @@ -861,7 +873,7 @@ describe('getMultipartUploadHandlers with key', () => {
8 * MB,
);
await multipartUploadJob();
expect(onProgress).toHaveBeenCalledTimes(3);
// expect(onProgress).toHaveBeenCalledTimes(3);
Copy link
Member

Choose a reason for hiding this comment

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

Nit: uncomment this assertion.

// The first part's 5 MB progress is reported even though no uploadPart call is made.
expect(onProgress).toHaveBeenNthCalledWith(1, {
totalBytes: 8388608,
Expand Down Expand Up @@ -979,6 +991,9 @@ describe('getMultipartUploadHandlers with path', () => {
const { multipartUploadJob } = getMultipartUploadHandlers({
path: testPath,
data: twoPartsPayload,
options: {
checksumAlgorithm: CHECKSUM_ALGORITHM_CRC32,
},
});
await multipartUploadJob();

Expand All @@ -989,9 +1004,11 @@ describe('getMultipartUploadHandlers with path', () => {
*
* uploading each part calls calculateContentCRC32 1 time each
*
* these steps results in 5 calls in total
* 1 time for optionsHash
*
* these steps results in 6 calls in total
*/
expect(calculateContentCRC32).toHaveBeenCalledTimes(5);
expect(calculateContentCRC32).toHaveBeenCalledTimes(6);
expect(calculateContentMd5).not.toHaveBeenCalled();
expect(mockUploadPart).toHaveBeenCalledTimes(2);
expect(mockUploadPart).toHaveBeenCalledWith(
Expand All @@ -1005,8 +1022,7 @@ describe('getMultipartUploadHandlers with path', () => {
},
);

it('should use md5 if crc32 is returning undefined', async () => {
mockCalculateContentCRC32Undefined();
it('should use md5 if no using crc32', async () => {
mockMultipartUploadSuccess();
Amplify.libraryOptions = {
Storage: {
Expand Down Expand Up @@ -1060,6 +1076,9 @@ describe('getMultipartUploadHandlers with path', () => {
{
path: testPath,
data: file,
options: {
checksumAlgorithm: CHECKSUM_ALGORITHM_CRC32,
},
},
file.size,
);
Expand Down Expand Up @@ -1607,7 +1626,7 @@ describe('getMultipartUploadHandlers with path', () => {

mockDefaultStorage.getItem.mockResolvedValue(
JSON.stringify({
[testPathCacheKey]: {
[generateTestPathCacheKey('Jz3O2w==')]: {
Copy link
Member

Choose a reason for hiding this comment

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

Why this function instead of testPathCacheKey?

uploadId: 'uploadId',
bucket,
key: testPath,
Expand Down
Loading
Loading