Fill in missing row and cell references in templates (#863) - #1015
michelebastione merged 1 commit into
Conversation
The "r" attribute on rows and cells is optional (ECMA-376 18.3.1.73 and 18.3.1.4): without it, a row follows the previous row and a cell follows the previous cell, which is how OpenXmlReader already reads them. The template code keys everything on "r", so a template written that way crashed in UpdateDimensionAndGetRowsInfo with an ArgumentNullException. FillMissingReferences fills in the missing ones when the template sheet is loaded, in both the create and update modes, so FillTemplate and MergeSameCells both work. References that are already there are left as they are, so a malformed one still gets the existing NotSupportedException.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughTemplate generation now reconstructs missing row and cell references in update and create modes. Tests cover templates with removed references and verify filled values, dimensions, and merged ranges. ChangesReference reconstruction
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix · Severity of issue fixed: Medium Suggested reviewers: Merge Risk: ⚪ Minimal · up to Template processing now supports XLSX files with omitted row or cell references in both create and update paths. The supplied regression coverage indicates expected values and merged ranges are preserved, with no actionable merge-blocking risk identified. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Thank you for your contribution! I'll review it soon! |
2 participants
Fixes #863
The
rattribute on rows and cells is optional in SpreadsheetML (ECMA-376 18.3.1.73 and 18.3.1.4). When it's missing, the row comes right after the previous row and the cell right after the previous cell, which is also howOpenXmlReaderalready reads these files. Some tools write xlsx files like that and Excel opens them fine, but the template code keys everything onr.The screenshot in #863 is from v1: the debugger stops at
//TODO: some xlsx without rwith aFormatExceptionfromint.Parse(row.GetAttribute("r")). On master the same kind of template fails a bit earlier:I added
FillMissingReferences, which fills in the missingrvalues when the template sheet is loaded. It runs in both the create and the update mode, soFillTemplateandMergeSameCellsare both covered, and nothing else in the template code changes. References that are already there are left alone, so a malformed one still gets the existingNotSupportedException. I also removed the TODO about this.Tests:
TestIssue863takesTestTemplateComplex.xlsx, removesrfrom every cell (and from every row too, or keeps the row ones as a second case) and checks the result is the same as filling the original template. There's an async version as well.TestIssue863_MergeSameCellsdoes the same withTestMergeWithTag.xlsx.All of them fail on master and pass with the fix, and the whole OpenXml test project passes on net8.0 and net10.0.
The same bug is on v1.x-maintenance, which is the version the reporter uses. I can port the fix there too if you want.
Summary by CodeRabbit