[
https://issues.apache.org/jira/browse/TIKA-4897?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18116129#comment-18116129
]
ASF GitHub Bot commented on TIKA-4897:
--------------------------------------
Copilot commented on code in PR #3184:
URL: https://github.com/apache/tika/pull/3184#discussion_r4028798703
##########
tika-pipes/tika-pipes-core/src/main/java/org/apache/tika/pipes/core/server/PipesWorker.java:
##########
@@ -498,17 +489,20 @@ private void writeMetadataAsJson(OutputStream os,
Metadata metadata) throws IOEx
ObjectMapper mapper = new ObjectMapper();
// Disable auto-close so we don't close the zip output stream
mapper.configure(com.fasterxml.jackson.core.JsonGenerator.Feature.AUTO_CLOSE_TARGET,
false);
- // Convert metadata to a map for JSON serialization
+ mapper.writeValue(os, metadataMap(metadata));
+ }
+
+ /** The same U+FFFD for a lone surrogate as /rmeta writes, so the two
outputs agree. */
+ static java.util.Map<String, Object> metadataMap(Metadata metadata) {
java.util.Map<String, Object> metadataMap = new
java.util.LinkedHashMap<>();
for (String name : metadata.names()) {
- String[] values = metadata.getValues(name);
- if (values.length == 1) {
- metadataMap.put(name, values[0]);
- } else {
- metadataMap.put(name, values);
+ String[] values = metadata.getValues(name).clone(); // the live
array: never rewrite it
+ for (int i = 0; i < values.length; i++) {
+ values[i] = StringUtils.wellFormed(values[i]);
}
+ metadataMap.put(name, values.length == 1 ? values[0] : values);
Review Comment:
This unconditionally clones every metadata value array, even when no
replacements are needed. Consider a lazy-copy approach (like
`MetadataSerializer.wellFormed(String[])`) that only clones and rewrites when
at least one element changes; this avoids extra allocations for the common case
where metadata is already well-formed.
##########
tika-pipes/tika-pipes-core/src/main/java/org/apache/tika/pipes/core/server/ServerProtocolIO.java:
##########
@@ -209,6 +219,34 @@ void resetLastTimings() {
* with a failure status makes the client treat an emitted document as
failed, so a
* retry emits it a second time.
*/
+ /**
+ * A success whose payload is lost is a parse failure for the parent; an
emitted status
+ * stays so the parent does not emit again, and a failure keeps its own
status and category.
+ */
+ static PipesResult.RESULT_STATUS
unserializableStatus(PipesResult.RESULT_STATUS status) {
Review Comment:
There appears to be a stray/dangling Javadoc block ending at line 221 that
is no longer attached to a declaration (likely due to inserting
`unserializableStatus` here). Please move/merge that comment so each Javadoc
block directly precedes the method it documents; this avoids confusing
generated docs and potential style/checkstyle issues.
##########
tika-serialization/src/test/java/org/apache/tika/serialization/serdes/MetadataSerializerSurrogateTest.java:
##########
@@ -0,0 +1,71 @@
+/*
+ * 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.serialization.serdes;
+
+import static org.junit.jupiter.api.Assertions.assertArrayEquals;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertSame;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fasterxml.jackson.dataformat.smile.SmileFactory;
+import org.junit.jupiter.api.Test;
+
+import org.apache.tika.config.loader.TikaObjectMapperFactory;
+import org.apache.tika.metadata.Metadata;
+
+/**
+ * A parser can hand back a lone surrogate (an HTML numeric character
reference for one, for
+ * instance). The UTF-8 and Smile generators reject it, and before this a
single such value
+ * failed the whole document's serialization, which in the pipes worker was
fatal.
+ */
+public class MetadataSerializerSurrogateTest {
+
+ private static final String LONE_HIGH = "abc\uDB2Cdef";
+ private static final String LONE_LOW = "\uDC00abc";
+ private static final String PAIR = "a😀b";
+
+ @Test
+ public void testWellFormed() {
+ assertEquals("abc�def", MetadataSerializer.wellFormed(LONE_HIGH));
+ assertEquals("�abc", MetadataSerializer.wellFormed(LONE_LOW));
+ assertEquals("a��b", MetadataSerializer.wellFormed("a\uDB2C\uDB2Cb"));
+ assertEquals("�", MetadataSerializer.wellFormed("\uDB2C"));
Review Comment:
These assertions embed the replacement character directly (\"�\"). To make
the intent unambiguous and avoid any source-encoding/editor display issues,
prefer using explicit escapes (e.g., `\"\\uFFFD\"`) or a named constant for
U+FFFD in the expected strings.
> Handle unpaired surrogates more robustly
> ----------------------------------------
>
> Key: TIKA-4897
> URL: https://issues.apache.org/jira/browse/TIKA-4897
> Project: Tika
> Issue Type: Task
> Reporter: Tim Allison
> Priority: Minor
>
> An unpaired surrogate causes less than ideal behavior during serialization.
> We should handle this more robustly.
> Â
> After a bit of agentic digging, this is a smile-only issue, so it only hits
> in IPC between tika-pipes client and server. This does not affect our
> standard json serdes.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
