better unit testing for FileID

notes.md

@@ -37,10 +37,6 @@
 
 - yvk repo: https://code.yavook.de/jmm/shrupl
 - sharry issue: https://github.com/eikek/sharry/issues/1659
-- ureq: https://stackoverflow.com/questions/59586787/rust-how-to-do-http-put-of-large-files
-- hashing: https://duckduckgo.com/?q=rust+get+file+hash&t=canonical&ia=web
-    - https://stackoverflow.com/q/69787906
-    - https://github.com/RustCrypto/hashes
 
 
 # Ideas

src/appstate.rs

@@ -1,6 +1,6 @@
 use std::{fmt, io, time::Duration};
 
-use indicatif::{ProgressBar, ProgressDrawTarget};
+use indicatif::ProgressBar;
 use log::{debug, warn};
 
 use crate::{
@@ -62,7 +62,6 @@ impl AppState {
 
         if let Some(upl) = self.inner.peek_uploading() {
             if bar.length().is_none() {
-                bar.set_draw_target(ProgressDrawTarget::stderr());
                 bar.set_length(upl.get_size());
                 bar.set_message(upl.get_name().to_owned());
                 bar.enable_steady_tick(Duration::from_millis(100));
@@ -70,8 +69,7 @@ impl AppState {
 
             bar.set_position(upl.get_offset());
             // BUG in `indicatif` crate?
-            // `set_position` does not force an immediate redraw, so we also call `inc_length` here
-            bar.inc_length(0);
+            // `set_position` does not force an immediate redraw like e.g. `inc_length`
         }
 
         f(bar);

src/bin/shrupl.rs

@@ -90,13 +90,13 @@ fn main() {
                     match p {
                         // Error 404 (File not found)
                         error::Parameter::FileID(fid) => {
-                            info!("requeueing file {fid:?}");
+                            info!("retrying file {fid:?}");
 
                             state.abort_upload();
                         }
                         // Error 404 (Share not found)
                         error::Parameter::ShareID(sid) => {
-                            // TODO ask
+                            output::prompt_rebuild_share();
                             info!("rebuilding share {sid:?}");
 
                             // rebuild share

src/cachefile.rs

@@ -6,7 +6,7 @@ use std::{
     time::Duration,
 };
 
-use indicatif::{ProgressBar, ProgressDrawTarget};
+use indicatif::ProgressBar;
 use log::{info, trace};
 use serde::{Deserialize, Serialize};
 
@@ -63,15 +63,7 @@ impl CacheFile {
                 bar: &ProgressBar,
             ) -> error::Result<()> {
                 bar.set_message(format!("checking {:?}", file.get_name()));
-
-                match file.check_hash(|bytes| bar.inc(bytes)) {
-                    Ok(true) => Ok(()),
-                    Ok(false) => Err(error::Error::unknown(format!(
-                        "Hash mismatch for file {:?}!",
-                        file.get_name()
-                    ))),
-                    Err(e) => Err(e.into()),
-                }
+                file.check_hash(|bytes| bar.inc(bytes))
             }
 
             info!("checking files in {state:?}");
@@ -87,7 +79,6 @@ impl CacheFile {
             };
 
             let bar = new_progressbar();
-            bar.set_draw_target(ProgressDrawTarget::stderr());
             bar.set_length(total_size);
             bar.enable_steady_tick(Duration::from_millis(50));
 
@@ -115,7 +106,6 @@ impl CacheFile {
             info!("hashing files {files:?}");
 
             let bar = new_progressbar();
-            bar.set_draw_target(ProgressDrawTarget::stderr());
             // BOOKMARK assumption: total file size < 2 EiB
             bar.set_length(files.iter().map(FileTrait::get_size).sum());
             bar.enable_steady_tick(Duration::from_millis(50));

src/error.rs

@@ -28,12 +28,15 @@ pub enum Error {
     #[error(transparent)]
     StdIo(#[from] std::io::Error),
 
-    #[error("response error: {0}")]
+    #[error("Response error: {0}")]
     Response(String),
 
     #[error("Invalid {0}")]
     InvalidParameter(Parameter),
 
+    #[error("Mismatch, expected {expected:?} but got {actual:?}")]
+    Mismatch { expected: String, actual: String },
+
     #[error("Unknown error: {0}")]
     Unknown(String),
 }
@@ -61,14 +64,18 @@ impl Error {
         Self::Response(into_string(e))
     }
 
-    pub fn unknown(e: impl ToString) -> Self {
-        Self::Unknown(into_string(e))
+    pub fn mismatch(expected: impl ToString, actual: impl ToString) -> Self {
+        Self::Mismatch {
+            expected: into_string(expected),
+            actual: into_string(actual),
+        }
     }
 
     #[must_use]
     pub fn is_fatal(&self) -> bool {
         match self {
             Self::InvalidParameter(p) => p.is_fatal(),
+            Self::Mismatch { .. } => true,
             Self::Unknown(_) => true,
             _ => false,
         }

src/file/checked.rs

@@ -53,12 +53,9 @@ impl Checked {
         }
     }
 
-    pub fn hash(&mut self, f: impl Fn(u64)) -> io::Result<()> {
+    pub fn hash(&mut self, f: impl Fn(u64)) -> error::Result<()> {
         if self.hash.is_some() {
-            return Err(io::Error::other(format!(
-                "file {:?} is already hashed!",
-                self.path.display()
-            )));
+            return Err(error::Error::mismatch("unhashed file", self.path.display()));
         }
 
         self.hash = Some(super::compute_file_hash(&self.path, self.size, f)?);
@@ -101,7 +98,7 @@ impl<'t> FileTrait<'t> for Checked {
         self.size
     }
 
-    fn check_hash(&self, on_progress: impl Fn(u64)) -> io::Result<bool> {
+    fn check_hash(&self, on_progress: impl Fn(u64)) -> error::Result<()> {
         super::check_file_hash(&self.path, self.size, self.hash.as_ref(), on_progress)
     }
 }

src/file/mod.rs

@@ -2,22 +2,19 @@ mod checked;
 mod chunk;
 mod uploading;
 
-use std::{
-    ffi::OsStr,
-    fs,
-    io::{self, Read},
-    path::Path,
-};
+use std::{ffi::OsStr, fs, io::Read, path::Path};
 
 use base64ct::{Base64, Encoding};
 use blake2b_simd::Params as Blake2b;
 
 pub use checked::Checked;
 pub use chunk::Chunk;
-use log::debug;
+use log::{debug, warn};
 pub use uploading::Uploading;
 
-fn compute_file_hash(path: &Path, size: u64, on_progress: impl Fn(u64)) -> io::Result<String> {
+use crate::error;
+
+fn compute_file_hash(path: &Path, size: u64, on_progress: impl Fn(u64)) -> error::Result<String> {
     let mut file = fs::File::open(path)?;
     let mut hasher = Blake2b::new().hash_length(64).to_state();
 
@@ -36,9 +33,7 @@ fn compute_file_hash(path: &Path, size: u64, on_progress: impl Fn(u64)) -> io::R
     }
 
     if bytes_read != size {
-        return Err(io::Error::other(format!(
-            "Hashed {bytes_read:?} bytes, known file size {size:?}!"
-        )));
+        return Err(error::Error::mismatch(size, bytes_read));
     }
 
     let result = Base64::encode_string(hasher.finalize().as_bytes());
@@ -51,15 +46,20 @@ fn check_file_hash(
     size: u64,
     hash: Option<&String>,
     on_progress: impl Fn(u64),
-) -> io::Result<bool> {
-    let Some(hash) = hash else {
-        debug!("no hash to check for {:?}!", path.display());
-        return Ok(false);
+) -> error::Result<()> {
+    let Some(expected) = hash else {
+        return Err(error::Error::mismatch("hash", path.display()));
     };
 
-    let result = *hash == compute_file_hash(path, size, on_progress)?;
-    debug!("matches {:?}: {result:?}", *hash);
-    Ok(result)
+    let actual = &compute_file_hash(path, size, on_progress)?;
+
+    if expected == actual {
+        debug!("hash matches {expected:?}");
+        Ok(())
+    } else {
+        warn!("hash mismatch for file {:?}", path.display());
+        Err(error::Error::mismatch(expected, actual))
+    }
 }
 
 pub trait FileTrait<'t> {
@@ -80,5 +80,5 @@ pub trait FileTrait<'t> {
     /// get the file's size
     fn get_size(&self) -> u64;
 
-    fn check_hash(&self, on_progress: impl Fn(u64)) -> io::Result<bool>;
+    fn check_hash(&self, on_progress: impl Fn(u64)) -> error::Result<()>;
 }

src/file/uploading.rs

@@ -7,7 +7,7 @@ use std::{
 use log::warn;
 use serde::{Deserialize, Serialize};
 
-use crate::sharry;
+use crate::{error, sharry};
 
 use super::{Checked, Chunk, FileTrait};
 
@@ -108,7 +108,7 @@ impl<'t> FileTrait<'t> for Uploading {
         self.size
     }
 
-    fn check_hash(&self, on_progress: impl Fn(u64)) -> io::Result<bool> {
+    fn check_hash(&self, on_progress: impl Fn(u64)) -> error::Result<()> {
         super::check_file_hash(&self.path, self.size, self.hash.as_ref(), on_progress)
     }
 }

src/impl_ureq.rs

@@ -42,7 +42,7 @@ fn find_cause(
                 error.into()
             }
         }
-        error => error::Error::unknown(error),
+        error => error::Error::Unknown(error.to_string()),
     }
 }
 

src/output.rs

@@ -1,9 +1,9 @@
 use std::{fmt, process, sync::LazyLock};
 
 use console::{StyledObject, style};
-use dialoguer::{Select, theme::ColorfulTheme};
+use dialoguer::{Confirm, Select, theme::ColorfulTheme};
 use indicatif::{ProgressBar, ProgressStyle};
-use log::{error, info};
+use log::{info, warn};
 
 type StaticStyled<'t> = LazyLock<StyledObject<&'t str>>;
 
@@ -36,6 +36,23 @@ pub fn prompt_continue() -> bool {
     selection == 0
 }
 
+pub fn prompt_rebuild_share() {
+    let prompt = format!(
+        "Target Share cannot be accessed. {}",
+        style("Completely restart upload?").cyan()
+    );
+
+    let selection = Confirm::with_theme(&ColorfulTheme::default())
+        .with_prompt(prompt)
+        .default(true)
+        .interact()
+        .unwrap_or(false);
+
+    if selection == false {
+        process::exit(0);
+    }
+}
+
 pub fn style_all<'t, F>(strs: &[&'t str], f: F) -> Vec<String>
 where
     F: Fn(StyledObject<&'t str>) -> StyledObject<&'t str>,
@@ -46,7 +63,7 @@ where
 #[must_use]
 #[allow(clippy::missing_panics_doc)]
 pub fn new_progressbar() -> ProgressBar {
-    ProgressBar::hidden().with_style(
+    ProgressBar::no_length().with_style(
         ProgressStyle::with_template(&format!(
             concat!(
                 "{{bar:50.cyan/blue}} {{msg:.magenta}}: ",
@@ -78,7 +95,7 @@ impl Log {
     pub fn handle(e: &crate::error::Error) {
         if e.is_fatal() {
             // react to fatal error
-            error!("fatal error: {e:?}");
+            warn!("fatal error: {e:?}");
             Self::error(e);
         }
 

src/sharry/api/id.rs

@@ -56,9 +56,10 @@ impl TryFrom<String> for FileID {
 
             Ok(result)
         } else {
-            Err(error::Error::unknown(format!(
-                "Could not extract File ID from {value:?}"
-            )))
+            Err(error::Error::mismatch(
+                "<proto>://<base>/api/v2/alias/upload/<share>/files/tus/<file>",
+                value,
+            ))
         }
     }
 }
@@ -68,16 +69,62 @@ mod tests {
     use super::*;
 
     #[test]
-    fn test_fileid_tryfrom_string() {
-        let good = "https://example.com/api/v2/alias/upload/SID123/files/tus/FID456".to_owned();
-        let good = FileID::try_from(good);
-        assert!(good.is_ok());
-        assert_eq!(good.unwrap().as_ref(), "FID456");
+    fn valid_urls_produce_expected_file_id() {
+        // a handful of valid‐looking URLs
+        let cases = vec![
+            (
+                "http://example.com/api/v2/alias/upload/SID123/files/tus/FID456",
+                "FID456",
+            ),
+            (
+                "https://my-host:8080/api/v2/alias/upload/another-SID/files/tus/some-file-id",
+                "some-file-id",
+            ),
+            (
+                "custom+scheme://host/api/v2/alias/upload/x/files/tus/y",
+                "y",
+            ),
+        ];
 
-        let bad = "https://example.com/api/v2/alias/upload//files/tus/FID456".to_owned(); // missing SID
-        assert!(FileID::try_from(bad).is_err());
+        for (good, expected_fid) in cases {
+            let s = good.to_string();
+            let file_id = FileID::try_from(s.clone()).expect("URL should parse successfully");
+            assert_eq!(
+                file_id.0, expected_fid,
+                "Expected `{}` → FileID({}), got {:?}",
+                good, expected_fid, file_id
+            );
+        }
+    }
 
-        let bad = "https://example.com/api/v2/alias/upload/SID123/files/tus/".to_owned(); // missing FID
-        assert!(FileID::try_from(bad).is_err());
+    #[test]
+    fn invalid_urls_return_error() {
+        let bad_inputs = vec![
+            // missing /api/v2/alias/upload
+            "http://example.com/files/tus/FID",
+            // missing /files/tus
+            "http://example.com/api/v2/alias/upload/SID123/FID456",
+            // trailing slash (doesn't match `$`)
+            "http://example.com/api/v2/alias/upload/SID/files/tus/FID/",
+            // empty fid
+            "http://example.com/api/v2/alias/upload/SID/files/tus/",
+            // random string
+            "just-a-random-string",
+        ];
+
+        for bad in bad_inputs {
+            let err = FileID::try_from(bad.to_string()).expect_err("URL should not parse");
+            // make sure it's the Mismatch variant, and that it contains the original input
+            match err {
+                error::Error::Mismatch { expected, actual } => {
+                    assert_eq!(
+                        expected, "<proto>://<base>/api/v2/alias/upload/<share>/files/tus/<file>",
+                        "Error should output expected format"
+                    );
+                    assert_eq!(actual, bad.to_string(), "Error should echo back the input");
+                }
+                _ => panic!("Expected Error::Mismatch for input `{bad}` but got {err:?}"),
+            }
+        }
     }
 }