tballison commented on PR #3018:
URL: https://github.com/apache/tika/pull/3018#issuecomment-5284793033
Thank you for this PR.
I had an agent review it. The most terrifying bit was that a new
ParseContext is being built.
Results:
```
The single most important thing found, which the PR description doesn't
mention: DataElement builds its type map by Class.forName on the enum constant
name.
ObjectDataBLOBDataElementData did not exist, so
DataElementType.ObjectDataBLOBDataElementData(10) had no mapping and threw,
aborting the whole package parse. Every OneDrive/365
OneNote file containing an embedded image or file was producing nothing
but a raw string dump. Three reviewers reached this independently. That belongs
in the JIRA and CHANGES.
Measured effect of the walk rewrite: testOneNoteFromOffice365-2.one 3 → 12
emitted strings, testOneNoteFromOffice365.one 8 → 14, with nothing the old code
emitted lost.
Build: green. 518 tests, 0 failures; checkstyle 0; rat 0 unapproved; tree
clean after spotless:apply.
---
Tier 1 — fix before merge
┌─────┬───────────────────────────────────────────────────────────────────────────────────────────────────────────────┬────────────────────────────────┬──────────────────────────┐
│ # │ Finding
│ Where │
Reviewers │
├─────┼───────────────────────────────────────────────────────────────────────────────────────────────────────────────┼────────────────────────────────┼──────────────────────────┤
│ 1 │ ArrayNumber.number is a raw int32 from 4 file bytes used directly
as an ArrayList-append loop bound → FF FF │ MSOneStorePackage.java:491,501 │
security (I verified) │
│ │ FF 7F in a 2KB file = OOM. OutOfMemoryError is an Error, so catch
(Exception) does not catch it │ │
│
├─────┼───────────────────────────────────────────────────────────────────────────────────────────────────────────────┼────────────────────────────────┼──────────────────────────┤
│ 2 │ <div class="page"> opened, walkCell runs, endElement follows — no
try/finally. Any throw leaves it open; the │ :227-229 │
security + correctness │
│ │ fallback then dumps legacy strings inside it → invalid XML /
StrictXHTMLValidator failure │
│ (I verified) │
├─────┼───────────────────────────────────────────────────────────────────────────────────────────────────────────────┼────────────────────────────────┼──────────────────────────┤
│ │ walkCell's if (visited.isEmpty()) fallback is all-or-nothing.
walkObject adds to visited before the │
│ │
│ 3 │ propertySet == null check, so one resolving root (even a BLOB with
no property set) disables the fallback → │ :349 │
correctness │
│ │ entire page body lost on partial root resolution
│ │
│
├─────┼───────────────────────────────────────────────────────────────────────────────────────────────────────────────┼────────────────────────────────┼──────────────────────────┤
│ │ When dataRootCell == null, splitCells promotes every cell to a
page. Measured: 2 pages/12 strings → 4 pages, │
│ │
│ 4 │ Section1Page1Content twice, a deleted page resurrected. Directly
in tension with the parser's new │ :267-271 │
correctness │
│ │ null-tolerance
│ │
│
├─────┼───────────────────────────────────────────────────────────────────────────────────────────────────────────────┼────────────────────────────────┼──────────────────────────┤
│ 5 │ parseCell returns null on 5 conditions and the caller skips.
Pre-PR these NPE'd → legacy dump → │
MSOneStoreParser.java:186-206 │ usability │
│ │ degraded-but-non-empty. Now: no exception, no logger, empty body +
successful parse │ │
│
├─────┼───────────────────────────────────────────────────────────────────────────────────────────────────────────────┼────────────────────────────────┼──────────────────────────┤
│ 6 │ hasPrimaryPicture keys off reference presence, not resolvability —
a dangling PictureContainer suppresses the │ :401-408 │
correctness + docs │
│ │ WebPictureContainer14 fallback and no image is extracted. The
comment promises the opposite │
│ │
├─────┼───────────────────────────────────────────────────────────────────────────────────────────────────────────────┼────────────────────────────────┼──────────────────────────┤
│ 7 │ Unbounded recursion depth (cycle guard is complete; depth cap
absent) → StackOverflowError, also an Error, │ :377-426, :316-335
│ security + correctness │
│ │ also escapes
│ │
│
├─────┼───────────────────────────────────────────────────────────────────────────────────────────────────────────────┼────────────────────────────────┼──────────────────────────┤
│ 8 │ 3-arg walkTree fabricates new ParseContext() → no ParseRecord →
embedded limits skipped entirely, default │ :197-201
│ all five non-correctness │
│ │ AutoDetectParser installed, caller's
DocumentSelector/FilenameFilter/PasswordProvider discarded │
│ reviewers + correctness │
└─────┴───────────────────────────────────────────────────────────────────────────────────────────────────────────────┴────────────────────────────────┴──────────────────────────┘
Finding 8 is the strongest consensus item in the review. It's public API
on an OSGi-exported package with zero in-tree consumers; 4.0.0 is the moment to
delete it.
---
Tier 2 — before the 4.0 freeze
- handleEmbedded catches only IOException (:663).
EmbeddedLimitReachedException (RuntimeException) and WriteLimitReachedException
(SAXException) escape to the swallowing catch →
user's configured limit produces string-dump garbage instead of a clean
stop. 3 reviewers.
- Bare Metadata on embedded docs (:653). No RESOURCE_NAME_KEY, no
EMBEDDED_RESOURCE_TYPE → FilenameFilter gating silently inert, /rmeta shows
embedded-1. The names are right there in
OneNotePropertyEnum.ImageFilename/EmbeddedFileName. 3 reviewers.
- Drop PAGE_SEPARATOR (:98,221). 4 reviewers. I confirmed div is in
XHTMLContentHandler.ENDLINE:46, so plain-text output already gets a newline —
this is a free deletion, not a
trade-off. MSOneStorePackageTest.java:90 pins it, so that assertion goes
too.
- O(n²) linear scans (MSOneStoreParser.java:273 + four find* helpers).
Measured 572 → 4,211 comparisons on a 70KB file; quadratic in revision count.
The PR already built
objectBlOBElementsById for BLOBs — do the same for object groups.
- Per-cell seenObjectGroupIds → 67 object-group instantiations for 40
distinct IDs on that same file.
- collectSectionReferencedCells sweeps unconditionally (:308-312) where
walkCell guards. Deleted pages can resurface and mask a current cell.
- Two find* methods went never-null → nullable with unchanged javadoc.
---
Maintainer decisions, not mechanical fixes
- dc:creator now includes original authors (:603-606) — measured {Du
Chang, Chang Du}. Defensible (the old sticky booleans were a real bug) but
untested on this path.
- Encrypted sections: base-revision groups whose own manifest lacks the
encryption root are now parsed as property sets rather than opaque. No
encrypted fixture exists — needs a run
to confirm it neither emits garbage nor throws.
- Split the PR? The API reviewer recommends narrow: the BLOB classes +
document-order walk are the fix and are low-risk; the EmbeddedDocumentExtractor
wiring carries findings 8, 9,
10 and could land separately.
- Binary fixture provenance — 52KB externally contributed .one;
rat-excluded, so no gate fires. Worth a one-line confirmation from the author.
---
Settled — do not re-raise
Several suspicious-looking things were checked hard and came back clean:
- removeSupersededObjects index bookkeeping is sound. Two reviewers traced
it independently.
- collectActions cursor arithmetic is correct — verified empirically
across all 260 property-set objects in the fixtures: 0 mismatches. ContextIDs
correctly consume neither cursor.
- CellID.extendGUID2 really is the object space — confirmed on real data
(four extendGUID1 values sharing one extendGUID2, same root object IDs).
- The changed timestamp expectation is more correct. 1623597638000 traces
to object 42:fabe12b6-… reached via root role 4 of the current page cell — not
a dropped snapshot. The old
value came from a stale metadata object.
- effectiveRootDeclares newest-wins, base-revision chain oldest-first,
HashMap ordering stable — all match their comments.
---
Hygiene
CHANGES.txt entry missing (draft available). ~15 comment-terseness
offenders. Dead code: dataRoot (already write-only at base commit), 3-arg
createInstance, objectBlOBElements; new
field objectBlOBElementsById copies the typo'd casing. Test gaps: nothing
pins document order (the headline claim), nothing pins the markup,
removeSupersededObjects and the
AuthorRole rewrite are untested, and the embedded-image test would pass if
the image were extracted twice.
```
--
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]
