Effects: effect.apply and effect.setParams refuse parameters the effect doesn't take (#1592)

crates/engine/src/cmd/effectcmd.rs

@@ -280,6 +280,20 @@ fn move_effect(s: &mut Session, p: &Value) -> Result<Value> {
     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<String> {
+    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<Value> {
     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();

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(())
+    }
+}

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() {

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]

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<String, Value> = 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();