diff --git a/crates/engine/src/cmd.rs b/crates/engine/src/cmd.rs index a247ed52..c046cd19 100644 --- a/crates/engine/src/cmd.rs +++ b/crates/engine/src/cmd.rs @@ -63,6 +63,8 @@ macro_rules! spec { struct Registry { list: Vec, index: HashMap<&'static str, usize>, + /// The parameter names each command's doc lists (`list` order; see [`doc_param_names`]). + names: Vec>>, } fn registry() -> &'static Registry { @@ -123,7 +125,8 @@ fn registry() -> &'static Registry { log::warn!("duplicate command id {}", s.id); } } - Registry { list, index } + let names = list.iter().map(|s| doc_param_names(s.params)).collect(); + Registry { list, index, names } }) } @@ -136,6 +139,113 @@ pub fn find_command(id: &str) -> Option<&'static CommandSpec> { r.index.get(id).and_then(|i| r.list.get(*i)) } +/// The parameter names command `id` takes, from its doc; `None` when the doc isn't a `{…}` list +/// (`node.set` documents its props, and refuses the ones it doesn't know itself). +pub fn param_names(id: &str) -> Option<&'static [&'static str]> { + let r = registry(); + r.index.get(id).and_then(|i| r.names.get(*i)).and_then(|n| n.as_deref()) +} + +/// Refuse params naming something the command doesn't take: a misspelt name would otherwise be +/// ignored and the command answer as if it hadn't been given. `ids` and `id` are always taken: +/// the layers a command acts on (`params::targets` reads either). +pub fn refuse_unknown_params(id: &str, params: &Value) -> Result<()> { + let (Some(names), Some(given)) = (param_names(id), params.as_object()) else { return Ok(()) }; + let taken = |k: &str| matches!(k, "ids" | "id") || names.contains(&k); + let unknown: Vec = given.keys().filter(|k| !taken(k)).map(|k| format!("`{k}`")).collect(); + if unknown.is_empty() { + return Ok(()); + } + let s = if unknown.len() == 1 { "" } else { "s" }; + let takes = if names.is_empty() { "none".to_string() } else { names.join(", ") }; + Err(crate::bad(id, format!("unknown parameter{s} {} (it takes {takes})", unknown.join(", ")))) +} + +/// The top-level items of a `{…}` params doc (`{ids?, direction? (horizontal|vertical)}` → +/// `ids?`, ` direction? (horizontal|vertical)`), with those of the alternatives after it +/// (`{x, y} | {points}`); `None` when the doc doesn't start with one. +pub(crate) fn doc_items(doc: &'static str) -> Option> { + let mut body = doc.trim_start().strip_prefix('{')?; + let (mut items, mut depth, mut start) = (Vec::new(), 0usize, 0); + loop { + let mut rest = None; + for (i, c) in body.char_indices() { + match c { + '{' | '[' | '(' => depth += 1, + '}' | ']' | ')' if depth > 0 => depth -= 1, + '}' | ',' if depth == 0 => { + items.push(body.get(start..i).unwrap_or_default()); + start = i + c.len_utf8(); + if c == '}' { + rest = body.get(start..); + break; + } + } + _ => {} + } + } + let rest = rest?; + match rest.trim_start().strip_prefix('|').and_then(|r| r.trim_start().strip_prefix('{')) { + Some(next) => (body, start) = (next, 0), + None => return Some(items), + } + } +} + +/// The names one doc item gives: its first word and the words joined to it by `|`, `/` or `+` +/// (`id?/ids?`, `path | data (base64) + name`, `value? + newValue?`), without the `?` of optional +/// ones. Bracketed text describes a name, and what follows a `:`, `=` or `→` its value. +pub(crate) fn item_names(item: &'static str) -> Vec<&'static str> { + let mut outside = Vec::new(); + let (mut depth, mut start) = (0usize, None); + for (i, c) in item.char_indices() { + let value_follows = depth == 0 && matches!(c, ':' | '=' | '→'); + let in_word = depth == 0 && !value_follows && !c.is_whitespace() && !matches!(c, '(' | '[' | '{' | ')' | ']' | '}'); + match c { + '(' | '[' | '{' => depth += 1, + ')' | ']' | '}' => depth = depth.saturating_sub(1), + _ => {} + } + match (in_word, start) { + (true, None) => start = Some(i), + (false, Some(s)) => { + outside.push(item.get(s..i).unwrap_or_default()); + start = None; + } + _ => {} + } + if value_follows { + break; + } + } + if let Some(s) = start { + outside.push(item.get(s..).unwrap_or_default()); + } + let is_name = + |n: &&str| n.chars().next().is_some_and(|c| c.is_ascii_alphabetic() || c == '_') && n.chars().all(|c| c.is_ascii_alphanumeric() || c == '_'); + let mut words = outside.into_iter(); + let (mut names, mut word) = (Vec::new(), words.next()); + while let Some(w) = word { + names.extend(w.split(['/', '|']).map(|n| n.trim_end_matches('?')).filter(is_name)); + word = match words.next() { + Some("|" | "/" | "+") => words.next(), + _ => None, + }; + } + names +} + +/// Every name a `{…}` params doc lists, once each (see [`item_names`]). +fn doc_param_names(doc: &'static str) -> Option> { + let mut names = Vec::new(); + for n in doc_items(doc)?.into_iter().flat_map(item_names) { + if !names.contains(&n) { + names.push(n); + } + } + Some(names) +} + /// `{"ok": true}` shorthand. pub fn ok() -> Result { Ok(serde_json::json!({"ok": true})) diff --git a/crates/engine/src/history_behaviour_tests.rs b/crates/engine/src/history_behaviour_tests.rs index 45e0c7bf..8cd5612e 100644 --- a/crates/engine/src/history_behaviour_tests.rs +++ b/crates/engine/src/history_behaviour_tests.rs @@ -188,7 +188,7 @@ fn threads_replies_and_resolving() { |c| c.replies.iter().map(|m| m.text.as_str()).eq(["Sure", "Done"]), "replies stay in the order they were written", ), - ("comment.resolve", json!({"id": c, "author": "Sam"}), |c| c.resolved, "anyone in the file can resolve a thread, not just its author"), + ("comment.resolve", json!({"id": c}), |c| c.resolved, "anyone in the file can resolve a thread, not just its author (it takes no author)"), ("comment.resolve", json!({"id": c, "resolved": false}), |c| !c.resolved, "a resolved thread can be reopened"), ("comment.resolve", json!({"id": c}), |c| c.resolved, "resolving again marks it resolved"), ( diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index ea135ca1..37fcaf7d 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -484,6 +484,8 @@ mod tiff_tests; #[cfg(test)] mod transform_behaviour_tests; #[cfg(test)] +mod unknown_params_tests; +#[cfg(test)] mod variable_tests; #[cfg(test)] mod variables_behaviour_tests; diff --git a/crates/engine/src/live_tests.rs b/crates/engine/src/live_tests.rs index 5cdccf69..1bae1b10 100644 --- a/crates/engine/src/live_tests.rs +++ b/crates/engine/src/live_tests.rs @@ -158,8 +158,8 @@ fn viewport(page: NodeId, x: f64, y: f64, w: f64, h: f64) -> Presence { #[test] fn without_a_link_live_sessions_are_unavailable() { let mut s = Session::new(); - for cmd in ["collab.host", "collab.join"] { - let e = s.execute(cmd, &json!({"link": "figcraft://join?host=a:1&file=f&token=t"})).unwrap_err(); + for (cmd, p) in [("collab.host", json!({})), ("collab.join", json!({"link": "figcraft://join?host=a:1&file=f&token=t"}))] { + let e = s.execute(cmd, &p).unwrap_err(); assert!(matches!(e, EngineError::Disabled(..)), "{cmd}: {e}"); } assert_eq!(s.execute("collab.leave", &json!({})).unwrap()["ended"], false); diff --git a/crates/engine/src/session/exec.rs b/crates/engine/src/session/exec.rs index add12ac5..90bcb407 100644 --- a/crates/engine/src/session/exec.rs +++ b/crates/engine/src/session/exec.rs @@ -18,6 +18,7 @@ impl Session { /// as it was. pub fn execute(&mut self, id: &str, params: &Value) -> Result { let spec = find_command(id).ok_or_else(|| EngineError::UnknownCommand(id.to_string()))?; + crate::cmd::refuse_unknown_params(id, params)?; board::refuse_on_board(self, id)?; // Revert replaces the document's uid: refuse before ending previews or syncing branches. if id == "file.revert" && self.live().doc_uid.is_some() { diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 811724f2..fd64e7ab 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -478,11 +478,19 @@ fn every_command_survives_junk_params() { continue; } for j in &junk { - let mut s = sess(); - let _ = s.execute("shape.rectangle", &json!({"x": 0, "y": 0, "width": 10, "height": 10})); - let _ = s.execute("text.create", &json!({"text": "t"})); - if let Err(EngineError::Internal { cmd, msg }) = s.execute(spec.id, j) { - panic!("{cmd} panicked on {j}: {msg}"); + // Each junk object as given (names the command doesn't take are refused) and cut down + // to the names it takes, so the junk values still reach its parsers. + let mut taken = j.clone(); + if let (Some(names), Some(o)) = (crate::cmd::param_names(spec.id), taken.as_object_mut()) { + o.retain(|k, _| matches!(k.as_str(), "ids" | "id") || names.contains(&k.as_str())); + } + for params in if taken == *j { vec![j] } else { vec![j, &taken] } { + let mut s = sess(); + let _ = s.execute("shape.rectangle", &json!({"x": 0, "y": 0, "width": 10, "height": 10})); + let _ = s.execute("text.create", &json!({"text": "t"})); + if let Err(EngineError::Internal { cmd, msg }) = s.execute(spec.id, params) { + panic!("{cmd} panicked on {params}: {msg}"); + } } } } diff --git a/crates/engine/src/unknown_params_tests.rs b/crates/engine/src/unknown_params_tests.rs new file mode 100644 index 00000000..4124085f --- /dev/null +++ b/crates/engine/src/unknown_params_tests.rs @@ -0,0 +1,85 @@ +//! A parameter name a command doesn't take is refused, not ignored: a misspelt name would make the +//! command answer as if it hadn't been given (#1413). + +use serde_json::{Value, json}; + +use crate::Session; +use crate::cmd::{all_commands, param_names}; + +fn err(s: &mut Session, cmd: &str, params: Value) -> String { + match s.execute(cmd, ¶ms) { + Err(e) => e.to_string(), + Ok(v) => panic!("{cmd} {params} ran: {v}"), + } +} + +/// The issue's repro: a misspelt `filter` listed the resolved thread as if no filter were given. +#[test] +fn a_misspelt_parameter_is_refused_and_named() { + let mut s = Session::new(); + s.execute("comment.add", &json!({"x": 10, "y": 10, "text": "a"})).unwrap(); + s.execute("comment.resolve", &json!({"id": 1})).unwrap(); + let e = err(&mut s, "comment.list", json!({"filterr": "open"})); + assert!(e.contains("unknown parameter `filterr`") && e.contains("it takes page, filter, sort"), "{e}"); + assert_eq!(s.execute("comment.list", &json!({"filter": "open"})).unwrap()["comments"].as_array().map(Vec::len), Some(0)); + let e = err(&mut s, "edit.undo", json!({"steps": 2, "stepz": 1})); + assert!(e.contains("unknown parameters `steps`, `stepz` (it takes none)"), "{e}"); +} + +/// `ids` and `id` name the layers a command acts on, documented or not (`params::targets` reads +/// either), so `{id}` keeps working where a doc only says `ids?`. +#[test] +fn ids_and_id_are_always_taken() { + let mut s = Session::new(); + let r = crate::params::id_of(&s.execute("shape.rectangle", &json!({"x": 0, "y": 0, "width": 10, "height": 10})).unwrap()["id"]).unwrap(); + assert_eq!(param_names("align.left"), Some(&["ids"][..])); + s.execute("align.left", &json!({"id": r})).unwrap(); + s.execute("edit.undo", &json!({"ids": []})).unwrap(); +} + +/// Names a doc gives as alternatives (`id? | ids?`, `id?/ids?`, `path | data`) are all taken, and +/// `null` params or no params at all stay fine. +#[test] +fn documented_alternatives_and_empty_params_are_taken() { + assert_eq!(param_names("vector.fillRule").map(|n| n.contains(&"id") && n.contains(&"ids")), Some(true)); + assert_eq!(param_names("text.setFeatures").map(|n| n.contains(&"id") && n.contains(&"ids")), Some(true)); + assert_eq!(param_names("file.import").map(|n| n.contains(&"path") && n.contains(&"data") && n.contains(&"name")), Some(true)); + assert_eq!(param_names("file.export").map(|n| n.contains(&"scale") && n.contains(&"size")), Some(true)); + assert_eq!(param_names("collab.share").map(|n| n.contains(&"via") && !n.contains(&"local")), Some(true)); + assert_eq!(param_names("component.editProperty").map(|n| n.contains(&"value") && n.contains(&"newValue")), Some(true)); + // Docs whose shape used to hide names: a stray brace, a comma in an example, a second object. + assert_eq!(param_names("proto.interaction.add").map(|n| n.contains(&"delayMs") && n.contains(&"dest")), Some(true)); + assert_eq!(param_names("collab.join"), Some(&["code", "link"][..])); + assert_eq!(param_names("proto.interaction.set").map(|n| n.contains(&"trigger") && n.contains(&"interaction")), Some(true)); + // Alternative groups: `{x, y} | {points: …}`. + assert_eq!(param_names("vector.edit.paint"), Some(&["x", "y", "points"][..])); + assert_eq!(param_names("vector.edit.cut"), Some(&["segment", "t", "vertex", "from", "to"][..])); + // `node.set` documents its props and checks them itself. + assert_eq!(param_names("node.set"), None); + let mut s = Session::new(); + for p in [json!(null), json!({})] { + s.execute("select.all", &p).unwrap(); + } +} + +/// Every top-level item of every `{…}` params doc gives a name, so no parameter a doc describes is +/// refused. +#[test] +fn every_doc_item_names_a_parameter() { + let nameless: Vec = all_commands() + .iter() + .flat_map(|c| crate::cmd::doc_items(c.params).unwrap_or_default().into_iter().map(move |i| (c.id, i))) + .filter(|(_, i)| !i.trim().is_empty() && crate::cmd::item_names(i).is_empty()) + .map(|(id, i)| format!("{id}: `{i}`")) + .collect(); + assert!(nameless.is_empty(), "doc items without a name: {nameless:#?}"); +} + +/// A batch step's unknown parameter is refused, naming the step. +#[test] +fn batch_steps_refuse_unknown_parameters() { + let mut s = Session::new(); + let e = + err(&mut s, "command.batch", json!({"commands": [{"command": "select.all"}, {"command": "comment.list", "params": {"filterr": "open"}}]})); + assert!(e.contains("comment.list") && e.contains("step 1: unknown parameter `filterr`"), "{e}"); +} diff --git a/crates/ui-egui/src/gradient_shorthand_tests.rs b/crates/ui-egui/src/gradient_shorthand_tests.rs index db368f8e..0e5a9f0f 100644 --- a/crates/ui-egui/src/gradient_shorthand_tests.rs +++ b/crates/ui-egui/src/gradient_shorthand_tests.rs @@ -14,7 +14,7 @@ fn engine_execute_takes_the_gradient_shorthand() { let r = exec(&mut u, "shape.rectangle", json!({"x": 0, "y": 0, "width": 200, "height": 100})).unwrap()["id"].clone(); let short = json!({"type": "linearGradient", "stops": ["#ff0000", "#0000ff"], "from": [0, 0.5], "to": [1, 0.5]}); exec(&mut u, "node.set", json!({"ids": [r], "fills": [short]})).unwrap(); - let n = exec(&mut u, "node.get", json!({"id": r, "depth": 0})).unwrap(); + let n = exec(&mut u, "node.get", json!({"id": r})).unwrap(); let f = &n["fills"][0]; assert_eq!((f["type"].clone(), f["kind"].clone(), f["width"].clone()), (json!("gradient"), json!("linear"), json!([0.0, 1.5])), "{n}"); assert_eq!(f["stops"].as_array().map(Vec::len), Some(2), "{n}"); diff --git a/docs/control-protocol.md b/docs/control-protocol.md index 448a9979..8c03bda5 100644 --- a/docs/control-protocol.md +++ b/docs/control-protocol.md @@ -97,7 +97,8 @@ Run any command (the same ids as the menus, shortcuts, Actions palette, CLI and calls never open dialogs: dialog requests are dropped. Returns the command's result (creation commands return `{"id": N}`). Paints (`fills`, `strokes`, `paint`) take a colour, the full paint JSON or the gradient shorthand `{"type":"linearGradient","stops":["#hex","#hex"],"from"?,"to"?}` -(as in `docs/mcp.md`). +(as in `docs/mcp.md`). A parameter the command's doc (`engine.commands`) doesn't list is an error +naming it and the ones it takes, never ignored; `ids` and `id` are always taken. ```json {"id":1,"method":"engine.execute","params":{"command":"shape.rectangle","params":{"x":10,"y":10,"width":100,"height":50}}}