Skip to content

object_store: fix: Incorrect parsing of https Path Style S3 url#4082

Merged
tustvold merged 4 commits intoapache:masterfrom
roeap:fix-aws-url
Apr 13, 2023
Merged

object_store: fix: Incorrect parsing of https Path Style S3 url#4082
tustvold merged 4 commits intoapache:masterfrom
roeap:fix-aws-url

Conversation

@roeap
Copy link
Copy Markdown
Contributor

@roeap roeap commented Apr 13, 2023

Which issue does this PR close?

Closes apache/arrow-rs-object-store#175

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

Copy link
Copy Markdown
Contributor

@tustvold tustvold left a comment

Choose a reason for hiding this comment

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

Should this also parse the bucket from the path of the URL?

@tustvold tustvold added the api-change Changes to the arrow API label Apr 13, 2023
@roeap
Copy link
Copy Markdown
Contributor Author

roeap commented Apr 13, 2023

Not sure, but happy to add it.

Guess it makes sense, since we cannot work without a bucket. Haven't considered it so far, since we were only processing the host.

@roeap
Copy link
Copy Markdown
Contributor Author

roeap commented Apr 13, 2023

We are now also checking if the first path segment exists, and assigning it to the bucket.

Some(("s3", region, "amazonaws", "com")) => {
self.region = Some(region.to_string());
if let Some(bucket) =
parsed.path_segments().and_then(|mut path| path.next())
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could we get a test of this perhaps?

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.

Of course and done - sorry for getting sloppy :).

@tustvold tustvold merged commit 485696e into apache:master Apr 13, 2023
@roeap roeap deleted the fix-aws-url branch April 13, 2023 20:37
alamb pushed a commit to alamb/arrow-rs that referenced this pull request Mar 20, 2025
…he#4082)

* fix: parse reagion from path-style urls, not bucket

* fix: test

* fix: parse s3 bucket from first path segment

* test: add test for parsing bucket from path style url
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api-change Changes to the arrow API

Projects

None yet

Development

Successfully merging this pull request may close these issues.

object_store: Incorrect parsing of https Path Style S3 url

2 participants