Skip to content

fix ListObjectVersions pagination comparison #12270

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
Merged
merged 2 commits into from
Feb 14, 2025
Merged

fix ListObjectVersions pagination comparison #12270

merged 2 commits into from
Feb 14, 2025

Conversation

bentsku
Copy link
Contributor

@bentsku bentsku commented Feb 13, 2025 •
edited
Loading

Motivation

Follow up from #12259, I've introduced flakiness in the tests and bad behavior (also reported). I saw the failed run here: https://github.com/localstack/localstack/actions/runs/13314341024/job/37184566141 and realized my mistake instantly 🤦‍♂️

I incorrectly assumed the b64 encoded data could be sorted lexicographically, but this was wrong. The increasing values are cycling from upper case, lower case, digits, back to upper case. However, there is one comparison that fails: z to 0.

This is because if we look at the proper Unicode code point for the characters, we go from y, z, 0, 1 to -> 49, 48, 122, 121. So we break the order at this one character. (if they had started with digits, things would have been fine 😝)

I've introduced a small utility function to properly compare version between each other, maybe using hexadecimal characters could have been fine, but who knows what people does with VersionId field 😅 keeping the format as close as possible to AWS might be preferable.

Also introduced unit tests for it, and an integration test to validate the paginator behavior.

Changes

  • fix the VersionId comparison to properly look at the raw data instead of the encoded one
  • add unit test for behavior
  • add integration test

fixes #12255

@bentsku bentsku added aws:s3 Amazon Simple Storage Service semver: patch Non-breaking changes which can be included in patch releases labels Feb 13, 2025
@bentsku bentsku added this to the 4.2 milestone Feb 13, 2025
@bentsku bentsku requested a review from k-a-il February 13, 2025 20:25
@bentsku bentsku self-assigned this Feb 13, 2025
@github-actions GitHub Actions
Copy link

S3 Image Test Results (AMD64 / ARM64)

  2 files  ±0    2 suites  ±0   8m 29s ⏱️ +44s
478 tests +1  426 ✅ +1   52 💤 ±0  0 ❌ ±0 
956 runs  +2  852 ✅ +2  104 💤 ±0  0 ❌ ±0 

Results for commit b2e34d8. ± Comparison against base commit 904f55a.

@github-actions GitHub Actions
Copy link

LocalStack Community integration with Pro

    2 files  ±    0      2 suites  ±0   1h 1m 19s ⏱️ - 51m 34s
1 719 tests  - 2 379  1 542 ✅  - 2 225  177 💤  - 154  0 ❌ ±0 
1 721 runs   - 2 379  1 542 ✅  - 2 225  179 💤  - 154  0 ❌ ±0 

Results for commit b2e34d8. ± Comparison against base commit 904f55a.

This pull request removes 2380 and adds 1 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.TestS3ListObjectVersions ‑ test_list_objects_versions_with_prefix_only_and_pagination_many_versions

@bentsku bentsku requested a review from cloutierMat February 14, 2025 09:18
cloutierMat
cloutierMat approved these changes Feb 14, 2025
View reviewed changes
Copy link
Contributor

@cloutierMat cloutierMat left a comment

Choose a reason for hiding this comment

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

Awesome! Thanks for the super proactive fix. 🙏 for keeping the pipeline 💚!

The naming is_version_older_than_other feels a bit off to me, but it might just be a Friday morning vibe! 🤣

@bentsku bentsku merged commit c3866e1 into master Feb 14, 2025
53 checks passed
@bentsku bentsku deleted the fix-s3-versions-pagination-again branch February 14, 2025 09:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cloutierMat cloutierMat cloutierMat approved these changes

@k-a-il k-a-il Awaiting requested review from k-a-il

Assignees

@bentsku bentsku

Labels
aws:s3 Amazon Simple Storage Service semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Milestone
4.2
Development

Successfully merging this pull request may close these issues.

bug: Deleting versions while listing them is broken since 3.0.0
2 participants
@bentsku @cloutierMat