Refuse an album name and a flag pair that cannot mean anything

src/cli.rs

@@ -552,7 +552,7 @@ pub enum AlbumCommand {
         album: String,
         #[arg(long)]
         name: Option<String>,
-        #[arg(long, short = 'd')]
+        #[arg(long, short = 'd', conflicts_with = "clear_description")]
         description: Option<String>,
         #[arg(long)]
         clear_description: bool,
@@ -994,6 +994,39 @@ mod tests {
         assert!(Cli::try_parse_from(["imogen", "edit", "asset-1", "--clear-description"]).is_ok());
     }
 
+    #[test]
+    fn an_album_description_cannot_be_both_written_and_removed() {
+        // The same contradiction as `edit`'s, in the command that did not have the guard:
+        // both parsed, `album update` took the clearing branch, and the description asked
+        // for was discarded without a word, exit 0.
+        assert!(
+            Cli::try_parse_from([
+                "imogen",
+                "album",
+                "update",
+                "Holiday",
+                "-d",
+                "new text",
+                "--clear-description"
+            ])
+            .is_err(),
+            "an album description cannot both be written and removed"
+        );
+        // Both sides alone, so a `conflicts_with` naming the wrong id cannot stop a
+        // legitimate single flag parsing without a test noticing.
+        assert!(
+            Cli::try_parse_from(["imogen", "album", "update", "Holiday", "-d", "new text"]).is_ok()
+        );
+        assert!(Cli::try_parse_from([
+            "imogen",
+            "album",
+            "update",
+            "Holiday",
+            "--clear-description"
+        ])
+        .is_ok());
+    }
+
     #[test]
     fn a_manifest_and_paths_are_not_both_accepted() {
         // Taking both would leave it unclear which metadata applied to what.

src/commands/albums.rs

@@ -5,7 +5,7 @@ use imogen_sdk::{AlbumCreate, AlbumUpdate, AssetFilter, AssetSelection};
 use serde_json::json;
 
 use crate::cli::{AlbumCommand, QueryArgs};
-use crate::context::Context;
+use crate::context::{album_name, Context};
 use crate::output::GREEN;
 
 pub async fn run(ctx: &Context, command: &AlbumCommand) -> Result<()> {
@@ -131,6 +131,7 @@ async fn create(
     description: Option<&str>,
     assets: &[String],
 ) -> Result<()> {
+    let name = album_name(name)?;
     let album = ctx
         .client
         .albums
@@ -157,6 +158,9 @@ async fn update(
     clear_description: bool,
     cover: Option<&str>,
 ) -> Result<()> {
+    // Before the album is even looked up: a rename that could never be undone by name is
+    // refused whatever else the command was asked to change.
+    let name = name.map(album_name).transpose()?;
     let album = ctx.find_album(reference).await?;
     let patch = AlbumUpdate {
         name: name.map(str::to_string),
@@ -294,3 +298,213 @@ async fn remove(ctx: &Context, reference: &str, assets: &[String]) -> Result<()>
     );
     Ok(())
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use std::sync::{Arc, Mutex};
+
+    use tokio::io::{AsyncReadExt, AsyncWriteExt};
+    use tokio::net::{TcpListener, TcpStream};
+
+    use crate::cli::GlobalArgs;
+    use crate::config::Profile;
+
+    const ONE_ALBUM: &str = r#"{"items":[
+        {"id":"album-1","ownerId":"me","name":"Holiday","description":null,
+         "coverAssetId":null,"assetCount":9,"createdAt":"2024-01-01T00:00:00Z",
+         "updatedAt":"2024-01-01T00:00:00Z","shareSlug":null}
+    ]}"#;
+
+    #[tokio::test]
+    async fn an_album_cannot_be_created_without_a_name() {
+        // `imogen album create "$NAME"` with the variable unset. The name was sent as it
+        // stood, and the album that came back could never be named again: `find_album`
+        // refuses an empty reference, and no non-empty one matches an empty name.
+        for name in ["", "   "] {
+            let stub = stub().await;
+            let error = create(&context(&stub.base_url), name, None, &[])
+                .await
+                .expect_err("an empty name is not a name");
+            assert!(
+                error.to_string().contains("An album needs a name"),
+                "said {error} instead of naming the problem"
+            );
+            assert!(
+                stub.calls().is_empty(),
+                "nothing should reach the wire for a name that could never be used again"
+            );
+        }
+    }
+
+    #[tokio::test]
+    async fn an_album_cannot_be_renamed_to_nothing() {
+        // The same write through the other door: an album that had a usable name loses it.
+        for name in ["", "   "] {
+            let stub = stub().await;
+            let error = update(
+                &context(&stub.base_url),
+                "album-1",
+                Some(name),
+                None,
+                false,
+                None,
+            )
+            .await
+            .expect_err("an empty name is not a name");
+            assert!(
+                error.to_string().contains("An album needs a name"),
+                "said {error} instead of naming the problem"
+            );
+            assert!(
+                stub.calls().is_empty(),
+                "the refusal comes before the album is even looked up"
+            );
+        }
+    }
+
+    #[tokio::test]
+    async fn a_new_album_is_created_under_the_trimmed_name() {
+        // `album create` has to agree with `upload --album`, which already creates under
+        // the trimmed name: otherwise the same string makes two albums that a listing
+        // cannot tell apart. `named_album` compares the stored name trimmed, so a padded
+        // one answers to something it is not spelled as.
+        let stub = stub().await;
+        create(&context(&stub.base_url), " Trip\n", None, &[])
+            .await
+            .unwrap();
+        let sent: serde_json::Value = serde_json::from_str(&stub.sent("post")[0]).unwrap();
+        assert_eq!(sent["name"], "Trip");
+    }
+
+    #[tokio::test]
+    async fn a_rename_is_stored_trimmed_too() {
+        let stub = stub().await;
+        update(
+            &context(&stub.base_url),
+            "album-1",
+            Some(" Trip\n"),
+            None,
+            false,
+            None,
+        )
+        .await
+        .unwrap();
+        let sent: serde_json::Value = serde_json::from_str(&stub.sent("patch")[0]).unwrap();
+        assert_eq!(sent["name"], "Trip");
+    }
+
+    fn context(server: &str) -> Context {
+        let global = GlobalArgs {
+            server: None,
+            profile: None,
+            token: None,
+            json: false,
+            quiet: true,
+            no_color: true,
+        };
+        Context::from_profile(
+            &global,
+            "test",
+            Profile {
+                server: server.to_string(),
+                client_id: None,
+                access_token: Some("token".into()),
+                refresh_token: None,
+                expires_in: 3600,
+                obtained_at: 0,
+                scope: String::new(),
+            },
+            true,
+        )
+    }
+
+    struct Stub {
+        base_url: String,
+        calls: Arc<Mutex<Vec<(String, String)>>>,
+    }
+
+    impl Stub {
+        /// Every request, as method and body.
+        fn calls(&self) -> Vec<(String, String)> {
+            self.calls.lock().unwrap().clone()
+        }
+
+        /// The bodies written by that method, which is what the album was named.
+        fn sent(&self, method: &str) -> Vec<String> {
+            self.calls()
+                .into_iter()
+                .filter(|(sent, _)| sent == method)
+                .map(|(_, body)| body)
+                .collect()
+        }
+    }
+
+    /// A stub imogen over a library of one album, speaking just enough HTTP/1.1 to list
+    /// albums, make one and change one, and recording what it was asked.
+    async fn stub() -> Stub {
+        let listener = TcpListener::bind("127.0.0.1:0").await.unwrap();
+        let base_url = format!("http://{}", listener.local_addr().unwrap());
+        let calls: Arc<Mutex<Vec<(String, String)>>> = Arc::new(Mutex::new(Vec::new()));
+
+        let recorded = calls.clone();
+        tokio::spawn(async move {
+            while let Ok((mut socket, _)) = listener.accept().await {
+                let recorded = recorded.clone();
+                tokio::spawn(async move {
+                    let Some((method, body)) = read_request(&mut socket).await else {
+                        return;
+                    };
+                    let one = r#"{"id":"album-1","ownerId":"me","name":"Trip","description":null,"coverAssetId":null,"assetCount":0,"createdAt":"2024-01-01T00:00:00Z","updatedAt":"2024-01-01T00:00:00Z","shareSlug":null}"#;
+                    let payload = if method == "get" { ONE_ALBUM } else { one };
+                    let response = format!(
+                        "HTTP/1.1 200 OK\r\nContent-Type: application/json\r\nContent-Length: {}\r\nConnection: close\r\n\r\n{payload}",
+                        payload.len()
+                    );
+                    recorded.lock().unwrap().push((method, body));
+                    let _ = socket.write_all(response.as_bytes()).await;
+                    let _ = socket.flush().await;
+                });
+            }
+        });
+
+        Stub { base_url, calls }
+    }
+
+    /// The method asked for, and the body sent with it.
+    async fn read_request(socket: &mut TcpStream) -> Option<(String, String)> {
+        let mut buffer = Vec::new();
+        let mut chunk = [0u8; 4096];
+        let head_end = loop {
+            let read = socket.read(&mut chunk).await.ok()?;
+            if read == 0 {
+                return None;
+            }
+            buffer.extend_from_slice(&chunk[..read]);
+            if let Some(at) = buffer.windows(4).position(|window| window == b"\r\n\r\n") {
+                break at;
+            }
+        };
+
+        let head = String::from_utf8_lossy(&buffer[..head_end]).to_ascii_lowercase();
+        let method = head.split_whitespace().next()?.to_string();
+        let expected: usize = head
+            .lines()
+            .find_map(|line| {
+                line.split_once(':').and_then(|(name, value)| {
+                    (name.trim() == "content-length").then(|| value.trim().parse().ok())?
+                })
+            })
+            .unwrap_or(0);
+
+        let mut body = buffer[head_end + 4..].to_vec();
+        while body.len() < expected {
+            let read = socket.read(&mut chunk).await.ok()?;
+            if read == 0 {
+                break;
+            }
+            body.extend_from_slice(&chunk[..read]);
+        }
+        Some((method, String::from_utf8_lossy(&body).to_string()))
+    }
+}

src/context.rs

@@ -347,6 +347,23 @@ fn named_album(albums: &[Album], reference: &str) -> Result<Option<Album>> {
     }
 }
 
+/// The name an album will be stored under: trimmed, and never empty.
+///
+/// The mirror of the guard in `albums_for`, on the side that writes. An album called ""
+/// is reachable only by its id ever after — an empty reference is refused when reading,
+/// and no non-empty one matches an empty name — so `album show`, `add`, `remove`,
+/// `delete` and `--album` could never pick it again. Trimmed for the same reason the
+/// reference is: `named_album` compares the stored name trimmed, so a name written with
+/// padding would answer to something it is not spelled as, and `album create " Trip "`
+/// would leave a second album a listing cannot tell from `upload --album Trip`'s.
+pub fn album_name(name: &str) -> Result<&str> {
+    let name = name.trim();
+    if name.is_empty() {
+        bail!("An album needs a name — an album with none could only ever be named by its id");
+    }
+    Ok(name)
+}
+
 /// Albums for a refusal to name. Each carries its id, because the names are what was
 /// ambiguous: printing "Holiday, Holiday" tells nobody which album to mean.
 fn describe(albums: &[&Album]) -> String {