Fixes url encoding for paths. #151
Conversation
Run the uri through URLEncoder.encode and then converting allowable characters :, / and [space] back. Taken from https://github.com/aws/aws-sdk-java/blob/master/aws-java-sdk-s3/src/main/java/com/amazonaws/services/s3/AmazonS3URI.java
| return URLEncoder.encode(uri, "UTF-8") | ||
| .replace("%3A", ":") | ||
| .replace("%2F", "/") | ||
| .replace("+", "%20"); |
There was a problem hiding this comment.
I have the feeling that there is a better way to do this. For URL-s there is java.net.URLDecoder. Perhaps we could do something similar? I mean -- in your code above, it would only catch three of the cases that should be decoded.
@ptirador , @steve-todorov , @BjoernAkAManf : Any better ideas?
There was a problem hiding this comment.
Years back when I originally submitted that PR, I took that code directly from the AWS SDK. The replacement back for : and / was because URLEncoder actually produces application/x-www-form-urlencoded which isn't exactly a URI but was the closest at the time. And I think there were times when S3Path.encode was called with the full absolute URI that contained the scheme (s3:) and everything else. So undoing that was necessary.
You are right though. There is an easier way. Spring has a UriUtils class that does just this. In fact, section 5 here says its a common mistake to encode more than just the path and that is where I got the suggestion.
Is it OK to add a dependency on Spring? If so, I'll redo this PR. It would effectively look like:
private String encode(String uri) { return UriUtils.encode(uri, "UTF-8"); }
There was a problem hiding this comment.
I've had a look at the code that you've linked back to in the AWS SDK and I see what you mean, but I think it will be quite a bit limited. Sure, it will work for the cases you're currently catching, but while we're doing this, we might as well do it right for all the other cases that need encoding/decoding as well.
We would like to keep this library as minimal, as possible. Adding a dependency to Spring would make it more complicated. We do intend to have Spring-specific code later on, but in a separate "add-on" module (we'll have a "core" one as well).
If we could find a more suitable lightweight library that can handle this sort of encoding/decoding, this would be great!
We do have com.google.guava:guava:jar:30.1-jre:compile on the classpath and, as far as I have checked, they do have some URL escaping. I'm not sure if this would be useful. Might be worth a closer look...?
If there really are no lightweight libraries that we can use, we might as well copy the UriUtils class to this project, or simply re-implement it ourseves. (Spring is under an Apache 2.0 license and so is this library and as long as we preserve the original licensing information and credit the authors, we should be okay). It would be better to find a simpler alternative, though, and I'm sure we can find some, if we dig around. :)
There was a problem hiding this comment.
@elerch , @markschreiber : Any thoughts?
There was a problem hiding this comment.
I analyzed the code paths in UriUtils eventually settling on HierarchicalUriComponents.encodeUriComponent() as the thing doing the work. Looks fairly straightforward. I'm happy to do the work, just want to make sure owners agree before I go down that path.
There was a problem hiding this comment.
If you ask me, this fix should ultimately be applied to the Amazon SDK (v2), so that users of the library (like us) don't have to do this sort of wizardry on our side. I am open to having this method copied over with a disclaimer comment that it was taken from the Spring SDK. The Guava library also contains a similar class (UrlEscapers), which, from what I can tell, seems more sophisticated. We are already using that library and perhaps it would be neater to use it?
@amarcionek, please, feel free to join our chat channel where we could further discuss this, if you like.
There was a problem hiding this comment.
The AWS SDK should be here right: https://github.com/aws/aws-sdk-java
Maybe open an Issue there with a potential fix aswell, while applying a Bandage in the Project here, throught the Guava class. Atleast for me, it seems that is easier to contain. However it might be a problem, considering that the UrlEscapers seems to require more manual work, while the Spring version is harder to misuse. Might be wrong though.
There was a problem hiding this comment.
The link you're pointing to is Amazon SDK v1 and we've migrated to v2.
BTW, @amarcionek, Amazon cut releases of their SDK-s every day, (and we upgrade to these releases almost every day). If you were to fix this on the side of Amazon's SDK, we would get this fix into our codebase as soon as they cut a release that includes it.
There was a problem hiding this comment.
This is the correct link to the repository of Amazon's SDK (v2).
| public void toUriSpecialChars() | ||
| { | ||
| Path path = getPath("/bucket/([fol! @#$%der])"); |
There was a problem hiding this comment.
From what I understand, this test should actually be called toUriWithSpecialCharactersThatShouldNodBeDecoded instead, right?
Could you please add a test case where there are actual characters that need to be decoded?
| { | ||
| // This should never happen unless there is something | ||
| // fundamentally broken with the running JVM. | ||
| throw new RuntimeException(e); |
There was a problem hiding this comment.
We should definitely add a short message here and maybe change the type to Error?
A message on the other hand should definitely say something along the Line "URI could not be encoded into an URL. JVM is broken". Not sure i agree with the assesment though, but the URI and URL Standard are broken anyway.
There was a problem hiding this comment.
Yeah, this is code that is actually copied from Amazon's SDK, from what I saw.
There was a problem hiding this comment.
However you do it I think you should be very explicit about the behavior in the docs. Valid S3 Paths are not always going to be fully compatible with file paths encoded as URIs. You could follow the lead of FSx for Lustre which, when backed by S3, effectively hides some valid S3 paths which cannot be converted into POSIX file paths.
Run the uri through URLEncoder.encode and then converting allowable characters :, / and [space] back. Taken from https://github.com/aws/aws-sdk-java/blob/master/aws-java-sdk-s3/src/main/java/com/amazonaws/services/s3/AmazonS3URI.java
Pull Request Description
This pull request closes Upplication/Amazon-S3-FileSystem-NIO2#117 and addresses one of the items in #72
Acceptance Test
mvn clean install -Pintegration-testsstill works.Questions
Does this pull request break backward compatibility?
Does this pull request require other pull requests to be merged first?
Does this require an update of the documentation?