Skip to content

fix(docx): bound the layout grid DOCX tables can declare - #4519

Merged
badGarnet merged 4 commits into
mainfrom
fix/docx-table-grid-limit
Oct 2, 2026
Merged

badGarnet merged 4 commits into
mainfrom
fix/docx-table-grid-limit

Conversation

@badGarnet

@badGarnet badGarnet commented Oct 2, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

w:gridBefore, w:gridAfter and w:gridSpan values were expanded into one matrix entry per layout-grid position when building text_as_html. A small document declaring millions of positions per row therefore used GB of memory. python-docx's row.cells also repeats a spanned cell once per position, and the HTML, nested-table and emphasis paths re-read that cell's content every time.

  • Grid budget. Each table's grid size is computed from the declared values without expanding them, against a document-wide DOCX_TABLE_MAX_CELLS budget (new env setting, default 5,000,000). A table over the remaining budget gets text_as_html=None and a warning. Its text is still extracted, because _iter_table_texts already walks physical tc elements.

  • Each cell read once. The HTML path computes each cell's text once per tc instead of once per grid position.

  • Physical tc walk. Nested-table text and emphasis now walk physical tc elements, skipping vMerge="continue", as _iter_table_texts already does. Neither expands spans any more.

  • Failure containment. A w:gridSpan/w:gridBefore/w:gridAfter value that can't be parsed omits only that table's text_as_html, with a warning, without expanding it or using budget; its text is still extracted. A table paragraph whose w:b/w:i can't be read contributes no emphasis, with a warning; each paragraph's emphasis is collected before any is emitted, so contents and tags stay aligned.

Measurements (2-column tables in a ~37 KB DOCX)

Case Before After
2 rows, gridSpan=100000 11.2s, 200,000 emphasis entries 0.4s, 2 entries
20 rows, gridSpan or gridBefore = 5,000,000 not run (≈100M grid positions) 0.3s, no text_as_html, text extracted

Behaviour change

emphasized_text_contents/_tags no longer repeat entries for a cell that spans several grid positions or merges vertically. Previously a bold cell spanning N positions was reported N times.

Tests

  • A table declaring 10M positions per row (via gridSpan and via gridBefore) gets no HTML, but keeps its text and its de-duplicated emphasis.

  • The budget is summed across tables: the first table gets HTML and the second doesn't.

  • A spanned cell renders as one colspan cell and reports its emphasis once.

  • An invalid or missing gridSpan value keeps the table's text, omits its HTML and uses none of the budget, while a later table still gets HTML.

  • Unreadable bold/italic values drop that paragraph's emphasis, with and without infer_table_structure, and extraction continues.

  • A vertically merged cell's emphasis is reported once, and a 10M-position span inside a nested table is not expanded.

ODT and DOC go through this code after conversion to DOCX. Their tests weren't run locally, because they need pandoc and LibreOffice.

🤖 Generated with Claude Code

Review in cubic

w:gridBefore, w:gridAfter and w:gridSpan values were expanded into one
matrix entry per layout-grid position when building text_as_html, so a
few-KB document declaring millions of positions per row used GB of memory.
row.cells also repeats a spanned cell once per position, and the HTML,
nested-table and emphasis paths re-read that cell each time.

- Compute each table's grid size from the declared values without
  expanding them, against a document-wide DOCX_TABLE_MAX_CELLS budget
  (default 5,000,000). Over budget, omit text_as_html with a warning; the
  table's text is still extracted.
- Read each cell's text once per tc instead of once per grid position.
- Walk physical tc elements for nested-table text and for emphasis, as
  _iter_table_texts already does, so neither expands spans and emphasis
  is no longer repeated for spanned or vertically merged cells.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 5 files

Shadow auto-approve: would not auto-approve because issues were found.

Re-trigger cubic

Comment thread unstructured/partition/docx.py Outdated
A malformed negative w:gridBefore, w:gridAfter or w:gridSpan expands to
no grid positions, but it was summed as negative, so it could cancel a
positive span and let row.cells expand millions of positions under the
budget. Count each declared value below zero as zero.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0 issues found across 2 files (changes from recent commits).

Shadow auto-approve: would not auto-approve. This PR does not meet the repository auto-approval settings.

Re-trigger cubic

@cragwolfe cragwolfe left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GPT Pro review of b6eed1f, verified against source and main. The grid budget and physical-cell traversal materially improve resource behavior, but two introduced failure-containment regressions prevent approval: malformed span attributes now abort extraction in the unguarded preflight, and malformed bold/italic attributes now abort extraction after removal of the emphasis exception boundary. Details and acceptance criteria are inline. Exact-head CI is successful (test run 36953048321); no local tests or source changes.

Nonblocking follow-up: inconsistent vertical-merge continuation widths can make python-docx expand the ancestor span beyond the declared budget estimate. Main already has this expansion, so this is residual hardening rather than an introduced regression. Consider rejecting inconsistent geometry before expansion, and add coverage for large nested spans and vertical-merge emphasis.

(authored by codex)

Comment thread unstructured/partition/docx.py Outdated
millions, so `None` is returned once this document's tables exceed `DOCX_TABLE_MAX_CELLS`
grid positions in total. The table's text is still extracted.
"""
n_grid_cells = sum(_row_grid_width(row) for row in table.rows)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[High] Preserve text fallback when gridSpan cannot be parsed. _row_grid_width() reads tc.grid_span before the existing row.cells try/except. <w:gridSpan w:val="bad"/> raises ValueError, and <w:gridSpan/> raises InvalidXmlError. Main catches that failure during HTML and emphasis traversal and still extracts physical-cell text; this head aborts before _iter_table_texts() runs. Catch invalid grid metadata, warn and omit this table's HTML without attempting an unknown-size expansion. Add nonnumeric/missing-span cases with a later healthy table, checking preserved text and budget behavior. Verified against main's exception boundary and python-docx's integer/XML parsing; no local tests executed.

(authored by codex)

Comment thread unstructured/partition/docx.py Outdated
if tc.vMerge == "continue":
continue
for paragraph in _Cell(tc, table).paragraphs:
yield from self._iter_paragraph_emphasis(paragraph)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[High] Keep failure containment around physical emphasis extraction. The removed try/except also protected _iter_paragraph_emphasis(): run.bold/run.italic can raise InvalidXmlError for <w:b w:val="bad"/> or the italic equivalent. Main skips the failed row's emphasis and returns readable table text/HTML; the new traversal propagates and prevents yielding the Table, including with infer_table_structure=False. Keep the physical tc walk and restore a bounded run/paragraph/cell recovery boundary so bad formatting degrades metadata rather than the document. Cover bold/italic failures in both inference modes, continued later content, aligned emphasis lists, and preserved deduplication.

(authored by codex)

Review follow-ups:

- The grid-size check read w:gridSpan/w:gridBefore/w:gridAfter outside
  any exception boundary, so a value that is not a number (or a missing
  w:val) aborted the document. Treat it as unknown: warn, omit that
  table's text_as_html without expanding it, use none of the budget, and
  keep its text.
- Restore failure containment for table emphasis, lost when the row.cells
  try/except went: a w:b or w:i value that cannot be read now drops that
  paragraph's emphasis with a warning. Each paragraph's emphasis is
  collected before any is yielded, so contents and tags stay aligned.
- Cover emphasis of a vertically merged cell and a large span inside a
  nested table.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0 issues found across 3 files (changes from recent commits).

Shadow auto-approve: would not auto-approve. This PR does not meet the repository auto-approval settings.

Re-trigger cubic

@cragwolfe cragwolfe left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fresh full-context GPT Pro re-review of 5cc60f9, verified against source and main: approved. Both prior material findings are resolved. Malformed grid values now omit only that table's HTML without expansion or budget consumption; physical-cell text extraction continues. Malformed bold/italic properties now drop only the failed paragraph's emphasis, collected atomically before emission, preserving later content and aligned metadata. The added tests cover both failures, both inference modes, vertical-merge deduplication and a large nested span. The complete change materially improves resource behavior over main.

Nonblocking follow-ups: validate inconsistent vertical-merge continuation widths before expansion (the same ancestor-span expansion exists on main); add a valid-emphasis-then-invalid-run case followed by another paragraph to explicitly cover paragraph atomicity, plus gridAfter and fresh-invocation budget coverage. These do not prevent approval.

Validation: complete untruncated production source/test bundle and fresh holistic Pro response, with every advisory finding checked against actual source. Exact-head checks show two successes and one neutral security result; no Actions test workflow is recorded for this new SHA, so the previous head's green tests are not evidence that these new regressions ran. No local tests or source changes were performed.

(authored by codex)

…imit

# Conflicts:
#	CHANGELOG.md
#	unstructured/__version__.py
#	unstructured/partition/utils/config.py
@badGarnet
badGarnet enabled auto-merge October 2, 2026 17:22
@badGarnet
badGarnet added this pull request to the merge queue Oct 2, 2026
Merged via the queue into main with commit 25f405c Oct 2, 2026
53 checks passed
@badGarnet
badGarnet deleted the fix/docx-table-grid-limit branch October 2, 2026 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants