Skip to content

Percent decode the list files when parsing response#93

Open
irevoire wants to merge 2 commits into
paolobarbolini:mainfrom
irevoire:percent-decode-list-files
Open

Percent decode the list files when parsing response#93
irevoire wants to merge 2 commits into
paolobarbolini:mainfrom
irevoire:percent-decode-list-files

Conversation

@irevoire
Copy link
Copy Markdown
Contributor

@irevoire irevoire commented Oct 12, 2023

Hello, while trying out the ListObjectsV2::parse_response command I noticed that we were not percent decoding the keys as documented here: https://docs.aws.amazon.com/AmazonS3/latest/API/API_ListObjectsV2.html#API_ListObjectsV2_RequestSyntax

I had no issue with minio as I don't use any strange characters. But the Digital Ocean S3 implementation does percent-encode the / character, which completely broke my code.
I updated my lib, but I think it should be handled by rusty_s3 directly, personally 🤔

What do you think?

@codecov
Copy link
Copy Markdown

codecov Bot commented Oct 12, 2023

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.31%. Comparing base (b31692f) to head (8308fc9).
⚠️ Report is 77 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #93      +/-   ##
==========================================
+ Coverage   90.23%   90.31%   +0.07%     
==========================================
  Files          29       29              
  Lines        1372     1383      +11     
==========================================
+ Hits         1238     1249      +11     
  Misses        134      134              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Owner

@paolobarbolini paolobarbolini 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 the PR. I had completely missed the fact they are encoding the keys.

// #[serde(rename = "EncodingType")]
// encoding_type: String,
#[serde(rename = "EncodingType")]
encoding_type: String,
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I'm wandering whether this should be an enum or not

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Like an Option<String>?

Since there is an official S3 specification, as far as I know, I have no idea.
What I can say is that DO, minio, and was all return the encoding_type set to url. But maybe some other providers don't.

I would say we keep it like that until someone gives us an implementation that doesn't work like that personally 😅

Comment thread src/actions/list_objects_v2.rs Outdated
Comment thread src/actions/list_objects_v2.rs Outdated
@irevoire
Copy link
Copy Markdown
Contributor Author

irevoire commented Oct 23, 2023

Hey, sorry I was on holiday last week.
I applied both your comment and @Kerollmops review comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants