From 42c7cc03a49fb846ee95cfa68ffa5d4ef6ac6f04 Mon Sep 17 00:00:00 2001 From: Bruno-Alumn <171815746+Bruno-Alumn@users.noreply.github.com> Date: Wed, 21 Oct 2026 01:07:47 +0000 Subject: [PATCH 1/4] Commands: an id or ids that isn't a photo id is an error, not the selection Several commands read their photo parameters leniently. An `ids` that wasn't a list of whole numbers (`"ids": 3`, `["3"]`) fell back to the selection or the active photo, and an `id` that wasn't a whole number (`"id": "5"`) fell back to the active photo, so a call meant for one photo acted on another: - ids: album.addPhotos / removePhotos / toggleTarget, library.select, develop.set, denoise.queue, faces.index, mask.applyEffect (mask ids) and app.export; - id: photo.relink (relinked the active photo to the file), photo.removeRegion / setRegion, faces.setName, stack.setTop / moveUp / moveDown / split, library.findSimilar, the photo queries, denoise.cancel (cancelled every photo) / status and ui.render. They now refuse such values with an error, as Session::targets already does for the other commands (shared cmd::photo_ids / photo_id). A missing or null value still means the selection or the active photo. --- crates/engine/src/cmd/cull.rs | 2 +- crates/engine/src/cmd/denoise.rs | 10 +-- crates/engine/src/cmd/develop.rs | 3 +- crates/engine/src/cmd/face_recognize.rs | 5 +- crates/engine/src/cmd/library.rs | 18 ++-- crates/engine/src/cmd/mask_effects.rs | 8 +- crates/engine/src/cmd/missing.rs | 2 +- crates/engine/src/cmd/mod.rs | 26 ++++++ crates/engine/src/cmd/organize.rs | 8 +- crates/engine/src/cmd/query.rs | 2 +- crates/engine/src/lib.rs | 2 + crates/engine/src/tests_ids_params.rs | 106 ++++++++++++++++++++++++ crates/ui-egui/src/control.rs | 18 +++- 13 files changed, 175 insertions(+), 35 deletions(-) create mode 100644 crates/engine/src/tests_ids_params.rs diff --git a/crates/engine/src/cmd/cull.rs b/crates/engine/src/cmd/cull.rs index 030cb6e93..1908bb70d 100644 --- a/crates/engine/src/cmd/cull.rs +++ b/crates/engine/src/cmd/cull.rs @@ -87,7 +87,7 @@ fn analyze(s: &mut Session, p: &Value) -> Result { /// content), most similar first; the view is filtered to them. fn find_similar(s: &mut Session, p: &Value) -> Result { const C: &str = "library.findSimilar"; - let id = p.get("id").and_then(Value::as_u64).map(PhotoId).or(s.active()).ok_or_else(|| super::bad(C, "no photo"))?; + let id = super::photo_id(p, C)?.or(s.active()).ok_or_else(|| super::bad(C, "no photo"))?; let min = p.get("similarity").and_then(Value::as_f64).unwrap_or(0.8) as f32; let all: Vec = s.catalog.photos().filter(|q| q.in_library()).map(|q| q.id).collect(); let sig = |s: &mut Session, id: PhotoId| -> Option<[f32; 64]> { diff --git a/crates/engine/src/cmd/denoise.rs b/crates/engine/src/cmd/denoise.rs index f93a2434a..644d3b89e 100644 --- a/crates/engine/src/cmd/denoise.rs +++ b/crates/engine/src/cmd/denoise.rs @@ -513,10 +513,6 @@ fn settings(s: &mut Session, p: &Value) -> Result { Ok(json!({"auto": st.auto(), "cacheGb": st.cache_bytes() >> 30, "threads": st.threads, "runOn": st.run_on().name()})) } -fn ids_param(p: &Value) -> Option> { - p.get("ids").and_then(Value::as_array).map(|a| a.iter().filter_map(Value::as_u64).map(PhotoId).collect()) -} - fn status_json(s: &Session, photo: Option) -> Value { let (files, bytes) = s.denoise_cache_usage(); let st = &s.denoise.settings; @@ -594,7 +590,7 @@ fn queue(s: &mut Session, p: &Value) -> Result { if s.denoise_products_dir().is_none() { return Err(bad(C, "denoise keeps its pictures in the library's folder: open a library on disk first")); } - let ids = match ids_param(p) { + let ids = match super::photo_ids(p, C)? { Some(ids) => ids, None => match str_param(p, "scope").unwrap_or("selected") { "selected" => s.selection.active.iter().chain(s.selection.ids.iter()).copied().collect(), @@ -612,14 +608,14 @@ fn queue(s: &mut Session, p: &Value) -> Result { } fn cancel(s: &mut Session, p: &Value) -> Result { - let photo = p.get("id").and_then(Value::as_u64).map(PhotoId); + let photo = super::photo_id(p, "denoise.cancel")?; let n = s.denoise_cancel(photo, bool_or(p, "all", photo.is_none())); Ok(json!({"cancelled": n})) } fn status(s: &mut Session, p: &Value) -> Result { s.denoise_refresh_active(false); - Ok(status_json(s, p.get("id").and_then(Value::as_u64).map(PhotoId))) + Ok(status_json(s, super::photo_id(p, "denoise.status")?)) } /// `denoise.pump {pace?}`: cheap to call every frame. Takes in what the background job has finished and keeps one going. diff --git a/crates/engine/src/cmd/develop.rs b/crates/engine/src/cmd/develop.rs index e3d59c567..8074fc6bf 100644 --- a/crates/engine/src/cmd/develop.rs +++ b/crates/engine/src/cmd/develop.rs @@ -199,8 +199,7 @@ pub fn specs() -> Vec { } refit_crop_angle(angle, before, d, dims); }; - if let Some(ids) = p.get("ids").and_then(Value::as_array) { - let ids: Vec = ids.iter().filter_map(Value::as_u64).map(PhotoId).collect(); + if let Some(ids) = super::photo_ids(p, "develop.set")? { let ops = ids .iter() .filter_map(|id| s.develop_of(*id).map(|d| (*id, d))) diff --git a/crates/engine/src/cmd/face_recognize.rs b/crates/engine/src/cmd/face_recognize.rs index 228a1ce3d..4ac7c4628 100644 --- a/crates/engine/src/cmd/face_recognize.rs +++ b/crates/engine/src/cmd/face_recognize.rs @@ -82,7 +82,7 @@ fn name_faces(s: &mut Session, p: &Value) -> Result { /// makes the face yours: a later "Detect Faces" run no longer replaces it. fn set_name(s: &mut Session, p: &Value) -> Result { const C: &str = "faces.setName"; - let id = p.get("id").and_then(Value::as_u64).map(lightcraft_catalog::PhotoId).or_else(|| s.active()).ok_or_else(|| bad(C, "no photo"))?; + let id = super::photo_id(p, C)?.or_else(|| s.active()).ok_or_else(|| bad(C, "no photo"))?; let index = p.get("index").and_then(Value::as_u64).and_then(|i| usize::try_from(i).ok()).ok_or_else(|| bad(C, "missing or invalid `index`"))?; let name = name_param(p, C)?; let mut meta = s.catalog.photo(id).ok_or_else(|| bad(C, "no such photo"))?.meta.clone(); @@ -470,8 +470,7 @@ mod imp { let embedder = current(s, C)?; collect(s); let budget = Duration::from_millis(p.get("budgetMs").and_then(Value::as_u64).unwrap_or(200).min(600_000)); - let first: Vec = - p.get("ids").and_then(Value::as_array).map(|a| a.iter().filter_map(Value::as_u64).map(PhotoId).collect()).unwrap_or_default(); + let first: Vec = super::super::photo_ids(p, C)?.unwrap_or_default(); let todo = pending(s, &first, false); let started = Instant::now(); let (mut embedded, mut failed, mut done) = (0, 0, 0); diff --git a/crates/engine/src/cmd/library.rs b/crates/engine/src/cmd/library.rs index 860cf089f..5a295202b 100644 --- a/crates/engine/src/cmd/library.rs +++ b/crates/engine/src/cmd/library.rs @@ -145,10 +145,6 @@ fn strs(p: &Value, key: &str) -> Vec { p.get(key).and_then(Value::as_array).map(|a| a.iter().filter_map(Value::as_str).map(str::to_string).collect()).unwrap_or_default() } -fn ids_param(p: &Value) -> Option> { - p.get("ids").and_then(Value::as_array).map(|a| a.iter().filter_map(Value::as_u64).map(PhotoId).collect()) -} - /// The targets an album can hold and how many were browsed (Local) photos, which album views /// never show (issue #684): those are skipped, and reported, instead of becoming invisible members. fn album_targets(s: &Session, targets: Vec) -> (Vec, usize) { @@ -623,7 +619,7 @@ pub fn specs() -> Vec { "{ids: [id], active?: id, mode?: replace|add|toggle|range} — every id must be a photo of the library", always, |s, p| { - let ids = ids_param(p).unwrap_or_default(); + let ids = super::photo_ids(p, "library.select")?.unwrap_or_default(); let active = p.get("active").and_then(Value::as_u64).map(PhotoId); let mode = str_param(p, "mode").unwrap_or("replace"); // An id that is not in the library is an error, and the selection stays as it was: @@ -829,8 +825,7 @@ pub fn specs() -> Vec { "{id?, index} — remove one face / pet region (by its position in the photo's regions) from the photo in the catalog; undoable. The XMP sidecar is never rewritten for this, even with auto-write on, so reading the metadata from the file brings the region back", always, |s, p| { - let id = - p.get("id").and_then(Value::as_u64).map(PhotoId).or_else(|| s.active()).ok_or_else(|| bad("photo.removeRegion", "no photo"))?; + let id = super::photo_id(p, "photo.removeRegion")?.or_else(|| s.active()).ok_or_else(|| bad("photo.removeRegion", "no photo"))?; let index = p .get("index") .and_then(Value::as_u64) @@ -856,7 +851,7 @@ pub fn specs() -> Vec { |s, p| { const C: &str = "photo.setRegion"; // a drag is one undo step however many frames it took: the caller commits once, on release - let id = p.get("id").and_then(Value::as_u64).map(PhotoId).or_else(|| s.active()).ok_or_else(|| bad(C, "no photo"))?; + let id = super::photo_id(p, C)?.or_else(|| s.active()).ok_or_else(|| bad(C, "no photo"))?; let index = p .get("index") .and_then(Value::as_u64) @@ -1267,7 +1262,7 @@ pub fn specs() -> Vec { has_library_selection, |s, p| { let id = album_param(p, "id", "album.addPhotos")?; - let targets = ids_param(p).unwrap_or_else(|| s.selected()); + let targets = super::photo_ids(p, "album.addPhotos")?.unwrap_or_else(|| s.selected()); let al = s.catalog.album(id).ok_or_else(|| bad("album.addPhotos", "no such album"))?; if al.is_smart() || al.folder { return Err(bad("album.addPhotos", "smart albums and folders can't hold photos")); @@ -1296,7 +1291,8 @@ pub fn specs() -> Vec { "{ids?} — B: adds the photos to the target album (the Quick Collection unless one is set), or removes them when they're all in it → {album, added, skipped}; Local photos are skipped", has_library_selection, |s, p| { - let targets = album_targets_or_err(s, "album.toggleTarget", ids_param(p).unwrap_or_else(|| s.selected()))?; + let targets = + album_targets_or_err(s, "album.toggleTarget", super::photo_ids(p, "album.toggleTarget")?.unwrap_or_else(|| s.selected()))?; let (targets, skipped) = targets; let target = s.target_album.filter(|a| s.catalog.album(*a).is_some_and(|al| !al.is_smart() && !al.folder)); let id = match target.or_else(|| s.catalog.quick_collection()) { @@ -1458,7 +1454,7 @@ pub fn specs() -> Vec { _ => return Err(bad("album.removePhotos", "no album: pass `id` or show an album")), }, }; - let targets = ids_param(p).unwrap_or_else(|| s.selected()); + let targets = super::photo_ids(p, "album.removePhotos")?.unwrap_or_else(|| s.selected()); let al = s.catalog.album(id).ok_or_else(|| bad("album.removePhotos", "no such album"))?; if al.is_smart() { return Err(bad("album.removePhotos", "smart albums update automatically: change their rules")); diff --git a/crates/engine/src/cmd/mask_effects.rs b/crates/engine/src/cmd/mask_effects.rs index 387909a6d..270abeed5 100644 --- a/crates/engine/src/cmd/mask_effects.rs +++ b/crates/engine/src/cmd/mask_effects.rs @@ -341,8 +341,12 @@ fn apply_effect(s: &mut Session, p: &Value) -> Result { // the masks: `ids`, `all`, else `id` or the selected one let ids: Vec = if p.get("all").and_then(Value::as_bool).unwrap_or(false) { d.masks.iter().map(|m| m.id).collect() - } else if let Some(a) = p.get("ids").and_then(Value::as_array) { - a.iter().filter_map(Value::as_u64).filter_map(|v| u32::try_from(v).ok()).collect() + } else if let Some(v) = p.get("ids").filter(|v| !v.is_null()) { + // mask ids; one that isn't (`ids: 3`, a string, a negative) is an error, not the selected mask + let a = v.as_array().ok_or_else(|| bad(C, "`ids` must be a list of mask ids"))?; + a.iter() + .map(|v| v.as_u64().and_then(|v| u32::try_from(v).ok()).ok_or_else(|| bad(C, "`ids` must be mask ids (whole numbers)"))) + .collect::>>()? } else { vec![selected_mask(s, p, C)?.id] }; diff --git a/crates/engine/src/cmd/missing.rs b/crates/engine/src/cmd/missing.rs index 6f194c652..1bff30d2b 100644 --- a/crates/engine/src/cmd/missing.rs +++ b/crates/engine/src/cmd/missing.rs @@ -58,7 +58,7 @@ pub(crate) fn relink_op(id: PhotoId, path: &str) -> Op { fn relink(s: &mut Session, p: &Value) -> Result { const C: &str = "photo.relink"; - let id = p.get("id").and_then(Value::as_u64).map(PhotoId).or(s.active()).ok_or_else(|| bad(C, "no photo (give `id`)"))?; + let id = super::photo_id(p, C)?.or(s.active()).ok_or_else(|| bad(C, "no photo (give `id`)"))?; let path = str_param(p, "path").ok_or_else(|| bad(C, "missing `path`"))?; let abs = std::path::absolute(path).map_err(|e| bad(C, e.to_string()))?; if !abs.is_file() { diff --git a/crates/engine/src/cmd/mod.rs b/crates/engine/src/cmd/mod.rs index 4b86d4012..762c357d7 100644 --- a/crates/engine/src/cmd/mod.rs +++ b/crates/engine/src/cmd/mod.rs @@ -192,6 +192,32 @@ pub(crate) fn names_photos(p: &Value) -> bool { ["ids", "id"].iter().any(|k| p.get(k).is_some_and(|v| !v.is_null())) } +/// The photos `ids` names: `None` when it is missing or null (the command then falls back to the +/// selection, or its own default), else every entry as a photo id. An `ids` that isn't a list of +/// whole numbers is an error, never `None`: falling back would act on the selection instead of +/// the photos the call meant (`{"ids": 3}`, `{"ids": ["3"]}`). +pub(crate) fn photo_ids(p: &Value, cmd: &str) -> Result>> { + match p.get("ids") { + None | Some(Value::Null) => Ok(None), + Some(Value::Array(a)) => a + .iter() + .map(|v| v.as_u64().map(lightcraft_catalog::PhotoId).ok_or_else(|| bad(cmd, "`ids` must be photo ids (whole numbers)"))) + .collect::>>() + .map(Some), + Some(_) => Err(bad(cmd, "`ids` must be a list of photo ids")), + } +} + +/// The photo `id` names: `None` when it is missing or null (the command then uses the active +/// photo), else that id. An `id` that isn't a whole number is an error, never `None`: falling back +/// would act on the active photo instead of the one the call meant (`{"id": "5"}`). +pub(crate) fn photo_id(p: &Value, cmd: &str) -> Result> { + match p.get("id") { + None | Some(Value::Null) => Ok(None), + Some(v) => v.as_u64().map(|n| Some(lightcraft_catalog::PhotoId(n))).ok_or_else(|| bad(cmd, "`id` must be a photo id (a whole number)")), + } +} + pub(crate) fn bad(cmd: &str, msg: impl Into) -> EngineError { EngineError::BadParams { cmd: cmd.into(), msg: msg.into() } } diff --git a/crates/engine/src/cmd/organize.rs b/crates/engine/src/cmd/organize.rs index 24cc2061c..df71695b9 100644 --- a/crates/engine/src/cmd/organize.rs +++ b/crates/engine/src/cmd/organize.rs @@ -161,17 +161,17 @@ pub fn specs() -> Vec { Ok(json!({"changed": commit_ops(s, "Remove from Stack", ops)?})) }), cmd!("stack.setTop", "Set as Top of Stack", ["Photo", "Stack"], Some("Shift+S"), "{id?} (default: the active photo)", in_stack, |s, p| { - let id = p.get("id").and_then(Value::as_u64).map(PhotoId).or(s.active()).ok_or_else(|| bad("stack.setTop", "no photo"))?; + let id = super::photo_id(p, "stack.setTop")?.or(s.active()).ok_or_else(|| bad("stack.setTop", "no photo"))?; let op = s.catalog.set_top_ops(id); Ok(json!({"changed": commit_ops(s, "Set Top of Stack", op.into_iter().collect())?})) }), cmd!("stack.moveUp", "Move Up in Stack", ["Photo", "Stack"], None, "{id?} — one place towards the top", in_stack, |s, p| { - let id = p.get("id").and_then(Value::as_u64).map(PhotoId).or(s.active()).ok_or_else(|| bad("stack.moveUp", "no photo"))?; + let id = super::photo_id(p, "stack.moveUp")?.or(s.active()).ok_or_else(|| bad("stack.moveUp", "no photo"))?; let op = s.catalog.move_in_stack_ops(id, -1); Ok(json!({"changed": commit_ops(s, "Move in Stack", op.into_iter().collect())?})) }), cmd!("stack.moveDown", "Move Down in Stack", ["Photo", "Stack"], None, "{id?} — one place away from the top", in_stack, |s, p| { - let id = p.get("id").and_then(Value::as_u64).map(PhotoId).or(s.active()).ok_or_else(|| bad("stack.moveDown", "no photo"))?; + let id = super::photo_id(p, "stack.moveDown")?.or(s.active()).ok_or_else(|| bad("stack.moveDown", "no photo"))?; let op = s.catalog.move_in_stack_ops(id, 1); Ok(json!({"changed": commit_ops(s, "Move in Stack", op.into_iter().collect())?})) }), @@ -183,7 +183,7 @@ pub fn specs() -> Vec { "{id?} — this photo and the ones after it become their own stack", in_stack, |s, p| { - let id = p.get("id").and_then(Value::as_u64).map(PhotoId).or(s.active()).ok_or_else(|| bad("stack.split", "no photo"))?; + let id = super::photo_id(p, "stack.split")?.or(s.active()).ok_or_else(|| bad("stack.split", "no photo"))?; let op = s.catalog.split_stack_ops(id).ok_or_else(|| bad("stack.split", "pick a photo below the top of a stack"))?; Ok(json!({"changed": commit_ops(s, "Split Stack", vec![op])?})) } diff --git a/crates/engine/src/cmd/query.rs b/crates/engine/src/cmd/query.rs index fbe4608f0..d131f4704 100644 --- a/crates/engine/src/cmd/query.rs +++ b/crates/engine/src/cmd/query.rs @@ -62,7 +62,7 @@ fn album_json(a: &Album, all: &[Album], cat: &lightcraft_catalog::Catalog, depth /// The photo a query is about: `id` when given (which must be in the library), else the active one. fn photo_arg(s: &Session, p: &Value, c: &str) -> crate::Result { - match p.get("id").and_then(Value::as_u64).map(PhotoId) { + match super::photo_id(p, c)? { Some(id) if s.catalog.photo(id).is_some() => Ok(id), Some(id) => Err(bad(c, format!("no such photo {}", id.0))), None => s.active().ok_or_else(|| bad(c, "no photo")), diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index 25e6ef52f..50a86a786 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -1275,6 +1275,8 @@ mod tests_history_cursor; #[cfg(test)] mod tests_hold_view; #[cfg(test)] +mod tests_ids_params; +#[cfg(test)] mod tests_import; #[cfg(test)] mod tests_import_move; diff --git a/crates/engine/src/tests_ids_params.rs b/crates/engine/src/tests_ids_params.rs new file mode 100644 index 000000000..1eae90473 --- /dev/null +++ b/crates/engine/src/tests_ids_params.rs @@ -0,0 +1,106 @@ +//! `ids` that aren't photo ids are an error, never "the selection": a command that fell back to the +//! selection on `{"ids": 3}` acted on photos the call didn't name. + +use lightcraft_catalog::{AlbumId, PhotoId}; +use serde_json::{Value, json}; + +use crate::Session; + +/// A demo session with the first two photos selected, the third active elsewhere in the grid, and +/// an album holding the selection. +fn session() -> (Session, Vec, AlbumId) { + let mut s = Session::with_demo(); + let vis = s.visible_cloned(); + s.execute("library.select", &json!({"ids": [vis[0].0, vis[1].0], "active": vis[0].0})).unwrap(); + let r = s.execute("album.create", &json!({"name": "Trip", "addSelected": true})).unwrap(); + let album = AlbumId(r["id"].as_u64().unwrap()); + (s, vis.to_vec(), album) +} + +const BAD_IDS: [fn() -> Value; 5] = [|| json!(3), || json!("3"), || json!(["3"]), || json!([3, -1]), || json!({"id": 3})]; + +#[test] +fn album_commands_refuse_ids_that_are_not_photo_ids() { + let (mut s, vis, album) = session(); + let members = |s: &Session| s.catalog.album(album).unwrap().photos.clone(); + let before = members(&s); + assert_eq!(before.len(), 2); + for bad in BAD_IDS.map(|f| f()) { + for cmd in ["album.removePhotos", "album.addPhotos"] { + let e = s.execute(cmd, &json!({"id": album.0, "ids": bad})).unwrap_err(); + assert!(e.to_string().contains("ids"), "{cmd} {bad}: {e}"); + assert_eq!(members(&s), before, "{cmd} {bad} changed the album"); + } + assert!(s.execute("album.toggleTarget", &json!({"ids": bad})).is_err(), "album.toggleTarget {bad}"); + } + // well-formed ids still work, and no ids still means the selection + s.execute("album.removePhotos", &json!({"id": album.0, "ids": [vis[1].0]})).unwrap(); + assert_eq!(members(&s), vec![vis[0]]); + s.execute("album.addPhotos", &json!({"id": album.0, "ids": null})).unwrap(); + assert_eq!(members(&s).len(), 2); +} + +#[test] +fn develop_set_refuses_ids_that_are_not_photo_ids() { + let (mut s, vis, _) = session(); + let exposure = |s: &Session, id: PhotoId| s.develop_of(id).unwrap().light.exposure; + let before: Vec = vis.iter().map(|id| exposure(&s, *id)).collect(); + for bad in BAD_IDS.map(|f| f()) { + let e = s.execute("develop.set", &json!({"control": "light.exposure", "value": 2.5, "ids": bad})).unwrap_err(); + assert!(e.to_string().contains("ids"), "{bad}: {e}"); + assert_eq!(vis.iter().map(|id| exposure(&s, *id)).collect::>(), before, "{bad} edited a photo"); + } + s.execute("develop.set", &json!({"control": "light.exposure", "value": 2.5, "ids": [vis[2].0]})).unwrap(); + assert_eq!((exposure(&s, vis[0]), exposure(&s, vis[2])), (before[0], 2.5)); +} + +#[test] +fn library_select_refuses_ids_that_are_not_photo_ids() { + let (mut s, vis, _) = session(); + let before = s.selection.ids.clone(); + for bad in BAD_IDS.map(|f| f()) { + assert!(s.execute("library.select", &json!({"ids": bad})).is_err(), "{bad}"); + assert_eq!(s.selection.ids, before, "{bad} changed the selection"); + } + s.execute("library.select", &json!({"ids": [vis[2].0]})).unwrap(); + assert_eq!(s.selection.ids, vec![vis[2]]); +} + +/// The same for a single `id`: a value that isn't a photo id used to mean the active photo +/// (`photo.relink` then relinked the active photo to the file, `photo.removeRegion` removed its face +/// box), or, for `denoise.cancel`, every photo. (The stack commands share the helper; `in_stack` +/// keeps them disabled on a photo outside a stack, so they aren't called here.) +#[test] +fn single_photo_commands_refuse_an_id_that_is_not_a_photo_id() { + let (mut s, vis, _) = session(); + let active = s.active().unwrap(); + let mut meta = s.catalog.photo(active).unwrap().meta.clone(); + meta.regions = vec![lightcraft_meta::Region { + rect: lightcraft_geom::Rect { x0: 0.1, y0: 0.1, x1: 0.3, y1: 0.3 }, + kind: lightcraft_meta::RegionKind::Face, + name: None, + description: None, + }]; + s.commit("setup", lightcraft_catalog::Op::SetMeta { id: active, meta: Box::new(meta) }).unwrap(); + let source = |s: &Session| format!("{:?}", s.catalog.photo(active).unwrap().source); + let before = source(&s); + let commits = s.commits(); + for bad in [json!("5"), json!(-1), json!(1.5), json!([vis[2].0]), json!({"id": 1})] { + for (cmd, mut params) in [ + ("photo.relink", json!({"path": "/elsewhere/other.jpg"})), + ("photo.removeRegion", json!({"index": 0})), + ("photo.setRegion", json!({"index": 0, "rect": {"x0": 0.2, "y0": 0.2, "x1": 0.4, "y1": 0.4}})), + ("faces.setName", json!({"index": 0, "name": "Ann"})), + ("library.findSimilar", json!({})), + ("denoise.cancel", json!({})), + ("photo.inspect", json!({})), + ] { + params["id"] = bad.clone(); + let e = s.execute(cmd, ¶ms).unwrap_err(); + assert!(e.to_string().contains("`id`"), "{cmd} {bad}: {e}"); + } + } + assert_eq!(s.commits(), commits, "nothing was changed"); + assert_eq!(source(&s), before); + assert_eq!(s.catalog.photo(active).unwrap().meta.regions.len(), 1); +} diff --git a/crates/ui-egui/src/control.rs b/crates/ui-egui/src/control.rs index 097b13c4d..ad2193daf 100644 --- a/crates/ui-egui/src/control.rs +++ b/crates/ui-egui/src/control.rs @@ -438,7 +438,13 @@ pub fn handle(app: &mut LightcraftApp, ctx: &egui::Context, req: &ControlRequest Outcome::Screenshot { path: s("path").map(str::to_string), headless: p.get("headless").and_then(Value::as_bool).unwrap_or(false) } } "ui.render" => { - let id = p.get("id").and_then(Value::as_u64).map(lightcraft_catalog::PhotoId).or(app.session.active()); + let id = match p.get("id").filter(|v| !v.is_null()) { + Some(v) => match v.as_u64() { + Some(n) => Some(lightcraft_catalog::PhotoId(n)), + None => return err("`id` must be a photo id (a whole number)"), + }, + None => app.session.active(), + }; let Some(id) = id else { return err("no photo") }; if let Some(Err(e)) = s("path").map(|path| app.session.check_write_target(path)) { return err(e); @@ -489,8 +495,14 @@ pub fn export_active(app: &mut LightcraftApp, p: &Value) -> Result = match p.get("ids").and_then(Value::as_array) { - Some(a) => a.iter().filter_map(|v| serde_json::from_value(v.clone()).ok()).collect(), + let ids: Vec<_> = match p.get("ids").filter(|v| !v.is_null()) { + // `ids` that aren't photo ids are an error, not "export the selection" + Some(v) => v + .as_array() + .ok_or("app.export: `ids` must be a list of photo ids")? + .iter() + .map(|v| v.as_u64().map(lightcraft_catalog::PhotoId).ok_or("app.export: `ids` must be photo ids (whole numbers)")) + .collect::>()?, None if !app.session.selection.ids.is_empty() => { // Batch order (and `{seq}`) follows the grid order. let sel: std::collections::HashSet<_> = app.session.selection.ids.iter().copied().collect(); From 622988d7f71ed8765d4474a20aa853e096ec40f6 Mon Sep 17 00:00:00 2001 From: Bruno-Alumn <171815746+Bruno-Alumn@users.noreply.github.com> Date: Wed, 21 Oct 2026 01:39:29 +0000 Subject: [PATCH 2/4] Masks and spots: an id or index that isn't one is an error, not the selected one mask_id read `id` with `as_u64().map(|v| v as u32)`, so text fell back to the selected mask (mask.delete {"id": "1"} deleted it) and an id past u32 wrapped onto another mask (4294967297 as u32 is 1). spot_index and spot.select did the same with `index`, and a colour range's `component` fell back to the first colour range part. They now refuse such values, as the photo ids do; a missing or null value still means the selection. --- crates/engine/src/cmd/masks.rs | 33 ++++++++++++++++++----- crates/engine/src/tests_ids_params.rs | 39 +++++++++++++++++++++++++++ 2 files changed, 65 insertions(+), 7 deletions(-) diff --git a/crates/engine/src/cmd/masks.rs b/crates/engine/src/cmd/masks.rs index 0efb3b658..4f7191ceb 100644 --- a/crates/engine/src/cmd/masks.rs +++ b/crates/engine/src/cmd/masks.rs @@ -199,8 +199,27 @@ fn connected_strokes(from: Point, last: &BrushStroke, to: BrushStroke) -> Vec
Result> { + match p.get("id") { + None | Some(Value::Null) => Ok(None), + Some(v) => v.as_u64().and_then(|n| u32::try_from(n).ok()).map(Some).ok_or_else(|| bad(c, "`id` must be a mask id")), + } +} + fn mask_id(p: &Value, active: Option, c: &str) -> Result { - p.get("id").and_then(Value::as_u64).map(|v| v as u32).or(active).ok_or_else(|| bad(c, "no mask selected (give `id`)")) + opt_mask_id(p, c)?.or(active).ok_or_else(|| bad(c, "no mask selected (give `id`)")) +} + +/// A whole-number index parameter `key`: `None` when it is missing or null, else an error unless it +/// is one (an index that isn't must not fall back to the selected item). +fn opt_index(p: &Value, key: &str, c: &str) -> Result> { + match p.get(key) { + None | Some(Value::Null) => Ok(None), + Some(v) => v.as_u64().and_then(|n| usize::try_from(n).ok()).map(Some).ok_or_else(|| bad(c, format!("`{key}` must be a whole number"))), + } } fn find(masks: &mut [Mask], id: u32, c: &str) -> Result { @@ -307,7 +326,7 @@ pub fn specs() -> Vec { let connect = bool_or(p, "connect", false); let mut connected = false; let next = s.active().and_then(|id| s.develop_of(id)).map(|d| d.next_mask_id()).unwrap_or(1); - let want = p.get("id").and_then(Value::as_u64).map(|v| v as u32).or(s.active_mask); + let want = opt_mask_id(p, "mask.brushStroke")?.or(s.active_mask); let adjust = super::mask_effects::new_mask_adjust(s, "brush"); let mut out = masks_edit(s, "mask.brushStroke", "Brush", |masks, active| { let i = match want.and_then(|m| masks.iter().position(|x| x.id == m)) { @@ -391,7 +410,7 @@ pub fn specs() -> Vec { let lab = lightcraft_pipeline::color_range_sample(&src, &info, &d, &req, lightcraft_geom::Point::new(x, y)) .ok_or_else(|| bad(c, "the point is outside the photo"))?; let mid = mask_id(p, s.active_mask, c)?; - let comp = p.get("component").and_then(Value::as_u64).map(|v| v as usize); + let comp = opt_index(p, "component", c)?; let mut out = Vec::new(); masks_edit(s, c, "Color Range", |masks, _| { let i = find(masks, mid, c)?; @@ -604,7 +623,7 @@ pub fn specs() -> Vec { } ), cmd!("mask.select", "Select Mask", [], None, "{id|null}", has_active, |s, p| { - s.active_mask = p.get("id").and_then(Value::as_u64).map(|v| v as u32); + s.active_mask = opt_mask_id(p, "mask.select")?; ok() }), cmd!("mask.delete", "Delete Mask", [], None, "{id?}", has_active, |s, p| { @@ -879,8 +898,8 @@ pub fn specs() -> Vec { ), cmd!("spot.select", "Select Spot", [], None, "{index|null}", has_active, |s, p| { let n = spots_len(s); - s.active_spot = match p.get("index").and_then(Value::as_u64) { - Some(i) if (i as usize) < n => Some(i as usize), + s.active_spot = match opt_index(p, "index", "spot.select")? { + Some(i) if i < n => Some(i), Some(_) => return Err(bad("spot.select", "no such spot")), None => None, }; @@ -1040,7 +1059,7 @@ fn spots_len(s: &Session) -> usize { /// `index` from the params, else the selected spot. fn spot_index(s: &Session, p: &Value, c: &str) -> Result { - let i = p.get("index").and_then(Value::as_u64).map(|v| v as usize).or(s.active_spot).ok_or_else(|| bad(c, "no spot selected (give `index`)"))?; + let i = opt_index(p, "index", c)?.or(s.active_spot).ok_or_else(|| bad(c, "no spot selected (give `index`)"))?; if i >= spots_len(s) { return Err(bad(c, "no such spot")); } diff --git a/crates/engine/src/tests_ids_params.rs b/crates/engine/src/tests_ids_params.rs index 1eae90473..824dbedc2 100644 --- a/crates/engine/src/tests_ids_params.rs +++ b/crates/engine/src/tests_ids_params.rs @@ -104,3 +104,42 @@ fn single_photo_commands_refuse_an_id_that_is_not_a_photo_id() { assert_eq!(source(&s), before); assert_eq!(s.catalog.photo(active).unwrap().meta.regions.len(), 1); } + +/// Mask ids and spot indexes the same way: `mask.delete {"id": "1"}` deleted the selected mask, and an +/// id past `u32` wrapped (`4294967297 as u32` is 1) onto another mask. +#[test] +fn mask_and_spot_commands_refuse_ids_that_are_not_ids() { + let (mut s, _, _) = session(); + s.execute("mask.add", &json!({"kind": "linear"})).unwrap(); + s.execute("mask.add", &json!({"kind": "radial"})).unwrap(); + s.execute("spot.add", &json!({"mode": "heal", "points": [[0.4, 0.5]], "size": 0.02, "source": [0.1, 0.0]})).unwrap(); + let state = |s: &Session| { + let d = s.develop_of(s.active().unwrap()).unwrap(); + (d.masks.iter().map(|m| (m.id, m.name.clone(), m.visible)).collect::>(), d.spots.len()) + }; + let before = state(&s); + assert_eq!((before.0.len(), before.1), (2, 1)); + let first = before.0[0].0; + let commits = s.commits(); + for bad in [json!(u64::from(u32::MAX) + 1 + u64::from(first)), json!("1"), json!(-1), json!(1.5), json!([first])] { + for (cmd, params) in [ + ("mask.delete", json!({})), + ("mask.rename", json!({"name": "renamed"})), + ("mask.visible", json!({"visible": false})), + ("mask.select", json!({})), + ] { + let mut params = params; + params["id"] = bad.clone(); + assert!(s.execute(cmd, ¶ms).is_err(), "{cmd} {bad}"); + } + for cmd in ["spot.delete", "spot.select"] { + assert!(s.execute(cmd, &json!({"index": bad})).is_err(), "{cmd} {bad}"); + } + } + assert_eq!(s.commits(), commits, "nothing was changed"); + assert_eq!(state(&s), before); + // well-formed ids and a null still work + s.execute("mask.delete", &json!({"id": first})).unwrap(); + s.execute("spot.select", &json!({"index": null})).unwrap(); + assert_eq!(state(&s).0.len(), 1); +} From 205a7ec1201519250bf004333ad47ed5419e2016 Mon Sep 17 00:00:00 2001 From: Bruno-Alumn <171815746+Bruno-Alumn@users.noreply.github.com> Date: Wed, 21 Oct 2026 01:49:30 +0000 Subject: [PATCH 3/4] photo.label: a missing label, or one that isn't text, is an error, not a clear photo.label read its label with str_param, which is None both when the label is missing and when it isn't a string, and None meant "none": so photo.label {} or {"label": 3} cleared the colour label of every selected photo. Only "none" or null clear now; anything else that isn't a known label is an error. Every caller in the app already passes a label. --- crates/engine/src/cmd/library.rs | 10 +++++++--- crates/engine/src/tests_ids_params.rs | 20 ++++++++++++++++++++ 2 files changed, 27 insertions(+), 3 deletions(-) diff --git a/crates/engine/src/cmd/library.rs b/crates/engine/src/cmd/library.rs index 5a295202b..3c6cd3bbf 100644 --- a/crates/engine/src/cmd/library.rs +++ b/crates/engine/src/cmd/library.rs @@ -804,9 +804,13 @@ pub fn specs() -> Vec { "{label: red|yellow|green|blue|purple|none, ids?}", has_selection, |s, p| { - let l = match str_param(p, "label") { - None | Some("none") => None, - Some(x) => Some(ColorLabel::parse(x).ok_or_else(|| bad("photo.label", "unknown label"))?), + // `none` (or null) clears; a missing label or one that isn't text is an error, not a clear + let l = match p.get("label") { + Some(Value::Null) => None, + Some(Value::String(x)) if x == "none" => None, + Some(Value::String(x)) => Some(ColorLabel::parse(x).ok_or_else(|| bad("photo.label", "unknown label"))?), + None => return Err(bad("photo.label", "missing `label` (red|yellow|green|blue|purple|none)")), + Some(_) => return Err(bad("photo.label", "label must be red|yellow|green|blue|purple|none")), }; for_targets(s, p, "Set Color Label", |id| Some(Op::SetLabel { id, label: l })) } diff --git a/crates/engine/src/tests_ids_params.rs b/crates/engine/src/tests_ids_params.rs index 824dbedc2..daff1f7ae 100644 --- a/crates/engine/src/tests_ids_params.rs +++ b/crates/engine/src/tests_ids_params.rs @@ -143,3 +143,23 @@ fn mask_and_spot_commands_refuse_ids_that_are_not_ids() { s.execute("spot.select", &json!({"index": null})).unwrap(); assert_eq!(state(&s).0.len(), 1); } + +/// `photo.label` without a label, or with one that isn't text, cleared the colour label of every +/// selected photo (a missing `label` read as `none`); now it is an error and only `none` or null clear. +#[test] +fn photo_label_refuses_a_missing_or_non_text_label() { + let (mut s, vis, _) = session(); + s.execute("photo.label", &json!({"label": "red"})).unwrap(); + let labels = |s: &Session| vis.iter().take(2).map(|id| s.catalog.photo(*id).unwrap().label).collect::>(); + let before = labels(&s); + assert!(before.iter().all(Option::is_some)); + for params in [json!({}), json!({"label": 3}), json!({"label": true}), json!({"label": ["red"]}), json!({"label": "crimson"})] { + assert!(s.execute("photo.label", ¶ms).is_err(), "{params}"); + assert_eq!(labels(&s), before, "{params} changed the labels"); + } + s.execute("photo.label", &json!({"label": null})).unwrap(); + assert!(labels(&s).iter().all(Option::is_none)); + s.execute("photo.label", &json!({"label": "green"})).unwrap(); + s.execute("photo.label", &json!({"label": "none"})).unwrap(); + assert!(labels(&s).iter().all(Option::is_none)); +} From cbf809a37eb83ddc4646ed8e90824fe348aa9acf Mon Sep 17 00:00:00 2001 From: Bruno-Alumn <171815746+Bruno-Alumn@users.noreply.github.com> Date: Wed, 21 Oct 2026 12:03:42 +0000 Subject: [PATCH 4/4] Masks: mask-effect ids and component indexes that aren't whole numbers are errors mask.applyEffect, saveEffect and updateEffect read an `id` that isn't a mask id as the selected mask (applyEffect {"id": "1"} changed mask 2); mask.update, mask.component and mask.refineDetail cast `component` with as-casts and fell back to component 0 or the last selection. They share the strict opt_mask_id / opt_index helpers now. --- crates/engine/src/cmd/mask_effects.rs | 8 ++---- crates/engine/src/cmd/masks.rs | 10 +++---- crates/engine/src/tests_ids_params.rs | 40 +++++++++++++++++++++++++++ 3 files changed, 47 insertions(+), 11 deletions(-) diff --git a/crates/engine/src/cmd/mask_effects.rs b/crates/engine/src/cmd/mask_effects.rs index 270abeed5..549434e02 100644 --- a/crates/engine/src/cmd/mask_effects.rs +++ b/crates/engine/src/cmd/mask_effects.rs @@ -287,12 +287,8 @@ fn effect_index(s: &Session, name: &str, c: &str) -> Result { /// The selected mask's adjustments on the active photo. fn selected_mask(s: &Session, p: &Value, c: &str) -> Result { let id = s.active().ok_or_else(|| bad(c, "no active photo"))?; - let mid = p - .get("id") - .and_then(Value::as_u64) - .and_then(|v| u32::try_from(v).ok()) - .or(s.active_mask) - .ok_or_else(|| bad(c, "no mask selected (give `id`)"))?; + // an `id` that isn't a mask id is an error, not the selected mask + let mid = super::masks::opt_mask_id(p, c)?.or(s.active_mask).ok_or_else(|| bad(c, "no mask selected (give `id`)"))?; let d = s.develop_of(id).unwrap_or_default(); d.masks.iter().find(|m| m.id == mid).cloned().ok_or_else(|| bad(c, format!("no mask {mid}"))) } diff --git a/crates/engine/src/cmd/masks.rs b/crates/engine/src/cmd/masks.rs index 4f7191ceb..e39f8aa3d 100644 --- a/crates/engine/src/cmd/masks.rs +++ b/crates/engine/src/cmd/masks.rs @@ -202,7 +202,7 @@ fn connected_strokes(from: Point, last: &BrushStroke, to: BrushStroke) -> Vec
Result> { +pub(crate) fn opt_mask_id(p: &Value, c: &str) -> Result> { match p.get("id") { None | Some(Value::Null) => Ok(None), Some(v) => v.as_u64().and_then(|n| u32::try_from(n).ok()).map(Some).ok_or_else(|| bad(c, "`id` must be a mask id")), @@ -382,7 +382,7 @@ pub fn specs() -> Vec { cmd!("mask.update", "Update Mask Shape", [], None, "{id?, component?: index (0), shape: MaskShape JSON}", has_active, |s, p| { let shape: MaskShape = serde_json::from_value(p.get("shape").cloned().unwrap_or_default()).map_err(|e| bad("mask.update", e.to_string()))?; - let comp = p.get("component").and_then(Value::as_u64).unwrap_or(0) as usize; + let comp = opt_index(p, "component", "mask.update")?.unwrap_or(0); let mid = mask_id(p, s.active_mask, "mask.update")?; masks_edit(s, "mask.update", "Edit Mask", |masks, _| { let i = find(masks, mid, "mask.update")?; @@ -501,8 +501,8 @@ pub fn specs() -> Vec { let mid = mask_id(p, s.active_mask, c)?; let d = s.develop_of(id).unwrap_or_default(); let m = d.masks.iter().find(|m| m.id == mid).ok_or_else(|| bad(c, format!("no mask {mid}")))?; - let k = match p.get("component").and_then(Value::as_u64) { - Some(k) => k as usize, + let k = match opt_index(p, "component", c)? { + Some(k) => k, None => m .components .iter() @@ -703,7 +703,7 @@ pub fn specs() -> Vec { |s, p| { let c = "mask.component"; let mid = mask_id(p, s.active_mask, c)?; - let k = p.get("component").and_then(Value::as_u64).ok_or_else(|| bad(c, "missing component"))? as usize; + let k = opt_index(p, "component", c)?.ok_or_else(|| bad(c, "missing component"))?; let action = str_param(p, "action").ok_or_else(|| bad(c, "missing action"))?.to_string(); let op: Option = match p.get("op") { Some(v) => Some(serde_json::from_value(v.clone()).map_err(|e| bad(c, e.to_string()))?), diff --git a/crates/engine/src/tests_ids_params.rs b/crates/engine/src/tests_ids_params.rs index daff1f7ae..8ce772ded 100644 --- a/crates/engine/src/tests_ids_params.rs +++ b/crates/engine/src/tests_ids_params.rs @@ -163,3 +163,43 @@ fn photo_label_refuses_a_missing_or_non_text_label() { s.execute("photo.label", &json!({"label": "none"})).unwrap(); assert!(labels(&s).iter().all(Option::is_none)); } + +/// The mask-effect commands and a mask's `component` the same way: `mask.saveEffect {"id": "2"}` +/// saved the selected mask's sliders, and `mask.update {"component": "1"}` reshaped component 0. +#[test] +fn mask_effects_and_components_refuse_values_that_are_not_ids() { + let (mut s, _, _) = session(); + s.execute("mask.add", &json!({"kind": "linear"})).unwrap(); + s.execute("mask.add", &json!({"kind": "radial"})).unwrap(); + s.execute("mask.saveEffect", &json!({"name": "Kept"})).unwrap(); + let state = |s: &Session| (format!("{:?}", s.develop_of(s.active().unwrap()).unwrap().masks), s.mask_effects.len()); + let before = state(&s); + let commits = s.commits(); + let d = s.develop_of(s.active().unwrap()).unwrap(); + let shape = serde_json::to_value(&d.masks.last().unwrap().components[0].shape).unwrap(); + for bad in [json!("1"), json!(-1), json!(1.5), json!([1]), json!(u64::from(u32::MAX) + 2)] { + for (cmd, params) in [ + ("mask.applyEffect", json!({"name": "Kept"})), + ("mask.saveEffect", json!({"name": "New"})), + ("mask.updateEffect", json!({"name": "Kept"})), + ] { + let mut params = params; + params["id"] = bad.clone(); + let e = s.execute(cmd, ¶ms).unwrap_err(); + assert!(e.to_string().contains("`id`"), "{cmd} {bad}: {e}"); + } + if bad.is_u64() { + continue; // a well-formed index past the end is "no such component", not a type error + } + for (cmd, params) in + [("mask.update", json!({"shape": shape})), ("mask.component", json!({"action": "invert"})), ("mask.refineDetail", json!({}))] + { + let mut params = params; + params["component"] = bad.clone(); + let e = s.execute(cmd, ¶ms).unwrap_err(); + assert!(e.to_string().contains("`component`"), "{cmd} {bad}: {e}"); + } + } + assert_eq!(s.commits(), commits, "nothing was changed"); + assert_eq!(state(&s), before); +}