diff --git a/crates/engine/src/cmd/edit.rs b/crates/engine/src/cmd/edit.rs index 339b6af0..2c90bf77 100644 --- a/crates/engine/src/cmd/edit.rs +++ b/crates/engine/src/cmd/edit.rs @@ -546,6 +546,129 @@ fn merge(dst: &mut Value, src: &Value, depth: usize) { } } +/// One step of a path into a feature's JSON definition. +#[derive(Clone)] +enum Step { + Key(String), + Index(usize), +} + +fn path_text(path: &[Step]) -> String { + let mut t = String::new(); + for s in path { + match s { + Step::Key(k) if t.is_empty() => t.push_str(k), + Step::Key(k) => t.push_str(&format!(".{k}")), + Step::Index(i) => t.push_str(&format!("[{i}]")), + } + } + t +} + +fn at_path<'a>(v: &'a Value, path: &[Step]) -> Option<&'a Value> { + path.iter().try_fold(v, |v, s| match s { + Step::Key(k) => v.get(k), + Step::Index(i) => v.get(i), + }) +} + +/// Does the stored feature read the field at `path` of `merged` (a feature's JSON with the edit +/// merged in)? serde skips a field it doesn't know whatever its value, but no field type takes a +/// flag, a text and nested lists alike. +fn reads_field(merged: &Value, path: &[Step]) -> bool { + let Some((last, parent)) = path.split_last() else { return true }; + [json!(true), json!("\u{1}"), json!([[[]]])].into_iter().any(|probe| { + let mut v = merged.clone(); + let slot = parent.iter().try_fold(&mut v, |v, s| match s { + Step::Key(k) => v.get_mut(k), + Step::Index(i) => v.get_mut(*i), + }); + match (slot, last) { + (Some(Value::Object(o)), Step::Key(k)) => { + o.insert(k.clone(), probe); + } + (Some(Value::Array(a)), Step::Index(i)) => match a.get_mut(*i) { + Some(x) => *x = probe, + None => return false, + }, + _ => return false, + } + serde_json::from_value::(v).is_err() + }) +} + +/// The first field of `set` that the feature's definition doesn't have. A field the definition +/// shows before or after the edit is known; one it leaves out (an unset option, a value at its +/// default) is known when the stored feature reads it. Setting an absent field to `null` +/// changes nothing either way and passes. +fn unknown_field( + set: &Value, + before: Option<&Value>, + after: Option<&Value>, + merged: &Value, + path: &mut Vec, + depth: usize, +) -> Option> { + if depth > 16 { + return None; + } + let check = |step: Step, v: &Value, path: &mut Vec| { + let one = [step]; + let (b, a) = (before.and_then(|x| at_path(x, &one)), after.and_then(|x| at_path(x, &one))); + let [step] = one; + path.push(step); + let found = if b.is_none() && a.is_none() && !v.is_null() && !reads_field(merged, path) { + Some(path.clone()) + } else { + unknown_field(v, b, a, merged, path, depth + 1) + }; + path.pop(); + found + }; + match set { + Value::Object(o) => o.iter().find_map(|(k, v)| check(Step::Key(k.clone()), v, path)), + Value::Array(items) => items.iter().enumerate().take(10_000).find_map(|(i, v)| check(Step::Index(i), v, path)), + _ => None, + } +} + +/// "unknown field `offset`; ConstructionPlane has `plane.base.name`, `plane.distance`…": the +/// editable fields where it was given, as paths. +fn unknown_field_message(field: &[Step], def: &Value, type_name: &str) -> String { + fn leaves(v: &Value, prefix: &str, top: bool, depth: usize, out: &mut Vec) { + match v { + Value::Object(o) if depth < 4 && !o.is_empty() => { + for (k, x) in o { + if top && NOT_EDITABLE.contains(&k.as_str()) { + continue; + } + let at = if prefix.is_empty() { k.clone() } else { format!("{prefix}.{k}") }; + leaves(x, &at, false, depth + 1, out); + } + } + _ if !prefix.is_empty() => out.push(format!("`{prefix}`")), + _ => {} + } + } + let parent = field.split_last().map(|(_, p)| p).unwrap_or_default(); + let mut known = Vec::new(); + if let Some(at) = at_path(def, parent) { + leaves(at, &path_text(parent), parent.is_empty(), 0, &mut known); + } + const SHOWN: usize = 40; + let more = known.len().saturating_sub(SHOWN); + known.truncate(SHOWN); + let mut list = known.join(", "); + if more > 0 { + list.push_str(&format!(" and {more} more")); + } + let field = path_text(field); + if list.is_empty() { + return format!("unknown field `{field}`; timeline.inspect lists {type_name}'s fields"); + } + format!("unknown field `{field}`; {type_name} has {list} (timeline.inspect shows them with their values)") +} + /// `profiles` (also a loft section's) given as the feature commands take it (`"all"`, /// `[index]`, `[{curves}]`, `[{point}]`...) becomes the stored form; the stored form passes. fn command_profiles(set: &Value, cmd: &str) -> Result { @@ -624,7 +747,7 @@ fn edit_feature(s: &mut Session, p: &Value) -> Result { let mut v = serde_json::to_value(&f).map_err(|e| EngineError::Other(e.to_string()))?; let set = &command_profiles(set, cmd)?; merge(&mut v, set, 0); - let mut nf: Feature = serde_json::from_value(v).map_err(|e| bad(cmd, format!("invalid feature: {e}")))?; + let mut nf: Feature = serde_json::from_value(v.clone()).map_err(|e| bad(cmd, format!("invalid feature: {e}")))?; nf.id = f.id; if let (FeatureKind::Extrude { extent: old, .. }, FeatureKind::Extrude { extent: new, .. }) = (&f.kind, &mut nf.kind) && old.direction != solidcraft_doc::Direction::Symmetric @@ -636,6 +759,13 @@ fn edit_feature(s: &mut Session, p: &Value) -> Result { if std::mem::discriminant(&nf.kind) != std::mem::discriminant(&f.kind) { return Err(bad(cmd, "the feature type cannot change")); } + // serde drops fields the definition doesn't have without a word: refuse them, or a typo + // would report success and change nothing. + let after = serde_json::to_value(&nf).map_err(|e| EngineError::Other(e.to_string()))?; + let before = serde_json::to_value(&f).map_err(|e| EngineError::Other(e.to_string()))?; + if let Some(field) = unknown_field(set, Some(&before), Some(&after), &v, &mut Vec::new(), 0) { + return Err(bad(cmd, unknown_field_message(&field, &before, f.kind.type_name()))); + } if let FeatureKind::BodyTransfer { bodies, from, to, transform } = &nf.kind { solidcraft_doc::validate_body_transfer(&s.doc, bodies, *from, *to, transform)?; nf.component = *to; diff --git a/crates/engine/src/cmd/edit_tests.rs b/crates/engine/src/cmd/edit_tests.rs index 1b5e9ee3..22ebcf03 100644 --- a/crates/engine/src/cmd/edit_tests.rs +++ b/crates/engine/src/cmd/edit_tests.rs @@ -492,3 +492,33 @@ fn timeline_folders_can_nest_and_reject_cycles() { run(&mut s, "timeline.ungroup", json!({"group": root})); assert_eq!(s.doc.timeline_groups.iter().find(|group| group.id == child).unwrap().parent, None, "deleting a folder promotes its children"); } + +/// A field the feature's definition doesn't have is refused with the fields it does have, +/// instead of reporting success and changing nothing (#1491). +#[test] +fn edit_refuses_unknown_fields() { + let mut s = Session::default(); + run(&mut s, "construct.plane.offset", json!({"base": "XZ", "offset": 50, "name": "P"})); + let e = s.execute("timeline.edit", &json!({"feature": "P", "set": {"offset": "-50"}})).unwrap_err().to_string(); + assert!(e.contains("unknown field `offset`") && e.contains("`plane.distance`"), "{e}"); + let e = s.execute("timeline.edit", &json!({"feature": "P", "set": {"plane": {"dist": "-50"}}})).unwrap_err().to_string(); + assert!(e.contains("unknown field `plane.dist`") && e.contains("`plane.distance`"), "{e}"); + let def = run(&mut s, "timeline.inspect", json!({"feature": "P"}))["definition"].clone(); + assert_eq!(def["plane"]["distance"], json!("50"), "nothing changed: {def}"); + // The field it does have edits as before. + run(&mut s, "timeline.edit", json!({"feature": "P", "set": {"plane": {"distance": "-50"}}})); + let def = run(&mut s, "timeline.inspect", json!({"feature": "P"}))["definition"].clone(); + assert_eq!(def["plane"]["distance"], json!("-50"), "{def}"); + + // Fields the definition leaves out at their default are still fields: `combine: false` + // and `bodies: []` aren't shown on a feature mirror, and set them as they are. + run(&mut s, "solid.box", json!({"length": 10, "width": 10, "height": 10, "name": "Base"})); + run(&mut s, "solid.mirror", json!({"features": ["Base"], "plane": "YZ", "name": "Twin"})); + run(&mut s, "timeline.edit", json!({"feature": "Twin", "set": {"combine": false, "bodies": []}})); + run(&mut s, "timeline.edit", json!({"feature": "Twin", "set": {"combine": true}})); + // An unknown field next to known ones refuses the whole edit. + let e = s.execute("timeline.edit", &json!({"feature": "Twin", "set": {"combine": false, "flip": true}})).unwrap_err().to_string(); + assert!(e.contains("unknown field `flip`") && e.contains("`combine`") && e.contains("`plane.name`"), "{e}"); + let def = run(&mut s, "timeline.inspect", json!({"feature": "Twin"}))["definition"].clone(); + assert_eq!(def["combine"], json!(true), "{def}"); +}