fix: Dynamic XML namespace prefixes for consistent Excel files#28
Conversation
…ility Dynamically generated sheetData content was missing namespace prefixes, causing XML inconsistency when worksheet root uses 'x:' prefix. Symptoms: Excel opens the file with correct file size but displays an empty worksheet - data is silently ignored due to malformed XML.
When saving, inconsistent namespace prefixes were used, resulting in invalid XML documents. The prefix is now dynamically determined from the document and used consistently across all elements. - Extract _nsPrefix from WorksheetXml. DocumentElement - Replace all hardcoded "x:" prefixes with _nsPrefix - Add using alias RegexMatch to resolve naming conflict - Fix $"" combined with AppendFormat/string.Format calls
📝 WalkthroughWalkthroughExcelWorksheet now computes and stores the worksheet XML namespace prefix and uses it when writing serialized worksheet XML tags; a Regex Match alias and using reorder were also added. ChangesXML Namespace-Aware Serialization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
EPPlus/ExcelWorksheet.cs (1)
3440-3449: Translate comment to English for consistency.The comment on line 3440 is in German. For codebase consistency and maintainability, it should be in English.
Suggested fix
- // Ermittle den Namespace-Prefix aus dem Dokument + // Determine the namespace prefix from the document var worksheetNode = _worksheetXml.DocumentElement;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
EPPlus/ExcelWorksheet.cs(27 hunks)
🔇 Additional comments (6)
EPPlus/ExcelWorksheet.cs (6)
58-58: LGTM!The
RegexMatchalias is a clean approach to avoid naming conflicts while maintaining code clarity.
375-375: LGTM!The field is correctly initialized with an empty string default, ensuring consistent behavior for both prefixed and non-prefixed XML output.
1465-1472: LGTM!The namespace prefix is correctly applied to the merged cells XML elements. The formatting with string interpolation is clean and readable.
3653-3653: LGTM!The namespace prefix is consistently applied to all
sheetDataelements and closing tags. The string interpolation pattern$"<{_nsPrefix}sheetData>"correctly handles both prefixed and non-prefixed XML output.Also applies to: 3784-3785
4064-4064: LGTM!The namespace prefix is correctly integrated into all hyperlink XML elements while preserving the existing logic for handling internal and external hyperlinks.
Also applies to: 4073-4074, 4097-4102, 4110-4110
3873-3873: LGTM!The namespace prefix is correctly applied to value elements while preserving the existing XML escaping logic.
There was a problem hiding this comment.
Pull request overview
This PR updates ExcelWorksheet's worksheet XML serializer to emit worksheet content with a dynamic namespace prefix derived from the loaded worksheet document, so saved XML stays namespace-aware when a worksheet uses prefixed main-namespace elements.
Changes:
- Added
_nsPrefixdetection inSaveXmlbased on the worksheet document element. - Updated streamed worksheet XML writing for rows, cells, formulas, merges, hyperlinks, column definitions, and page breaks to prepend the detected prefix.
- Refactored a couple of
Matchreferences to use an alias after introducing a new namespace import.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@EPPlus/ExcelWorksheet.cs`:
- Around line 3437-3451: The code collapses sheetData into a hardcoded
unprefixed element causing prefixed templates to lose their namespace; update
GetWorkSheetXml (and places that call CreateNode("d:sheetData")) to use the
actual prefix found by GetWorksheetNamespacePrefix or the stored _nsPrefix when
creating/replacing the empty sheetData node so the created node name is prefix +
"sheetData" (or ":sheetData" omitted if no prefix), and ensure you operate
on/replace the existing _worksheetXml sheetData node rather than inserting a new
unprefixed one.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7ff65796-9a63-423d-aca3-9f531c28b488
📒 Files selected for processing (1)
EPPlus/ExcelWorksheet.cs
There was a problem hiding this comment.
🧹 Nitpick comments (1)
EPPlus/ExcelWorksheet.cs (1)
3437-3526: ⚡ Quick winAdd regression tests for both default and explicit worksheet namespace prefixes.
Given how many emitters now depend on
_nsPrefix, add coverage for both<worksheet xmlns="...">and<x:worksheet xmlns:x="...">inputs to assert valid, single-instancesheetData/row/c/v/mergeCells/hyperlinksoutput.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@EPPlus/ExcelWorksheet.cs` around lines 3437 - 3526, Add regression tests that cover both default and explicit worksheet namespace prefixes to exercise GetWorksheetNamespacePrefix and SaveXml (which sets _nsPrefix). Create two test inputs: one with <worksheet xmlns="..."> and one with <x:worksheet xmlns:x="...">, call the SaveXml flow (or the public method that triggers SaveXml) and assert the serialized output contains exactly one instance of each emitted block (sheetData, row, c, v, mergeCells, hyperlinks, rowBreaks, colBreaks) and that element names are correctly prefixed (or unprefixed) matching _nsPrefix; ensure tests fail if duplicate blocks or incorrect prefixes are emitted so future changes to GetWorksheetNamespacePrefix/SaveXml are covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@EPPlus/ExcelWorksheet.cs`:
- Around line 3437-3526: Add regression tests that cover both default and
explicit worksheet namespace prefixes to exercise GetWorksheetNamespacePrefix
and SaveXml (which sets _nsPrefix). Create two test inputs: one with <worksheet
xmlns="..."> and one with <x:worksheet xmlns:x="...">, call the SaveXml flow (or
the public method that triggers SaveXml) and assert the serialized output
contains exactly one instance of each emitted block (sheetData, row, c, v,
mergeCells, hyperlinks, rowBreaks, colBreaks) and that element names are
correctly prefixed (or unprefixed) matching _nsPrefix; ensure tests fail if
duplicate blocks or incorrect prefixes are emitted so future changes to
GetWorksheetNamespacePrefix/SaveXml are covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6b630176-a887-4657-9910-e6a8d5e8e1e6
📒 Files selected for processing (1)
EPPlus/ExcelWorksheet.cs
Summary by CodeRabbit