Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(storage): add delimiter support to list API #13517

Merged
merged 10 commits into from
Jul 8, 2024
Merged

feat(storage): add delimiter support to list API #13517

merged 10 commits into from
Jul 8, 2024

Conversation

israx
Copy link
Member

@israx israx commented Jun 19, 2024 •
edited
Loading

Description of changes

This change will enable delimiter support by passing a Delimiter option in the underlying ListObjectsV2 API.

Main changes to list API:

  • add subpathStrategy to input
  • add excludedSubpaths to output

Issue #, if available

Description of how you validated changes

  • unit tests
  • smoke tests
  • e2e test is pending

Checklist

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

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

@israx israx changed the title feat(storage): add delimiter option to list API feat(storage): add delimiter support to list API Jun 21, 2024
@israx israx marked this pull request as ready for review June 21, 2024 19:55
ashika112
ashika112 reviewed Jun 21, 2024
View reviewed changes
packages/storage/src/types/outputs.ts Outdated Show resolved Hide resolved
packages/storage/src/types/options.ts Outdated Show resolved Hide resolved
packages/storage/src/types/options.ts Outdated Show resolved Hide resolved
packages/storage/src/providers/s3/apis/internal/list.ts Outdated Show resolved Hide resolved
packages/storage/src/providers/s3/apis/internal/list.ts Outdated Show resolved Hide resolved
HuiSF
HuiSF reviewed Jun 21, 2024
View reviewed changes
packages/storage/src/types/options.ts Outdated Show resolved Hide resolved
packages/storage/src/types/options.ts Outdated Show resolved Hide resolved
packages/storage/src/providers/s3/apis/internal/list.ts Outdated Show resolved Hide resolved
packages/storage/src/providers/s3/apis/internal/list.ts
@@ -79,13 +81,15 @@ export const list = async (
Prefix: isInputWithPrefix ? `${generatedPrefix}${objectKey}` : objectKey,
MaxKeys: options?.listAll ? undefined : options?.pageSize,
ContinuationToken: options?.listAll ? undefined : options?.nextToken,
Delimiter: getDelimiter(options.subpathStrategy),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What will be the value for the delimiter query parameter when the getDelimiter() returns undefined?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the looks of it, undefined seems to passed to the request too.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah undefined is passed when subpathStrategy is undefined or include

Copy link
Member

@HuiSF HuiSF Jun 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have an signed url example for this API call?
Is it going to be something like the following?

/?list-type=2&continuation-token=ContinuationToken&delimiter=undefined

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: i know its connivence but i think the intend is clear if we add delimiter only when we need it into the listInputArgs

Copy link
Member Author

@israx israx Jun 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not included. Bellow I attached the examples. Btw the ListObjectV2 API is already taking undefined values.

  • Delimiter receives an undefined value.
Screenshot 2024-06-25 at 2 37 15 PM
  • Delimiter receives a value.
Screenshot 2024-06-25 at 2 41 14 PM 1

jimblanc
jimblanc reviewed Jun 24, 2024
View reviewed changes
packages/storage/src/providers/s3/apis/internal/list.ts Outdated Show resolved Hide resolved
packages/storage/src/providers/s3/apis/internal/list.ts Outdated Show resolved Hide resolved
packages/storage/src/types/options.ts Outdated Show resolved Hide resolved
packages/storage/src/types/options.ts Outdated Show resolved Hide resolved
packages/storage/src/types/options.ts
* that are delimited by the `/` character.
*
*
* @example
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm torn if these examples should go on the API definition for discoverability (particularly on API docs), or if the type is sufficient. Wdyt (I haven't tested how it manifests in the IDE)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving these examples on the type should be sufficient. They will manifest when the API is used.
Screenshot 2024-06-25 at 10 15 28 AM

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if someone doesn't use a ts enabled IDE :D

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

our official docs will be the best option then : )

packages/storage/src/types/options.ts Outdated Show resolved Hide resolved
@israx israx changed the title feat(storage): add delimiter support to list API [DO NOT MERGE]-feat(storage): add delimiter support to list API Jun 25, 2024
@israx israx changed the title [DO NOT MERGE]-feat(storage): add delimiter support to list API feat(storage): add delimiter support to list API Jun 25, 2024
jimblanc
jimblanc previously approved these changes Jun 25, 2024
View reviewed changes
Copy link
Contributor

@jimblanc jimblanc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, one nit

packages/storage/src/providers/s3/apis/internal/list.ts Outdated Show resolved Hide resolved
@haverchuck
Copy link
Contributor

Do we have tests for server side usage?

@404ghosting
Copy link

@ashwinkumar6 please provide your valuable feedback

HuiSF
HuiSF previously approved these changes Jul 1, 2024
View reviewed changes
ashika112
ashika112 approved these changes Jul 1, 2024
View reviewed changes
Copy link
Member

@ashika112 ashika112 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @israx Great work on this 🎉

@israx israx merged commit 95ba49c into aws-amplify:main Jul 8, 2024
30 checks passed
@israx israx mentioned this pull request Jul 17, 2024
Merged
6 tasks
jjarvisp added a commit to jjarvisp/amplify-js that referenced this pull request Jul 23, 2024
@jjarvisp
Squashed commit of the following:
563dd8f 
commit 381ca11
Author: ashika112 <[email protected]>
Date:   Tue Jul 23 13:01:22 2024 -0700

    update bundle size

commit df890f1
Merge: 043d913 b473ce3
Author: ashika112 <[email protected]>
Date:   Tue Jul 23 12:52:15 2024 -0700

    Merge branch 'main' of github.com:aws-amplify/amplify-js into feat/multi-bucket

commit b473ce3
Author: Ashwin Kumar <[email protected]>
Date:   Tue Jul 23 10:13:25 2024 -0700

    fix(rtn-web-browser): signInWithRedirect needs to be called twice on Android

    * fix(rtn-web-browser): signInWithRedirect needs to be called twice on Android

    * chore: add code owner

    * address feedback

    * update codeowners

    * update codeowners

    * Update .github/CODEOWNERS

    ---------

    Co-authored-by: Ashwin Kumar <[email protected]>

commit 3d70792
Author: israx <[email protected]>
Date:   Tue Jul 23 08:19:21 2024 -0400

    fix(storage): omit subPathStrategy when prefix is defined (aws-amplify#13618)

    * fix(storage): omit subPathStrategy when prefix is defined (aws-amplify#13606)

    * fix: omit subPathStrategy on prefix

    * chore: fix build

    * chore: address feedback

    * chore: omit subpathStrategy from options

    * chore: add unit tests

    * chore: update tests

    * chore: fix test

    * chore: move subpathstrategy to service options

    * chore: update comment

    * chore: fix type

commit 9fb5988
Merge: 60a559f 4c7e6ed
Author: AllanZhengYP <[email protected]>
Date:   Mon Jul 22 17:30:32 2024 -0700

    chore: Merge release into main (aws-amplify#13623)

commit 4c7e6ed
Author: aws-amplify-bot <[email protected]>
Date:   Mon Jul 22 22:38:34 2024 +0000

    chore(release): Update API docs [skip release]

commit 5c372cc
Author: aws-amplify-bot <[email protected]>
Date:   Mon Jul 22 22:37:14 2024 +0000

    chore(release): Publish [skip release]

     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - [email protected]
     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - [email protected]

commit 4efe9be
Author: aws-amplify-bot <[email protected]>
Date:   Mon Jul 22 22:37:01 2024 +0000

    chore(release): Set core metadata [skip release]

commit 9cc959a
Merge: 02abc66 60a559f
Author: AllanZhengYP <[email protected]>
Date:   Mon Jul 22 15:01:44 2024 -0700

    release(required): Amplify JS release (aws-amplify#13622)

commit 60a559f
Author: Hui Zhao <[email protected]>
Date:   Fri Jul 19 16:36:55 2024 -0700

    fix(aws-amplify): createKeyValueStorageFromCookieStorageAdapter misses default path and secure values (aws-amplify#13508)

    * fix(aws-amplify): createKeyValueStorageFromCookieStorageAdapter misses default path and secure values

    * Ensure Path is being serialized

    * Delete cookie without path attr before setting it

commit aa7ae18
Merge: 60885b7 02abc66
Author: AllanZhengYP <[email protected]>
Date:   Fri Jul 19 10:16:51 2024 -0700

    chore: Merge release into main (aws-amplify#13616)

commit 02abc66
Author: aws-amplify-bot <[email protected]>
Date:   Fri Jul 19 01:38:05 2024 +0000

    chore(release): Update API docs [skip release]

commit cae6274
Author: aws-amplify-bot <[email protected]>
Date:   Fri Jul 19 01:36:39 2024 +0000

    chore(release): Publish [skip release]

     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - [email protected]
     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - [email protected]

commit 71683a4
Author: aws-amplify-bot <[email protected]>
Date:   Fri Jul 19 01:36:24 2024 +0000

    chore(release): Set core metadata [skip release]

commit 880dfdc
Merge: dc2fc7b 60885b7
Author: AllanZhengYP <[email protected]>
Date:   Thu Jul 18 17:03:14 2024 -0700

    release: Amplify JS release (aws-amplify#13612)

commit 60885b7
Author: israx <[email protected]>
Date:   Thu Jul 18 17:28:30 2024 -0400

    Revert "fix(storage): omit subPathStrategy when prefix is defined (aws-amplify#13614)

    Revert "fix(storage): omit subPathStrategy when prefix is defined (aws-amplify#13606)"

    This reverts commit d3b9546.

commit d3b9546
Author: israx <[email protected]>
Date:   Thu Jul 18 14:31:27 2024 -0400

    fix(storage): omit subPathStrategy when prefix is defined (aws-amplify#13606)

    * fix: omit subPathStrategy on prefix

    * chore: fix build

    * chore: address feedback

commit af46e39
Author: Hui Zhao <[email protected]>
Date:   Wed Jul 17 12:15:58 2024 -0700

    fix(rtn-push-notification): wrong completion handler key (aws-amplify#13604)

commit 1cc40e3
Merge: e3a10ca 6f3e0d8
Author: Jim Blanchard <[email protected]>
Date:   Thu Jul 11 09:41:22 2024 -0500

    chore: Merge release into main (aws-amplify#13570)

commit 6f3e0d8
Merge: dc2fc7b e3a10ca
Author: Jim Blanchard <[email protected]>
Date:   Thu Jul 11 09:28:45 2024 -0500

    Merge branch 'main' into temp/release-merge-1720476144

commit e3a10ca
Author: James Jarvis <[email protected]>
Date:   Tue Jul 9 16:30:02 2024 -0400

    downgrade react-native-get-random-values from >=1.9.0 to >=1.8.0 (aws-amplify#13573)

    The purpose of this change is to downgrade the
    react-native-get-random-values peer dependency from >=1.9.0 to
    >=1.8.0.Expo 50 utilizes 1.8.0 as a dependency and amplify-js dependency
    of 1.9.0 produced installation friction when getting started via the
    recommended workflow. Dropping the peer dep version will remove
    installation friction while having no impact on current library
    functionality.

commit dc2fc7b
Author: aws-amplify-bot <[email protected]>
Date:   Mon Jul 8 22:05:06 2024 +0000

    chore(release): Update API docs [skip release]

commit 884f77a
Author: aws-amplify-bot <[email protected]>
Date:   Mon Jul 8 22:03:39 2024 +0000

    chore(release): Publish [skip release]

     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - [email protected]
     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - [email protected]

commit cf6fe30
Author: aws-amplify-bot <[email protected]>
Date:   Mon Jul 8 22:03:24 2024 +0000

    chore(release): Set core metadata [skip release]

commit 1d353e7
Merge: 8cc3fd5 0a07393
Author: Ashwin Kumar <[email protected]>
Date:   Mon Jul 8 14:14:12 2024 -0700

    release: Amplify JS release (aws-amplify#13569)

commit 0a07393
Author: israx <[email protected]>
Date:   Mon Jul 8 11:17:57 2024 -0400

    feat(storage): enable storage Next JS app (aws-amplify#13555)

    feat: enable storage next js app

commit 95ba49c
Author: israx <[email protected]>
Date:   Mon Jul 8 11:06:49 2024 -0400

    feat(storage): add delimiter support to list API (aws-amplify#13517)

    * feat: add types

    * feat: enable delimiter

    * chore: add unit tests

    * chore: bump bundle size

    * chore: add tsdocs

    * chore: address feedback

    * chore: address feedback

    * chore: address feedback

commit 0f5f4cb
Author: Rami Elwan <[email protected]>
Date:   Mon Jul 1 21:42:54 2024 +0200

    fix(datastore): handle case when getting auth info for expired token users (aws-amplify#13536)

    fix: handle case when getting auth info for expired token users

    Co-authored-by: Aaron S <[email protected]>

commit b6de5f9
Author: Hui Zhao <[email protected]>
Date:   Wed Jun 26 11:23:35 2024 -0700

    chore: enabling linting on __tests__ for the react-native packages (aws-amplify#13545)

    * chore: enabling linting on __tests__ for the react-native packages

    * chore(rtn-push-notification): run yarn lint:fix

    * chore(rtn-push-notification): manual fix linter errors

commit 2ef7c59
Author: Ashwin Kumar <[email protected]>
Date:   Tue Jun 25 18:00:35 2024 -0700

    chore: update PRs checklist to also add/update E2E tests (aws-amplify#13543)

    * chore: update PRs checklist to also add/update E2E tests

    * Update .github/PULL_REQUEST_TEMPLATE.md

    Co-authored-by: Hui Zhao <[email protected]>

    * address feedback

    ---------

    Co-authored-by: Ashwin Kumar <[email protected]>
    Co-authored-by: Hui Zhao <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ashika112 ashika112 ashika112 approved these changes

@haverchuck haverchuck haverchuck approved these changes

@HuiSF HuiSF HuiSF approved these changes

@jimblanc jimblanc jimblanc approved these changes

@ukhan-amazon ukhan-amazon Awaiting requested review from ukhan-amazon

@cshfang cshfang Awaiting requested review from cshfang cshfang is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@israx @haverchuck @404ghosting @jimblanc @HuiSF @ashika112