Fix listing of S3 prefixes not terminated by a slash #3

Merged
matteo merged 5 commits from eay/serves3:hackathon_slash_fix_erik_and_eren into main 2024-06-12 12:05:38 +02:00
Contributor

Fixes #2.

Fixes #2.
eay added 1 commit 2024-04-17 17:09:30 +02:00
matteo changed title from fix listing of S3 prefixes not terminated by a slash to Fix listing of S3 prefixes not terminated by a slash 2024-04-19 09:41:09 +02:00
matteo requested changes 2024-04-19 09:49:28 +02:00
matteo left a comment
Owner

Thank you for your contribution! Could you perform some of the requested changes? They are only minor points.

And thanks for adding a unit test!

Thank you for your contribution! Could you perform some of the requested changes? They are only minor points. And thanks for adding a unit test!
src/main.rs Outdated
@ -185,6 +187,7 @@ async fn s3_fileview(path: &PathBuf) -> Result<Vec<FileViewItem>, Error> {
let files = list.contents.iter().map(|obj| {
let path = obj.key.strip_prefix(&prefix);
path.map(|path| FileViewItem {
parent:s3_folder_path.clone(),
Owner

The last usage of a string can simply be moved instead of cloned to avoid a needless copy.

The last usage of a string can simply be moved instead of cloned to avoid a needless copy.
matteo marked this conversation as resolved
src/main.rs Outdated
@ -241,0 +247,4 @@
#[cfg(test)]
mod tests {
use rstest::rstest;
// Note this useful idiom: importing names from outer (for mod tests) scope.
Owner

Remove comment

Remove comment
matteo marked this conversation as resolved
src/main.rs Outdated
@ -241,0 +260,4 @@
#[case(u64::MIN, format!("{:.3} B",u64::MIN as f64))]
fn test_size_bytes_to_human(#[case] bytes: u64, #[case] expected: String) {
println!("{}",size_bytes_to_human(bytes));
Owner

Formatting, please run cargo fmt on the sources

Formatting, please run `cargo fmt` on the sources
matteo marked this conversation as resolved
@ -45,3 +45,3 @@
{% for object in objects %}
<tr>
<td><a href="{{ object.path | urlencode }}" data-size-bytes="{{ object.size_bytes }}">{{ object.path }}</a></td>
<td><a href="/{{ object.parent | urlencode }}{{ object.path | urlencode }}" data-size-bytes="{{ object.size_bytes }}">{{ object.path }}</a></td>
Owner

This way of making paths absolutes would not work if the service was to be deployed as a subfolder instead than a virtualhost in the HTTP server configuration.

Imagine the service running via a reverse proxy at https://some.website.com/subfolder.

Then a link of the form /parent/object would resolve to https://some.website.com/parent/object instead of https://some.website.com/subfolder/parent/object.

It is therefore better to use the uri! macro to build the path instead. Could you please move the building of the URI in the Rust portion of the code, and here just use the computed result?

Rocket can be configured then to handle this correctly (which is up to the end user).

This way of making paths absolutes would not work if the service was to be deployed as a subfolder instead than a virtualhost in the HTTP server configuration. Imagine the service running via a reverse proxy at `https://some.website.com/subfolder`. Then a link of the form `/parent/object` would resolve to `https://some.website.com/parent/object` instead of `https://some.website.com/subfolder/parent/object`. It is therefore better to use the [uri! macro](https://api.rocket.rs/v0.5/rocket/macro.uri) to build the path instead. Could you please move the building of the URI in the Rust portion of the code, and here just use the computed result? Rocket can be configured then to handle this correctly (which is up to the end user).
matteo marked this conversation as resolved
eay added 1 commit 2024-05-28 14:10:11 +02:00
matteo requested review from matteo 2024-06-02 00:47:07 +02:00
matteo force-pushed hackathon_slash_fix_erik_and_eren from 73da6a8f5f to 4defbcec1f 2024-06-02 16:47:56 +02:00 Compare
Owner

@eay I refactored the code to add integration tests and a more sane structure.

However, I don't seem to be able to reproduce the original problem anymore with my tests.

Could you try on your side to see if the problem is still there, and eventually adapt the tests accordingly? Thanks!

@eay I refactored the code to add integration tests and a more sane structure. However, I don't seem to be able to reproduce the original problem anymore with my tests. Could you try on your side to see if the problem is still there, and eventually adapt the tests accordingly? Thanks!
matteo added 1 commit 2024-06-02 16:55:29 +02:00
matteo pinned this 2024-06-02 16:56:24 +02:00
matteo unpinned this 2024-06-02 16:57:25 +02:00
eay added 1 commit 2024-06-12 11:55:38 +02:00
matteo merged commit 59c0543fd2 into main 2024-06-12 12:05:38 +02:00
matteo deleted branch hackathon_slash_fix_erik_and_eren 2024-06-12 12:05:38 +02:00
Sign in to join this conversation.
No reviewers
No Label
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: matteo/serves3#3
No description provided.