From 180928c001c121a6b66ac6476b5bafb6e3895533 Mon Sep 17 00:00:00 2001 From: CouchRiv <270190108+CouchRiv@users.noreply.github.com> Date: Mon, 12 Oct 2026 23:05:09 -0400 Subject: [PATCH 1/2] Data validation: a list typed without quotes is saved as a quoted list data.validation {type: "list", formula1: "Open,In progress,Closed"}, which is how Excel's Source box takes a list, worked inside GridCraft but was saved as Open,In progress,Closed. In a file an inline list is a string literal, "Open,In progress,Closed" (what Excel writes and what our own reader expects); unquoted, formula1 reads as a formula of names. - validation quotes such a source when the rule is created, doubling inner quotes, using the test list_items already used to tell a literal list from a reference or a name (now is_literal_list). A source typed with = (a formula), a reference, a name, a sheet-qualified range and an already quoted list are stored as before. - list_items reads a quoted list as a string literal: "" inside stands for one quote, so an item with a quote reads back the same after the change and from a file. Creating the rule is the one place that still knows whether the user typed =, so the XLSX writer is left alone: it can't tell OFFSET(...) entered as a formula from a literal list, and a rule read from a file is already quoted. Co-Authored-By: Claude Opus 5.5 --- crates/engine/src/cmd/data.rs | 94 ++++++++++++++++++++++++++++++++--- 1 file changed, 88 insertions(+), 6 deletions(-) diff --git a/crates/engine/src/cmd/data.rs b/crates/engine/src/cmd/data.rs index e4aa3baa6..f268f40e0 100644 --- a/crates/engine/src/cmd/data.rs +++ b/crates/engine/src/cmd/data.rs @@ -929,11 +929,13 @@ fn validation(s: &mut Session, p: &Json) -> Result { "custom" => ValidationKind::Custom, _ => ValidationKind::Any, }; + let f1 = str_param(p, "formula1").unwrap_or(""); + let f1 = if kind == ValidationKind::List { list_source(f1) } else { f1.trim_start_matches('=').to_string() }; let dv = Validation { ranges: ranges.clone(), kind, op: op_param(str_param(p, "operator")), - f1: str_param(p, "formula1").unwrap_or("").trim_start_matches('=').to_string(), + f1, f2: str_param(p, "formula2").map(|f| f.trim_start_matches('=').to_string()), allow_blank: bool_param(p, "ignoreBlank").unwrap_or(true), in_cell_dropdown: bool_param(p, "dropdown").unwrap_or(true), @@ -957,14 +959,36 @@ fn validation(s: &mut Session, p: &Json) -> Result { }) } +/// Whether a list rule's source is a literal list (`"a,b"`, or `a,b` typed without quotes) rather +/// than a reference or a name to evaluate. +fn is_literal_list(f: &str) -> bool { + f.starts_with('"') + || (!f.contains('!') + && gridcraft_formula::parse(f).map(|e| !matches!(e, gridcraft_formula::Expr::Ref(_) | gridcraft_formula::Expr::Name(_))).unwrap_or(true)) +} + +/// A list rule's source as a file stores it. A literal list typed without quotes, as in Excel's +/// Source box (`Open,In progress,Closed`), becomes the quoted string Excel saves +/// (`"Open,In progress,Closed"`, inner quotes doubled); unquoted, the file would read it as a +/// formula. A formula (`=…`), a reference, a name or an already quoted list is kept as before. +fn list_source(f: &str) -> String { + let t = f.trim(); + if t.is_empty() || t.starts_with('=') || t.starts_with('"') || !is_literal_list(t) { + return f.trim_start_matches('=').to_string(); + } + format!("\"{}\"", t.replace('"', "\"\"")) +} + /// List entries of a list validation (literal or from a range). pub fn list_items(wb: &Workbook, sheet: usize, dv: &Validation) -> Vec { let f = dv.f1.trim(); - if f.starts_with('"') - || (!f.contains('!') - && gridcraft_formula::parse(f).map(|e| !matches!(e, gridcraft_formula::Expr::Ref(_) | gridcraft_formula::Expr::Name(_))).unwrap_or(true)) - { - return f.trim_matches('"').split(',').map(|x| x.trim().to_string()).filter(|x| !x.is_empty()).collect(); + if is_literal_list(f) { + // A quoted list is a string literal: `""` inside stands for one `"`. + let items = match f.strip_prefix('"').and_then(|x| x.strip_suffix('"')) { + Some(body) => body.replace("\"\"", "\""), + None => f.trim_matches('"').to_string(), + }; + return items.split(',').map(|x| x.trim().to_string()).filter(|x| !x.is_empty()).collect(); } let at = dv.ranges.first().map(|r| r.start).unwrap_or_default(); match gridcraft_calc::evaluate(wb, sheet, at, f) { @@ -1381,4 +1405,62 @@ mod tests { assert_eq!(split_delimited("a,b,,c", &[','], true, '"'), ["a", "b", "c"]); assert_eq!(split_delimited("\"x,y\",z", &[','], false, '"'), ["x,y", "z"]); } + + #[test] + fn a_list_typed_without_quotes_is_saved_as_a_quoted_list() { + // In a file an inline list is a string literal (`"a,b"`); unquoted it reads as a formula. + // References, names and formulas stay unquoted, and every rule survives a save and reopen. + let mut s = Session::new(); + s.new_workbook(); + s.execute("range.setValues", json!({"range": "K1:K3", "values": [["Red"], ["Green"], ["Blue"]]})).unwrap(); + s.execute("formulas.defineName", json!({"name": "Colors", "refersTo": "=Sheet1!$K$1:$K$3"})).unwrap(); + let rules = [ + ("A1", "Open,In progress,Closed", r#""Open,In progress,Closed""#), + ("B1", r#""Open,In progress,Closed""#, r#""Open,In progress,Closed""#), + ("C1", " 1,2,3 ", r#""1,2,3""#), + ("D1", r#"Say "hi",Bye"#, r#""Say ""hi"",Bye""#), + ("E1", "=$K$1:$K$3", "$K$1:$K$3"), + ("F1", "$K$1:$K$3", "$K$1:$K$3"), + ("G1", "=Colors", "Colors"), + ("H1", "Sheet1!$K$1:$K$3", "Sheet1!$K$1:$K$3"), + ("I1", "=OFFSET($K$1,0,0,3,1)", "OFFSET($K$1,0,0,3,1)"), + ]; + for (cell, source, _) in rules { + s.execute("data.validation", json!({"range": cell, "type": "list", "formula1": source})).unwrap(); + } + let accepted = [ + ("A1", "In progress"), + ("B1", "Closed"), + ("C1", "2"), + ("D1", r#"Say "hi""#), + ("E1", "Green"), + ("F1", "Blue"), + ("G1", "Red"), + ("H1", "Green"), + ]; + let check = |wb: &Workbook| { + for (cell, input) in accepted { + let at = CellRef::parse(cell).unwrap(); + assert!(check_validation(wb, 0, at, input).is_none(), "{cell} refuses {input}"); + assert!(check_validation(wb, 0, at, "Pending").is_some(), "{cell} accepts Pending"); + } + }; + let wb = s.doc().unwrap().wb.clone(); + check(&wb); + + let sheet_xml = |wb: &Workbook| { + let bytes = gridcraft_xlsx::write_xlsx(wb).unwrap(); + let mut zip = zip::ZipArchive::new(std::io::Cursor::new(bytes)).unwrap(); + let mut xml = String::new(); + std::io::Read::read_to_string(&mut zip.by_name("xl/worksheets/sheet1.xml").unwrap(), &mut xml).unwrap(); + xml + }; + let xml = sheet_xml(&wb); + for (cell, _, saved) in rules { + assert!(xml.contains(&format!(r#"sqref="{cell}">{saved}"#)), "{cell}: {saved} missing from {xml}"); + } + let (reopened, _) = gridcraft_xlsx::read_xlsx(&gridcraft_xlsx::write_xlsx(&wb).unwrap()).unwrap(); + check(&reopened); + assert_eq!(sheet_xml(&reopened), xml, "a second save writes the same rules"); + } } From 1a6979c948ccd8c727546c1fa2e6ab234d6f1eb8 Mon Sep 17 00:00:00 2001 From: CouchRiv <270190108+CouchRiv@users.noreply.github.com> Date: Mon, 12 Oct 2026 23:32:31 -0400 Subject: [PATCH 2/2] Data validation: refuse an unknown type, operator or error style MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit data.validation took any `type`, `operator` or `errorStyle` and fell back to a default: a typo such as `type: "lst"` saved an "any value" rule (`type="none"`), which validates nothing, and an unknown operator became `between`. Each is now an error that lists the accepted values; `any` stays a type, and the operator aliases (`greaterThan`…) still work. Co-Authored-By: Claude Opus 5.5 --- crates/engine/src/cmd/data.rs | 61 +++++++++++++++++++++++++++++++---- 1 file changed, 55 insertions(+), 6 deletions(-) diff --git a/crates/engine/src/cmd/data.rs b/crates/engine/src/cmd/data.rs index f268f40e0..46cc12b1e 100644 --- a/crates/engine/src/cmd/data.rs +++ b/crates/engine/src/cmd/data.rs @@ -927,7 +927,32 @@ fn validation(s: &mut Session, p: &Json) -> Result { "time" => ValidationKind::Time, "textLength" => ValidationKind::TextLength, "custom" => ValidationKind::Custom, - _ => ValidationKind::Any, + "any" => ValidationKind::Any, + // An unknown type used to give an "any value" rule, saved as `type="none"`: no validation. + other => return Err(bad("data.validation", format!("unknown type `{other}` (any, whole, decimal, list, date, time, textLength, custom)"))), + }; + const OPERATORS: [&str; 12] = [ + "between", + "notBetween", + "equal", + "notEqual", + "greater", + "greaterThan", + "less", + "lessThan", + "greaterOrEqual", + "greaterThanOrEqual", + "lessOrEqual", + "lessThanOrEqual", + ]; + if let Some(op) = str_param(p, "operator").filter(|op| !OPERATORS.contains(op)) { + return Err(bad("data.validation", format!("unknown operator `{op}` ({})", OPERATORS.join(", ")))); + } + let error_style = match str_param(p, "errorStyle") { + None | Some("stop") => ErrorStyle::Stop, + Some("warning") => ErrorStyle::Warning, + Some("information") => ErrorStyle::Information, + Some(other) => return Err(bad("data.validation", format!("unknown errorStyle `{other}` (stop, warning, information)"))), }; let f1 = str_param(p, "formula1").unwrap_or(""); let f1 = if kind == ValidationKind::List { list_source(f1) } else { f1.trim_start_matches('=').to_string() }; @@ -944,11 +969,7 @@ fn validation(s: &mut Session, p: &Json) -> Result { show_input: true, error_title: str_param(p, "errorTitle").unwrap_or("").into(), error_message: str_param(p, "errorMessage").unwrap_or("").into(), - error_style: match str_param(p, "errorStyle") { - Some("warning") => ErrorStyle::Warning, - Some("information") => ErrorStyle::Information, - _ => ErrorStyle::Stop, - }, + error_style, show_error: true, }; edit(s, |cx| { @@ -1463,4 +1484,32 @@ mod tests { check(&reopened); assert_eq!(sheet_xml(&reopened), xml, "a second save writes the same rules"); } + + #[test] + fn a_rule_with_an_unknown_type_operator_or_error_style_is_refused() { + // A typo used to give a rule that validates nothing (`type="none"`) or the wrong check, + // with no error. + let mut s = Session::new(); + s.new_workbook(); + for params in [ + json!({"range": "A1", "type": "lst", "formula1": "\"a,b\""}), + json!({"range": "A1", "type": "whole", "operator": "greaterthan", "formula1": "1"}), + json!({"range": "A1", "type": "whole", "operator": ">", "formula1": "1"}), + json!({"range": "A1", "type": "list", "formula1": "\"a,b\"", "errorStyle": "warn"}), + ] { + let e = s.execute("data.validation", params.clone()).unwrap_err().to_string(); + assert!(e.contains("unknown"), "{params}: {e}"); + } + assert!(s.doc().unwrap().wb.active().unwrap().validations.is_empty()); + for params in [ + json!({"range": "A1", "type": "any"}), + json!({"range": "A2", "type": "whole", "operator": "greaterThan", "formula1": "1"}), + json!({"range": "A3", "type": "list", "formula1": "\"a,b\"", "errorStyle": "warning"}), + ] { + s.execute("data.validation", params).unwrap(); + } + let sh = s.doc().unwrap().wb.active().unwrap().clone(); + assert_eq!(sh.validations.len(), 3); + assert!(check_validation(&s.doc().unwrap().wb, 0, CellRef::parse("A2").unwrap(), "0").is_some()); + } }