Merge #851

crates/engine/src/cmd/edit.rs

@@ -261,7 +261,7 @@ pub fn specs() -> Vec<CommandSpec> {
         }),
         spec!("effect.set", "Set effect", "{ids?, index, effect {..}}", true, |s, p| {
             let i = req_index("effect.set", p, "index")?;
-            let e: Effect = parse("effect.set", p, "effect")?.ok_or_else(|| bad("effect.set", "`effect` required"))?;
+            let e = super::props::effect_from("effect.set", p, "effect")?.ok_or_else(|| bad("effect.set", "`effect` required"))?;
             for id in req_targets("effect.set", s, p)? {
                 let n = s.doc_mut()?.get_mut(id)?;
                 if let Some(slot) = n.effects.get_mut(i) {
@@ -1382,8 +1382,8 @@ fn pattern_fill(s: &mut Session, p: &Value) -> Result<Value> {
 }
 
 fn effect_add(s: &mut Session, p: &Value) -> Result<Value> {
-    let e: Effect = match get(p, "effect") {
-        Some(v) => serde_json::from_value(v.clone()).map_err(|e| bad("effect.add", e.to_string()))?,
+    let e: Effect = match super::props::effect_from("effect.add", p, "effect")? {
+        Some(e) => e,
         None => {
             let ty = string(p, "type").unwrap_or_else(|| "dropShadow".into());
             Effect::of_type(&ty).ok_or_else(|| bad("effect.add", format!("unknown effect type `{ty}`")))?

crates/engine/src/cmd/props.rs

@@ -61,6 +61,48 @@ pub fn snake(v: &Value) -> Value {
     }
 }
 
+/// `v` as an effect list, refusing keys an effect doesn't read: serde dropped them without a word,
+/// so a shadow given `offsetY` kept the default offset.
+pub fn effects_from(cmd: &str, v: &Value) -> Result<Vec<Effect>> {
+    let effects: Vec<Effect> = serde_json::from_value(snake(v)).map_err(|e| bad(cmd, format!("effects: {e}")))?;
+    for (i, given) in v.as_array().into_iter().flatten().enumerate() {
+        refuse_ignored_effect_keys(cmd, &format!("effects[{i}]"), given, |v| serde_json::from_value(snake(v)).ok())?;
+    }
+    Ok(effects)
+}
+
+/// `p[key]` as one effect, read as serde reads it (its aliases too), refusing keys it doesn't read.
+pub fn effect_from(cmd: &str, p: &Value, key: &str) -> Result<Option<Effect>> {
+    let Some(v) = get(p, key) else { return Ok(None) };
+    let effect: Effect = serde_json::from_value(v.clone()).map_err(|e| bad(cmd, format!("`{key}`: {e}")))?;
+    refuse_ignored_effect_keys(cmd, &format!("`{key}`"), v, |v| serde_json::from_value(v.clone()).ok())?;
+    Ok(Some(effect))
+}
+
+/// Refuse the keys of effect `given` that `parse` ignores, naming the keys the effect takes. Set to
+/// null, a key it reads fails to parse or changes the effect; a key it ignores changes nothing.
+fn refuse_ignored_effect_keys(cmd: &str, at: &str, given: &Value, parse: impl Fn(&Value) -> Option<Effect>) -> Result<()> {
+    let Some(fields) = given.as_object() else { return Ok(()) };
+    let parsed = parse(given);
+    let ignored: Vec<&str> = fields
+        .iter()
+        .filter(|(_, v)| !v.is_null())
+        .filter(|(k, _)| {
+            let mut probe = fields.clone();
+            probe.insert((*k).clone(), Value::Null);
+            parse(&Value::Object(probe)).is_some_and(|e| Some(e) == parsed)
+        })
+        .map(|(k, _)| k.as_str())
+        .collect();
+    if ignored.is_empty() {
+        return Ok(());
+    }
+    let takes = parsed.and_then(|e| serde_json::to_value(e).ok()).map(crate::inspect::camel).unwrap_or_default();
+    let kind = takes.get("type").and_then(Value::as_str).unwrap_or("effect").to_string();
+    let names: Vec<&str> = takes.as_object().into_iter().flatten().map(|(k, _)| k.as_str()).collect();
+    Err(bad(cmd, format!("{at}: unknown properties: {} (a {kind} takes {})", ignored.join(", "), names.join(", "))))
+}
+
 pub(crate) fn enum_of<T: serde::de::DeserializeOwned>(v: &Value) -> Option<T> {
     let s = v.as_str()?;
     // Accept camelCase, snake/upper-case and spaced variants.
@@ -482,7 +524,7 @@ pub fn apply(cmd: &str, doc: &mut Document, id: NodeId, props: &Map<String, Valu
                 }
             }
             "cornerSmoothing" => doc.get_mut(id)?.corners.smoothing = f64_of(v).unwrap_or(0.0).clamp(0.0, 1.0),
-            "effects" => doc.get_mut(id)?.effects = serde_json::from_value(snake(v)).map_err(|e| bad(cmd, format!("effects: {e}")))?,
+            "effects" => doc.get_mut(id)?.effects = effects_from(cmd, v)?,
             "clip" | "clipsContent" => doc.get_mut(id)?.clip = v.as_bool().ok_or_else(|| bad(cmd, "`clip` must be a boolean"))?,
             "isMask" => doc.get_mut(id)?.is_mask = v.as_bool().unwrap_or(false),
             "devStatus" => {

crates/engine/src/effects_tests.rs

@@ -51,6 +51,32 @@ fn effect_set_edits_noise_texture_and_glass() {
     assert!(s.execute("effect.set", &json!({"ids": [r], "index": 0, "effect": {"type": "glass", "depth": "deep"}})).is_err());
 }
 
+/// A key an effect doesn't have is refused, naming what it takes: serde dropped it without a word,
+/// so a shadow given `offsetX`/`offsetY` (the names variable bindings use) kept the default offset.
+#[test]
+fn effects_refuse_keys_they_dont_have() {
+    let mut s = Session::new();
+    let r = rect(&mut s);
+    let shadow = json!({"type": "dropShadow", "color": "#00000040", "offsetX": 0, "offsetY": 1, "radius": 3});
+    let err = s.execute("node.set", &json!({"ids": [r], "props": {"effects": [shadow]}})).unwrap_err().to_string();
+    assert!(err.contains("effects[0]") && err.contains("offsetX, offsetY") && err.contains("offset"), "{err}");
+    assert!(s.doc().unwrap().node(r).unwrap().effects.is_empty(), "nothing stored");
+    let err = s.execute("effect.add", &json!({"ids": [r], "effect": {"type": "layerBlur", "radious": 8}})).unwrap_err().to_string();
+    assert!(err.contains("radious"), "{err}");
+    s.execute("effect.add", &json!({"ids": [r], "type": "dropShadow"})).unwrap();
+    let err = s.execute("effect.set", &json!({"ids": [r], "index": 0, "effect": {"type": "dropShadow", "spred": 2}})).unwrap_err().to_string();
+    assert!(err.contains("spred"), "{err}");
+    // Every kind's own keys still go in, camelCase or snake_case, and so does what inspect reports.
+    for ty in ["innerShadow", "layerBlur", "backgroundBlur", "noise", "texture", "glass"] {
+        s.execute("effect.add", &json!({"ids": [r], "type": ty})).unwrap();
+    }
+    let reported = crate::inspect::node_json(s.doc().unwrap(), r, 0)["effects"].clone();
+    s.execute("node.set", &json!({"ids": [r], "props": {"effects": reported}})).unwrap();
+    let camel = json!({"type": "dropShadow", "offset": [0, 1], "showBehind": true, "visible": true});
+    let snake = json!({"type": "innerShadow", "offset": [0, 1], "show_behind": false});
+    s.execute("node.set", &json!({"ids": [r], "props": {"effects": [camel, snake]}})).unwrap();
+}
+
 #[test]
 fn scaling_scales_grain_and_glass() {
     let mut s = Session::new();