diff --git a/src/cli.rs b/src/cli.rs index 2c5be10..7e49822 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -471,7 +471,7 @@ pub struct EditArgs { pub unarchive: bool, /// Set the description - #[arg(long, short = 'd')] + #[arg(long, short = 'd', conflicts_with = "clear_description")] pub description: Option, /// Remove the description @@ -479,7 +479,7 @@ pub struct EditArgs { pub clear_description: bool, /// Correct the capture time, as ISO-8601 or YYYY-MM-DD - #[arg(long)] + #[arg(long, conflicts_with = "reset_captured_at")] pub captured_at: Option, /// Discard a capture-time correction and go back to the imported date @@ -487,7 +487,7 @@ pub struct EditArgs { pub reset_captured_at: bool, /// Set where it was taken: lat,lon[,altitude] - #[arg(long)] + #[arg(long, conflicts_with = "clear_location")] pub location: Option, /// Forget where it was taken @@ -932,6 +932,68 @@ mod tests { ); } + #[test] + fn setting_and_clearing_a_location_are_not_both_accepted() { + // Taking both used to let the clear win silently, which loses the value the + // person actually asked to set. + assert!( + Cli::try_parse_from([ + "imogen", + "edit", + "asset-1", + "--location", + "50.1,-5.5", + "--clear-location" + ]) + .is_err(), + "a location cannot both be set and forgotten" + ); + assert!( + Cli::try_parse_from(["imogen", "edit", "asset-1", "--location", "50.1,-5.5"]).is_ok() + ); + assert!(Cli::try_parse_from(["imogen", "edit", "asset-1", "--clear-location"]).is_ok()); + } + + #[test] + fn setting_and_clearing_the_other_editable_fields_are_not_both_accepted() { + // The same contradiction as --location, in the two sibling pairs of the same + // command. --captured-at with --reset-captured-at reached the server as both, and + // the server drops the date; -d with --clear-description sent a null. + assert!( + Cli::try_parse_from([ + "imogen", + "edit", + "asset-1", + "--captured-at", + "2020-01-01", + "--reset-captured-at" + ]) + .is_err(), + "a capture time cannot both be corrected and reset" + ); + assert!( + Cli::try_parse_from([ + "imogen", + "edit", + "asset-1", + "-d", + "a caption", + "--clear-description" + ]) + .is_err(), + "a description cannot both be written and removed" + ); + // Both sides of each pair 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", "edit", "asset-1", "--reset-captured-at"]).is_ok()); + assert!( + Cli::try_parse_from(["imogen", "edit", "asset-1", "--captured-at", "2020-01-01"]) + .is_ok() + ); + assert!(Cli::try_parse_from(["imogen", "edit", "asset-1", "-d", "a caption"]).is_ok()); + assert!(Cli::try_parse_from(["imogen", "edit", "asset-1", "--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. diff --git a/src/commands/assets.rs b/src/commands/assets.rs index 90b3cc7..2d68241 100644 --- a/src/commands/assets.rs +++ b/src/commands/assets.rs @@ -544,6 +544,8 @@ fn build_patch(args: &EditArgs) -> Result { if args.unarchive { patch.archived = Some(false); } + // Each setting flag below is declared to conflict with the clearing one that follows + // it, so clap has already refused the pair and neither can overwrite the other. if let Some(description) = &args.description { patch.description = Some(Some(description.clone())); } diff --git a/src/commands/people.rs b/src/commands/people.rs index d1bfbd0..aece7b5 100644 --- a/src/commands/people.rs +++ b/src/commands/people.rs @@ -286,7 +286,21 @@ mod tests { const PEOPLE: &str = r#"{"items":[ {"id":"person-1","name":"Alice","coverFaceId":null,"photoCount":3,"hidden":false}, - {"id":"person-2","name":"Bob","coverFaceId":null,"photoCount":1,"hidden":false} + {"id":"person-2","name":"Bob","coverFaceId":null,"photoCount":1,"hidden":false}, + {"id":"person-3","name":"Al","coverFaceId":null,"photoCount":2,"hidden":false} + ]}"#; + + /// A library with nobody to be ambiguous with, which is where an empty reference does + /// its damage quietly rather than colliding with a second match. + const ONE_PERSON: &str = r#"{"items":[ + {"id":"person-1","name":"Alice","coverFaceId":null,"photoCount":3,"hidden":false} + ]}"#; + + /// Two clusters carrying the same whole name, which is the state `people merge` exists + /// to resolve and therefore one an exact-name match has to cope with. + const TWO_ALS: &str = r#"{"items":[ + {"id":"person-1","name":"Al","coverFaceId":null,"photoCount":3,"hidden":false}, + {"id":"person-2","name":"Al","coverFaceId":null,"photoCount":1,"hidden":false} ]}"#; fn person(id: &str, name: Option<&str>) -> Person { @@ -372,6 +386,76 @@ mod tests { assert!(sent["personId"].is_null()); } + #[tokio::test] + async fn an_empty_destination_is_refused_before_anything_moves() { + // `--to "$PERSON"` with the variable unset. `contains("")` is true of every name, + // so this used to resolve to the only person in the library and move faces onto + // them, exit 0. + for reference in ["", " "] { + let stub = stub_returning(ONE_PERSON).await; + let error = reassign( + &context(&stub.base_url), + &["face-1".into()], + Some(reference), + ) + .await + .expect_err("an empty --to is not a person"); + assert!( + error.to_string().contains("Name somebody"), + "said {error} instead of naming the problem" + ); + assert!( + stub.calls().is_empty(), + "nothing should reach the wire for a reference that cannot name anybody" + ); + } + } + + #[tokio::test] + async fn an_exact_name_beats_the_longer_one_it_is_a_prefix_of() { + // "Al" is somebody's whole name as well as the start of "Alice". Refusing it as + // ambiguous left that person unreachable by the name they actually have. + let stub = stub().await; + reassign(&context(&stub.base_url), &["face-1".into()], Some("Al")) + .await + .unwrap(); + + let sent: serde_json::Value = serde_json::from_str(&stub.calls()[1].1).unwrap(); + assert_eq!(sent["personId"], "person-3"); + } + + #[tokio::test] + async fn a_name_two_people_share_is_still_refused() { + // Person 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_ALS).await; + let error = reassign(&context(&stub.base_url), &["face-1".into()], Some("Al")) + .await + .expect_err("two people are called Al"); + assert!( + error.to_string().contains("use an id instead"), + "said {error} instead of refusing" + ); + } + + #[tokio::test] + async fn a_name_with_a_stray_newline_still_finds_them() { + // The same shell interpolation the empty guard is about: `--to "$(cat name.txt)"` + // arrives as "Alice\n". Refusing an empty reference on its trimmed form and then + // searching on the untrimmed one would report nobody called "Alice\n". + let stub = stub().await; + reassign( + &context(&stub.base_url), + &["face-1".into()], + Some("Alice\n"), + ) + .await + .unwrap(); + + let sent: serde_json::Value = serde_json::from_str(&stub.calls()[1].1).unwrap(); + assert_eq!(sent["personId"], "person-1"); + } + fn context(server: &str) -> Context { let global = GlobalArgs { server: None, @@ -412,6 +496,11 @@ mod tests { /// makes and to record what it was asked. The SDK's own conformance suite does the /// same rather than depending on a server framework nothing else here needs. async fn stub() -> Stub { + stub_returning(PEOPLE).await + } + + /// The same stub over a library of the caller's choosing. + async fn stub_returning(people: &'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())); @@ -431,8 +520,8 @@ mod tests { "HTTP/1.1 204 No Content\r\nConnection: close\r\n\r\n".to_string() } else { format!( - "HTTP/1.1 200 OK\r\nContent-Type: application/json\r\nContent-Length: {}\r\nConnection: close\r\n\r\n{PEOPLE}", - PEOPLE.len() + "HTTP/1.1 200 OK\r\nContent-Type: application/json\r\nContent-Length: {}\r\nConnection: close\r\n\r\n{people}", + people.len() ) }; recorded.lock().unwrap().push((path, body)); diff --git a/src/context.rs b/src/context.rs index 69de761..bca1fa5 100644 --- a/src/context.rs +++ b/src/context.rs @@ -230,12 +230,39 @@ impl Context { } } + /// 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. 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 + // `reassign` move data. Refused before the lookup, so nothing reaches the wire. + // Trimmed once, so the guard and the search agree: the shell interpolation this + // is about is also where a trailing newline comes from. + let reference = reference.trim(); + if reference.is_empty() { + bail!("Name somebody by id or name — an empty reference cannot pick anybody"); + } let people = self.client.people.list(true).await?; if let Some(exact) = people.iter().find(|person| person.id == reference) { return Ok(exact.clone()); } let lowered = reference.to_lowercase(); + // Only when it is somebody's whole name and nobody else's. Person names are not + // unique — `people merge` exists because grouping produces two clusters for one + // person — so two called "Al" fall through to the ambiguity refusal below rather + // than resolving to whichever the server listed first. + let mut whole_name = people.iter().filter(|person| { + person + .name + .as_deref() + .is_some_and(|name| name.to_lowercase() == lowered) + }); + if let (Some(named), None) = (whole_name.next(), whole_name.next()) { + return Ok(named.clone()); + } let matches: Vec<&Person> = people .iter() .filter(|person| {