collective
Browse / github.com/storytold/gridcraft / patch
XLSX: rows with a duplicate, out-of-order or wrong-row cell address are repaired on import (fixes #2531)
XLSX: cells with duplicate, out-of-order or wrong-row addresses are placed silently **Summary** - The worksheet reader places every cell where its `r` attribute says, with no check that the address belongs to the enclosing `row`, follows the previous cell, or has not been used already. - A later cell with a repeated address silently replaces an earlier one; an unaddressed cell after a stray address lands at an unrelated position. No import warning is produced in any of these cases. - Excel never writes such rows and treats them as corruption to repair. Proposed: drop the offending cells with an import warning, keeping what was already read. Observed on main (3099df93) with
- Status
- open
- Score
- 0.00
- Adopters
- 0 other operators
- Author
- zyzabruh (imported from pull request #2578, open)
- Base
17b9f6ff0125209fe707efc919690632ff49cb0b- Size
- +183 −5 in 5 files
- Review
- pending
- Shared
- 2026-10-28
- Id
6a3e35484f030c2f86dc5253
How it went
No other operator has reported using it yet.
Diff
raw · crates/xlsx/src/sheet_read.rs crates/xlsx/src/tests/array_export.rs crates/xlsx/src/tests/malformed.rs crates/xlsx/src/tests/richdata_tests.rs docs/file-format-parity.md
@@ -38,6 +38,11 @@ struct RowState { /// Row-membership checks served so far, for the work cap above. dynamic_checks: u64, dropped: u64,+ /// Cells dropped because their address was repeated, ran backwards or named another row,+ /// and cells a repeated row element replaced on a row it had already read.+ misplaced: u64,+ /// Row number of the last row element read: Excel writes each row once, in increasing order.+ last_row: Option<u32>, rows: u64, } @@ -109,6 +114,9 @@ pub fn read_sheet(cx: &mut Ctx<'_>, part: &str, name: &str) -> Result<Sheet, IoE if st.dropped > 0 { cx.warn(format!("sheet \"{name}\": {} cells or rows outside the 1,048,576 × 16,384 grid were dropped", st.dropped)); }+ if st.misplaced > 0 {+ cx.warn(format!("sheet \"{name}\": {} cells with a duplicate, out-of-order or wrong-row address were dropped or replaced", st.misplaced));+ } drop(bytes); let mut fit_to_page = false;@@ -412,7 +420,19 @@ fn read_row(cx: &mut Ctx<'_>, sheet: &mut Sheet, st: &mut RowState, row: &El) { st.dropped += 1; return; }- st.next_row = r + 1;+ // Excel writes each row once, in ascending order, and repairs a file that does not — but an+ // element repeating the row it just wrote is not repaired, it is merged: the later value+ // wins, and its new cells and row formatting join the row (observed in Excel 16). An element+ // whose number does not go past the last one read is therefore either that repeat or a row+ // going backwards, which Excel removes together with its cells. Neither may move the row+ // bookkeeping: a backward element must not become the mark the next element is compared+ // against, and an element with no address must not reuse a row number already read.+ let ordered = st.last_row.is_none_or(|last| r > last);+ let repeat = st.last_row == Some(r);+ if ordered {+ st.next_row = r + 1;+ st.last_row = Some(r);+ } let mut info = LineInfo::default(); if let Some(ht) = row.attr_f64("ht") { info.custom = row.flag("customHeight", false);@@ -429,7 +449,7 @@ fn read_row(cx: &mut Ctx<'_>, sheet: &mut Sheet, st: &mut RowState, row: &El) { { info.style = Some(cx.style(Some(s))); }- if info != LineInfo::default() {+ if (ordered || repeat) && info != LineInfo::default() { sheet.rows.insert(r, info); } let mut next_col = 0u32;@@ -448,6 +468,22 @@ fn read_row(cx: &mut Ctx<'_>, sheet: &mut Sheet, st: &mut RowState, row: &El) { st.dropped += 1; continue; }+ // Excel writes a row's cells in ascending column order, all in that row; a file with any+ // other shape is damaged, and Excel repairs it ("Removed Records: Cell information").+ // Placing such cells as they are written would move values, let a later duplicate shadow+ // an earlier cell and land an unaddressed cell on an unrelated column, all without a word+ // to the user. A repeat is read the same way, with its own column cursor, so only the+ // cells it names wrongly are turned away; a backward element still drops every cell.+ if (!ordered && !repeat) || pos.row != r || pos.col < next_col {+ st.misplaced += 1;+ continue;+ }+ // A repeat may name a cell the row already holds. Excel takes the later value without a+ // word, and #2531 asks that nothing change silently, so the replacement is counted with+ // the cells that are dropped.+ if repeat && sheet.cell(pos).is_some() {+ st.misplaced += 1;+ } next_col = pos.col + 1; read_cell(cx, sheet, st, c, pos); }@@ -516,7 +552,9 @@ fn read_cell(cx: &mut Ctx<'_>, sheet: &mut Sheet, st: &mut RowState, c: &El, pos let dynamic = c.attr("cm").is_some(); let formula = c.child("f").and_then(|f| read_formula(cx, st, f, pos, dynamic)); let value = if formula.is_none() && in_dynamic(st, pos) { Value::Empty } else { value };- sheet.cells.set(pos, Cell { value, formula: formula.map(Arc::new), style });+ // `set_cell` rather than `cells.set`: a scalar must clear a picture the same address held,+ // or the picture side map outlives the cell and saving writes the old picture back over it.+ sheet.set_cell(pos, Cell { value, formula: formula.map(Arc::new), style }); } fn read_formula(cx: &mut Ctx<'_>, st: &mut RowState, f: &El, pos: CellRef, dynamic: bool) -> Option<Formula> {@@ -164,7 +164,7 @@ fn scalar_formulas_constants_and_legacy_cse_do_not_require_dynamic_metadata() { #[test] fn legacy_index_match_formulas_remain_scalar_after_xlsx_roundtrip() { let source = super::minimal(- r#"<sheetData><row r="1"><c r="A1" t="inlineStr"><is><t>a</t></is></c><c r="B1"><v>0.1</v></c><c r="D1" t="inlineStr"><is><t>c</t></is></c><c r="E1" t="inlineStr"><is><t>x</t></is></c><c r="F1"><f>INDEX($B$1:$B$3,MATCH(D1,$A$1:$A$3,0))</f><v>0.7</v></c><c r="F3" t="str"><f>IF(E1="","",INDEX($B$1:$B$3,MATCH(D1,$A$1:$A$3,0)))</f><v>0.7</v></c><c r="F5"><f>VLOOKUP(D1,$A$1:$B$3,2,FALSE)</f><v>0.7</v></c><c r="F6"><f>SUM($B$1:$B$3)</f><v>1.2</v></c><c r="F7"><f>MATCH(D1,$A$1:$A$3,0)</f><v>3</v></c></row><row r="2"><c r="A2" t="inlineStr"><is><t>b</t></is></c><c r="B2"><v>0.4</v></c><c r="F2"><f>IFERROR(INDEX($B$1:$B$3,MATCH(D1,$A$1:$A$3,0)),0)</f><v>0.7</v></c></row><row r="3"><c r="A3" t="inlineStr"><is><t>c</t></is></c><c r="B3"><v>0.7</v></c></row></sheetData>"#,+ r#"<sheetData><row r="1"><c r="A1" t="inlineStr"><is><t>a</t></is></c><c r="B1"><v>0.1</v></c><c r="D1" t="inlineStr"><is><t>c</t></is></c><c r="E1" t="inlineStr"><is><t>x</t></is></c><c r="F1"><f>INDEX($B$1:$B$3,MATCH(D1,$A$1:$A$3,0))</f><v>0.7</v></c></row><row r="2"><c r="A2" t="inlineStr"><is><t>b</t></is></c><c r="B2"><v>0.4</v></c><c r="F2"><f>IFERROR(INDEX($B$1:$B$3,MATCH(D1,$A$1:$A$3,0)),0)</f><v>0.7</v></c></row><row r="3"><c r="A3" t="inlineStr"><is><t>c</t></is></c><c r="B3"><v>0.7</v></c><c r="F3" t="str"><f>IF(E1="","",INDEX($B$1:$B$3,MATCH(D1,$A$1:$A$3,0)))</f><v>0.7</v></c></row><row r="5"><c r="F5"><f>VLOOKUP(D1,$A$1:$B$3,2,FALSE)</f><v>0.7</v></c></row><row r="6"><c r="F6"><f>SUM($B$1:$B$3)</f><v>1.2</v></c></row><row r="7"><c r="F7"><f>MATCH(D1,$A$1:$A$3,0)</f><v>3</v></c></row></sheetData>"#, &[], "", "",@@ -331,6 +331,101 @@ fn ordinary_overlapping_column_ranges_keep_last_declaration() { assert_eq!(wb.styles.get(last.style.unwrap()).num_fmt.as_str(), "0.00"); } +/// A row whose cells repeat an address, run backwards or name another row, and a row element+/// that repeats a row number already read: Excel never writes such shapes, repairs the cells+/// that break the shape ("Removed Records: Cell information") and merges a repeated row element,+/// later value winning (observed in Excel 16). The reader follows it: the first value within one+/// element, the later value across elements, and one import warning counting every cell dropped+/// or replaced (#2531).+#[test]+fn misplaced_cell_addresses_are_dropped_with_one_warning() {+ use gridcraft_core::{CellRef, Value};+ let at = |a: &str| CellRef::parse(a).unwrap();+ let read = |body: &str| read_xlsx(&minimal(body, &[], "", "")).unwrap();+ let warning = |rep: &crate::ReadReport| -> Vec<String> { rep.warnings.iter().filter(|w| w.contains("address")).cloned().collect() };++ // A repeated address keeps its first value instead of taking the later one.+ let (wb, rep) = read(r#"<sheetData><row r="1"><c r="A1"><v>1</v></c><c r="B1"><v>2</v></c><c r="A1"><v>999</v></c></row></sheetData>"#);+ let s = wb.sheet(0).unwrap();+ assert_eq!(s.value(at("A1")), Value::Number(1.0));+ assert_eq!(s.value(at("B1")), Value::Number(2.0));+ assert_eq!(warning(&rep), ["sheet \"Data\": 1 cells with a duplicate, out-of-order or wrong-row address were dropped or replaced"]);++ // A cell out of column order is dropped, and the cells read before it keep their places.+ let (wb, rep) = read(r#"<sheetData><row r="1"><c r="A1"><v>1</v></c><c r="C1"><v>3</v></c><c r="B1"><v>2</v></c></row></sheetData>"#);+ let s = wb.sheet(0).unwrap();+ assert_eq!((s.value(at("A1")), s.value(at("B1")), s.value(at("C1"))), (Value::Number(1.0), Value::Empty, Value::Number(3.0)));+ assert_eq!(warning(&rep), ["sheet \"Data\": 1 cells with a duplicate, out-of-order or wrong-row address were dropped or replaced"]);++ // A cell whose address names another row is not moved to that row.+ let (wb, rep) = read(r#"<sheetData><row r="1"><c r="A1"><v>1</v></c><c r="D7"><v>7</v></c></row></sheetData>"#);+ let s = wb.sheet(0).unwrap();+ assert_eq!(s.value(at("A1")), Value::Number(1.0));+ assert!(s.cell(at("D7")).is_none(), "a cell named another row and must not be placed there");+ assert_eq!(warning(&rep), ["sheet \"Data\": 1 cells with a duplicate, out-of-order or wrong-row address were dropped or replaced"]);++ // A second row element for a row already read joins it, the way Excel opens such a file: the+ // later value wins, and the replacement is reported rather than silent (#2531).+ let (wb, rep) = read(r#"<sheetData><row r="1"><c r="A1"><v>1</v></c></row><row r="1"><c r="A1"><v>999</v></c></row></sheetData>"#);+ let s = wb.sheet(0).unwrap();+ assert_eq!(s.value(at("A1")), Value::Number(999.0));+ assert_eq!(warning(&rep), ["sheet \"Data\": 1 cells with a duplicate, out-of-order or wrong-row address were dropped or replaced"]);++ // The repeat keeps what the first element did not carry: a new cell and its row height, both+ // left in place by Excel, which opens such a file without repairing anything.+ let (wb, rep) = read(+ r#"<sheetData><row r="1"><c r="A1"><v>1</v></c><c r="B1"><v>2</v></c></row><row r="1" ht="40" customHeight="1"><c r="A1"><v>999</v></c><c r="C1"><v>77</v></c></row></sheetData>"#,+ );+ let s = wb.sheet(0).unwrap();+ assert_eq!((s.value(at("A1")), s.value(at("B1")), s.value(at("C1"))), (Value::Number(999.0), Value::Number(2.0), Value::Number(77.0)));+ assert_eq!(s.rows.get(&0).and_then(|line| line.size), Some(crate::tables::pt_to_px(40.0)));+ assert_eq!(warning(&rep), ["sheet \"Data\": 1 cells with a duplicate, out-of-order or wrong-row address were dropped or replaced"]);++ // A row element that goes back to a row number already read is dropped with its cells, and+ // its row formatting does not replace the first row's either.+ let (wb, rep) = read(+ r#"<sheetData><row r="1"><c r="A1"><v>1</v></c></row><row r="2" ht="40" customHeight="1"><c r="A2"><v>2</v></c></row><row r="1" ht="9" customHeight="1"><c r="A1"><v>999</v></c></row></sheetData>"#,+ );+ let s = wb.sheet(0).unwrap();+ assert_eq!((s.value(at("A1")), s.value(at("A2"))), (Value::Number(1.0), Value::Number(2.0)));+ assert!(!s.rows.contains_key(&0), "the row element that went back must not carry formatting either");+ assert!(s.rows.get(&1).is_some_and(|line| line.size.is_some()), "the ordinary row keeps its own height");+ assert_eq!(warning(&rep), ["sheet \"Data\": 1 cells with a duplicate, out-of-order or wrong-row address were dropped or replaced"]);++ // A row that goes back drops its cells and its formatting, and does not move the mark rows are+ // compared against: the row 3 after it is still read as the repeat of the first row 3, which+ // Excel merges (later value and its height win) while it removes only the backward row 2.+ let (wb, rep) = read(+ r#"<sheetData><row r="3"><c r="A3"><v>3</v></c></row><row r="2"><c r="A2"><v>2</v></c></row><row r="3" ht="9" customHeight="1"><c r="A3"><v>999</v></c></row></sheetData>"#,+ );+ let s = wb.sheet(0).unwrap();+ assert_eq!(s.value(at("A3")), Value::Number(999.0), "the repeated row 3 joins the first one, as Excel has it");+ assert!(s.cell(at("A2")).is_none(), "the row that went back drops its cells");+ assert_eq!(s.rows.get(&2).and_then(|line| line.size), Some(crate::tables::pt_to_px(9.0)), "and the repeat's row height applies");+ assert_eq!(warning(&rep), ["sheet \"Data\": 2 cells with a duplicate, out-of-order or wrong-row address were dropped or replaced"]);++ // Nor does a rejected row move the cursor a row without an address follows: after 3 and 2+ // it lands on row 4, not back on the row 3 that was already read.+ let (wb, rep) =+ read(r#"<sheetData><row r="3"><c r="A3"><v>3</v></c></row><row r="2"><c r="A2"><v>2</v></c></row><row><c><v>7</v></c></row></sheetData>"#);+ let s = wb.sheet(0).unwrap();+ assert_eq!((s.value(at("A3")), s.value(at("A4"))), (Value::Number(3.0), Value::Number(7.0)));+ assert_eq!(warning(&rep), ["sheet \"Data\": 1 cells with a duplicate, out-of-order or wrong-row address were dropped or replaced"]);++ // An unaddressed cell follows the last cell that was kept, not a dropped stray one.+ let (wb, rep) = read(r#"<sheetData><row r="1"><c r="A1"><v>1</v></c><c r="D7"><v>7</v></c><c><v>8</v></c></row></sheetData>"#);+ let s = wb.sheet(0).unwrap();+ assert_eq!((s.value(at("A1")), s.value(at("B1"))), (Value::Number(1.0), Value::Number(8.0)));+ assert!(s.cell(at("E1")).is_none(), "the dropped cell must not move the unaddressed one");+ assert_eq!(warning(&rep), ["sheet \"Data\": 1 cells with a duplicate, out-of-order or wrong-row address were dropped or replaced"]);++ // A row that skips columns is ordinary and stays silent.+ let (wb, rep) = read(r#"<sheetData><row r="1"><c r="A1"><v>1</v></c><c r="C1"><v>3</v></c><c r="E1"><v>5</v></c></row></sheetData>"#);+ let s = wb.sheet(0).unwrap();+ assert_eq!(s.value(at("E1")), Value::Number(5.0));+ assert!(warning(&rep).is_empty(), "{:?}", rep.warnings);+}+ /// Show Outline Symbols off (`showOutlineSymbols="0"`) is read and written back. #[test] fn outline_symbols_flag_round_trips() {@@ -270,3 +270,33 @@ fn picture_metadata_limits_and_relationship_read_errors_do_not_block_workbooks() assert!(report.warnings.iter().any(|w| w.contains("cell-picture data could not be read")), "{:?}", report.warnings); } }++/// A repeated row element that replaces a picture cell with a scalar keeps the scalar: the+/// picture side map must not outlive the cell it hung on, or saving writes the old picture back+/// over the replacement (#2531).+#[test]+fn a_repeated_row_element_replaces_a_picture_with_its_scalar() {+ let mut parts = independent_parts(false);+ replace(+ &mut parts,+ "sheets/data.xml",+ "<row r=\"1\"><c r=\"A1\" t=\"e\" vm=\"2\"><v>#VALUE!</v></c>",+ "<row r=\"1\"><c r=\"A1\" t=\"e\" vm=\"2\"><v>#VALUE!</v></c></row><row r=\"1\"><c r=\"A1\"><v>42</v></c>",+ );+ assert!(+ std::str::from_utf8(&parts.iter().find(|(n, _)| n == "sheets/data.xml").unwrap().1).unwrap().contains("<v>42</v>"),+ "the fixture must carry the second row element"+ );++ let (wb, report) = read_xlsx(&zip(&parts)).unwrap();+ let sheet = wb.sheet(0).unwrap();+ assert_eq!(sheet.value(at("A1")), Value::Number(42.0), "the replacement value wins");+ assert!(!sheet.cell_pictures.contains_key(&at("A1")), "the picture the repeat replaced must not outlive it");+ assert!(report.warnings.iter().any(|w| w.contains("dropped or replaced")), "{:?}", report.warnings);++ // Saving keeps the replacement instead of restoring the picture it replaced.+ let (back, _) = read_xlsx(&write_xlsx(&wb).unwrap()).unwrap();+ let sheet = back.sheet(0).unwrap();+ assert_eq!(sheet.value(at("A1")), Value::Number(42.0));+ assert!(!sheet.cell_pictures.contains_key(&at("A1")));+}@@ -1,6 +1,6 @@ # File-format parity -> **Last reviewed:** 2026-10-11 · **Last updated:** 2026-10-21 · **Change:** minor (.xls dialog sheets are skipped instead of read as worksheets; `.xls` import reads each sheet's display options from WINDOW2: right-to-left, gridlines, headings, zeros, formulas; XLSX text boxes and shapes keep their paragraphs' right-to-left direction on save; CSV/TSV/TXT can open with every field kept as text via `file.open {csvAsText}`; formulas over Excel's 8,192 characters, and `%` or call chains past the nesting limit, are parse errors instead of stack overflows when a file is opened; XLSX PivotTables read and write `rowGrandTotals`/`colGrandTotals` the way Excel does, so a table saved with only one grand total opens with the right one; HTML export keeps right-to-left; CSV and text import detects likely Windows-1256 Arabic when invalid UTF-8 has Arabic byte runs; other invalid UTF-8 remains Windows-1252; HTML export places pictures and charts, as inline SVG, at their anchors) · **Target:** Microsoft Excel (Microsoft 365), Excel for Mac 16.113.4+> **Last reviewed:** 2026-10-11 · **Last updated:** 2026-10-25 · **Change:** minor (XLSX worksheet rows whose cells repeat an address, run backwards or name another row are repaired on import the way Excel repairs them — "Removed Records: Cell information" — while a row element repeating a row number already read is merged the way Excel merges it (later value wins, its new cells and row formatting join the row); one warning per sheet reports how many cells were dropped or replaced, and an unaddressed cell follows the last cell kept; observed against Excel 16 with hand-built files; .xls dialog sheets are skipped instead of read as worksheets; `.xls` import reads each sheet's display options from WINDOW2: right-to-left, gridlines, headings, zeros, formulas; XLSX text boxes and shapes keep their paragraphs' right-to-left direction on save; CSV/TSV/TXT can open with every field kept as text via `file.open {csvAsText}`; formulas over Excel's 8,192 characters, and `%` or call chains past the nesting limit, are parse errors instead of stack overflows when a file is opened; XLSX PivotTables read and write `rowGrandTotals`/`colGrandTotals` the way Excel does, so a table saved with only one grand total opens with the right one; HTML export keeps right-to-left; CSV and text import detects likely Windows-1256 Arabic when invalid UTF-8 has Arabic byte runs; other invalid UTF-8 remains Windows-1252; HTML export places pictures and charts, as inline SVG, at their anchors) · **Target:** Microsoft Excel (Microsoft 365), Excel for Mac 16.113.4 Every format Excel opens or saves, with GridCraft's read and write support, fidelity and tests. Part of [target-app-parity.md](target-app-parity.md). **File formats: ~50% ready, 80–120 h**@@ -22,6 +22,20 @@ that would be skipped as defaults are excluded before charging the limit. Excess the existing file-too-large error; accepted overlaps retain their existing last-wins behavior (#1022). +A worksheet row's cells are read in the shape Excel writes them: ascending column order, all in+the row's own row, one element per row number — Excel writes exactly that (an Excel 16 save of+cells entered in a scrambled order came back `<row r="1">A1,B1` then `r="3"`, `r="5"`, `r="7"`).+Against hand-built files Excel 16 repairs a cell that repeats an address inside a row, a column+that runs backwards, a cell naming another row and a row element that goes back to a row number+already read ("Removed Records: Cell information from /xl/worksheets/sheet1.xml part"), leaving+the records GridCraft keeps: the first value read for an address, the cells before the break in+column order, and nothing on the row that went back. Placing the others as written silently moved+values, let a later duplicate shadow an earlier cell and landed an unaddressed cell on an+unrelated column. The one shape Excel does not repair is a row element repeating the row number+it just wrote: Excel merges it without a word — later value wins, its new cells and its row+formatting join the row — so GridCraft merges it the same way. One import warning per sheet+reports how many cells were dropped or replaced (#2531).+ Ink created with Draw is written as standard, unfilled DrawingML polylines, without visible point captions or private metadata. Supported numeric polylines reopen as editable ink, retaining points, colour, integer width and anchor. Curves, filled paths, transforms and unsupported line styles are@@ -110,6 +124,7 @@ refresh. Counted under features (Get & Transform, ~10% ready), not here. | Date | Change | Summary | |---|---|---|+| 2026-10-25 | minor | XLSX worksheet rows: a cell that repeats an address, runs backwards or names a row other than its `<row>`, and the cells of a row element that goes back to a row number already read, are dropped instead of being placed silently; a row element repeating the row number already read is merged instead, as Excel 16 opens it (later value wins, its new cells and row formatting join the row). One import warning per sheet names the count of cells dropped or replaced, and an unaddressed cell follows the last cell that was kept (#2531), checked against Excel 16 (`malformed.rs`) | | 2026-10-21 | minor | HTML (Save as Web Page) writes pictures (`data:` URI `<img>`) and charts (inline SVG from the chart crate's drawing primitives, text included) at their anchors; the table uses the sheet's column widths and row heights when the sheet has either, and output for sheets without pictures or charts is unchanged | | 2026-10-20 | trivial | XLSX shared strings: Japanese phonetic runs (ふりがな, `<rPh sb="…" eb="…">` with `<phoneticPr>`) are read into the workbook and written back next to the string with the same text, so an open + save no longer deletes them; bad offsets and readings are dropped with a warning, a `<phoneticPr>` without runs is kept on its own, and when duplicate `<si>` items share a text but not a reading only the first reading is written (reported on open) (`phonetic_tests.rs`) | | 2026-10-20 | minor | A workbook theme's major/minor per-script typefaces (`<a:font script="Arab" typeface="…"/>`, also Hebr, Jpan and the rest) are read and written back in file order after `<a:cs>`; they were dropped before. A hostile file is held to 64 entries of 256 characters |