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

@@ -287,12 +287,8 @@ fn effect_index(s: &Session, name: &str, c: &str) -> Result<usize> {
 /// The selected mask's adjustments on the active photo.
 fn selected_mask(s: &Session, p: &Value, c: &str) -> Result<Mask> {
     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}")))
 }

crates/engine/src/cmd/masks.rs

@@ -202,7 +202,7 @@ fn connected_strokes(from: Point, last: &BrushStroke, to: BrushStroke) -> Vec<Br
 /// The mask `id` names: `None` when it is missing or null. An `id` that isn't a mask id (text, a
 /// negative or fractional number, or one past `u32`, which `as u32` would have wrapped onto another
 /// mask) is an error, never the selected mask.
-fn opt_mask_id(p: &Value, c: &str) -> Result<Option<u32>> {
+pub(crate) fn opt_mask_id(p: &Value, c: &str) -> Result<Option<u32>> {
     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<CommandSpec> {
         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<CommandSpec> {
                 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<CommandSpec> {
             |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<MaskOp> = match p.get("op") {
                     Some(v) => Some(serde_json::from_value(v.clone()).map_err(|e| bad(c, e.to_string()))?),

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, &params).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, &params).unwrap_err();
+            assert!(e.to_string().contains("`component`"), "{cmd} {bad}: {e}");
+        }
+    }
+    assert_eq!(s.commits(), commits, "nothing was changed");
+    assert_eq!(state(&s), before);
+}