Merge pull request #3972 from dotCooCoo/fix/trace-params-unknown-keys

Image Trace: imageTrace.make refuses unknown keys inside params

crates/engine/src/cmd/buildcmds.rs

@@ -951,40 +951,44 @@ fn view_param(cmd: &str, p: &Value) -> Result<Option<TraceView>> {
     v.as_str().and_then(TraceView::from_id).map(Some).ok_or_else(|| bad(cmd, format!("view must be one of {}", ids())))
 }
 
-fn trace_params(p: &Value) -> Result<tr::TraceParams> {
+/// The parameters command `c` traces with: the preset's, with `params` over them.
+fn trace_params(c: &str, p: &Value) -> Result<tr::TraceParams> {
     let name = str_param(p, "preset").unwrap_or("Default");
-    let enhanced = switch_or(p, "enhanced", false, "imageTrace.make")?;
+    let enhanced = switch_or(p, "enhanced", false, c)?;
     // (A preset without an enhanced version is traced as it is.)
     let base = enhanced.then(|| tr::enhanced_preset(name)).flatten().or_else(|| tr::preset(name));
-    let base = base.ok_or_else(|| bad("imageTrace.make", format!("unknown preset `{name}` (see imageTrace.presets)")))?;
+    let base = base.ok_or_else(|| bad(c, format!("unknown preset `{name}` (see imageTrace.presets)")))?;
     let Some(over) = p.get("params").filter(|v| !v.is_null()) else { return Ok(base) };
     let mut v = serde_json::to_value(&base).map_err(|e| EngineError::Other(e.to_string()))?;
-    let (Some(o), Some(src)) = (v.as_object_mut(), over.as_object()) else { return Err(bad("imageTrace.make", "params must be an object")) };
+    let (Some(o), Some(src)) = (v.as_object_mut(), over.as_object()) else { return Err(bad(c, "params must be an object")) };
+    // A key that isn't one of the settings is an error, with the nearest setting.
+    if let Some(key) = src.keys().find(|k| !o.contains_key(k.as_str())) {
+        return Err(super::params::unknown(c, key, &o.keys().map(String::as_str).collect::<Vec<_>>()));
+    }
     for (k, val) in src {
         o.insert(k.clone(), val.clone());
     }
-    let params: tr::TraceParams = serde_json::from_value(v).map_err(|e| bad("imageTrace.make", e.to_string()))?;
+    let params: tr::TraceParams = serde_json::from_value(v).map_err(|e| bad(c, e.to_string()))?;
     if !params.fills && !params.strokes {
-        return Err(bad("imageTrace.make", "Create needs fills, strokes or both"));
+        return Err(bad(c, "Create needs fills, strokes or both"));
     }
     if !(params.stroke_width.is_finite() && params.stroke_width > 0.0) {
-        return Err(bad("imageTrace.make", "strokeWidth must be a positive number of pixels"));
+        return Err(bad(c, "strokeWidth must be a positive number of pixels"));
     }
     Ok(params)
 }
 
 /// Document Library in Color mode: look the library's colours up into `params.swatches` → each
-/// one as traced (sRGB) with the colour to fill with.
-fn trace_library(s: &Session, params: &mut tr::TraceParams) -> Result<Vec<([u8; 3], Color)>> {
-    const C: &str = "imageTrace.make";
+/// one as traced (sRGB) with the colour to fill with. Errors name command `c`.
+fn trace_library(c: &str, s: &Session, params: &mut tr::TraceParams) -> Result<Vec<([u8; 3], Color)>> {
     if params.mode != tr::Mode::Color || params.palette != tr::Palette::DocumentLibrary {
         return Ok(vec![]);
     }
     let key = params.library.as_str();
     let colors = super::swatchlib::library_colors(s, key)
-        .ok_or_else(|| bad(C, format!("no swatch library `{key}` (see swatch.library.list; \"document\": the document's swatches)")))?;
+        .ok_or_else(|| bad(c, format!("no swatch library `{key}` (see swatch.library.list; \"document\": the document's swatches)")))?;
     if colors.is_empty() {
-        return Err(bad(C, format!("`{key}` has no colours")));
+        return Err(bad(c, format!("`{key}` has no colours")));
     }
     let library: Vec<([u8; 3], Color)> = colors
         .into_iter()
@@ -1088,9 +1092,9 @@ fn grouped(d: &mut Document, nodes: Vec<Node>, parents: &[Option<usize>]) -> Vec
 }
 
 fn trace_make(s: &mut Session, p: &Value, expand: bool) -> Result<Value> {
-    const C: &str = "imageTrace.make";
-    let mut params = trace_params(p)?;
-    let view = view_param(C, p)?;
+    let c = if expand { "imageTrace.makeAndExpand" } else { "imageTrace.make" };
+    let mut params = trace_params(c, p)?;
+    let view = view_param(c, p)?;
     let params_json = serde_json::to_value(&params).map_err(|e| EngineError::Other(e.to_string()))?;
     // The preset's name if the parameters are exactly that preset's or its enhanced version's,
     // else "Custom".
@@ -1114,12 +1118,12 @@ fn trace_make(s: &mut Session, p: &Value, expand: bool) -> Result<Value> {
                     _ => None,
                 }
             })
-            .ok_or_else(|| bad(C, "select an image (or an Image Trace group)"))?
+            .ok_or_else(|| bad(c, "select an image (or an Image Trace group)"))?
     };
     let blob = s.doc()?.doc.images.get(&img.key).ok_or_else(|| EngineError::Other(format!("image data `{}` is missing", img.key)))?;
     // An image over the tracer's size is traced scaled down (the paths are scaled back below).
     let raster = tr::Raster::decode_to_fit(&blob.bytes).map_err(|e| EngineError::Other(e.to_string()))?;
-    let library = trace_library(s, &mut params)?;
+    let library = trace_library(c, s, &mut params)?;
     let res = tr::trace_within(&raster, &params, MAX_TRACE_ANCHORS).map_err(|e| EngineError::Other(e.to_string()))?;
     let warnings = trace_warnings((img.width, img.height), (raster.width, raster.height));
     // Pixel space of the decoded raster → the image object's pixel space → document.

crates/engine/src/cmd/params.rs

@@ -202,7 +202,7 @@ pub fn check_keys(id: &str, doc: &'static str, params: &Value) -> Result<()> {
 
 /// The parameter error of command `id` for `key`, which isn't one of `keys`: with the nearest of
 /// them, or all of them.
-fn unknown(id: &str, key: &str, keys: &[&str]) -> crate::EngineError {
+pub(crate) fn unknown(id: &str, key: &str, keys: &[&str]) -> crate::EngineError {
     bad(
         id,
         match nearest(key, keys) {

crates/engine/src/tests_build.rs

@@ -2161,6 +2161,40 @@ fn image_trace_gradients_fills_with_a_gradient() {
     assert!(flat.len() > 1 && flat.iter().all(|n| matches!(n.appearance.fill_paint(), Paint::Solid { .. })));
 }
 
+/// A key inside `params` that isn't a setting is refused with the nearest setting, the image is
+/// left as it was, and the error names the command that was run.
+#[test]
+fn image_trace_refuses_an_unknown_setting() {
+    for id in ["imageTrace.make", "imageTrace.makeAndExpand"] {
+        let mut s = session();
+        let img = image_doc(&mut s);
+        let e = s.execute(id, &json!({"preset": "6 Colors", "params": {"mode": "color", "colours": 3}})).unwrap_err().to_string();
+        assert!(e.contains(&format!("`{id}`")) && e.contains("unknown parameter `colours`; did you mean `colors`?"), "{e}");
+        assert!(matches!(node(&s, img).kind, NodeKind::Image(_)));
+        let e = s.execute(id, &json!({"params": {"zz": 1}})).unwrap_err().to_string();
+        assert!(e.contains("unknown parameter `zz`; the command takes: ") && e.contains("colorDetail"), "{e}");
+        let e = s.execute(id, &json!({"enhanced": "yes"})).unwrap_err().to_string();
+        assert!(e.contains(&format!("`{id}`")), "{e}");
+        let library = json!({"preset": "6 Colors", "params": {"palette": "documentLibrary", "library": "No Such Library"}});
+        let e = s.execute(id, &library).unwrap_err().to_string();
+        assert!(e.contains(&format!("`{id}`")) && e.contains("no swatch library"), "{e}");
+    }
+}
+
+/// Every preset's settings, as imageTrace.presets lists them, trace when passed back whole.
+#[test]
+fn image_trace_takes_every_preset_back_as_params() {
+    let list = session().execute("imageTrace.presets", &json!({})).unwrap();
+    for preset in list["presets"].as_array().unwrap() {
+        for params in [&preset["params"], &preset["enhanced"]].into_iter().filter(|p| !p.is_null()) {
+            let mut s = session();
+            image_doc(&mut s);
+            let r = s.execute("imageTrace.make", &json!({"preset": preset["name"], "params": params}));
+            assert!(r.is_ok(), "{}: {r:?}", preset["name"]);
+        }
+    }
+}
+
 /// A preset's enhanced version: the preset with Auto Grouping and Shapes, under the preset's name.
 #[test]
 fn image_trace_enhanced_presets() {

docs/mcp.md

@@ -1447,7 +1447,8 @@ already walks Image Trace. Short agent entry points:
   format's first extension (`png8` gives `.png`, `template` gives `.vctemplate`). A step without `format` keeps the
   source file's extension (`.vectorcraft` over data sets).
 - Image Trace: `imageTrace.presets`, `imageTrace.make {id?, preset?, params?, view?}` (or `imageTrace.makeAndExpand`),
-  `imageTrace.setView`, `imageTrace.expand`, `imageTrace.release`.
+  `imageTrace.setView`, `imageTrace.expand`, `imageTrace.release`. A key inside `params` that is not one of the trace
+  settings, such as `colours`, is an error that names it and the nearest setting (`colors`), as for a command's own keys.
 - Live Paint: `livePaint.make`, `livePaint.merge`, `livePaint.fill {group?, point}`, `livePaint.strokeEdge`,
   `livePaint.info {group}`, `livePaint.release`, `livePaint.expand`, and `livePaint.gapOptions {ids?, detect?, size?,
   color?, closeGaps?}` (Object › Live Paint › Gap Options). With `detect: true`, paint stops at the gaps that run