Merge pull request #6727 from dotCooCoo/fix/file-out-param-types

Package, Place, Export: the Package, Place and export commands reject a param of the wrong type

crates/engine/src/cmd/fileio/export.rs

@@ -9,14 +9,14 @@ use vectorcraft_doc::{Document, Node, NodeId, NodeKind, SymbolBoxes};
 
 use crate::DocState;
 
-use super::super::{Result, Session, bad, bool_or, edit, f64_or, str_param};
+use super::super::{Result, Session, bad, check_switches, edit, number_or, switch_or, text_param};
 use super::encode::single_artboard;
 use super::{
     ARTBOARD_PARAMS, Format, default_name, encode_all, encode_with_warnings, merge, with_black_output, writable, write_encoded, write_or_return,
 };
 
 pub(super) fn serialize(s: &mut Session, p: &Value) -> Result<Value> {
-    let f = writable("document.serialize", Some(str_param(p, "format").unwrap_or("vectorcraft")), None)?;
+    let f = writable("document.serialize", Some(text_param(p, "format", "document.serialize")?.unwrap_or("vectorcraft")), None)?;
     let expanded = expand_presets(s, "document.serialize", p)?;
     let p = with_black_output(s, &expanded);
     let (doc, display_doc_title) = source(s, f, &p, "document.serialize")?;
@@ -53,11 +53,11 @@ pub(super) fn serialize(s: &mut Session, p: &Value) -> Result<Value> {
 }
 
 pub(super) fn export(s: &mut Session, p: &Value) -> Result<Value> {
-    let path = str_param(p, "path");
+    let path = text_param(p, "path", "document.export")?;
     if let Some(path) = path {
         super::check_not_lossy_overwrite(s.doc()?, path, p, "document.export")?;
     }
-    let f = writable("document.export", str_param(p, "format"), path)?;
+    let f = writable("document.export", text_param(p, "format", "document.export")?, path)?;
     let expanded = expand_presets(s, "document.export", p)?;
     let p = with_black_output(s, &expanded);
     let (doc, display_doc_title) = source(s, f, &p, "document.export")?;
@@ -86,7 +86,7 @@ pub fn export_doc<'a>(st: &'a DocState, f: &Format, p: &Value) -> Result<Cow<'a,
 /// The document an export of `st` writes: the whole document, or with `selectedOnly` just the
 /// selected objects, each in its layers and groups (a clipping group keeps its clipping path).
 pub fn export_source<'a>(st: &'a DocState, p: &Value) -> Result<Cow<'a, Document>> {
-    if !bool_or(p, "selectedOnly", false) {
+    if !switch_or(p, "selectedOnly", false, "document.export")? {
         return Ok(Cow::Borrowed(&*st.doc));
     }
     let keep: HashSet<NodeId> = edit::roots_of(&st.doc, st.selection.in_paint_order(&st.doc)).into_iter().collect();
@@ -237,7 +237,8 @@ fn expand_presets<'a>(s: &Session, cmd: &str, p: &'a Value) -> Result<Cow<'a, Va
 /// The document an export of `f` writes: the active one (see [`export_source`]), or for text
 /// with `selectionOnly` the selected objects alone.
 fn source<'a>(s: &'a mut Session, f: &Format, p: &Value, cmd: &str) -> Result<(Cow<'a, Document>, bool)> {
-    if f.id == "txt" && bool_or(p, "selectionOnly", false) {
+    check_switches(p, &["selectedOnly"], cmd)?;
+    if switch_or(p, "selectionOnly", false, cmd)? && f.id == "txt" {
         let doc = selection(s, cmd, true)?.0;
         let display_doc_title = super::has_own_title(&doc);
         return Ok((Cow::Owned(doc), display_doc_title));
@@ -309,11 +310,11 @@ pub(crate) fn isolated(doc: &Document, ids: &[NodeId], name: &str) -> Option<(Do
 /// are guides, not artwork, and are left out.
 pub(super) fn export_selection(s: &mut Session, p: &Value) -> Result<Value> {
     const C: &str = "document.exportSelection";
-    if let Some(path) = str_param(p, "path") {
+    let path = text_param(p, "path", C)?;
+    if let Some(path) = path {
         super::check_not_lossy_overwrite(s.doc()?, path, p, C)?;
     }
-    let path = str_param(p, "path");
-    let f = writable(C, str_param(p, "format"), path)?;
+    let f = writable(C, text_param(p, "format", C)?, path)?;
     let expanded = expand_presets(s, C, p)?;
     let p = with_black_output(s, &expanded);
     let (d, bounds) = selection(s, C, f.id == "txt")?;
@@ -327,11 +328,12 @@ pub(super) fn export_selection(s: &mut Session, p: &Value) -> Result<Value> {
 /// `transparent`, with the encoder's warnings.
 pub(super) fn export_for_office(s: &mut Session, p: &Value) -> Result<Value> {
     const C: &str = "document.exportForOffice";
-    if let Some(path) = str_param(p, "path") {
+    let path = text_param(p, "path", C)?;
+    if let Some(path) = path {
         super::check_not_lossy_overwrite(s.doc()?, path, p, C)?;
     }
-    let path = str_param(p, "path");
-    let ppi = f64_or(p, "ppi", 150.0);
+    let ppi = number_or(p, "ppi", 150.0, C)?;
+    let transparent = switch_or(p, "transparent", false, C)?;
     let doc = &s.doc()?.doc;
     let artboard = match p.get("artboard") {
         None | Some(Value::Null) => 0,
@@ -340,7 +342,7 @@ pub(super) fn export_for_office(s: &mut Session, p: &Value) -> Result<Value> {
     if artboard >= doc.artboards.len() {
         return Err(bad(C, format!("no artboard {artboard} (0-based; the document has {})", doc.artboards.len())));
     }
-    let background = if bool_or(p, "transparent", false) { "transparent" } else { "white" };
+    let background = if transparent { "transparent" } else { "white" };
     let opts = with_black_output(s, &json!({ "ppi": ppi, "background": background, "artboard": artboard }));
     let (bytes, warnings) = encode_with_warnings(doc, "png", &opts)?;
     // The pixel size, from the PNG header.

crates/engine/src/cmd/package.rs

@@ -14,7 +14,7 @@ use serde_json::{Value, json};
 
 use super::fileio::{create_dir, write_file};
 use super::links::LinkBase;
-use super::{CommandSpec, Result, Session, bad, bool_or, cmd, has_doc, str_param, unique_name};
+use super::{CommandSpec, Result, Session, bad, cmd, has_doc, switch_or, text_param, unique_name};
 
 const C: &str = "file.package";
 
@@ -451,19 +451,20 @@ impl Session {
 /// name, the report's Document Info text and the document.
 fn prepare(s: &mut Session, p: &Value) -> Result<Package> {
     let o = Options {
-        copy_links: bool_or(p, "copyLinks", true),
-        links_folder: bool_or(p, "linksFolder", true),
-        relink: bool_or(p, "relink", true),
-        copy_fonts: bool_or(p, "copyFonts", true),
-        report: bool_or(p, "report", true),
+        copy_links: switch_or(p, "copyLinks", true, C)?,
+        links_folder: switch_or(p, "linksFolder", true, C)?,
+        relink: switch_or(p, "relink", true, C)?,
+        copy_fonts: switch_or(p, "copyFonts", true, C)?,
+        report: switch_or(p, "report", true, C)?,
     };
+    let given_name = text_param(p, "name", C)?;
+    let folder = text_param(p, "folder", C)?;
     let doc_path = s.doc()?.path.clone().ok_or_else(|| bad(C, "save the document first: Package collects a saved document"))?;
     let stem = Path::new(&doc_path).file_stem().map_or_else(|| "Untitled".into(), |s| s.to_string_lossy().into_owned());
-    let name = str_param(p, "name").map_or_else(|| format!("{stem} Folder"), str::to_string);
+    let name = given_name.map_or_else(|| format!("{stem} Folder"), str::to_string);
     if name.trim().is_empty() || name == "." || name == ".." || name.contains(['/', '\\', ':', '*', '?', '"', '<', '>', '|']) {
         return Err(bad(C, format!("`{name}` can't name a folder")));
     }
-    let folder = str_param(p, "folder");
     let root = folder.map(|f| Path::new(f).join(&name));
     let info = if o.report { Some(super::docinfo::report(s, false)?) } else { None };
     let doc = s.doc()?.doc.clone();

crates/engine/src/cmd/place.rs

@@ -47,7 +47,10 @@ use vectorcraft_geom::{Affine, Point, Rect, shapes};
 use vectorcraft_pdf::CropTo;
 
 use super::fileio::{self, Format, RasterImage};
-use super::{CommandSpec, Result, Session, always, bad, bool_or, cmd, finite_numbers, has_doc, object_param, str_param, unique_name};
+use super::{
+    CommandSpec, Result, Session, always, bad, bool_or, check_numbers, check_switches, cmd, finite_numbers, has_doc, object_param, str_param,
+    switch_or, unique_name,
+};
 use crate::{EngineError, MAX_COORD};
 
 const PLACE: &str = "file.place";
@@ -556,8 +559,9 @@ fn place(s: &mut Session, p: &Value) -> Result<Value> {
 }
 
 fn prepare(s: &Session, p: &Value) -> Result<PlaceJob> {
-    let replace = bool_or(p, "replace", false);
-    let template = bool_or(p, "template", false);
+    let replace = switch_or(p, "replace", false, PLACE)?;
+    let template = switch_or(p, "template", false, PLACE)?;
+    check_switches(p, &["link"], PLACE)?;
     let at = at_param(p)?;
     let rect = rect_param(p)?;
     if replace && (template || at.is_some() || rect.is_some()) {
@@ -778,6 +782,8 @@ impl Session {
     /// desktop the Place dialog's OK with several files does this off the UI thread).
     pub fn prepare_place_queue(&self, params: &Value) -> Result<QueueJob> {
         has_doc(self).map_err(|why| EngineError::Disabled(QUEUE.into(), why))?;
+        check_switches(params, &["link", "template"], QUEUE)?;
+        check_numbers(params, &["thumbnail"], QUEUE)?;
         Ok(QueueJob { params: params.clone(), files: queued_files(params)? })
     }
 
@@ -846,6 +852,8 @@ fn info(_: &mut Session, p: &Value) -> Result<Value> {
 /// `file.place.info` with `p`, without the session (on the desktop the Place dialog reads its
 /// files' lines this way off the UI thread).
 pub fn read_info(p: &Value) -> Result<Value> {
+    check_switches(p, &["link"], "file.place.info")?;
+    check_numbers(p, &["thumbnail"], "file.place.info")?;
     // A PDF/.ai page linked or not has the same size and art.
     let mut l = load(p, "file.place.info", None, false)?;
     let natural = l.natural;

crates/engine/src/tests/file.rs

@@ -61,6 +61,35 @@ fn serialize_and_reopen() {
     assert!(png.len() > 100);
 }
 
+#[test]
+fn export_params_of_the_wrong_type_are_refused_and_null_keeps_the_default() {
+    let mut s = session();
+    s.execute("text.create", &json!({"x": 10, "y": 40, "text": "Hello"})).unwrap();
+    s.execute("shape.rectangle", &json!({"x": 200, "y": 100, "width": 80, "height": 40})).unwrap();
+    for (cmd, p, msg) in [
+        ("document.serialize", json!({"format": 5}), "`format` must be a string"),
+        ("document.serialize", json!({"format": "svg", "selectedOnly": "true"}), "`selectedOnly` must be true or false"),
+        ("document.serialize", json!({"format": "txt", "selectionOnly": 1}), "`selectionOnly` must be true or false"),
+        ("document.serialize", json!({"format": "txt", "selectionOnly": true, "selectedOnly": "false"}), "`selectedOnly` must be true or false"),
+        ("document.export", json!({"format": "svg", "path": 5}), "`path` must be a string"),
+        ("document.exportSelection", json!({"format": "png", "path": 5}), "`path` must be a string"),
+        ("document.exportSelection", json!({"format": 5}), "`format` must be a string"),
+        ("document.exportForOffice", json!({"ppi": "300"}), "`ppi` must be a number"),
+        ("document.exportForOffice", json!({"transparent": "true"}), "`transparent` must be true or false"),
+        ("document.exportForOffice", json!({"path": 5}), "`path` must be a string"),
+    ] {
+        let e = match s.execute(cmd, &p) {
+            Ok(r) => panic!("{cmd} {p} gave {r}"),
+            Err(e) => e.to_string(),
+        };
+        assert!(e.contains(msg), "{cmd} {p}: {e}");
+    }
+    let r = s.execute("document.serialize", &json!({"format": null, "selectedOnly": null})).unwrap();
+    assert_eq!(r, s.execute("document.serialize", &json!({})).unwrap());
+    let r = s.execute("document.exportForOffice", &json!({"ppi": null, "transparent": null, "path": null})).unwrap();
+    assert_eq!(r, s.execute("document.exportForOffice", &json!({})).unwrap());
+}
+
 #[test]
 fn save_and_open_path() {
     let mut s = session();

crates/engine/src/tests/package.rs

@@ -98,6 +98,33 @@ fn an_unsaved_document_cant_be_packaged() {
     assert!(Session::new().execute("file.package", &json!({})).is_err(), "no document");
 }
 
+#[test]
+fn a_param_of_the_wrong_type_is_refused_and_null_keeps_the_default() {
+    let dir = Folder::new("package-types");
+    let mut s = poster(&dir);
+    let out = dir.file("out");
+    for (p, msg) in [
+        (json!({"folder": out, "copyLinks": "false"}), "`copyLinks` must be true or false"),
+        (json!({"folder": out, "linksFolder": 0}), "`linksFolder` must be true or false"),
+        (json!({"folder": out, "relink": "no"}), "`relink` must be true or false"),
+        (json!({"folder": out, "copyFonts": "false"}), "`copyFonts` must be true or false"),
+        (json!({"folder": out, "report": 1}), "`report` must be true or false"),
+        (json!({"folder": out, "name": 5}), "`name` must be a string"),
+        (json!({"folder": 5}), "`folder` must be a string"),
+    ] {
+        let e = match s.execute("file.package", &p) {
+            Ok(r) => panic!("{p} gave {r}"),
+            Err(e) => e.to_string(),
+        };
+        assert!(e.contains(msg), "{p}: {e}");
+        assert!(!std::path::Path::new(&out).exists(), "{p} wrote nothing");
+    }
+    let r = run(&mut s, "file.package", json!({"folder": null, "name": null, "copyLinks": null, "copyFonts": null, "report": null}));
+    assert_eq!(r["name"], "poster Folder.zip");
+    let files: Vec<&str> = r["files"].as_array().unwrap().iter().map(|f| f.as_str().unwrap()).collect();
+    assert_eq!(files, ["poster.vectorcraft", "Links/red.png", "Links/blue.png", "Fonts/SourceSans3-Regular.ttf", "poster Report.txt"]);
+}
+
 #[test]
 fn without_a_folder_the_zip_has_the_same_files() {
     let dir = Folder::new("package-zip");

crates/engine/src/tests/place.rs

@@ -954,6 +954,51 @@ fn bad_place_params_are_errors() {
     assert!(top_children(&s).is_empty());
 }
 
+#[test]
+fn place_params_of_the_wrong_type_are_refused_and_null_keeps_the_default() {
+    let mut s = session();
+    let ok = png(2, 2, [0; 4], None);
+    let with = |extra: Value| {
+        let mut p = file("x.png", &ok);
+        if let (Some(o), Value::Object(e)) = (p.as_object_mut(), extra) {
+            o.extend(e);
+        }
+        p
+    };
+    let queue = |extra: Value| {
+        let mut p = json!({ "files": [file("x.png", &ok)] });
+        if let (Some(o), Value::Object(e)) = (p.as_object_mut(), extra) {
+            o.extend(e);
+        }
+        p
+    };
+    for (cmd, p, msg) in [
+        ("file.place", with(json!({"link": "false"})), "`link` must be true or false"),
+        ("file.place", with(json!({"replace": 1})), "`replace` must be true or false"),
+        ("file.place", with(json!({"template": "true"})), "`template` must be true or false"),
+        ("file.place.queue", queue(json!({"link": "false"})), "`link` must be true or false"),
+        ("file.place.queue", queue(json!({"template": 1})), "`template` must be true or false"),
+        ("file.place.queue", queue(json!({"thumbnail": "8"})), "`thumbnail` must be a number"),
+        ("file.place.info", with(json!({"link": "no"})), "`link` must be true or false"),
+        ("file.place.info", with(json!({"thumbnail": "8"})), "`thumbnail` must be a number"),
+    ] {
+        let e = match s.execute(cmd, &p) {
+            Ok(r) => panic!("{cmd} gave {r}"),
+            Err(e) => e.to_string(),
+        };
+        assert!(e.contains(msg), "{cmd}: {e}");
+        assert_eq!(s.tool_id(), "selection", "{cmd}: the tool stays");
+        assert!(top_children(&s).is_empty(), "{cmd}: nothing placed");
+    }
+    let placed = s.execute("file.place", &with(json!({"link": null, "replace": null, "template": null}))).unwrap();
+    assert_eq!(placed["ids"].as_array().unwrap().len(), 1);
+    assert_eq!(top_children(&s).len(), 1, "placed on the current layer, not a template layer");
+    let info = s.execute("file.place.info", &with(json!({"link": null, "thumbnail": null}))).unwrap();
+    assert!(info.get("thumbnailBase64").is_none(), "{info}");
+    let r = s.execute("file.place.queue", &queue(json!({"link": null, "template": null, "thumbnail": null}))).unwrap();
+    assert_eq!(r["count"], 1, "{r}");
+}
+
 /// An SVG placed with `crop: "crop"` given is framed and clipped by its viewBox, as a PDF by its
 /// page, so art drawn on a shared canvas keeps its place and scale; by default (and with
 /// "bounding") it is fitted by its art's bounds (#788).

docs/mcp.md

@@ -1302,6 +1302,11 @@ doesn't take. Each of them refuses an unknown key inside an option object it tak
 → invalid parameters for `document.export`: unknown parameter `marks.useDocumentBleed`; `marks` takes: …
 ```
 
+Of the keys each one takes, `document.export`, `document.exportSelection`, `document.serialize` and
+`document.exportForOffice` refuse a `path` or `format` that is not a string, a `selectedOnly`, `selectionOnly` or
+`transparent` that is not true or false, and a `ppi` that is not a number, naming the key, and write nothing. Null is
+the same as leaving the key out.
+
 The save commands limit them the same way. `document.save`, `file.saveAs`, `file.saveCopy`, `file.saveAsTemplate`,
 `app.save` and `file.save` refuse a key that no save format takes, given beside the path or in `options`, and an
 unknown key inside a PDF option object or the `svg` object, also when the file is not SVG. A save still leaves out a key
@@ -5142,7 +5147,9 @@ that draws the image does not decode the file again.
 `file.place.info` describes a file without placing it and `image.info` reports a placed image's link, colour mode and
 effective ppi. `file.place.queue` loads the place cursor (the `place` tool) with several files: headless, drive it
 with `pointer_gesture` (a click places at 100% with the top-left corner there, a drag at the dragged size) and
-`press_key` (Left/Right/Up/Down cycle, Escape discards the current file).
+`press_key` (Left/Right/Up/Down cycle, Escape discards the current file). `file.place`, `file.place.queue` and
+`file.place.info` refuse, of the keys each one takes, a `link`, `replace` or `template` that is not true or false and a `thumbnail` that is not a
+number, naming the key, and place nothing; null is the same as leaving the key out.
 
 ```json
 {"name":"run_command","arguments":{"command":"file.place","params":{"path":"/tmp/photo.jpg","at":[300,200]}}}
@@ -5816,7 +5823,8 @@ type uses in `Fonts/` (fonts whose licence doesn't allow embedding are listed in
 the copies (each file copied once); one that can't be read is copied as it is, and a cycle of placed documents or one
 past the nesting limit stops there, each with a line in `warnings` and the report. A `folder/name` where a packaged file
 would replace the document or one of its linked files (the document's own folder, say) is an error, and nothing is
-written. Every option defaults to true. Without `folder` (the web, or an agent that wants the bytes) the
+written. Every option defaults to true. An option that is not true or false, or a `folder` or `name` that is not a
+string, is an error naming the key, and nothing is written; null is the same as leaving the key out. Without `folder` (the web, or an agent that wants the bytes) the
 result carries the same files as a zip (`{name: "<name>.zip", dataBase64}`, entries under `<name>/`).
 
 `document.info {selectionOnly?, category?, format?: "text"}` adds `sections` (`[{id, title, rows: [[label, value]]}]`: