fix(storage): omit unserializable properties when hash uploadDataOptions (#14151)

Author: AllanZhengYP

packages/aws-amplify/package.json

@@ -515,7 +515,7 @@
 			"name": "[Storage] uploadData (S3)",
 			"path": "./dist/esm/storage/index.mjs",
 			"import": "{ uploadData }",
-			"limit": "22.87 kB"
+			"limit": "22.95 kB"
 		}
 	]
 }

packages/storage/__tests__/providers/s3/apis/internal/uploadData/multipartHandlers.test.ts

@@ -44,10 +44,10 @@ const bucket = 'bucket';
 const region = 'region';
 const defaultKey = 'key';
 const defaultContentType = 'application/octet-stream';
-const defaultCacheKey =
-	'Jz3O2w==_8388608_application/octet-stream_bucket_public_key';
+const emptyOptionHash = 'o6a/Qw=='; // crc32 for '{}'
+const defaultCacheKey = `${emptyOptionHash}_8388608_application/octet-stream_bucket_public_key`;
 const testPath = 'testPath/object';
-const testPathCacheKey = `Jz3O2w==_8388608_${defaultContentType}_${bucket}_custom_${testPath}`;
+const testPathCacheKey = `${emptyOptionHash}_8388608_${defaultContentType}_${bucket}_custom_${testPath}`;
 
 const mockCreateMultipartUpload = jest.mocked(createMultipartUpload);
 const mockUploadPart = jest.mocked(uploadPart);
@@ -545,14 +545,14 @@ describe('getMultipartUploadHandlers with key', () => {
 		describe('cache validation', () => {
 			it.each([
 				{
-					name: 'wrong part count',
+					name: 'mismatched part count between cached upload and actual upload',
 					parts: [{ PartNumber: 1 }, { PartNumber: 2 }, { PartNumber: 3 }],
 				},
 				{
-					name: 'wrong part numbers',
+					name: 'mismatched part numbers between cached upload and actual upload',
 					parts: [{ PartNumber: 1 }, { PartNumber: 1 }],
 				},
-			])('should throw with $name', async ({ parts }) => {
+			])('should throw integrity error with $name', async ({ parts }) => {
 				mockMultipartUploadSuccess();
 
 				const mockDefaultStorage = defaultStorage as jest.Mocked<
@@ -619,6 +619,42 @@ describe('getMultipartUploadHandlers with key', () => {
 			expect(mockListParts).not.toHaveBeenCalled();
 		});
 
+		it('should omit unserializable option properties when calculating the hash key', async () => {
+			mockMultipartUploadSuccess();
+			const serializableOptions = {
+				useAccelerateEndpoint: true,
+				bucket: { bucketName: 'bucket', region: 'us-west-2' },
+				expectedBucketOwner: '123',
+				contentDisposition: 'attachment',
+				contentEncoding: 'deflate',
+				contentType: 'text/html',
+				customEndpoint: 'abc',
+				metadata: {},
+				preventOverwrite: true,
+				checksumAlgorithm: 'crc-32' as const,
+			};
+			const size = 8 * MB;
+			const { multipartUploadJob } = getMultipartUploadHandlers(
+				{
+					key: defaultKey,
+					data: new ArrayBuffer(size),
+					options: {
+						...serializableOptions,
+						// The following options will be omitted
+						locationCredentialsProvider: jest.fn(),
+						onProgress: jest.fn(),
+						resumableUploadsCache: mockDefaultStorage,
+					},
+				},
+				size,
+			);
+			await multipartUploadJob();
+			expect(mockCalculateContentCRC32).toHaveBeenNthCalledWith(
+				1,
+				JSON.stringify(serializableOptions),
+			);
+		});
+
 		it('should send createMultipartUpload request if the upload task is not cached', async () => {
 			mockMultipartUploadSuccess();
 			const size = 8 * MB;
@@ -693,7 +729,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}_Jz3O2w==_8388608_application\/octet-stream_bucket_public_key/,
+					new RegExp(`someName_\\d{13}_${defaultCacheKey}`),
 				),
 			]);
 		});
@@ -1451,6 +1487,42 @@ describe('getMultipartUploadHandlers with path', () => {
 			expect(mockListParts).not.toHaveBeenCalled();
 		});
 
+		it('should omit unserializable option properties when calculating the hash key', async () => {
+			mockMultipartUploadSuccess();
+			const serializableOptions = {
+				useAccelerateEndpoint: true,
+				bucket: { bucketName: 'bucket', region: 'us-west-2' },
+				expectedBucketOwner: '123',
+				contentDisposition: 'attachment',
+				contentEncoding: 'deflate',
+				contentType: 'text/html',
+				customEndpoint: 'abc',
+				metadata: {},
+				preventOverwrite: true,
+				checksumAlgorithm: 'crc-32' as const,
+			};
+			const size = 8 * MB;
+			const { multipartUploadJob } = getMultipartUploadHandlers(
+				{
+					path: testPath,
+					data: new ArrayBuffer(size),
+					options: {
+						...serializableOptions,
+						// The following options will be omitted
+						locationCredentialsProvider: jest.fn(),
+						onProgress: jest.fn(),
+						resumableUploadsCache: mockDefaultStorage,
+					},
+				},
+				size,
+			);
+			await multipartUploadJob();
+			expect(mockCalculateContentCRC32).toHaveBeenNthCalledWith(
+				1,
+				JSON.stringify(serializableOptions),
+			);
+		});
+
 		it('should send createMultipartUpload request if the upload task is cached but outdated', async () => {
 			mockDefaultStorage.getItem.mockResolvedValue(
 				JSON.stringify({

packages/storage/src/providers/s3/apis/internal/uploadData/multipart/uploadCache.ts

@@ -10,6 +10,8 @@ import { UPLOADS_STORAGE_KEY } from '../../../../utils/constants';
 import { ResolvedS3Config } from '../../../../types/options';
 import { Part, listParts } from '../../../../utils/client/s3data';
 import { logger } from '../../../../../../utils';
+// TODO: Remove this interface when we move to public advanced APIs.
+import { UploadDataInput as UploadDataWithPathInputWithAdvancedOptions } from '../../../../../../internals/types/inputs';
 
 const ONE_HOUR = 1000 * 60 * 60;
 
@@ -96,6 +98,28 @@ const listCachedUploadTasks = async (
 	}
 };
 
+/**
+ * Serialize the uploadData API options to string so it can be hashed.
+ */
+export const serializeUploadOptions = (
+	options: UploadDataWithPathInputWithAdvancedOptions['options'] & {
+		resumableUploadsCache?: KeyValueStorageInterface;
+	} = {},
+): string => {
+	const unserializableOptionProperties: string[] = [
+		'onProgress',
+		'resumableUploadsCache', // Internally injected implementation not set by customers
+		'locationCredentialsProvider', // Internally injected implementation not set by customers
+	] satisfies (keyof typeof options)[];
+	const serializableOptions = Object.fromEntries(
+		Object.entries(options).filter(
+			([key]) => !unserializableOptionProperties.includes(key),
+		),
+	);
+
+	return JSON.stringify(serializableOptions);
+};
+
 interface UploadsCacheKeyOptions {
 	size: number;
 	contentType?: string;

packages/storage/src/providers/s3/apis/internal/uploadData/multipart/uploadHandlers.ts

@@ -40,7 +40,11 @@ import { StorageOperationOptionsInput } from '../../../../../../types/inputs';
 import { IntegrityError } from '../../../../../../errors/IntegrityError';
 
 import { uploadPartExecutor } from './uploadPartExecutor';
-import { getUploadsCacheKey, removeCachedUpload } from './uploadCache';
+import {
+	getUploadsCacheKey,
+	removeCachedUpload,
+	serializeUploadOptions,
+} from './uploadCache';
 import { getConcurrentUploadsProgressTracker } from './progressTracker';
 import { loadOrCreateMultipartUpload } from './initialUpload';
 import { getDataChunker } from './getDataChunker';
@@ -151,9 +155,9 @@ export const getMultipartUploadHandlers = (
 			resolvedAccessLevel = resolveAccessLevel(accessLevel);
 		}
 
-		const optionsHash = (
-			await calculateContentCRC32(JSON.stringify(uploadDataOptions))
-		).checksum;
+		const { checksum: optionsHash } = await calculateContentCRC32(
+			serializeUploadOptions(uploadDataOptions),
+		);
 
 		if (!inProgressUpload) {
 			const { uploadId, cachedParts, finalCrc32 } =