[
https://issues.apache.org/jira/browse/TIKA-4814?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18104607#comment-18104607
]
ASF GitHub Bot commented on TIKA-4814:
--------------------------------------
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.
```
> Onenote parsing only takes last item on each page
> -------------------------------------------------
>
> Key: TIKA-4814
> URL: https://issues.apache.org/jira/browse/TIKA-4814
> Project: Tika
> Issue Type: Bug
> Components: parser
> Affects Versions: 3.3.2
> Environment:
> {code:java}
> ❯ java --version
> openjdk 26.0.2 2026-07-21 OpenJDK Runtime Environment Homebrew (build 26.0.2)
> OpenJDK 64-Bit Server VM Homebrew (build 26.0.2, mixed mode, sharing)
> ❯ uname -a
> Darwin Henrys-MBP.localdomain 25.6.0 Darwin Kernel Version 25.6.0: Sat Jul 11
> 15:25:34 PDT 2026; root:xnu-12377.161.13~4/RELEASE_ARM64_T6020 arm64{code}
> Reporter: Henry Lindeman
> Priority: Major
> Attachments: Downloadme.onepkg
>
>
> I made a test onenote file and tried to parse it with the cli as described in
> the tika readme, and it only showed the last element on each page
> {code:java}
> ❯ java -jar tika-app/target/tika-app-4.0.0-SNAPSHOT.jar --text
> ../datasets/onenote/ToDownload/Downloadme.onepkg
> INFO [main] 12:56:37,927 org.apache.tika.cli.TikaCLI As a convenience,
> TikaCLI has turned on several non-default features
> as specified in
> tika-app/src/main/resources/tika-config-default-single-file.json.
> See: TIKA-2374, TIKA-4017, TIKA-4354 and TIKA-4472).
> This is not the default behavior in Tika generally or in tika-server.
> Downloadme/Open Notebook.onetoc2
> Downloadme/Untitled Section.one
> Highlighted Text
> Comic sans
> Downloadme/Section 2.one
> {code}
> onedrive sharing link:
> [Downloadme|https://1drv.ms/o/c/98705a8ca0add89e/IgBEW7OVk9slSKq8sJeJUTkbAQ6K3mNiiRNLGRNh5sniyq0?e=g3aSgg]
>
> I also vibe-coded a fix for this. lmk if you want it.
> [https://github.com/henry-lindeman-glean/tika/pull/1]
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
