Skip to content

Added pagination and filtering for s3 list buckets operation #12609

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.

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

Merged

Conversation

bryansan-local
Copy link
Contributor

@bryansan-local bryansan-local commented May 12, 2025

Motivation

Right now, the list buckets operation is ignoring the Prefix, MaxBuckets, BucketRegion and ContinuationToken which results in the operation always returning all buckets.

Closes #12585

Changes

  • Process correctly Prefix, MaxBuckets, BucketRegion and ContinuationToken to filter and paginate accordingly.
  • Added integration tests + parity validations for these scenarios

@bryansan-local bryansan-local self-assigned this May 12, 2025
@localstack-bot
Copy link
Collaborator

localstack-bot commented May 12, 2025

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@bryansan-local bryansan-local changed the title Added support for pagination and filtering in s3 bucket listing. Adde… Added pagination and filtering for s3 list buckets operation May 12, 2025
@bryansan-local bryansan-local added the semver: patch Non-breaking changes which can be included in patch releases label May 12, 2025
@bryansan-local
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

localstack-bot added a commit that referenced this pull request May 12, 2025
Copy link

github-actions bot commented May 12, 2025

S3 Image Test Results (AMD64 / ARM64)

  2 files    2 suites   8m 41s ⏱️
493 tests 443 ✅  50 💤 0 ❌
986 runs  886 ✅ 100 💤 0 ❌

Results for commit 3c33f88.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented May 12, 2025

LocalStack Community integration with Pro

    2 files  ±    0      2 suites  ±0   1h 2m 13s ⏱️ - 40m 16s
1 800 tests  - 2 625  1 575 ✅  - 2 467  225 💤  - 158  0 ❌ ±0 
1 802 runs   - 2 625  1 575 ✅  - 2 467  227 💤  - 158  0 ❌ ±0 

Results for commit 3c33f88. ± Comparison against base commit b961fee.

This pull request removes 2630 and adds 5 tests. Note that renamed tests count towards both.
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_lambda_dynamodb
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_opensearch_crud
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_search_books
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_setup
tests.aws.scenario.kinesis_firehose.test_kinesis_firehose.TestKinesisFirehoseScenario ‑ test_kinesis_firehose_s3
tests.aws.scenario.lambda_destination.test_lambda_destination_scenario.TestLambdaDestinationScenario ‑ test_destination_sns
tests.aws.scenario.lambda_destination.test_lambda_destination_scenario.TestLambdaDestinationScenario ‑ test_infra
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_prefill_dynamodb_table
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_stepfunctions_input_recipient_list[step_function_input0-SUCCEEDED]
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_stepfunctions_input_recipient_list[step_function_input1-SUCCEEDED]
…
tests.aws.services.s3.test_s3_list_operations.TestS3ListBuckets ‑ test_list_buckets_by_bucket_region
tests.aws.services.s3.test_s3_list_operations.TestS3ListBuckets ‑ test_list_buckets_by_prefix_with_case_sensitivity
tests.aws.services.s3.test_s3_list_operations.TestS3ListBuckets ‑ test_list_buckets_when_continuation_token_is_empty
tests.aws.services.s3.test_s3_list_operations.TestS3ListBuckets ‑ test_list_buckets_with_continuation_token
tests.aws.services.s3.test_s3_list_operations.TestS3ListBuckets ‑ test_list_buckets_with_max_buckets

♻️ This comment has been updated with latest results.

@bryansan-local bryansan-local requested a review from alexrashed May 12, 2025 19:07
@bryansan-local bryansan-local marked this pull request as ready for review May 13, 2025 11:40
@bryansan-local bryansan-local requested a review from bentsku as a code owner May 13, 2025 11:40
Copy link
Contributor

@bentsku bentsku left a comment

Choose a reason for hiding this comment

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

Awesome, great work, this is a nice addition!

I have a few comments, one blocking regarding the snapshot transformations of fields that are changing between every runs.

There's also a case that seems to be missing: when the continuation token provided is an empty string.

Once these are addressed, we're good to go 🚀 nice work!

Comment on lines 584 to 588
if continuation_token == "":
raise InvalidArgument(
"The continuation token provided is incorrect",
ArgumentName="continuation-token",
)
Copy link
Contributor

Choose a reason for hiding this comment

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

it seems we do not have a snapshot test covering this, it would be great to add a snapshot validated test for it

Copy link
Contributor Author

@bryansan-local bryansan-local May 13, 2025

Choose a reason for hiding this comment

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

I have checked what AWS does with this empty continuation token scenario and it turns out that it allows it instead of throwing an error. Thus, I have removed this raise error code block and created a test to validate that this implementation accepts and ignores an empty continuation token. Let me know what you think about it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bentsku is this fine for you? Do you agree?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is good, thanks for checking this! 👍

Copy link
Contributor

@bentsku bentsku left a comment

Choose a reason for hiding this comment

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

Thanks for addressing the comments! This looks good 👌 I've answered the BucketRegion negative case question, and after that we should be good to go!
Seems like the test failures are unrelated, and linked to the Transcribe service?

@bryansan-local
Copy link
Contributor Author

Thanks for addressing the comments! This looks good 👌 I've answered the BucketRegion negative case question, and after that we should be good to go! Seems like the test failures are unrelated, and linked to the Transcribe service?

Yea, no idea at all why those unrelated tests are failing :(

@bryansan-local bryansan-local marked this pull request as draft May 14, 2025 09:18
@bryansan-local bryansan-local marked this pull request as ready for review May 14, 2025 09:54
@bryansan-local bryansan-local requested a review from bentsku May 14, 2025 09:54
Copy link
Contributor

@bentsku bentsku left a comment

Choose a reason for hiding this comment

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

LGTM! This is great, thanks for taking the time to address the comments, this looks awesome! 💯

Also, thank you for taking the time to properly transform the region field, making sure the tests are always passing even with non-default regions.

Let's ship it! :shipit:

snapshot.match("list-objects-with-empty-continuation-token", response)

@markers.aws.validated
# In some regions AWS returns the Owner display name (us-west-2) but in some it doesnt (eu-central-1)
Copy link
Contributor

Choose a reason for hiding this comment

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

nice comment, thanks for adding it! 👌

@bryansan-local bryansan-local merged commit 2f84418 into master May 14, 2025
39 checks passed
@bryansan-local bryansan-local deleted the feature/pagination_and_filtering_in_s3_bucket_listing branch May 14, 2025 15:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: aws java sdk v1 seems to not be able to retrieve region from localstack S3 bucket
3 participants