From c40113b75586caa0da7c1fd640b7bb832c9506ed Mon Sep 17 00:00:00 2001 From: Taha-Mahmoodi <85902429+Taha-Mahmoodi@users.noreply.github.com> Date: Sat, 24 Oct 2026 12:31:31 +0430 Subject: [PATCH] Fix #3965: a malformed or unknown id is refused under the command's name, never replaced by the selection --- crates/engine/src/cmd/gradient.rs | 4 +- crates/engine/src/cmd/mod.rs | 20 +++++--- crates/engine/src/cmd/paint.rs | 2 +- crates/engine/src/cmd/params.rs | 26 ++++++++++ crates/engine/src/lib.rs | 19 ++++++-- .../engine/src/tests_ids_without_selection.rs | 48 ++++++++++++++++++- docs/mcp.md | 5 +- docs/target-app-parity.md | 1 + 8 files changed, 109 insertions(+), 16 deletions(-) diff --git a/crates/engine/src/cmd/gradient.rs b/crates/engine/src/cmd/gradient.rs index 16451ddf0..a861eeb34 100644 --- a/crates/engine/src/cmd/gradient.rs +++ b/crates/engine/src/cmd/gradient.rs @@ -586,7 +586,9 @@ fn set_gradient_geom(s: &mut Session, p: &Value) -> Result { let stroke = edits_stroke(s, p, item, !s.fill_active, C)?; let item = item.of_kind(s, !stroke); let targets = match item { - ItemTarget::Top => leaf_targets(s, &super::given_objects(p).map_or_else(|| selected_roots(s), |ids| Ok(outermost(&s.doc()?.doc, ids)))?)?, + ItemTarget::Top => { + leaf_targets(s, &super::given_objects(s, p, C)?.map_or_else(|| selected_roots(s), |ids| Ok(outermost(&s.doc()?.doc, ids)))?)? + } _ => item.targets(s, p)?, }; if targets.is_empty() { diff --git a/crates/engine/src/cmd/mod.rs b/crates/engine/src/cmd/mod.rs index 3d5a45bd3..c09279e2a 100644 --- a/crates/engine/src/cmd/mod.rs +++ b/crates/engine/src/cmd/mod.rs @@ -367,12 +367,20 @@ pub(crate) fn id_param(p: &Value, key: &str) -> Option { pub(crate) fn ids_param(p: &Value, key: &str) -> Option> { p.get(key).and_then(Value::as_array).map(|a| a.iter().filter_map(Value::as_u64).map(NodeId).collect()) } -/// The objects `p` gives: its `ids`, else its `id` as `ids` of one (as [`targets`] reads them); -/// `None` when it gives neither. -pub(crate) fn given_objects(p: &Value) -> Option> { - match p.get("ids").filter(|v| !v.is_null()) { - Some(_) => ids_param(p, "ids"), - None => id_param(p, "id").map(|id| vec![id]), +/// The objects `p` gives `command`: its `ids`, else its `id` as `ids` of one (as [`targets`] reads +/// them), each checked ([`checked_id`]): a malformed one, or one naming no object, is an error, +/// never the selection. `None` when it gives neither (or gives them as `null`). +pub(crate) fn given_objects(s: &Session, p: &Value, command: &str) -> Result>> { + if p.get("ids").is_some_and(|v| !v.is_null()) { + return checked_ids_param(s, p, "ids", command).map(Some); + } + match p.get("id").filter(|v| !v.is_null()) { + None => Ok(None), + // A step reference a batch couldn't resolve, as `targets` reports it. + 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(v) => Ok(Some(vec![checked_id(s, v, command)?])), } } /// `v` as an index, a count or a byte offset: past `usize::MAX` reads as `usize::MAX`, past the end of any list. diff --git a/crates/engine/src/cmd/paint.rs b/crates/engine/src/cmd/paint.rs index 47b1ee91c..e7c963d02 100644 --- a/crates/engine/src/cmd/paint.rs +++ b/crates/engine/src/cmd/paint.rs @@ -684,7 +684,7 @@ fn default_paint(s: &mut Session, p: &Value) -> Result { /// active item of the proxy's kind is recoloured in each selected object's own stack instead. fn proxy_recolor(s: &mut Session, p: &Value, cmd: &str, label: &str, f: &dyn Fn(Color) -> Color) -> Result { let stroke = stroke_param(s, p, cmd)?; - let ids = match super::given_objects(p) { + let ids = match super::given_objects(s, p, cmd)? { Some(ids) => ids, None => super::edit::selected_roots(s)?, }; diff --git a/crates/engine/src/cmd/params.rs b/crates/engine/src/cmd/params.rs index dae1cb1a1..1c8ba5ffc 100644 --- a/crates/engine/src/cmd/params.rs +++ b/crates/engine/src/cmd/params.rs @@ -161,6 +161,32 @@ pub(crate) fn takes_key(doc: &'static str, key: &str) -> bool { doc_keys(doc).is_ok_and(|parsed| parsed.keys.contains(&key)) || accepted(doc, 0).is_some_and(|keys| keys.contains(&key)) } +/// Does the description `doc` take `id` as `ids` of one object (`{ids?|id?}`, `{id?, ids?: … +/// (instead of id)}`, `{id} or {ids: […]}`): it names both keys, and no params object of it names +/// `ids` beside a required `id`, which is something else (`plugin.run {id, ids?}`: the plug-in's)? +pub(crate) fn id_stands_for_ids(doc: &'static str) -> bool { + fn head(alternative: &str) -> &str { + alternative.split([':', '=', '(']).next().unwrap_or_default().trim() + } + if !takes_key(doc, "ids") || !takes_key(doc, "id") { + return false; + } + let mut rest = doc; + while let Ok((body, after)) = object_body(rest) { + let entries = split_top(body, ','); + let required_id = entries.iter().any(|e| split_top(e, '|').len() == 1 && head(e) == "id"); + let names_ids = entries.iter().any(|e| split_top(e, '|').iter().any(|a| head(a).trim_end_matches('?').trim_end() == "ids")); + if required_id && names_ids { + return false; + } + match after.trim_start().strip_prefix("or ").or_else(|| after.trim_start().strip_prefix("| ")).filter(|r| r.starts_with('{')) { + Some(next) => rest = next, + None => break, + } + } + true +} + /// The edits (insert, delete, replace, swap of neighbours) that turn `a` into `b`, ignoring case. fn distance(a: &str, b: &str) -> usize { let (a, b): (Vec, Vec) = (a.chars().map(|c| c.to_ascii_lowercase()).collect(), b.chars().map(|c| c.to_ascii_lowercase()).collect()); diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index 57c51ef64..eb8a6fbc8 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -1760,6 +1760,12 @@ impl Session { // passes that its command's description doesn't name fails the test. #[cfg(test)] self.check_params(id, params)?; + // Objects given as `id` (where it stands for `ids` of one) or as `ids` beside it are checked + // before anything else: a malformed one, or one naming no object, is an error under this + // command's id, never the selection in its place. + if cmd::params::id_stands_for_ids(spec.params) && self.active().is_some() { + cmd::given_objects(self, params, id)?; + } // Clear's menu needs a selection; an agent supplying explicit targets only needs a // document. The command validates those targets before making any edit. let given = given_ids(spec.params, params); @@ -2791,12 +2797,15 @@ pub(crate) const SELECTS_GIVEN_IDS: &[&str] = &[ /// The non-empty `ids` given to a command whose params description names `ids` as a key (the /// key callers outside the app may pass it, [`cmd::params::takes_key`]), or its `id` as `ids` of -/// one when the description names that key too and no `ids` are given, as [`cmd::targets`] reads -/// them. +/// one when no `ids` are given and the description takes `id` so +/// ([`cmd::params::id_stands_for_ids`]), as [`cmd::targets`] reads them. fn given_ids(description: &'static str, params: &Value) -> Option> { - let by_id = params.get("ids").is_none_or(Value::is_null); - if !cmd::params::takes_key(description, "ids") || (by_id && !cmd::params::takes_key(description, "id")) { + if !cmd::params::takes_key(description, "ids") { return None; } - cmd::given_objects(params).filter(|ids| !ids.is_empty()) + match params.get("ids").filter(|v| !v.is_null()) { + Some(_) => cmd::ids_param(params, "ids"), + None => cmd::id_param(params, "id").filter(|_| cmd::params::id_stands_for_ids(description)).map(|id| vec![id]), + } + .filter(|ids| !ids.is_empty()) } diff --git a/crates/engine/src/tests_ids_without_selection.rs b/crates/engine/src/tests_ids_without_selection.rs index 127ca2034..1d1f4b325 100644 --- a/crates/engine/src/tests_ids_without_selection.rs +++ b/crates/engine/src/tests_ids_without_selection.rs @@ -62,6 +62,12 @@ fn only_an_ids_key_counts() { assert_eq!(given_ids("{ids?|id?}", &json!({"id": "a1"})), None, "not an object id"); assert_eq!(given_ids("{ids?|id?}", &json!({"id": 3, "ids": [4]})), Some(vec![NodeId(4)]), "`ids` first"); assert_eq!(given_ids("{ids?|id?}", &json!({"id": 3, "ids": null})), Some(vec![NodeId(3)])); + // A required `id` beside `ids` is something else (`plugin.run`'s plug-in); `{id} or {ids}` is + // one object or several. + assert_eq!(given_ids("{id, params?: {…}, ids?: [..]}", &one), None, "a plug-in's id"); + assert!(!cmd::params::id_stands_for_ids("{id, params?: {…}, ids?: [..]}")); + assert!(cmd::params::id_stands_for_ids("{id} or {ids: [id…]}: toggles them")); + assert!(cmd::params::id_stands_for_ids("{ids|id, target}")); } /// `{"id": x}` with params besides it, and the same as `{"ids": [x]}`. @@ -102,7 +108,7 @@ fn id_does_what_ids_of_one_does_with_nothing_selected() { let (fresh, [a, inner, _, d], t) = doc_with_type(); let (mut checked, mut failures, mut refused, mut refused_ran) = (vec![], vec![], vec![], vec![]); for spec in command_specs() { - if !cmd::params::takes_key(spec.params, "ids") || !cmd::params::takes_key(spec.params, "id") { + if !cmd::params::id_stands_for_ids(spec.params) { continue; } // Those the menus refuse with nothing selected are the ones that refused `id`. @@ -127,6 +133,46 @@ fn id_does_what_ids_of_one_does_with_nothing_selected() { assert!(refused.len() >= 26 && refused_ran.len() >= 14, "{refused_ran:?} ran of {refused:?}; checked {}", checked.len()); } +/// Every command that takes `id` as `ids` of one ([`cmd::params::id_stands_for_ids`]), with an +/// object selected: an `id` that is malformed (a string, negative, a fraction, an object) or names +/// no object is refused under the command's id, as the same value in `ids` is, and the document +/// and the selection stay as they were. It never stands for the selection instead. +#[test] +fn a_bad_id_is_refused_as_the_same_ids_are_and_changes_nothing() { + let (_, [.., d], _) = doc_with_type(); + let (mut failures, mut checked) = (vec![], 0); + for spec in command_specs() { + if !cmd::params::id_stands_for_ids(spec.params) { + continue; + } + checked += 1; + for bad in [json!("bad"), json!(-1), json!(1.5), json!({"id": 1}), json!(999_999)] { + let run = |key: &str, value: serde_json::Value| { + let (mut s, ..) = doc_with_type(); + s.execute("select.set", &json!({ "ids": [d] })).unwrap(); + let before = s.doc().unwrap().doc.clone(); + let mut p = params_for(spec.id, d); + p[key] = value; + let r = s.execute(spec.id, &p).map_err(|e| e.to_string()); + let st = s.doc().unwrap(); + (r, st.doc == before && st.selection.objects == vec![NodeId(d)]) + }; + let (with_id, with_ids) = (run("id", bad.clone()), run("ids", json!([bad.clone()]))); + if with_id.0.is_ok() || !with_id.1 || with_id != with_ids { + failures.push(format!( + "{} {bad}: with id {:.160} (unchanged: {}); with ids {:.160}", + spec.id, + format!("{:?}", with_id.0), + with_id.1, + format!("{:?}", with_ids.0) + )); + } + } + } + assert!(failures.is_empty(), "{} fail:\n{}", failures.len(), failures.join("\n")); + assert!(checked >= 100, "{checked} commands"); +} + /// Every command that names `ids` and is refused with nothing selected: given the object as `ids` /// it does what it does with the object selected (the same result or the same error, and the same /// document), so no command is let through to run on an empty selection. diff --git a/docs/mcp.md b/docs/mcp.md index e2c5a4435..2e2f5138b 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -349,8 +349,9 @@ Coordinates are points in document space: y points down, the origin is the first document is 612 × 792 (US Letter). New objects become the selection. Most commands act on the selection or on explicit `ids`, which stand in for the selection, so they work with nothing selected. Where a command's params say `ids?|id?` (or `id?, ids?: … (instead of id)`), `id` is `ids` of one object: `{"id": 7}` does what `{"ids": [7]}` -does, with or without a selection. A command that leaves out an object selected inside a selected group also leaves -it out when `ids` list it together with a group that holds it. +does, with or without a selection, and an `id` or `ids` that is malformed (not a non-negative integer) or names no +object is refused under the command's name, never replaced by the selection. A command that leaves out an object +selected inside a selected group also leaves it out when `ids` list it together with a group that holds it. Paint values (`fill`, `stroke`) accept `"#rrggbb"`, `"none"`, `[r,g,b]` (0..1), `{"c","m","y","k"}`, `{"gray"}`, or a full `paint.setFill` params object (`{"gradient": …}`, `{"swatch": "name"}`). diff --git a/docs/target-app-parity.md b/docs/target-app-parity.md index ede5e43cc..70d41e315 100644 --- a/docs/target-app-parity.md +++ b/docs/target-app-parity.md @@ -1092,6 +1092,7 @@ applies to fills). Tests: `color::gradient::tests` (perceptual midpoint, expansi | Date | Change | Summary | |---|---|---| +| 2026-10-24 | minor | Commands: a command whose params take `ids?|id?` (or `id?, ids?: … (instead of id)`) takes `id` as `ids` of one with nothing selected too, instead of refusing it as "nothing selected" (29 such commands needed a selection); Invert, Complement, Gradient Vector and Make Pixel Perfect act on the object given as `id`; an `id` or `ids` that is malformed or names no object is refused under the command's name instead of the selection standing in for it (`plugin.run`'s required `id`, the plug-in's, is not an object) (`tests_ids_without_selection::id_does_what_ids_of_one_does_with_nothing_selected`, `a_bad_id_is_refused_as_the_same_ids_are_and_changes_nothing`); scores unchanged | | 2026-10-24 | minor | Commands: a command whose params take `ids?|id?` (or `id?, ids?: … (instead of id)`) takes `id` as `ids` of one with nothing selected too, instead of refusing it as "nothing selected" (29 such commands needed a selection); Invert, Complement, Gradient Vector and Make Pixel Perfect act on the object given as `id` (`tests_ids_without_selection::id_does_what_ids_of_one_does_with_nothing_selected`); scores unchanged | | 2026-10-24 | minor | Type advanced: ruby (ふりがな) is kept, where it is laid out, by Type › Create Outlines and in EPS, EMF/WMF and legacy `.ai` exports (it was dropped there), see [type-parity.md](type-parity.md); scores and hours unchanged | | 2026-10-24 | minor | Path operations: Shape Builder pieces: the planar map contracts the sweep's stubs between two cuts of a line's passes a rounding error apart (bounded by each cluster's extent), so dense hatches at exact binary steps no longer add a speck per crossing; Path operations stays 80% and Shape Builder 70% while #937 is open (estimated) |