Derive Clone for ObjectStore builders and Make URL Parsing Stricter (#3419) #3424
Conversation
Make URL parsing more strict
tustvold
changed the title
| /// | ||
| /// This is a separate member function to allow fallible computation to | ||
| /// be deferred until [`Self::build`] which in turn allows deriving [`Clone`] | ||
| fn parse_url(&mut self, url: &str) -> Result<()> { |
There was a problem hiding this comment.
Previously invalid URLs would be silently ignored, they now result in an error which I think makes for a better UX
| @@ -365,10 +368,10 @@ pub struct MicrosoftAzureBuilder { | |||
| tenant_id: Option<String>, | |||
| sas_query_pairs: Option<Vec<(String, String)>>, | |||
| authority_host: Option<String>, | |||
| url: Option<String>, | |||
There was a problem hiding this comment.
The MicrosoftAzureBuilder is missing the Clone in the derive list.
object_store/src/aws/mod.rs
Outdated
| }, | ||
| _ => match prefix.split_once('.') { | ||
| // https://<bucket>.s3.<region>.amazonaws.com | ||
| Some((b, r)) if r.starts_with("s3.") => b, |
There was a problem hiding this comment.
Should we set region and virtual_hosted_style_request here before returning the bucket? Otherwise I think the user would have to set these separetely for things to work.
There was a problem hiding this comment.
I think we should make sure we cover this case with a test so we are sure it works / and it isn't broken in a future refactoring
| assert_eq!(builder.bucket_name, Some("bucket".to_string())); | ||
|
|
||
| let builder = | ||
| AmazonS3Builder::new().with_url("https://bucket.s3.region.amazonaws.com"); | ||
| let mut builder = AmazonS3Builder::new(); |
There was a problem hiding this comment.
perhaps should we add a test for parsing errors?
(also applies to the other test_urls functions)
object_store/src/aws/mod.rs
Outdated
| }, | ||
| _ => match prefix.split_once('.') { | ||
| // https://<bucket>.s3.<region>.amazonaws.com | ||
| Some((b, r)) if r.starts_with("s3.") => b, |
There was a problem hiding this comment.
I think we should make sure we cover this case with a test so we are sure it works / and it isn't broken in a future refactoring
|
Many thanks! I have been looking into the verde support. The structs re-exported from http::headers do not work out of the box but I am close, I think. Will submit a PR after this one, unless @tustvold already started on it. |
|
Benchmark runs are scheduled for baseline = b371f41 and contender = 1889e33. 1889e33 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Which issue does this PR close?
Closes #3419
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?
No, this does not making any breaking changes to released APIs
FYI @roeap perhaps you would be able to review this 🙏