elastic · joegallo · Oct 3, 2023 · Sep 8, 2023
diff --git a/server/src/main/java/org/elasticsearch/action/ingest/SimulateDocumentBaseResult.java b/server/src/main/java/org/elasticsearch/action/ingest/SimulateDocumentBaseResult.java
@@ -51,12 +51,17 @@ public final class SimulateDocumentBaseResult implements SimulateDocumentResult
     }
 
     public SimulateDocumentBaseResult(IngestDocument ingestDocument) {
+        Exception failure = null;
+        WriteableIngestDocument wid = null;
         if (ingestDocument != null) {
-            this.ingestDocument = new WriteableIngestDocument(ingestDocument);
-        } else {
-            this.ingestDocument = null;
+            try {
+                wid = new WriteableIngestDocument(ingestDocument);
+            } catch (Exception ex) {
+                failure = ex;
+            }
         }
-        this.failure = null;
+        this.ingestDocument = wid;
+        this.failure = failure;
     }
 
     public SimulateDocumentBaseResult(Exception failure) {

diff --git a/server/src/main/java/org/elasticsearch/action/ingest/SimulateProcessorResult.java b/server/src/main/java/org/elasticsearch/action/ingest/SimulateProcessorResult.java
@@ -126,7 +126,19 @@ public SimulateProcessorResult(
     ) {
         this.processorTag = processorTag;
         this.description = description;
-        this.ingestDocument = (ingestDocument == null) ? null : new WriteableIngestDocument(ingestDocument);
+        WriteableIngestDocument wid = null;
+        if (ingestDocument != null) {
+            try {
+                wid = new WriteableIngestDocument(ingestDocument);
+            } catch (Exception ex) {
+                // if there was a failure already, then track it as a suppressed exception
+                if (failure != null) {
+                    ex.addSuppressed(failure);
+                }
+                failure = ex;
+            }
+        }
+        this.ingestDocument = wid;
         this.failure = failure;
         this.conditionalWithResult = conditionalWithResult;
         this.type = type;

diff --git a/server/src/main/java/org/elasticsearch/action/ingest/WriteableIngestDocument.java b/server/src/main/java/org/elasticsearch/action/ingest/WriteableIngestDocument.java
@@ -26,7 +26,6 @@
 import java.util.Date;
 import java.util.HashMap;
 import java.util.Map;
-import java.util.Objects;
 
 import static org.elasticsearch.xcontent.ConstructingObjectParser.constructorArg;
 import static org.elasticsearch.xcontent.ConstructingObjectParser.optionalConstructorArg;
@@ -57,7 +56,7 @@ final class WriteableIngestDocument implements Writeable, ToXContentFragment {
                 sourceAndMetadata.put(Metadata.VERSION_TYPE.getFieldName(), a[5]);
             }
             sourceAndMetadata.putAll((Map<String, Object>) a[6]);
-            return new WriteableIngestDocument(new IngestDocument(sourceAndMetadata, (Map<String, Object>) a[7]));
+            return new WriteableIngestDocument(sourceAndMetadata, (Map<String, Object>) a[7]);
         }
     );
     static {
@@ -84,9 +83,24 @@ final class WriteableIngestDocument implements Writeable, ToXContentFragment {
         PARSER.declareObject(constructorArg(), INGEST_DOC_PARSER, new ParseField(DOC_FIELD));
     }
 
+    /**
+     * Builds a writeable ingest document that wraps a copy of the passed-in, non-null ingest document.
+     *
+     * @throws IllegalArgumentException if the passed-in ingest document references itself
+     */
     WriteableIngestDocument(IngestDocument ingestDocument) {
         assert ingestDocument != null;
-        this.ingestDocument = ingestDocument;
+        this.ingestDocument = new IngestDocument(ingestDocument); // internal defensive copy
+    }
+
+    /**
+     * Builds a writeable ingest document by constructing the wrapped ingest document from the passed-in maps.
+     * <p>
+     * This is intended for cases like deserialization, where we know the passed-in maps aren't self-referencing,
+     * and where a defensive copy is unnecessary.
+     */
+    private WriteableIngestDocument(Map<String, Object> sourceAndMetadata, Map<String, Object> ingestMetadata) {
+        this.ingestDocument = new IngestDocument(sourceAndMetadata, ingestMetadata);
     }
 
     WriteableIngestDocument(StreamInput in) throws IOException {
@@ -132,23 +146,6 @@ public static WriteableIngestDocument fromXContent(XContentParser parser) {
         return PARSER.apply(parser, null);
     }
 
-    @Override
-    public boolean equals(Object o) {
-        if (this == o) {
-            return true;
-        }
-        if (o == null || getClass() != o.getClass()) {
-            return false;
-        }
-        WriteableIngestDocument that = (WriteableIngestDocument) o;
-        return Objects.equals(ingestDocument, that.ingestDocument);
-    }
-
-    @Override
-    public int hashCode() {
-        return Objects.hash(ingestDocument);
-    }
-
     @Override
     public String toString() {
         return ingestDocument.toString();

diff --git a/server/src/main/java/org/elasticsearch/ingest/IngestDocument.java b/server/src/main/java/org/elasticsearch/ingest/IngestDocument.java
@@ -9,6 +9,7 @@
 package org.elasticsearch.ingest;
 
 import org.elasticsearch.common.Strings;
+import org.elasticsearch.common.util.CollectionUtils;
 import org.elasticsearch.common.util.LazyMap;
 import org.elasticsearch.index.VersionType;
 import org.elasticsearch.index.mapper.IdFieldMapper;
@@ -82,16 +83,26 @@ public IngestDocument(
     }
 
     /**
-     * Copy constructor that creates a new {@link IngestDocument} which has exactly the same properties as the one provided as argument
+     * Copy constructor that creates a new {@link IngestDocument} which has exactly the same properties as the one provided.
+     *
+     * @throws IllegalArgumentException if the passed-in ingest document references itself
      */
     public IngestDocument(IngestDocument other) {
-        this(deepCopyMap(other.sourceAndMetadata), deepCopyMap(other.ingestMetadata));
+        this(deepCopyMap(ensureNoSelfReferences(other.sourceAndMetadata)), deepCopyMap(other.ingestMetadata));
+    }
+
+    /**
+     * Internal helper utility method to get around the issue that a {@code this(...) } constructor call must be the first statement
+     * in a constructor. This is only for use in the {@link IngestDocument#IngestDocument(IngestDocument)} copy constructor, it's not a
+     * general purpose method.
+     */
+    private static Map<String, Object> ensureNoSelfReferences(Map<String, Object> source) {
+        CollectionUtils.ensureNoSelfReferences(source, null);
+        return source;
     }
 
     /**
-     * Constructor needed for testing that allows to create a new {@link IngestDocument} given the provided elasticsearch metadata,
-     * source and ingest metadata. This is needed because the ingest metadata will be initialized with the current timestamp at
-     * init time, which makes equality comparisons impossible in tests.
+     * Constructor to create an IngestDocument from its constituent maps. The maps are shallow copied.
      */
     public IngestDocument(Map<String, Object> sourceAndMetadata, Map<String, Object> ingestMetadata) {
         this.sourceAndMetadata = sourceAndMetadata;

diff --git a/server/src/main/java/org/elasticsearch/ingest/TrackingResultProcessor.java b/server/src/main/java/org/elasticsearch/ingest/TrackingResultProcessor.java
@@ -99,7 +99,7 @@ public void execute(IngestDocument ingestDocument, BiConsumer<IngestDocument, Ex
                                 pipelineProcessor.getType(),
                                 pipelineProcessor.getTag(),
                                 pipelineProcessor.getDescription(),
-                                new IngestDocument(ingestDocument),
+                                ingestDocument,
                                 e,
                                 conditionalWithResult
                             )
@@ -149,7 +149,7 @@ public void execute(IngestDocument ingestDocument, BiConsumer<IngestDocument, Ex
                             actualProcessor.getType(),
                             actualProcessor.getTag(),
                             actualProcessor.getDescription(),
-                            new IngestDocument(ingestDocument),
+                            ingestDocument,
                             e,
                             conditionalWithResult
                         )
@@ -173,7 +173,7 @@ public void execute(IngestDocument ingestDocument, BiConsumer<IngestDocument, Ex
                             actualProcessor.getType(),
                             actualProcessor.getTag(),
                             actualProcessor.getDescription(),
-                            new IngestDocument(ingestDocument),
+                            ingestDocument,
                             conditionalWithResult
                         )
                     );

diff --git a/server/src/test/java/org/elasticsearch/action/ingest/SimulateDocumentBaseResultTests.java b/server/src/test/java/org/elasticsearch/action/ingest/SimulateDocumentBaseResultTests.java
@@ -88,7 +88,7 @@ protected Predicate<String> getRandomFieldsExcludeFilter() {
     }
 
     public static void assertEqualDocs(SimulateDocumentBaseResult response, SimulateDocumentBaseResult parsedResponse) {
-        assertEquals(response.getIngestDocument(), parsedResponse.getIngestDocument());
+        assertIngestDocument(response.getIngestDocument(), parsedResponse.getIngestDocument());
         if (response.getFailure() != null) {
             assertNotNull(parsedResponse.getFailure());
             assertThat(parsedResponse.getFailure().getMessage(), containsString(response.getFailure().getMessage()));

diff --git a/server/src/test/java/org/elasticsearch/action/ingest/SimulateExecutionServiceTests.java b/server/src/test/java/org/elasticsearch/action/ingest/SimulateExecutionServiceTests.java
@@ -108,7 +108,7 @@ public void testExecuteItem() throws Exception {
         assertThat(processor.getInvokedCounter(), equalTo(2));
         assertThat(actualItemResponse, instanceOf(SimulateDocumentBaseResult.class));
         SimulateDocumentBaseResult simulateDocumentBaseResult = (SimulateDocumentBaseResult) actualItemResponse;
-        assertThat(simulateDocumentBaseResult.getIngestDocument(), equalTo(ingestDocument));
+        assertIngestDocument(simulateDocumentBaseResult.getIngestDocument(), ingestDocument);
         assertThat(simulateDocumentBaseResult.getFailure(), nullValue());
     }
 

diff --git a/server/src/test/java/org/elasticsearch/action/ingest/SimulateProcessorResultTests.java b/server/src/test/java/org/elasticsearch/action/ingest/SimulateProcessorResultTests.java
@@ -144,7 +144,7 @@ protected Predicate<String> getRandomFieldsExcludeFilter() {
 
     static void assertEqualProcessorResults(SimulateProcessorResult response, SimulateProcessorResult parsedResponse) {
         assertEquals(response.getProcessorTag(), parsedResponse.getProcessorTag());
-        assertEquals(response.getIngestDocument(), parsedResponse.getIngestDocument());
+        assertIngestDocument(response.getIngestDocument(), parsedResponse.getIngestDocument());
         if (response.getFailure() != null) {
             assertNotNull(parsedResponse.getFailure());
             assertThat(parsedResponse.getFailure().getMessage(), containsString(response.getFailure().getMessage()));

diff --git a/server/src/test/java/org/elasticsearch/action/ingest/WriteableIngestDocumentTests.java b/server/src/test/java/org/elasticsearch/action/ingest/WriteableIngestDocumentTests.java
@@ -24,7 +24,6 @@
 
 import java.io.IOException;
 import java.util.Arrays;
-import java.util.Collections;
 import java.util.HashMap;
 import java.util.Map;
 import java.util.StringJoiner;
@@ -36,67 +35,18 @@
 import static org.hamcrest.Matchers.instanceOf;
 import static org.hamcrest.Matchers.is;
 import static org.hamcrest.Matchers.not;
+import static org.hamcrest.Matchers.sameInstance;
 
 public class WriteableIngestDocumentTests extends AbstractXContentTestCase<WriteableIngestDocument> {
 
-    public void testEqualsAndHashcode() throws Exception {
-        Map<String, Object> sourceAndMetadata = RandomDocumentPicks.randomSource(random());
-        int numFields = randomIntBetween(1, IngestDocument.Metadata.values().length);
-        for (int i = 0; i < numFields; i++) {
-            sourceAndMetadata.put(randomFrom(IngestDocument.Metadata.values()).getFieldName(), randomAlphaOfLengthBetween(5, 10));
-        }
-        Map<String, Object> ingestMetadata = new HashMap<>();
-        numFields = randomIntBetween(1, 5);
-        for (int i = 0; i < numFields; i++) {
-            ingestMetadata.put(randomAlphaOfLengthBetween(5, 10), randomAlphaOfLengthBetween(5, 10));
-        }
-        WriteableIngestDocument ingestDocument = new WriteableIngestDocument(new IngestDocument(sourceAndMetadata, ingestMetadata));
-
-        boolean changed = false;
-        Map<String, Object> otherSourceAndMetadata;
-        if (randomBoolean()) {
-            otherSourceAndMetadata = RandomDocumentPicks.randomSource(random());
-            changed = true;
-        } else {
-            otherSourceAndMetadata = new HashMap<>(sourceAndMetadata);
-        }
-        if (randomBoolean()) {
-            numFields = randomIntBetween(1, IngestDocument.Metadata.values().length);
-            for (int i = 0; i < numFields; i++) {
-                otherSourceAndMetadata.put(randomFrom(IngestDocument.Metadata.values()).getFieldName(), randomAlphaOfLengthBetween(5, 10));
-            }
-            changed = true;
-        }
-
-        Map<String, Object> otherIngestMetadata;
-        if (randomBoolean()) {
-            otherIngestMetadata = new HashMap<>();
-            numFields = randomIntBetween(1, 5);
-            for (int i = 0; i < numFields; i++) {
-                otherIngestMetadata.put(randomAlphaOfLengthBetween(5, 10), randomAlphaOfLengthBetween(5, 10));
-            }
-            changed = true;
-        } else {
-            otherIngestMetadata = Collections.unmodifiableMap(ingestMetadata);
-        }
+    @Override
+    protected boolean assertToXContentEquivalence() {
+        return false;
+    }
 
-        WriteableIngestDocument otherIngestDocument = new WriteableIngestDocument(
-            new IngestDocument(otherSourceAndMetadata, otherIngestMetadata)
-        );
-        if (changed) {
-            assertThat(ingestDocument, not(equalTo(otherIngestDocument)));
-            assertThat(otherIngestDocument, not(equalTo(ingestDocument)));
-        } else {
-            assertThat(ingestDocument, equalTo(otherIngestDocument));
-            assertThat(otherIngestDocument, equalTo(ingestDocument));
-            assertThat(ingestDocument.hashCode(), equalTo(otherIngestDocument.hashCode()));
-            WriteableIngestDocument thirdIngestDocument = new WriteableIngestDocument(
-                new IngestDocument(Collections.unmodifiableMap(sourceAndMetadata), Collections.unmodifiableMap(ingestMetadata))
-            );
-            assertThat(thirdIngestDocument, equalTo(ingestDocument));
-            assertThat(ingestDocument, equalTo(thirdIngestDocument));
-            assertThat(ingestDocument.hashCode(), equalTo(thirdIngestDocument.hashCode()));
-        }
+    @Override
+    protected void assertEqualInstances(WriteableIngestDocument expectedInstance, WriteableIngestDocument newInstance) {
+        assertIngestDocument(expectedInstance.getIngestDocument(), newInstance.getIngestDocument());
     }
 
     public void testSerialization() throws IOException {
@@ -148,7 +98,7 @@ public void testToXContent() throws IOException {
         }
 
         IngestDocument serializedIngestDocument = new IngestDocument(toXContentSource, toXContentIngestMetadata);
-        assertThat(serializedIngestDocument, equalTo(serializedIngestDocument));
+        assertIngestDocument(serializedIngestDocument, serializedIngestDocument);
     }
 
     public void testXContentHashSetSerialization() throws Exception {
@@ -169,6 +119,14 @@ public void testXContentHashSetSerialization() throws Exception {
         }
     }
 
+    public void testCopiesTheIngestDocument() {
+        IngestDocument document = createRandomIngestDoc();
+        WriteableIngestDocument wid = new WriteableIngestDocument(document);
+
+        assertIngestDocument(wid.getIngestDocument(), document);
+        assertThat(wid.getIngestDocument(), not(sameInstance(document)));
+    }
+
     static IngestDocument createRandomIngestDoc() {
         XContentType xContentType = randomFrom(XContentType.values());
         BytesReference sourceBytes = RandomObjects.randomSource(random(), xContentType);

diff --git a/server/src/test/java/org/elasticsearch/ingest/IngestDocumentTests.java b/server/src/test/java/org/elasticsearch/ingest/IngestDocumentTests.java
@@ -1016,10 +1016,50 @@ public void testIngestMetadataTimestamp() throws Exception {
     }
 
     public void testCopyConstructor() {
-        IngestDocument ingestDocument = RandomDocumentPicks.randomIngestDocument(random());
-        IngestDocument copy = new IngestDocument(ingestDocument);
-        assertThat(ingestDocument.getSourceAndMetadata(), not(sameInstance(copy.getSourceAndMetadata())));
-        assertIngestDocument(ingestDocument, copy);
+        {
+            // generic test with a random document and copy
+            IngestDocument ingestDocument = RandomDocumentPicks.randomIngestDocument(random());
+            IngestDocument copy = new IngestDocument(ingestDocument);
+
+            // these fields should not be the same instance
+            assertThat(ingestDocument.getSourceAndMetadata(), not(sameInstance(copy.getSourceAndMetadata())));
+
+            // but the two objects should be very much equal to each other
+            assertIngestDocument(ingestDocument, copy);
+        }
+
+        {
+            // manually punch in a few values
+            IngestDocument ingestDocument = RandomDocumentPicks.randomIngestDocument(random());
+            ingestDocument.setFieldValue("_index", "foo1");
+            ingestDocument.setFieldValue("_id", "bar1");
+            ingestDocument.setFieldValue("hello", "world1");
+            IngestDocument copy = new IngestDocument(ingestDocument);
+
+            // make sure the copy matches
+            assertIngestDocument(ingestDocument, copy);
+
+            // change the copy
+            copy.setFieldValue("_index", "foo2");
+            copy.setFieldValue("_id", "bar2");
+            copy.setFieldValue("hello", "world2");
+
+            // the original shouldn't have changed
+            assertThat(ingestDocument.getFieldValue("_index", String.class), equalTo("foo1"));
+            assertThat(ingestDocument.getFieldValue("_id", String.class), equalTo("bar1"));
+            assertThat(ingestDocument.getFieldValue("hello", String.class), equalTo("world1"));
+        }
+
+        {
+            // the copy constructor rejects self-references
+            IngestDocument ingestDocument = RandomDocumentPicks.randomIngestDocument(random());
+            List<Object> someList = new ArrayList<>();
+            someList.add("some string");
+            someList.add(someList); // the list contains itself
+            ingestDocument.setFieldValue("someList", someList);
+            Exception e = expectThrows(IllegalArgumentException.class, () -> new IngestDocument(ingestDocument));
+            assertThat(e.getMessage(), equalTo("Iterable object is self-referencing itself"));
+        }
     }
 
     public void testCopyConstructorWithZonedDateTime() {