krickert commented on PR #2921:
URL: https://github.com/apache/tika/pull/2921#issuecomment-4907456263
@nddipiazza great feedback..
> ## Review: e2e test coverage for the new typed `Document` contract
> Reviewed the design overall — the shift from mirroring Tika's metadata
taxonomy in protobuf to a small, stable `document.proto` (208 lines) plus
per-parser mapper code is a solid answer to the #2916 feedback. A few things
worth addressing before merge:
>
> ### Testing gap: e2e coverage does not exercise the new contract
> The only change in `tika-e2e-tests/tika-grpc` is a 2-line fix in
`HandlerTypeTest.java`:
>
> ```diff
> - String htmlContent = htmlReply.getFieldsMap().get("X-TIKA:content");
> + String htmlContent = htmlReply.getDocument().getMarkdown();
> ```
>
> (same for the TEXT handler case). This is a compile-fix to keep the
pre-existing assertion working against the new API — it only proves
`document.markdown` is non-empty over a real gRPC round-trip for two handler
types.
>
> It does **not** exercise, end-to-end, through the live server:
>
> * `DocumentMetadata` typed fields (title/authors/dates/counts) — no e2e
assertion anywhere
> * `extra` tagged-tail typing (int/bool/date/string) — no e2e assertion
> * `blocks` structured content tree (headings/tables/lists/code) —
untouched by any e2e test
> * `embedded` recursion for container formats (e.g. an Office doc with an
embedded image) — no e2e coverage at all
> * `format_category` routing hint — untouched
>
> The other e2e tests (`FileSystemFetcherTest`, `IgniteConfigStoreTest`,
`ExternalTestBase`) only ever inspect `getFetchKey()`/`getStatus()` on
`FetchAndParseReply` — none of them look at `getDocument()`, so the
bulk-corpus/streaming/ignite e2e paths give zero signal on the new typed
contract.
>
> The real proof of correctness for the mapping logic lives entirely in
`tika-grpc-mapper`'s unit tests (`DocumentBuilderTest`,
`MarkdownBlockTreeBuilderTest`, one test class per transformer), which feed
fixture-derived `Metadata`/markdown into `DocumentBuilder` in-process. That's
good for the mapping logic itself, but it bypasses the actual gRPC wire
serialization, the live server (`TikaGrpcServerImpl`), the pipes client, and
fetcher plumbing entirely.
>
> **Ask**: add (or extend `HandlerTypeTest`) at least one e2e case per
format that fetches a real file through the live gRPC server and asserts on
`document.metadata` (a couple of typed fields), `document.extra` (at least one
tagged key), and one `document.embedded` case (e.g. an Office/PDF file with an
embedded image) — not just that `markdown` is non-empty. Right now this new,
larger surface area has no live-server coverage at all.
>
> ### Other findings from code inspection
> 1. **Likely-dead error path**: `DocumentBuilder.build()`'s `primary ==
null` branch (returns `FAILED` with "No metadata returned from parse") looks
unreachable in production — the only caller, `TikaGrpcServerImpl`, always
passes a non-null `Metadata` (`tikaMetadata = new Metadata()` as fallback when
`emitData()`/metadata list is empty). Worth double-checking this is
intentional, since a real fetch failure with no emit data currently produces an
"empty-but-success-shaped" `Document` rather than hitting this explicit error
branch.
> 2. **Duplication**: 7 of 8 format transformers (`Generic`, `Pdf`, `Html`,
`Image`, `Office`, `Rtf`, `Epub`) repeat an identical block mapping
TITLE/DESCRIPTION/CREATOR/SUBJECT/LANGUAGE/CREATED/MODIFIED. Consider factoring
this into a shared helper in `TransformSupport` called by each transformer,
keeping only the format-specific additions per-transformer.
> 3. Minor: `parseTimeMs` in `TikaGrpcServerImpl` measures fetch+parse
combined (timer starts before `pipesClient.process()`), though the field's
doc/intent reads as parse-only time.
>
> Nice work on the overall shape of the contract — the main blocker from my
read is the e2e coverage gap above.
My bad; the e2e suite really was only asserting "markdown is non-empty".
Added three live-server cases to `HandlerTypeTest`:
- **PDF** (`testPDF.pdf`): asserts typed `document.metadata` (title, page
count), a boolean-typed `document.extra` key (`pdf:encrypted`),
`document.blocks`, and `format_category`.
- **HTML** (`sample.html`): asserts typed title, `FORMAT_WEB`, and a
string-typed extra key.
- **Embedded** (`test_recursive_embedded.docx`): asserts `FORMAT_OFFICE`,
typed created/modified dates, and that `document.embedded` recurses with
per-child metadata.
Two things fell out of writing these:
1. The extra-tail typing was actually broken over the live server -
`MetadataTagger` relies on `Property.get(key)`, but Tika's property registry is
populated lazily by class init, so in the server JVM `pdf:encrypted` came back
as a *string*, not a boolean. The unit tests never caught it because they
happen to load the vocabulary classes first. Fixed by force-loading the
tika-core metadata vocabularies in a static initializer in `MetadataTagger`,
and the new e2e test now pins the behavior.
2. The e2e configs needed `grpc.allowComponentManagement` /
`allowPerRequestConfig` opt-ins after TIKA-4764 landed on main, otherwise every
test dies with `PERMISSION_DENIED` - updated the three config JSONs.
Note the CI e2e job doesn't currently execute these (the `-pl tika-e2e-tests
-am verify` invocation only builds the aggregator POM and no plugins are
provisioned) - that's a pre-existing infra gap, but the suite passes locally
against a live server.
--
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]
