Repository navigation
fix(epub): report an unreadable spine item instead of losing the book - #2509
Lukas (L4XB) wants to merge 2 commits into
Conversation
The spine loop had no error handling, so one chapter zipfile cannot decompress took the whole book with it. A bad CRC on chapter two returned a FileConversionException and chapters one and three with it, even though both were intact. Read each item before converting it and report the ones that fail. Reading first is what makes the failure attributable: z.open raises immediately for an encrypted entry, but a bad CRC only surfaces mid-read, inside the HTML converter's own call. The loop also now uses the zip_names set it already built rather than rebuilding namelist() for every item. A readable book converts byte for byte as before.
…pine-item main renamed test_epub_converter.py to test_epub.py (microsoft#2579). The two spine-item tests and their helper stay at the end of the archive href section, and main's __main__ block replaces the old one.
XU (kokokoXUY)
left a comment
There was a problem hiding this comment.
Measured locally on fb2264d against main 4cc9fa1, with a
three-chapter book whose middle chapter is damaged:
- bad CRC on a stored chapter: main raises "BadZipFile: Bad CRC-32 for file 'OEBPS/c2.xhtml'"
and the reader gets nothing at all; here the result is FIRST_BODY /
"This part could not be read: Bad CRC-32 for file 'OEBPS/c2.xhtml'" / THIRD_BODY, in spine
order, matching what the PR body claims. - encrypted chapter (flag bit 0): main raises "RuntimeError: File 'OEBPS/c2.xhtml' is
encrypted, password required for extraction"; here the chapter is reported in place. - compression method 99, which is the shape an AES entry has: main raises
"NotImplementedError: That compression method is not supported"; here it is reported. - no regressions found: a clean book, a spine item absent from the archive, a chapter whose
bytes are random noise, and a zero-length chapter all produce identical output before and
after.
Tests: packages/markitdown/tests/test_epub.py is 10 passed here and 8 passed on main; putting
the new test file on main leaves
test_unreadable_spine_item_does_not_cost_the_rest_of_the_book failing with
zipfile.BadZipFile: Bad CRC-32 (1 failed, 1 passed), so the new test is a real red test and its
reader-side precondition check does its job. The patch applies cleanly to current main and both
files are LF-only.
One gap, the same one that affects the ZIP converter in #2508: a corrupt DEFLATE chapter makes
zipfile raise zlib.error, which is not in the caught tuple (zlib.error inherits straight from
Exception, so zipfile.BadZipFile does not cover it). A chapter with one flipped byte inside its
deflated stream still costs the whole book here, exactly as on main: "Error -3 while
decompressing data: invalid code lengths set". Adding "import zlib" and zlib.error to the tuple
closes it - the same archive then converts with that chapter reported as "This part could not be
read: Error -3 while decompressing data: invalid code lengths set" and the other chapters intact
(measured by applying the change locally, then reverting it). Since #2508 carries the identical
tuple, it may be worth handling both in one pass.
The bug
EpubConverter.convertwalks the spine with no error handling at all, so one chapterzipfilecannot decompress takes the whole book with it.Measured on a three-chapter EPUB whose second chapter has one flipped byte, so its CRC
fails:
FileConversionException: EpubConverter threw BadZipFile with message: Bad CRC-32 for file 'OEBPS/c2.xhtml'**Title:** Book/CHAPTER ONE/This part could not be read: Bad CRC-32 for file 'OEBPS/c2.xhtml'/CHAPTER THREEChapters one and three were intact both times. Before, the reader got neither.
Same principle as #2508 for the ZIP converter, one converter over: a part that cannot be
read should cost that part, not the document.
The change
Read each spine item, then convert it:
Reading first is what makes the failure attributable.
z.open()raises immediately for anencrypted entry, but a bad CRC only surfaces mid-read — which, with the old streaming call,
happened inside
HtmlConverter.convert, where the EPUB converter has no idea which memberit came from. The four exception types cover the ways
zipfilerefuses a member: encryptedwithout a password, bad CRC, a compression method this build does not carry, and a truncated
entry.
The loop also now tests membership against the
zip_namesset the method already built forhref resolution, instead of rebuilding
namelist()once per spine item.A readable book converts byte for byte as before — that is what the second test pins.
Tests
tests/test_epub.py(renamed fromtest_epub_converter.pyin #2579), using the_build_epubhelper already there:test_unreadable_spine_item_does_not_cost_the_rest_of_the_book— three chapters, themiddle one damaged. It first asserts plain
zipfilereally does raiseBadZipFileon thatmember, so the fixture cannot quietly stop reproducing the bug, then asserts the surviving
chapters are present and in spine order, the damaged body is absent, and the failure is
named.
test_a_readable_book_carries_no_failure_note— the control, so the guard cannot startannotating healthy books.
The first fails on
main.