From bf2923c4bad89ed58d63c874a31a6ee63ebae636 Mon Sep 17 00:00:00 2001 From: Marco Arnulfo Date: Tue, 13 Oct 2026 03:04:37 +0200 Subject: [PATCH 1/2] check: the package round trip reports relationship kinds the save lost (part of #860) --- apps/gridcraft-cli/src/check.rs | 79 ++++++++++++++++++++++++++++++--- docs/cli.md | 3 +- 2 files changed, 75 insertions(+), 7 deletions(-) diff --git a/apps/gridcraft-cli/src/check.rs b/apps/gridcraft-cli/src/check.rs index 9fc81e3..347cf6e 100644 --- a/apps/gridcraft-cli/src/check.rs +++ b/apps/gridcraft-cli/src/check.rs @@ -8,8 +8,9 @@ //! them are skipped. //! //! **Round trip.** The recalculated workbook is written with GridCraft's XLSX writer, the package is -//! validated (content types cover every part, every internal relationship target exists) and read -//! back, and the two workbooks are compared feature by feature. +//! validated (content types cover every part, every internal relationship target exists, no kind of +//! relationship the source had is missing) and read back, and the two workbooks are compared feature +//! by feature. use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet}; use std::io::{Cursor, Read as _}; @@ -38,6 +39,9 @@ const MAX_FILES: usize = 100_000; const MAX_XML_PART: u64 = 64 * 1024 * 1024; /// Most zip entries looked at while validating a written package. const MAX_ENTRIES: usize = 200_000; +/// Relationship kinds (lower case) whose absence after a save isn't a loss: the calculation chain is +/// a cache Excel rebuilds, and GridCraft never writes it. +const REBUILT_KINDS: [&str; 1] = ["calcchain"]; #[derive(Clone, Copy, Debug)] pub struct Options { @@ -286,7 +290,7 @@ fn check_inner(path: &str, bytes: &[u8], opts: &Options) -> FileReport { calc.recalc_all(&mut fresh); let trust_cache = !full_calc_on_load(bytes); report.calc = oracle(&cached, &fresh, &calc, trust_cache, opts); - report.round_trip = round_trip(&fresh, opts); + report.round_trip = round_trip(&fresh, bytes, opts); report } @@ -514,8 +518,9 @@ pub fn oracle(cached: &Workbook, fresh: &Workbook, calc: &Calc, trust_cache: boo // ---------------------------------------------------------------- round trip -/// Writes `wb` with GridCraft's XLSX writer, validates the package, reads it back and compares. -pub fn round_trip(wb: &Workbook, opts: &Options) -> RoundTrip { +/// Writes `wb` with GridCraft's XLSX writer, validates the package against itself and against +/// `source` (the file `wb` was read from), reads it back and compares. +pub fn round_trip(wb: &Workbook, source: &[u8], opts: &Options) -> RoundTrip { let bytes = match gridcraft_engine::io::save_bytes(wb, "roundtrip.xlsx") { Ok(b) => b, Err(e) => return RoundTrip { error: Some(format!("write failed: {e}")), categories: vec![] }, @@ -525,6 +530,13 @@ pub fn round_trip(wb: &Workbook, opts: &Options) -> RoundTrip { Ok(issues) => issues.into_iter().for_each(|i| package.push(|| i)), Err(e) => package.push(|| e), } + if let (Ok(before), Ok(after)) = (relationship_kinds(source), relationship_kinds(&bytes)) { + for (key, kind) in &before { + if !after.contains_key(key) && !REBUILT_KINDS.contains(&key.as_str()) { + package.push(|| format!("lost relationship {kind}")); + } + } + } let back = match gridcraft_engine::io::open_bytes("roundtrip.xlsx", &bytes) { Ok((b, _)) => b, Err(e) => return RoundTrip { error: Some(format!("reading the written file failed: {e}")), categories: vec![package.done()] }, @@ -888,6 +900,22 @@ pub fn validate_package(bytes: &[u8]) -> Result, String> { Ok(issues) } +/// Every relationship kind (last segment of the type, the same in the transitional and strict +/// namespaces) in the package's relationship parts, keyed in lower case. +fn relationship_kinds(bytes: &[u8]) -> Result, String> { + let mut zip = zip::ZipArchive::new(Cursor::new(bytes)).map_err(|e| format!("not a zip package: {e}"))?; + let mut kinds = BTreeMap::new(); + for n in zip_names(&mut zip).into_iter().filter(|n| part_key(n).ends_with(".rels")) { + let Ok(els) = read_zip_part(&mut zip, &n).and_then(|xml| xml_elements(&xml)) else { continue }; + for (_, attrs) in els.iter().filter(|(el, _)| el == "Relationship") { + if let Some(kind) = attr(attrs, "Type").and_then(|t| t.rsplit('/').next()) { + kinds.entry(kind.to_ascii_lowercase()).or_insert_with(|| kind.to_string()); + } + } + } + Ok(kinds) +} + // ---------------------------------------------------------------- output #[derive(Default)] @@ -1277,7 +1305,8 @@ mod tests { fn round_trip_of_a_simple_book_is_clean() { let mut wb = book(&[("A1", "2", n(2.0)), ("B1", "=A1*3", n(6.0)), ("A2", "x", Value::text("x"))]); wb.sheet_mut(0).unwrap().merges.push(gridcraft_engine::core::RangeRef::parse("C1:D2").unwrap()); - let rt = round_trip(&wb, &Options::default()); + let source = gridcraft_engine::io::save_bytes(&wb, "t.xlsx").unwrap(); + let rt = round_trip(&wb, &source, &Options::default()); assert_eq!(rt.error, None); assert!(rt.is_clean(), "{rt:?}"); assert!(rt.categories.iter().any(|c| c.name == "package")); @@ -1342,6 +1371,44 @@ mod tests { assert_eq!(validate_package(&own).unwrap(), Vec::::new()); } + /// `bytes` with `extra` relationships added to the workbook's rels part and `parts` added. + fn with_workbook_rels(bytes: &[u8], extra: &str, parts: &[(&str, &str)]) -> Vec { + let mut src = zip::ZipArchive::new(Cursor::new(bytes)).unwrap(); + let mut out = zip::ZipWriter::new(Cursor::new(Vec::new())); + for i in 0..src.len() { + let mut f = src.by_index(i).unwrap(); + let name = f.name().to_string(); + let mut body = String::new(); + f.read_to_string(&mut body).unwrap(); + if name == "xl/_rels/workbook.xml.rels" { + body = body.replace("", &format!("{extra}")); + } + out.start_file(name, zip::write::SimpleFileOptions::default()).unwrap(); + out.write_all(body.as_bytes()).unwrap(); + } + for (name, body) in parts { + out.start_file(*name, zip::write::SimpleFileOptions::default()).unwrap(); + out.write_all(body.as_bytes()).unwrap(); + } + out.finish().unwrap().into_inner() + } + + #[test] + fn package_reports_relationship_kinds_the_output_lost() { + let plain = gridcraft_engine::io::save_bytes(&book(&[("A1", "1", n(1.0))]), "t.xlsx").unwrap(); + // A kind GridCraft doesn't know is dropped on save; a calculation chain is never written + // (Excel rebuilds it), so its absence isn't a loss. + let source = with_workbook_rels( + &plain, + r#""#, + &[("xl/madeUp.xml", ""), ("xl/calcChain.xml", "")], + ); + let r = check_bytes("t.xlsx", &source, &Options::default()); + let package = r.round_trip.categories.iter().find(|c| c.name == "package").unwrap(); + assert_eq!(package.examples, ["lost relationship madeUp"]); + assert!(check_bytes("t.xlsx", &plain, &Options::default()).round_trip.is_clean()); + } + #[test] fn unopenable_files_are_reported() { let r = check_bytes("broken.xlsx", b"PK\x03\x04 not really", &Options::default()); diff --git a/docs/cli.md b/docs/cli.md index b3ee318..0a1cad2 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -85,7 +85,8 @@ up by *signature*, `expected type → actual type` (e.g. `number → #SPILL!`), **Round trip.** The recalculated workbook is written with GridCraft's XLSX writer, the package is validated (every part has a content type, every content-type override and internal relationship target exists, relationship ids are -unique: what Excel would "repair") and read back. Categories compared: `package`, `sheets` (names and order), +unique: what Excel would "repair"), compared with the source (no kind of relationship it had, such as `xmlMaps`, is +missing; a calculation chain doesn't count, Excel rebuilds it) and read back. Categories compared: `package`, `sheets` (names and order), `values`, `formulas`, `numFmts` (per cell), `merges`, `names`, `tables` (name, range, columns), `condFormats` (rules per sheet), `validations`, `charts` (kinds and series counts), `comments`, `hyperlinks`, `pivots`. `#SPILL!`/`#CALC!` compare equal to `#VALUE!` and `#CIRC!` to 0, as that's what the file format stores. From 6dc9138a6426be4005af81f3e4a64018a2095b46 Mon Sep 17 00:00:00 2001 From: Marco Arnulfo Date: Tue, 13 Oct 2026 03:52:00 +0200 Subject: [PATCH 2/2] check: internal links and empty parts aren't package losses (part of #860) --- apps/gridcraft-cli/src/check.rs | 149 +++++++++++++++++++++++++++----- docs/cli.md | 3 +- 2 files changed, 131 insertions(+), 21 deletions(-) diff --git a/apps/gridcraft-cli/src/check.rs b/apps/gridcraft-cli/src/check.rs index 347cf6e..13a2e56 100644 --- a/apps/gridcraft-cli/src/check.rs +++ b/apps/gridcraft-cli/src/check.rs @@ -901,21 +901,76 @@ pub fn validate_package(bytes: &[u8]) -> Result, String> { } /// Every relationship kind (last segment of the type, the same in the transitional and strict -/// namespaces) in the package's relationship parts, keyed in lower case. +/// namespaces) in the package's relationship parts, keyed in lower case. Relationships that carry +/// nothing are left out, as a save may rightly drop them: a link to a place in the workbook +/// (`#Sheet1!A1`, written as a cell's `location`) and a target part with nothing in it. fn relationship_kinds(bytes: &[u8]) -> Result, String> { let mut zip = zip::ZipArchive::new(Cursor::new(bytes)).map_err(|e| format!("not a zip package: {e}"))?; + let names = zip_names(&mut zip); + let by_key: HashMap = names.iter().map(|n| (part_key(n), n)).collect(); let mut kinds = BTreeMap::new(); - for n in zip_names(&mut zip).into_iter().filter(|n| part_key(n).ends_with(".rels")) { - let Ok(els) = read_zip_part(&mut zip, &n).and_then(|xml| xml_elements(&xml)) else { continue }; + for n in names.iter().filter(|n| part_key(n).ends_with(".rels")) { + let Some(src_dir) = rels_source_dir(&part_key(n)) else { continue }; + let Ok(els) = read_zip_part(&mut zip, n).and_then(|xml| xml_elements(&xml)) else { continue }; for (_, attrs) in els.iter().filter(|(el, _)| el == "Relationship") { - if let Some(kind) = attr(attrs, "Type").and_then(|t| t.rsplit('/').next()) { - kinds.entry(kind.to_ascii_lowercase()).or_insert_with(|| kind.to_string()); + let Some(kind) = attr(attrs, "Type").and_then(|t| t.rsplit('/').next()) else { continue }; + let target = attr(attrs, "Target").unwrap_or(""); + if kind.eq_ignore_ascii_case("hyperlink") && target.starts_with('#') { + continue; } + let external = attr(attrs, "TargetMode").is_some_and(|m| m.eq_ignore_ascii_case("External")); + if !external + && let Some(part) = by_key.get(&resolve_target(&src_dir, target).to_ascii_lowercase()) + && !holds_something(&mut zip, part, kind) + { + continue; + } + kinds.entry(kind.to_ascii_lowercase()).or_insert_with(|| kind.to_string()); } } Ok(kinds) } +/// The directory of the part a rels part describes: `xl` for `xl/_rels/workbook.xml.rels`, the +/// empty string for the package's `_rels/.rels`. +fn rels_source_dir(key: &str) -> Option { + let (dir, _) = key.strip_suffix(".rels")?.rsplit_once('/')?; + Some(dir.strip_suffix("_rels")?.trim_end_matches('/').to_string()) +} + +/// Whether a part has content: an element under its root, or for comments (whose authors list +/// comes first) a comment, for VML a shape. Read only as far as needed; an unreadable part counts. +fn holds_something(zip: &mut Zip<'_>, name: &str, kind: &str) -> bool { + use quick_xml::events::Event; + let Ok(f) = zip.by_name(name) else { return true }; + let mut reader = quick_xml::Reader::from_reader(std::io::BufReader::new(f.take(MAX_XML_PART))); + let wanted: Option<&[u8]> = match kind.to_ascii_lowercase().as_str() { + "comments" => Some(b"comment"), + "vmldrawing" => Some(b"shape"), + _ => None, + }; + let mut buf = Vec::new(); + let mut in_root = false; + loop { + match reader.read_event_into(&mut buf) { + Ok(Event::Start(e)) | Ok(Event::Empty(e)) => { + let found = match wanted { + Some(w) => local_name(e.name().as_ref()) == w, + None => in_root, + }; + if found { + return true; + } + in_root = true; + } + Ok(Event::Eof) => return false, + Ok(_) => {} + Err(_) => return true, + } + buf.clear(); + } +} + // ---------------------------------------------------------------- output #[derive(Default)] @@ -1371,42 +1426,96 @@ mod tests { assert_eq!(validate_package(&own).unwrap(), Vec::::new()); } - /// `bytes` with `extra` relationships added to the workbook's rels part and `parts` added. - fn with_workbook_rels(bytes: &[u8], extra: &str, parts: &[(&str, &str)]) -> Vec { + const WB_RELS: &str = "xl/_rels/workbook.xml.rels"; + const SHEET_RELS: &str = "xl/worksheets/_rels/sheet1.xml.rels"; + const MADE_UP: &str = r#""#; + /// Not empty: an empty part isn't a loss. + const MADE_UP_PART: (&str, &str) = ("xl/madeUp.xml", ""); + + /// `bytes` with relationships added to rels parts (created when missing) and `parts` added. + fn with_rels(bytes: &[u8], rels: &[(&str, &str)], parts: &[(&str, &str)]) -> Vec { let mut src = zip::ZipArchive::new(Cursor::new(bytes)).unwrap(); let mut out = zip::ZipWriter::new(Cursor::new(Vec::new())); + let mut pending: Vec<(&str, &str)> = rels.to_vec(); for i in 0..src.len() { let mut f = src.by_index(i).unwrap(); let name = f.name().to_string(); let mut body = String::new(); f.read_to_string(&mut body).unwrap(); - if name == "xl/_rels/workbook.xml.rels" { - body = body.replace("", &format!("{extra}")); + if let Some(at) = pending.iter().position(|(r, _)| *r == name) { + body = body.replace("", &format!("{}", pending.remove(at).1)); } out.start_file(name, zip::write::SimpleFileOptions::default()).unwrap(); out.write_all(body.as_bytes()).unwrap(); } - for (name, body) in parts { - out.start_file(*name, zip::write::SimpleFileOptions::default()).unwrap(); + let created = pending.iter().map(|(r, extra)| { + (*r, format!(r#"{extra}"#)) + }); + for (name, body) in created.collect::>().iter().map(|(n, b)| (*n, b.as_str())).chain(parts.iter().copied()) { + out.start_file(name, zip::write::SimpleFileOptions::default()).unwrap(); out.write_all(body.as_bytes()).unwrap(); } out.finish().unwrap().into_inner() } + /// The `package` examples of checking `source`. + fn package_of(source: &[u8]) -> Vec { + let r = check_bytes("t.xlsx", source, &Options::default()); + r.round_trip.categories.iter().find(|c| c.name == "package").unwrap().examples.clone() + } + + fn plain() -> Vec { + gridcraft_engine::io::save_bytes(&book(&[("A1", "1", n(1.0))]), "t.xlsx").unwrap() + } + #[test] fn package_reports_relationship_kinds_the_output_lost() { - let plain = gridcraft_engine::io::save_bytes(&book(&[("A1", "1", n(1.0))]), "t.xlsx").unwrap(); // A kind GridCraft doesn't know is dropped on save; a calculation chain is never written // (Excel rebuilds it), so its absence isn't a loss. - let source = with_workbook_rels( - &plain, - r#""#, - &[("xl/madeUp.xml", ""), ("xl/calcChain.xml", "")], + let calc_chain = r#""#; + let source = with_rels(&plain(), &[(WB_RELS, &format!("{MADE_UP}{calc_chain}"))], &[MADE_UP_PART, ("xl/calcChain.xml", "")]); + assert_eq!(package_of(&source), ["lost relationship madeUp"]); + assert!(check_bytes("t.xlsx", &plain(), &Options::default()).round_trip.is_clean()); + } + + // In the three tests below the made-up kind stays lost, so the comparison is known to run. + + #[test] + fn an_internal_link_written_as_a_relationship_is_not_a_loss() { + // openpyxl writes `#Sheet1!B2` as an external relationship; GridCraft writes `location`. + let link = r##""##; + let source = with_rels(&plain(), &[(WB_RELS, MADE_UP), (SHEET_RELS, link)], &[MADE_UP_PART]); + assert_eq!(package_of(&source), ["lost relationship madeUp"]); + } + + #[test] + fn comments_without_a_comment_are_not_a_loss() { + let rels = r#""#; + let comments = r#"A"#; + let vml = r#""#; + let source = with_rels( + &plain(), + &[(WB_RELS, MADE_UP), (SHEET_RELS, rels)], + &[MADE_UP_PART, ("xl/comments1.xml", comments), ("xl/drawings/vmlDrawing1.vml", vml)], ); - let r = check_bytes("t.xlsx", &source, &Options::default()); - let package = r.round_trip.categories.iter().find(|c| c.name == "package").unwrap(); - assert_eq!(package.examples, ["lost relationship madeUp"]); - assert!(check_bytes("t.xlsx", &plain, &Options::default()).round_trip.is_clean()); + assert_eq!(package_of(&source), ["lost relationship madeUp"]); + } + + #[test] + fn empty_drawings_and_person_lists_are_not_a_loss() { + let drawing = r#""#; + let person = + r#""#; + let source = with_rels( + &plain(), + &[(WB_RELS, &format!("{MADE_UP}{person}")), (SHEET_RELS, drawing)], + &[ + MADE_UP_PART, + ("xl/drawings/drawing1.xml", r#""#), + ("xl/persons/person.xml", r#""#), + ], + ); + assert_eq!(package_of(&source), ["lost relationship madeUp"]); } #[test] diff --git a/docs/cli.md b/docs/cli.md index 0a1cad2..d804129 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -86,7 +86,8 @@ up by *signature*, `expected type → actual type` (e.g. `number → #SPILL!`), **Round trip.** The recalculated workbook is written with GridCraft's XLSX writer, the package is validated (every part has a content type, every content-type override and internal relationship target exists, relationship ids are unique: what Excel would "repair"), compared with the source (no kind of relationship it had, such as `xmlMaps`, is -missing; a calculation chain doesn't count, Excel rebuilds it) and read back. Categories compared: `package`, `sheets` (names and order), +missing; a calculation chain doesn't count, Excel rebuilds it, nor do a link to a place in the workbook and a +relationship to an empty part, such as a comments part without comments) and read back. Categories compared: `package`, `sheets` (names and order), `values`, `formulas`, `numFmts` (per cell), `merges`, `names`, `tables` (name, range, columns), `condFormats` (rules per sheet), `validations`, `charts` (kinds and series counts), `comments`, `hyperlinks`, `pivots`. `#SPILL!`/`#CALC!` compare equal to `#VALUE!` and `#CIRC!` to 0, as that's what the file format stores.