feat(storage): introduce preventOverwrite option to uploadData via HeadObject #13640
Conversation
There was a problem hiding this comment.
Thank you @eppjame!
The change looks good to me. I just have a few sanity check questions.
packages/storage/__tests__/providers/s3/apis/uploadData/putObjectJob.test.ts
Show resolved
Hide resolved
packages/storage/src/providers/s3/apis/uploadData/validateObjectNotExists.ts
Show resolved
Hide resolved
| @@ -107,6 +109,13 @@ export const getMultipartUploadHandlers = ( | |||
| resolvedAccessLevel = resolveAccessLevel(accessLevel); | |||
| } | |||
|
|
|||
| if (preventOverwrite) { | |||
There was a problem hiding this comment.
Does it make more sense to be called right before calling complete MPU API?
There was a problem hiding this comment.
Yes, that has a better chance of catching recent uploads. Updated.
| try { | ||
| await headObject(s3Config, input); | ||
|
|
||
| throw new StorageError({ |
There was a problem hiding this comment.
so this error gets thrown only when target key is already present? Can we be more clear on the error message? or is there more than that we check for here?
There was a problem hiding this comment.
I'll defer this quetion to @eppjame but one advantage of making the message generic is to prevent users to enumerate the resources and gain knowledge about the bucket.
| await headObject(s3Config, input); | ||
|
|
||
| throw new StorageError({ | ||
| name: 'PreconditionFailed', |
There was a problem hiding this comment.
Can we get this value as const value? — In case we are unit testing this specific error we would refer to the variable instead of the plain text.
AllanZhengYP
merged commit 0b1ec2e
into
aws-amplify:storage-browser/integrity
Description of changes
preventOverwrite, and invoke above validation when it is enabledIssue #, if available
Description of how you validated changes
Checklist
yarn testpassesChecklist for repo maintainers
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.