Graphs: graph.setType, setData and create refuse params of the wrong type

graph.setType read `axisMin: "10"` as a cleared bound, so the axis went
back to the calculated range, and it ignored a string or number given for
a true/false option, a string for a number and a number for a text option.
graph.setData and graph.create ignored a `csv` that is not a string and
`series`, `categories` or `rows` that are not lists, and read a string in
`rows` as a blank cell.

The three commands now refuse such a value with an error naming the key,
and leave the graph and the undo history as they were. A null is still
read as a key left out, and a null axis bound still goes back to the
calculated value. A negative tick count is refused too.

crates/engine/src/cmd/graph.rs

@@ -17,7 +17,9 @@ use vectorcraft_doc::{
 use vectorcraft_geom::{Affine, BezPath, FillRule, PathData, Point, Rect, shapes};
 
 use super::typecmd::refresh_bounds;
-use super::{CommandSpec, EngineError, Result, Session, bad, bool_or, cmd, f64_or, f64_req, has_doc, has_selection, named_id, ok, str_param};
+use super::{
+    CommandSpec, EngineError, Result, Session, bad, cmd, f64_req, has_doc, has_selection, named_id, number_or, ok, str_param, switch_or, text_param,
+};
 
 mod axis;
 mod columns;

crates/engine/src/cmd/graph/axis.rs

@@ -1,4 +1,4 @@
-use super::{GraphSpec, Value};
+use super::{GraphSpec, Result, Value, bad};
 
 pub(super) const LABEL_SIZE: f64 = 9.0;
 
@@ -22,6 +22,16 @@ pub(super) fn count_param(p: &Value, key: &str, max: u64) -> Option<usize> {
     usize::try_from(n.min(max)).ok()
 }
 
+/// [`count_param`], or `None` when `p` leaves `key` out or gives null. A negative number is an error, and so is any
+/// other value, such as `"5"`.
+pub(super) fn count_given(p: &Value, key: &str, max: u64, cmd: &str) -> Result<Option<usize>> {
+    match p.get(key) {
+        None | Some(Value::Null) => Ok(None),
+        Some(v) if !v.is_number() => Err(bad(cmd, format!("`{key}` must be a number, not {v}"))),
+        Some(_) => count_param(p, key, max).map(Some).ok_or_else(|| bad(cmd, format!("`{key}` must be 0 or more"))),
+    }
+}
+
 /// A "nice" axis: (min, max, step).
 pub(super) fn nice_axis(lo: f64, hi: f64, ticks: usize) -> (f64, f64, f64) {
     let (lo, hi) = if (hi - lo).abs() < 1e-12 { (lo.min(0.0), lo.max(0.0) + 1.0) } else { (lo, hi) };

crates/engine/src/cmd/graph/commands.rs

@@ -12,7 +12,7 @@ pub fn specs() -> Vec<CommandSpec> {
             "Create Graph",
             [],
             None,
-            "{type: column|stackedColumn|bar|stackedBar|line|area|scatter|pie|radar, x, y, width, height, series?: [..], categories?: [..], rows?: [[number|null]], csv?} → {id}; an empty CSV cell or a null is a blank value, a number in straight quotes is a label",
+            "{type: column|stackedColumn|bar|stackedBar|line|area|scatter|pie|radar, x, y, width, height, series?: [..], categories?: [..], rows?: [[number|null]], csv?} → {id}; an empty CSV cell or a null is a blank value, a number in straight quotes is a label; a `type` that is not a string, a `width` or `height` that is not a number, and data of the wrong type (as `graph.setData` reads it) are refused",
             has_doc,
             create
         ),
@@ -21,7 +21,7 @@ pub fn specs() -> Vec<CommandSpec> {
             "Data…",
             ["Object", "Graph"],
             None,
-            "{id?, series?, categories?, rows?, csv?: first row = series labels (first cell empty); then one row per category: label; values…; an empty cell or a null is a blank value; a quoted number a label; a `|` in a label a line break, transpose?: true swaps rows and columns (Transpose row/column), switchXY?: true swaps each scatter series' y and x columns (Switch x/y)} replace the graph's data; no data → the current {csv, series, categories, rows}",
+            "{id?, series?, categories?, rows?, csv?: first row = series labels (first cell empty); then one row per category: label; values…; an empty cell or a null is a blank value; a quoted number a label; a `|` in a label a line break, transpose?: true swaps rows and columns (Transpose row/column), switchXY?: true swaps each scatter series' y and x columns (Switch x/y)} replace the graph's data; a `csv` that is not a string, `series` or `categories` that are not lists, `rows` that are not lists of numbers and nulls, or a `transpose` or `switchXY` that is not true or false is refused and the graph stays as it was; no data → the current {csv, series, categories, rows}",
             has_selection,
             set_data
         ),
@@ -30,7 +30,7 @@ pub fn specs() -> Vec<CommandSpec> {
             "Type…",
             ["Object", "Graph"],
             None,
-            "{id?, type?, seriesIndexes?: [index], columnWidth?: % (1–1000; past 100 columns overlap), clusterWidth?: % (1–1000), legend?: bool, markPoints?: bool, connectPoints?: bool, edgeToEdge?: bool (line graphs: true runs the lines across the whole plot, false puts the points at the centres of their categories), ticks?: n, axisMin?, axisMax?, valueAxis?: left|right|both (the bar graphs' value axis stays along the bottom), separateScales?: bool, rightTicks?: n, rightAxisMin?, rightAxisMax?, tickLength?: none|short|full, tickMarks?: n per division, rightTickLength?, rightTickMarks?, categoryTickLength?: none|short|full, categoryTickMarks?: n, ticksBetweenLabels?: bool, prefix?, suffix?, rightPrefix?, rightSuffix?: text around the value axis numbers, firstRowInFront?: bool, firstColumnInFront?: bool (which overlapping columns are drawn on top: the first category over the next within each series, the first series over the others), filledLines?: bool (Draw Filled Lines: connecting lines as 3 pt bands filled with the series colour), dropShadow?: bool (Add Drop Shadow: a translucent black shadow down and right of the columns, bars, lines and pie wedges), legendAcrossTop?: bool (Add Legend Across Top: the legend in rows above the plot), pieLegend?: none|standard|wedges (Legends in Wedges: the series labels inside their wedges), piePosition?: ratio|even|stacked (several pies sized by their totals, at one size (the default), or stacked), pieSort?: all|first|none (wedges largest first in each pie, in the first pie's order, or in data order (the default)); the query lists the pie options for pie graphs} change the graph type and options; with `seriesIndexes`, or (no `id`) with only series selected with Group Selection, `type` goes to those series only and `valueAxis` (left|right; both is refused) puts them on that value axis, while the other options still apply to the whole graph (Combine different graph types: column, stacked column, line and area mix, and so do bar and stacked bar; a series given the graph's type follows the graph again) (axisMin and axisMax together override the calculated value axis: exactly that range in `ticks` divisions, 5 when 0; with the value axis on both sides, separateScales gives the series on the right axis a scale of their own, set the same way with rightTicks, rightAxisMin and rightAxisMax); no options → the current ones",
+            "{id?, type?, seriesIndexes?: [index], columnWidth?: % (1–1000; past 100 columns overlap), clusterWidth?: % (1–1000), legend?: bool, markPoints?: bool, connectPoints?: bool, edgeToEdge?: bool (line graphs: true runs the lines across the whole plot, false puts the points at the centres of their categories), ticks?: n, axisMin?: number|null (null = calculated), axisMax?: number|null, valueAxis?: left|right|both (the bar graphs' value axis stays along the bottom), separateScales?: bool, rightTicks?: n, rightAxisMin?, rightAxisMax?, tickLength?: none|short|full, tickMarks?: n per division, rightTickLength?, rightTickMarks?, categoryTickLength?: none|short|full, categoryTickMarks?: n, ticksBetweenLabels?: bool, prefix?, suffix?, rightPrefix?, rightSuffix?: text around the value axis numbers, firstRowInFront?: bool, firstColumnInFront?: bool (which overlapping columns are drawn on top: the first category over the next within each series, the first series over the others), filledLines?: bool (Draw Filled Lines: connecting lines as 3 pt bands filled with the series colour), dropShadow?: bool (Add Drop Shadow: a translucent black shadow down and right of the columns, bars, lines and pie wedges), legendAcrossTop?: bool (Add Legend Across Top: the legend in rows above the plot), pieLegend?: none|standard|wedges (Legends in Wedges: the series labels inside their wedges), piePosition?: ratio|even|stacked (several pies sized by their totals, at one size (the default), or stacked), pieSort?: all|first|none (wedges largest first in each pie, in the first pie's order, or in data order (the default)); the query lists the pie options for pie graphs} change the graph type and options; with `seriesIndexes`, or (no `id`) with only series selected with Group Selection, `type` goes to those series only and `valueAxis` (left|right; both is refused) puts them on that value axis, while the other options still apply to the whole graph (Combine different graph types: column, stacked column, line and area mix, and so do bar and stacked bar; a series given the graph's type follows the graph again) (axisMin and axisMax together override the calculated value axis: exactly that range in `ticks` divisions, 5 when 0; with the value axis on both sides, separateScales gives the series on the right axis a scale of their own, set the same way with rightTicks, rightAxisMin and rightAxisMax); a value of the wrong type, such as \"10\" for a number or \"true\" for a bool, or a negative count, is refused and the graph stays as it was; a null is read as a key left out, except that a null axis bound goes back to the calculated value; no options → the current ones",
             has_selection,
             set_type
         ),

crates/engine/src/cmd/graph/data.rs

@@ -1,6 +1,6 @@
 use super::edit::{regenerate, spec_of, target};
 use super::series::{switch_xy, transpose};
-use super::{Document, EngineError, GraphKind, GraphSpec, NodeId, Result, Session, Value, bad, bool_or, json, str_param};
+use super::{Document, EngineError, GraphKind, GraphSpec, NodeId, Result, Session, Value, bad, json, switch_or, text_param};
 
 // ---------- data ----------
 
@@ -101,7 +101,7 @@ pub(crate) fn apply_csv(d: &mut Document, id: NodeId, csv: &str) -> Result<()> {
         return Err(bad("graph.applyCsv", format!("node {} is not a graph", id.0)));
     };
     let mut spec = current.clone();
-    apply_data(&mut spec, &json!({ "csv": csv }));
+    apply_data(&mut spec, &json!({ "csv": csv }), "graph.applyCsv")?;
     keep_right_series(&mut spec);
     if spec == *current {
         return Ok(());
@@ -115,34 +115,58 @@ fn keep_right_series(g: &mut GraphSpec) {
     g.right_series.retain(|i| *i < count);
 }
 
-pub(super) fn apply_data(g: &mut GraphSpec, p: &Value) -> bool {
+/// Apply the data `p` gives (`csv`, `series`, `categories`, `rows`) to `g`; true when it gave any. A null is read as a
+/// key left out. A `csv` that is not a string, a `series` or `categories` that is not a list, and `rows` that are not
+/// lists of numbers and nulls are errors, and `g` is left as it was.
+pub(super) fn apply_data(g: &mut GraphSpec, p: &Value, cmd: &str) -> Result<bool> {
+    let list = |key: &str| -> Result<Option<&Vec<Value>>> {
+        match p.get(key) {
+            None | Some(Value::Null) => Ok(None),
+            Some(Value::Array(a)) => Ok(Some(a)),
+            Some(v) => Err(bad(cmd, format!("`{key}` must be a list, not {v}"))),
+        }
+    };
+    let csv = text_param(p, "csv", cmd)?;
+    let strings = |a: &Vec<Value>| a.iter().map(|x| x.as_str().map(str::to_string).unwrap_or_else(|| x.to_string())).collect::<Vec<_>>();
+    let series = list("series")?.map(strings);
+    let categories = list("categories")?.map(strings);
+    // A null is a blank cell.
+    let rows = list("rows")?
+        .map(|rows| {
+            rows.iter()
+                .map(|r| {
+                    let bad_row = || bad(cmd, format!("`rows` must be lists of numbers or nulls, not {r}"));
+                    r.as_array()
+                        .ok_or_else(bad_row)?
+                        .iter()
+                        .map(|v| if v.is_null() { Ok(None) } else { v.as_f64().map(|v| Some(v).filter(|v| v.is_finite())).ok_or_else(bad_row) })
+                        .collect::<Result<Vec<_>>>()
+                })
+                .collect::<Result<Vec<_>>>()
+        })
+        .transpose()?;
     let mut changed = false;
-    if let Some(csv) = str_param(p, "csv") {
+    if let Some(csv) = csv {
         let (s, c, r) = parse_csv(csv);
         g.series = s;
         g.categories = c;
         g.set_cells(r);
         changed = true;
     }
-    let strings =
-        |v: &Value| v.as_array().map(|a| a.iter().map(|x| x.as_str().map(str::to_string).unwrap_or_else(|| x.to_string())).collect::<Vec<_>>());
-    if let Some(s) = p.get("series").and_then(strings) {
+    if let Some(s) = series {
         g.series = s;
         changed = true;
     }
-    if let Some(c) = p.get("categories").and_then(strings) {
+    if let Some(c) = categories {
         g.categories = c;
         changed = true;
     }
-    if let Some(rows) = p.get("rows").and_then(Value::as_array) {
-        // `null` (or anything that isn't a finite number) is a blank cell.
-        g.set_cells(
-            rows.iter().map(|r| r.as_array().map(|a| a.iter().map(|v| v.as_f64().filter(|v| v.is_finite())).collect()).unwrap_or_default()).collect(),
-        );
+    if let Some(rows) = rows {
+        g.set_cells(rows);
         changed = true;
     }
     drop_empty_rows(g);
-    changed
+    Ok(changed)
 }
 
 /// Drop rows with neither a value nor a label; a label-only row stays, its values blank. Each kept row keeps its own
@@ -184,13 +208,13 @@ pub(super) fn set_data(s: &mut Session, p: &Value) -> Result<Value> {
     const C: &str = "graph.setData";
     let id = target(s, p, C)?;
     let mut spec = spec_of(s, id)?;
-    let flip = bool_or(p, "transpose", false);
-    let swap = bool_or(p, "switchXY", false);
+    let flip = switch_or(p, "transpose", false, C)?;
+    let swap = switch_or(p, "switchXY", false, C)?;
     if swap && spec.kind != GraphKind::Scatter {
         return Err(bad(C, "Switch x/y is for scatter graphs"));
     }
     // Transpose and Switch x/y act after any new data in the same call.
-    let changed = apply_data(&mut spec, p);
+    let changed = apply_data(&mut spec, p, C)?;
     if flip {
         // The categories become series: at most as many as a graph has series.
         if spec.cells().len() > vectorcraft_doc::MAX_GRAPH_SERIES {

crates/engine/src/cmd/graph/edit.rs

@@ -1,12 +1,22 @@
-use super::axis::{MAX_TICK_MARKS, count_param, one_line};
+use super::axis::{MAX_TICK_MARKS, count_given, one_line};
 use super::data::apply_data;
 use super::generate::{children_bounds, generate};
 use super::series::{picked_series, reselect_series};
 use super::{
-    Affine, Document, EngineError, GraphKind, GraphSpec, Node, NodeId, Rect, Result, Session, TickLength, Value, ValueAxisSide, bad, bool_or, f64_or,
-    f64_req, json, named_id, str_param,
+    Affine, Document, EngineError, GraphKind, GraphSpec, Node, NodeId, Rect, Result, Session, TickLength, Value, ValueAxisSide, bad, f64_req, json,
+    named_id, number_or, switch_or, text_param,
 };
 
+/// An axis bound `p` gives for `key`: `None` when left out, `Some(None)` for null (calculated from the data), or the
+/// number. Any other value, such as `"10"`, is an error.
+fn bound_param(p: &Value, key: &str, cmd: &str) -> Result<Option<Option<f64>>> {
+    match p.get(key) {
+        None => Ok(None),
+        Some(Value::Null) => Ok(Some(None)),
+        Some(v) => v.as_f64().map(|x| Some(Some(x))).ok_or_else(|| bad(cmd, format!("`{key}` must be a number or null, not {v}"))),
+    }
+}
+
 /// Regenerate graph `id` from `spec` (adjusting the plot rect for any move/scale since the last
 /// generation, which is recorded in the spec's `placed` bounds).
 pub(super) fn regenerate(d: &mut Document, id: NodeId, mut spec: GraphSpec) -> Result<()> {
@@ -33,12 +43,14 @@ pub(super) fn regenerate(d: &mut Document, id: NodeId, mut spec: GraphSpec) -> R
 
 pub(super) fn create(s: &mut Session, p: &Value) -> Result<Value> {
     const C: &str = "graph.create";
-    let kind =
-        str_param(p, "type").map(|t| GraphKind::parse(t).ok_or_else(|| bad(C, format!("unknown graph type `{t}`")))).transpose()?.unwrap_or_default();
+    let kind = text_param(p, "type", C)?
+        .map(|t| GraphKind::parse(t).ok_or_else(|| bad(C, format!("unknown graph type `{t}`"))))
+        .transpose()?
+        .unwrap_or_default();
     let (x, y) = (f64_req(p, "x", C)?, f64_req(p, "y", C)?);
-    let (w, h) = (f64_or(p, "width", 200.0).abs().max(1.0), f64_or(p, "height", 150.0).abs().max(1.0));
+    let (w, h) = (number_or(p, "width", 200.0, C)?.abs().max(1.0), number_or(p, "height", 150.0, C)?.abs().max(1.0));
     let mut spec = GraphSpec { kind, rect: Rect::new(x, y, x + w, y + h), ..GraphSpec::default() };
-    if !apply_data(&mut spec, p) {
+    if !apply_data(&mut spec, p, C)? {
         // Starter data so the new graph shows something (edit it with Object → Graph → Data…).
         spec.series = vec!["Series 1".into(), "Series 2".into()];
         spec.categories = vec!["A".into(), "B".into(), "C".into(), "D".into()];
@@ -150,7 +162,8 @@ pub(super) fn set_type(s: &mut Session, p: &Value) -> Result<Value> {
     spec.right_series.retain(|i| *i < count);
     spec.right_series.sort_unstable();
     spec.right_series.dedup();
-    if p.get("seriesIndexes").is_some() && str_param(p, "type").is_none() && str_param(p, "valueAxis").is_none() {
+    let (new_type, new_axis) = (text_param(p, "type", C)?, text_param(p, "valueAxis", C)?);
+    if p.get("seriesIndexes").is_some() && new_type.is_none() && new_axis.is_none() {
         return Err(bad(C, "`seriesIndexes` needs a `type` or a `valueAxis`"));
     }
     let picked = picked_series(s, p, id, count, C)?;
@@ -196,7 +209,7 @@ pub(super) fn set_type(s: &mut Session, p: &Value) -> Result<Value> {
     }
     // Series picked with Group Selection stay selected through the regeneration (their groups are new nodes).
     let reselect: Vec<u32> = if p.get("seriesIndexes").is_none() { picked.iter().filter_map(|i| u32::try_from(*i).ok()).collect() } else { vec![] };
-    if let Some(t) = str_param(p, "type") {
+    if let Some(t) = new_type {
         let kind = GraphKind::parse(t).ok_or_else(|| bad(C, format!("unknown graph type `{t}`")))?;
         if picked.is_empty() {
             spec.kind = kind;
@@ -215,7 +228,7 @@ pub(super) fn set_type(s: &mut Session, p: &Value) -> Result<Value> {
             }
         }
     }
-    if let Some(v) = str_param(p, "valueAxis") {
+    if let Some(v) = new_axis {
         let side = ValueAxisSide::parse(v).ok_or_else(|| bad(C, format!("unknown value axis `{v}` (left, right or both)")))?;
         if picked.is_empty() {
             spec.value_axis = side;
@@ -231,54 +244,58 @@ pub(super) fn set_type(s: &mut Session, p: &Value) -> Result<Value> {
             }
         }
     }
-    spec.separate_scales = bool_or(p, "separateScales", spec.separate_scales);
+    spec.separate_scales = switch_or(p, "separateScales", spec.separate_scales, C)?;
     for (key, slot) in [
         ("tickLength", &mut spec.tick_length),
         ("rightTickLength", &mut spec.right_tick_length),
         ("categoryTickLength", &mut spec.category_tick_length),
     ] {
-        if let Some(v) = str_param(p, key) {
+        if let Some(v) = text_param(p, key, C)? {
             *slot = TickLength::parse(v).ok_or_else(|| bad(C, format!("unknown {key} `{v}` (none, short or full)")))?;
         }
     }
     for (key, slot) in
         [("tickMarks", &mut spec.tick_marks), ("rightTickMarks", &mut spec.right_tick_marks), ("categoryTickMarks", &mut spec.category_tick_marks)]
     {
-        *slot = count_param(p, key, MAX_TICK_MARKS as u64).map_or(*slot, |n| n.max(1));
+        *slot = count_given(p, key, MAX_TICK_MARKS as u64, C)?.map_or(*slot, |n| n.max(1));
     }
-    spec.ticks_between_labels = bool_or(p, "ticksBetweenLabels", spec.ticks_between_labels);
-    if let Some(v) = str_param(p, "piePosition") {
+    spec.ticks_between_labels = switch_or(p, "ticksBetweenLabels", spec.ticks_between_labels, C)?;
+    if let Some(v) = text_param(p, "piePosition", C)? {
         spec.pie_position =
             vectorcraft_doc::PiePosition::parse(v).ok_or_else(|| bad(C, format!("unknown piePosition `{v}` (even, ratio or stacked)")))?;
     }
-    if let Some(v) = str_param(p, "pieSort") {
+    if let Some(v) = text_param(p, "pieSort", C)? {
         spec.pie_sort = vectorcraft_doc::PieSort::parse(v).ok_or_else(|| bad(C, format!("unknown pieSort `{v}` (none, all or first)")))?;
     }
     for (key, slot) in
         [("prefix", &mut spec.prefix), ("suffix", &mut spec.suffix), ("rightPrefix", &mut spec.right_prefix), ("rightSuffix", &mut spec.right_suffix)]
     {
-        if let Some(v) = str_param(p, key) {
+        if let Some(v) = text_param(p, key, C)? {
             // A label, not a paragraph: one line, at most MAX_AFFIX characters.
             *slot = one_line(v);
         }
     }
-    spec.right_ticks = count_param(p, "rightTicks", 100).unwrap_or(spec.right_ticks);
-    if let Some(v) = p.get("rightAxisMin") {
-        spec.right_axis_min = v.as_f64();
+    spec.right_ticks = count_given(p, "rightTicks", 100, C)?.unwrap_or(spec.right_ticks);
+    for (key, slot) in [
+        ("rightAxisMin", &mut spec.right_axis_min),
+        ("rightAxisMax", &mut spec.right_axis_max),
+        ("axisMin", &mut spec.axis_min),
+        ("axisMax", &mut spec.axis_max),
+    ] {
+        if let Some(v) = bound_param(p, key, C)? {
+            *slot = v;
+        }
     }
-    if let Some(v) = p.get("rightAxisMax") {
-        spec.right_axis_max = v.as_f64();
-    }
-    spec.column_width = f64_or(p, "columnWidth", spec.column_width).clamp(1.0, 1000.0);
-    spec.cluster_width = f64_or(p, "clusterWidth", spec.cluster_width).clamp(1.0, 1000.0);
-    spec.legend = bool_or(p, "legend", spec.legend);
-    spec.drop_shadow = bool_or(p, "dropShadow", spec.drop_shadow);
-    spec.first_row_in_front = bool_or(p, "firstRowInFront", spec.first_row_in_front);
-    spec.first_column_in_front = bool_or(p, "firstColumnInFront", spec.first_column_in_front);
-    spec.filled_lines = bool_or(p, "filledLines", spec.filled_lines);
-    spec.legend_across_top = bool_or(p, "legendAcrossTop", spec.legend_across_top);
+    spec.column_width = number_or(p, "columnWidth", spec.column_width, C)?.clamp(1.0, 1000.0);
+    spec.cluster_width = number_or(p, "clusterWidth", spec.cluster_width, C)?.clamp(1.0, 1000.0);
+    spec.legend = switch_or(p, "legend", spec.legend, C)?;
+    spec.drop_shadow = switch_or(p, "dropShadow", spec.drop_shadow, C)?;
+    spec.first_row_in_front = switch_or(p, "firstRowInFront", spec.first_row_in_front, C)?;
+    spec.first_column_in_front = switch_or(p, "firstColumnInFront", spec.first_column_in_front, C)?;
+    spec.filled_lines = switch_or(p, "filledLines", spec.filled_lines, C)?;
+    spec.legend_across_top = switch_or(p, "legendAcrossTop", spec.legend_across_top, C)?;
     // After `legend`, which the Graph Type dialog sends too.
-    if let Some(v) = str_param(p, "pieLegend") {
+    if let Some(v) = text_param(p, "pieLegend", C)? {
         (spec.legend, spec.pie_legend_in_wedges) = match v.to_ascii_lowercase().as_str() {
             "none" => (false, false),
             "standard" => (true, false),
@@ -286,16 +303,10 @@ pub(super) fn set_type(s: &mut Session, p: &Value) -> Result<Value> {
             _ => return Err(bad(C, format!("unknown pieLegend `{v}` (none, standard or wedges)"))),
         };
     }
-    spec.mark_points = bool_or(p, "markPoints", spec.mark_points);
-    spec.connect_points = bool_or(p, "connectPoints", spec.connect_points);
-    spec.edge_to_edge = bool_or(p, "edgeToEdge", spec.edge_to_edge);
-    spec.ticks = count_param(p, "ticks", 100).unwrap_or(spec.ticks);
-    if let Some(v) = p.get("axisMin") {
-        spec.axis_min = v.as_f64();
-    }
-    if let Some(v) = p.get("axisMax") {
-        spec.axis_max = v.as_f64();
-    }
+    spec.mark_points = switch_or(p, "markPoints", spec.mark_points, C)?;
+    spec.connect_points = switch_or(p, "connectPoints", spec.connect_points, C)?;
+    spec.edge_to_edge = switch_or(p, "edgeToEdge", spec.edge_to_edge, C)?;
+    spec.ticks = count_given(p, "ticks", 100, C)?.unwrap_or(spec.ticks);
     let label = format!("{} Graph", spec.kind.label());
     s.edit("Graph Type", |d, sel| {
         regenerate(d, id, spec)?;

crates/engine/src/cmd/graph/tests.rs

@@ -2638,9 +2638,12 @@ fn counts_come_back_from_the_dialog_as_floats_and_are_capped() {
     s.execute("graph.setType", &json!({"tickMarks": 3.0, "ticks": 4.0, "categoryTickMarks": 2.6})).unwrap();
     let g = spec(&s, id);
     assert_eq!((g.tick_marks, g.ticks, g.category_tick_marks), (3, 4, 3));
-    s.execute("graph.setType", &json!({"categoryTickMarks": 0, "rightTickMarks": 1e300, "rightTicks": -2.0})).unwrap();
+    s.execute("graph.setType", &json!({"categoryTickMarks": 0, "rightTickMarks": 1e300})).unwrap();
     let g = spec(&s, id);
     assert_eq!((g.category_tick_marks, g.right_tick_marks, g.right_ticks), (1, 20, 0));
+    // A negative count is refused.
+    s.execute("graph.setType", &json!({"rightTicks": -2.0})).unwrap_err();
+    assert_eq!(spec(&s, id).right_ticks, 0);
     // A file's values come back as drawn.
     let mut fields = s.execute("graph.setType", &json!({})).unwrap();
     assert_eq!(fields["rightTickMarks"], 20);
@@ -4498,3 +4501,93 @@ fn a_legend_label_with_kept_negative_space_before_stays_below_the_entry_above_it
     let (first, next) = (bounds("2024"), bounds("20\n25"));
     assert!(next.y0 >= first.y1, "the second legend label starts below the first one: {first:?} {next:?}");
 }
+
+/// A graph command given a value of the wrong type refuses it, naming the key, and leaves the graph and the undo
+/// history as they were; a null axis bound still goes back to the calculated value.
+#[test]
+fn graph_commands_refuse_params_of_the_wrong_type() {
+    let mut s = Session::new();
+    s.execute("file.new", &json!({})).unwrap();
+    let id = graph(&mut s, "line");
+    s.execute("graph.setType", &json!({"id": id.0, "axisMin": 0, "axisMax": 10})).unwrap();
+    let spec = |s: &Session| s.doc().unwrap().doc.node(id).unwrap().extras().graph.as_deref().cloned().unwrap();
+    let undo_len = |s: &Session| s.doc().unwrap().history.undo.len();
+    let (before, steps) = (spec(&s), undo_len(&s));
+    let refused = |s: &mut Session, cmd: &str, p: serde_json::Value, msg: &str| {
+        let e = s.execute(cmd, &p).unwrap_err().to_string();
+        assert!(e.contains(msg), "{cmd} {p}: {e}");
+    };
+    for (key, v, msg) in [
+        ("axisMin", json!("10"), "`axisMin` must be a number or null, not \"10\""),
+        ("axisMax", json!(true), "`axisMax` must be a number or null"),
+        ("rightAxisMin", json!([1]), "`rightAxisMin` must be a number or null"),
+        ("rightAxisMax", json!({}), "`rightAxisMax` must be a number or null"),
+        ("columnWidth", json!("50"), "`columnWidth` must be a number, not \"50\""),
+        ("clusterWidth", json!(false), "`clusterWidth` must be a number"),
+        ("ticks", json!("5"), "`ticks` must be a number, not \"5\""),
+        ("ticks", json!(-2), "`ticks` must be 0 or more"),
+        ("rightTicks", json!("5"), "`rightTicks` must be a number"),
+        ("tickMarks", json!("2"), "`tickMarks` must be a number"),
+        ("rightTickMarks", json!(-1), "`rightTickMarks` must be 0 or more"),
+        ("categoryTickMarks", json!(true), "`categoryTickMarks` must be a number"),
+        ("legend", json!("false"), "`legend` must be true or false"),
+        ("markPoints", json!(0), "`markPoints` must be true or false"),
+        ("connectPoints", json!("no"), "`connectPoints` must be true or false"),
+        ("edgeToEdge", json!(1), "`edgeToEdge` must be true or false"),
+        ("separateScales", json!("true"), "`separateScales` must be true or false"),
+        ("ticksBetweenLabels", json!(1), "`ticksBetweenLabels` must be true or false"),
+        ("firstRowInFront", json!("yes"), "`firstRowInFront` must be true or false"),
+        ("firstColumnInFront", json!(0), "`firstColumnInFront` must be true or false"),
+        ("filledLines", json!("on"), "`filledLines` must be true or false"),
+        ("dropShadow", json!(1), "`dropShadow` must be true or false"),
+        ("legendAcrossTop", json!("x"), "`legendAcrossTop` must be true or false"),
+        ("type", json!(5), "`type` must be a string"),
+        ("valueAxis", json!(true), "`valueAxis` must be a string"),
+        ("tickLength", json!(1), "`tickLength` must be a string"),
+        ("rightTickLength", json!(1), "`rightTickLength` must be a string"),
+        ("categoryTickLength", json!(1), "`categoryTickLength` must be a string"),
+        ("prefix", json!(1), "`prefix` must be a string"),
+        ("suffix", json!(true), "`suffix` must be a string"),
+        ("rightPrefix", json!([]), "`rightPrefix` must be a string"),
+        ("rightSuffix", json!(2), "`rightSuffix` must be a string"),
+        ("pieLegend", json!(1), "`pieLegend` must be a string"),
+        ("piePosition", json!(1), "`piePosition` must be a string"),
+        ("pieSort", json!(1), "`pieSort` must be a string"),
+    ] {
+        refused(&mut s, "graph.setType", json!({"id": id.0, key: v}), msg);
+    }
+    for (key, v, msg) in [
+        ("csv", json!(5), "`csv` must be a string"),
+        ("series", json!("a"), "`series` must be a list, not \"a\""),
+        ("categories", json!({}), "`categories` must be a list"),
+        ("rows", json!("1,2"), "`rows` must be a list"),
+        ("rows", json!([1, 2]), "`rows` must be lists of numbers or nulls, not 1"),
+        ("rows", json!([[1, "2"]]), "`rows` must be lists of numbers or nulls"),
+        ("transpose", json!("true"), "`transpose` must be true or false"),
+        ("switchXY", json!(1), "`switchXY` must be true or false"),
+    ] {
+        refused(&mut s, "graph.setData", json!({"id": id.0, key: v}), msg);
+    }
+    assert_eq!(spec(&s), before);
+    assert_eq!(undo_len(&s), steps);
+    let count = |s: &Session| s.doc().unwrap().doc.node_count();
+    let objects = count(&s);
+    for (key, v, msg) in [
+        ("type", json!(1), "`type` must be a string"),
+        ("width", json!("300"), "`width` must be a number, not \"300\""),
+        ("height", json!(true), "`height` must be a number"),
+        ("csv", json!(1), "`csv` must be a string"),
+        ("rows", json!([[true]]), "`rows` must be lists of numbers or nulls"),
+    ] {
+        let mut p = json!({"type": "column", "x": 0, "y": 0, "width": 100, "height": 100});
+        p[key] = v;
+        refused(&mut s, "graph.create", p, msg);
+    }
+    assert_eq!((count(&s), undo_len(&s)), (objects, steps));
+    // A null is a key left out, except for an axis bound, where it goes back to the calculated value.
+    s.execute("graph.setType", &json!({"id": id.0, "legend": null, "ticks": null, "prefix": null, "axisMin": null})).unwrap();
+    assert_eq!(spec(&s).axis_min, None);
+    assert_eq!(spec(&s).axis_max, Some(10.0));
+    s.execute("graph.setData", &json!({"id": id.0, "rows": [[1, null], [2, 3]], "series": null, "transpose": null})).unwrap();
+    assert_eq!(spec(&s).cells(), vec![vec![Some(1.0), None], vec![Some(2.0), Some(3.0)]]);
+}

docs/mcp.md

@@ -1353,6 +1353,20 @@ message, as in `` `steps` must be a number, not "8" ``, and change nothing: `ste
 `highlight` in `object.mesh.create`; and `opacity` in `object.mesh.setPointColor`. A null is read as a key left out.
 `object.mesh.create` refuses an `at` that is not two numbers with `` `at` must be [x, y] ``.
 
+The graph commands refuse a value of the wrong type, naming the key, and leave the graph and the undo history as they
+were. `graph.setType` refuses `` `columnWidth` must be a number, not "50" `` for `columnWidth` and `clusterWidth`;
+`` `legend` must be true or false `` for `legend`, `markPoints`, `connectPoints`, `edgeToEdge`, `separateScales`,
+`ticksBetweenLabels`, `firstRowInFront`, `firstColumnInFront`, `filledLines`, `dropShadow` and `legendAcrossTop`;
+`` `type` must be a string `` for `type`, `valueAxis`, the three tick lengths, `prefix`, `suffix`, `rightPrefix`,
+`rightSuffix`, `pieLegend`, `piePosition` and `pieSort`; and `` `ticks` must be a number, not "5" `` (or
+`` `ticks` must be 0 or more `` for a negative number) for `ticks`, `rightTicks` and the three tick mark counts.
+`axisMin`, `axisMax`, `rightAxisMin` and `rightAxisMax` take a number, or null to go back to the value calculated from
+the data; any other value is refused with `` `axisMin` must be a number or null, not "10" ``. `graph.setData` and
+`graph.create` refuse a `csv` that is not a string, `series` or `categories` that are not lists, and `rows` that are not
+lists of numbers and nulls; `graph.setData` also refuses a `transpose` or `switchXY` that is not true or false, and
+`graph.create` a `type` that is not a string or a `width` or `height` that is not a number. A null is read as a key
+left out.
+
 The transform, Align, Object Properties and Live Shape Properties commands refuse a string, a number or another value
 that is not true or false with `` `strokes` must be true or false ``, naming the key, and leave the document and its
 undo history as they were: the options `copy`, `strokes`, `corners`, `patterns`, `objects` and `typeAreas` in