From 6f90717e712d5052d64512c997a1d9a066b83cc7 Mon Sep 17 00:00:00 2001 From: MortenHusted Date: Sun, 18 Oct 2026 23:35:50 +0200 Subject: [PATCH] M3: effects refuse keys they don't read instead of dropping them node.set refuses unknown properties, but the effects inside it were read by serde, which drops keys it doesn't know. A drop shadow given offsetX/offsetY (the names variable bindings use for its offset) was stored with the default offset [0, 4] and the call said ok. effect.add and effect.set dropped them the same way. The three now refuse what the effect doesn't read and say what it takes: "effects[0]: unknown properties: offsetX, offsetY (a dropShadow takes blend, color, offset, radius, showBehind, spread, type, visible)". A key counts as read when setting it to null makes the effect fail to parse or come out different, so serde's aliases count as read without a list of them here. Co-Authored-By: Claude Opus 5.5 --- crates/engine/src/cmd/edit.rs | 6 ++-- crates/engine/src/cmd/props.rs | 44 +++++++++++++++++++++++++++++- crates/engine/src/effects_tests.rs | 26 ++++++++++++++++++ 3 files changed, 72 insertions(+), 4 deletions(-) diff --git a/crates/engine/src/cmd/edit.rs b/crates/engine/src/cmd/edit.rs index a4769a35..3c4a180d 100644 --- a/crates/engine/src/cmd/edit.rs +++ b/crates/engine/src/cmd/edit.rs @@ -261,7 +261,7 @@ pub fn specs() -> Vec { }), 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) { @@ -1376,8 +1376,8 @@ fn pattern_fill(s: &mut Session, p: &Value) -> Result { } fn effect_add(s: &mut Session, p: &Value) -> Result { - 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}`")))? diff --git a/crates/engine/src/cmd/props.rs b/crates/engine/src/cmd/props.rs index 8a1f4e1c..8fd4ff40 100644 --- a/crates/engine/src/cmd/props.rs +++ b/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> { + let effects: Vec = 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> { + 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) -> 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(v: &Value) -> Option { let s = v.as_str()?; // Accept camelCase, snake/upper-case and spaced variants. @@ -462,7 +504,7 @@ pub fn apply(cmd: &str, doc: &mut Document, id: NodeId, props: &Map 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" => { diff --git a/crates/engine/src/effects_tests.rs b/crates/engine/src/effects_tests.rs index be69fac2..607b7711 100644 --- a/crates/engine/src/effects_tests.rs +++ b/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();