[
https://issues.apache.org/jira/browse/TIKA-4766?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18097663#comment-18097663
]
ASF GitHub Bot commented on TIKA-4766:
--------------------------------------
nddipiazza commented on PR #2921:
URL: https://github.com/apache/tika/pull/2921#issuecomment-5026875462
Follow-up after digging further into the `tika-grpc`/`tika-grpc-api`
contract specifically (cc @tballison since this bears directly on your #2916
feedback).
## Two scope concerns in the current diff
**1. `ParseBytesRequest`/`ParseBytes` RPC is new scope, not described
anywhere in this PR.**
`tika.proto` adds a whole new RPC (`rpc ParseBytes(ParseBytesRequest)
returns (FetchAndParseReply)`) for parsing inline bytes without registering a
fetcher. It isn't mentioned in the PR description, the "what's in /
deliberately not in this PR" table, or the client migration table — it has
nothing to do with the typed `Document` contract.
The implementation in `TikaGrpcServerImpl` is also a real workaround, not a
thin RPC:
- On every server start it creates a temp directory
(`Files.createTempDirectory("tika-grpc-parse-bytes")`) and rewrites the loaded
Tika config to inject a synthetic `file-system-fetcher` rooted there
(`augmentWithParseBytesFetcher`), so the forked pipes worker can "fetch" bytes
the caller pushed inline.
- Neither `shutdown()` nor `postShutdown()` clean up `parseBytesDir`, nor
the temp config file written by `augmentWithParseBytesFetcher`
(`Files.createTempFile("tika-grpc-config", ".json")`). Every server start leaks
a temp dir + temp file that's never removed.
- It changes config-loading behavior for every request (the augmented config
becomes the one config path the server ever loads) for the sake of one RPC.
Given the whole point of this PR (and #2916) is keeping tika-grpc small and
reviewable, I'd pull `ParseBytes` into its own PR/JIRA with its own review.
**2. `FormatCategory` re-bakes per-format buckets into the wire contract.**
`document.proto`'s `FormatCategory` enum
(`PDF/OFFICE/IMAGE/HTML/RTF/EPUB/WARC/GENERIC`) plus its detector
(`FormatCategoryDetector`, mime/extension string matching) reintroduces the
pattern called out in #2916 point 3 — re-deriving something Tika already told
us (`content_type` already carries the detected MIME type) via a
hand-maintained second classification. Every new "coarse category" is a public
proto enum change — the same wire churn this PR's premise is meant to avoid.
RTF/EPUB/WARC already getting dedicated enum values shows the pull. The docs
pitch "adding a format = adding a DocumentTransformer, wire contract doesn't
move" isn't quite true here.
Aside from those two, the actual mapper transformers
(`PdfDocumentTransformer`, `OfficeDocumentTransformer`,
`EpubDocumentTransformer`, `WarcDocumentTransformer`, etc.) are appropriately
thin now — 40-70 lines each, no format-structure-extraction logic living in
`tika-grpc-mapper` — and the module's main-scope deps are clean (`tika-core` +
`tika-grpc-api` + `commonmark` + `protobuf-java`; every parser module is
test-scoped only). That part of the #2916 "wrong altitude" concern looks
resolved.
## Proposal: stage this as a sequence of small PRs
Given @tballison's core objection was scale/maintenance risk of a big bang
change, I think this whole effort would merge faster broken into independently
reviewable, independently testable PRs — mostly a split of what already exists
here, not a rewrite:
1. **Minimal typed contract.** `document.proto` trimmed to: `Document`
envelope (`id`, `content_type`, `parsed_at`, `status`, `origin`),
`DocumentMetadata` with **DC fields only**
(title/authors/description/keywords/languages/publishers/identifiers/created/modified/rights),
the `MetadataField`/`MetadataValue` tagged tail, `ParseStatus`. No
`blocks`/`markdown`, no `FormatCategory`, no `embedded`. Pair with
`GenericDocumentTransformer` only, wired into `tika-grpc`
(`FetchAndParseReply.document`) with a real e2e test through the live server.
This is close to the ~80-line proto Tim said he'd be okay with, and directly
answers the top #2916 concern on its own.
2. **Per-format typed transformers**, one PR (or 2-3 grouped):
`PdfDocumentTransformer`, `OfficeDocumentTransformer`,
`HtmlDocumentTransformer`, `ImageDocumentTransformer`. Additive only, no proto
change — review is just "is this mapping correct."
3. **Content tree** (`blocks`/`markdown`) as its own PR — a genuinely
separate feature (commonmark parsing, tables/lists/inline runs), with its own
live-server e2e round-trip test.
4. **`embedded` recursion** as its own PR — distinct complexity (depth
limits, cycles), with an Office/PDF-with-embedded-image e2e test.
5. **Niche formats** (Epub, Warc, Rtf, Creative Commons) last, only once the
mechanism is proven on common formats — each small and additive.
Would drop or defer indefinitely:
- `FormatCategory` — let clients bucket on `content_type` instead, unless a
client has a concrete need.
- `ParseBytes` — separate JIRA/PR entirely, unrelated to this contract.
- External pluggable parsers, `.md` parser — already correctly deferred in
the description.
Each stage above is independently testable (unit + e2e) and independently
mergeable, which should make review a lot less daunting than reviewing ~3.9k
lines at once.
> 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)
