diff --git a/AGENTS.md b/AGENTS.md index 5e20c133f..d1c24923e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -106,6 +106,17 @@ cmake --build cmake-build-relwithdebinfo --target translate # CLI: file → HTM need the data, and several gigabytes should not arrive because someone turned tests on. The `update_test_data` target moves existing checkouts onto the pins. The two private repositories need credentials. +- **pugixml is built in compact mode** (`PUGIXML_COMPACT`, set on the imported + target in `CMakeLists.txt`, paired with `pugixml/*:header_only` in + `conanfile.py`). The odf/ooxml/svg/xml engines keep the parsed DOM resident as + their backing store, so its size *is* the document's: a 12-byte node instead + of 64 took a 297 MB `content.xml` from 594 MB to 116 MB. The define is ABI + affecting and mixing it across translation units is silent corruption, not a + link error — hence the imported target, and no prebuilt library to mismatch + against. A new target that includes `pugixml.hpp` has to get it too, and the + installed internal headers expose pugixml types, so `odr` carries the define + INTERFACE and `package_info` declares it. `util/xml_util.cpp` asserts the + layout it compiled against. ## Releasing diff --git a/CHANGELOG.md b/CHANGELOG.md index 76061a0ae..e72af7e7d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,20 @@ The release run heads these entries with the version and opens a fresh `text/html` and `application/xhtml+xml`. Classification only: no `open`, no `translate_html`, and never detected from its bytes. +- **Breaking** An ods sheet stores a repeated cell once, at the range it + covers, rather than once per position: `SheetCell::position()` reports the + anchor of that range instead of the position the cell was looked up at, and + `Sheet::cell()` hands back the same element everywhere in it. +- An ods `table:number-columns-repeated` beside a `table:number-rows-repeated` + no longer inflates — the grid's own `1048576 × 1024` asked for three billion + elements out of four hundred bytes. +- An xlsx `` whose `ref` names more positions than the sheet has + cells opens again; `ref="A1:XFD1048576"` used to visit all 17 billion. +- Decoding a spreadsheet costs about half the memory it did — pugixml is built + in compact mode and the ods sheet index is sorted vectors rather than maps. + A consumer that includes an internal header now needs `PUGIXML_COMPACT`, and + the conan package declares it. + ## v6.11.0 - 2026-08-29 - No view declares `` any more. A link back into what diff --git a/CMakeLists.txt b/CMakeLists.txt index ab129a0d5..c822cfe34 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -54,6 +54,12 @@ endif () add_compile_options("$<$:/utf-8>") find_package(pugixml REQUIRED) +# A compact `xml_node_struct` is 12 bytes rather than 64, and the engines keep +# the dom resident as their backing store. The define is ABI affecting and a +# mismatch is silent, so it rides on the imported target and is paired with +# `header_only` in conanfile.py. See AGENTS.md. +set_property(TARGET pugixml::pugixml APPEND PROPERTY + INTERFACE_COMPILE_DEFINITIONS PUGIXML_COMPACT) find_package(md4c REQUIRED) find_package(miniz REQUIRED) find_package(cryptopp REQUIRED) @@ -311,6 +317,10 @@ target_link_libraries(odr uchardet::uchardet utf8::cpp ) +# pugixml is linked PRIVATE, but the installed internal headers expose its +# types - so a consumer of those needs the same layout. `cpp_info.defines` in +# conanfile.py is the same thing for the conan package. +target_compile_definitions(odr INTERFACE PUGIXML_COMPACT) if (ODR_WITH_HTTP_SERVER) find_package(httplib REQUIRED) diff --git a/conanfile.py b/conanfile.py index 0b70df147..04d005b63 100644 --- a/conanfile.py +++ b/conanfile.py @@ -38,6 +38,9 @@ class OpenDocumentCoreConan(ConanFile): "with_apple": False, "with_wasm": False, "bundle_assets": False, + # paired with PUGIXML_COMPACT in CMakeLists.txt: no prebuilt library + # to mismatch against the node layout the define changes + "pugixml/*:header_only": True, } exports_sources = ["apple/*", "cli/*", "cmake/*", "jni/*", "python/*", "resources/dist/*", "wasm/*", "src/*", "CMakeLists.txt"] @@ -103,3 +106,6 @@ def package(self): def package_info(self): self.cpp_info.libs = ["odr"] + # the installed internal headers expose pugixml types, whose layout + # PUGIXML_COMPACT changes + self.cpp_info.defines = ["PUGIXML_COMPACT"] diff --git a/src/odr/internal/odf/AGENTS.md b/src/odr/internal/odf/AGENTS.md index 5092e886c..bfd23b336 100644 --- a/src/odr/internal/odf/AGENTS.md +++ b/src/odr/internal/odf/AGENTS.md @@ -19,13 +19,15 @@ Unlike the binary engines (`oldms/`, `pdf/`), which parse bytes into their own structures, ODF keeps the parsed **`content.xml` / `styles.xml` DOMs resident** (`Document` owns `m_content_xml`, `m_styles_xml`). The `ElementRegistry` is a thin *index over* that DOM: every `ElementRegistry::Element` stores a live -`pugi::xml_node` alongside its tree ids. Style/content/attribute access always +`pugi::xml_node` alongside its tree ids. The DOM being the backing store, and +not a parse artifact, is why pugixml is built in compact mode — see the top +level [`AGENTS.md`](../../../../AGENTS.md). Style/content/attribute access always goes back to the node. **This is why ODF alone can edit and save**: a text edit is a local DOM splice, and `save` re-serialises the mutated tree. The other engines throw away the source, so their models are read-only. -Everything else follows the shared registry/adapter pattern: flat -`std::vector`, id = index + 1, `null_element_id == 0`, parent/child/ +Everything else follows the shared registry/adapter pattern: a flat element +store, id = index + 1, `null_element_id == 0`, parent/child/ sibling ids, per-subtype side maps (`m_texts`, `m_tables`, `m_sheets`, `m_sheet_cells`). One mega `ElementAdapter` multiply-inherits every abstract per-type adapter and dispatches by returning `this`/`nullptr` on `element_type`. @@ -46,11 +48,25 @@ element and breaks the run. Reading expands `text:s`→N spaces (via `text:c`), `text:tab`→`\t`. **Sheets are modelled sparsely, off-tree.** A `Sheet` side-struct holds -position-keyed `columns`/`rows`/`cells` maps rather than a child chain. Repeated -columns/rows/cells are stored **once** at a range key and resolved with -`lookup_greater_than`, so a 5000-row `number-columns-repeated` doesn't inflate. -Only non-empty cells get a real `sheet_cell` Element; empty ones are recorded as -repeated ranges. Cells carry a `TablePosition` + `is_repeated` flag. +`columns`/`rows`/`cells` keyed by position rather than a child chain. Repeated +columns/rows/cells are stored **once**, at the *end* of the range they repeat +over, and resolved with an upper bound — whether or not the cell has content. +That last part is load-bearing: expanding a repeat per position let a 400-byte +document ask for a `1048576 × 1024` grid of elements, both counts being legal +repeats. Only non-empty cells get a real `sheet_cell` Element; empty ones are +recorded as ranges alone. Cells carry a `TablePosition` (the anchor of the +range, not each position it covers) + `is_repeated` flag. + +The three containers are **sorted vectors, not maps**: parsing appends in +document order, so the keys only grow, and a rb-tree node costs more than the 12 +bytes it carries — on a million-row sheet, 275 MB of maps against 89 MB of +vectors. The cells of every row live in one array per sheet, each row recording +where its own run starts, so a sheet is two allocations rather than one per row. +`register_cell` therefore has to follow its row's `register_row`. + +The elements themselves are a `std::deque`: `create_element` hands back a +reference the parser holds on to, and a vector both invalidates it and peaks +holding two copies. **Styles resolve to a flattened `ResolvedStyle`, eagerly.** `StyleRegistry` first builds name→node indices from *both* files (automatic and named styles land diff --git a/src/odr/internal/odf/odf_document.cpp b/src/odr/internal/odf/odf_document.cpp index cb05d94eb..d425856b8 100644 --- a/src/odr/internal/odf/odf_document.cpp +++ b/src/odr/internal/odf/odf_document.cpp @@ -469,7 +469,7 @@ class ElementAdapter final : public abstract::ElementAdapter, TableDimensions result; TableCursor cursor; - for (const pugi::xml_node row : table_rows(node)) { + for_each_table_row(node, [&](const pugi::xml_node row) { const auto rows_repeated = row.attribute("table:number-rows-repeated").as_uint(1); cursor.add_row(rows_repeated); @@ -492,7 +492,7 @@ class ElementAdapter final : public abstract::ElementAdapter, result.columns = new_cols; } } - } + }); return result; } @@ -731,20 +731,20 @@ class ElementAdapter final : public abstract::ElementAdapter, TableDimensions result; TableCursor cursor; - for (const pugi::xml_node column : table_columns(node)) { + for_each_table_column(node, [&](const pugi::xml_node column) { const auto columns_repeated = column.attribute("table:number-columns-repeated").as_uint(1); cursor.add_column(columns_repeated); - } + }); result.columns = cursor.column(); cursor = {}; - for (const pugi::xml_node row : table_rows(node)) { + for_each_table_row(node, [&](const pugi::xml_node row) { const auto rows_repeated = row.attribute("table:number-rows-repeated").as_uint(1); cursor.add_row(rows_repeated); - } + }); result.rows = cursor.row(); diff --git a/src/odr/internal/odf/odf_element_registry.cpp b/src/odr/internal/odf/odf_element_registry.cpp index 1ea0dbcf2..2bc72be2e 100644 --- a/src/odr/internal/odf/odf_element_registry.cpp +++ b/src/odr/internal/odf/odf_element_registry.cpp @@ -1,7 +1,6 @@ #include -#include - +#include #include namespace odr::internal::odf { @@ -246,13 +245,26 @@ void ElementRegistry::check_sheet_cell_id(const ElementIdentifier id) const { void ElementRegistry::Sheet::register_column(const std::uint32_t column, const std::uint32_t repeated, const pugi::xml_node element) { - columns[column + repeated] = {.node = element}; + const std::uint32_t end = column + repeated; + if (!columns.empty() && columns.back().end >= end) { + columns.back() = {.end = end, .node = element}; + return; + } + columns.push_back({.end = end, .node = element}); } void ElementRegistry::Sheet::register_row(const std::uint32_t row, const std::uint32_t repeated, const pugi::xml_node element) { - rows[row + repeated].node = element; + const std::uint32_t end = row + repeated; + if (!rows.empty() && rows.back().end >= end) { + rows.back().end = end; + rows.back().node = element; + return; + } + rows.push_back({.end = end, + .first_cell = static_cast(cells.size()), + .node = element}); } void ElementRegistry::Sheet::register_cell(const std::uint32_t column, @@ -261,40 +273,56 @@ void ElementRegistry::Sheet::register_cell(const std::uint32_t column, const std::uint32_t rows_repeated, const pugi::xml_node element, const ElementIdentifier element_id) { - Cell &cell = rows[row + rows_repeated].cells[column + columns_repeated]; - cell.node = element; - cell.element_id = element_id; + const std::uint32_t row_end = row + rows_repeated; + if (rows.empty() || rows.back().end != row_end) { + throw std::invalid_argument( + "ElementRegistry::Sheet::register_cell: no row to hold the cell"); + } + + const std::uint32_t end = column + columns_repeated; + if (cells.size() > rows.back().first_cell && cells.back().end >= end) { + cells.back() = {.end = end, .node = element, .element_id = element_id}; + return; + } + cells.push_back({.end = end, .node = element, .element_id = element_id}); } +namespace { + +/// The entry whose range covers @p at, i.e. the first one ending past it. +template +const Entry *lookup(const std::span entries, + const std::uint32_t at) { + const auto it = std::ranges::upper_bound(entries, at, {}, &Entry::end); + return it != std::end(entries) ? &*it : nullptr; +} + +} // namespace + const ElementRegistry::Sheet::Column * ElementRegistry::Sheet::column(const std::uint32_t column) const { - if (const auto it = util::map::lookup_greater_than(columns, column); - it != std::end(columns)) { - return &it->second; - } - return nullptr; + return lookup(columns, column); } const ElementRegistry::Sheet::Row * ElementRegistry::Sheet::row(const std::uint32_t row) const { - if (const auto it = util::map::lookup_greater_than(rows, row); - it != std::end(rows)) { - return &it->second; - } - return nullptr; + return lookup(rows, row); } const ElementRegistry::Sheet::Cell * ElementRegistry::Sheet::cell(const std::uint32_t column, const std::uint32_t row) const { - if (const Row *row_entry = this->row(row); row_entry != nullptr) { - const auto &cells = row_entry->cells; - if (const auto cell_it = util::map::lookup_greater_than(cells, column); - cell_it != std::end(cells)) { - return &cell_it->second; - } - } - return nullptr; + const Row *row_entry = this->row(row); + return row_entry != nullptr ? lookup(row_cells(*row_entry), column) + : nullptr; +} + +std::span +ElementRegistry::Sheet::row_cells(const Row &row) const { + const auto next = &row + 1; + const std::size_t end = + next != rows.data() + rows.size() ? next->first_cell : cells.size(); + return {cells.data() + row.first_cell, end - row.first_cell}; } [[nodiscard]] pugi::xml_node diff --git a/src/odr/internal/odf/odf_element_registry.hpp b/src/odr/internal/odf/odf_element_registry.hpp index 0c7515828..d8358891f 100644 --- a/src/odr/internal/odf/odf_element_registry.hpp +++ b/src/odr/internal/odf/odf_element_registry.hpp @@ -7,7 +7,9 @@ #include #include +#include #include +#include #include #include @@ -36,25 +38,33 @@ class ElementRegistry final { pugi::xml_node last; }; + /// Columns, rows and cells keyed by the *end* of the range they repeat over + /// and resolved with an upper bound, so a run of 5000 is one entry. Sorted + /// vectors, not maps: parsing appends in document order. The cells of every + /// row live in one array per sheet, each row holding where its run starts. struct Sheet final { struct Column final { + std::uint32_t end{0}; pugi::xml_node node; }; struct Cell final { + std::uint32_t end{0}; pugi::xml_node node; ElementIdentifier element_id{null_element_id}; }; struct Row final { + std::uint32_t end{0}; + std::uint32_t first_cell{0}; pugi::xml_node node; - std::map cells; }; TableDimensions dimensions; - std::map columns; - std::map rows; + std::vector columns; + std::vector rows; + std::vector cells; ElementIdentifier first_shape_id{null_element_id}; ElementIdentifier last_shape_id{null_element_id}; @@ -63,6 +73,7 @@ class ElementRegistry final { pugi::xml_node element); void register_row(std::uint32_t row, std::uint32_t repeated, pugi::xml_node element); + /// Has to follow the @ref register_row of the row it belongs to. void register_cell(std::uint32_t column, std::uint32_t row, std::uint32_t columns_repeated, std::uint32_t rows_repeated, pugi::xml_node element, @@ -73,6 +84,9 @@ class ElementRegistry final { [[nodiscard]] const Cell *cell(std::uint32_t column, std::uint32_t row) const; + /// The cells of @p row - one of this sheet's `rows` - in column order. + [[nodiscard]] std::span row_cells(const Row &row) const; + [[nodiscard]] pugi::xml_node column_node(std::uint32_t column) const; [[nodiscard]] pugi::xml_node row_node(std::uint32_t row) const; [[nodiscard]] pugi::xml_node cell_node(std::uint32_t column, @@ -126,7 +140,10 @@ class ElementRegistry final { void append_sheet_cell(ElementIdentifier sheet_id, ElementIdentifier cell_id); private: - std::vector m_elements; + /// A deque, not a vector: `create_element` hands back a reference the parser + /// holds on to, and a vector both invalidates it and peaks holding two + /// copies. + std::deque m_elements; std::unordered_map m_texts; std::unordered_map m_tables; std::unordered_map m_sheets; diff --git a/src/odr/internal/odf/odf_parser.cpp b/src/odr/internal/odf/odf_parser.cpp index 57f9c7ac7..250d5eca2 100644 --- a/src/odr/internal/odf/odf_parser.cpp +++ b/src/odr/internal/odf/odf_parser.cpp @@ -4,7 +4,6 @@ #include #include -#include #include #include @@ -118,7 +117,7 @@ parse_table(ElementRegistry ®istry, const pugi::xml_node node) { // TODO inflate table first? - for (const pugi::xml_node column_node : table_columns(node)) { + for_each_table_column(node, [&](const pugi::xml_node column_node) { const std::uint32_t repeat = column_node.attribute("table:number-columns-repeated").as_uint(1); for (std::uint32_t i = 0; i < repeat; ++i) { @@ -126,13 +125,13 @@ parse_table(ElementRegistry ®istry, const pugi::xml_node node) { auto [column_id, _] = parse_any_element_tree(registry, column_node); registry.append_column(element_id, column_id); } - } + }); - for (const pugi::xml_node row_node : table_rows(node)) { + for_each_table_row(node, [&](const pugi::xml_node row_node) { // TODO log warning if repeated auto [row_id, _] = parse_any_element_tree(registry, row_node); registry.append_child(element_id, row_id); - } + }); return {element_id, node.next_sibling()}; } @@ -154,89 +153,57 @@ parse_sheet(ElementRegistry ®istry, const pugi::xml_node node) { TableCursor cursor; - for (const pugi::xml_node column_node : table_columns(node)) { + for_each_table_column(node, [&](const pugi::xml_node column_node) { const std::uint32_t columns_repeated = column_node.attribute("table:number-columns-repeated").as_uint(1); sheet.register_column(cursor.column(), columns_repeated, column_node); cursor.add_column(columns_repeated); - } + }); sheet.dimensions.columns = cursor.column(); cursor = {}; - for (const pugi::xml_node row_node : table_rows(node)) { + for_each_table_row(node, [&](const pugi::xml_node row_node) { const std::uint32_t rows_repeated = row_node.attribute("table:number-rows-repeated").as_uint(1); sheet.register_row(cursor.row(), rows_repeated, row_node); // TODO covered cells - const bool row_empty = std::ranges::all_of( - row_node.children("table:table-cell"), is_cell_empty); - - if (row_empty) { - // TODO covered cells - for (const pugi::xml_node cell_node : - row_node.children("table:table-cell")) { - const std::uint32_t columns_repeated = - cell_node.attribute("table:number-columns-repeated").as_uint(1); - const std::uint32_t colspan = - cell_node.attribute("table:number-columns-spanned").as_uint(1); - const std::uint32_t rowspan = - cell_node.attribute("table:number-rows-spanned").as_uint(1); - - sheet.register_cell(cursor.column(), cursor.row(), columns_repeated, - rows_repeated, cell_node, null_element_id); - - cursor.add_cell(colspan, rowspan, columns_repeated); + for (const pugi::xml_node cell_node : + row_node.children("table:table-cell")) { + const std::uint32_t columns_repeated = + cell_node.attribute("table:number-columns-repeated").as_uint(1); + const std::uint32_t colspan = + cell_node.attribute("table:number-columns-spanned").as_uint(1); + const std::uint32_t rowspan = + cell_node.attribute("table:number-rows-spanned").as_uint(1); + const bool is_repeated = columns_repeated > 1 || rows_repeated > 1; + + ElementIdentifier cell_id = null_element_id; + if (!is_cell_empty(cell_node)) { + const auto &[id, unused1, unused2] = registry.create_sheet_cell_element( + cell_node, cursor.position(), is_repeated); + cell_id = id; + registry.append_sheet_cell(element_id, cell_id); } - cursor.add_row(rows_repeated); - continue; - } + sheet.register_cell(cursor.column(), cursor.row(), columns_repeated, + rows_repeated, cell_node, cell_id); - for (std::uint32_t row_repeat = 0; row_repeat < rows_repeated; - ++row_repeat) { - sheet.register_row(cursor.row(), 1, row_node); - - // TODO covered cells - for (const pugi::xml_node cell_node : - row_node.children("table:table-cell")) { - const std::uint32_t columns_repeated = - cell_node.attribute("table:number-columns-repeated").as_uint(1); - const std::uint32_t colspan = - cell_node.attribute("table:number-columns-spanned").as_uint(1); - const std::uint32_t rowspan = - cell_node.attribute("table:number-rows-spanned").as_uint(1); - const bool is_repeated = columns_repeated > 1 || rows_repeated > 1; - - if (is_cell_empty(cell_node)) { - sheet.register_cell(cursor.column(), cursor.row(), columns_repeated, - 1, cell_node, null_element_id); - - cursor.add_cell(colspan, rowspan, columns_repeated); - continue; - } - - for (std::uint32_t column_repeat = 0; column_repeat < columns_repeated; - ++column_repeat) { - const auto &[cell_id, unused1, unused2] = - registry.create_sheet_cell_element(cell_node, cursor.position(), - is_repeated); - registry.append_sheet_cell(element_id, cell_id); - sheet.register_cell(cursor.column(), cursor.row(), 1, 1, cell_node, - cell_id); - parse_any_element_children(registry, cell_id, cell_node); - - cursor.add_cell(colspan, rowspan, 1); - } + if (cell_id != null_element_id) { + parse_any_element_children(registry, cell_id, cell_node); } - cursor.add_row(1); + cursor.add_cell(colspan, rowspan, columns_repeated); } - } + + // TODO a rowspan out of a repeated row is dropped - `add_row` clears the + // cursor's pending ranges for a repeat > 1 + cursor.add_row(rows_repeated); + }); sheet.dimensions.rows = cursor.row(); diff --git a/src/odr/internal/odf/odf_table.cpp b/src/odr/internal/odf/odf_table.cpp index f08d12721..1efc62b3c 100644 --- a/src/odr/internal/odf/odf_table.cpp +++ b/src/odr/internal/odf/odf_table.cpp @@ -29,17 +29,17 @@ bool is_displayed(const pugi::xml_node group) { return !display || display.as_bool(true); } -void collect(const pugi::xml_node parent, const std::string_view name, - const std::span group_names, - std::vector &out) { +void walk(const pugi::xml_node parent, const std::string_view name, + const std::span group_names, + const TableNodeVisitor &visit) { for (const pugi::xml_node child : parent.children()) { const std::string_view child_name = child.name(); if (child_name == name) { - out.push_back(child); + visit(child); } else if (std::ranges::find(group_names, child_name) != std::end(group_names) && is_displayed(child)) { - collect(child, name, group_names, out); + walk(child, name, group_names, visit); } } } @@ -50,16 +50,14 @@ void collect(const pugi::xml_node parent, const std::string_view name, namespace odr::internal { -std::vector odf::table_rows(const pugi::xml_node table) { - std::vector result; - collect(table, "table:table-row", row_group_names, result); - return result; +void odf::for_each_table_row(const pugi::xml_node table, + const TableNodeVisitor &visit) { + walk(table, "table:table-row", row_group_names, visit); } -std::vector odf::table_columns(const pugi::xml_node table) { - std::vector result; - collect(table, "table:table-column", column_group_names, result); - return result; +void odf::for_each_table_column(const pugi::xml_node table, + const TableNodeVisitor &visit) { + walk(table, "table:table-column", column_group_names, visit); } } // namespace odr::internal diff --git a/src/odr/internal/odf/odf_table.hpp b/src/odr/internal/odf/odf_table.hpp index 18c10da2f..20d3f470c 100644 --- a/src/odr/internal/odf/odf_table.hpp +++ b/src/odr/internal/odf/odf_table.hpp @@ -1,6 +1,6 @@ #pragma once -#include +#include namespace pugi { class xml_node; @@ -8,11 +8,15 @@ class xml_node; namespace odr::internal::odf { -/// A table's rows in document order, including those a grouping element holds -/// - the `table:table-*-rows` family, which nests ([ODF 1.2] 9.1.7). -[[nodiscard]] std::vector table_rows(pugi::xml_node table); +using TableNodeVisitor = std::function; -/// The column counterpart of `table_rows()` ([ODF 1.2] 9.1.6). -[[nodiscard]] std::vector table_columns(pugi::xml_node table); +/// Calls @p visit for a table's rows in document order, including those a +/// grouping element holds - the `table:table-*-rows` family, which nests +/// ([ODF 1.2] 9.1.7). A visitor, not a container: a sheet is walked repeatedly +/// and each walk would materialise every row. +void for_each_table_row(pugi::xml_node table, const TableNodeVisitor &visit); + +/// The column counterpart of @ref for_each_table_row ([ODF 1.2] 9.1.6). +void for_each_table_column(pugi::xml_node table, const TableNodeVisitor &visit); } // namespace odr::internal::odf diff --git a/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_parser.cpp b/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_parser.cpp index becf7edd1..c30d7fb6e 100644 --- a/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_parser.cpp +++ b/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_parser.cpp @@ -149,6 +149,21 @@ parse_sheet_element(ElementRegistry ®istry, const ParseContext &context, TableDimensions(range.to().row - range.from().row + 1, range.to().column - range.from().column + 1); + // A `ref` may name the whole grid, 17 billion positions - so past the + // point where the range is bigger than the sheet has cells, walk the cells. + const std::uint64_t area = + static_cast(range.to().row - range.from().row + 1) * + (range.to().column - range.from().column + 1); + + if (area > sheet.cells.size()) { + for (const auto &[position, cell] : sheet.cells) { + if (position != range.from() && range.contains(position)) { + registry.sheet_cell_element_at(cell.element_id).is_covered = true; + } + } + continue; + } + for (std::uint32_t row = range.from().row; row <= range.to().row; ++row) { for (std::uint32_t column = range.from().column; column <= range.to().column; ++column) { diff --git a/src/odr/internal/util/xml_util.cpp b/src/odr/internal/util/xml_util.cpp index a931e4ede..074636260 100644 --- a/src/odr/internal/util/xml_util.cpp +++ b/src/odr/internal/util/xml_util.cpp @@ -17,6 +17,11 @@ namespace odr::internal::util { +// `PUGIXML_COMPACT` (CMakeLists.txt) changes the size of every node, and a +// translation unit that misses it links fine and then reads the wrong layout. +static_assert(sizeof(pugi::xml_node_struct) == 12); +static_assert(sizeof(pugi::xml_attribute_struct) == 8); + pugi::xml_document xml::parse(const std::string &in) { pugi::xml_document result; if (const auto success = result.load_string(in.c_str()); !success) { diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 3a896cf05..00a37bce1 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -72,6 +72,7 @@ add_executable(odr_test "src/internal/rtf/rtf_tokenizer_test.cpp" "src/internal/odf/odf_flat_file_test.cpp" + "src/internal/odf/odf_sheet_repeat_test.cpp" "src/internal/odf/odf_table_test.cpp" "src/internal/oldms/doc_test.cpp" @@ -81,6 +82,7 @@ add_executable(odr_test "src/internal/ooxml/ooxml_crypto_test.cpp" "src/internal/ooxml/ooxml_text_style_test.cpp" + "src/internal/ooxml/ooxml_spreadsheet_merge_test.cpp" "src/internal/ooxml/ooxml_util_test.cpp" "src/internal/ooxml/ooxml_presentation_style_test.cpp" diff --git a/test/src/internal/odf/odf_sheet_repeat_test.cpp b/test/src/internal/odf/odf_sheet_repeat_test.cpp new file mode 100644 index 000000000..d3cb872fb --- /dev/null +++ b/test/src/internal/odf/odf_sheet_repeat_test.cpp @@ -0,0 +1,88 @@ +#include +#include + +#include +#include +#include +#include +#include + +#include +#include +#include +#include + +#include + +#include +#include + +using namespace odr; +using namespace odr::internal; + +namespace { + +std::string flat_sheet(const std::string &body) { + return R"()" + R"()" + R"()" + R"()" + + body + + R"()"; +} + +/// One row repeated @p rows_repeated times holding one cell repeated +/// @p columns_repeated times, all of it non-empty. +std::string repeated_rows(const std::uint32_t rows_repeated, + const std::uint32_t columns_repeated) { + return R"()" + R"(x)"; +} + +std::shared_ptr document_of(const std::string &source) { + const std::unique_ptr file = + open_strategy::open_document_file(std::make_shared(source), + Logger::null()); + return file->document(); +} + +} // namespace + +/// Both repeats are legal, and expanding them would ask for three billion +/// elements from four hundred bytes. +TEST(OdfSheetRepeat, a_repeated_cell_is_one_element) { + const std::string source = flat_sheet(repeated_rows(1048576, 1024)); + const std::shared_ptr held = document_of(source); + const auto *document = dynamic_cast(held.get()); + ASSERT_NE(document, nullptr); + + EXPECT_LT(document->element_registry().size(), 16); +} + +TEST(OdfSheetRepeat, a_repeated_cell_reads_at_every_position_it_covers) { + const std::string source = flat_sheet(repeated_rows(4, 3)); + const std::shared_ptr held = document_of(source); + + const odr::Document public_document(held); + const Sheet sheet = + (*public_document.root_element().children().begin()).as_sheet(); + + EXPECT_EQ(sheet.dimensions().rows, 4); + EXPECT_EQ(sheet.dimensions().columns, 3); + + for (std::uint32_t row = 0; row < 4; ++row) { + for (std::uint32_t column = 0; column < 3; ++column) { + const SheetCell cell = sheet.cell(column, row); + ASSERT_TRUE(cell) << column << "," << row; + const Element paragraph = *cell.children().begin(); + EXPECT_EQ((*paragraph.children().begin()).as_text().content(), "x"); + } + } +} diff --git a/test/src/internal/odf/odf_table_test.cpp b/test/src/internal/odf/odf_table_test.cpp index cf3869b55..a800be7e5 100644 --- a/test/src/internal/odf/odf_table_test.cpp +++ b/test/src/internal/odf/odf_table_test.cpp @@ -26,6 +26,20 @@ using namespace odr::internal::odf; namespace { +std::vector rows_of(const pugi::xml_node table) { + std::vector result; + for_each_table_row(table, + [&](const pugi::xml_node row) { result.push_back(row); }); + return result; +} + +std::vector columns_of(const pugi::xml_node table) { + std::vector result; + for_each_table_column( + table, [&](const pugi::xml_node column) { result.push_back(column); }); + return result; +} + std::vector names_of(const std::vector &nodes) { std::vector result; for (const pugi::xml_node node : nodes) { @@ -109,7 +123,7 @@ TEST(OdfTable, rows_directly_under_the_table) { )"); - EXPECT_EQ(names_of(table_rows(table)), (std::vector{"a", "b"})); + EXPECT_EQ(names_of(rows_of(table)), (std::vector{"a", "b"})); } TEST(OdfTable, rows_inside_a_grouping_element) { @@ -124,7 +138,7 @@ TEST(OdfTable, rows_inside_a_grouping_element) { )"); - EXPECT_EQ(names_of(table_rows(table)), + EXPECT_EQ(names_of(rows_of(table)), (std::vector{"header", "body"})); } @@ -143,7 +157,7 @@ TEST(OdfTable, row_groups_nest) { )"); - EXPECT_EQ(names_of(table_rows(table)), + EXPECT_EQ(names_of(rows_of(table)), (std::vector{"a", "b", "c"})); } @@ -162,7 +176,7 @@ TEST(OdfTable, a_collapsed_group_is_not_shown) { )"); - EXPECT_EQ(names_of(table_rows(table)), (std::vector{"shown"})); + EXPECT_EQ(names_of(rows_of(table)), (std::vector{"shown"})); } TEST(OdfTable, columns_inside_a_grouping_element) { @@ -178,7 +192,7 @@ TEST(OdfTable, columns_inside_a_grouping_element) { )"); - EXPECT_EQ(names_of(table_columns(table)), + EXPECT_EQ(names_of(columns_of(table)), (std::vector{"a", "b", "c"})); } diff --git a/test/src/internal/ooxml/ooxml_spreadsheet_merge_test.cpp b/test/src/internal/ooxml/ooxml_spreadsheet_merge_test.cpp new file mode 100644 index 000000000..fb037c9a2 --- /dev/null +++ b/test/src/internal/ooxml/ooxml_spreadsheet_merge_test.cpp @@ -0,0 +1,107 @@ +#include +#include +#include +#include +#include + +#include +#include +#include +#include +#include +#include + +#include + +#include +#include +#include + +using namespace odr; +using namespace odr::internal; + +namespace { + +void insert(zip::ZipArchive &zip, const std::string &path, + const std::string &content) { + zip.insert_file(std::end(zip), RelPath(path), + std::make_shared(content)); +} + +/// The smallest workbook that opens: one sheet, whose `` and +/// `` are @p sheet_data and @p merge_cells. +std::shared_ptr workbook(const std::string &sheet_data, + const std::string &merge_cells) { + zip::ZipArchive zip; + insert( + zip, "[Content_Types].xml", + R"()" + R"()" + R"()" + R"()"); + insert( + zip, "_rels/.rels", + R"()" + R"()" + R"()"); + insert( + zip, "xl/workbook.xml", + R"()" + R"()"); + insert( + zip, "xl/_rels/workbook.xml.rels", + R"()" + R"()" + R"()"); + insert( + zip, "xl/styles.xml", + R"()"); + insert( + zip, "xl/worksheets/sheet1.xml", + R"()" + R"()" + + sheet_data + R"()" + merge_cells + R"()"); + + std::stringstream out; + zip.save(out); + return std::make_shared(out.str()); +} + +Sheet first_sheet(const Document &document) { + return (*document.root_element().children().begin()).as_sheet(); +} + +Document decode(const std::shared_ptr &file) { + return Document( + open_strategy::open_document_file(file, Logger::null())->document()); +} + +constexpr const char *two_cells = + R"(a)" + R"(b)"; + +} // namespace + +TEST(OoxmlSpreadsheetMerge, a_merge_covers_the_cells_it_spans) { + const Document document = decode(workbook( + two_cells, R"()")); + const Sheet sheet = first_sheet(document); + + EXPECT_EQ(sheet.cell(0, 0).span().columns, 2); + EXPECT_FALSE(sheet.cell(0, 0).is_covered()); + EXPECT_TRUE(sheet.cell(1, 0).is_covered()); +} + +/// The whole grid is 17 billion positions, so the covered cells have to be +/// found by walking what was read. +TEST(OoxmlSpreadsheetMerge, + a_merge_over_the_whole_grid_is_bounded_by_the_file) { + const Document document = decode( + workbook(two_cells, + R"()")); + const Sheet sheet = first_sheet(document); + + EXPECT_FALSE(sheet.cell(0, 0).is_covered()); + EXPECT_TRUE(sheet.cell(1, 0).is_covered()); +}