Repository navigation
fix(docx): preserve equations and text across XML encodings - #2602
LeonSGP (LeonSGP43) wants to merge 2 commits into
Conversation
XU (kokokoXUY)
left a comment
There was a problem hiding this comment.
Review: honor the XML declaration when converting DOCX equations
I read the patch, rebuilt both sides locally, and exercised the new preprocessing
path beyond the added test. The change is a single line and it does fix the
reported bug. I also found one class of inputs where trusting the declaration is
worse than the previous unconditional UTF-8 decode, so I would add a small
fallback before this ships. Details below.
Note on notation: sample strings that contain non-ASCII characters are written
with ASCII placeholders (<CJK>, <emoji>, <accented>) because this review
body is ASCII-only; every placeholder stands for real text in the local runs.
What the patch does
_pre_process_math passed content.decode() to BeautifulSoup; the decode used
UTF-8 regardless of the XML declaration, so a word/document.xml stored as
UTF-16 raised UnicodeDecodeError, pre_process_docx swallowed the exception
(except Exception: pass, pre_process.py:284-287), and the document was
converted with the raw OMML untouched - the prose survived and the equations
disappeared. Passing the bytes lets the XML parser use the declaration and BOM.
What I verified locally
- Reported failure reproduced on the base commit. With a BOM-prefixed
UTF-16LE/BEword/document.xml,_pre_process_mathraises
UnicodeDecodeError: 'utf-8' codec can't decode byte 0xff in position 0: invalid start byteand the conversion returnsBefore<CJK><emoji>After. - The fix works for inline and block equations. utf-8, utf-16-le and
utf-16-be all giveBefore<CJK><emoji>$x$After; foroMathParaall three
giveBefore\n\n$$y=1$$\n\nAfter, where base producedBefore\n\nAfterfor
both UTF-16 variants. - No truncation at scale. A 400-paragraph document with one equation per
paragraph produces byte-identical preprocessed XML for utf-8, utf-16-le and
utf-16-be (32,911 bytes, 400$x$markers, 400 paragraphs) and identical
8,688-character markdown. - Red/green. Your test against the unchanged base source: 2 failed, 1
passed (the utf-16-le and utf-16-be cases). Against the patched source: 3
passed. Wholetest_docx.py: base 2 failed / 101 passed / 1 skipped, head
103 passed / 1 skipped. Whole package suite (minustest_cu.py, which needs
the azure extra that is not installed here): head 28 failed / 949 passed / 3
skipped, base 28 failed / 946 passed / 3 skipped, and the failure sets are
identical - all network or provider dependent (18 intest_cli_vectors, 7 in
test_cli_misc, 2 intest_docintel, 1test_speech_transcription). - Formatting and mergeability. black 23.7.0 leaves both changed files
unchanged.git merge-tree --write-tree main headexits 0 (tree
3901dfa4). The branch is one commit behind main (#2601), which is what
mergeable_state: behindreflects. - Two bonus fixes confirmed. Documents whose declaration names a legacy
single-byte encoding now keep their equations (windows-1252 and iso-8859-1
declared with cp1252 bytes: base lost the equation, head keeps it), and the
"declared utf-8 but stored as UTF-16" case that used to fail the whole
conversion with anExpatErrornow converts. Re-serialization normalizes the
declaration toencoding="utf-8", so downstream Mammoth never sees a stale
declaration.
One risk I would guard before merging
BeautifulSoup's XML parser recovers from errors instead of raising, so when the
declaration disagrees with the bytes the parser silently drops part of the tree,
and pre_process_docx writes that partial document back into the zip. Four
inputs are worse on the patched code than on base (in all of them the
declaration does not match the bytes):
- declared
utf-16orutf-16le, bytes are utf-8: base converts
(Head <CJK>$x$Tail), head writes a 39-byteword/document.xmlcontaining
only the declaration and fails with
ExpatError: no element found: line 2, column 0. - declared
ascii, bytes are utf-8 with non-ASCII text: base returns the full
text plus math, head returns empty markdown - silent data loss, which is
worse than an exception. - declared
iso-8859-1, bytes are utf-8: base correct, head returns mojibake
for the accented characters. - UTF-16LE/BE without BOM (invalid per the XML spec, but produced in the wild):
base converted the text and dropped only the math, head fails the conversion.
A fallback keeps the benefit and removes the regression, for example:
soup = BeautifulSoup(content, features="xml")
if soup.find() is None or (not soup.get_text().strip() and content.strip()):
# The parser recovered from a declaration that does not match the bytes by
# dropping the document. Fall back to the previous UTF-8 decode so such
# files keep converting.
soup = BeautifulSoup(content.decode("utf-8", "replace"), features="xml")A regression test such as "declaration says utf-16, bytes are utf-8, document
still converts" would lock that in. I do not think this needs to block the fix:
those documents are malformed, the old code only coped by ignoring the
declaration, and silently returning empty output is the case I would most like
to see covered.
Smaller notes
_pre_process_strike(pre_process.py:119-125) still decodes as UTF-8 and
its fast pathb"dstrike" not in contentcan never match UTF-16 bytes, so
double strikethrough is silently not normalized in UTF-16 documents (probe:
the UTF-16 LE/BE variants render as plainstruck <CJK>, the utf-8 variant as
~~struck <CJK>~~). The same approach would close that gap; it is a
pre-existing issue, not something this PR needs to fix.- The CLA check is still queued and both workflow runs are
action_required
because this fork has not contributed before, so a maintainer has to approve
the runs before CI reports anything.
How this was produced
I used local detached worktrees of the base commit and the PR head, rebuilt
.docx fixtures from packages/markitdown/tests/test_files/test.docx with
word/document.xml re-encoded in each encoding, and ran both
_pre_process_math directly and the public MarkItDown().convert_stream path
over every variant above. AI assistance was used for the review and the local
experiments; all numbers quoted come from those runs on this machine.
Verdict
I would merge this: it fixes a real conversion bug with a one-line change,
the tests are honest about what they cover, and the failure mode it removes
(silently dropping equations) is user visible. The fallback above would make it
robust against self-inconsistent XML, and I would keep the _pre_process_strike
gap in mind for a follow-up.
|
LeonSGP (@LeonSGP43) please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
Summary
A valid DOCX whose word/document.xml is UTF-16 encoded loses its equations because math preprocessing decodes it as UTF-8. Parse the original XML bytes so the declaration and BOM are honored, preserving the original UTF-8 and BOM-prefixed UTF-16 LE/BE regression cases.
The bytes path also exposed a recovery problem: UTF-8 XML incorrectly declaring utf-16, utf-16le, or ascii can be silently truncated or serialized without a document root. Disable lxml recovery for the first BeautifulSoup bytes parse. If it rejects the XML, retain the original strict UTF-8 decoding path. If that decoding also fails, the existing outer fallback preserves the input bytes.
Ten additional real DOCX conversion cases check both inline and block equations with incorrect declarations and valid windows-1252/iso-8859-1 bytes. The ASCII case includes a 5000-character prefix, so a nonempty partial tree cannot satisfy the complete Markdown assertion. Correct legacy encodings continue to use their declaration. No new dependency is added.
Validation
Local macOS, Python 3.12.8, checkout imports verified for both the project .venv and Hatch environment:
The hosted Python/OS matrix and live cloud OCR/LLM providers were not exercised. Skips are not provider acceptance. CLA still requires the contributor's own action; no maintainer approval is claimed.
Scope and related work
Rechecked all 364 open PR file lists and the bodies/full diffs of the 15 PRs touching DOCX preprocessing, including #2515 and #2504. Their performance and XML-security changes address different causes. This update remains in the existing PR.
A syntactically valid legacy declaration paired with UTF-8 bytes can be ambiguous; this change does not guess an alternative encoding. BOM-less UTF-16 probes retain the baseline prose-preservation behavior but still omit equations. General malformed XML, strike handling, security parsing, and lists are outside this patch.
AI assistance
Developed with Codex using gpt-6.1-sol. Real DOCX reproductions, regression comparisons, checkout imports, complete local package checks, and the patch were verified locally.