From 175ad4a1e34fdd3cae1c6b772e8d218dee8f3850 Mon Sep 17 00:00:00 2001 From: bflatastic Date: Tue, 13 Oct 2026 10:34:48 +0700 Subject: [PATCH] Effects: effect.apply and effect.setParams refuse parameters the effect doesn't take (#1592) --- crates/effects/src/lib.rs | 2 +- crates/engine/src/cmd/effectcmd.rs | 16 ++++- crates/engine/src/cmd/params.rs | 80 +++++++++++++++++++++--- crates/engine/src/lib.rs | 2 +- crates/engine/src/tests_params.rs | 46 +++++++++++++- crates/engine/src/tests_rasterfilters.rs | 15 ++++- crates/engine/tests/param_keys.rs | 22 +++++++ crates/mcp/src/headless.rs | 2 +- crates/ui-egui/src/control.rs | 2 +- crates/ui-egui/src/lib.rs | 2 +- crates/ui-egui/src/menus.rs | 4 +- docs/mcp.md | 16 ++++- 12 files changed, 187 insertions(+), 22 deletions(-) diff --git a/crates/effects/src/lib.rs b/crates/effects/src/lib.rs index 9b520e27..0c859583 100644 --- a/crates/effects/src/lib.rs +++ b/crates/effects/src/lib.rs @@ -249,7 +249,7 @@ pub fn effect_catalog() -> Vec { "distort.freeDistort", "Free Distort…", DT, - "{corners: [[x,y]×4] new positions of the bounding-box corners TL, TR, BR, BL in unit box coordinates (identity [[0,0],[1,0],[1,1],[0,1]])}", + "{corners: [[x,y]×4] (new positions of the bounding-box corners TL, TR, BR, BL in unit box coordinates; identity [[0,0],[1,0],[1,1],[0,1]])}", json!({"corners": [[0.0, 0.0], [1.0, 0.0], [1.0, 1.0], [0.0, 1.0]]}), ), g("distort.puckerBloat", "Pucker & Bloat…", DT, "{amount: % (-200 pucker … 200 bloat, default 0)}", json!({"amount": 0.0})), diff --git a/crates/engine/src/cmd/effectcmd.rs b/crates/engine/src/cmd/effectcmd.rs index 842e6991..ada48cf8 100644 --- a/crates/engine/src/cmd/effectcmd.rs +++ b/crates/engine/src/cmd/effectcmd.rs @@ -280,6 +280,20 @@ fn move_effect(s: &mut Session, p: &Value) -> Result { Ok(json!({ "ids": ids_json(&ids), "index": index, "item": item.flatten() })) } +/// The ids of the effects `effect.setParams` with `p` edits, for the check of their parameters +/// (none when `p` doesn't say which: running it reports why). +pub(crate) fn set_params_effects(s: &Session, p: &Value) -> Vec { + const C: &str = "effect.setParams"; + let (Ok(index), Ok(item), Ok(ids), Ok(st)) = (index_param(p, "index", C), item_target(s, p, C), appearance_targets(s, p), s.doc()) else { + return vec![]; + }; + let effect = |id: &NodeId| { + let ap = &st.doc.node(*id)?.appearance; + Some(ap.effects_at(item.effects_item(ap, C).ok()?)?.get(index)?.id.clone()) + }; + ids.iter().filter_map(effect).collect() +} + fn set_params(s: &mut Session, p: &Value) -> Result { const C: &str = "effect.setParams"; let index = index_param(p, "index", C)?; @@ -566,7 +580,7 @@ mod tests { for other in ["threeD.revolve", "threeD.extrude", "threeD.rotate"] { assert!(s.execute("effect.apply", &json!({"effect": other, "ids": [id.0], "item": null})).is_err(), "{effect} + {other}"); } - s.execute("effect.setParams", &json!({"index": 0, "params": {"rotationY": 50, "depth": 120}, "ids": [id.0], "item": null})).unwrap(); + s.execute("effect.setParams", &json!({"index": 0, "params": {"rotationY": 50, "rotationX": 10}, "ids": [id.0], "item": null})).unwrap(); assert_ne!(effects::solid_art(&node(&s, id)).unwrap(), art, "{effect}: options change the art"); assert!(s.execute("effect.setParams", &json!({"index": 0, "params": {"bevel": "nope"}, "ids": [id.0], "item": null})).is_err()); let svg = s.execute("document.serialize", &json!({"format": "svg"})).unwrap(); diff --git a/crates/engine/src/cmd/params.rs b/crates/engine/src/cmd/params.rs index 13142f25..7989071f 100644 --- a/crates/engine/src/cmd/params.rs +++ b/crates/engine/src/cmd/params.rs @@ -10,8 +10,10 @@ //! //! Callers outside the app (the control channel, MCP, the CLI) are answered with a parameter error //! for a key the description doesn't name ([`check_params`]): a misspelt `widht` no longer runs the -//! command with its default. [`Session::execute`] itself takes what it is given, so the journal of -//! an older version still replays. +//! command with its default. So are the `params` of an effect (#1592): `effect.apply` and +//! `effect.setParams` answer a key the effect doesn't take, or a value of another type than its +//! default, with a parameter error ([`check_effect_params`]). [`Session::execute`] itself takes +//! what it is given, so the journal of an older version still replays. use serde_json::Value; @@ -189,15 +191,56 @@ pub fn check_keys(id: &str, doc: &'static str, params: &Value) -> Result<()> { return Ok(()); } let Some(keys) = accepted(doc, 0) else { return Ok(()) }; - let Some(key) = given.keys().find(|k| !keys.contains(&k.as_str())) else { return Ok(()) }; - Err(bad( + match given.keys().find(|k| !keys.contains(&k.as_str())) { + Some(key) => Err(unknown(id, key, &keys)), + None => Ok(()), + } +} + +/// The parameter error of command `id` for `key`, which isn't one of `keys`: with the nearest of +/// them, or all of them. +fn unknown(id: &str, key: &str, keys: &[&str]) -> crate::EngineError { + bad( id, - match nearest(key, &keys) { + match nearest(key, keys) { Some(near) => format!("unknown parameter `{key}`; did you mean `{near}`?"), None if keys.is_empty() => format!("unknown parameter `{key}`; the command takes no parameters"), None => format!("unknown parameter `{key}`; the command takes: {}", keys.join(", ")), }, - )) + ) +} + +/// `params` of built-in effect `effect`, as command `id` was given them (#1592): only keys the +/// effect takes (those of its dialog defaults and of its params description), each of its +/// default's type. Numbers out of range pass: the effect clamps them when it draws. An effect +/// plug-in resolves its own against its manifest, and an unknown effect is the command's to report. +pub fn check_effect_params(id: &str, effect: &str, params: &Value) -> Result<()> { + let Value::Object(given) = params else { return Ok(()) }; + if effect.starts_with(vectorcraft_plugins::effect::PREFIX) { + return Ok(()); + } + let Some(info) = vectorcraft_render::effects::effect_info(effect) else { return Ok(()) }; + let defaults = info.defaults.as_object(); + let mut keys: Vec<&str> = defaults.into_iter().flat_map(|d| d.keys().map(String::as_str)).collect(); + // A description that doesn't parse names no more keys (the tests rule that out). + for key in doc_keys(info.params).map(|d| d.keys).unwrap_or_default() { + if !keys.contains(&key) { + keys.push(key); + } + } + for (key, v) in given { + if !keys.contains(&key.as_str()) { + return Err(unknown(id, key, &keys)); + } + match defaults.and_then(|d| d.get(key)) { + Some(Value::Number(_)) if !v.is_number() => return Err(bad(id, format!("`{key}` must be a number"))), + Some(Value::Bool(_)) if !v.is_boolean() => return Err(bad(id, format!("`{key}` must be true or false"))), + // (A colour is also taken as `[r, g, b]` or as a colour object.) + Some(Value::String(_)) if v.is_number() || v.is_boolean() => return Err(bad(id, format!("`{key}` must be a string"))), + _ => {} + } + } + Ok(()) } /// Commands whose params aren't checked yet (#1442): they read keys in ways the audit of the @@ -333,10 +376,16 @@ pub const UNCHECKED: &[&str] = &[ "webExport.settings", ]; -/// [`check_keys`] for an engine command, the steps of a `command.batch` included. A command the -/// engine doesn't have passes: running it reports that. +/// [`check_keys`] for an engine command, the steps of a `command.batch` included, and +/// [`check_effect_params`] for the effect `effect.apply` adds. A command the engine doesn't have +/// passes: running it reports that. pub fn check_params(id: &str, params: &Value) -> Result<()> { let Some(spec) = find_command(id) else { return Ok(()) }; + if let ("effect.apply" | "appearance.addEffect", Some(effect), Some(given)) = + (id, params.get("effect").or_else(|| params.get("id")).and_then(Value::as_str), params.get("params")) + { + check_effect_params(id, effect, given)?; + } if UNCHECKED.contains(&id) { return Ok(()); } @@ -350,3 +399,18 @@ pub fn check_params(id: &str, params: &Value) -> Result<()> { } Ok(()) } + +impl crate::Session { + /// [`check_params`], and [`check_effect_params`] for the effects `effect.setParams` edits in + /// this session's document (not as a step of a `command.batch`: what it edits is only there + /// once the steps before it ran). + pub fn check_params(&self, id: &str, params: &Value) -> Result<()> { + check_params(id, params)?; + if let ("effect.setParams", Some(given)) = (id, params.get("params")) { + for effect in super::effectcmd::set_params_effects(self, params) { + check_effect_params(id, &effect, given)?; + } + } + Ok(()) + } +} diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index f63c7be4..fc3f5bb4 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -1317,7 +1317,7 @@ impl Session { // The crate's own tests are the audit of the params descriptions (#1442): a key a test // passes that its command's description doesn't name fails the test. #[cfg(test)] - cmd::params::check_params(id, params)?; + self.check_params(id, params)?; // 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 enabled = if id == "edit.clear" && params.get("ids").is_some() { diff --git a/crates/engine/src/tests_params.rs b/crates/engine/src/tests_params.rs index 9c4d1393..dd3664db 100644 --- a/crates/engine/src/tests_params.rs +++ b/crates/engine/src/tests_params.rs @@ -2,7 +2,7 @@ use serde_json::json; -use crate::cmd::params::{UNCHECKED, check_keys, check_params, doc_keys}; +use crate::cmd::params::{UNCHECKED, check_effect_params, check_keys, check_params, doc_keys}; use crate::{EngineError, Session, command_specs, find_command}; fn keys(doc: &str) -> Vec<&str> { @@ -95,6 +95,50 @@ fn a_batch_checks_its_steps() { assert_eq!(refusal("command.batch", json!({"steps": []})), "unknown parameter `steps`; the command takes: label, commands, artboard"); } +/// #1592: the `params` of an effect are held to the effect's own keys and types, where the effect +/// is named (`effect.apply`) and where the document says which it is (`effect.setParams`). +#[test] +fn effect_params_are_the_effects_own() { + use vectorcraft_render::effects::effect_catalog; + for e in effect_catalog() { + doc_keys(e.params).unwrap_or_else(|why| panic!("{}: {why}", e.id)); + check_effect_params("effect.apply", e.id, &e.defaults).unwrap_or_else(|why| panic!("{}'s defaults: {why}", e.id)); + } + let apply = |effect: &str, params| json!({"effect": effect, "params": params}); + assert_eq!(refusal("effect.apply", apply("blur.gaussian", json!({"blurr": 3}))), "unknown parameter `blurr`; the command takes: radius"); + assert_eq!( + refusal("appearance.addEffect", apply("stylize.dropShadow", json!({"opacty": 50}))), + "unknown parameter `opacty`; did you mean `opacity`?" + ); + assert_eq!(refusal("effect.apply", apply("blur.gaussian", json!({"radius": "big"}))), "`radius` must be a number"); + assert_eq!(refusal("effect.apply", apply("threeD.revolve", json!({"shade": 1}))), "`shade` must be true or false"); + assert_eq!(refusal("effect.apply", apply("threeD.revolve", json!({"edge": 1}))), "`edge` must be a string"); + // Out of range is the effect's to clamp; a key only its description names is one it takes; + // a plug-in's are its manifest's; an unknown effect is the command's to report. + for (effect, params) in [ + ("stylize.dropShadow", json!({"opacity": 500, "x": -3, "color": "#ff0000"})), + ("stylize.dropShadow", json!({"color": [1, 0, 0]})), + ("threeD.revolve", json!({"segments": 100000})), + ("path.outlineStroke", json!({"width": 2})), + ("plugin.anything", json!({"whatever": 1})), + ("no.such", json!({"whatever": 1})), + ] { + assert!(check_params("effect.apply", &apply(effect, params.clone())).is_ok(), "{effect} {params}"); + } + // `effect.setParams` names an index: the session knows the effect there. + let mut s = Session::new(); + s.execute("file.new", &json!({})).unwrap(); + s.execute("shape.rectangle", &json!({"x": 10, "y": 10, "width": 50, "height": 40})).unwrap(); + s.execute("effect.apply", &apply("blur.gaussian", json!({"radius": 2}))).unwrap(); + for (params, why) in [(json!({"blurr": 3}), "unknown parameter `blurr`"), (json!({"radius": true}), "`radius` must be a number")] { + let e = s.execute("effect.setParams", &json!({"index": 0, "params": params})).unwrap_err().to_string(); + assert!(e.contains(why), "{e}"); + } + s.execute("effect.setParams", &json!({"index": 0, "params": {"radius": 1e9}, "visible": false})).unwrap(); + // No effect at the index: the command says so, not the check. + assert!(s.check_params("effect.setParams", &json!({"index": 7, "params": {"blurr": 3}})).is_ok()); +} + /// What a command notes for its journal entry is a key its description names, so the entry /// passes the check a caller's params get. #[test] diff --git a/crates/engine/src/tests_rasterfilters.rs b/crates/engine/src/tests_rasterfilters.rs index cde6adc8..28a93435 100644 --- a/crates/engine/src/tests_rasterfilters.rs +++ b/crates/engine/src/tests_rasterfilters.rs @@ -106,10 +106,19 @@ fn listed_applied_edited_and_undone() { assert_eq!((&e["raster"], &e["menu"]), (&json!(true), &json!(["Effect", menu])), "{id}"); } } - // Junk parameters are stored as given and read clamped; the canvas still draws. + // Numbers far out of range are stored as given and read clamped; the canvas still draws. for (id, _) in EFFECTS { - s.execute("effect.apply", &json!({"effect": id, "ids": [g], "params": {"amount": 1e308, "radius": -1e308, "threshold": "x", "quality": 7}})) - .unwrap(); + let defaults = vectorcraft_render::effects::default_params(id).unwrap(); + let junk: serde_json::Map = defaults + .as_object() + .unwrap() + .iter() + .filter(|(_, v)| v.is_number()) + .enumerate() + .map(|(i, (k, _))| (k.clone(), json!(if i % 2 == 0 { 1e308 } else { -1e308 }))) + .collect(); + assert!(!junk.is_empty(), "{id}"); + s.execute("effect.apply", &json!({"effect": id, "ids": [g], "params": junk})).unwrap(); } let _ = render(&s); let n = s.doc().unwrap().doc.node(NodeId(g)).unwrap().clone(); diff --git a/crates/engine/tests/param_keys.rs b/crates/engine/tests/param_keys.rs index ee4d11ce..cc0551e8 100644 --- a/crates/engine/tests/param_keys.rs +++ b/crates/engine/tests/param_keys.rs @@ -22,6 +22,28 @@ fn a_journal_with_a_key_no_longer_read_still_replays() { assert_eq!((st.doc.artboards[0].rect.width(), st.doc.layers[0].children().unwrap().len()), (300.0, 1)); } +/// So do an effect's own parameters (#1592): a journal or a file with a key the effect doesn't +/// take, or a value of another type, replays and opens; the effect reads what it knows, clamped. +#[test] +fn effect_params_no_longer_taken_still_replay_and_open() { + let mut s = Session::new(); + s.execute("file.new", &json!({})).unwrap(); + s.execute("shape.rectangle", &json!({"x": 10, "y": 10, "width": 50, "height": 40})).unwrap(); + let old = [ + ("effect.apply", json!({"effect": "blur.gaussian", "params": {"blurr": 3, "radius": "big"}})), + ("effect.setParams", json!({"index": 0, "params": {"amount": 1e308}})), + ]; + for (id, params) in &old { + assert!(s.check_params(id, params).is_err(), "{id}"); + s.execute(id, params).unwrap(); + } + let doc = &s.active().unwrap().doc; + let back = vectorcraft_format::load(&vectorcraft_format::save(doc, false)).unwrap(); + let stored = |d: &vectorcraft_doc::Document| d.layers[0].children().unwrap()[0].appearance.effects[0].params.clone(); + assert_eq!(stored(&back), stored(doc)); + assert_eq!(stored(&back)["blurr"], json!(3)); +} + /// What a command adds to its own journal entry passes the check, so a recorded journal can be /// sent back through the control channel or MCP. #[test] diff --git a/crates/mcp/src/headless.rs b/crates/mcp/src/headless.rs index 7050a65d..c30208ab 100644 --- a/crates/mcp/src/headless.rs +++ b/crates/mcp/src/headless.rs @@ -128,7 +128,7 @@ impl Headless { // A key the command's description doesn't name is an error, not a default (#1442). match HOST_COMMANDS.iter().find(|c| c.0 == id) { Some((_, _, doc)) => vectorcraft_engine::check_keys(id, doc, params), - None => vectorcraft_engine::check_params(id, params), + None => self.session.check_params(id, params), } .map_err(|e| e.to_string())?; match id { diff --git a/crates/ui-egui/src/control.rs b/crates/ui-egui/src/control.rs index 327a5b16..dc8e4957 100644 --- a/crates/ui-egui/src/control.rs +++ b/crates/ui-egui/src/control.rs @@ -144,7 +144,7 @@ pub fn handle(app: &mut VectorcraftApp, ctx: &egui::Context, req: &ControlReques let params = p.get("params").cloned().unwrap_or(json!({})); let params = if params.is_null() { json!({}) } else { params }; // A key the command's description doesn't name is an error, not a default (#1442). - if let Err(e) = crate::menus::check_params(id, ¶ms) { + if let Err(e) = crate::menus::check_params(&app.session, id, ¶ms) { return err(e); } wrap(app.run(id, params)) diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index 1f165d59..e6bfe9b4 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -623,7 +623,7 @@ impl VectorcraftApp { // In debug builds the app's own calls are held to the params descriptions too (#1442): a // panel passing a key its command doesn't read shows up in the tests. if cfg!(debug_assertions) { - menus::check_params(id, ¶ms)?; + menus::check_params(&self.session, id, ¶ms)?; } if let Some(r) = menus::run_ui_command(self, id, ¶ms) { return r; diff --git a/crates/ui-egui/src/menus.rs b/crates/ui-egui/src/menus.rs index fe43ad58..bb0fbc1f 100644 --- a/crates/ui-egui/src/menus.rs +++ b/crates/ui-egui/src/menus.rs @@ -905,11 +905,11 @@ pub(crate) fn opt_bool(p: &Value, key: &str) -> Result, String> { /// `params` has only keys the params description of engine command `id` names, or the parameter /// error naming the one it doesn't (#1442). The UI's own commands aren't checked yet: their /// descriptions haven't been audited against what they read. -pub fn check_params(id: &str, params: &Value) -> Result<(), String> { +pub fn check_params(session: &vectorcraft_engine::Session, id: &str, params: &Value) -> Result<(), String> { if UI_COMMANDS.iter().any(|c| c.0 == id) { return Ok(()); } - vectorcraft_engine::check_params(id, params).map_err(|e| e.to_string()) + session.check_params(id, params).map_err(|e| e.to_string()) } pub fn run_ui_command(app: &mut VectorcraftApp, id: &str, p: &Value) -> Option> { diff --git a/docs/mcp.md b/docs/mcp.md index 43414bd9..d56619ea 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -675,8 +675,20 @@ is refused before the command runs: → invalid parameters for `file.new`: unknown parameter `widht`; did you mean `width`? ``` -Only the top level is checked: the keys of an object inside a value (`artboardLayout: {…}`, an effect's `params`) are -the command's to read. A description with a `…` entry takes more keys than it lists: those of the command it names +Only the top level is checked: the keys of an object inside a value (`artboardLayout: {…}`) are the command's to +read. The one exception is an effect's `params`: `effect.apply` (and `appearance.addEffect`) and `effect.setParams` +check them against the effect's own entry in `effect.list`, its `defaults` and the keys its `params` text names. A key +the effect doesn't take, or a value of another type than its default (a string for a number, a number for a switch), +is refused the same way: + +```json +{"id": "effect.apply", "params": {"effect": "blur.gaussian", "params": {"blurr": 3}}} +→ invalid parameters for `effect.apply`: unknown parameter `blurr`; the command takes: radius +``` + +A number outside the range the effect documents is still stored as given and clamped when the effect draws. A +plug-in effect's parameters are resolved against its manifest instead, and `effect.setParams` as a step of +`command.batch` isn't checked (the effect it edits may only exist once the steps before it ran). A description with a `…` entry takes more keys than it lists: those of the command it names (`…settings as file.new`), or any key when it names none, as `document.export` and `document.save` do for the options of the format they write (one object can carry several formats' options). A command that takes its own reply back (`document.setup`, `text.areaOptions`) says so the same way. The app's own journal isn't checked: an action or