TIKA-4766: typed Document parse contract for tika-grpc

[krickert]

Summary

Follow-up to #2916, reshaped per the review there. Instead of mirroring Tika's open metadata taxonomy in protobuf (~5k lines of proto, per-format messages), this PR types the thing that is actually stable: the parsed document. One small contract — document.proto is 208 lines — and format specifics live in per-parser mapping code, never in the wire.

FetchAndParseReply.fields (map<string,string>, field 2, now reserved) is replaced by FetchAndParseReply.document.

How this answers the #2916 review

Concern from #2916	Where it landed
"11k lines to nail down maybe 80% of an open set"	208-line contract; whole PR is ~3.9k lines, most of it mapper code + tests
Clients rebuild when metadata definitions change	A metadata key is now data (extra tail), not schema — add/rename/retype a Tika Property and no client regenerates anything
Lossless catch-all as source of truth	Document.extra carries every Tika key, multivalue-preserving; a test asserts nothing is dropped
Special handling only for DC + core props	DocumentMetadata types only the bounded cross-format fields (title, authors, dates as Timestamp, counts, dimensions, rights)
Break it into individually reviewable tasks	This PR is the contract only; see "Deliberately not in this PR"

The shape

1. Content tree: markdown (the same render ToMarkdownContentHandler already produces since TIKA-4730) plus blocks — that markdown parsed once, format-agnostically, into a structured tree of headings/paragraphs/lists/tables/code blocks/inline runs (CommonMark + GFM, a spec that does not churn). This is what a downstream NLP/RAG/embeddings consumer actually wants: typed tables and sections, not a string to re-parse.
2. Typed common metadata: DocumentMetadata, grouped by concern, not by source format. Dates are Timestamps, counts are ints — not strings that 12 language clients each re-parse.
3. Tagged tail: extra — every remaining key, typed only where Tika's own Property declares a type (integer/real/boolean/date), string otherwise, never guessed.
4. embedded recurses: a PDF with an embedded image is a parent Document with a fully typed child — no forcing two formats into one bucket (this was the oneof problem from TIKA-4766: Typed parse response grpc #2916).
5. Adding a format = adding a DocumentTransformer (see tika-grpc-mapper/docs/EXTENSIONS.md); PdfDocumentTransformer is 65 lines and the wire contract does not move.

Deliberately not in this PR (follow-ups, each its own PR)

• Pluggable external parsers: registering a third-party gRPC service whose output rides along on the Document as a google.protobuf.Any — so wildly different result shapes (e.g. a document-layout model's tree) never require Tika to model them. Built and tested on a branch; kept out to keep this reviewable.
• A Markdown parser for .md input files (separate JIRA).
• Richer typed fields, if and only if real cross-format demand appears — they'd be additive optional fields, compatible both directions.

Open decisions where reviewer preference wins

1. Tail shape: repeated MetadataField with a typed value oneof (as implemented) vs the map<string, StringList> suggested in TIKA-4766: Typed parse response grpc #2916. The typed-where-declared tail preserves types without guessing; the map is maximally churn-proof. Swapping is a one-message change — happy to go either way.
2. markdown + blocks both: today both ship (string render + structured tree). If payload size matters, a per-request flag choosing one is easy.
3. Hard removal vs staged: fields is hard-removed (4.0, nothing consumes it yet); can switch to deprecate-then-remove if preferred.

Client migration

Before	After
fields["X-TIKA:content"]	document.markdown (or walk document.blocks)
fields["Content-Type"]	document.content_type
Ad hoc title/author/date strings	document.metadata.title / .authors / .created (Timestamp)
Any other key	document.extra (typed by declared Property type, string otherwise)

Test plan

• ./mvnw -pl tika-grpc-api,tika-grpc-mapper,tika-grpc test — green (transformer tests against real parse fixtures per format, block-tree tests, DocumentBuilder envelope/status/embedded tests, server tests reading FetchAndParseReply.document)
• tika-grpc-api jar bundles META-INF/org.apache.tika.grpc.v1.descriptors (verified: contains document.proto)
• e2e tika-grpc-e2e-test compiles against the new API
• CI

Downstream context: this contract is what the OpenNLP gRPC work (OPENNLP-1833) will consume as input — Tika parse → typed document → NLP/embeddings without re-parsing strings.

[krickert]

Follow-up work is broken out into TIKA-4771 (pluggable external parsers) and TIKA-4772 (document event streaming) rather than growing this PR.

[krickert]

@nddipiazza @tballison what do you think of this design instead? far less fields - ability to roll your own protobuf model in the future. Best of both worlds. Document structure is very markdown-friendly. I'll make the output of this be able to be the input of the grpc OpenNLP grpc server.

[nddipiazza]

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:

- 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.

[nddipiazza]

Overall this is a solid design and a much better-scoped answer to the #2916 feedback: a small stable 208-line proto, format logic pushed into per-parser Java transformers, and a lossless typed+tagged tail. Requesting changes mainly on test coverage for the new contract, plus a couple of correctness/maintainability issues found while reading the mapper code (see inline comments).

[nddipiazza]

This is the only change anywhere in tika-e2e-tests/tika-grpc for this PR (this line and the analogous one at line 334 for the TEXT handler). It's a compile-fix that keeps the pre-existing "content is non-empty" assertion alive against the new API, but it does not exercise the new typed contract through the live gRPC server at all.

None of the e2e tests (HandlerTypeTest, FileSystemFetcherTest, IgniteConfigStoreTest, ExternalTestBase) assert on:

• document.metadata (typed title/authors/dates/counts)
• document.extra (tagged-tail typing: int/bool/date/string)
• document.blocks (structured content tree: headings/tables/lists/code)
• document.embedded (recursion for container formats, e.g. an Office doc with an embedded image)
• document.format_category

FileSystemFetcherTest/IgniteConfigStoreTest/ExternalTestBase only ever inspect getFetchKey()/getStatus() on FetchAndParseReply and never call getDocument() at all, so the bulk-corpus/streaming/ignite e2e paths give zero signal on the new contract this PR introduces.

The real proof of correctness lives entirely in tika-grpc-mapper's unit tests, which feed fixture-derived Metadata/markdown into DocumentBuilder in-process -- good for the mapping logic, but it bypasses the actual gRPC wire serialization, the live server (TikaGrpcServerImpl), the pipes client, and fetcher plumbing entirely.

Requesting: add (or extend this test) at least one e2e case per format that fetches a real file through the live server and asserts on a couple of document.metadata fields, at least one document.extra tagged key, and one document.embedded case -- not just that markdown is non-empty.

[nddipiazza]

This primary == null branch looks unreachable in production. The only production caller, TikaGrpcServerImpl, computes Metadata primary = metadataList.isEmpty() ? tikaMetadata : metadataList.get(0), and tikaMetadata is always new Metadata() (never null) -- so this guard is only exercised today via DocumentBuilderTest calling DocumentBuilder.build(null, ...) directly.

Practical effect: on a real fetch/parse failure where emitData()/the metadata list is empty, the server currently builds a Document from an empty Metadata object rather than hitting this explicit "No metadata returned from parse" error path. The ParseStatus.status still ends up FAILED via mapPipesStatus, so it's not silently treated as success, but the specific error message here never surfaces. Worth either wiring the server to pass null when there's truly no metadata, or removing this branch if it's intentionally test-only.

[nddipiazza]

This same 7-line block (TITLE/DESCRIPTION/CREATOR/SUBJECT/LANGUAGE/CREATED/MODIFIED) is duplicated verbatim across 7 of the 8 transformers (Generic, Pdf, Html, Image, Office, Rtf, Epub). Consider factoring it into a shared TransformSupport.mapCommonFields(tika, meta, consumed) helper called by each transformer, leaving only the format-specific additions (page/word counts, dimensions, etc.) per-transformer. This would also reduce the risk of the common mapping drifting between formats over time.

[nddipiazza]

parseTimeMs is measured from before pipesClient.process() is called, so it includes fetch time as well as parse time, not just parse time. Either rename the field/timer to reflect that it's fetch+parse latency, or move parseStart to wherever the actual parse begins if ParseStatus.parse_time_ms is meant to be parse-only.