Fix #785: commands refuse an id that isn't an object id instead of using the selection

Commands whose `id` or `ids` the shared check in Session::execute does not
read still parsed them with id_param/ids_param, which drop anything that is
not a non-negative integer, and then acted on the selection or the current
layer. `object.blend.make {"ids": 3}` blended the selection,
`object.envelope.makeWithWarp {"ids": [2, "x"]}` warped the selection,
`graph.setData {"id": "2"}` changed the selected graph and
`layer.newSublayer {"parent": "2"}` added a sublayer to the current layer.

- cmd::named_ids and cmd::named_id read `ids`/`id` as targets does: a value
  that is not an object id, or an unresolved step reference, is an error.
- The symbol and brush, Live Paint, Shape Builder and Image Trace, blend,
  envelope and mesh, perspective, Liquify, Puppet Warp and graph helpers use
  them in place of the lenient readers.
- layer.clippingMask.toggle checks `id`, and layer.newSublayer checks
  `parent`, with checked_id before falling back to the highlighted row or the
  current layer.

crates/engine/src/cmd.rs

@@ -512,11 +512,44 @@ pub fn color_value(v: &Value) -> Option<Color> {
     }
 }
 
+/// `v` as an object id: an error unless it is a non-negative integer.
+fn object_id(v: &Value) -> Result<NodeId> {
+    v.as_u64().map(NodeId).ok_or_else(|| EngineError::Other(format!("object ids are non-negative integers, not {v}")))
+}
+
+/// The error for `id` given as step reference `r`, which only a batch resolves.
+fn unresolved_reference(r: &str) -> EngineError {
+    EngineError::Other(format!("`id` is the step reference {r:?}, which only `run` and `command_batch` resolve"))
+}
+
+/// The objects `p` names for a command whose `id` can only be an object id: its `ids`, else its
+/// `id` as `ids` of one. A value that isn't an object id is an error, never the selection (#785).
+/// `None` when `p` gives neither (or gives them as `null`). The ids are not looked up in the
+/// document.
+pub(crate) fn named_ids(p: &Value) -> Result<Option<Vec<NodeId>>> {
+    match p.get("ids").filter(|v| !v.is_null()) {
+        Some(Value::Array(a)) => return a.iter().map(object_id).collect::<Result<Vec<_>>>().map(Some),
+        Some(v) => return Err(EngineError::Other(format!("`ids` must be an array of object ids, not {v}"))),
+        None => {}
+    }
+    Ok(named_id(p)?.map(|id| vec![id]))
+}
+
+/// The object `p`'s `id` names, for a command whose `id` can only be an object id: a value that
+/// isn't an object id is an error, never the selection (#785). `None` when `p` leaves `id` out (or
+/// gives it as `null`). The id is not looked up in the document.
+pub(crate) fn named_id(p: &Value) -> Result<Option<NodeId>> {
+    match p.get("id").filter(|v| !v.is_null()) {
+        None => Ok(None),
+        Some(Value::String(r)) if r.starts_with('$') => Err(unresolved_reference(r)),
+        Some(v) => object_id(v).map(Some),
+    }
+}
+
 /// Objects a command targets: explicit `ids` param or the selection.
 pub(crate) fn targets(s: &Session, p: &Value) -> Result<Vec<NodeId>> {
     // Objects given are used as given: a value that isn't an object id fails rather than the
     // command acting on the selection instead (#785).
-    let object_id = |v: &Value| v.as_u64().map(NodeId).ok_or_else(|| EngineError::Other(format!("object ids are non-negative integers, not {v}")));
     match p.get("ids").filter(|v| !v.is_null()) {
         Some(Value::Array(a)) => return a.iter().map(object_id).collect(),
         Some(v) => return Err(EngineError::Other(format!("`ids` must be an array of object ids, not {v}"))),
@@ -526,9 +559,7 @@ pub(crate) fn targets(s: &Session, p: &Value) -> Result<Vec<NodeId>> {
         Some(v @ Value::Number(_)) => Ok(vec![object_id(v)?]),
         // A step reference a batch couldn't resolve (other strings are other ids, such as an
         // effect's).
-        Some(Value::String(r)) if r.starts_with('$') => {
-            Err(EngineError::Other(format!("`id` is the step reference {r:?}, which only `run` and `command_batch` resolve")))
-        }
+        Some(Value::String(r)) if r.starts_with('$') => Err(unresolved_reference(r)),
         _ => Ok(s.doc()?.selection.objects.clone()),
     }
 }

crates/engine/src/cmd/brushsym.rs

@@ -19,7 +19,7 @@ use super::edit::{outermost, selected_roots};
 use super::pathops::{remove_each, separate};
 use super::swatch::{anchor, names_param};
 use super::{
-    CommandSpec, Result, Session, bad, check_numbers, cmd, color_value, find_command, has_doc, has_selection, id_param, ids_param, number_or, ok,
+    CommandSpec, Result, Session, bad, check_numbers, cmd, color_value, find_command, has_doc, has_selection, id_param, named_ids, number_or, ok,
     str_param, switch_or,
 };
 use crate::EngineError;
@@ -314,7 +314,7 @@ fn ids_json(ids: &[NodeId]) -> Value {
 }
 
 pub(crate) fn roots(s: &Session, p: &Value) -> Result<Vec<NodeId>> {
-    match ids_param(p, "ids").or_else(|| id_param(p, "id").map(|i| vec![i])) {
+    match named_ids(p)? {
         Some(mut v) => {
             // Explicit ids: only existing, non-layer objects, none inside another of them.
             let d = &s.doc()?.doc;

crates/engine/src/cmd/buildcmds.rs

@@ -29,7 +29,7 @@ use super::edit::{outermost, selected_roots};
 use super::paint::paint_from;
 use super::pathops::{Beside, shape_node};
 use super::{
-    CommandSpec, EngineError, Result, Session, always, bad, check_numbers, cmd, color_value, f64_or, finite_numbers, has_doc, id_param, ids_param,
+    CommandSpec, EngineError, Result, Session, always, bad, check_numbers, cmd, color_value, f64_or, finite_numbers, has_doc, id_param, named_ids,
     number_or, point_param, str_param, switch_or,
 };
 
@@ -220,7 +220,7 @@ fn has_multi_or_ids(s: &Session) -> std::result::Result<(), String> {
 }
 
 fn root_ids(s: &Session, p: &Value) -> Result<Vec<NodeId>> {
-    let ids = match ids_param(p, "ids").or_else(|| id_param(p, "id").map(|i| vec![i])) {
+    let ids = match named_ids(p)? {
         Some(v) => v,
         None => selected_roots(s)?,
     };

crates/engine/src/cmd/distortcmds.rs

@@ -16,7 +16,7 @@ use vectorcraft_tools::distort::{PinSet, arap, collect_points, mesh_for, project
 use super::edit::{duplicate_in, outermost, selected_roots};
 use super::{
     CommandSpec, Result, Session, bad, check_numbers, check_switches, cmd, f64_or, f64_req, finite_numbers, has_doc, has_selection, id_param,
-    ids_param, matrix_param, number_or, ok, point_indices, point_param, str_param, switch_or, targets,
+    matrix_param, named_id, named_ids, number_or, ok, point_indices, point_param, str_param, switch_or, targets,
 };
 use crate::EngineError;
 
@@ -658,7 +658,7 @@ fn liquify(s: &mut Session, p: &Value) -> Result<Value> {
     if pts.iter().any(|(q, f)| !q.x.is_finite() || !q.y.is_finite() || !f.is_finite()) {
         return Err(bad(C, "points must be finite"));
     }
-    let roots = match ids_param(p, "ids").or_else(|| id_param(p, "id").map(|i| vec![i])) {
+    let roots = match named_ids(p)? {
         Some(v) => v,
         None => s.doc()?.selection.objects.clone(),
     };
@@ -708,7 +708,7 @@ fn liquify(s: &mut Session, p: &Value) -> Result<Value> {
 
 /// The objects a Puppet Warp command works on: `id`/`ids`, else `default`.
 fn puppet_ids(p: &Value, default: impl FnOnce() -> Result<Vec<NodeId>>) -> Result<Vec<NodeId>> {
-    let ids = match ids_param(p, "ids").or_else(|| id_param(p, "id").map(|i| vec![i])) {
+    let ids = match named_ids(p)? {
         Some(v) => v,
         None => default()?,
     };

crates/engine/src/cmd/distortcmds/perspective.rs

@@ -1,6 +1,6 @@
 use super::{
     Affine, Arc, Document, EngineError, Homography, NodeId, NodeKind, PerspectiveGrid, Plane, Point, Result, Session, Value, bad, check_numbers,
-    check_switches, collect_points, duplicate_in, finite_numbers, id_param, ids_param, json, matrix_param, number_or, ok, outermost, persp,
+    check_switches, collect_points, duplicate_in, finite_numbers, json, matrix_param, named_id, named_ids, number_or, ok, outermost, persp,
     point_param, project_node, selected_roots, str_param, switch_or,
 };
 
@@ -92,7 +92,7 @@ fn plane_param(g: &PerspectiveGrid, p: &Value, cmd: &str) -> Result<Plane> {
 }
 
 pub(super) fn roots(s: &Session, p: &Value) -> Result<Vec<NodeId>> {
-    match ids_param(p, "ids").or_else(|| id_param(p, "id").map(|i| vec![i])) {
+    match named_ids(p)? {
         Some(v) => Ok(outermost(&s.doc()?.doc, v)),
         None => selected_roots(s),
     }
@@ -478,9 +478,8 @@ pub(super) fn plane_move(s: &mut Session, p: &Value) -> Result<Value> {
 
 pub(super) fn plane_match(s: &mut Session, p: &Value) -> Result<Value> {
     let st = s.doc()?;
-    let id = id_param(p, "id")
-        .or_else(|| st.selection.objects.first().copied())
-        .ok_or_else(|| EngineError::Other("select an object in perspective".into()))?;
+    let id =
+        named_id(p)?.or_else(|| st.selection.objects.first().copied()).ok_or_else(|| EngineError::Other("select an object in perspective".into()))?;
     let mut g = grid_of(&st.doc);
     let (plane, depth) = g.attachment_of(&st.doc, id).ok_or_else(|| EngineError::Other("the object isn't in perspective".into()))?;
     g.set_offset(plane, depth);
@@ -586,8 +585,7 @@ pub fn perspective_click(s: &Session, cmd: &str, params: &Value, at: Point) -> O
 /// mode, to be edited; exiting isolation projects it again ([`finish_edit_text`]).
 pub(super) fn edit_text(s: &mut Session, p: &Value) -> Result<Value> {
     let st = s.doc()?;
-    let id =
-        id_param(p, "id").or_else(|| st.selection.objects.first().copied()).ok_or_else(|| EngineError::Other("select type in perspective".into()))?;
+    let id = named_id(p)?.or_else(|| st.selection.objects.first().copied()).ok_or_else(|| EngineError::Other("select type in perspective".into()))?;
     if !st.doc.node(id).is_some_and(|n| matches!(n.kind, NodeKind::Text(_)) && n.projection().is_some()) {
         return Err(EngineError::Other("select type in perspective".into()));
     }

crates/engine/src/cmd/graph.rs

@@ -17,7 +17,7 @@ use vectorcraft_doc::{
 use vectorcraft_geom::{Affine, BezPath, FillRule, PathData, Point, Rect, shapes};
 
 use super::typecmd::refresh_bounds;
-use super::{CommandSpec, EngineError, Result, Session, bad, bool_or, cmd, f64_or, f64_req, has_doc, has_selection, id_param, ok, str_param};
+use super::{CommandSpec, EngineError, Result, Session, bad, bool_or, cmd, f64_or, f64_req, has_doc, has_selection, named_id, ok, str_param};
 
 mod axis;
 mod columns;

crates/engine/src/cmd/graph/edit.rs

@@ -4,7 +4,7 @@ use super::generate::{children_bounds, generate};
 use super::series::{picked_series, reselect_series};
 use super::{
     Affine, Document, EngineError, GraphKind, GraphSpec, Node, NodeId, Rect, Result, Session, TickLength, Value, ValueAxisSide, bad, bool_or, f64_or,
-    f64_req, id_param, json, str_param,
+    f64_req, json, named_id, str_param,
 };
 
 /// Regenerate graph `id` from `spec` (adjusting the plot rect for any move/scale since the last
@@ -57,10 +57,14 @@ pub(super) fn create(s: &mut Session, p: &Value) -> Result<Value> {
     Ok(json!({ "id": id.0 }))
 }
 
-/// The selected graph (or the graph containing the selection).
+/// The graph `id` names (or the graph containing it), else the selected graph (or the graph
+/// containing the selection).
 pub(super) fn target(s: &Session, p: &Value, cmd: &str) -> Result<NodeId> {
     let st = s.doc()?;
-    let start = id_param(p, "id").into_iter().chain(st.selection.objects.iter().copied());
+    let start = match named_id(p)? {
+        Some(id) => vec![id],
+        None => st.selection.objects.clone(),
+    };
     for id in start {
         let mut cur = Some(id);
         while let Some(c) = cur {

crates/engine/src/cmd/layer.rs

@@ -456,7 +456,12 @@ fn clip_toggle(s: &mut Session, p: &Value) -> Result<Value> {
         [id] if matches!(st.doc.node(*id).map(|n| &n.kind), Some(NodeKind::Group { .. })) => Some(*id),
         _ => None,
     };
-    let id = id_param(p, "id").or(row).or(group).or(st.current_layer()).ok_or_else(|| bad("layer.clippingMask.toggle", "no layer"))?;
+    // An `id` given is used as given: one that isn't an object id fails (#785).
+    let given = match p.get("id").filter(|v| !v.is_null()) {
+        Some(v) => Some(checked_id(s, v, "layer.clippingMask.toggle")?),
+        None => None,
+    };
+    let id = given.or(row).or(group).or(st.current_layer()).ok_or_else(|| bad("layer.clippingMask.toggle", "no layer"))?;
     let clip = s.edit("Make/Release Clipping Mask", |d, _| {
         let n = d.node(id).ok_or(EngineError::NoNode(id))?;
         if !matches!(n.kind, NodeKind::Layer { .. } | NodeKind::Group { .. }) {
@@ -519,7 +524,12 @@ fn new_sublayer(s: &mut Session, p: &Value) -> Result<Value> {
     const C: &str = "layer.newSublayer";
     let st = s.doc()?;
     let row = st.highlighted_rows().last().and_then(|r| st.doc.layer_containing(*r));
-    let parent = id_param(p, "parent").or(row).or(st.current_layer()).ok_or_else(|| bad(C, "no parent layer"))?;
+    // A `parent` given is used as given: one that isn't an object id fails (#785).
+    let given = match p.get("parent").filter(|v| !v.is_null()) {
+        Some(v) => Some(checked_id(s, v, C)?),
+        None => None,
+    };
+    let parent = given.or(row).or(st.current_layer()).ok_or_else(|| bad(C, "no parent layer"))?;
     if !st.doc.node(parent).is_some_and(Node::is_layer) {
         return Err(bad(C, "the parent must be a layer or sublayer"));
     }

crates/engine/src/cmd/live.rs

@@ -21,7 +21,7 @@ use super::expand::gradient_sampler;
 use super::pathops::Beside;
 use super::{
     CommandSpec, EngineError, Result, Session, always, bad, check_numbers, check_switches, cmd, color_value, f64_or, f64_req, finite_numbers,
-    has_doc, has_multi, has_selection, id_param, ids_param, number_or, ok, str_param, switch_or,
+    has_doc, has_multi, has_selection, id_param, named_ids, number_or, ok, str_param, switch_or,
 };
 
 mod blend;

crates/engine/src/cmd/live/common.rs

@@ -1,16 +1,16 @@
 use super::mesh::whole;
 use super::{
     Arc, BlendEase, BlendEaseType, BlendOrientation, BlendSpacing, Document, EngineError, Node, NodeId, NodeKind, Result, Selection, Session, Value,
-    bad, check_numbers, has_selection, ids_param, json, selected_roots, str_param, switch_or,
+    bad, check_numbers, has_selection, json, named_ids, selected_roots, str_param, switch_or,
 };
 
 pub(super) fn ids_json(ids: &[NodeId]) -> Value {
     json!({ "ids": ids.iter().map(|i| i.0).collect::<Vec<_>>() })
 }
 
-/// Explicit `ids` (in paint order) or the selected roots.
+/// Explicit `ids`, or `id` as `ids` of one (in paint order), or the selected roots.
 pub(super) fn roots_param(s: &Session, p: &Value) -> Result<Vec<NodeId>> {
-    if let Some(ids) = ids_param(p, "ids") {
+    if let Some(ids) = named_ids(p)? {
         let st = s.doc()?;
         for id in &ids {
             if st.doc.node(*id).is_none() {
@@ -192,7 +192,7 @@ pub(super) fn starts_param(p: &Value, roots: &[NodeId]) -> Result<Vec<(NodeId, u
     const C: &str = "object.blend.make";
     let Some(v) = p.get("starts").filter(|v| !v.is_null()) else { return Ok(vec![]) };
     let list = v.as_array().ok_or_else(|| bad(C, "`starts` must be an array of anchor indices (or nulls)"))?;
-    let owners = ids_param(p, "ids").unwrap_or_else(|| roots.to_vec());
+    let owners = named_ids(p)?.unwrap_or_else(|| roots.to_vec());
     let mut out = vec![];
     for (id, a) in owners.iter().zip(list) {
         if a.is_null() {

crates/engine/src/tests.rs

@@ -3607,6 +3607,50 @@ fn given_targets_are_used_as_given_and_range_style_takes_the_selected_type() {
     assert!(s.execute("text.setRangeStyle", &json!({"id": "two", "size": 40})).is_err());
 }
 
+/// #785: commands whose `ids` or `id` the shared check doesn't read (it reads `id` where it stands
+/// for `ids` of one) refuse one that isn't an object id. They don't act on the selection or the
+/// current layer instead, and the document and its undo history stay as they are.
+#[test]
+fn malformed_ids_are_refused_by_commands_that_default_to_the_selection() {
+    let mut s = session();
+    let a = rect(&mut s, 100.0, 100.0, 80.0, 60.0);
+    let b = rect(&mut s, 140.0, 120.0, 80.0, 60.0);
+    let graph = s.execute("graph.create", &json!({"type": "column", "x": 400, "y": 300, "width": 120, "height": 80, "rows": [[1, 2]]})).unwrap();
+    let ids = [json!([a.0, "x"]), json!(3)];
+    let id = [json!("2"), json!("$1.id"), json!(2.5)];
+    let commands = [
+        ("object.blend.make", json!({}), "ids", &ids[..], json!([a.0, b.0])),
+        ("object.envelope.makeWithWarp", json!({}), "ids", &ids[..], json!([a.0, b.0])),
+        ("graph.setData", json!({"rows": [[3, 4]]}), "id", &id[..], json!([graph["id"]])),
+        ("imageTrace.make", json!({}), "id", &id[..], json!([a.0])),
+        ("perspective.plane.matchObject", json!({}), "id", &id[..], json!([a.0])),
+        ("perspective.editText", json!({}), "id", &id[..], json!([a.0])),
+        ("layer.clippingMask.toggle", json!({}), "id", &id[..], json!([a.0, b.0])),
+        ("layer.newSublayer", json!({}), "parent", &id[..], json!([a.0])),
+    ];
+    let mut wrong = vec![];
+    for (cmd, params, key, values, selected) in commands {
+        s.execute("select.set", &json!({"ids": selected})).unwrap();
+        for bad in values {
+            let mut p = params.clone();
+            if let Some(p) = p.as_object_mut() {
+                p.insert(key.into(), bad.clone());
+            }
+            let (doc, undo) = (s.doc().unwrap().doc.clone(), s.doc().unwrap().history.undo.len());
+            let r = s.execute(cmd, &p);
+            let refused = r.as_ref().err().map(|e| e.to_string()).is_some_and(|e| e.contains("object id") || e.contains("step reference"));
+            let same = s.doc().unwrap().doc == doc && s.doc().unwrap().history.undo.len() == undo;
+            if !(refused && same) {
+                wrong.push(format!("{cmd} {p}: {r:?}, document unchanged: {same}"));
+            }
+            if !same {
+                s.execute("edit.undo", &json!({})).ok();
+            }
+        }
+    }
+    assert!(wrong.is_empty(), "{}", wrong.join("\n"));
+}
+
 #[test]
 fn malformed_anchor_selection_never_replaces_or_partially_changes_selection() {
     let mut s = session();