nddipiazza commented on code in PR #2921: URL: https://github.com/apache/tika/pull/2921#discussion_r3536858289
########## tika-grpc-mapper/src/main/java/org/apache/tika/grpc/mapper/DocumentBuilder.java: ########## @@ -0,0 +1,149 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.tika.grpc.mapper; + +import java.time.Instant; +import java.util.List; + +import org.apache.tika.grpc.mapper.content.MarkdownBlockTreeBuilder; +import org.apache.tika.grpc.mapper.transform.DocumentTransformers; +import org.apache.tika.grpc.mapper.transform.FormatCategoryDetector; +import org.apache.tika.grpc.v1.Document; +import org.apache.tika.grpc.v1.ParseStatus; +import org.apache.tika.grpc.v1.SourceOrigin; +import org.apache.tika.metadata.Metadata; +import org.apache.tika.metadata.TikaCoreProperties; + +/** + * Builds a {@link Document} from Tika's parse output: the pipes default content handler + * renders the extracted content as markdown, so this class parses that markdown into + * the structured {@code Block} tree once (format-agnostic) and delegates metadata + * mapping to {@link DocumentTransformers} (format-specific, one transformer per + * concern). Embedded documents recurse into {@link Document#getEmbeddedList()}. + */ +public final class DocumentBuilder { + + private static final DocumentTransformers TRANSFORMERS = DocumentTransformers.defaults(); + + private DocumentBuilder() { + } + + /** Maps Tika parse output (primary metadata plus optional embedded metadata list) to {@link Document}. */ + public static Document build(Metadata primary, List<Metadata> allMetadata, String markdownBody, + String docId, String pipesStatus, long parseTimeMs) { + if (primary == null) { Review Comment: 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. ########## tika-e2e-tests/tika-grpc/src/test/java/org/apache/tika/pipes/filesystem/HandlerTypeTest.java: ########## @@ -313,7 +313,7 @@ void testParseContextJson() throws Exception { Assertions.assertEquals("PARSE_SUCCESS", htmlReply.getStatus(), "Parse should succeed with HTML handler type"); - String htmlContent = htmlReply.getFieldsMap().get("X-TIKA:content"); + String htmlContent = htmlReply.getDocument().getMarkdown(); Review Comment: 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. ########## tika-grpc-mapper/src/main/java/org/apache/tika/grpc/mapper/transform/PdfDocumentTransformer.java: ########## @@ -0,0 +1,65 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.tika.grpc.mapper.transform; + +import java.util.Locale; +import java.util.Set; + +import org.apache.tika.grpc.v1.Document; +import org.apache.tika.grpc.v1.DocumentMetadata; +import org.apache.tika.metadata.Metadata; +import org.apache.tika.metadata.Office; +import org.apache.tika.metadata.PagedText; +import org.apache.tika.metadata.TikaCoreProperties; + +/** + * PDF. Maps the common, cross-format facts into typed DocumentMetadata. + * + * PDF-specific properties (encryption flags, XMP, permissions, incremental updates, ...) + * are NOT given their own proto fields. They flow into the tagged tail, typed where Tika + * declares the type and string otherwise. That is the whole point: format richness lives + * in code plus the tail, not in the wire contract - so the schema stays small and stable + * and clients never rebuild when we add or change a PDF property. + */ +public final class PdfDocumentTransformer implements DocumentTransformer { + + @Override + public boolean appliesTo(Metadata tika) { + String contentType = tika.get(Metadata.CONTENT_TYPE); + return contentType != null && contentType.toLowerCase(Locale.ROOT).contains("pdf"); + } + + @Override + public void transform(Metadata tika, Document.Builder document, Set<String> consumed) { + DocumentMetadata.Builder meta = document.getMetadataBuilder(); + + // Common, cross-format fields -> typed (a date is a Timestamp, a count is an int). + TransformSupport.setString(tika, TikaCoreProperties.TITLE, meta::setTitle, consumed); Review Comment: 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. ########## tika-grpc/src/main/java/org/apache/tika/pipes/grpc/TikaGrpcServerImpl.java: ########## @@ -312,16 +318,21 @@ private void fetchAndParseImpl(FetchAndParseRequest request, if (pipesResult.status().equals(PipesResult.RESULT_STATUS.FETCH_EXCEPTION)) { fetchReplyBuilder.setErrorMessage(pipesResult.message()); } + long parseTimeMs = (System.nanoTime() - parseStart) / 1_000_000L; Review Comment: `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. -- 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]
