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.

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

fix s3 key handling with trailing slash #9856

Merged
merged 3 commits into from Dec 12, 2023
Merged

fix s3 key handling with trailing slash #9856

merged 3 commits into from Dec 12, 2023

Conversation

bentsku
Copy link
Contributor

@bentsku bentsku commented Dec 12, 2023

Motivation

This would fix #9837. We've had multiple iterations of this, we were still unquoting the key to compare with the base_url but there was no need for it, I think this was a remnant of pre-Werkzeug 3.0

Changes

  • update the logic to be more resilient in the parser (proper splitting of the path, multiple trailing slashes...)
  • add more test cases

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

github-actions bot commented Dec 12, 2023

S3 Image Test Results (AMD64 / ARM64)

    2 files      2 suites   3m 7s ⏱️
381 tests 331 ✔️   50 💤 0
762 runs  662 ✔️ 100 💤 0

Results for commit 1b266de.

♻️ This comment has been updated with latest results.

@coveralls
Copy link

coveralls commented Dec 12, 2023

Coverage Status

coverage: 84.028% (+0.001%) from 84.027%
when pulling 1b266de on fix-s3-encoding
into d68c9de on master.

Copy link

github-actions bot commented Dec 12, 2023

LocalStack Community integration with Pro

       2 files  ±0         2 suites  ±0   1h 11m 18s ⏱️ -7s
2 404 tests +4  2 176 ✔️ +4  228 💤 ±0  0 ±0 
2 405 runs  +4  2 176 ✔️ +4  229 💤 ±0  0 ±0 

Results for commit 1b266de. ± Comparison against base commit d68c9de.

♻️ This comment has been updated with latest results.

Copy link
Member

@alexrashed alexrashed left a comment

Choose a reason for hiding this comment

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

Nice catch! Quite a tricky case! I only have two comments, when these are clarified we should be good to go! :) 🦸🏽

localstack/aws/protocol/parser.py Outdated Show resolved Hide resolved
localstack/aws/protocol/parser.py Show resolved Hide resolved
@bentsku bentsku merged commit e0bbe20 into master Dec 12, 2023
32 checks passed
@bentsku bentsku deleted the fix-s3-encoding branch December 12, 2023 21:22
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: S3 listObjects and folder names containing a space
3 participants