diff --git a/crates/doc/src/tables.rs b/crates/doc/src/tables.rs index ce1a09f..dad7b91 100644 --- a/crates/doc/src/tables.rs +++ b/crates/doc/src/tables.rs @@ -373,6 +373,17 @@ impl DimStyle { } _ => coerced, }; + // Sizes that can't be negative (text height and tolerance scale: not zero either). + let x = coerced.as_f64(); + let out_of_range = match field { + "textHeight" | "tolScale" => x.is_some_and(|x| x <= 0.0), + "scale" | "arrowSize" | "tickSize" => x.is_some_and(|x| x < 0.0), + _ => false, + }; + if out_of_range { + rejected.push(k.clone()); + continue; + } obj.insert(field.to_string(), coerced); match serde_json::from_value::(cur.clone()) { Ok(ns) => *self = ns, diff --git a/crates/engine/src/cmd/annotate.rs b/crates/engine/src/cmd/annotate.rs index 1f6f91f..809d6e5 100644 --- a/crates/engine/src/cmd/annotate.rs +++ b/crates/engine/src/cmd/annotate.rs @@ -240,6 +240,9 @@ fn radial(s: &mut Session, p: &Value, diameter: bool) -> Result { let at = point_param(p, "at").unwrap_or(c + Vec2::from_angle(std::f64::consts::FRAC_PI_4)); (c, c + (at - c).normalized() * r) }; + if c.near(pt, 1e-12) { + return Err(bad(id, "`point` must differ from `center`")); + } let k = if diameter { dim(s, DimKind::Diameter, c + (c - pt), Vec2::ZERO, Vec2::ZERO, pt, Vec2::ZERO, "") } else { @@ -281,6 +284,9 @@ fn run_angular(s: &mut Session, p: &Value) -> Result { let a = point_req("dimangular", p, "p1")?; let b = point_req("dimangular", p, "p2")?; let at = point_req("dimangular", p, "at")?; + if v.near(a, 1e-12) || v.near(b, 1e-12) { + return Err(bad("dimangular", "`p1` and `p2` must differ from `vertex`")); + } let k = dim(s, DimKind::Angular3P, at, a, b, v, Vec2::ZERO, ""); Ok(json!({ "handle": add_dim(s, k)?.hex() })) } diff --git a/crates/engine/src/cmd/draw.rs b/crates/engine/src/cmd/draw.rs index ea5266b..a4de16e 100644 --- a/crates/engine/src/cmd/draw.rs +++ b/crates/engine/src/cmd/draw.rs @@ -99,6 +99,11 @@ fn run_line(s: &mut Session, p: &Value) -> Result { if pts.len() < 2 { return Err(bad("line", "need at least 2 points")); } + // A repeated point would make a zero-length line. + let pts = distinct_points(&pts); + if pts.len() < 2 { + return Err(bad("line", "need at least 2 different points")); + } let mut hs = Vec::new(); for w in pts.windows(2) { if let [a, b] = w { @@ -108,6 +113,7 @@ fn run_line(s: &mut Session, p: &Value) -> Result { if bool_or(p, "closed", false) && pts.len() > 2 && let (Some(a), Some(b)) = (pts.last(), pts.first()) + && !a.near(*b, 1e-12) { hs.push(s.add_entity(line(*a, *b))?.hex()); } @@ -123,29 +129,30 @@ fn vertex_value(v: &Value) -> Option { } fn run_pline(s: &mut Session, p: &Value) -> Result { - let vs: Vec = p - .get("vertices") - .or_else(|| p.get("points")) - .and_then(Value::as_array) - .map(|a| a.iter().filter_map(vertex_value).collect()) - .unwrap_or_default(); + let raw = p.get("vertices").or_else(|| p.get("points")).and_then(Value::as_array); + for v in raw.into_iter().flatten() { + size_param("pline", v, "startWidth", true)?; + size_param("pline", v, "endWidth", true)?; + } + let vs: Vec = raw.map(|a| a.iter().filter_map(vertex_value).collect()).unwrap_or_default(); if vs.len() < 2 { return Err(bad("pline", "`vertices` needs at least 2 points")); } + let width = size_param("pline", p, "width", true)?.unwrap_or(0.0); let mut k = lwpoly(vs, bool_or(p, "closed", false)); if let EntityKind::LwPolyline(pl) = &mut k { - pl.const_width = f64_or(p, "width", 0.0).max(0.0); + pl.const_width = width; } added(s.add_entity(k)?) } fn run_circle(s: &mut Session, p: &Value) -> Result { let c = if let Some(center) = point_param(p, "center") { - let r = p - .get("radius") - .and_then(Value::as_f64) - .or_else(|| p.get("diameter").and_then(Value::as_f64).map(|d| d / 2.0)) - .ok_or_else(|| bad("circle", "`radius` or `diameter` is required"))?; + // Checked before `Circle::new`, which would turn a negative radius positive. + let r = match size_param("circle", p, "radius", false)? { + Some(r) => r, + None => size_param("circle", p, "diameter", false)?.ok_or_else(|| bad("circle", "`radius` or `diameter` is required"))? / 2.0, + }; Circle::new(center, r) } else if let (Some(a), Some(b)) = (point_param(p, "p1"), point_param(p, "p2")) { match point_param(p, "p3") { @@ -155,7 +162,7 @@ fn run_circle(s: &mut Session, p: &Value) -> Result { } else { return Err(bad("circle", "give {center, radius} or {p1, p2[, p3]}")); }; - if c.radius <= 0.0 || !c.radius.is_finite() { + if !(c.radius > 1e-12 && c.radius.is_finite()) { return Err(bad("circle", "radius must be positive")); } added(s.add_entity(circle(c.center, c.radius))?) @@ -168,6 +175,11 @@ fn run_arc(s: &mut Session, p: &Value) -> Result { Arc::from_start_center_end(st, c, e) } else if let Some(c) = point_param(p, "center") { let r = f64_req("arc", p, "radius")?; + // Checked before `Arc::new`, which would turn a negative radius positive (zero is + // refused below). + if r < 0.0 { + return Err(bad("arc", "`radius` must be positive")); + } Arc::new(c, r, f64_req("arc", p, "start")?.to_radians(), f64_req("arc", p, "end")?.to_radians()) } else { return Err(bad("arc", "give {p1,p2,p3}, {start,center,end} or {center,radius,start,end}")); @@ -179,16 +191,23 @@ fn run_arc(s: &mut Session, p: &Value) -> Result { } fn run_polygon(s: &mut Session, p: &Value) -> Result { - let n = p.get("sides").and_then(Value::as_u64).unwrap_or(4) as usize; + // A `sides` that is not a whole number (e.g. -3) is refused, not replaced by the default 4. + let n = match p.get("sides") { + None | Some(Value::Null) => 4, + Some(v) => v.as_u64().and_then(|n| usize::try_from(n).ok()).unwrap_or(0), + }; if !(3..=1024).contains(&n) { return Err(bad("polygon", "sides must be 3..1024")); } let vs = if let Some(e) = points_param(p, "edge") { let (Some(a), Some(b)) = (e.first(), e.get(1)) else { return Err(bad("polygon", "edge needs 2 points")) }; + if a.near(*b, 1e-12) { + return Err(bad("polygon", "edge endpoints must differ")); + } polygon_from_edge(*a, *b, n) } else { let c = point_req("polygon", p, "center")?; - let r = f64_req("polygon", p, "radius")?; + let r = size_param("polygon", p, "radius", false)?.ok_or_else(|| bad("polygon", "`radius` (number) is required"))?; let inscribed = bool_or(p, "inscribed", true); // Without an angle, the bottom edge is horizontal, as with a typed radius. let rp = match p.get("angle").and_then(Value::as_f64).filter(|a| a.is_finite()) { @@ -236,8 +255,8 @@ fn run_ray(s: &mut Session, p: &Value) -> Result { fn run_spline(s: &mut Session, p: &Value) -> Result { let sp = if let Some(f) = points_param(p, "fit") { - if f.len() < 2 { - return Err(bad("spline", "need 2+ fit points")); + if distinct_points(&f).len() < 2 { + return Err(bad("spline", "need 2+ different fit points")); } Spline::from_fit_points(&f) } else if let Some(c) = points_param(p, "control") { @@ -265,7 +284,12 @@ fn donut(center: Vec2, inside: f64, outside: f64) -> EntityKind { fn run_donut(s: &mut Session, p: &Value) -> Result { let c = point_req("donut", p, "center")?; - added(s.add_entity(donut(c, f64_or(p, "inside", 0.5), f64_or(p, "outside", 1.0)))?) + let inside = size_param("donut", p, "inside", true)?.unwrap_or(0.5); + let outside = size_param("donut", p, "outside", false)?.unwrap_or(1.0); + if inside > outside { + return Err(bad("donut", "`inside` must not be larger than `outside`")); + } + added(s.add_entity(donut(c, inside, outside))?) } /// Justification code → (halign, valign). diff --git a/crates/engine/src/cmd/draw2.rs b/crates/engine/src/cmd/draw2.rs index 96bec28..9f062ba 100644 --- a/crates/engine/src/cmd/draw2.rs +++ b/crates/engine/src/cmd/draw2.rs @@ -1560,7 +1560,11 @@ pub(crate) fn run_rectang2(s: &mut Session, p: &Value) -> Result { .map(|c| (c.first().and_then(Value::as_f64).unwrap_or(0.0), c.get(1).and_then(Value::as_f64).unwrap_or(0.0))) .map(|(x, y)| (if x.is_finite() { x } else { 0.0 }, if y.is_finite() { y } else { 0.0 })) .unwrap_or((0.0, 0.0)); - let fillet = f64_or(p, "fillet", 0.0); + if ch.0 < 0.0 || ch.1 < 0.0 { + return Err(bad("rectang", "`chamfer` distances must be zero or positive")); + } + let fillet = size_param("rectang", p, "fillet", true)?.unwrap_or(0.0); + let width = size_param("rectang", p, "width", true)?.unwrap_or(0.0); let (w, h) = if let Some(d) = p.get("dimensions").and_then(Value::as_array) { let l = d.first().and_then(Value::as_f64).filter(|v| v.is_finite()).ok_or_else(|| bad("rectang", "dimensions: [length, width]"))?; let wd = d.get(1).and_then(Value::as_f64).filter(|v| v.is_finite()).ok_or_else(|| bad("rectang", "dimensions: [length, width]"))?; @@ -1589,7 +1593,7 @@ pub(crate) fn run_rectang2(s: &mut Session, p: &Value) -> Result { if w.abs() < 1e-12 || h.abs() < 1e-12 || !w.is_finite() || !h.is_finite() { return Err(bad("rectang", "rectangle has no area")); } - let k = rect_kind(a, w, h, rot, fillet, ch, f64_or(p, "width", 0.0), f64_or(p, "elevation", 0.0)); + let k = rect_kind(a, w, h, rot, fillet, ch, width, f64_or(p, "elevation", 0.0)); let hd = s.add_entity(k)?; let th = f64_or(p, "thickness", 0.0); if th != 0.0 { diff --git a/crates/engine/src/cmd/hatch.rs b/crates/engine/src/cmd/hatch.rs index 8c6f538..b016efb 100644 --- a/crates/engine/src/cmd/hatch.rs +++ b/crates/engine/src/cmd/hatch.rs @@ -206,6 +206,8 @@ impl HatchSettings { } fn make(s: &mut Session, p: &Value, gradient: bool) -> Result { + // A zero or negative pattern scale would be clamped to a near-zero spacing. + size_param(if gradient { "gradient" } else { "hatch" }, p, "scale", false)?; let st = HatchSettings::from(s, p, gradient); let mut loops = Vec::new(); if let Some(pts) = points_param(p, "points") { diff --git a/crates/engine/src/cmd/mod.rs b/crates/engine/src/cmd/mod.rs index 80953b9..174d5c6 100644 --- a/crates/engine/src/cmd/mod.rs +++ b/crates/engine/src/cmd/mod.rs @@ -198,6 +198,29 @@ pub(crate) fn f64_or(p: &Value, key: &str, default: f64) -> f64 { pub(crate) fn f64_req(cmd: &str, p: &Value, key: &str) -> Result { p.get(key).and_then(Value::as_f64).filter(|v| v.is_finite()).ok_or_else(|| bad(cmd, format!("`{key}` (number) is required"))) } +/// An optional size: `None` when absent or null, else a finite number above zero (or zero too +/// with `zero_ok`). Anything else is refused, not clamped or made absolute, so a bad value never +/// makes a degenerate object. +pub(crate) fn size_param(cmd: &str, p: &Value, key: &str, zero_ok: bool) -> Result> { + match p.get(key) { + None | Some(Value::Null) => Ok(None), + Some(v) => match v.as_f64().filter(|x| x.is_finite()) { + Some(x) if x > 0.0 || (zero_ok && x == 0.0) => Ok(Some(x)), + _ => Err(bad(cmd, format!("`{key}` must be {}", if zero_ok { "zero or positive" } else { "positive" }))), + }, + } +} +/// `pts` without consecutive repeats (closer than `1e-12`), as the interactive LINE ignores a +/// point picked twice. +pub(crate) fn distinct_points(pts: &[Vec2]) -> Vec { + let mut out: Vec = Vec::with_capacity(pts.len()); + for p in pts { + if !out.last().is_some_and(|l| l.near(*p, 1e-12)) { + out.push(*p); + } + } + out +} pub(crate) fn bool_or(p: &Value, key: &str, default: bool) -> bool { p.get(key).and_then(Value::as_bool).unwrap_or(default) } diff --git a/crates/engine/src/cmd/props.rs b/crates/engine/src/cmd/props.rs index 0cfb9da..ea8dd56 100644 --- a/crates/engine/src/cmd/props.rs +++ b/crates/engine/src/cmd/props.rs @@ -854,8 +854,15 @@ fn run_mleaderstyle(s: &mut Session, p: &Value) -> Result { } fn run_ptype(s: &mut Session, p: &Value) -> Result { + // PDMODE is a shape 0..=4 plus 32 (circle) and/or 64 (square). + let mode = match p.get("pdmode") { + None | Some(Value::Null) => None, + Some(v) => Some( + v.as_i64().filter(|m| (0..=100).contains(m) && m & !96 <= 4).ok_or_else(|| bad("ddptype", "`pdmode` must be 0..4, plus 32 and/or 64"))?, + ), + }; let d = s.doc_mut()?; - if let Some(m) = p.get("pdmode").and_then(Value::as_i64) { + if let Some(m) = mode { d.header.set_i64("PDMODE", m); } if let Some(z) = p.get("pdsize").and_then(Value::as_f64) { diff --git a/crates/engine/src/cmd/table.rs b/crates/engine/src/cmd/table.rs index 29d4388..ae8bb3f 100644 --- a/crates/engine/src/cmd/table.rs +++ b/crates/engine/src/cmd/table.rs @@ -118,11 +118,12 @@ fn run_table(s: &mut Session, p: &Value) -> Result { if rows > MAX_ROWS || cols > MAX_COLS || cols == 0 { return Err(bad("table", format!("rows must be ≤ {MAX_ROWS} and columns 1..={MAX_COLS}"))); } - let pos = |k: &str| p.get(k).and_then(Value::as_f64).filter(|v| v.is_finite() && *v > 0.0); + let row_height = size_param("table", p, "rowHeight", false)?; + let col_width = size_param("table", p, "colWidth", false)?; let header: Option> = p.get("header").and_then(Value::as_array).map(|a| a.iter().map(|v| v.as_str().unwrap_or("").to_string()).collect()); let st = style_of(s); - let t = make_table(&st, at, rows, cols, pos("rowHeight"), pos("colWidth"), str_param(p, "title"), header, &cells); + let t = make_table(&st, at, rows, cols, row_height, col_width, str_param(p, "title"), header, &cells); let h = s.add_entity(EntityKind::Table(t))?; Ok(json!({ "handle": h.hex() })) } diff --git a/crates/engine/tests/json_param_validation.rs b/crates/engine/tests/json_param_validation.rs new file mode 100644 index 0000000..4f3b07e --- /dev/null +++ b/crates/engine/tests/json_param_validation.rs @@ -0,0 +1,111 @@ +//! The JSON forms of the draw and annotation commands refuse sizes and points that would make a +//! degenerate or invalid object (a negative radius made positive, a zero hatch scale, a +//! zero-length line …) instead of adding it (#343). + +use cadcraft_engine::Session; +use cadcraft_engine::doc::EntityKind; +use serde_json::{Value, json}; + +fn count(s: &Session) -> usize { + s.doc().map(|d| d.model.iter().count()).unwrap_or(0) +} + +/// A session with a 10×10 square (for HATCH's internal point) and a circle (for DIMRADIUS). +fn session() -> Session { + let mut s = Session::new(); + s.execute("rectang", &json!({"p1": [0, 0], "p2": [10, 10]})).unwrap(); + s +} + +#[test] +fn invalid_sizes_and_points_are_refused() { + let cases: &[(&str, Value, &str)] = &[ + ("circle", json!({"center": [0, 0], "radius": -5}), "`radius` must be positive"), + ("circle", json!({"center": [0, 0], "radius": 1e-300}), "radius must be positive"), + ("circle", json!({"center": [0, 0], "diameter": -2}), "`diameter` must be positive"), + ("arc", json!({"center": [0, 0], "radius": -5, "start": 0, "end": 90}), "`radius` must be positive"), + ("polygon", json!({"sides": -3, "center": [0, 0], "radius": 5}), "sides must be 3..1024"), + ("polygon", json!({"sides": 6, "center": [0, 0], "radius": -5}), "`radius` must be positive"), + ("polygon", json!({"sides": 6, "center": [0, 0], "radius": 0}), "`radius` must be positive"), + ("polygon", json!({"sides": 6, "edge": [[1, 1], [1, 1]]}), "edge endpoints must differ"), + ("donut", json!({"center": [0, 0], "inside": -1, "outside": 2}), "`inside` must be zero or positive"), + ("donut", json!({"center": [0, 0], "inside": 0, "outside": 0}), "`outside` must be positive"), + ("donut", json!({"center": [0, 0], "inside": 5, "outside": 2}), "must not be larger"), + ("rectang", json!({"p1": [0, 0], "p2": [10, 5], "fillet": -1}), "`fillet` must be zero or positive"), + ("rectang", json!({"p1": [0, 0], "p2": [10, 5], "chamfer": [-1, 1]}), "`chamfer` distances"), + ("rectang", json!({"p1": [0, 0], "p2": [10, 5], "width": -1}), "`width` must be zero or positive"), + ("pline", json!({"vertices": [[0, 0], [1, 0]], "width": -1}), "`width` must be zero or positive"), + ("pline", json!({"vertices": [{"p": [0, 0], "startWidth": -1}, [1, 0]]}), "`startWidth` must be zero or positive"), + ("line", json!({"points": [[2, 2], [2, 2]]}), "2 different points"), + ("spline", json!({"fit": [[0, 0], [0, 0], [0, 0]]}), "different fit points"), + ("hatch", json!({"points": [[1, 1]], "scale": 0}), "`scale` must be positive"), + ("hatch", json!({"points": [[1, 1]], "scale": -1}), "`scale` must be positive"), + ("table", json!({"at": [0, 0], "rows": 2, "cols": 2, "rowHeight": -1}), "`rowHeight` must be positive"), + ("table", json!({"at": [0, 0], "rows": 2, "cols": 2, "colWidth": 0}), "`colWidth` must be positive"), + ("dimradius", json!({"center": [3, 3], "point": [3, 3]}), "must differ from `center`"), + ("dimdiameter", json!({"center": [3, 3], "point": [3, 3]}), "must differ from `center`"), + ("dimangular", json!({"vertex": [0, 0], "p1": [0, 0], "p2": [5, 0], "at": [1, 1]}), "must differ from `vertex`"), + ("ddptype", json!({"pdmode": 999}), "`pdmode` must be"), + ("ddptype", json!({"pdmode": 5}), "`pdmode` must be"), + ]; + for (cmd, p, want) in cases { + let mut s = session(); + let before = count(&s); + let err = s.execute(cmd, p).expect_err(&format!("{cmd} {p} should be refused")).to_string(); + assert!(err.contains(want), "{cmd} {p}: {err}"); + assert_eq!(count(&s), before, "{cmd} {p} added an object"); + } +} + +#[test] +fn valid_values_still_work() { + let cases: &[(&str, Value)] = &[ + ("circle", json!({"center": [0, 0], "radius": 5})), + ("circle", json!({"center": [0, 0], "diameter": 4})), + ("arc", json!({"center": [0, 0], "radius": 5, "start": 0, "end": 90})), + ("polygon", json!({"center": [0, 0], "radius": 5})), + ("polygon", json!({"sides": 6, "edge": [[0, 0], [1, 0]]})), + ("donut", json!({"center": [0, 0], "inside": 0, "outside": 2})), + ("donut", json!({"center": [0, 0], "inside": 2, "outside": 2})), + ("rectang", json!({"p1": [0, 0], "p2": [10, 5], "fillet": 0, "width": 0, "chamfer": [0, 0]})), + ("pline", json!({"vertices": [{"p": [0, 0], "startWidth": 0, "endWidth": 1}, [1, 0]], "width": 0.5})), + ("hatch", json!({"points": [[1, 1]], "scale": 2})), + ("table", json!({"at": [20, 0], "rows": 2, "cols": 2, "rowHeight": 1, "colWidth": 3})), + ("dimradius", json!({"center": [3, 3], "point": [5, 3]})), + ]; + for (cmd, p) in cases { + let mut s = session(); + let before = count(&s); + s.execute(cmd, p).unwrap_or_else(|e| panic!("{cmd} {p}: {e}")); + assert_eq!(count(&s), before + 1, "{cmd} {p}"); + } + let mut s = Session::new(); + for m in [0, 3, 35, 66, 100] { + s.execute("ddptype", &json!({"pdmode": m, "pdsize": -5})).unwrap(); + } +} + +#[test] +fn repeated_line_points_make_no_zero_length_line() { + let mut s = Session::new(); + let r = s.execute("line", &json!({"points": [[0, 0], [0, 0], [5, 0], [5, 0], [5, 5]], "closed": true})).unwrap(); + assert_eq!(r["handles"].as_array().map(Vec::len), Some(3), "{r}"); + let d = s.doc().unwrap(); + assert!(d.model.iter().all(|e| matches!(&e.kind, EntityKind::Line(l) if l.a != l.b))); +} + +#[test] +fn dimension_style_sizes_out_of_range_are_ignored() { + let mut s = Session::new(); + let r = s.execute("dimstyle", &json!({"name": "D", "DIMTXT": -1, "DIMASZ": -1, "DIMSCALE": -2, "DIMGAP": 0.1})).unwrap(); + let ignored: Vec<&str> = r["ignored"].as_array().map(|a| a.iter().filter_map(Value::as_str).collect()).unwrap_or_default(); + for k in ["DIMTXT", "DIMASZ", "DIMSCALE"] { + assert!(ignored.contains(&k), "{r}"); + } + let st = s.doc().unwrap().dim_style("D").unwrap(); + assert!(st.text_height > 0.0 && st.arrow_size >= 0.0 && st.scale >= 0.0); + assert!((st.text_gap - 0.1).abs() < 1e-12); + // Zero arrows and DIMSCALE 0 stay valid. + let r = s.execute("dimstyle", &json!({"name": "D", "DIMASZ": 0, "DIMSCALE": 0})).unwrap(); + assert!(r.get("ignored").is_none(), "{r}"); +}