Do not overwrite saved media files

[noctux]

If a file with the requested filename already exists in --media-dir, divert the write to a new, uniquely named file (based on the original filename) via the tempfile crate.

It's all a bit complicated because there appears to exist no readily available async variation of mkstemp-like functionality, so there is a need to wrap it in a spawn_blocking, but later reopen it outside for then obtaining an async tokio::fs::file.
Comments on making that more concise are more than welcome :)

[martinetd]

Thanks! This has been annoying me as well!
I was looking at the rust sdk a couple of weeks ago for this and they have a get_media_file() which uses tempfile as well but it's not usable directly (can't keep the temporary file the way they expose it), so I think this is good as you've done.

I made a couple of style comments but I think it's fine to merge as is (I had another patch touching the same code so I've rebased this patch to test, but shouldn't have changed anything)
I agree the async spawn block is a bit ugly but I guess it's fine; I'm half thinking having a pure tokio fs loop trying random filenames would do just as well a tempfile but since you have this working I think it's good.

Let me know what you want, happy to just merge this one as is if you're happy with it, otherwise feel free to fiddle with it until you're happy :)

[martinetd]

Playing with Path's file_stem()/extension() might be a bit more "rust-like" (I'm not convinced and_then() and friends is better than an intermediate let though, just written to play with it)

use std::ffi::OsStr;
use std::path::Path;

fn main() { 
    let p = Path::new("foo.double.ext");

    println!(
        "Hello: {:?} / {:?}",
        p.file_stem()
            .and_then(OsStr::to_str)
            .map(|s| format!("{}-", s))
            .unwrap_or_else(|| "noname-".to_string()),
        p.extension().unwrap_or_default()
    );
}

[noctux]

Yeah, I had that in an earlier version. What put me off a bit of this solution was that it will force me to put a single String through at least two new intermediate representations (Str -> Path(Buf) -> OsStr -> String) with the hidden burden of additional errorhandling as the conversions are assumed to be potentially lossy (which they are not for our case as the input was a valid string and we only add ASCII chars).

But I can see that this makes the intent clearer; even though the code actually got a bit longer than before... I've adapted the PR to use the ideomatic functionality.

[martinetd]

hmm, yeah... I hadn't realized the suffix needs the dot too.

OTOH this can be improved a bit further, I started making the filename a PathBuf instead of path, that avoids the initial String allocation:

    let filename = Path::new(filename);
    tokio::task::spawn_blocking(move || {
        let prefix = filename
            .file_stem()
            .and_then(OsStr::to_str)
            .map(|s| format!("{}-", s))
            .unwrap_or_else(|| "noname-".to_string());
        let suffix = filename
            .extension()
            .and_then(OsStr::to_str)
            .map(|s| format!(".{}", s))
            .unwrap_or_default();

        let (filehandle, filepath) = tempfile::Builder::new()
            .prefix(&prefix)
            .suffix(&suffix)
            .tempfile_in(dir)?
            .keep()?;

But then since both prefix/suffix are also allocated, we can do this before the closure and avoid allocating the PathBuf as well:

    let filename = Path::new(filename);
    let prefix = filename
        .file_stem()
        .and_then(OsStr::to_str)
        .map(|s| format!("{}-", s))
        .unwrap_or_else(|| "noname-".to_string());
    let suffix = filename
        .extension()
        .and_then(OsStr::to_str)
        .map(|s| format!(".{}", s))
        .unwrap_or_default();

    tokio::task::spawn_blocking(move || {
        let (filehandle, filepath) = tempfile::Builder::new()
            .prefix(&prefix)
            .suffix(&suffix)
            .tempfile_in(dir)?
            .keep()?;

So this still has error checks but I think this is optimal in number of allocations (rust shouldn't bother copying if there was no non-utf8 chars)...
Well, it's all nitpicking anyway, we're not saving files thousands of times a second (I think?!) :)

(I'll merge my version anyway since your comment made me look and I think it's a bit cleaner, but I'd have been perfectly happy with your's)

[noctux]

Let me know what you want, happy to just merge this one as is if you're happy with it, otherwise feel free to fiddle with it until you're happy :)

Thank you for the review, your comments all made a lot of sense so I've applied them :) So if these changes are fine with you, this should be ready for merge from my side as well.

[martinetd]

tested both overlapping and non-overlapping file, thank you!

[noctux]

Yeah, that makes a lot of sense. Thanks for the improvements and the merge (and matrircd in general, it seems we have a similar itch to scratch and it seems to be a solid "stick" so far :P)