feat: optional checksum algorithm for upload #13849

wuuxigh · 2024-09-24T23:46:23Z

Description of changes

Added optional checksumAlgorithm option for uploadData API to enable CRC-32 checksum calculation.
CRC-32 now works within react native environment upon enablement with this new option.
Upload cache key now includes a UploadDataOptions CRC-32 hash to avoid collision.

Issue #, if available

Description of how you validated changes

Updated unit tests utilizing the checksumAlgorithm option for CRC-32.
Local run of react native e2e test.

Checklist

PR description included
yarn test passes
Unit Tests are changed or added
Relevant documentation is changed or added (and PR referenced)

Checklist for repo maintainers

Verify E2E tests for existing workflows are working as expected or add E2E tests for newly added workflows
New source file paths included in this PR have been added to CODEOWNERS, if appropriate

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

packages/storage/src/providers/s3/apis/uploadData/putObjectJob.ts

AllanZhengYP · 2024-09-27T00:12:07Z

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

+		const optionsHash = (
+			await calculateContentCRC32(JSON.stringify(uploadDataOptions))
+		).checksum;


Thank you for making this change 👍 @wuuxigh it does fix a sharp edge with current implementation.

On the other hand, we are going to add an option to disable this upload caching feature completely. Partially because of this sharp edge, and there are more sharp edges. We plan to have a more thorough refactor to how we cache the uploads.

For now I think this change is OK to be merged in, even though it will not be used by StorageBrowser.

cc @jimblanc

Ack, when that other change goes in we'll probably need to go through and make optionsHash optional on interfaces.

packages/storage/src/providers/s3/utils/getCombinedCrc32.ts

AllanZhengYP · 2024-09-27T00:16:28Z

packages/storage/src/providers/s3/apis/uploadData/multipart/initialUpload.ts

+		const finalCrc32 =
+			checksumAlgorithm === 'crc-32'
+				? await getCombinedCrc32(data, size)
+				: undefined;


Sanity check: do we have special error message for client-side hashing errors?

No new error messages were added in this PR.

AllanZhengYP · 2024-09-27T00:26:25Z

packages/storage/src/providers/s3/types/options.ts

+		/**
+		 * Indicates the algorithm used to create the checksum for the object.
+		 * This checksum can be used as a data integrity check to verify that the data received is the same data that was originally sent.
+		 * @default undefined


I think the decription is a bit verbose since integrity check means to verify that the data received is the same data that was originally sent.

Suggested change

/**

* Indicates the algorithm used to create the checksum for the object.

* This checksum can be used as a data integrity check to verify that the data received is the same data that was originally sent.

* @default undefined

/**

* Indicates the algorithm used to create the checksum for the object.

* The checksum created is for data integrity check by service-side.

* By default this checksum is not created.

* @default undefined

packages/storage/src/providers/s3/utils/crc32.ts

AllanZhengYP · 2024-09-27T00:28:20Z

packages/storage/src/providers/s3/utils/crc32.ts

 	new Uint8Array((hexString.match(/\w{2}/g)! ?? []).map(h => parseInt(h, 16)))
 		.buffer;

-const hexToBase64 = (hexString: string) =>
+export const hexToBase64 = (hexString: string) =>


Ditto, move this to own file.

jimblanc · 2024-09-27T14:57:33Z

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

@@ -126,7 +128,7 @@ export const getUploadsCacheKey = ({
 		levelStr = accessLevel === 'guest' ? 'public' : accessLevel;
 	}

-	const baseId = `${size}_${resolvedContentType}_${bucket}_${levelStr}_${key}`;
+	const baseId = `${optionsHash}_${size}_${resolvedContentType}_${bucket}_${levelStr}_${key}`;


Sanity check: What will happen here for any ongoing uploads when this change is released? Is the change backwards compatible?

It will not be backwards compatible and in progress uploads will need to start from scratch.

jimblanc · 2024-09-27T14:58:31Z

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

+		const optionsHash = (
+			await calculateContentCRC32(JSON.stringify(uploadDataOptions))
+		).checksum;


Ack, when that other change goes in we'll probably need to go through and make optionsHash optional on interfaces.

jimblanc · 2024-09-27T15:02:52Z

packages/storage/src/providers/s3/types/options.ts

+		/**
+		 * Indicates the algorithm used to create the checksum for the object.
+		 * This checksum can be used as a data integrity check to verify that the data received is the same data that was originally sent.
+		 * @default undefined


To build on what Allan suggested, I'd make it even less verbose:

Suggested change

/**

* Indicates the algorithm used to create the checksum for the object.

* This checksum can be used as a data integrity check to verify that the data received is the same data that was originally sent.

* @default undefined

/**

* The algorithm used to compute a checksum for the object. Used to verify that the data received by S3

* matches what was originally sent. Disabled by default.

*

* @default undefined

jimblanc · 2024-09-27T15:08:56Z

packages/storage/src/providers/s3/utils/getCombinedCrc32.native.ts

+		offset += crc32Hash.length;
+	}
+
+	return `${(await calculateContentCRC32(combinedArray.buffer)).checksum}-${crc32List.length}`;


Probably a stupid question, but why is the length appended to the hash?

Length is needed to indicate that this is a combined checksum expected by s3.

AllanZhengYP

Looks good to me!

AllanZhengYP

Looks good to me! Thank you for adding this.

…)" This reverts commit 02cb08a.

This reverts commit 02cb08a.

wuuxigh added 9 commits September 16, 2024 12:04

feat: opt in checksum

7d2cff9

fix: revert local prettier suggestion

8689812

fix: up size limit for storage upload data

01b3bd5

feat: react native crc32

92bbe33

fix: up bundle size limit and fix typo

bc6ce6b

feat: add documentation for checksumAlgorithm

3cae42c

Merge branch 'staging' into opt-in-checksum

e75e313

Merge branch 'staging' into opt-in-checksum

0786145

fix: update bundle size limit

ebd6109

wuuxigh requested review from a team as code owners September 24, 2024 23:46

wuuxigh changed the title ~~Feat: opt in checksum with optional checksum algorithm for upload~~ feat: optional checksum algorithm for upload Sep 24, 2024

fix: update bundle size limit

8d721e2

AllanZhengYP reviewed Sep 27, 2024

View reviewed changes

jimblanc reviewed Sep 27, 2024

View reviewed changes

ashwinkumar6 mentioned this pull request Oct 2, 2024

fix: multipart upload storage crc32 integrity #13878

Merged

6 tasks

fix: address pr feedbacks

f951902

AllanZhengYP previously approved these changes Oct 3, 2024

View reviewed changes

fix: bundle-size limit

b51a94d

wuuxigh dismissed AllanZhengYP’s stale review via b51a94d October 3, 2024 01:13

jimblanc previously approved these changes Oct 8, 2024

View reviewed changes

AllanZhengYP previously approved these changes Oct 9, 2024

View reviewed changes

Merge branch 'storage-browser/integrity' into opt-in-checksum

1c4034d

AllanZhengYP dismissed stale reviews from jimblanc and themself via 1c4034d October 9, 2024 17:38

jimblanc approved these changes Oct 9, 2024

View reviewed changes

AllanZhengYP approved these changes Oct 9, 2024

View reviewed changes

AllanZhengYP merged commit 02cb08a into aws-amplify:storage-browser/integrity Oct 9, 2024
28 checks passed

AllanZhengYP added a commit to AllanZhengYP/amplify-js that referenced this pull request Oct 10, 2024

Revert "feat: optional checksum algorithm for upload (aws-amplify#13849…

cde437a

…)" This reverts commit 02cb08a.

AllanZhengYP added a commit that referenced this pull request Oct 10, 2024

revert: optional checksum algorithm for upload (#13849) (#13910)

dbe5c7b

This reverts commit 02cb08a.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: optional checksum algorithm for upload #13849

feat: optional checksum algorithm for upload #13849

wuuxigh commented Sep 24, 2024 •

edited

Loading

AllanZhengYP Sep 27, 2024

jimblanc Sep 27, 2024

AllanZhengYP Sep 27, 2024

wuuxigh Oct 1, 2024

AllanZhengYP Sep 27, 2024

AllanZhengYP Sep 27, 2024

jimblanc Sep 27, 2024

wuuxigh Oct 1, 2024

jimblanc Sep 27, 2024

jimblanc Sep 27, 2024

jimblanc Sep 27, 2024

wuuxigh Oct 1, 2024

AllanZhengYP left a comment

AllanZhengYP left a comment

feat: optional checksum algorithm for upload #13849

feat: optional checksum algorithm for upload #13849

Conversation

wuuxigh commented Sep 24, 2024 • edited Loading

Description of changes

Issue #, if available

Description of how you validated changes

Checklist

Checklist for repo maintainers

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AllanZhengYP left a comment

Choose a reason for hiding this comment

AllanZhengYP left a comment

Choose a reason for hiding this comment

wuuxigh commented Sep 24, 2024 •

edited

Loading