pjfanning commented on PR #971:
URL: https://github.com/apache/poi/pull/971#issuecomment-5104924950
I got Claude to analyse this PR.
I only occasionally have spare tokens to do these analyses.
---
Issues Found
🔴 Critical
1. Duplicate runs in XWPFParagraph
In buildRunsInOrderFromXml, when a CTSdtRun is encountered, the PR adds the
XWPFSDT to iruns AND then also creates XWPFRun instances for every CTR inside
the SDT content, adding them to runs. This means:
- getText() (which iterates iruns) will include the SDT content twice —
once via XWPFSDT.getContent().getText() and once via the individual XWPFRun
entries
- getRuns() will return runs that conceptually belong inside an SDT but
appear as standalone runs, breaking the abstraction
Recommendation: Either don't add the inner CTRs to runs, or don't add the
XWPFSDT to iruns. Pick one representation, not both.
2. isRunInParagraphEditContainer may always return true for SDT content
The method walks up the ancestor chain looking for CTP | CTHyperlink |
CTSimpleField. But SDT content elements sit inside a CTSdtRun which sits inside
a CTP. So the cursor will find the enclosing CTP and return true for every CTR
inside any SDT, making the check essentially a no-op. If the intent is to
filter out CTRs that aren't in a valid paragraph context, this guard doesn't
actually filter anything.
2. Recommendation: Clarify what this method is actually guarding against. If
it's always true for SDT content, remove it for clarity.
🟠 Major
3. addRow(XWPFTableRow, int) and insertNewTableRow(int) — complex cursor
logic may break XML ordering
When inserting before an SDT-wrapped row, the code inserts into
CTSdtContentRow at the SDT's internal position. But the tableRows list is flat
— it doesn't track which rows are SDT-wrapped. If a user inserts at position
pos, the code assumes tableRows.get(pos) is the correct "next row" in XML
order, but after multiple insertions/removals the flat list may not reflect the
actual XML structure. This could lead to rows being inserted in the wrong XML
location.
3. Recommendation: Consider whether the flat tableRows list adequately
models the nested SDT structure, or if a more robust position-tracking
mechanism is needed.
4. removeRow(int) — cursor-based parent detection is fragile
The code uses cursor.toParent() to find whether the row is in CTTbl or
CTSdtContentRow. If the XML structure has additional wrapping elements (e.g.,
CTTrPr or other future wrappers), the parent won't match either case and the
row won't be removed from the XML — only from tableRows. This creates a silent
inconsistency.
4. Recommendation: Add a fallback or log a warning if the parent is neither
CTTbl nor CTSdtContentRow.
5. getRow(int) bounds check changed from ctTbl.sizeOfTrArray() to
tableRows.size()
The old code used ctTbl.sizeOfTrArray() as the bounds check. The new code
uses tableRows.size(). This is correct for SDT-wrapped rows, but it means
getRow() can now return rows that aren't direct children of ctTbl — callers
that do getRow(i).getCtRow() and expect it to be at ctTbl.getTrArray(i) will
break.
5. Recommendation: Document this behavioral change clearly in the Javadoc.
🟡 Minor
6. hasSdtRows() iterates the entire table to check for any SDT row
This is called in the constructor only when table.sizeOfTrArray() == 0. It's
fine for correctness but could be more efficient — a single selectPath with a
namespace-aware query could do this in one step rather than iterating all
children.
7. Code style inconsistency
- Some places use if (sdtContent != null)\n{ (brace on next line), others
use if (sdtContent != null) { (same line). Apache POI conventions use same-line
braces.
- (CTSdtRun)o vs (CTSdtRun) o — inconsistent spacing after cast.
- Missing @since annotations on new public/package-private methods.
8. insertNewTableRow creates CTRow.Factory.newInstance() then calls
newRow.set(addedRow)
This pattern creates an empty row, then copies another empty row into it.
The set() call copies the XML content, but the addedRow from addNewTr() is
already a proper empty row. The extra Factory.newInstance() + set() is
unnecessary overhead. Just use the row returned by addNewTr() / insertNewTr()
directly, as the original code did.
9. Missing @Override or @Internal annotations on new methods like
processCTRow, collectCTRowsInnerSdtRow, hasSdtRows, processCTRs,
isRunInParagraphEditContainer.
🔵 Test Coverage
10. Tests were not included in the diff I reviewed — the web fetch couldn't
extract the test file diffs. However, the PR description mentions tests were
added. Key test scenarios that should be covered:
- Table with SDT-wrapped rows: verify getNumberOfRows(), getRow(),
getText() return correct values
- Table with mix of SDT-wrapped and direct rows
- Nested SDT rows (SDT within SDT)
- Paragraph with SDT-wrapped runs: verify getRuns() and getText() behavior
- addRow / removeRow / insertNewTableRow on SDT-wrapped tables
- Round-trip (write + read) preserves SDT structure
---
Positive Aspects
- The core approach of using XmlCursor to traverse SDT-wrapped content is
sound and matches how other parts of Apache POI handle XML navigation
- The collectCTRowsInnerSdtRow recursive method correctly handles
arbitrarily nested SDT rows
- The hasSdtRows check prevents the constructor from creating a spurious
empty table when all rows are SDT-wrapped
- Refactoring processCTRow out of the constructor is a good DRY improvement
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
