From c95b36b2de1d8accdc5d04490a266781e3a0d2ca Mon Sep 17 00:00:00 2001 From: Jim Phillips <5315024+ergofobe@users.noreply.github.com> Date: Tue, 15 Sep 2026 23:04:33 -0400 Subject: [PATCH 1/4] Refuse an album reference that cannot pick one album MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `imogen trash --album "$ALBUM"` with the variable unset trashed the library: `contains("")` is true of every name, so an empty reference resolved to whichever album the server listed first and `trash` was handed a filter matching all of it, exit 0. `find_album` also did not trim, so " " did the same. The guard cannot land alone. `album_or_create` matched on `Err(_)`, treating every failure as "not there yet", so a refused empty reference would have made an album called "" — and an ambiguous name was already making a duplicate rather than asking which album was meant. And the exact-name tier took the first album of that name without checking it was the only one; album names have no unique index, so two called "Holiday" resolved arbitrarily. The same shortcut was attempted for people while closing #30 and reintroduced the bug it was closing. So all three together, in one lookup the two callers share: `look_up_album` refuses an empty or ambiguous reference and answers `Ok(None)` only for a name nothing carries, which is the one case `album_or_create` may answer by creating one. The whole-name tier now requires the name to be unique, as `find_person`'s does, and the reference is trimmed once so the guard and the search agree — a name arriving as "Holiday\n" from a shell still finds it. Closes #38 Co-Authored-By: Claude Opus 5 --- src/context.rs | 347 +++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 321 insertions(+), 26 deletions(-) diff --git a/src/context.rs b/src/context.rs index bca1fa5..53b5ca7 100644 --- a/src/context.rs +++ b/src/context.rs @@ -175,29 +175,40 @@ impl Context { Ok(timeline.buckets.iter().map(|bucket| bucket.count).sum()) } - /// An album by id, or by enough of its name to be unambiguous. Naming one is what a - /// person will actually do; refusing an ambiguous name is better than picking one. - pub async fn find_album(&self, reference: &str) -> Result { + /// An album by id, by its whole name when exactly one album carries it, or by enough + /// of its name to be unambiguous. `Ok(None)` is the one case a caller may answer by + /// making an album: an empty reference and an ambiguous one are errors, because + /// neither says which album was meant and creating one is not the answer to either. + /// + /// The reference arrives trimmed, so the guard and the search agree on what it is. + async fn look_up_album(&self, reference: &str) -> Result> { + // `contains("")` is true of every name, so `imogen trash --album "$ALBUM"` with + // the variable unset would otherwise resolve to whichever album was listed first + // and trash all of it. Refused before the lookup, so nothing reaches the wire. + if reference.is_empty() { + bail!("Name an album by id or name — an empty reference cannot pick one"); + } let albums = self.client.albums.list().await?; if let Some(exact) = albums.iter().find(|album| album.id == reference) { - return Ok(exact.clone()); + return Ok(Some(exact.clone())); } let lowered = reference.to_lowercase(); - if let Some(named) = albums + // Only when it is an album's whole name and no other album's. Album names have no + // unique index, so two called "Holiday" fall through to the ambiguity refusal + // below rather than resolving to whichever the server listed first. + let mut whole_name = albums .iter() - .find(|album| album.name.to_lowercase() == lowered) - { - return Ok(named.clone()); + .filter(|album| album.name.to_lowercase() == lowered); + if let (Some(named), None) = (whole_name.next(), whole_name.next()) { + return Ok(Some(named.clone())); } let matches: Vec<&Album> = albums .iter() .filter(|album| album.name.to_lowercase().contains(&lowered)) .collect(); match matches.len() { - 1 => Ok(matches[0].clone()), - 0 => Err(anyhow!( - "No album called \"{reference}\". `imogen album list` shows them, and `imogen album create` makes one." - )), + 1 => Ok(Some(matches[0].clone())), + 0 => Ok(None), _ => { let names: Vec<&str> = matches.iter().map(|a| a.name.as_str()).collect(); Err(anyhow!( @@ -208,33 +219,46 @@ impl Context { } } + /// An album by id, or by enough of its name to be unambiguous. Naming one is what a + /// person will actually do; refusing an ambiguous name is better than picking one. + pub async fn find_album(&self, reference: &str) -> Result { + let reference = reference.trim(); + self.look_up_album(reference).await?.ok_or_else(|| { + anyhow!( + "No album called \"{reference}\". `imogen album list` shows them, and `imogen album create` makes one." + ) + }) + } + /// The album of that name, made if it is not there yet. A description is only used /// when the album is new: it never overwrites one somebody has already written. + /// + /// Only a name nothing answers to is made: this used to treat every failure as "not + /// there yet", so an ambiguous name quietly added a third album of that name. pub async fn album_or_create( &self, reference: &str, description: Option<&str>, ) -> Result { - match self.find_album(reference).await { - Ok(album) => Ok(album), - Err(_) => self - .client - .albums - .create(&imogen_sdk::AlbumCreate { - name: reference.to_string(), - description: description.map(str::to_string), - ..Default::default() - }) - .await - .context("Could not create the album"), + let reference = reference.trim(); + if let Some(album) = self.look_up_album(reference).await? { + return Ok(album); } + self.client + .albums + .create(&imogen_sdk::AlbumCreate { + name: reference.to_string(), + description: description.map(str::to_string), + ..Default::default() + }) + .await + .context("Could not create the album") } /// A person by id, by their whole name when exactly one person carries it, or by /// enough of it to be unambiguous. The whole-name tier is why somebody called "Al" is /// reachable at all rather than lost to "Alice" starting with it. `find_album` has the - /// same three tiers but neither of the guards below: it takes the first exact name - /// without checking it is unique, and accepts an empty reference. + /// same three tiers and the same two guards, for the same reasons. pub async fn find_person(&self, reference: &str) -> Result { // `contains("")` is true of every name, so an unset `$PERSON` in a script would // otherwise resolve to whoever happened to be listed first — and `merge` and @@ -322,3 +346,274 @@ impl From for imogen_sdk::AssetVariant { } } } + +#[cfg(test)] +mod tests { + use super::*; + use std::sync::Mutex; + + use tokio::io::{AsyncReadExt, AsyncWriteExt}; + use tokio::net::{TcpListener, TcpStream}; + + use crate::cli::GlobalArgs; + use crate::config::Profile; + + /// A library with nothing to be ambiguous with, which is where an empty reference does + /// its damage quietly rather than colliding with a second match. + 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} + ]}"#; + + /// One album whose whole name is the start of another's, which is why the exact-name + /// tier exists at all. + const ALBUMS: &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}, + {"id":"album-2","ownerId":"me","name":"Holiday 2024","description":null, + "coverAssetId":null,"assetCount":4,"createdAt":"2024-01-01T00:00:00Z", + "updatedAt":"2024-01-01T00:00:00Z","shareSlug":null}, + {"id":"album-3","ownerId":"me","name":"Weekend","description":null, + "coverAssetId":null,"assetCount":2,"createdAt":"2024-01-01T00:00:00Z", + "updatedAt":"2024-01-01T00:00:00Z","shareSlug":null} + ]}"#; + + /// Album names have no unique index, so two albums can carry the same one. + const TWO_HOLIDAYS: &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}, + {"id":"album-2","ownerId":"me","name":"Holiday","description":null, + "coverAssetId":null,"assetCount":4,"createdAt":"2024-01-01T00:00:00Z", + "updatedAt":"2024-01-01T00:00:00Z","shareSlug":null} + ]}"#; + + #[tokio::test] + async fn an_empty_album_is_refused_before_anything_reaches_the_wire() { + // `imogen trash --album "$ALBUM"` with the variable unset. `contains("")` is true + // of every name, so this used to resolve to the only album in the library and + // hand `trash` a filter that matched all of it, exit 0. + for reference in ["", " "] { + let stub = stub_returning(ONE_ALBUM).await; + let error = context(&stub.base_url) + .to_filter(&QueryArgs { + album: Some(reference.to_string()), + ..Default::default() + }) + .await + .expect_err("an empty --album is not an album"); + assert!( + error.to_string().contains("Name an album"), + "said {error} instead of naming the problem" + ); + assert!( + stub.calls().is_empty(), + "nothing should reach the wire for a reference that cannot pick an album" + ); + } + } + + #[tokio::test] + async fn an_empty_reference_makes_no_album_either() { + // The guard above cannot land on its own: `album_or_create` treated every failure + // as "not there yet", so a refused empty reference would make an album called "". + let stub = stub_returning(ONE_ALBUM).await; + let error = context(&stub.base_url) + .album_or_create(" ", None) + .await + .expect_err("an empty --album is not an album"); + assert!( + error.to_string().contains("Name an album"), + "said {error} instead of naming the problem" + ); + assert!( + stub.created().is_empty(), + "an album that cannot be named should not be created" + ); + } + + #[tokio::test] + async fn a_name_two_albums_share_is_refused() { + let stub = stub_returning(TWO_HOLIDAYS).await; + let error = context(&stub.base_url) + .find_album("Holiday") + .await + .expect_err("two albums are called Holiday"); + assert!( + error.to_string().contains("matches several albums"), + "said {error} instead of refusing" + ); + } + + #[tokio::test] + async fn an_ambiguous_name_creates_no_duplicate() { + // `album_or_create` matched on `Err(_)`, so an ambiguous name made a third album + // of the same name rather than asking which of the two was meant. + let stub = stub_returning(TWO_HOLIDAYS).await; + let error = context(&stub.base_url) + .album_or_create("Holiday", None) + .await + .expect_err("two albums are called Holiday"); + assert!( + error.to_string().contains("matches several albums"), + "said {error} instead of refusing" + ); + assert!( + stub.created().is_empty(), + "an ambiguous name should not add a third album of that name" + ); + } + + #[tokio::test] + async fn an_exact_name_beats_the_longer_one_it_is_a_prefix_of() { + let stub = stub_returning(ALBUMS).await; + let album = context(&stub.base_url).find_album("holiday").await.unwrap(); + assert_eq!(album.id, "album-1"); + } + + #[tokio::test] + async fn a_name_with_a_stray_newline_still_finds_it() { + // `--album "$(cat name.txt)"` arrives as "Holiday\n". Refusing an empty reference + // on its trimmed form and then searching on the untrimmed one would report no + // album called "Holiday\n". + let stub = stub_returning(ALBUMS).await; + let album = context(&stub.base_url) + .find_album("Holiday\n") + .await + .unwrap(); + assert_eq!(album.id, "album-1"); + } + + #[tokio::test] + async fn a_new_album_is_created_under_the_trimmed_name() { + let stub = stub_returning(ONE_ALBUM).await; + context(&stub.base_url) + .album_or_create("Trip\n", None) + .await + .unwrap(); + let sent: serde_json::Value = serde_json::from_str(&stub.created()[0]).unwrap(); + assert_eq!( + sent["name"], "Trip", + "the name searched for is the name made" + ); + } + + 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>>, + } + + impl Stub { + /// Every request, as method, path and body. + fn calls(&self) -> Vec<(String, String, String)> { + self.calls.lock().unwrap().clone() + } + + /// The bodies of the album creations, which is what a duplicate looks like from + /// the outside. + fn created(&self) -> Vec { + self.calls() + .into_iter() + .filter(|(method, path, _)| method == "post" && path == "/api/v1/albums") + .map(|(_, _, body)| body) + .collect() + } + } + + /// A stub imogen over a library of the caller's choosing, speaking just enough + /// HTTP/1.1 to list albums and to make one, and recording what it was asked. + async fn stub_returning(albums: &'static str) -> Stub { + let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); + let base_url = format!("http://{}", listener.local_addr().unwrap()); + let calls: Arc>> = 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, path, body)) = read_request(&mut socket).await else { + return; + }; + let created = r#"{"id":"album-new","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 == "post" { created } else { albums }; + 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, path, body)); + let _ = socket.write_all(response.as_bytes()).await; + let _ = socket.flush().await; + }); + } + }); + + Stub { base_url, calls } + } + + /// The method and path asked for, and the body sent with them. + async fn read_request(socket: &mut TcpStream) -> Option<(String, 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 mut words = head.split_whitespace(); + let method = words.next()?.to_string(); + let path = words.next()?.split('?').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, path, String::from_utf8_lossy(&body).to_string())) + } +} From 6adc58b009f12528084c1355fdf64f48bc9338b1 Mon Sep 17 00:00:00 2001 From: Jim Phillips <5315024+ergofobe@users.noreply.github.com> Date: Tue, 15 Sep 2026 23:07:33 -0400 Subject: [PATCH 2/4] File into an album by its whole name, or make that name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of the lookup above: routing `album_or_create` through the partial-name tier makes a name impossible to have. With "Holiday 2024" and "Holiday 2025" in the library, `imogen upload --album Holiday` now refused as ambiguous, where before it made the album — and with only "Holiday 2024" there it quietly filed the photographs into that one instead, which is the same class of defect as #38 itself: a reference that picked an album nobody named. Filing into an album is a name, not a search, so the two callers now share only the exact tier — `named_album`: an id, or a name exactly one album carries. `find_album` widens to partial names after it, as a lookup should; `album_or_create` makes the name instead. An id that matches nothing is still a new album's name there, which is what `--album` promises. The refusal for two albums of one name lists their ids rather than the same name twice, and says an id is the way out, as `find_person`'s does. Also pins the failure `Err(_)` swallowed: a library that cannot be listed at all is not a library saying there is no such album, and must not become a new one. Co-Authored-By: Claude Opus 5 --- src/context.rs | 179 ++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 139 insertions(+), 40 deletions(-) diff --git a/src/context.rs b/src/context.rs index 53b5ca7..8c0a41f 100644 --- a/src/context.rs +++ b/src/context.rs @@ -175,40 +175,36 @@ impl Context { Ok(timeline.buckets.iter().map(|bucket| bucket.count).sum()) } - /// An album by id, by its whole name when exactly one album carries it, or by enough - /// of its name to be unambiguous. `Ok(None)` is the one case a caller may answer by - /// making an album: an empty reference and an ambiguous one are errors, because - /// neither says which album was meant and creating one is not the answer to either. - /// - /// The reference arrives trimmed, so the guard and the search agree on what it is. - async fn look_up_album(&self, reference: &str) -> Result> { + /// The albums a reference is resolved against, once the reference is one that could + /// pick anything at all. + async fn albums_to_search(&self, reference: &str) -> Result> { // `contains("")` is true of every name, so `imogen trash --album "$ALBUM"` with // the variable unset would otherwise resolve to whichever album was listed first // and trash all of it. Refused before the lookup, so nothing reaches the wire. if reference.is_empty() { bail!("Name an album by id or name — an empty reference cannot pick one"); } - let albums = self.client.albums.list().await?; - if let Some(exact) = albums.iter().find(|album| album.id == reference) { - return Ok(Some(exact.clone())); + Ok(self.client.albums.list().await?) + } + + /// An album by id, or by enough of its name to be unambiguous. Naming one is what a + /// person will actually do; refusing an ambiguous name is better than picking one. + pub async fn find_album(&self, reference: &str) -> Result { + let reference = reference.trim(); + let albums = self.albums_to_search(reference).await?; + if let Some(album) = named_album(&albums, reference)? { + return Ok(album); } let lowered = reference.to_lowercase(); - // Only when it is an album's whole name and no other album's. Album names have no - // unique index, so two called "Holiday" fall through to the ambiguity refusal - // below rather than resolving to whichever the server listed first. - let mut whole_name = albums - .iter() - .filter(|album| album.name.to_lowercase() == lowered); - if let (Some(named), None) = (whole_name.next(), whole_name.next()) { - return Ok(Some(named.clone())); - } let matches: Vec<&Album> = albums .iter() .filter(|album| album.name.to_lowercase().contains(&lowered)) .collect(); match matches.len() { - 1 => Ok(Some(matches[0].clone())), - 0 => Ok(None), + 1 => Ok(matches[0].clone()), + 0 => Err(anyhow!( + "No album called \"{reference}\". `imogen album list` shows them, and `imogen album create` makes one." + )), _ => { let names: Vec<&str> = matches.iter().map(|a| a.name.as_str()).collect(); Err(anyhow!( @@ -219,29 +215,25 @@ impl Context { } } - /// An album by id, or by enough of its name to be unambiguous. Naming one is what a - /// person will actually do; refusing an ambiguous name is better than picking one. - pub async fn find_album(&self, reference: &str) -> Result { - let reference = reference.trim(); - self.look_up_album(reference).await?.ok_or_else(|| { - anyhow!( - "No album called \"{reference}\". `imogen album list` shows them, and `imogen album create` makes one." - ) - }) - } - /// The album of that name, made if it is not there yet. A description is only used /// when the album is new: it never overwrites one somebody has already written. /// - /// Only a name nothing answers to is made: this used to treat every failure as "not - /// there yet", so an ambiguous name quietly added a third album of that name. + /// Only the exact name counts here, where `find_album` would go on to search partial + /// ones: filing into an album is a name, not a search. Settling for "Holiday 2024" + /// when asked for "Holiday" puts photographs somewhere nobody named, and refusing + /// "Holiday" as ambiguous between "Holiday 2024" and "Holiday 2025" would leave a new + /// album of that name impossible to make. pub async fn album_or_create( &self, reference: &str, description: Option<&str>, ) -> Result { let reference = reference.trim(); - if let Some(album) = self.look_up_album(reference).await? { + let albums = self.albums_to_search(reference).await?; + // Only a name nothing carries is made. This used to treat every failure as "not + // there yet", so an ambiguous name quietly added another album of that name — and + // a library that could not be listed at all did the same. + if let Some(album) = named_album(&albums, reference)? { return Ok(album); } self.client @@ -323,6 +315,38 @@ impl Context { } } +/// The album a reference names outright: its id, or a name exactly one album carries. +/// `Ok(None)` is nothing answering to it exactly — which `album_or_create` answers by +/// making one and `find_album` by widening the search to partial names. +/// +/// The whole-name tier has to check that the name is unique, because album names have no +/// unique index: two called "Holiday" are refused rather than resolving to whichever the +/// server listed first. The same shortcut was attempted for people while closing #30 and +/// reintroduced the bug it was closing. +fn named_album(albums: &[Album], reference: &str) -> Result> { + if let Some(exact) = albums.iter().find(|album| album.id == reference) { + return Ok(Some(exact.clone())); + } + let lowered = reference.to_lowercase(); + let carrying: Vec<&Album> = albums + .iter() + .filter(|album| album.name.to_lowercase() == lowered) + .collect(); + match carrying.len() { + 1 => Ok(Some(carrying[0].clone())), + 0 => Ok(None), + // Listing the names would print the same one twice; the ids are what tells them + // apart and what the person needs to pass instead. + _ => { + let ids: Vec<&str> = carrying.iter().map(|a| a.id.as_str()).collect(); + Err(anyhow!( + "Several albums are called \"{reference}\"; use an id instead: {}", + ids.join(", ") + )) + } + } +} + impl QueryArgs { pub fn is_empty(&self) -> bool { self.query.is_none() @@ -390,6 +414,17 @@ mod tests { "updatedAt":"2024-01-01T00:00:00Z","shareSlug":null} ]}"#; + /// Two albums a partial name matches and neither carries, which is a name that can + /// still be made. + const TWO_YEARS: &str = r#"{"items":[ + {"id":"album-1","ownerId":"me","name":"Holiday 2024","description":null, + "coverAssetId":null,"assetCount":9,"createdAt":"2024-01-01T00:00:00Z", + "updatedAt":"2024-01-01T00:00:00Z","shareSlug":null}, + {"id":"album-2","ownerId":"me","name":"Holiday 2025","description":null, + "coverAssetId":null,"assetCount":4,"createdAt":"2024-01-01T00:00:00Z", + "updatedAt":"2024-01-01T00:00:00Z","shareSlug":null} + ]}"#; + #[tokio::test] async fn an_empty_album_is_refused_before_anything_reaches_the_wire() { // `imogen trash --album "$ALBUM"` with the variable unset. `contains("")` is true @@ -436,14 +471,21 @@ mod tests { #[tokio::test] async fn a_name_two_albums_share_is_refused() { + // Album names are not unique. An exact match that took the first of them would be + // the same arbitrary resolution as the empty reference, just harder to notice. let stub = stub_returning(TWO_HOLIDAYS).await; let error = context(&stub.base_url) .find_album("Holiday") .await .expect_err("two albums are called Holiday"); + let said = error.to_string(); assert!( - error.to_string().contains("matches several albums"), - "said {error} instead of refusing" + said.contains("use an id instead"), + "said {said} instead of refusing" + ); + assert!( + said.contains("album-1") && said.contains("album-2"), + "listing the same name twice says nothing; {said} has to name the ids" ); } @@ -457,7 +499,7 @@ mod tests { .await .expect_err("two albums are called Holiday"); assert!( - error.to_string().contains("matches several albums"), + error.to_string().contains("use an id instead"), "said {error} instead of refusing" ); assert!( @@ -466,6 +508,49 @@ mod tests { ); } + #[tokio::test] + async fn filing_into_an_album_goes_by_the_whole_name_or_makes_it() { + // A name is a name here, not a search: uploading into "Holiday" while the library + // holds "Holiday 2024" and "Holiday 2025" makes the album asked for. Neither + // settling for one of them nor refusing the name as ambiguous would leave any way + // to have an album called "Holiday". + let stub = stub_returning(TWO_YEARS).await; + context(&stub.base_url) + .album_or_create("Holiday", None) + .await + .unwrap(); + let sent: serde_json::Value = serde_json::from_str(&stub.created()[0]).unwrap(); + assert_eq!(sent["name"], "Holiday"); + } + + #[tokio::test] + async fn filing_into_an_album_does_not_settle_for_a_longer_name() { + // The single-match half of the same rule: "Holi" is nobody's album name, so it is + // made rather than quietly filing the photographs into "Holiday". + let stub = stub_returning(ONE_ALBUM).await; + context(&stub.base_url) + .album_or_create("Holi", None) + .await + .unwrap(); + let sent: serde_json::Value = serde_json::from_str(&stub.created()[0]).unwrap(); + assert_eq!(sent["name"], "Holi"); + } + + #[tokio::test] + async fn a_library_that_cannot_be_listed_makes_nothing() { + // The failure `Err(_)` used to swallow: a server that cannot say what albums it + // has is not a server saying there is no such album. + let stub = stub_refusing_to_list().await; + context(&stub.base_url) + .album_or_create("Holiday", None) + .await + .expect_err("the albums could not be listed"); + assert!( + stub.created().is_empty(), + "a listing that failed should not become a new album" + ); + } + #[tokio::test] async fn an_exact_name_beats_the_longer_one_it_is_a_prefix_of() { let stub = stub_returning(ALBUMS).await; @@ -550,6 +635,16 @@ mod tests { /// A stub imogen over a library of the caller's choosing, speaking just enough /// HTTP/1.1 to list albums and to make one, and recording what it was asked. async fn stub_returning(albums: &'static str) -> Stub { + stub_listing(Some(albums)).await + } + + /// The same stub, answering the album listing with a rejection. 400 rather than 500, + /// which the SDK retries: the point is a listing that failed, not the waiting. + async fn stub_refusing_to_list() -> Stub { + stub_listing(None).await + } + + async fn stub_listing(albums: Option<&'static str>) -> Stub { let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); let base_url = format!("http://{}", listener.local_addr().unwrap()); let calls: Arc>> = Arc::new(Mutex::new(Vec::new())); @@ -563,9 +658,13 @@ mod tests { return; }; let created = r#"{"id":"album-new","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 == "post" { created } else { albums }; + let (status, payload) = match (method.as_str(), albums) { + ("post", _) => ("200 OK", created), + (_, Some(albums)) => ("200 OK", albums), + (_, None) => ("400 Bad Request", r#"{"error":"nope"}"#), + }; let response = format!( - "HTTP/1.1 200 OK\r\nContent-Type: application/json\r\nContent-Length: {}\r\nConnection: close\r\n\r\n{payload}", + "HTTP/1.1 {status}\r\nContent-Type: application/json\r\nContent-Length: {}\r\nConnection: close\r\n\r\n{payload}", payload.len() ); recorded.lock().unwrap().push((method, path, body)); From 9e0409321bae5c6698c7977aff6a58b88d052ac2 Mon Sep 17 00:00:00 2001 From: Jim Phillips <5315024+ergofobe@users.noreply.github.com> Date: Tue, 15 Sep 2026 23:10:06 -0400 Subject: [PATCH 3/4] Keep the trim with the guard, and say which album is which MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of the two commits above. `albums_to_search` guarded an empty reference but left trimming to its callers, so a third caller passing a raw `--album` value would reintroduce #38 with nothing to catch it. Both now live in `albums_for`, which hands back the trimmed reference alongside the albums: the two cannot be done separately. The partial-name refusal still listed names alone, so two albums called "Holiday" printed as "Holiday, Holiday" — the very thing the whole-name refusal was changed to avoid one tier above. Both now name each album with its id and say an id is the way past. Co-Authored-By: Claude Opus 5 --- src/context.rs | 75 ++++++++++++++++++++++++++++++++++---------------- 1 file changed, 51 insertions(+), 24 deletions(-) diff --git a/src/context.rs b/src/context.rs index 8c0a41f..2d17605 100644 --- a/src/context.rs +++ b/src/context.rs @@ -175,23 +175,26 @@ impl Context { Ok(timeline.buckets.iter().map(|bucket| bucket.count).sum()) } - /// The albums a reference is resolved against, once the reference is one that could - /// pick anything at all. - async fn albums_to_search(&self, reference: &str) -> Result> { + /// The reference as it will be searched for, and the albums to search it against. + /// Trimming and the empty guard live together here, so a caller cannot do one without + /// the other — which is the whole of #38. + async fn albums_for<'a>(&self, reference: &'a str) -> Result<(&'a str, Vec)> { // `contains("")` is true of every name, so `imogen trash --album "$ALBUM"` with // the variable unset would otherwise resolve to whichever album was listed first // and trash all of it. Refused before the lookup, so nothing reaches the wire. + // Trimmed first, so the shell interpolation this is about cannot smuggle an empty + // reference past the guard as " ". + let reference = reference.trim(); if reference.is_empty() { bail!("Name an album by id or name — an empty reference cannot pick one"); } - Ok(self.client.albums.list().await?) + Ok((reference, self.client.albums.list().await?)) } /// An album by id, or by enough of its name to be unambiguous. Naming one is what a /// person will actually do; refusing an ambiguous name is better than picking one. pub async fn find_album(&self, reference: &str) -> Result { - let reference = reference.trim(); - let albums = self.albums_to_search(reference).await?; + let (reference, albums) = self.albums_for(reference).await?; if let Some(album) = named_album(&albums, reference)? { return Ok(album); } @@ -205,13 +208,13 @@ impl Context { 0 => Err(anyhow!( "No album called \"{reference}\". `imogen album list` shows them, and `imogen album create` makes one." )), - _ => { - let names: Vec<&str> = matches.iter().map(|a| a.name.as_str()).collect(); - Err(anyhow!( - "\"{reference}\" matches several albums: {}", - names.join(", ") - )) - } + // Named with their ids, like the refusal in `named_album` and for the same + // reason: two of the matches can carry the same name, and an id is the way + // past either refusal. + _ => Err(anyhow!( + "\"{reference}\" matches several albums; use an id instead: {}", + describe(&matches) + )), } } @@ -228,8 +231,7 @@ impl Context { reference: &str, description: Option<&str>, ) -> Result { - let reference = reference.trim(); - let albums = self.albums_to_search(reference).await?; + let (reference, albums) = self.albums_for(reference).await?; // Only a name nothing carries is made. This used to treat every failure as "not // there yet", so an ambiguous name quietly added another album of that name — and // a library that could not be listed at all did the same. @@ -335,18 +337,23 @@ fn named_album(albums: &[Album], reference: &str) -> Result> { match carrying.len() { 1 => Ok(Some(carrying[0].clone())), 0 => Ok(None), - // Listing the names would print the same one twice; the ids are what tells them - // apart and what the person needs to pass instead. - _ => { - let ids: Vec<&str> = carrying.iter().map(|a| a.id.as_str()).collect(); - Err(anyhow!( - "Several albums are called \"{reference}\"; use an id instead: {}", - ids.join(", ") - )) - } + _ => Err(anyhow!( + "Several albums are called \"{reference}\"; use an id instead: {}", + describe(&carrying) + )), } } +/// 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 { + albums + .iter() + .map(|album| format!("{} ({})", album.name, album.id)) + .collect::>() + .join(", ") +} + impl QueryArgs { pub fn is_empty(&self) -> bool { self.query.is_none() @@ -508,6 +515,26 @@ mod tests { ); } + #[tokio::test] + async fn a_partial_name_matching_several_albums_names_them_by_id_too() { + // The sibling refusal, and the same problem: the matches can carry one name, so + // listing names alone would say "Holiday, Holiday". + let stub = stub_returning(TWO_HOLIDAYS).await; + let error = context(&stub.base_url) + .find_album("holi") + .await + .expect_err("two albums contain holi"); + let said = error.to_string(); + assert!( + said.contains("use an id instead"), + "said {said} instead of refusing" + ); + assert!( + said.contains("album-1") && said.contains("album-2"), + "{said} has to say which album is which" + ); + } + #[tokio::test] async fn filing_into_an_album_goes_by_the_whole_name_or_makes_it() { // A name is a name here, not a search: uploading into "Holiday" while the library From 90422a613cd46845295a2fec9ce22b49cfadbace Mon Sep 17 00:00:00 2001 From: Jim Phillips <5315024+ergofobe@users.noreply.github.com> Date: Tue, 15 Sep 2026 23:12:45 -0400 Subject: [PATCH 4/4] Compare the stored album name trimmed as well MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Third review round. With the reference trimmed and the name not, an album called " Trip " — nothing stops this program, the web interface or an import from writing one — stopped answering to "Trip", and `imogen upload --album Trip` made a second album indistinguishable from the first in a listing. The partial tier used to cover this by accident; the exact tier has to do it on purpose. Co-Authored-By: Claude Opus 5 --- src/context.rs | 27 ++++++++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-) diff --git a/src/context.rs b/src/context.rs index 2d17605..d489332 100644 --- a/src/context.rs +++ b/src/context.rs @@ -330,9 +330,12 @@ fn named_album(albums: &[Album], reference: &str) -> Result> { return Ok(Some(exact.clone())); } let lowered = reference.to_lowercase(); + // The stored name is trimmed to compare, as the reference was: an album called + // " Trip " — from this program, the web interface, or an import — is the album + // somebody filing into "Trip" means, and a second one of that name is not. let carrying: Vec<&Album> = albums .iter() - .filter(|album| album.name.to_lowercase() == lowered) + .filter(|album| album.name.trim().to_lowercase() == lowered) .collect(); match carrying.len() { 1 => Ok(Some(carrying[0].clone())), @@ -421,6 +424,14 @@ mod tests { "updatedAt":"2024-01-01T00:00:00Z","shareSlug":null} ]}"#; + /// An album whose stored name carries padding, which nothing stops a client or an + /// import from writing. + const PADDED_NAME: &str = r#"{"items":[ + {"id":"album-1","ownerId":"me","name":" Trip ","description":null, + "coverAssetId":null,"assetCount":9,"createdAt":"2024-01-01T00:00:00Z", + "updatedAt":"2024-01-01T00:00:00Z","shareSlug":null} + ]}"#; + /// Two albums a partial name matches and neither carries, which is a name that can /// still be made. const TWO_YEARS: &str = r#"{"items":[ @@ -515,6 +526,20 @@ mod tests { ); } + #[tokio::test] + async fn an_album_whose_stored_name_is_padded_is_still_the_album_of_that_name() { + // `imogen album create " Trip "`, or an import that kept the padding. Comparing + // the trimmed reference against the untrimmed name would miss it, and filing into + // "Trip" would make a second album nobody can tell from the first. + let stub = stub_returning(PADDED_NAME).await; + let album = context(&stub.base_url) + .album_or_create("Trip", None) + .await + .unwrap(); + assert_eq!(album.id, "album-1"); + assert!(stub.created().is_empty(), "the album was already there"); + } + #[tokio::test] async fn a_partial_name_matching_several_albums_names_them_by_id_too() { // The sibling refusal, and the same problem: the matches can carry one name, so