timeline.edit: refuse fields the feature's definition doesn't have (#1507)

A field the definition doesn't have is refused with the fields it does have, instead of reporting success and changing nothing (#1491).

Co-Authored-By: Erikas <[email protected]>
Co-Authored-By: Claude Sonnet 5.5 <[email protected]>

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::<Feature>(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<Step>,
+    depth: usize,
+) -> Option<Vec<Step>> {
+    if depth > 16 {
+        return None;
+    }
+    let check = |step: Step, v: &Value, path: &mut Vec<Step>| {
+        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<String>) {
+        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<Value> {
@@ -624,7 +747,7 @@ fn edit_feature(s: &mut Session, p: &Value) -> Result<Value> {
     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<Value> {
     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;

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}");
+}