support index on path missing a trailing slash #612

quentin · 2023-08-08T13:28:21Z

quentin commented

2023-08-08 13:28:21 +00:00

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

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.

src/web/web_server.rs Outdated

					
				@ -171,0 +174,4 @@

							.object_table

							.get(&bucket_id, &key.to_string())

							.await

							.ok()

lx commented

2023-08-28 08:54:59 +00:00

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) } ```

src/web/web_server.rs

					
				@ -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..];

lx commented

2023-08-28 08:59:59 +00:00

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).

src/web/web_server.rs

					
				@ -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);