feat: add gen2 path parameter to getProperties and getUrl #13144
Conversation
erinleigh90
changed the title
packages/storage/__tests__/providers/s3/apis/getProperties.test.ts
Outdated
Show resolved
Hide resolved
packages/storage/__tests__/providers/s3/apis/getProperties.test.ts
Outdated
Show resolved
Hide resolved
packages/storage/__tests__/providers/s3/apis/getProperties.test.ts
Outdated
Show resolved
Hide resolved
|
Amazing work 🎉 |
Co-authored-by: ashika112 <155593080+ashika112@users.noreply.github.com>
| options, | ||
| ); | ||
| if (getUrlOptions?.validateObjectExistence) { | ||
| await getProperties( |
There was a problem hiding this comment.
I know this is existing code, but is the idea here that this will throw if there is no such object? Like why call it and not use it's response?
There was a problem hiding this comment.
Yes, that is the idea. Validated that it is working as expected, throwing an error if there is no such object
| /** | ||
| * Output type for S3 getProperties API. | ||
| */ | ||
| export type GetPropertiesOutput = ItemKey; | ||
| export type GetPropertiesOutput = |
There was a problem hiding this comment.
Do we need a StrictUnion here?
There was a problem hiding this comment.
Hmmm. StrictUnion on output types may not needed. Strict union is need mostly for DX validation. Output will have that through Overload that should be enough.
There was a problem hiding this comment.
nit question, Is this scenario possible
I think without StrictUnion, sometimes properties of key could be allowed in path. In case the internal getProperties API using this GetPropertiesOutput type by mistakes appends a keyProperty to both key and path would this be able to catch it
There was a problem hiding this comment.
We should be returning what we need for each scenario. Can validate in a unit test that we are not returning both in any scenario, but I don't see how the key could simply slip through in an if/else scenario.
| /** | ||
| * Output type for S3 getProperties API. | ||
| */ | ||
| export type GetPropertiesOutput = ItemKey; | ||
| export type GetPropertiesOutput = |
There was a problem hiding this comment.
nit question, Is this scenario possible
I think without StrictUnion, sometimes properties of key could be allowed in path. In case the internal getProperties API using this GetPropertiesOutput type by mistakes appends a keyProperty to both key and path would this be able to catch it
packages/storage/__tests__/providers/s3/apis/getProperties.test.ts
Outdated
Show resolved
Hide resolved
packages/storage/src/providers/s3/apis/internal/getProperties.ts
Outdated
Show resolved
Hide resolved
Description of changes
Adds support for gen2 path to getUrl and getProperties API's.
Issue #, if available
Description of how you validated changes
Checklist
yarn testpassesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.