feat(storage): Add API support for Expected Bucket Owner #13914
Conversation
feat(storage): add new internal downloadData api (aws-amplify#13887)
joon-won
requested review from
haverchuck,
cshfang,
jimblanc,
HuiSF and
a team
as code owners
…y/expected-bucket-owner rebase
There was a problem hiding this comment.
Left some questions above. This change still misses unit tests.
We have dedicated funcational test for custom API handlers, can you add them as well? link
packages/storage/src/providers/s3/utils/resolveS3ConfigAndInput.ts
Outdated
Show resolved
Hide resolved
packages/storage/src/providers/s3/utils/resolveS3ConfigAndInput.ts
Outdated
Show resolved
Hide resolved
| return { | ||
| accountID, | ||
| }; |
There was a problem hiding this comment.
Usually a validator function does not return anything. The validateStorageOperationInput is special since we don't have better way to meeting the requirement than to return something. If you take a look at other validators in this folder, they don't return anything.
There was a problem hiding this comment.
I see, I've included bucketownervalidator in validateStorageOperationInput since it might be the place we consolidate validators, that's why it also resembled its behavior. I'll update it not to return any
packages/storage/src/providers/s3/utils/validateStorageOperationInput.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Missing other internals APIs?
There was a problem hiding this comment.
With current plan, Jim will work on internals I think next week
| ...assignStringVariables({ 'x-amz-checksum-crc32': input.ChecksumCRC32 }), | ||
| ...assignStringVariables({ | ||
| 'x-amz-checksum-crc32': input.ChecksumCRC32, | ||
| 'x-amz-expected-bucket-owner': input.ExpectedBucketOwner, |
There was a problem hiding this comment.
Sanity check: What happens here if expected bucket owner is missing (since its optional)? Does the header still get sent with a null/undefined value?
There was a problem hiding this comment.
Testing from my E2E app, the header won't be included when there is no expectedBucketOwner set up thus/or receives null/undefined for it
packages/storage/src/providers/s3/utils/client/s3data/listObjectsV2.ts
Outdated
Show resolved
Hide resolved
packages/storage/src/providers/s3/utils/client/s3data/putObject.ts
Outdated
Show resolved
Hide resolved
packages/storage/src/providers/s3/utils/client/s3data/uploadPart.ts
Outdated
Show resolved
Hide resolved
packages/storage/src/providers/s3/utils/validateBucketOwnerID.ts
Outdated
Show resolved
Hide resolved
fix(storage): internals list function not able to decide which output…
packages/storage/src/providers/s3/apis/internal/uploadData/multipart/uploadHandlers.ts
Show resolved
Hide resolved
packages/storage/src/providers/s3/utils/client/s3data/abortMultipartUpload.ts
Show resolved
Hide resolved
joon-won
merged commit 96c2dc7
into
aws-amplify:storage-browser/integrity
Description of changes
expectedBucketOwneroptionUnit tests will be included in a separate PR
Issue #, 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.