support index on path missing a trailing slash #612

Merged
lx merged 2 commits from compat/index-without-trailing-slash into main 2023-08-28 10:15:02 +00:00
Owner

Fixes #557

New behavior

[quentin@lheureduthe:~/Documents/dev/deuxfleurs/garage]$ curl -L -i -H 'Host:example.com' http://127.0.0.1:3902/subfolder
HTTP/1.1 302 Found
location: /subfolder/
content-length: 0
date: Tue, 08 Aug 2023 13:01:52 GMT

HTTP/1.1 200 OK
content-type: text/html
last-modified: Tue, 08 Aug 2023 12:34:05 GMT
accept-ranges: bytes
etag: "4c2383f5c88e9110642953b5dd7c88a1"
content-length: 7
date: Tue, 08 Aug 2023 13:01:52 GMT

coucou

Existing behaviors

[quentin@lheureduthe:~/Documents/dev/deuxfleurs/garage]$ curl -L -i -H 'Host:example.com' http://127.0.0.1:3902/subfolder/
HTTP/1.1 200 OK
content-type: text/html
last-modified: Tue, 08 Aug 2023 12:34:05 GMT
accept-ranges: bytes
etag: "4c2383f5c88e9110642953b5dd7c88a1"
content-length: 7
date: Tue, 08 Aug 2023 13:01:55 GMT

coucou
[quentin@lheureduthe:~/Documents/dev/deuxfleurs/garage]$ curl -L -i -H 'Host:example.com' http://127.0.0.1:3902/subfolder/index.html
HTTP/1.1 200 OK
content-type: text/html
last-modified: Tue, 08 Aug 2023 12:34:05 GMT
accept-ranges: bytes
etag: "4c2383f5c88e9110642953b5dd7c88a1"
content-length: 7
date: Tue, 08 Aug 2023 13:01:58 GMT

coucou

Content of the bucket

[quentin@lheureduthe:~/Documents/dev/deuxfleurs/garage]$ aws s3 ls --recursive s3://example.com
2023-08-08 14:34:05          7 subfolder/index.html

Example with a 404

[quentin@lheureduthe:~/Documents/dev/deuxfleurs/garage]$ curl -L -i -H 'Host:example.com' http://127.0.0.1:3902/subfolder2
HTTP/1.1 404 Not Found
content-type: application/xml
content-length: 25
date: Tue, 08 Aug 2023 13:02:24 GMT

API error: Key not found
Fixes #557 ## New behavior ``` [quentin@lheureduthe:~/Documents/dev/deuxfleurs/garage]$ curl -L -i -H 'Host:example.com' http://127.0.0.1:3902/subfolder HTTP/1.1 302 Found location: /subfolder/ content-length: 0 date: Tue, 08 Aug 2023 13:01:52 GMT HTTP/1.1 200 OK content-type: text/html last-modified: Tue, 08 Aug 2023 12:34:05 GMT accept-ranges: bytes etag: "4c2383f5c88e9110642953b5dd7c88a1" content-length: 7 date: Tue, 08 Aug 2023 13:01:52 GMT coucou ``` ## Existing behaviors ``` [quentin@lheureduthe:~/Documents/dev/deuxfleurs/garage]$ curl -L -i -H 'Host:example.com' http://127.0.0.1:3902/subfolder/ HTTP/1.1 200 OK content-type: text/html last-modified: Tue, 08 Aug 2023 12:34:05 GMT accept-ranges: bytes etag: "4c2383f5c88e9110642953b5dd7c88a1" content-length: 7 date: Tue, 08 Aug 2023 13:01:55 GMT coucou ``` ``` [quentin@lheureduthe:~/Documents/dev/deuxfleurs/garage]$ curl -L -i -H 'Host:example.com' http://127.0.0.1:3902/subfolder/index.html HTTP/1.1 200 OK content-type: text/html last-modified: Tue, 08 Aug 2023 12:34:05 GMT accept-ranges: bytes etag: "4c2383f5c88e9110642953b5dd7c88a1" content-length: 7 date: Tue, 08 Aug 2023 13:01:58 GMT coucou ``` ## Content of the bucket ``` [quentin@lheureduthe:~/Documents/dev/deuxfleurs/garage]$ aws s3 ls --recursive s3://example.com 2023-08-08 14:34:05 7 subfolder/index.html ``` ## Example with a 404 ``` [quentin@lheureduthe:~/Documents/dev/deuxfleurs/garage]$ curl -L -i -H 'Host:example.com' http://127.0.0.1:3902/subfolder2 HTTP/1.1 404 Not Found content-type: application/xml content-length: 25 date: Tue, 08 Aug 2023 13:02:24 GMT API error: Key not found ```
quentin force-pushed compat/index-without-trailing-slash from fbd40bac0d to 63da1d2443 2023-08-08 13:29:07 +00:00 Compare
quentin added the
scope
s3-api
label 2023-08-08 13:29:41 +00:00
lx requested changes 2023-08-28 09:08:45 +00:00
lx left a comment
Owner

One logic error (check_key_exists should return a Result<bool>) and small stylistic improvement suggestion.

One logic error (check_key_exists should return a `Result<bool>`) and small stylistic improvement suggestion.
@ -171,0 +174,4 @@
.object_table
.get(&bucket_id, &key.to_string())
.await
.ok()
Owner

You should not use .ok() here but ?, because if object_table.get returns an error we cannot consider that the result of this function is correct.

Overall, please rewrite the function as:

async fn check_key_exists(self: &Arc<Self>, bucket_id: Uuid, key: &str) -> Result<bool> {
    let exists = self.garage
        .object_table
        .get(&bucket_id, &key.to_string())
        .await?
        .map(|object| object
            .versions()
            .iter()
            .any(|v| v.is_data())
        )
        .unwrap_or(false);
    Ok(exists)
}
You should not use `.ok()` here but `?`, because if `object_table.get` returns an error we cannot consider that the result of this function is correct. Overall, please rewrite the function as: ```rust async fn check_key_exists(self: &Arc<Self>, bucket_id: Uuid, key: &str) -> Result<bool> { let exists = self.garage .object_table .get(&bucket_id, &key.to_string()) .await? .map(|object| object .versions() .iter() .any(|v| v.is_data()) ) .unwrap_or(false); Ok(exists) } ```
@ -332,3 +365,1 @@
Cow::Borrowed(pu8) => Ok((&pu8[1..]).into()),
Cow::Owned(pu8) => Ok(pu8[1..].to_string().into()),
},
let base_key = &path_utf8[1..];
Owner

361-365 can be rewritten more elegantly as:

let base_key = match path_utf8.strip_prefix("/") {
    Some(bk) => bk,
    None => return Err(Error::BadRequest("Path must start with a / (slash)".into())),
};

This avoids slicing which is "unelegant" as it is not guaranteed to be correct (here we know that it works but with additionnal reasoning).

361-365 can be rewritten more elegantly as: ```rust let base_key = match path_utf8.strip_prefix("/") { Some(bk) => bk, None => return Err(Error::BadRequest("Path must start with a / (slash)".into())), }; ``` This avoids slicing which is "unelegant" as it is not guaranteed to be correct (here we know that it works but with additionnal reasoning).
@ -334,1 +365,3 @@
},
let base_key = &path_utf8[1..];
let is_bucket_root = base_key.len() == 0;
let is_trailing_slash = path_utf8.chars().last().map(|v| v == '/').unwrap_or(false);
Owner

can be written shorter as path_utf8.ends_with('/')

can be written shorter as `path_utf8.ends_with('/')`
lx added 1 commit 2023-08-28 10:05:39 +00:00
web_server.rs: handle error properly and refactor
All checks were successful
continuous-integration/drone/pr Build is passing
continuous-integration/drone/push Build is passing
c5cafa0000
lx merged commit 2bbe2da5ad into main 2023-08-28 10:15:02 +00:00
Sign in to join this conversation.
No reviewers
lx
No milestone
No project
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: Deuxfleurs/garage#612
No description provided.