Fix S3 query canonicalization (#2800) #2801
Conversation
object_store/src/lib.rs
Outdated
| let path = Path::parse("foo bar/I contain spaces.parquet").unwrap(); | ||
| storage.put(&path, Bytes::from(vec![0, 1])).await.unwrap(); | ||
| storage.head(&path).await.unwrap(); | ||
| let files = flatten_list_stream(storage, Some(&Path::from("foo bar"))) | ||
| .await | ||
| .unwrap(); | ||
| assert_eq!(files, vec![path.clone()]); | ||
| storage.delete(&path).await.unwrap(); | ||
|
|
||
| let files = flatten_list_stream(storage, Some(&Path::from("foo bar"))) | ||
| .await | ||
| .unwrap(); | ||
| assert!(files.is_empty(), "{:?}", files); |
There was a problem hiding this comment.
This adds an explicit test of paths containing spaces, FYI @andygrove
|
Looks like there are more yaks to shave 😭 Edit: So it would appear that both emulators don't handle this correctly... I guess this is why they all say not to stick spaces in paths, everything is broken 😆 I need to get on with some other stuff, but I will come back to this and file upstream tickets for this |
|
I opted to suppress the broken emulators, instead of working around the issue, as they are bugs in the emulators. Running against the real services behaves correctly. |
|
Benchmark runs are scheduled for baseline = f845d6e and contender = da8f742. da8f742 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 #2800
Contains tests for #2799
Rationale for this change
I was lazy when implementing query canonicalization, and figured we could just use the query from the URL provided they were in the correct order. Sadly this is not the case as AWS has specific requirements for percent encoding.
What changes are included in this PR?
Implements request canonicalisation as per the spec
Are there any user-facing changes?
No