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

s3x - Coversion from URI to Path and Then Back to URI Loses the Initial Configuration #237

Closed
thsandu opened this issue Oct 27, 2023 · 5 comments

Comments

@thsandu
Copy link

thsandu commented Oct 27, 2023

I have issues working with conversions from URI to Path and back when using the s3x and the cool endpoint overriding functionality. It seems to me that when we are using the toUri() on a path, the initial overridden configuration is lost.

I forked your repo to add a unit test.

As you can see here, the s3 case is working: https://github.com/thsandu/aws-java-nio-spi-for-s3-issues-with-s3x/blob/b22de4a8dd68745191d797225c78c035ad30abf1/src/test/java/software/amazon/nio/spi/s3/S3PathTest.java#L365

When I so something similar on the s3x case, I get a modified result: https://github.com/thsandu/aws-java-nio-spi-for-s3-issues-with-s3x/blob/b22de4a8dd68745191d797225c78c035ad30abf1/src/test/java/software/amazon/nio/spi/s3x/S3PathTest.java#L51

        URI uri = URI.create("s3x://somewhere.com:1010/bucket/subfolder/afile.txt");
        Path path = Paths.get(uri);

        Path convertedPath = Paths.get(path.toUri());

        then(convertedPath).isEqualTo(path);

The test yields:

expected: /subfolder/afile.txt
 but was: /afile.txt
@stefanofornari
Copy link
Contributor

Hi @thsandu, sorry for the late reply. Thanks for reporting the issue and providing the unit test, I will look into it.

@thsandu
Copy link
Author

thsandu commented Nov 7, 2023

Hi @thsandu, sorry for the late reply. Thanks for reporting the issue and providing the unit test, I will look into it.

No worries, the issue with the reading of file contents from localstack was also more important for us.

stefanofornari added a commit to stefanofornari/aws-java-nio-spi-for-s3 that referenced this issue Nov 7, 2023
stefanofornari added a commit to stefanofornari/aws-java-nio-spi-for-s3 that referenced this issue Nov 7, 2023
stefanofornari added a commit to stefanofornari/aws-java-nio-spi-for-s3 that referenced this issue Nov 7, 2023
stefanofornari added a commit to stefanofornari/aws-java-nio-spi-for-s3 that referenced this issue Nov 7, 2023
stefanofornari added a commit to stefanofornari/aws-java-nio-spi-for-s3 that referenced this issue Nov 7, 2023
markjschreiber pushed a commit that referenced this issue Nov 8, 2023
@markjschreiber
Copy link
Contributor

Closing with fix from @stefanofornari in #283. Feel free to re-open if this is still an issue.

@thsandu
Copy link
Author

thsandu commented Nov 8, 2023

Thank you for the fix @stefanofornari
I see now that the endpoint is not lost any more. I still have a question regarding the AWS authority. Is it by design that the aws.accessKey and aws.sercretAccessKey are not considered in the toUri method?

Adding this to your new test would fail:

uri = URI.create("s3x://key:[email protected]:1010/bucket/subfolder/afile.txt"); then(Paths.get(uri).toUri()).isEqualTo(uri);

@stefanofornari
Copy link
Contributor

Hi @thsandu
yes, the rational is that I wanted to keep in the URI addressing information only, while I left aside credentials as they can also take to leak of sensitive information (e.g. logging). I may be open to reconsider it if other services work differently.

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

No branches or pull requests

3 participants