Copilot commented on code in PR #4853:
URL: https://github.com/apache/solr/pull/4853#discussion_r3929084895
##########
solr/licenses/tika-core-4.0.0.jar.sha1:
##########
@@ -0,0 +1 @@
+f370d8619de9e19329b2cc117943c83468c2ff9f
Review Comment:
The old `solr/licenses/tika-core-3.3.2.jar.sha1` is still present, but no
lockfile resolves that artifact after this upgrade. The full license validation
compares every file under `solr/licenses` with referenced dependency files, so
`checkDanglingLicenseFiles` will fail until the stale checksum is deleted.
##########
solr/modules/extraction/src/java/org/apache/solr/handler/extraction/ExtractingDocumentLoader.java:
##########
@@ -143,6 +143,7 @@ public void load(
.tikaServerRecursive(tikaserverRecursive)
.tikaServerTimeoutSeconds(tikaTimeoutSecs)
.tikaServerRequestHeaders(Map.of())
+
.tikaServerConfigJson(params.get(ExtractingParams.TIKASERVER_CONFIG_JSON))
Review Comment:
Malformed `tikaserver.config` and config-plus-recursive are raised by the
backend as `BAD_REQUEST`, but each extraction catch block below wraps every
exception in a new `SERVER_ERROR`. Thus the public `/update/extract` path
returns 500 for these user input errors even though the direct-backend tests
expect 400. Preserve existing `SolrException` status codes (or validate before
the wrappers) and cover this through the request handler.
##########
solr/modules/extraction/src/java/org/apache/solr/handler/extraction/TikaServerExtractionBackend.java:
##########
@@ -273,7 +317,33 @@ InputStream callTikaServer(InputStream inputStream,
ExtractionRequest request) t
}
int code = response.getStatus();
- if (code < 200 || code >= 300) {
+ InputStream responseStream = listener.getInputStream();
+ // Tika 4.x's raw /tika* endpoints (non-recursive) return 422 whenever a
container-level
+ // exception occurred during parsing -- including a non-aborting one like
a writeLimit
+ // truncation -- but the body still carries whatever content was
successfully extracted
+ // (there's no envelope to carry the exception itself on these endpoints;
use /rmeta for
+ // that). A request that extracted nothing at all (e.g. a wrong password)
also gets 422, but
+ // with an empty body -- treat that case as the failure it is instead of a
silent empty
+ // "success". Peek the first byte to tell the two apart.
+ if (code == 422 && !request.tikaServerRecursive) {
+ PushbackInputStream peekable = new PushbackInputStream(responseStream,
1);
+ int firstByte = peekable.read();
+ if (firstByte == -1) {
+ throw new SolrException(
Review Comment:
Every non-recursive 422 with at least one response byte is now treated as
success, although this status signals that Tika hit a container parsing
exception. With the default `ignoreTikaException=false`, a corrupt or truncated
document can therefore be indexed as a successful partial extraction. Preserve
failure by default, or pass an explicit partial/ignore policy into the backend
rather than using body presence as the success criterion.
##########
solr/modules/extraction/src/java/org/apache/solr/handler/extraction/ExtractingParams.java:
##########
@@ -157,4 +157,14 @@ public interface ExtractingParams {
/** Default or per-request timeout in seconds for TikaServer HTTP calls. */
String TIKASERVER_TIMEOUT_SECS = "tikaserver.timeoutSeconds";
+
+ /**
+ * Optional raw JSON object sent as the "config" part of a per-request
TikaServer configuration
+ * call (e.g. {@code {"pdf-parser":{"ocr":{"strategy":"no_ocr"}}}}). Tika
Server 4.x removed its
+ * X-Tika-* configuration headers in favor of this JSON mechanism; the
server must additionally
+ * have {@code allowPerRequestConfig=true} set, or the request is rejected
with 403. Ignored for
+ * recursive (tikaserver.recursive) requests, since TikaServer has no
XML-output variant of
+ * /rmeta/config.
Review Comment:
This says recursive requests ignore the option, but `callTikaServer` rejects
the combination with `BAD_REQUEST`, and the new test asserts that rejection.
Document the actual behavior so API consumers do not expect the parameter to be
silently ignored.
##########
solr/modules/extraction/src/test/org/apache/solr/handler/extraction/ExtractingRequestHandlerTestAbstract.java:
##########
@@ -440,17 +440,20 @@ public void testLiterals() throws Exception {
"one",
"literal.extractionLiteral",
"two",
- "fmap.X-Parsed-By",
+ // Tika 4.x renamed its metadata keys under a single lowercase tk:
prefix (TIKA-4816)
+ "fmap.tk:parsed-by",
"ignored_parser",
- "fmap.X-TIKA:Parsed-By",
+ "fmap.tk:parsed-by-full-set",
"ignored_parser",
Review Comment:
These expectation changes expose a user-facing metadata rename from
`X-TIKA:*`/`X-Parsed-By` to `tk:*`; existing `fmap.<metadata>` parameters will
stop matching after the upgrade. Add a prominent Solr 10 major-change note with
the old-to-new key migration (and mention the recursive per-request/password
limitation) rather than leaving users to infer it from tests.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
