File & directory deletion feature #1093

cyqsimon · 2023-04-03T10:20:32Z

Closes #397.

Todo

CLI options
Backend API
Frontend controls
Tests

cyqsimon · 2023-04-03T10:29:34Z

Lines 329 to 330 in 82dd826

    
           // Allow file upload 
        
           app.service(web::resource("/upload").route(web::post().to(file_upload::upload_file)));

I am curious why the upload feature is implemented like so. I guess it's good for unforeseen future extensibility, but there is semantic duplication between the /upload endpoint and the POST method.

The real question I want to ask, is should I implement the deletion feature in a similar manner? I.e. something like this:

app.service(web::resource("/delete").route(web::delete().to(file_rm::rm_file)));

svenstaro · 2023-04-03T10:56:56Z

Are you suggesting it might have been better to just have POST always try to upload stuff? It would be simpler for sure but as you are saying, it's not great if we ever want a second POST for something. I think this is currently an ok compromise.

I see how this is a bit of a conundrum with delete. I also want to add WebDAV at some point and that will introduce even more semantics. Perhaps it's the best to go with what you suggested just now and eat the semantic duplication for the time being. It's not like it can't be changed later.

bgusach · 2023-08-25T13:34:37Z

Hi,

unfortunately I can't directly help... just wanted to say that I appreciate the effort and if it gets done at some point, my life would be somehow better :)

cyqsimon · 2023-08-28T08:07:58Z

Hi,

unfortunately I can't directly help... just wanted to say that I appreciate the effort and if it gets done at some point, my life would be somehow better :)

Thanks for reminding me. I have too many things on hand and honestly just forgot about this whole matter. I will find some time to work on it this week, hopefully.

data/style.scss

cyqsimon · 2024-01-29T09:26:45Z

Rebased on #1331.

cyqsimon · 2024-01-30T02:31:51Z

Rebased on master.

cyqsimon · 2024-06-29T11:56:40Z

Rebased on master.

svenstaro · 2024-07-07T00:37:31Z

Could you add some tests? Since this is going to be deleting things, it needs some proper tests.

cyqsimon · 2024-07-07T03:21:30Z

Could you add some tests? Since this is going to be deleting things, it needs some proper tests.

Yeah I'm working on that on and off. The tests are actually written but several are failing. My original intention was to combine the tests and the fixes into one commit, but I guess there's no harm in splitting them up. I'll push the tests first and then look to fix them.

cyqsimon · 2024-07-07T03:26:18Z

If you can spare some time, please take a look at the test suite; make sure they are testing the correct behaviour. Thanks.

svenstaro · 2024-07-07T03:28:02Z

Yeah I'm working on that on and off. The tests are actually written but several are failing. My original intention was to combine the tests and the fixes into one commit, but I guess there's no harm in splitting them up. I'll push the tests first and then look to fix them.

No worries. There's no rush to get this in. Just take your time and make sure things look good.

If you can spare some time, please take a look at the test suite; make sure they are testing the correct behaviour. Thanks.

I will try to squeeze a review in but no promises, I'm fairly low on time currently.

cyqsimon · 2024-09-15T16:12:35Z

Rebased on #1454.

cyqsimon · 2024-09-15T16:30:12Z

This should be ready now, finally. Everything works as expected, as far as I can tell. But you may wish to merge the other PR first.

tests/fixtures/mod.rs

svenstaro · 2024-09-21T22:02:32Z

Needs a rebase and then I will do a proper review. :)

cyqsimon · 2024-09-22T13:30:10Z

Actually, let me do an interactive rebase to reorganise things. Then it'd be a bit tidier.

This allows the encoding set definitions to be `include!`ed into integration tests

qtekk · 2024-10-16T12:56:24Z

hey, how's the feature going?

cyqsimon · 2024-10-16T14:47:06Z

hey, how's the feature going?

Implementation is complete. Just waiting on review.

@svenstaro You got time to take a look?

svenstaro · 2024-10-18T16:59:50Z

I will take some time. Might take a look on the weekend.

cyqsimon · 2025-01-07T13:18:13Z

Hi Sven, please don't forget about this one 😅

svenstaro · 2025-01-10T09:30:55Z

Sorry, reviewing now.

svenstaro · 2025-01-10T09:32:16Z

Thoughts:

Do we want any kind of confirmation? Currently you might miss-click and a file is just gone.
Do we want to warn in case a path given to -R is not contained in the served paths? For instance, miniserve -R /tmp /some/dir doesn't do anything.
Should we abort if we provide a path to a file but also -R? We don't do this with -u but it might make sense as a followup? So for instance, miniserve -R /tmp /some/file.

cyqsimon · 2025-01-10T14:34:21Z

Good points.

Do we want any kind of confirmation? Currently you might miss-click and a file is just gone.

That would be nice. I would also want a toggle/checkbox that allows turning off confirmation, in case where you want to delete several things en-mass.

I'll give this a shot, but I'm not great at front end coding. We'll see.

Do we want to warn in case a path given to -R is not contained in the served paths? For instance, miniserve -R /tmp /some/dir doesn't do anything.

I think there's some misunderstanding. The paths accepted by -u and -R are actually URL paths relative to /, not file system paths relative to CWD. This means:

the specified directory should never escape the serve root, excepting symlink shenanigans (all covered by tests)
all paths are valid because new directories can be created on the fly; even something like -R ../.. is legal because navigating up the serve root is prevented, so effectively it's the same as -R .
-R /foo/bar and -R foo/bar are equivalent

So I believe the implementation needs no changes, but it would be prudent to note this in the CLI help document though. I'll make that happen. Also it might be a good idea to log a message at INFO level noting this behaviour.

Should we abort if we provide a path to a file but also -R? We don't do this with -u but it might make sense as a followup? So for instance, miniserve -R /tmp /some/file.

Oh I actually didn't know we support serving a single file. That's a nice feature.

Hmm yeah this is very interesting indeed. Because the semantics of -u and -R aren't really compatible with this mode of operation.

But first of all, should we allow removing (or for -u, overwriting) in this mode at all? Personally I believe the answer is yes, but I'd like to hear your opinions too.

And if the answer to the previous question is yes, obviously -u and -R with arguments would make no sense (since a file cannot have subdirectories). Should this be a warning or a hard error?

cyqsimon · 2025-01-10T14:41:27Z

For points 2 and 3, the changes would apply to both -u and -R. So I think it's better organisationally if these form a separate PR, queued after this one.

svenstaro

Review done. I think we're almost there. :)

data/style.scss

svenstaro · 2025-01-10T10:56:26Z

tests/rm_files.rs

+include!(concat!(
+    env!("CARGO_MANIFEST_DIR"),
+    "/",
+    "src/path_utils.rs"
+));


This is a really weird to include something. Is there a reason to do this over use miniserve::path_utils? You would have to make it public but that's fine.

svenstaro · 2025-01-10T14:12:42Z

tests/rm_files.rs

+    format!("rm?path=/{}", make_get_path(unencoded_path))
+}
+
+fn assert_rm_ok(base_url: Url, unencoded_paths: &[impl AsRef<Path>]) -> anyhow::Result<()> {


We don't use anyhow::Result in any tests. Could you please take a look at how we do this in upload_files.rs and follow that? Basically use fixtures::Error with normal Results.

svenstaro · 2025-01-10T14:58:08Z

tests/fixtures/mod.rs

+/// Files nested at different levels under the same root directory
+///
+/// This is not a `&[&str]` because in percent-encoding, path segments and full paths
+/// are encoded differently.
 #[allow(dead_code)]
-pub static DEEPLY_NESTED_FILE: &str = "very/deeply/nested/test.rs";
+pub static NESTED_FILES_UNDER_SINGLE_ROOT: LazyLock<Vec<&Path>> = LazyLock::new(|| {
+    vec![
+        Path::new("someDir/alpha"),
+        Path::new("someDir/some_sub_dir/bravo"),
+    ]
+});


I still don't really understand. Where is this relevant and why now?

svenstaro · 2025-01-10T15:04:06Z

tests/navigation.rs

+            dir.to_str().unwrap()
+        })
+        .collect::<Vec<_>>();
+    dbg!(&dir_names);


Do we want to keep this dbg!() here? If so, it should at least get a better message with it.

svenstaro · 2025-01-10T15:37:50Z

tests/rm_files.rs

+/// The `check_paths_exist` parameter allows skipping this check before and after
+/// the deletion attempt in case these paths should be inaccessible via GET.


That's a good start but please also describe the intent of the function as well.

svenstaro · 2025-01-10T15:38:05Z

tests/rm_files.rs

+    format!("rm?path=/{}", make_get_path(unencoded_path))
+}
+
+fn assert_rm_ok(base_url: Url, unencoded_paths: &[impl AsRef<Path>]) -> anyhow::Result<()> {


Please give this function a proper doc comment as it's quite complex.

svenstaro · 2025-01-10T16:04:24Z

tests/rm_files.rs

+fn rm_disabled_by_default(server: TestServer) -> anyhow::Result<()> {
+    let paths = [FILES, DIRECTORIES]
+        .concat()
+        .into_iter()
+        .map(Path::new)
+        .chain(iter::once(DEEPLY_NESTED_FILE.as_ref()))
+        .collect::<Vec<_>>();
+    assert_rm_err(server.url(), &paths, true)


Hm, for all of these days I wonder whether it wouldn't be nicer and more obvious to use rstest more in the way that it was intended and have test-cases generated? Currently all paths are tested in the same test which is not really the intention of rstest. Maybe take a look at how we do things in upload_files.rs and try to follow that?

svenstaro · 2025-01-10T16:31:29Z

Excuse the conflicts, should be fairly easy to rebase, though.

svenstaro · 2025-01-10T16:47:12Z

Good points.

Do we want any kind of confirmation? Currently you might miss-click and a file is just gone.

That would be nice. I would also want a toggle/checkbox that allows turning off confirmation, in case where you want to delete several things en-mass.

I'll give this a shot, but I'm not great at front end coding. We'll see.

Yeah, I think that would be safer and probably also more convenient. I can't think of many cases where I want to just click something and it'll be deleted right then and there without confirmation. I suspect users have also grown to expect confirmations for deletions.

Do we want to warn in case a path given to -R is not contained in the served paths? For instance, miniserve -R /tmp /some/dir doesn't do anything.

I think there's some misunderstanding. The paths accepted by -u and -R are actually URL paths relative to /, not file system paths relative to CWD. This means:
1. the specified directory should never escape the serve root, excepting symlink shenanigans (all covered by tests)

2. all paths are valid because new directories can be created on the fly; even something like `-R ../..` is legal because navigating up the serve root is prevented, so effectively it's the same as `-R .`

3. `-R /foo/bar` and `-R foo/bar` are equivalent
So I believe the implementation needs no changes, but it would be prudent to note this in the CLI help document though. I'll make that happen. Also it might be a good idea to log a message at INFO level noting this behaviour.

You are correct, of course. I didn't consider that the paths can become valid during runtime. I think documenting this more clearly would make sense for sure as it had even me confused :D

Should we abort if we provide a path to a file but also -R? We don't do this with -u but it might make sense as a followup? So for instance, miniserve -R /tmp /some/file.

Oh I actually didn't know we support serving a single file. That's a nice feature.

Hmm yeah this is very interesting indeed. Because the semantics of -u and -R aren't really compatible with this mode of operation.

But first of all, should we allow removing (or for -u, overwriting) in this mode at all? Personally I believe the answer is yes, but I'd like to hear your opinions too.

I'm not really sure it makes sense. How would we even present that to the user? Currently serving a single file just gives you that file directly without any UI. I'm not sure supporting -u or -R would make any sense outside of an API-esque usage scenario.

And if the answer to the previous question is yes, obviously -u and -R with arguments would make no sense (since a file cannot have subdirectories). Should this be a warning or a hard error?

svenstaro · 2025-01-10T16:47:42Z

For points 2 and 3, the changes would apply to both -u and -R. So I think it's better organisationally if these form a separate PR, queued after this one.

Agreed, no need to do it in this one.

svenstaro · 2025-01-28T17:30:27Z

@cyqsimon wanna push this over the finish line?

cyqsimon · 2025-01-28T21:51:17Z

@cyqsimon wanna push this over the finish line?

Yeah but I'm on vacation at the moment and connectivity is shoddy. Thanks for the reminder though, will try to find some time for this.

cyqsimon force-pushed the rm-rmdir branch from 82dd826 to 2fdd5bf Compare July 10, 2023 05:58

cyqsimon mentioned this pull request Sep 5, 2023

Minor refactor on upload code #1228

Merged

cyqsimon mentioned this pull request Jan 26, 2024

Fix inaccurate uses of sanitize_path #1327

Merged

cyqsimon force-pushed the rm-rmdir branch from 2fdd5bf to 8fef88b Compare January 26, 2024 07:40

cyqsimon mentioned this pull request Jan 26, 2024

How about add "delete file" feature? #397

Open

cyqsimon commented Jan 26, 2024

View reviewed changes

data/style.scss Show resolved Hide resolved

cyqsimon force-pushed the rm-rmdir branch from 9e4e337 to ca94dce Compare January 26, 2024 11:14

cyqsimon mentioned this pull request Jan 29, 2024

Refactor errors #1331

Merged

cyqsimon force-pushed the rm-rmdir branch from 7f6dfdf to 71cdbcd Compare January 29, 2024 09:26

cyqsimon force-pushed the rm-rmdir branch from ea0c972 to e374f7a Compare January 30, 2024 02:31

cyqsimon force-pushed the rm-rmdir branch from e374f7a to d25fe8a Compare June 29, 2024 11:56

cyqsimon mentioned this pull request Sep 15, 2024

Make URL encoding fully WHATWG-compliant #1454

Merged

cyqsimon force-pushed the rm-rmdir branch from 5f09fc8 to 7d5374d Compare September 15, 2024 16:11

cyqsimon marked this pull request as ready for review September 15, 2024 16:30

cyqsimon commented Sep 15, 2024

View reviewed changes

tests/fixtures/mod.rs Show resolved Hide resolved

cyqsimon force-pushed the rm-rmdir branch from 700baa6 to 849feb4 Compare September 16, 2024 15:18

cyqsimon force-pushed the rm-rmdir branch from 849feb4 to a90fee6 Compare September 22, 2024 13:29

cyqsimon added 6 commits September 22, 2024 21:34

rm feature backend impl

007baaf

rm feature frontend impl

9d38752

Frontend styling

55804bc

Add tests for rm feature

f566cb4

Separate path_utils.rs from listing.rs

7c97704

This allows the encoding set definitions to be `include!`ed into integration tests

Fix failing tests

2cb106b

cyqsimon force-pushed the rm-rmdir branch from a90fee6 to 2cb106b Compare September 22, 2024 14:29

cyqsimon requested a review from svenstaro September 23, 2024 14:43

svenstaro requested changes Jan 10, 2025

View reviewed changes

		/// The `check_paths_exist` parameter allows skipping this check before and after
		/// the deletion attempt in case these paths should be inaccessible via GET.

File & directory deletion feature #1093

Are you sure you want to change the base?

File & directory deletion feature #1093

Conversation

cyqsimon commented Apr 3, 2023 • edited Loading

Todo

cyqsimon commented Apr 3, 2023

svenstaro commented Apr 3, 2023

bgusach commented Aug 25, 2023

cyqsimon commented Aug 28, 2023

cyqsimon commented Jan 29, 2024 • edited Loading

cyqsimon commented Jan 30, 2024

cyqsimon commented Jun 29, 2024

svenstaro commented Jul 7, 2024

cyqsimon commented Jul 7, 2024

cyqsimon commented Jul 7, 2024

svenstaro commented Jul 7, 2024

cyqsimon commented Sep 15, 2024

cyqsimon commented Sep 15, 2024

svenstaro commented Sep 21, 2024

cyqsimon commented Sep 22, 2024

qtekk commented Oct 16, 2024

cyqsimon commented Oct 16, 2024

svenstaro commented Oct 18, 2024

cyqsimon commented Jan 7, 2025

svenstaro commented Jan 10, 2025

svenstaro commented Jan 10, 2025 • edited Loading

cyqsimon commented Jan 10, 2025 • edited Loading

cyqsimon commented Jan 10, 2025

svenstaro left a comment

Choose a reason for hiding this comment

svenstaro Jan 10, 2025

Choose a reason for hiding this comment

svenstaro Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

svenstaro Jan 10, 2025

Choose a reason for hiding this comment

svenstaro Jan 10, 2025

Choose a reason for hiding this comment

svenstaro Jan 10, 2025

Choose a reason for hiding this comment

svenstaro Jan 10, 2025

Choose a reason for hiding this comment

svenstaro Jan 10, 2025

Choose a reason for hiding this comment

svenstaro commented Jan 10, 2025

svenstaro commented Jan 10, 2025

svenstaro commented Jan 10, 2025

svenstaro commented Jan 28, 2025

cyqsimon commented Jan 28, 2025

cyqsimon commented Apr 3, 2023 •

edited

Loading

cyqsimon commented Jan 29, 2024 •

edited

Loading

svenstaro commented Jan 10, 2025 •

edited

Loading

cyqsimon commented Jan 10, 2025 •

edited

Loading

svenstaro Jan 10, 2025 •

edited

Loading