Skip to content

[TIKA-4814] Retrieve objects and blobs from onenote in dom order - #3018

Open
henry-lindeman-glean wants to merge 9 commits into
apache:mainfrom
henry-lindeman-glean:hmlin-fix-onenote-squash
Open

[TIKA-4814] Retrieve objects and blobs from onenote in dom order#3018
henry-lindeman-glean wants to merge 9 commits into
apache:mainfrom
henry-lindeman-glean:hmlin-fix-onenote-squash

Conversation

@henry-lindeman-glean

Copy link
Copy Markdown

Thanks for your contribution to Apache Tika! Your help is appreciated!

Before opening the pull request, please verify that

  • there is an open issue on the Tika issue tracker which describes the problem or the improvement. We cannot accept pull requests without an issue because the change wouldn't be listed in the release notes.
  • the issue ID (TIKA-XXXX)
    • is referenced in the title of the pull request
    • and placed in front of your commit messages surrounded by square brackets ([TIKA-XXXX] Issue or pull request title)
  • commits are squashed into a single one (or few commits for larger changes)
  • Tika is successfully built and unit tests pass by running ./mvnw clean test
  • there should be no conflicts when merging the pull request branch into the recent main branch. If there are conflicts, please try to rebase the pull request branch on top of a freshly pulled main branch
  • if you add new module that downstream users will depend upon add it to relevant group in tika-bom/pom.xml.

We will be able to faster integrate your pull request if these conditions are met. If you have any questions how to fix your problem or about using Tika in general, please sign up for the Tika mailing list. Thanks!

Tested by running java -jar tika-app/target/tika-app-4.0.0-SNAPSHOT.jar --text Downloadme.onepkg on this file (renamed so I can upload it lol)
parsing-test.zip

Before (on simpler version with only the first three pages):

❯ 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

After

❯ java -jar ~/Glean/tika/tika-app/target/tika-app-4.0.0-SNAPSHOT.jar --text parsing-test.zip
INFO  [main] 16:11:14,757 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.
INFO  [main] 16:11:19,031 org.apache.tika.parser.ocr.TesseractOCRParser Tesseract is installed and is being invoked. This can add greatly to processing time.  If you do not want tesseract to be applied to your files see: https://cwiki.apache.org/confluence/display/TIKA/TikaOCR#TikaOCR-disable-ocr

Users/hmlin/Glean/datasets/onenote/Downloadme/parsing-test/Test section.one
Page

Wednesday, August 12, 2026

2:29 PM

Image below



fiew Help Q Tell me what you want to do

vuvy BIU?2Y Av Aes

Page

ap GEARED Wednesday, August 12,




/iew
Help
Tell me what you want to do
V
11
v
BIU QVA A .
Page
+ Add page
Wednesday, August 12,

Image above





Users/hmlin/Glean/datasets/onenote/Downloadme/parsing-test/parsing-test.onetoc2


Users/hmlin/Glean/datasets/onenote/Downloadme/parsing-test/Section-1.one
Page 1

Wednesday, August 12, 2026

10:24 AM

Text

More text

Bold Text

Italic Text

Underlined test

Red text

Highlighted text



----------------------------------------

Page sdskgjhsdlf

Wednesday, August 12, 2026

10:26 AM

Bullet 1

Bullet 2

Indented bullet

Indented square

Number 1

Number 2

Letter a

Numeral I

Outdented number

More text

Table cell 1a

Table cell 4a

Table cell 2b

Table cell 3b

Table cell 4b

Comic sans





Users/hmlin/Glean/datasets/onenote/Downloadme/parsing-test/Section-2.one
Page 1

Wednesday, August 12, 2026

10:28 AM

Idk



----------------------------------------

Pictures or something

Wednesday, August 12, 2026

10:29 AM



Table 2. Basic dimensions for car side counterweight layouts with speed up to 1.75 m/s

tar

Le a nr a a
(ka) (m) BB x DD (mm) type: (ey! (mm) FWi FW2 FWi FW2 WW wo” WW WD
Sar a ep Cr to Saar
eS Ce gee Cae a at eee
emo Bea EL eae ee sl a al
stn eee at ea ee

ee ime ee Pee me tae ae Toe Coe ae i aoe ele

ieee Fea eH ie ee a es ee eB

pen HRB pe Hi a
ep (ie re Tee eat ae ian ae oe ee at oe ele

wenam PROTEST cco Lee teat ae aS a a Tet a
Be ear se Ce ee ee rae ae ae Ce ae st eal

a pet er eae a om a ee

Tem rive ERE CEI wee HSH eee ae et et
be reer| se Parte Cee tree ee tee Cae ee et ee

HE ec EH a ae a i

a ee ee ne ee ee

en EER CET re Fe tga tm ae a ee et oe al
SE te a
eee ae Peete et ae ae ee et ee
Tense HEE ap Heh Cee ie aaa ee oe ee
ee ee eae eee

pee ER te ee et ee
a

A eg a

seenee HEE 2 FRE as ee Ce ar eae
Se ee eee ee ee ete eee

a aco EEE tea eet ret tae tai aa

sem ee a ae aie meee el

we | fa an i ae ee ee eel

wea ce Pet ge me a re ee a
veeterono EaR| O FCEEE e Sa
See ae en ee ee ee ee
tetas Ca ge a Ce ae ge ae a

tem ia oe Can ae er ie ee a lt ale

ee ee ee
ee a ee eee ae ae ea ee

eee ae re eae

mene Poet] Eee tee eee ie ee et a
Soi) Cee ee ie ae re ee ee

oa ee ee a en ee PE

TH ee Ce ae ae Ce ae ae ae et a ele
nasnem ae“? Cee tae eh ie oe ae et a el
moi) Cah eee a Cie ae Ca re ee et re ei

em A Pe
RR eB re Leet ete aa re ie a
eee eae re eae eee ae ee Cae et ae ees
oe ne, ee EE
eae ee ee
A eee ee

weenam REET oxo Petes teeta aee oe atc fet te et
wet | ea ae ein ae aa re a i ea

Tee ne RE CRERT wee ESTES ie age ie at a
top ige| a ae ae ge aa ea it ei

eer] 2 Seta aera (ie Cae eer a





This is a picture of a table

Here's some floating text



----------------------------------------

Very wide

Wednesday, August 12, 2026

10:31 AM

Hello

Hello

Hello

Hello

Images of the pages in the notebook
image
image
image
image
image
image

(the last page is added as a test fixture in this PR - testOneNoteEmbeddedImage.one)

It appears that the earlier behavior was to only take the last item on each page? (and since the last item on s2p1 is a drawing it gets null text there)

Signed-off-by: Henry Lindeman <henry.lindeman@glean.com>
@tballison

Copy link
Copy Markdown
Contributor

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.

@tballison

Copy link
Copy Markdown
Contributor

We definitely need to improve onenote parsing. Thank you for leading the effort.

@nddipiazza

Copy link
Copy Markdown
Contributor

going to resolve this conflict, apply a code review, fix any issues i see, then merge

@nddipiazza nddipiazza mentioned this pull request Aug 13, 2026
@tballison

tballison commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

@henry-lindeman-glean and @nddipiazza I've put a zip in google drive. @henry-lindeman-glean can you send me your gmail or similar?

@henry-lindeman-glean

Copy link
Copy Markdown
Author

oops; updated in my gh profile

Signed-off-by: Henry Lindeman <henry.lindeman@glean.com>
…te-squash

Signed-off-by: Henry Lindeman <henry.lindeman@glean.com>
@henry-lindeman-glean

Copy link
Copy Markdown
Author

@tballison I think I addressed all the agent review concerns (also ran several rounds of ai review myself). Can I get another look? lmk if you want me to break it up / re-squash. I also have some code for handling lists and tables that I've left out for another PR since this one was getting big.

@tballison

Copy link
Copy Markdown
Contributor

Sounds good. I'm focused on the 4.0.0 release in the next couple of days. The good news is that I don't think main will be changing much and causing you merge conflicts. 🤣

If @nddipiazza has a chance to look that'd be great, if not, I'll probably have time towards the end of this week or maybe next.

It is not small and will take time+tokens to review.

I think we should also run some fuzzers against it with the files I shared as seeds.

Signed-off-by: Henry Lindeman <henry.lindeman@glean.com>
@tballison

Copy link
Copy Markdown
Contributor

Did another deep dive. Changes are really good. Found a few more things via Claude:

1. The headline O(n²) fix isn't one on the package side (confirmed by skeptic). findStorageIndex*Mapping calls indexStorageMappings() on every lookup
     (MSOneStorePackage.java:205-253), which rebuilds two full key lists and list-equals them before the O(1) get — still O(n) per lookup with more allocation
     than the old linear scan; lookup count scales with cells + chain revisions, so still quadratic overall. Production never mutates the mapping lists
     mid-parse; the staleness machinery exists only to satisfy MSOneStoreParserTest.testStorageMappingIndexesSeePublicListUpdates, a mutation-visibility
     contract the test itself invented. Fix: build once (or identity/dirty check), delete both key classes + indexed* fields (~70 lines), drop/rewrite the
     pinning test. The parser-side maps are genuinely fixed.
  2. CHANGES.txt: duplicate TIKA-4327 entry (main already has one, line ~548); ~250 lines of trailing-whitespace churn on historical sections; the two new
     entries themselves add trailing whitespace. Keep only the TIKA-4814 entry.
  3. Silent-empty modes need one-line observability (skeptic-endorsed shape): keep the all-or-nothing fallback design (partial fallback would dump stale
     superseded objects), but add LOG.warn + a parse-warning when a root declare fails to resolve / a referenced object group is missing, upgrade the
     live-content cell skips (MSOneStoreParser.java:283,289) from DEBUG to WARN, and add one WARN in OneNoteParser.java:171 before the legacy dump. Today a
     damaged file can parse "successfully" to an empty body with zero signal.
  4. Two factually wrong javadocs: EmbeddedResourceInfo carries PropertyAction's description (MSOneStorePackage.java:599-602);
     ObjectDataBLOBDataElementData.java:39-45 says "returns the length" on a deserialize method. Plus the parser-side find* javadocs still lack the nullability
     note their package-side twins got.

  Cheap test additions worth requesting

  - Markup is pinned nowhere — every test uses text handlers, so the div-balance fix (prior Tier-1 #2) is unpinned. Swap one synthetic walk to
    ToXMLContentHandler, assert class="page" count/balance and closure-on-throw.
  - removeSupersededObjects untested (the test groups contain no objects to supersede).
  - Real fixture: embedded-image test isn't exactly-once (assertFalse(isEmpty()) passes on double extraction — the exact prior complaint); page order never
    asserted on a real file; and the synthetic order test passes trivially if "page one" is dropped entirely (indexOf = −1) — I verified this one myself; add
    assertContains first.
  - Depth caps on collectActions/collectReferencedCells unpinned (only walkObject's is); a mixed root-resolution test pinning the chosen fallback behavior.
  - ORIGINAL_AUTHORS asserted nowhere; two CREATOR assertions depend on HashSet iteration order.

Signed-off-by: Henry Lindeman <henry.lindeman@glean.com>
Signed-off-by: Henry Lindeman <henry.lindeman@glean.com>
@tballison

Copy link
Copy Markdown
Contributor

@henry-lindeman-glean any feedback on the jazzer skill? That's great that you ran Jazzer!

@henry-lindeman-glean

Copy link
Copy Markdown
Author

seems pretty good. just found another stack overflow lol

@tballison

Copy link
Copy Markdown
Contributor

Oh, so you didn't use the skill? Oh, well...

@henry-lindeman-glean

Copy link
Copy Markdown
Author

didn't realize there was one. um. will try that next

@tballison

Copy link
Copy Markdown
Contributor

https://github.com/apache/tika/blob/main/.skills/oss-fuzz/SKILL.md

Let me know how your agent does with it. Please take a look at the others, and please consider opening PRs to improve those.

See AGENTS.md for an overview of what we have so far.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants