tballison commented on PR #3018:
URL: https://github.com/apache/tika/pull/3018#issuecomment-5347737204
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.
```
--
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]
