From 709a9952909e036b573cc34096dcaa815cbb6109 Mon Sep 17 00:00:00 2001 From: prxnvt <53838599+prxnvt@users.noreply.github.com> Date: Wed, 21 Oct 2026 15:25:10 -0400 Subject: [PATCH] Model: one damaged macro no longer stops a JSON workbook opening Workbook::macros was read straight into typed macros, so one entry this build couldn't read (a step type from a later build, an unknown onConflict value, an entry that isn't an object) failed the whole file with "not a GridCraft JSON workbook". The field is now read as raw entries into Workbook::macros_read, and macros::load reads each one with parse_macro, as for xlsx: short-form steps are accepted, the depth limit is checked before deserializing, and a bad entry is dropped with the same warning. Only the first 1,000 entries are read, as in xlsx. --- crates/engine/src/macros/mod.rs | 11 ++++++++++- crates/engine/src/macros/tests.rs | 28 ++++++++++++++++++++++++++-- crates/model/src/macros.rs | 10 ++++++++++ crates/model/src/workbook.rs | 8 +++++++- 4 files changed, 53 insertions(+), 4 deletions(-) diff --git a/crates/engine/src/macros/mod.rs b/crates/engine/src/macros/mod.rs index 155b9182f..6b2150e66 100644 --- a/crates/engine/src/macros/mod.rs +++ b/crates/engine/src/macros/mod.rs @@ -103,6 +103,15 @@ pub fn with_names(wb: &Workbook) -> crate::Result> { pub fn load(wb: &mut Workbook) -> Vec { let mut warnings = Vec::new(); let mut bad = 0usize; + // Entries of the JSON format's field, each read on its own so a damaged one drops alone. + let read = std::mem::take(&mut wb.macros_read); + bad = bad.saturating_add(read.len().saturating_sub(gridcraft_model::MAX_MACROS)); + for v in read.iter().take(gridcraft_model::MAX_MACROS) { + match parse_macro(v) { + Ok(m) => wb.macros.push(m), + Err(_) => bad += 1, + } + } // One pass takes the hidden macro names out of the workbook, grouped by macro in saved order. let mut stored: BTreeMap<(u64, String), Vec> = BTreeMap::new(); let mut kept = Vec::new(); @@ -126,7 +135,7 @@ pub fn load(wb: &mut Workbook) -> Vec { Err(_) => bad += 1, } } - // Macros that came as a field (the JSON format) go through the same checks. + // All of them go through the same limits, and a clashing name gets a suffix. let mut all = std::mem::take(&mut wb.macros); all.extend(found); for m in all { diff --git a/crates/engine/src/macros/tests.rs b/crates/engine/src/macros/tests.rs index 189bf752e..9d5b90a37 100644 --- a/crates/engine/src/macros/tests.rs +++ b/crates/engine/src/macros/tests.rs @@ -168,9 +168,10 @@ fn damaged_or_oversized_macros_are_dropped_with_a_warning() { wb.macros[0].steps = vec![Step::command("x", json!({})); MAX_MACRO_STEPS + 1]; let bytes = crate::io::save_bytes(&wb, "many.json").unwrap(); let (back, warnings) = crate::io::open_bytes("many.json", &bytes).unwrap(); - assert_eq!(back.macros.len(), MAX_MACROS); + // Only the first MAX_MACROS entries are read, as in xlsx. + assert_eq!(back.macros.len(), MAX_MACROS - 1); assert_eq!(back.macros[0].name, "M1"); - assert!(warnings[0].starts_with("5 macros"), "{warnings:?}"); + assert!(warnings[0].starts_with("6 macros"), "{warnings:?}"); // A macro too large for the hidden names is refused, so a save never fails on it. let huge = "x".repeat(crate::cmd::stash::MAX_ITEM_CHARS); @@ -254,3 +255,26 @@ fn many_hidden_macro_names_are_read_in_one_pass() { assert!(warnings.len() == 1 && warnings[0].starts_with(&format!("{items} macros")), "{warnings:?}"); assert!(took < std::time::Duration::from_secs(3), "{took:?}"); } + +#[test] +fn a_damaged_json_macro_drops_alone() { + // One macro this build can't read (a newer step type, an unknown value, the wrong shape) is + // dropped with the usual warning; the workbook and its other macros open. + let mut wb = Workbook::new(); + wb.sheet_mut(0).unwrap().set_value(gridcraft_core::CellRef::new(0, 0), gridcraft_core::Value::text("kept")); + let mut v: serde_json::Value = serde_json::from_slice(&crate::io::save_bytes(&wb, "book.json").unwrap()).unwrap(); + v["macros"] = json!([ + {"name": "Good"}, + {"name": "Newer", "steps": [{"type": "fromALaterBuild"}]}, + {"name": "Odd", "output": {"onConflict": "explode"}}, + {"name": "Short", "steps": [{"command": "cell.set", "params": {"cell": "A1", "input": "x"}}]}, + "not a macro" + ]); + let (back, warnings) = crate::io::open_bytes("book.json", v.to_string().as_bytes()).unwrap(); + let names: Vec<&str> = back.macros.iter().map(|m| m.name.as_str()).collect(); + assert_eq!(names, ["Good", "Short"]); + assert!(matches!(&back.macros[1].steps[0].kind, StepKind::Command { id, .. } if id == "cell.set")); + assert!(back.macros_read.is_empty()); + assert!(warnings.len() == 1 && warnings[0].starts_with("3 macros"), "{warnings:?}"); + assert_eq!(back.sheet(0).unwrap().value(gridcraft_core::CellRef::new(0, 0)), gridcraft_core::Value::text("kept")); +} diff --git a/crates/model/src/macros.rs b/crates/model/src/macros.rs index caee00805..39f799615 100644 --- a/crates/model/src/macros.rs +++ b/crates/model/src/macros.rs @@ -119,6 +119,16 @@ impl Macro { } } +/// Reads the `macros` field of a JSON workbook as raw entries, whatever they hold, so they can be +/// checked one at a time. +pub(crate) fn entries<'de, D: serde::Deserializer<'de>>(d: D) -> Result, D::Error> { + Ok(match serde_json::Value::deserialize(d)? { + serde_json::Value::Array(list) => list, + serde_json::Value::Null => vec![], + other => vec![other], + }) +} + /// A range the macro works on, with the shape and content it expects. #[derive(Clone, Debug, PartialEq, Serialize, Deserialize)] #[serde(default, rename_all = "camelCase")] diff --git a/crates/model/src/workbook.rs b/crates/model/src/workbook.rs index b90186c40..4ac73a1ae 100644 --- a/crates/model/src/workbook.rs +++ b/crates/model/src/workbook.rs @@ -115,8 +115,13 @@ pub struct Workbook { pub csv_delimiter: Option, /// GridCraft macros (typed command lists, not VBA), in the order the macro list shows them. /// The JSON format keeps them as this field; xlsx keeps them in hidden defined names. - #[serde(default, skip_serializing_if = "Vec::is_empty")] + #[serde(default, skip_serializing_if = "Vec::is_empty", skip_deserializing)] pub macros: Vec, + /// The `macros` entries of a JSON file as read, before the engine checks each one on its own + /// and moves it into `macros`, so one damaged or newer macro doesn't stop the file opening. + /// Never written. + #[serde(rename = "macros", default, skip_serializing, deserialize_with = "crate::macros::entries")] + pub macros_read: Vec, /// Unmodelled file content kept for saving (package root and workbook level). Not in JSON. #[serde(skip)] pub preserved: Option>, @@ -151,6 +156,7 @@ impl Workbook { vba_project: None, csv_delimiter: None, macros: vec![], + macros_read: vec![], preserved: None, source_dir: None, }