tballison commented on PR #3520:
URL: https://github.com/apache/tika/pull/3520#issuecomment-6639552072
A couple of important finds and some that are meh? Let me know what you
think.
```
Contract (fix first)
1. A hybrid file's book text comes out twice. Three reviewers found this
independently.
- Most kindlegen .mobi files are hybrids, carrying the same book in
both halves. The second (KF8) half is emitted
as an embedded document with full text, so /tika and text output
contain the book twice, /rmeta has two
near-duplicate documents, and the work is done twice.
- It's marked ALTERNATE_FORMAT_CHUNK, which in Tika means OOXML
altChunk: content that is part of the document,
not a second copy.
- The test fixture hides this because its two halves have different
text.
- Suggested shape: parse the first half by default and make the KF8
half opt-in, the way
RFC822Parser.extractAllAlternatives works.
2. The hybrid rebuild gets the record numbering backwards. I checked this
against KindleUnpack's source.
- In a real hybrid, the second header's record-index fields count from
the KF8 record 0; KindleUnpack adds that
record's index to each of them. The PR assumes they count from the
start of the file and subtracts.
- Most fields therefore become "none". A hybrid whose second half uses
HUFF/CDIC compression would lose its text.
- The generator makes the same assumption, so the test confirms the bug.
- The PR says it matched a real 19-file sample. PalmDOC hybrids hide
the bug because nothing they need reads
those fields yet.
3. KF8 text includes the CSS and SVG. The parser ignores the FDST record,
which marks where each content flow starts
and ends. Raw stylesheet rules after </html> end up as book text. The
fix depends on #2.
4. Encrypted books never set tika:encrypted. Recording a warning instead
of throwing (the settled choice) skips the
code that normally sets the flag. Setting it explicitly fixes this.
5. TikaCLITest.testSupportedFormatsPartialIsCurrent will fail. I confirmed
the generated
docs/.../partials/supported-formats.adoc has no mobi entry; it needs
regenerating.
Edge cases
6. Small files can force huge HTML parses. With HUFF/CDIC, a file of about
45 KB can decode to the 64 MB text cap,
and jsoup then builds roughly a 2 GB tree in memory. Two things make it
worse:
- The HTML pass writes metadata into new Metadata() instead of
Metadata.newInstance(context), so repeated <meta>
tags skip the write limiter and hit the quadratic Metadata.add path.
The estimate is about an hour of CPU,
extrapolated from component timings, not run end to end.
- Fix: use newInstance(context) and lower the default cap, or tie it to
the expansion ratio.
7. The number of EXTH metadata records is unbounded. Header metadata
entries are limited only by record 0's size,
and each value is added to metadata (quadratic). Roughly 10 minutes of
CPU for a 10 MB file, and out-of-memory
near the 256 MB cap. Cap the record count and the number of values per
key.
8. A hybrid holds about 4–5 copies of the book in memory at once, and
builds them before asking whether embedded
documents should be parsed at all. Ask first, then build the rebuilt
file straight from slices.
9. One bad HUFF code loses all the text decoded so far, even from records
that decoded fine. Four reviewers flagged
it. It contradicts the PR's own "read what's there" policy for
truncated and otherwise broken books.
10. Some .prc/.azw files go from empty success to an exception. A Palm app
or PalmDOC .prc, or a Topaz or KFX .azw
(format details unverified), is routed here by file extension alone
and now throws. Return metadata only, or
document it. Plain PalmDOC books would be a cheap follow-up, since the
decompressor is already there.
11. .azw files now detect as vnd.amazon.ebook instead of
x-mobipocket-ebook. This affects anyone routing on the
type. It needs a CHANGES line and a detection test (BOOKMOBI bytes
plus a .azw/.azw3 name).
12. The 256 MB cap is hard-coded and undocumented, and a book over it
returns no metadata at all. Comic and manga
books can plausibly be that large (unverified).
13. Dates: EXTH 106 dates are parsed by hand instead of with TikaDates.
Date-only values gain an invented midnight
UTC time, and non-ISO dates are silently dropped.
14. Unknown or zero codepage falls back to UTF-8. KindleUnpack uses
windows-1252, so old .prc files would get
garbled accented characters.
15. Test gaps:
- The 256 MB cap has no test; a sparse file would cover it without a
256 MB fixture.
- The fuzz test never feeds mutated input into the hybrid's KF8 half
and has no @Timeout.
- Two tests suppress exceptions without asserting there wasn't one.
- Untested: EXTH 524 taking precedence over the header locale, stored
(uncompressed) text, the title fallback,
and a positive cde-type.
```
--
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]
