Skip to content

Commit

Permalink
revert: optional checksum algorithm for upload (#13849) (#13910)
Browse files Browse the repository at this point in the history 
This reverts commit 02cb08a.
  • Loading branch information
@AllanZhengYP
AllanZhengYP authored Oct 10, 2024
1 parent fb4237b commit dbe5c7b
Show file tree
Hide file tree
Showing 16 changed files with 123 additions and 692 deletions.
56 changes: 18 additions & 38 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 @@ -20,10 +20,7 @@ import {
StorageValidationErrorCode,
validationErrorMap,
} from '../../../../../src/errors/types/validation';
import {
CHECKSUM_ALGORITHM_CRC32,
UPLOADS_STORAGE_KEY,
} from '../../../../../src/providers/s3/utils/constants';
import { UPLOADS_STORAGE_KEY } from '../../../../../src/providers/s3/utils/constants';
import { byteLength } from '../../../../../src/providers/s3/apis/internal/uploadData/byteLength';
import { CanceledError } from '../../../../../src/errors/CanceledError';
import { StorageOptions } from '../../../../../src/types';
Expand All @@ -50,15 +47,9 @@ const bucket = 'bucket';
const region = 'region';
const defaultKey = 'key';
const defaultContentType = 'application/octet-stream';
const defaultCacheKey =
'/twwTw==_8388608_application/octet-stream_bucket_public_key';
const defaultCacheKey = '8388608_application/octet-stream_bucket_public_key';
const testPath = 'testPath/object';
const testPathCacheKey = `/twwTw==_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 testPathCacheKey = `8388608_${defaultContentType}_${bucket}_custom_${testPath}`;

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

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

it('should use md5 if no using crc32', async () => {
it('should use md5 if crc32 is returning undefined', async () => {
mockCalculateContentCRC32Undefined();
mockMultipartUploadSuccess();
Amplify.libraryOptions = {
Storage: {
Expand Down Expand Up @@ -381,9 +372,6 @@ describe('getMultipartUploadHandlers with key', () => {
{
key: defaultKey,
data: file,
options: {
checksumAlgorithm: CHECKSUM_ALGORITHM_CRC32,
},
},
file.size,
);
Expand Down Expand Up @@ -601,7 +589,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}_\/twwTw==_8388608_application\/octet-stream_bucket_public_key/,
/someName_\d{13}_8388608_application\/octet-stream_bucket_public_key/,
),
]);
});
Expand Down Expand Up @@ -812,7 +800,7 @@ describe('getMultipartUploadHandlers with key', () => {
>;
mockDefaultStorage.getItem.mockResolvedValue(
JSON.stringify({
[generateDefaultCacheKey('o6a/Qw==')]: {
[defaultCacheKey]: {
uploadId: 'uploadId',
bucket,
key: defaultKey,
Expand Down Expand Up @@ -954,9 +942,6 @@ describe('getMultipartUploadHandlers with path', () => {
const { multipartUploadJob } = getMultipartUploadHandlers({
path: testPath,
data: twoPartsPayload,
options: {
checksumAlgorithm: CHECKSUM_ALGORITHM_CRC32,
},
});
await multipartUploadJob();

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

it('should use md5 if no using crc32', async () => {
it('should use md5 if crc32 is returning undefined', async () => {
mockCalculateContentCRC32Undefined();
mockMultipartUploadSuccess();
Amplify.libraryOptions = {
Storage: {
Expand Down Expand Up @@ -1039,9 +1023,6 @@ describe('getMultipartUploadHandlers with path', () => {
{
path: testPath,
data: file,
options: {
checksumAlgorithm: CHECKSUM_ALGORITHM_CRC32,
},
},
file.size,
);
Expand Down Expand Up @@ -1552,10 +1533,9 @@ describe('getMultipartUploadHandlers with path', () => {
const mockDefaultStorage = defaultStorage as jest.Mocked<
typeof defaultStorage
>;

mockDefaultStorage.getItem.mockResolvedValue(
JSON.stringify({
[generateTestPathCacheKey('o6a/Qw==')]: {
[testPathCacheKey]: {
uploadId: 'uploadId',
bucket,
key: testPath,
Expand Down
158 changes: 62 additions & 96 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 @@ -15,8 +15,6 @@ 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';
import '../testUtils';
import { UploadDataChecksumAlgorithm } from '../../../../../src/providers/s3/types/options';
import { CHECKSUM_ALGORITHM_CRC32 } from '../../../../../src/providers/s3/utils/constants';

global.Blob = BlobPolyfill as any;
global.File = FilePolyfill as any;
Expand Down Expand Up @@ -77,77 +75,66 @@ mockPutObject.mockResolvedValue({
/* TODO Remove suite when `key` parameter is removed */
describe('putObjectJob with key', () => {
beforeEach(() => {
mockPutObject.mockClear();
jest.spyOn(CRC32, 'calculateContentCRC32').mockRestore();
});

it.each<{ checksumAlgorithm: UploadDataChecksumAlgorithm | undefined }>([
{ checksumAlgorithm: CHECKSUM_ALGORITHM_CRC32 },
{ checksumAlgorithm: undefined },
])(
'should supply the correct parameters to putObject API handler with checksumAlgorithm as $checksumAlgorithm',
async ({ checksumAlgorithm }) => {
const abortController = new AbortController();
const inputKey = 'key';
const data = 'data';
const mockContentType = 'contentType';
const contentDisposition = 'contentDisposition';
const contentEncoding = 'contentEncoding';
const mockMetadata = { key: 'value' };
const onProgress = jest.fn();
const useAccelerateEndpoint = true;
it('should supply the correct parameters to putObject API handler', async () => {
const abortController = new AbortController();
const inputKey = 'key';
const data = 'data';
const mockContentType = 'contentType';
const contentDisposition = 'contentDisposition';
const contentEncoding = 'contentEncoding';
const mockMetadata = { key: 'value' };
const onProgress = jest.fn();
const useAccelerateEndpoint = true;

const job = putObjectJob(
{
key: inputKey,
data,
options: {
contentDisposition,
contentEncoding,
contentType: mockContentType,
metadata: mockMetadata,
onProgress,
useAccelerateEndpoint,
checksumAlgorithm,
},
},
abortController.signal,
);
const result = await job();
expect(result).toEqual({
const job = putObjectJob(
{
key: inputKey,
eTag: 'eTag',
versionId: 'versionId',
contentType: 'contentType',
metadata: { key: 'value' },
size: undefined,
});
expect(mockPutObject).toHaveBeenCalledTimes(1);
await expect(mockPutObject).toBeLastCalledWithConfigAndInput(
{
credentials,
region,
abortSignal: abortController.signal,
onUploadProgress: expect.any(Function),
useAccelerateEndpoint: true,
userAgentValue: expect.any(String),
},
{
Bucket: bucket,
Key: `public/${inputKey}`,
Body: data,
ContentType: mockContentType,
ContentDisposition: contentDisposition,
ContentEncoding: contentEncoding,
Metadata: mockMetadata,
ChecksumCRC32:
checksumAlgorithm === CHECKSUM_ALGORITHM_CRC32
? 'rfPzYw=='
: undefined,
data,
options: {
contentDisposition,
contentEncoding,
contentType: mockContentType,
metadata: mockMetadata,
onProgress,
useAccelerateEndpoint,
},
);
},
);
},
abortController.signal,
);
const result = await job();
expect(result).toEqual({
key: inputKey,
eTag: 'eTag',
versionId: 'versionId',
contentType: 'contentType',
metadata: { key: 'value' },
size: undefined,
});
expect(mockPutObject).toHaveBeenCalledTimes(1);
await expect(mockPutObject).toBeLastCalledWithConfigAndInput(
{
credentials,
region,
abortSignal: abortController.signal,
onUploadProgress: expect.any(Function),
useAccelerateEndpoint: true,
userAgentValue: expect.any(String),
},
{
Bucket: bucket,
Key: `public/${inputKey}`,
Body: data,
ContentType: mockContentType,
ContentDisposition: contentDisposition,
ContentEncoding: contentEncoding,
Metadata: mockMetadata,
ChecksumCRC32: 'rfPzYw==',
},
);
});

it('should set ContentMD5 if object lock is enabled', async () => {
jest
Expand Down Expand Up @@ -206,6 +193,7 @@ describe('putObjectJob with key', () => {
Key: 'public/key',
Body: data,
ContentType: 'application/octet-stream',
ChecksumCRC32: 'rfPzYw==',
},
);
});
Expand Down Expand Up @@ -237,6 +225,7 @@ describe('putObjectJob with key', () => {
Key: 'public/key',
Body: data,
ContentType: 'application/octet-stream',
ChecksumCRC32: 'rfPzYw==',
},
);
});
Expand All @@ -249,39 +238,18 @@ describe('putObjectJob with path', () => {
jest.spyOn(CRC32, 'calculateContentCRC32').mockRestore();
});

it.each<{ checksumAlgorithm: UploadDataChecksumAlgorithm | undefined }>([
{ checksumAlgorithm: CHECKSUM_ALGORITHM_CRC32 },
{ checksumAlgorithm: undefined },
]);

test.each<{
path: string | (() => string);
expectedKey: string;
checksumAlgorithm: UploadDataChecksumAlgorithm | undefined;
}>([
{
path: testPath,
expectedKey: testPath,
checksumAlgorithm: CHECKSUM_ALGORITHM_CRC32,
},
{
path: () => testPath,
expectedKey: testPath,
checksumAlgorithm: CHECKSUM_ALGORITHM_CRC32,
},
test.each([
{
path: testPath,
expectedKey: testPath,
checksumAlgorithm: undefined,
},
{
path: () => testPath,
expectedKey: testPath,
checksumAlgorithm: undefined,
},
])(
'should supply the correct parameters to putObject API handler when path is $path and checksumAlgorithm is $checksumAlgorithm',
async ({ path: inputPath, expectedKey, checksumAlgorithm }) => {
'should supply the correct parameters to putObject API handler when path is $path',
async ({ path: inputPath, expectedKey }) => {
const abortController = new AbortController();
const data = 'data';
const mockContentType = 'contentType';
Expand All @@ -302,7 +270,6 @@ describe('putObjectJob with path', () => {
metadata: mockMetadata,
onProgress,
useAccelerateEndpoint,
checksumAlgorithm,
},
},
abortController.signal,
Expand Down Expand Up @@ -334,10 +301,7 @@ describe('putObjectJob with path', () => {
ContentDisposition: contentDisposition,
ContentEncoding: contentEncoding,
Metadata: mockMetadata,
ChecksumCRC32:
checksumAlgorithm === CHECKSUM_ALGORITHM_CRC32
? 'rfPzYw=='
: undefined,
ChecksumCRC32: 'rfPzYw==',
},
);
},
Expand Down Expand Up @@ -475,6 +439,7 @@ describe('putObjectJob with path', () => {
Key: 'path/',
Body: data,
ContentType: 'application/octet-stream',
ChecksumCRC32: 'rfPzYw==',
},
);
});
Expand Down Expand Up @@ -506,6 +471,7 @@ describe('putObjectJob with path', () => {
Key: 'path/',
Body: data,
ContentType: 'application/octet-stream',
ChecksumCRC32: 'rfPzYw==',
},
);
});
Expand Down
Loading

0 comments on commit dbe5c7b

Please sign in to comment.