refactor: Remove unused blob functions (#6563)

Author: Hocuri

src/blob.rs

@@ -14,8 +14,7 @@ use image::codecs::jpeg::JpegEncoder;
 use image::ImageReader;
 use image::{DynamicImage, GenericImage, GenericImageView, ImageFormat, Pixel, Rgba};
 use num_traits::FromPrimitive;
-use tokio::io::AsyncWriteExt;
-use tokio::{fs, io, task};
+use tokio::{fs, task};
 use tokio_stream::wrappers::ReadDirStream;
 
 use crate::config::Config;
@@ -48,73 +47,6 @@ enum ImageOutputFormat {
 }
 
 impl<'a> BlobObject<'a> {
-    /// Creates a new file, returning a tuple of the name and the handle.
-    async fn create_new_file(
-        context: &Context,
-        dir: &Path,
-        stem: &str,
-        ext: &str,
-    ) -> Result<(String, fs::File)> {
-        const MAX_ATTEMPT: u32 = 16;
-        let mut attempt = 0;
-        let mut name = format!("{stem}{ext}");
-        loop {
-            attempt += 1;
-            let path = dir.join(&name);
-            match fs::OpenOptions::new()
-                // Using `create_new(true)` in order to avoid race conditions
-                // when creating multiple files with the same name.
-                .create_new(true)
-                .write(true)
-                .open(&path)
-                .await
-            {
-                Ok(file) => return Ok((name, file)),
-                Err(err) => {
-                    if attempt >= MAX_ATTEMPT {
-                        return Err(err).context("failed to create file");
-                    } else if attempt == 1 && !dir.exists() {
-                        fs::create_dir_all(dir).await.log_err(context).ok();
-                    } else {
-                        name = format!("{}-{}{}", stem, rand::random::<u32>(), ext);
-                    }
-                }
-            }
-        }
-    }
-
-    /// Creates a new blob object with unique name by copying an existing file.
-    ///
-    /// This creates a new blob
-    /// and copies an existing file into it.  This is done in a
-    /// in way which avoids race-conditions when multiple files are
-    /// concurrently created.
-    pub async fn create_and_copy(context: &'a Context, src: &Path) -> Result<BlobObject<'a>> {
-        let mut src_file = fs::File::open(src)
-            .await
-            .with_context(|| format!("failed to open file {}", src.display()))?;
-        let (stem, ext) = BlobObject::sanitize_name_and_split_extension(&src.to_string_lossy());
-        let (name, mut dst_file) =
-            BlobObject::create_new_file(context, context.get_blobdir(), &stem, &ext).await?;
-        let name_for_err = name.clone();
-        if let Err(err) = io::copy(&mut src_file, &mut dst_file).await {
-            // Attempt to remove the failed file, swallow errors resulting from that.
-            let path = context.get_blobdir().join(&name_for_err);
-            fs::remove_file(path).await.ok();
-            return Err(err).context("failed to copy file");
-        }
-
-        // Ensure that all buffered bytes are written
-        dst_file.flush().await?;
-
-        let blob = BlobObject {
-            blobdir: context.get_blobdir(),
-            name: format!("$BLOBDIR/{name}"),
-        };
-        context.emit_event(EventType::NewBlobFile(blob.as_name().to_string()));
-        Ok(blob)
-    }
-
     /// Creates a blob object by copying or renaming an existing file.
     /// If the source file is already in the blobdir, it will be renamed,
     /// otherwise it will be copied to the blobdir first.
@@ -209,27 +141,6 @@ impl<'a> BlobObject<'a> {
         })
     }
 
-    /// Creates a blob from a file, possibly copying it to the blobdir.
-    ///
-    /// If the source file is not a path to into the blob directory
-    /// the file will be copied into the blob directory first.  If the
-    /// source file is already in the blobdir it will not be copied
-    /// and only be created if it is a valid blobname, that is no
-    /// subdirectory is used and [BlobObject::sanitize_name_and_split_extension] does not
-    /// modify the filename.
-    ///
-    /// Paths into the blob directory may be either defined by an absolute path
-    /// or by the relative prefix `$BLOBDIR`.
-    pub async fn new_from_path(context: &'a Context, src: &Path) -> Result<BlobObject<'a>> {
-        if src.starts_with(context.get_blobdir()) {
-            BlobObject::from_path(context, src)
-        } else if src.starts_with("$BLOBDIR/") {
-            BlobObject::from_name(context, src.to_str().unwrap_or_default().to_string())
-        } else {
-            BlobObject::create_and_copy(context, src).await
-        }
-    }
-
     /// Returns a [BlobObject] for an existing blob from a path.
     ///
     /// The path must designate a file directly in the blobdir and
@@ -301,50 +212,6 @@ impl<'a> BlobObject<'a> {
         }
     }
 
-    /// The name is returned as a tuple, the first part
-    /// being the stem or basename and the second being an extension,
-    /// including the dot.  E.g. "foo.txt" is returned as `("foo",
-    /// ".txt")` while "bar" is returned as `("bar", "")`.
-    ///
-    /// The extension part will always be lowercased.
-    fn sanitize_name_and_split_extension(name: &str) -> (String, String) {
-        let name = sanitize_filename(name);
-        // Let's take a tricky filename,
-        // "file.with_lots_of_characters_behind_point_and_double_ending.tar.gz" as an example.
-        // Assume that the extension is 32 chars maximum.
-        let ext: String = name
-            .chars()
-            .rev()
-            .take_while(|c| {
-                (!c.is_ascii_punctuation() || *c == '.') && !c.is_whitespace() && !c.is_control()
-            })
-            .take(33)
-            .collect::<Vec<_>>()
-            .iter()
-            .rev()
-            .collect();
-        // ext == "nd_point_and_double_ending.tar.gz"
-
-        // Split it into "nd_point_and_double_ending" and "tar.gz":
-        let mut iter = ext.splitn(2, '.');
-        iter.next();
-
-        let ext = iter.next().unwrap_or_default();
-        let ext = if ext.is_empty() {
-            String::new()
-        } else {
-            format!(".{ext}")
-            // ".tar.gz"
-        };
-        let stem = name
-            .strip_suffix(&ext)
-            .unwrap_or_default()
-            .chars()
-            .take(64)
-            .collect();
-        (stem, ext.to_lowercase())
-    }
-
     /// Checks whether a name is a valid blob name.
     ///
     /// This is slightly less strict than stanitise_name, presumably

src/blob/blob_tests.rs

@@ -104,55 +104,15 @@ async fn test_create_long_names() {
     assert!(blobname.len() < 70);
 }
 
-#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
-async fn test_create_and_copy() {
-    let t = TestContext::new().await;
-    let src = t.dir.path().join("src");
-    fs::write(&src, b"boo").await.unwrap();
-    let blob = BlobObject::create_and_copy(&t, src.as_ref()).await.unwrap();
-    assert_eq!(blob.as_name(), "$BLOBDIR/src");
-    let data = fs::read(blob.to_abs_path()).await.unwrap();
-    assert_eq!(data, b"boo");
-
-    let whoops = t.dir.path().join("whoops");
-    assert!(BlobObject::create_and_copy(&t, whoops.as_ref())
-        .await
-        .is_err());
-    let whoops = t.get_blobdir().join("whoops");
-    assert!(!whoops.exists());
-}
-
-#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
-async fn test_create_from_path() {
-    let t = TestContext::new().await;
-
-    let src_ext = t.dir.path().join("external");
-    fs::write(&src_ext, b"boo").await.unwrap();
-    let blob = BlobObject::new_from_path(&t, src_ext.as_ref())
-        .await
-        .unwrap();
-    assert_eq!(blob.as_name(), "$BLOBDIR/external");
-    let data = fs::read(blob.to_abs_path()).await.unwrap();
-    assert_eq!(data, b"boo");
-
-    let src_int = t.get_blobdir().join("internal");
-    fs::write(&src_int, b"boo").await.unwrap();
-    let blob = BlobObject::new_from_path(&t, &src_int).await.unwrap();
-    assert_eq!(blob.as_name(), "$BLOBDIR/internal");
-    let data = fs::read(blob.to_abs_path()).await.unwrap();
-    assert_eq!(data, b"boo");
-}
 #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
 async fn test_create_from_name_long() {
     let t = TestContext::new().await;
     let src_ext = t.dir.path().join("autocrypt-setup-message-4137848473.html");
     fs::write(&src_ext, b"boo").await.unwrap();
-    let blob = BlobObject::new_from_path(&t, src_ext.as_ref())
-        .await
-        .unwrap();
+    let blob = BlobObject::create_and_deduplicate(&t, &src_ext, &src_ext).unwrap();
     assert_eq!(
         blob.as_name(),
-        "$BLOBDIR/autocrypt-setup-message-4137848473.html"
+        "$BLOBDIR/06f010b24d1efe57ffab44a8ad20c54.html"
     );
 }
 
@@ -166,64 +126,6 @@ fn test_is_blob_name() {
     assert!(!BlobObject::is_acceptible_blob_name("foo\x00bar"));
 }
 
-#[test]
-fn test_sanitise_name() {
-    let (stem, ext) = BlobObject::sanitize_name_and_split_extension(
-        "Я ЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯ.txt",
-    );
-    assert_eq!(ext, ".txt");
-    assert!(!stem.is_empty());
-
-    // the extensions are kept together as between stem and extension a number may be added -
-    // and `foo.tar.gz` should become `foo-1234.tar.gz` and not `foo.tar-1234.gz`
-    let (stem, ext) = BlobObject::sanitize_name_and_split_extension("wot.tar.gz");
-    assert_eq!(stem, "wot");
-    assert_eq!(ext, ".tar.gz");
-
-    let (stem, ext) = BlobObject::sanitize_name_and_split_extension(".foo.bar");
-    assert_eq!(stem, "file");
-    assert_eq!(ext, ".foo.bar");
-
-    let (stem, ext) = BlobObject::sanitize_name_and_split_extension("foo?.bar");
-    assert!(stem.contains("foo"));
-    assert!(!stem.contains('?'));
-    assert_eq!(ext, ".bar");
-
-    let (stem, ext) = BlobObject::sanitize_name_and_split_extension("no-extension");
-    assert_eq!(stem, "no-extension");
-    assert_eq!(ext, "");
-
-    let (stem, ext) =
-        BlobObject::sanitize_name_and_split_extension("path/ignored\\this: is* forbidden?.c");
-    assert_eq!(ext, ".c");
-    assert!(!stem.contains("path"));
-    assert!(!stem.contains("ignored"));
-    assert!(stem.contains("this"));
-    assert!(stem.contains("forbidden"));
-    assert!(!stem.contains('/'));
-    assert!(!stem.contains('\\'));
-    assert!(!stem.contains(':'));
-    assert!(!stem.contains('*'));
-    assert!(!stem.contains('?'));
-
-    let (stem, ext) = BlobObject::sanitize_name_and_split_extension(
-        "file.with_lots_of_characters_behind_point_and_double_ending.tar.gz",
-    );
-    assert_eq!(
-        stem,
-        "file.with_lots_of_characters_behind_point_and_double_ending"
-    );
-    assert_eq!(ext, ".tar.gz");
-
-    let (stem, ext) = BlobObject::sanitize_name_and_split_extension("a. tar.tar.gz");
-    assert_eq!(stem, "a. tar");
-    assert_eq!(ext, ".tar.gz");
-
-    let (stem, ext) = BlobObject::sanitize_name_and_split_extension("Guia_uso_GNB (v0.8).pdf");
-    assert_eq!(stem, "Guia_uso_GNB (v0.8)");
-    assert_eq!(ext, ".pdf");
-}
-
 #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
 async fn test_add_white_bg() {
     let t = TestContext::new().await;
@@ -236,7 +138,7 @@ async fn test_add_white_bg() {
         let avatar_src = t.dir.path().join("avatar.png");
         fs::write(&avatar_src, bytes).await.unwrap();
 
-        let mut blob = BlobObject::new_from_path(&t, &avatar_src).await.unwrap();
+        let mut blob = BlobObject::create_and_deduplicate(&t, &avatar_src, &avatar_src).unwrap();
         let img_wh = 128;
         let maybe_sticker = &mut false;
         let strict_limits = true;
@@ -285,7 +187,7 @@ async fn test_selfavatar_outside_blobdir() {
         constants::BALANCED_AVATAR_SIZE,
     );
 
-    let mut blob = BlobObject::new_from_path(&t, avatar_path).await.unwrap();
+    let mut blob = BlobObject::create_and_deduplicate(&t, avatar_path, avatar_path).unwrap();
     let maybe_sticker = &mut false;
     let strict_limits = true;
     blob.recode_to_size(&t, None, maybe_sticker, 1000, 3000, strict_limits)
@@ -643,9 +545,9 @@ impl SendImageCheckMediaquality<'_> {
         check_image_size(file_saved, compressed_width, compressed_height);
 
         if original_width == compressed_width {
-            assert_extension(&alice, alice_msg, extension).await;
+            assert_extension(&alice, alice_msg, extension);
         } else {
-            assert_extension(&alice, alice_msg, "jpg").await;
+            assert_extension(&alice, alice_msg, "jpg");
         }
 
         let bob_msg = bob.recv_msg(&sent).await;
@@ -668,16 +570,16 @@ impl SendImageCheckMediaquality<'_> {
         let img = check_image_size(file_saved, compressed_width, compressed_height);
 
         if original_width == compressed_width {
-            assert_extension(&bob, bob_msg, extension).await;
+            assert_extension(&bob, bob_msg, extension);
         } else {
-            assert_extension(&bob, bob_msg, "jpg").await;
+            assert_extension(&bob, bob_msg, "jpg");
         }
 
         Ok(img)
     }
 }
 
-async fn assert_extension(context: &TestContext, msg: Message, extension: &str) {
+fn assert_extension(context: &TestContext, msg: Message, extension: &str) {
     assert!(msg
         .param
         .get(Param::File)
@@ -703,8 +605,7 @@ async fn assert_extension(context: &TestContext, msg: Message, extension: &str)
     );
     assert_eq!(
         msg.param
-            .get_blob(Param::File, context)
-            .await
+            .get_file_blob(context)
             .unwrap()
             .unwrap()
             .suffix()

src/chat.rs

@@ -904,8 +904,7 @@ impl ChatId {
                 if msg.viewtype == Viewtype::Vcard {
                     let blob = msg
                         .param
-                        .get_blob(Param::File, context)
-                        .await?
+                        .get_file_blob(context)?
                         .context("no file stored in params")?;
                     msg.try_set_vcard(context, &blob.to_abs_path()).await?;
                 }
@@ -2751,8 +2750,7 @@ async fn prepare_msg_blob(context: &Context, msg: &mut Message) -> Result<()> {
     } else if msg.viewtype.has_file() {
         let mut blob = msg
             .param
-            .get_blob(Param::File, context)
-            .await?
+            .get_file_blob(context)?
             .with_context(|| format!("attachment missing for message of type #{}", msg.viewtype))?;
         let send_as_is = msg.viewtype == Viewtype::File;