Skip to content

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

Open
henry-lindeman-glean wants to merge 7 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 7 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>
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