[
https://issues.apache.org/jira/browse/TIKA-4766?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18094373#comment-18094373
]
ASF GitHub Bot commented on TIKA-4766:
--------------------------------------
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.
> Replace tika-grpc fields map with a typed Document parse contract
> -----------------------------------------------------------------
>
> Key: TIKA-4766
> URL: https://issues.apache.org/jira/browse/TIKA-4766
> Project: Tika
> Issue Type: New Feature
> Components: tika-pipes
> Affects Versions: 4.0.0
> Reporter: Kristian Rickert
> Priority: Major
> Labels: grpc, pipes, protobuf
>
> Replace the flat {{FetchAndParseReply.fields}} map ({{map<string,string>}})
> with a typed parse result.
> Approach (reshaped from the original {{ParseResponse}} design after review in
> [PR #2916|https://github.com/apache/tika/pull/2916]): a single small
> {{Document}} proto (~200 lines) rather than per-format metadata messages.
> * *Content*: a structured markdown block tree -- headings, paragraphs, lists,
> tables, code blocks, inline runs (CommonMark + GFM) -- plus the rendered
> markdown string {{ToMarkdownContentHandler}} already produces (TIKA-4730).
> * *Typed common metadata*: title, authors, keywords, languages,
> created/modified as {{Timestamp}}s, page/word/character counts, dimensions,
> rights.
> * *Lossless tagged tail*: every remaining metadata key,
> multivalue-preserving, typed only where Tika's own {{Property}} declares a
> type, string otherwise -- never guessed.
> * *Embedded documents* recurse as fully typed child {{Document}}s.
> * Format specifics live in per-parser {{DocumentTransformer}} code
> ({{tika-grpc-mapper}}), never in the wire contract, so metadata churn never
> forces a client rebuild.
> Modules: {{tika-grpc-api}} (proto + generated messages + bundled
> {{FileDescriptorSet}}), {{tika-grpc-mapper}}, {{tika-grpc}} integration.
> Breaking change for clients reading {{fields}} (field number reserved).
> PR: [https://github.com/apache/tika/pull/2921] (supersedes
> [#2916|https://github.com/apache/tika/pull/2916])
> Follow-ups will be tracked in separate issues: pluggable external parsers
> (opaque {{Any}} extension results from registered gRPC services), and a
> Markdown input parser.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
