feat(storage): internal GetProperties API #13869

AllanZhengYP · 2024-09-30T23:10:14Z

Description of changes

Following changes are made as part of this PR:

Remove the locationCredentialsProvider option from the public API input options' common interface:
Add a separate getProperties API exported from @aws-amplify/storage/internals path that contains locationCredentialsProvider option. The resolving logic is unchanged.

Issue #, if available

Description of how you validated changes

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.

…Is and add it to separate internal API; Internal getProperties implementation

packages/storage/src/internals/apis/getProperties.ts

packages/storage/src/internals/index.ts

packages/storage/src/internals/types/inputs.ts

packages/storage/src/providers/s3/apis/internal/copy.ts

packages/storage/src/providers/s3/apis/internal/getProperties.ts

packages/storage/src/internals/types/inputs.ts

Co-authored-by: Jim Blanchard <[email protected]>

packages/storage/__tests__/internals/apis/getProperties.test.ts

Co-authored-by: Jim Blanchard <[email protected]>

Samaritan1011001 · 2024-10-01T20:33:53Z

packages/storage/src/internals/types/inputs.ts

+				path: InputType['path'];
+				options?: PublicInputOptionsType & ExtendedOptionsType;
+			}
+		: InputType extends StorageCopyInputWithPath


It always falls back to check this Copy Input type StorageCopyInputWithPath? Isn't this type ExtendInputWithAdvancedOptions a generic one used for multiple APIs?

Yeah copy API signature is different any other Storage APIs. That's why I check the copy input interface explicitly. Here copy is the only API that does not extend from StorageOperationInputWithPath

Can we split the copy logic out and explicitly define it when it comes to copy? Will make the whole thing read much simpler IMO

ashika112 · 2024-10-01T20:51:39Z

packages/storage/__tests__/internals/apis/getProperties.test.ts

+jest.mock('../../../src/providers/s3/apis/internal/getProperties');
+const mockedGetPropertiesInternal = jest.mocked(getPropertiesInternal);
+
+describe('getProperties (internal)', () => {


this is getting confusing 😅

packages/storage/src/internals/apis/getProperties.ts

ashika112 · 2024-10-01T20:59:43Z

packages/storage/src/providers/s3/apis/internal/copy.ts

@@ -44,7 +58,7 @@ const storageBucketAssertion = (

 export const copy = async (
 	amplify: AmplifyClassV6,
-	input: CopyInput | CopyWithPathInput,
+	input: CopyInput | InputWithPathAndAdvancedOptions,


why not CopyWithPathInputAndAdvancedOptions?

Same for other places as well.

ashika112 · 2024-10-01T21:05:19Z

packages/storage/src/internals/types/inputs.ts

+/**
+ * @internal
+ */
+export type GetPropertiesInput = ExtendInputWithAdvancedOptions<


Im confused here. Why are we creating this and not using this type? but rather have a duplicate again here?

+1, we should keep types as simple as we can, and avoid type programming as much as we can 😅 when we deprecating the key use cases, it should just simple splitting and deleting.

@ashika112 I think I was debating all the different conventions, GetPropertiesInput name cannot be referred as-is in the other places. I can change to use this definition everywhere.

ashika112 · 2024-10-01T21:09:12Z

packages/storage/src/internals/types/inputs.ts

+				path: InputType['path'];
+				options?: PublicInputOptionsType & ExtendedOptionsType;
+			}
+		: InputType extends StorageCopyInputWithPath


Can we split the copy logic out and explicitly define it when it comes to copy? Will make the whole thing read much simpler IMO

packages/storage/src/internals/types/inputs.ts

Samaritan1011001 · 2024-10-01T22:09:22Z

packages/storage/src/internals/types/inputs.ts

+	InputType extends StorageOperationInputWithPath &
+		StorageOperationOptionsInput<infer PublicInputOptionsType>
+		? {
+				path: InputType['path'];


What about key? Not supporting it here?

No, we only support Gen2 signature with extended option.

packages/storage/__tests__/internals/apis/getProperties.test.ts

HuiSF · 2024-10-01T22:55:13Z

packages/storage/src/internals/types/inputs.ts

+/**
+ * @internal
+ */
+export type GetPropertiesInput = ExtendInputWithAdvancedOptions<


+1, we should keep types as simple as we can, and avoid type programming as much as we can 😅 when we deprecating the key use cases, it should just simple splitting and deleting.

jimblanc

LGTM

ashwinkumar6 · 2024-10-01T18:26:54Z

packages/storage/src/providers/s3/apis/internal/copy.ts

@@ -21,6 +25,16 @@ import { assertValidationError } from '../../../../errors/utils/assertValidation
 import { copyObject } from '../../utils/client/s3data';
 import { getStorageUserAgentValue } from '../../utils/userAgent';
 import { logger } from '../../../../utils';
+// TODO: Remove this interface when we move to public advanced APIs.
+import { ExtendInputWithAdvancedOptions } from '../../../../internals/types/inputs';


thx love these TODO items

ashwinkumar6 · 2024-10-01T18:36:46Z

packages/storage/src/internals/apis/getProperties.ts

+export function getProperties(
+	input: GetPropertiesInput,
+): Promise<GetPropertiesOutput> {
+	return getPropertiesInternal(Amplify, {
+		path: input.path,
+		options: {
+			useAccelerateEndpoint: input?.options?.useAccelerateEndpoint,
+			bucket: input?.options?.bucket,
+			locationCredentialsProvider: input?.options?.locationCredentialsProvider,
+		},
+		// Type casting is necessary because `getPropertiesInternal` supports both Gen1 and Gen2 signatures, but here
+		// given in input can only be Gen2 signature, the return can only ben Gen2 signature.
+	}) as Promise<GetPropertiesOutput>;
+}


super nit,

Suggested change

export function getProperties(

input: GetPropertiesInput,

): Promise<GetPropertiesOutput> {

return getPropertiesInternal(Amplify, {

path: input.path,

options: {

useAccelerateEndpoint: input?.options?.useAccelerateEndpoint,

bucket: input?.options?.bucket,

locationCredentialsProvider: input?.options?.locationCredentialsProvider,

},

// Type casting is necessary because `getPropertiesInternal` supports both Gen1 and Gen2 signatures, but here

// given in input can only be Gen2 signature, the return can only ben Gen2 signature.

}) as Promise<GetPropertiesOutput>;

}

export const getProperties = (

input: GetPropertiesInput

): Promise<GetPropertiesOutput> =>

getPropertiesInternal(Amplify, {

path: input.path,

options: {

useAccelerateEndpoint: input?.options?.useAccelerateEndpoint,

bucket: input?.options?.bucket,

locationCredentialsProvider: input?.options?.locationCredentialsProvider,

},

// Type casting is necessary because `getPropertiesInternal` supports both Gen1 and Gen2 signatures, but here

// given in input can only be Gen2 signature, the return can only ben Gen2 signature.

}) as Promise<GetPropertiesOutput>;

feat(storage): remove locationCredentialsProvider option in public AP…

a3ef0af

…Is and add it to separate internal API; Internal getProperties implementation

AllanZhengYP changed the title ~~feat(storage): remove locationCredentialsProvider option in public AP…~~ feat(storage): remove locationCredsProvider option in public APIs Sep 30, 2024

AllanZhengYP changed the title ~~feat(storage): remove locationCredsProvider option in public APIs~~ feat(storage): move locationCredsProvider option from public APIs Sep 30, 2024

AllanZhengYP added 3 commits September 30, 2024 16:38

fix(storage): revert moving location credentials provider interface

d4ed4bf

chore: add FIXME comments for post-GA changes

6f1b4bb

chore: remove unused TemporaryCredentialsProvider

e3c517a

jimblanc reviewed Oct 1, 2024

View reviewed changes

calebpollman reviewed Oct 1, 2024

View reviewed changes

packages/storage/src/internals/types/inputs.ts Outdated Show resolved Hide resolved

chore: address feedbacks

0c367c0

Co-authored-by: Jim Blanchard <[email protected]>

AllanZhengYP force-pushed the storage-browser/internal-get-properties branch from 3239e6b to 0c367c0 Compare October 1, 2024 17:10

test: add internal getProperties unit test

7860519

AllanZhengYP force-pushed the storage-browser/internal-get-properties branch from 5814597 to 7860519 Compare October 1, 2024 17:48

AllanZhengYP marked this pull request as ready for review October 1, 2024 17:49

AllanZhengYP requested review from ukhan-amazon, haverchuck, cshfang, HuiSF and a team as code owners October 1, 2024 17:49

AllanZhengYP requested review from jimblanc and calebpollman October 1, 2024 18:02

AllanZhengYP changed the title ~~feat(storage): move locationCredsProvider option from public APIs~~ feat(storage): internal GetProperties API Oct 1, 2024

jimblanc previously approved these changes Oct 1, 2024

View reviewed changes

packages/storage/__tests__/internals/apis/getProperties.test.ts Outdated Show resolved Hide resolved

chore: address feedbacks

7fe5590

Co-authored-by: Jim Blanchard <[email protected]>

AllanZhengYP dismissed jimblanc’s stale review via 7fe5590 October 1, 2024 18:40

AllanZhengYP requested a review from jimblanc October 1, 2024 18:40

Samaritan1011001 reviewed Oct 1, 2024

View reviewed changes

ashika112 reviewed Oct 1, 2024

View reviewed changes

Samaritan1011001 reviewed Oct 1, 2024

View reviewed changes

HuiSF reviewed Oct 1, 2024

View reviewed changes

chore: address more feedbacks

ba41b0a

jimblanc approved these changes Oct 2, 2024

View reviewed changes

HuiSF approved these changes Oct 2, 2024

View reviewed changes

ashika112 approved these changes Oct 2, 2024

View reviewed changes

ashwinkumar6 approved these changes Oct 2, 2024

View reviewed changes

AllanZhengYP merged commit 287d0e5 into aws-amplify:storage-browser/integrity Oct 2, 2024
28 checks passed

AllanZhengYP deleted the storage-browser/internal-get-properties branch October 2, 2024 20:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(storage): internal GetProperties API #13869

feat(storage): internal GetProperties API #13869

AllanZhengYP commented Sep 30, 2024 •

edited

Loading

Samaritan1011001 Oct 1, 2024

AllanZhengYP Oct 1, 2024

ashika112 Oct 1, 2024

ashika112 Oct 1, 2024

ashika112 Oct 1, 2024

ashika112 Oct 1, 2024

ashika112 Oct 1, 2024

HuiSF Oct 1, 2024

AllanZhengYP Oct 2, 2024 •

edited

Loading

ashika112 Oct 1, 2024

Samaritan1011001 Oct 1, 2024

AllanZhengYP Oct 2, 2024

HuiSF Oct 1, 2024

jimblanc left a comment

ashwinkumar6 Oct 1, 2024

ashwinkumar6 Oct 1, 2024

feat(storage): internal GetProperties API #13869

feat(storage): internal GetProperties API #13869

Conversation

AllanZhengYP commented Sep 30, 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

AllanZhengYP Oct 2, 2024 • edited Loading

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

jimblanc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AllanZhengYP commented Sep 30, 2024 •

edited

Loading

AllanZhengYP Oct 2, 2024 •

edited

Loading