Skip to content

fix S3 List* pagination CommonPrefixes #9608

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
merged 1 commit into from
Nov 13, 2023
Merged

Conversation

bentsku
Copy link
Contributor

@bentsku bentsku commented Nov 12, 2023

Motivation

As reported in #9605, we were not handling properly the Max* argument when combined with Delimiter and Prefix, which returns CommonPrefixes.

Here's the description of the Delimiter parameter, which helps understand the behavior:

The delimiter grouping the included keys. A delimiter is a character that you specify to group keys. All keys that contain the same string between the prefix and the first occurrence of the delimiter are grouped under a single result element in CommonPrefixes. These groups are counted as one result against the max-keys limitation. These keys are not returned elsewhere in the response.

The behavior is most List* operations is pretty close, but has subtle changes between them.

Changes

  • Split all tests related to List*, especially around pagination, as those are quite verbose and the test_s3 file is becoming extremely huge (>10k lines) into its own file.
  • Create new tests validating this behavior for every List* operation except ListParts which doesn't have the CommonPrefix behavior.

EDIT: it was green before rebasing, pipeline is struggling (kclpy): https://app.circleci.com/pipelines/github/localstack/localstack/20034/workflows/a3805d65-c1b9-4708-a79b-f5ceed202830

Testing notes

I've also re-ran external test suites with this change and it's all green there as well.

@bentsku bentsku added aws:s3 Amazon Simple Storage Service semver: patch Non-breaking changes which can be included in patch releases labels Nov 12, 2023
@bentsku bentsku added this to the 3.0 milestone Nov 12, 2023
@bentsku bentsku self-assigned this Nov 12, 2023
Copy link

github-actions bot commented Nov 12, 2023

LocalStack Community integration with Pro

       2 files         2 suites   1h 20m 40s ⏱️
2 312 tests 2 013 ✔️ 299 💤 0
2 313 runs  2 013 ✔️ 300 💤 0

Results for commit d4827b0.

♻️ This comment has been updated with latest results.

@coveralls
Copy link

coveralls commented Nov 12, 2023

Coverage Status

coverage: 84.014% (+0.01%) from 84.004%
when pulling d4827b0 on fix-s3-common-prefixes
into 0750764 on master.

@bentsku bentsku marked this pull request as ready for review November 12, 2023 17:08
@bentsku bentsku requested a review from macnev2013 as a code owner November 12, 2023 17:08
@bentsku bentsku force-pushed the fix-s3-common-prefixes branch from ad25c04 to d4827b0 Compare November 13, 2023 13:07
Copy link
Contributor

@macnev2013 macnev2013 left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏻

@bentsku bentsku merged commit dfdf585 into master Nov 13, 2023
@bentsku bentsku deleted the fix-s3-common-prefixes branch November 13, 2023 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
aws:s3 Amazon Simple Storage Service 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: listing objects with delimiter, max-keys and continuation-token returns same CommonPrefixes multiple times
3 participants