Repository navigation
fix(epub): recognize XML elements independently of namespace prefixes - #2672
Alex (ac1982) wants to merge 2 commits into
Conversation
|
@microsoft-github-policy-service agree |
PRABHU KIRAN VANDRANKI (VANDRANKI)
left a comment
There was a problem hiding this comment.
This is a community review. It does not clear the merge gate.
The prefix handling is a real improvement, but I found a regression for EPUBs that use a namespace other than the current EPUB 2/3 ones. Details below. I ran everything on Windows with Python 3.13, PR head against main 4cc9fa1.
What works (ran):
- The three new parametrized tests in
test_epub.pyfail on main (3 failed, 8 passed) and pass on the PR.tests/test_epub.pyon the PR: 11 passed. Filtering the whole test directory with-k epubon the PR: 12 passed. - I did not run the full suite, and I did not run it against a real-world EPUB that uses prefixed OPF elements.
Regression I reproduced. The old code used getElementsByTagName("item"), ("itemref") and ("dc:title"), so it matched whatever namespace the file declared. The new _get_elements first asks for the exact namespace and only falls back to elements with namespaceURI is None. An element that has a different non-empty namespace matches neither. I built minimal archives with one chapter and a dc:title/dc:creator:
| input | main | PR |
|---|---|---|
| standard EPUB 2/3 namespaces | title, author, chapter all found | same |
| container.xml with no namespace | all found | all found |
OPF default namespace http://openebook.org/namespaces/oeb-package/1.0/ (the OEB 1.2 package namespace) |
all found | title and author found, chapter text missing |
xmlns:dc="http://purl.org/dc/elements/1.0/" |
title and author found | title None, author missing |
xmlns:dc="http://purl.org/dc/elements/1.1" (no trailing slash) |
title and author found | title None, author missing |
The first of those is the worst outcome: the conversion succeeds with an empty body, which is the same silent loss this PR is trying to fix for prefixed elements. I do not know how common these files are. OEB 1.2 predates EPUB 2, so I expect them to be rare. The trailing-slash variant is a malformed file, but main tolerated it. If you want to keep the old leniency, one option is to fall back to matching on localName when the namespace lookup finds nothing, instead of only when namespaceURI is None. I have not tried that change.
Smaller notes:
- The new test is placed after the
if __name__ == "__main__": raise SystemExit(pytest.main([__file__]))block. Under pytest it is collected fine (I confirmed that), but running the file directly would exit before it is defined. - In
_get_all_texts_from_nodes,tag_name.removeprefix("dc:")hardwires every caller to the Dublin Core namespace. I checked that the existing callers inconvertonly passdc:tags; I did not audit anything outside this file.
|
Thanks for the concrete compatibility cases. I reproduced the OEB 1.2 and both Dublin Core failures, plus an alternate container URI, at the previous PR head (4 failed, 11 passed). The fallback now uses the original tag names when standard-namespace matching finds nothing, preserving the old tolerance while retaining arbitrary-prefix support for standard namespaces. Added six compatibility cases including the two unnamespaced controls, and moved the direct-test entry point to the end. All 17 EPUB tests pass, including direct file execution. The text helper only serves the existing Dublin Core callers. Full suite: 1103 passed, 12 skipped, one failure for a missing local FLAC executable. After installing FLAC, that test and the EPUB file passed together (18 passed). Black and diff checks pass. I updated the PR description with those exact results and the remaining platform/real-file limits. |
EPUB conversion depends on XML prefix spelling. A container using
<ocf:rootfile>raisesIndexError; a package using<opf:item>/<opf:itemref>silently loses chapter content; changing the Dublin Coredc:prefix drops metadata.Resolve elements by namespace URI and local name. If the standard namespace has no matching elements, retain the previous exact tag-name lookup (
rootfile,item,itemref, ordc:…). This preserves the existing tolerance for missing, legacy and malformed namespace URIs without matching arbitrary foreign local names. The private text helpers are only used for Dublin Core fields.Three generated-archive regressions cover container/package/metadata prefixes and spine ordering. Following review, six additional cases cover the OEB 1.2 package namespace, Dublin Core 1.0, the missing-slash DC URI, an alternate container URI, and unnamespaced container/package elements. The first four fail at the previous PR head, including silent chapter/metadata loss; all pass after restoring the fallback. The test entry point is now below all test definitions.
Validation on Python 3.12 / Linux arm64:
PYTHONPATH=packages/markitdown/src .venv/bin/python -m pytest packages/markitdown/tests -q: 1103 passed, 12 skipped, 1 failed because the local FLAC executable was missing.test_epub.pyalso collected and passed all 17 cases. The full suite was not rerun after installing the dependency.python -m pre_commit run --all-files: passed (Black 23.7.0);git diff --check: passed.Searched existing issues and all-state PRs for EPUB namespace/prefix/metadata namespace; no matching fix found. Prepared with AI assistance. Community review does not clear the maintainer merge gate.