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

AWS S3: Allow source keys with non-ASCII characters #2270

Merged
merged 5 commits into from
Apr 27, 2020

Conversation

mpdn
Copy link
Contributor

@mpdn mpdn commented Apr 16, 2020

Adds missing URL encoding for source keys when copying.

@mpdn mpdn marked this pull request as ready for review April 16, 2020 09:46
@mpdn mpdn changed the title Fix bug where copy failed for source keys with special characters S3: Fix bug where copy failed for source keys with special characters Apr 16, 2020
Copy link
Member

@ennru ennru left a comment

Choose a reason for hiding this comment

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

Thank you for this fix.
Just a minor thing to update.

@ennru ennru added this to the 2.0.0 milestone Apr 24, 2020
Co-Authored-By: Enno <458526+ennru@users.noreply.github.com>
@ennru
Copy link
Member

ennru commented Apr 24, 2020

Some tests fail because of this change as the / now gets encoded.
https://travis-ci.com/github/akka/alpakka/jobs/322886269#L605

@mpdn
Copy link
Contributor Author

mpdn commented Apr 24, 2020

I corrected HttpRequestSpec to assume urlencoded keys. Alternatively, we could have chosen not to urlencode /, but this seems like a simpler solution. There shouldn't be any problem with always urlencoding /.

@ennru
Copy link
Member

ennru commented Apr 24, 2020

Please run scalafmtAll.

@ennru
Copy link
Member

ennru commented Apr 24, 2020

There is another test failure
https://travis-ci.com/github/akka/alpakka/jobs/323013018#L675

@mpdn
Copy link
Contributor Author

mpdn commented Apr 27, 2020

That seems odd. I suspect that might actually be a bug in MinIO as it seems like the space is causing problems. Anyway, just removing the space makes the test work at least. Alpakka still supports more characters than before, and AFAIK this should work against S3.

Copy link
Member

@ennru ennru left a comment

Choose a reason for hiding this comment

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

LGTM.

@ennru ennru changed the title S3: Fix bug where copy failed for source keys with special characters AWS S3: Allow source keys with non-ASCII characters Apr 27, 2020
@ennru ennru merged commit 8384ae2 into akka:master Apr 27, 2020
@ennru
Copy link
Member

ennru commented Apr 27, 2020

Thank you, great fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants