Merge pull request #7044 from dotCooCoo/fix/effect-apply-strict-params

Effects: effect.apply refuses a wrongly typed parameter that has no default

crates/engine/src/cmd/effectcmd.rs

@@ -16,7 +16,7 @@ use vectorcraft_render::effects;
 use super::appearance::{ItemTarget, aligned, appearance_targets, index_param, item_indices, item_target, item_target_at};
 use super::expand::has_seeded;
 use super::rasterfx;
-use super::{CommandSpec, EngineError, NodeId, Result, Session, always, bad, cmd, fileio, has_doc, str_param, switch_or};
+use super::{CommandSpec, EngineError, NodeId, Result, Session, always, bad, check_switches, cmd, fileio, has_doc, str_param, switch_or};
 
 /// The name of the `svg.filter` effect drawing `xml`: its `<filter>` id. An effect names the
 /// document's filter of that name, so `xml` that differs from the document's filter of the same
@@ -76,7 +76,7 @@ fn effect_specs() -> Vec<CommandSpec> {
             "Effect Options",
             [],
             None,
-            "{index: int, params?: {…} (merged into the current parameters), warpStyle?: string (an existing Warp style suffix, e.g. arc or flag; changes only an applied Warp in place), threeDType?: plane|extrude|revolve|inflate (the 3D and Materials panel's object type: changes only an applied 3D effect in place into Rotate, Extrude & Bevel, Revolve or Inflate; the settings the new effect also takes keep their values, rotation angles wrapped into -180..180 outside Revolve, and the others start at its defaults; params then merge into the new effect's; the type the effect already has, with nothing else given, changes nothing and records no undo step), visible?: bool, item?: appearance item index|null (as effect.remove), ids?|id?: [..], target?: \"object\"|\"contents\", aligned?: bool (index and item are rows of the first target's stack: each other target edits its equal one, as the Appearance panel does for a mixed selection)} → {ids}",
+            "{index: int, params?: {…} (merged into the current parameters), warpStyle?: string (an existing Warp style suffix, e.g. arc or flag; changes only an applied Warp in place; params are checked against the new style), threeDType?: plane|extrude|revolve|inflate (the 3D and Materials panel's object type: changes only an applied 3D effect in place into Rotate, Extrude & Bevel, Revolve or Inflate; the settings the new effect also takes keep their values, rotation angles wrapped into -180..180 outside Revolve, and the others start at its defaults; params then merge into the new effect's; the type the effect already has, with nothing else given, changes nothing and records no undo step), visible?: bool (any other value but null is refused, when a step plays too), item?: appearance item index|null (as effect.remove), ids?|id?: [..], target?: \"object\"|\"contents\", aligned?: bool (index and item are rows of the first target's stack: each other target edits its equal one, as the Appearance panel does for a mixed selection)} → {ids}",
             has_doc,
             set_params
         ),
@@ -649,15 +649,28 @@ pub(crate) fn set_params_effects(s: &Session, p: &Value) -> Vec<String> {
         return vec![];
     };
     let Ok(targets) = effect_targets(s, p, item, index, C) else { return vec![] };
-    // A 3D effect whose type `threeDType` changes takes the new effect's parameters.
+    // A 3D effect whose type `threeDType` changes, and a Warp whose style `warpStyle` changes, take
+    // the new effect's parameters.
     let kind = three_d_type(p, C).ok().flatten();
+    let warp = p.get("warpStyle").and_then(Value::as_str).filter(|style| effects::WarpStyle::from_id(style).is_some());
     let effect = |&(id, at, index): &(NodeId, Option<usize>, usize)| {
         let id = &st.doc.node(id)?.appearance.effects_at(at)?.get(index)?.id;
-        Some(kind.filter(|_| effects::is_three_d(id)).map_or_else(|| id.clone(), str::to_string))
+        Some(edited_effect_id(id, kind, warp))
     };
     targets.iter().filter_map(effect).collect()
 }
 
+/// The id effect `id` has once `effect.setParams` sets its 3D type to `kind` or its Warp style to
+/// `warp` (each when given and when `id` is an effect of that family).
+fn edited_effect_id(id: &str, kind: Option<&str>, warp: Option<&str>) -> String {
+    let is_warp = id.strip_prefix("warp.").and_then(effects::WarpStyle::from_id).is_some();
+    match (kind, warp) {
+        (Some(to), _) if effects::is_three_d(id) => to.to_string(),
+        (_, Some(style)) if is_warp => format!("warp.{style}"),
+        _ => id.to_string(),
+    }
+}
+
 /// The effect `threeDType` in `p` turns a 3D effect into ([`effects::THREE_D_TYPES`]), when given.
 fn three_d_type(p: &Value, cmd: &str) -> Result<Option<&'static str>> {
     let key = "threeDType";
@@ -718,6 +731,7 @@ fn set_params(s: &mut Session, p: &Value) -> Result<Value> {
             Ok(format!("warp.{style}"))
         })
         .transpose()?;
+    check_switches(p, &["visible"], C)?;
     let visible = p.get("visible").and_then(Value::as_bool);
     let item = item_target(s, p, C)?;
     let edited = set_params_effects(s, p);

crates/engine/src/cmd/effectcmd/tests.rs

@@ -626,6 +626,50 @@ fn warp_options_non_warp_target_rejection_is_atomic() {
     assert!(!state.is_dirty());
 }
 
+/// `effect.setParams` refuses a `visible` that is not true or false, when called and when a step
+/// plays, and the effect, the document and the undo history stay as they were. `null` leaves the
+/// visibility as it is.
+#[test]
+fn set_params_refuses_a_visible_that_is_not_true_or_false() {
+    let (mut s, id) = session_with_rect();
+    s.execute("effect.apply", &json!({"effect": "distort.roughen", "item": null})).unwrap();
+    s.doc_mut().unwrap().mark_saved();
+    let before = s.doc().unwrap().doc.clone();
+    let undo = s.doc().unwrap().history.undo.len();
+    for visible in [json!(1), json!(0), json!("false"), json!([])] {
+        let p = json!({"index": 0, "item": null, "visible": visible, "params": {"size": 9}});
+        for replay in [false, true] {
+            let r = if replay { s.replay("effect.setParams", &p) } else { s.execute("effect.setParams", &p) };
+            let err = r.err().map(|e| e.to_string()).unwrap_or_default();
+            assert!(err.contains("`visible` must be true or false"), "{visible} (replay {replay}): {err}");
+            let state = s.doc().unwrap();
+            assert_eq!(state.doc, before, "{visible} (replay {replay})");
+            assert_eq!(state.history.undo.len(), undo, "{visible} (replay {replay})");
+            assert!(!state.is_dirty());
+        }
+    }
+    s.execute("effect.setParams", &json!({"index": 0, "item": null, "visible": null, "params": {"size": 9}})).unwrap();
+    let n = node(&s, id);
+    assert!(n.appearance.effects[0].visible);
+    assert_eq!(n.appearance.effects[0].params["size"], 9);
+}
+
+/// The effect `effect.setParams` checks the parameters against is the one it leaves: a Warp's new
+/// style and a 3D effect's new type, and the effect as it is otherwise.
+#[test]
+fn set_params_checks_the_effect_it_leaves() {
+    use super::edited_effect_id;
+    assert_eq!(edited_effect_id("warp.arc", None, Some("flag")), "warp.flag");
+    assert_eq!(edited_effect_id("warp.arc", None, None), "warp.arc");
+    assert_eq!(edited_effect_id("distort.roughen", None, Some("flag")), "distort.roughen");
+    assert_eq!(edited_effect_id("threeD.extrude", Some("threeD.revolve"), None), "threeD.revolve");
+    assert_eq!(edited_effect_id("warp.arc", Some("threeD.revolve"), None), "warp.arc");
+    let (mut s, _) = session_with_rect();
+    s.execute("effect.apply", &json!({"effect": "warp.arc", "item": null})).unwrap();
+    let p = json!({"index": 0, "item": null, "warpStyle": "flag", "params": {"bend": 25}});
+    assert_eq!(super::set_params_effects(&s, &p), ["warp.flag"]);
+}
+
 #[test]
 fn list_remove_and_set_params() {
     let (mut s, id) = session_with_rect();

crates/engine/src/cmd/params.rs

@@ -15,8 +15,8 @@
 //! commands a command runs, the steps of a `command.batch` or an `action.batch` included: a
 //! misspelled `widht` no longer runs the 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`]).
+//! `effect.setParams` answer a key the effect doesn't take, or a value of another type than the
+//! effect reads, with a parameter error ([`check_effect_params`]).
 //!
 //! A command with a parameter schema (`CommandSpec::schema`) is also checked against the schema
 //! before it runs ([`crate::schemas::check`]): the schema refuses a value of the wrong type or out
@@ -263,15 +263,15 @@ pub(crate) fn unknown(id: &str, key: &str, keys: &[&str]) -> crate::EngineError
 }
 
 /// `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, a `color` the effect can draw ([`check_effect_color`]) and a Levels or Curves
-/// `channel` the effect has ([`check_effect_channel`]). 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.
+/// effect takes (those of its dialog defaults and of its params description), each of the type
+/// the effect reads ([`check_effect_types`]), a `color` the effect can draw ([`check_effect_color`])
+/// and a Levels or Curves `channel` the effect has ([`check_effect_channel`]). Numbers out of range
+/// pass: the effect clamps them when it draws. An installed effect plug-in's are checked 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(());
+        return check_effect_types(id, effect, params);
     }
     let Some(info) = vectorcraft_render::effects::effect_info(effect) else { return Ok(()) };
     let defaults = info.defaults.as_object();
@@ -282,18 +282,10 @@ pub fn check_effect_params(id: &str, effect: &str, params: &Value) -> Result<()>
             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"))),
-            _ => {}
-        }
+    if let Some(key) = given.keys().find(|k| !keys.contains(&k.as_str())) {
+        return Err(unknown(id, key, &keys));
     }
+    check_effect_types(id, effect, params)?;
     // An SVG filter's XML is one drawable <filter> element.
     if effect == vectorcraft_render::effects::SVG_FILTER
         && let Some(xml) = given.get("xml")
@@ -329,6 +321,59 @@ pub fn check_effect_params(id: &str, effect: &str, params: &Value) -> Result<()>
     check_effect_channel(id, effect, params)
 }
 
+/// The type of the values in `params` of effect `effect`, as command `id` was given them: a key
+/// the effect has a dialog default for takes a value of its default's type, a key only the
+/// effect's params description names takes the type the effect reads ([`undefaulted_type`]), and
+/// an installed effect plug-in's take the types its manifest gives them. The `style` of Crop Marks
+/// is `roman` or `japanese`. A number given as text (`"10 pt"`), `true` for a number, `1` for a
+/// switch and `null` for either are refused.
+fn check_effect_types(id: &str, effect: &str, params: &Value) -> Result<()> {
+    let Value::Object(given) = params else { return Ok(()) };
+    if effect.starts_with(vectorcraft_plugins::effect::PREFIX) {
+        return match vectorcraft_plugins::effect::installed(effect) {
+            Some(plugin) => plugin.manifest().resolve_params(params).map(drop).map_err(|why| bad(id, why.to_string())),
+            None => Ok(()),
+        };
+    }
+    let Some(info) = vectorcraft_render::effects::effect_info(effect) else { return Ok(()) };
+    for (key, v) in given {
+        let want = info.defaults.get(key).cloned().or_else(|| undefaulted_type(effect, key));
+        match want {
+            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"))),
+            _ => {}
+        }
+    }
+    // Crop Marks draw one of two styles.
+    if effect == vectorcraft_render::effects::CROP_MARKS
+        && let Some(style) = given.get("style")
+        && style.as_str().and_then(vectorcraft_doc::marks::MarkStyle::parse).is_none()
+    {
+        return Err(bad(id, "`style` must be roman or japanese"));
+    }
+    Ok(())
+}
+
+/// A value of the type effect `effect` reads for `key`, a parameter its params description names
+/// without a dialog default: the earlier 3D Perspective and light (`perspective`, `lightAzimuth`,
+/// `lightElevation`) and Scribble `variation` are numbers, an Outline Stroke's `width` is a number
+/// and a Gaussian Blur's `linear` is true or false. A Glass or Texturizer `textureImage`, an SVG
+/// filter's `name` and `xml`, a Warp's `version` and the `style` of Crop Marks have checks of their
+/// own; they aren't listed.
+pub(crate) fn undefaulted_type(effect: &str, key: &str) -> Option<Value> {
+    use vectorcraft_render::effects;
+    let number = || Some(Value::from(0.0));
+    match key {
+        "perspective" | "lightAzimuth" | "lightElevation" if effects::is_three_d(effect) => number(),
+        "variation" if effect == "stylize.scribble" => number(),
+        "width" if effect == "path.outlineStroke" => number(),
+        "linear" if effect == "blur.gaussian" => Some(Value::Bool(false)),
+        _ => None,
+    }
+}
+
 /// Can an effect read `v` as its `color`: a string [`Color::from_hex`] reads (`#rrggbb`, `#rgb`,
 /// `#rrggbbaa`, black, white, red, green, blue), `[r, g, b]` whose first three entries are numbers
 /// (read on a 0..1 scale) or a color object (`{"model": "rgb", "r": …}`)? The effect draws its

crates/engine/src/tests/params.rs

@@ -600,6 +600,105 @@ fn effect_params_are_the_effects_own() {
     assert!(s.check_params("effect.setParams", &json!({"index": 7, "params": {"blurr": 3}})).is_ok());
 }
 
+/// An effect parameter that only the effect's params description names, with no dialog default,
+/// is held to the type the effect reads: the earlier 3D Perspective and light, Scribble's
+/// `variation` and an Outline Stroke's `width` are numbers, and a Gaussian Blur's `linear` is true
+/// or false. The `style` of Crop Marks is `roman` or `japanese`. Every such key of every effect is
+/// listed, or has a check of its own. A key left out keeps its default.
+#[test]
+fn an_effect_parameter_without_a_default_is_held_to_its_type() {
+    use crate::cmd::params::undefaulted_type;
+    use vectorcraft_render::effects::effect_catalog;
+    // Read by checks of their own: a stored texture or a texture to load, an SVG filter, a Warp's
+    // map and the style of Crop Marks.
+    let own_check = |effect: &str, key: &str| match key {
+        "textureImage" | "name" | "xml" => true,
+        "version" => effect.starts_with("warp."),
+        "style" => effect == "cropMarks",
+        _ => false,
+    };
+    let mut unlisted = vec![];
+    for e in effect_catalog() {
+        for key in doc_keys(e.params).unwrap().keys {
+            if e.defaults.get(key).is_none() && undefaulted_type(e.id, key).is_none() && !own_check(e.id, key) {
+                unlisted.push(format!("{}.{key}", e.id));
+            }
+        }
+    }
+    assert!(unlisted.is_empty(), "parameters without a default or a listed type: {unlisted:?}");
+    let apply = |effect: &str, params| json!({"effect": effect, "params": params});
+    for (effect, params, why) in [
+        ("threeD.revolve", json!({"perspective": true}), "`perspective` must be a number"),
+        ("threeD.extrude", json!({"lightAzimuth": "30"}), "`lightAzimuth` must be a number"),
+        ("threeD.rotate", json!({"lightElevation": null}), "`lightElevation` must be a number"),
+        ("threeD.inflate", json!({"perspective": "50%"}), "`perspective` must be a number"),
+        ("path.outlineStroke", json!({"width": "10 pt"}), "`width` must be a number"),
+        ("stylize.scribble", json!({"variation": false}), "`variation` must be a number"),
+        ("blur.gaussian", json!({"radius": 2, "linear": 1}), "`linear` must be true or false"),
+        ("cropMarks", json!({"style": "bogus"}), "`style` must be roman or japanese"),
+        ("cropMarks", json!({"style": 1}), "`style` must be roman or japanese"),
+        ("cropMarks", json!({"style": null}), "`style` must be roman or japanese"),
+    ] {
+        assert_eq!(refusal("effect.apply", apply(effect, params.clone())), why, "{effect} {params}");
+        assert_eq!(refusal("appearance.addEffect", apply(effect, params.clone())), why, "{effect} {params}");
+    }
+    for (effect, params) in [
+        ("threeD.revolve", json!({"perspective": 40, "lightAzimuth": 30.5, "lightElevation": 0})),
+        ("path.outlineStroke", json!({"width": 10})),
+        ("stylize.scribble", json!({"variation": 2})),
+        ("blur.gaussian", json!({"linear": true})),
+        ("blur.gaussian", json!({})),
+        ("cropMarks", json!({"style": "japanese"})),
+        ("cropMarks", json!({})),
+    ] {
+        assert!(check_params("effect.apply", &apply(effect, params.clone())).is_ok(), "{effect} {params}");
+    }
+}
+
+/// `effect.apply`, `appearance.addEffect` and `effect.setParams` refuse such a parameter of the
+/// wrong type when they run, and so does a step of a `command.batch` whose effect a step before it
+/// added, with the document and its undo history as they were. A saved step still plays with it,
+/// as with a key the effect no longer takes.
+#[test]
+fn effect_commands_refuse_an_undefaulted_parameter_of_the_wrong_type() {
+    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 state = |s: &Session| {
+        let st = s.doc().unwrap();
+        (st.history.undo.len(), st.doc.clone())
+    };
+    let before = state(&s);
+    let apply = |effect: &str, params| json!({"effect": effect, "params": params});
+    for (id, params, why) in [
+        ("effect.apply", apply("path.outlineStroke", json!({"width": "10 pt"})), "`width` must be a number"),
+        ("appearance.addEffect", apply("blur.gaussian", json!({"linear": 1})), "`linear` must be true or false"),
+        ("effect.apply", apply("threeD.revolve", json!({"perspective": "50"})), "`perspective` must be a number"),
+    ] {
+        let e = s.execute(id, &params).unwrap_err().to_string();
+        assert!(e.contains(why), "{id} {params}: {e}");
+        assert_eq!(state(&s), before, "{id} {params}");
+    }
+    let batch = json!({"commands": [
+        {"command": "effect.apply", "params": apply("path.outlineStroke", json!({"width": 3}))},
+        {"command": "effect.setParams", "params": {"index": 0, "params": {"width": "10 pt"}}},
+    ]});
+    let e = s.execute("command.batch", &batch).unwrap_err().to_string();
+    assert!(e.contains("`width` must be a number"), "{e}");
+    assert_eq!(state(&s), before);
+    s.execute("effect.apply", &apply("path.outlineStroke", json!({"width": 3}))).unwrap();
+    let applied = state(&s);
+    for params in [json!({"width": "10 pt"}), json!({"width": null}), json!({"width": true})] {
+        let e = s.execute("effect.setParams", &json!({"index": 0, "params": params.clone()})).unwrap_err().to_string();
+        assert!(e.contains("`width` must be a number"), "{params}: {e}");
+        assert_eq!(state(&s), applied, "{params}");
+    }
+    s.execute("effect.setParams", &json!({"index": 0, "params": {"width": 4}})).unwrap();
+    s.replay("effect.setParams", &json!({"index": 0, "params": {"retired": "x", "width": "10 pt"}})).unwrap();
+    let st = s.doc().unwrap();
+    assert_eq!(st.doc.layers[0].children().unwrap()[0].appearance.effects[0].params["width"], json!("10 pt"));
+}
+
 /// The Transform effect (`distort.transform`) is held to its params description: `effect.apply`
 /// and `effect.setParams` refuse each of its on/off options given as text, such as `reflectX:
 /// "false"`, and each of its numbers given as text, such as `copies: "0"`, and leave the effect

crates/engine/src/tests/plugins.rs

@@ -235,8 +235,16 @@ fn live_effect_plugins_reshape_editable_art_and_expand() {
     run(&mut s, "select.all", json!({}));
     run(&mut s, "object.move", json!({"dx": 50, "dy": 0}));
     assert_eq!(effected(&s), *node(&s, a).path_data().unwrap());
+    // A parameter of another type than the manifest gives it is refused.
+    let before = (s.doc().unwrap().doc.clone(), s.doc().unwrap().history.undo.len());
+    let wrong = json!({"ids": [a.0], "effect": "plugin.test.engine.tri", "params": {"size": "4 pt"}});
+    let e = s.execute("effect.apply", &wrong).unwrap_err().to_string();
+    assert!(e.contains("`size` must be a number"), "{e}");
+    assert_eq!((s.doc().unwrap().doc.clone(), s.doc().unwrap().history.undo.len()), before);
     run(&mut s, "effect.apply", json!({"ids": [a.0], "effect": "plugin.test.engine.tri", "params": {"size": 4}}));
     assert_eq!(effected(&s), triangle());
+    let e = s.execute("effect.setParams", &json!({"ids": [a.0], "index": 1, "params": {"size": true}})).unwrap_err().to_string();
+    assert!(e.contains("`size` must be a number"), "{e}");
     assert_eq!(node(&s, a).appearance.effects[1].params, json!({"size": 4}));
     // Uninstalled: the geometry is drawn as it is.
     run(&mut s, "plugin.remove", json!({"id": "test.engine.tri"}));