diff --git a/crates/engine/src/cmd/component.rs b/crates/engine/src/cmd/component.rs index 0fb64816..e35c3f5b 100644 --- a/crates/engine/src/cmd/component.rs +++ b/crates/engine/src/cmd/component.rs @@ -68,7 +68,7 @@ pub fn specs() -> Vec { let cmd = "instance.create"; let c = req_id(cmd, p, "component")?; let page = s.page()?; - let parent = id(p, "parent").unwrap_or(page); + let parent = crate::params::opt_id(cmd, p, "parent")?.unwrap_or(page); let doc = s.doc_mut()?; let c = main_of(doc, c).ok_or_else(|| bad(cmd, "`component` must be a component or component set"))?; let (x, y) = match (num(p, "x"), num(p, "y")) { diff --git a/crates/engine/src/cmd/create.rs b/crates/engine/src/cmd/create.rs index 2150701a..9b471290 100644 --- a/crates/engine/src/cmd/create.rs +++ b/crates/engine/src/cmd/create.rs @@ -167,7 +167,7 @@ pub fn create(s: &mut Session, p: &Value) -> Result { let t = string(p, "type").ok_or_else(|| bad(cmd, "`type` is required"))?; let (mut kind, base) = kind_for(&t).ok_or_else(|| bad(cmd, format!("unknown type `{t}`")))?; let page = s.page()?; - let parent = id(p, "parent").unwrap_or(page); + let parent = crate::params::opt_id(cmd, p, "parent")?.unwrap_or(page); let doc = s.doc()?; let pn = doc.get(parent)?; if !pn.kind.is_container() { @@ -255,7 +255,7 @@ pub fn create(s: &mut Session, p: &Value) -> Result { node.width = tw; node.height = th; } - let index = num(p, "index").map(|i| i.max(0.0) as usize); + let index = crate::params::opt_index(cmd, p, "index")?; doc.insert(node, parent, index)?; if let Some(Value::Object(props)) = get(p, "props") { super::props::apply(cmd, doc, nid, props)?; @@ -340,7 +340,7 @@ pub fn place_image(s: &mut Session, p: &Value) -> Result { _ => ImageScaleMode::Fill, }; let page = s.page()?; - let parent = id(p, "parent").unwrap_or(page); + let parent = crate::params::opt_id(cmd, p, "parent")?.unwrap_or(page); let ds = s.ds_mut()?; let doc = ds.doc_mut(); let (hash, iw, ih) = figcraft_format::add_image(doc, bytes).map_err(|e| bad(cmd, e.to_string()))?; diff --git a/crates/engine/src/id_params_behaviour_tests.rs b/crates/engine/src/id_params_behaviour_tests.rs new file mode 100644 index 00000000..c5207b44 --- /dev/null +++ b/crates/engine/src/id_params_behaviour_tests.rs @@ -0,0 +1,161 @@ +//! Layer ids in params: naming something that isn't a layer is a mistake, and says so, rather than +//! quietly meaning "none". +//! +//! `node.create`'s `parent` was `id(p, "parent").unwrap_or(page)`, so any value that isn't an id — +//! a `{"$step": n}` nothing had resolved, a name, a negative number — became "no parent" and the +//! layer landed on the page while the call reported success. The same shape sat on `image.place` +//! and `instance.create`'s parents, on `index`, and on the `ids` of every command that targets +//! layers (an entry that wasn't an id was dropped and the rest ran). + +use crate::params::id_of; +use crate::{EngineError, Session}; +use figcraft_doc::*; +use serde_json::{Value, json}; + +/// The page's children, by name (bottom first). +fn top(s: &Session) -> Vec { + let doc = s.doc().unwrap(); + let page = s.page().unwrap(); + doc.node(page).unwrap().children.iter().filter_map(|c| doc.node(*c).map(|n| n.name.clone())).collect() +} + +/// The message of a failure, or `None` when the call went through. +fn failed(r: std::result::Result) -> Option { + match r { + Ok(_) => None, + Err(e) => Some(e.to_string()), + } +} + +fn rect(parent: Value) -> Value { + json!({"type": "rectangle", "name": "child", "parent": parent, "x": 0, "y": 0, "width": 10, "height": 10}) +} + +fn card(s: &mut Session) -> NodeId { + let v = s.execute("node.create", &json!({"type": "frame", "name": "Card", "x": 0, "y": 0, "width": 100, "height": 100})).unwrap(); + id_of(&v["id"]).unwrap() +} + +#[test] +fn a_parent_that_is_not_an_id_is_refused_rather_than_becoming_the_page() { + let mut s = Session::new(); + let card = card(&mut s); + for parent in [json!({"$step": 0}), json!("Card"), json!(-1), json!(true)] { + let msg = failed(s.execute("node.create", &rect(parent.clone()))).unwrap_or_else(|| panic!("{parent} must be refused")); + assert!(msg.contains("`parent` must be a node id"), "{parent}: {msg}"); + } + assert_eq!(top(&s), ["Card"], "nothing landed on the page"); + let doc = s.doc().unwrap(); + assert!(doc.node(card).unwrap().children.is_empty(), "nor inside the frame"); +} + +#[test] +fn leaving_the_parent_out_or_null_still_means_the_page() { + let mut s = Session::new(); + let card = card(&mut s); + for parent in [None, Some(json!(null))] { + let mut p = rect(json!(0)); + match parent { + None => _ = p.as_object_mut().unwrap().remove("parent"), + Some(v) => p["parent"] = v, + } + s.execute("node.create", &p).unwrap(); + } + assert_eq!(top(&s), ["Card", "child", "child"]); + let doc = s.doc().unwrap(); + assert!(doc.node(card).unwrap().children.is_empty()); +} + +#[test] +fn a_parent_id_that_names_nothing_is_still_reported() { + let mut s = Session::new(); + card(&mut s); + let msg = failed(s.execute("node.create", &rect(json!(99999)))).unwrap_or_else(|| panic!("a missing layer must fail")); + assert!(msg.contains("no such node 99999"), "{msg}"); +} + +#[test] +fn an_index_that_is_not_a_whole_non_negative_number_is_refused() { + let mut s = Session::new(); + let card = card(&mut s); + // (A boolean is a number to `f64_of` — `true` reads as 1 — as for `x`, `width` and every + // other number, so it is deliberately not in this list.) + for index in [json!("last"), json!(1.5), json!(-1), json!(-0.5), json!(1e12)] { + let mut p = rect(json!(card)); + p["index"] = index.clone(); + let msg = failed(s.execute("node.create", &p)).unwrap_or_else(|| panic!("index {index} must be refused")); + assert!(msg.contains("`index`"), "{index}: {msg}"); + } + let doc = s.doc().unwrap(); + assert!(doc.node(card).unwrap().children.is_empty(), "nothing was inserted"); +} + +#[test] +fn a_whole_index_still_inserts_where_it_says() { + let mut s = Session::new(); + let card = card(&mut s); + for name in ["a", "b"] { + s.execute("node.create", &json!({"type": "rectangle", "name": name, "parent": card, "x": 0, "y": 0, "width": 10, "height": 10})).unwrap(); + } + s.execute("node.create", &json!({"type": "rectangle", "name": "mid", "parent": card, "index": 1, "x": 0, "y": 0, "width": 10, "height": 10})) + .unwrap(); + let doc = s.doc().unwrap(); + let names: Vec<&str> = doc.node(card).unwrap().children.iter().filter_map(|c| doc.node(*c).map(|n| n.name.as_str())).collect(); + assert_eq!(names, ["a", "mid", "b"]); +} + +#[test] +fn ids_entries_that_are_not_ids_are_refused_instead_of_being_dropped() { + let mut s = Session::new(); + let card = card(&mut s); + // The junk used to be dropped and the rest applied, so `["nope", card]` renamed the frame. + for ids in [json!(["nope"]), json!(["nope", card]), json!([{"$step": 0}, card]), json!([null])] { + let msg = + failed(s.execute("node.set", &json!({"ids": ids, "props": {"name": "renamed"}}))).unwrap_or_else(|| panic!("{ids} must be refused")); + assert!(msg.contains("`ids`"), "{ids}: {msg}"); + } + let doc = s.doc().unwrap(); + assert_eq!(doc.node(card).unwrap().name, "Card", "nothing was renamed"); +} + +#[test] +fn an_ids_that_is_not_an_array_is_refused_but_null_means_the_selection() { + let mut s = Session::new(); + let card = card(&mut s); + let msg = + failed(s.execute("node.set", &json!({"ids": "nope", "props": {"name": "renamed"}}))).unwrap_or_else(|| panic!("a non-array ids must fail")); + assert!(msg.contains("`ids` must be an array"), "{msg}"); + // An explicit null says the same as leaving it out: the selection. + s.execute("select.set", &json!({"ids": [card]})).unwrap(); + s.execute("node.set", &json!({"ids": null, "props": {"name": "Selected"}})).unwrap(); + assert_eq!(s.doc().unwrap().node(card).unwrap().name, "Selected"); +} + +#[test] +fn a_single_id_that_is_not_an_id_is_refused() { + let mut s = Session::new(); + card(&mut s); + let msg = + failed(s.execute("node.set", &json!({"id": {"$step": 0}, "props": {"name": "renamed"}}))).unwrap_or_else(|| panic!("a bad id must fail")); + assert!(msg.contains("`id` must be a node id"), "{msg}"); +} + +#[test] +fn an_image_and_an_instance_parent_are_checked_the_same_way() { + let mut s = Session::new(); + card(&mut s); + let img = failed(s.execute("image.place", &json!({"data": "aGk=", "parent": {"$step": 0}}))); + assert!(img.unwrap_or_default().contains("`parent` must be a node id"), "image.place"); + let inst = failed(s.execute("instance.create", &json!({"component": 99999, "parent": {"$step": 0}}))); + assert!(inst.is_some(), "instance.create refuses both a bad component and a bad parent"); +} + +#[test] +fn a_huge_bad_value_is_cut_short_in_the_error() { + let mut s = Session::new(); + card(&mut s); + let junk = "é".repeat(10_000); + let msg = failed(s.execute("node.set", &json!({"ids": junk, "props": {"name": "renamed"}}))).unwrap_or_else(|| panic!("must fail")); + assert!(msg.contains("`ids` must be an array") && msg.ends_with("…)"), "{msg}"); + assert!(msg.chars().count() < 200, "{} chars", msg.chars().count()); +} diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index 5e27c51a..f3a09cdb 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -1578,6 +1578,8 @@ mod history_behaviour_tests; #[cfg(test)] mod html_codegen_tests; #[cfg(test)] +mod id_params_behaviour_tests; +#[cfg(test)] mod import_export_tests; #[cfg(test)] mod inspect_behaviour_tests; diff --git a/crates/engine/src/params.rs b/crates/engine/src/params.rs index bbaa42a4..35a74e0e 100644 --- a/crates/engine/src/params.rs +++ b/crates/engine/src/params.rs @@ -88,6 +88,36 @@ pub fn req_id(cmd: &str, p: &Value, key: &str) -> Result { id(p, key).ok_or_else(|| bad(cmd, format!("`{key}` must be a node id"))) } +/// The offending value for an error message, cut to a few dozen characters so a huge param +/// doesn't end up in the error whole. +fn shown(v: &Value) -> String { + const MAX: usize = 40; + let text = v.to_string(); + match text.char_indices().nth(MAX) { + Some((cut, _)) => format!("{}…", &text[..cut]), + None => text, + } +} + +/// `key` when it is there: a layer id, or an error naming it. Missing and `null` mean "none", so a +/// command can tell "no parent given" (use the page) from "a parent was given that isn't an id" — +/// which is a mistake, not the default. Without this, `{"$step": 0}` reaching a param that isn't +/// resolved yet silently becomes "no parent" and the layer lands on the page. +pub fn opt_id(cmd: &str, p: &Value, key: &str) -> Result> { + match get(p, key) { + None | Some(Value::Null) => Ok(None), + Some(v) => id_of(v).map(Some).ok_or_else(|| bad(cmd, format!("`{key}` must be a node id (got {})", shown(v)))), + } +} + +/// `key` when it is there: a whole, non-negative number, or an error. Missing means "none". +pub fn opt_index(cmd: &str, p: &Value, key: &str) -> Result> { + match get(p, key) { + None | Some(Value::Null) => Ok(None), + Some(_) => req_index(cmd, p, key).map(Some), + } +} + /// `ids` (array), `id` (single), or the current selection. pub fn targets(s: &Session, p: &Value) -> Vec { if let Some(Value::Array(a)) = get(p, "ids") { @@ -99,8 +129,25 @@ pub fn targets(s: &Session, p: &Value) -> Vec { s.selection() } -/// Like [`targets`] but an empty result is an error. +/// Like [`targets`] but an unparseable `ids` entry, an unparseable `id`, or an empty result is an +/// error: naming something that isn't a layer is a mistake, not an empty selection. An explicit +/// `null` says the same as leaving the key out. pub fn req_targets(cmd: &str, s: &Session, p: &Value) -> Result> { + match get(p, "ids") { + Some(Value::Array(a)) => { + if let Some(v) = a.iter().find(|v| id_of(v).is_none()) { + return Err(bad(cmd, format!("`ids`: every entry must be a node id (got {})", shown(v)))); + } + } + Some(Value::Null) | None => { + if let Some(v) = get(p, "id").filter(|v| !v.is_null()) + && id_of(v).is_none() + { + return Err(bad(cmd, format!("`id` must be a node id (got {})", shown(v)))); + } + } + Some(v) => return Err(bad(cmd, format!("`ids` must be an array (got {})", shown(v)))), + } let t = targets(s, p); if t.is_empty() { return Err(EngineError::Disabled(cmd.to_string(), "nothing is selected (pass `ids`)".into()));