From c927cbea6abf154c48ecba2a26ff0e63bb7e58d3 Mon Sep 17 00:00:00 2001 From: Joe Gallo Date: Fri, 8 Sep 2023 15:21:41 -0400 Subject: [PATCH] Refactor WriteableIngestDocument (#99324) --- .../ingest/SimulateDocumentBaseResult.java | 13 +++- .../ingest/SimulateProcessorResult.java | 14 +++- .../ingest/WriteableIngestDocument.java | 37 +++++---- .../elasticsearch/ingest/IngestDocument.java | 21 +++-- .../ingest/TrackingResultProcessor.java | 6 +- .../SimulateDocumentBaseResultTests.java | 2 +- .../ingest/SimulateExecutionServiceTests.java | 2 +- .../ingest/SimulateProcessorResultTests.java | 2 +- .../ingest/WriteableIngestDocumentTests.java | 76 +++++-------------- .../ingest/IngestDocumentTests.java | 48 +++++++++++- .../ingest/TrackingResultProcessorTests.java | 22 +++--- .../ingest/IngestDocumentMatcher.java | 29 +++++-- .../ingest/IngestDocumentMatcherTests.java | 19 +++++ 13 files changed, 176 insertions(+), 115 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/action/ingest/SimulateDocumentBaseResult.java b/server/src/main/java/org/elasticsearch/action/ingest/SimulateDocumentBaseResult.java index 1cfbc7974020a..6f52da1c8e471 100644 --- 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 index 78a5a3cb4e918..f97705990cd95 100644 --- 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 index 537e459d9a9d2..1aec7c25c7a27 100644 --- 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) a[6]); - return new WriteableIngestDocument(new IngestDocument(sourceAndMetadata, (Map) a[7])); + return new WriteableIngestDocument(sourceAndMetadata, (Map) 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. + *

+ * 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 sourceAndMetadata, Map 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 index 357b85b6cc1ac..ba029501d7770 100644 --- 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 ensureNoSelfReferences(Map 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 sourceAndMetadata, Map 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 index 54c8decc3da44..be6a105b103ac 100644 --- 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 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 index d8315f9c380c4..7f31866224ea3 100644 --- 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 index f9d10462f9604..cb309943d8d68 100644 --- 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 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 index a0acf115dd556..e25a5be741116 100644 --- 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 { - public void testEqualsAndHashcode() throws Exception { - Map 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 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 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 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 index b4a0fddc3d644..ed3064ccfd0e7 100644 --- 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 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() { diff --git a/server/src/test/java/org/elasticsearch/ingest/TrackingResultProcessorTests.java b/server/src/test/java/org/elasticsearch/ingest/TrackingResultProcessorTests.java index 0f70afd8402b0..760b81610cc04 100644 --- a/server/src/test/java/org/elasticsearch/ingest/TrackingResultProcessorTests.java +++ b/server/src/test/java/org/elasticsearch/ingest/TrackingResultProcessorTests.java @@ -29,6 +29,7 @@ import static org.elasticsearch.ingest.CompoundProcessor.ON_FAILURE_MESSAGE_FIELD; import static org.elasticsearch.ingest.CompoundProcessor.ON_FAILURE_PROCESSOR_TAG_FIELD; import static org.elasticsearch.ingest.CompoundProcessor.ON_FAILURE_PROCESSOR_TYPE_FIELD; +import static org.elasticsearch.ingest.IngestDocumentMatcher.assertIngestDocument; import static org.elasticsearch.ingest.PipelineProcessorTests.createIngestService; import static org.elasticsearch.ingest.TrackingResultProcessor.decorate; import static org.hamcrest.CoreMatchers.equalTo; @@ -68,7 +69,7 @@ public void testActualProcessor() throws Exception { assertThat(actualProcessor.getInvokedCounter(), equalTo(1)); assertThat(resultList.size(), equalTo(1)); - assertThat(resultList.get(0).getIngestDocument(), equalTo(expectedResult.getIngestDocument())); + assertIngestDocument(resultList.get(0).getIngestDocument(), expectedResult.getIngestDocument()); assertThat(resultList.get(0).getFailure(), nullValue()); assertThat(resultList.get(0).getProcessorTag(), equalTo(expectedResult.getProcessorTag())); } @@ -236,7 +237,7 @@ public void testActualCompoundProcessorWithIgnoreFailure() throws Exception { ); assertThat(testProcessor.getInvokedCounter(), equalTo(1)); assertThat(resultList.size(), equalTo(1)); - assertThat(resultList.get(0).getIngestDocument(), equalTo(expectedResult.getIngestDocument())); + assertIngestDocument(resultList.get(0).getIngestDocument(), expectedResult.getIngestDocument()); assertThat(resultList.get(0).getFailure(), sameInstance(exception)); assertThat(resultList.get(0).getProcessorTag(), equalTo(expectedResult.getProcessorTag())); } @@ -291,7 +292,7 @@ public void testActualCompoundProcessorWithFalseConditional() throws Exception { assertFalse(resultList.get(2).getIngestDocument().hasField(key2)); assertTrue(resultList.get(2).getIngestDocument().hasField(key3)); - assertThat(resultList.get(2).getIngestDocument(), equalTo(expectedResult.getIngestDocument())); + assertIngestDocument(resultList.get(2).getIngestDocument(), expectedResult.getIngestDocument()); assertThat(resultList.get(2).getFailure(), nullValue()); assertThat(resultList.get(2).getProcessorTag(), nullValue()); } @@ -351,7 +352,7 @@ public void testActualPipelineProcessor() throws Exception { assertTrue(resultList.get(2).getIngestDocument().hasField(key2)); assertFalse(resultList.get(2).getIngestDocument().hasField(key3)); - assertThat(resultList.get(3).getIngestDocument(), equalTo(expectedResult.getIngestDocument())); + assertIngestDocument(resultList.get(3).getIngestDocument(), expectedResult.getIngestDocument()); assertThat(resultList.get(3).getFailure(), nullValue()); assertThat(resultList.get(3).getProcessorTag(), nullValue()); } @@ -450,7 +451,7 @@ public void testActualPipelineProcessorWithTrueConditional() throws Exception { assertTrue(resultList.get(3).getIngestDocument().hasField(key2)); assertFalse(resultList.get(3).getIngestDocument().hasField(key3)); - assertThat(resultList.get(4).getIngestDocument(), equalTo(expectedResult.getIngestDocument())); + assertIngestDocument(resultList.get(4).getIngestDocument(), expectedResult.getIngestDocument()); assertThat(resultList.get(4).getFailure(), nullValue()); assertThat(resultList.get(4).getProcessorTag(), nullValue()); } @@ -542,7 +543,7 @@ public void testActualPipelineProcessorWithFalseConditional() throws Exception { assertThat(resultList.get(2).getConditionalWithResult().v1(), equalTo(scriptName)); assertThat(resultList.get(2).getConditionalWithResult().v2(), is(Boolean.FALSE)); - assertThat(resultList.get(3).getIngestDocument(), equalTo(expectedResult.getIngestDocument())); + assertIngestDocument(resultList.get(3).getIngestDocument(), expectedResult.getIngestDocument()); assertThat(resultList.get(3).getFailure(), nullValue()); assertThat(resultList.get(3).getProcessorTag(), nullValue()); } @@ -611,7 +612,7 @@ public void testActualPipelineProcessorWithHandledFailure() throws Exception { assertTrue(resultList.get(3).getIngestDocument().hasField(key2)); assertFalse(resultList.get(3).getIngestDocument().hasField(key3)); - assertThat(resultList.get(4).getIngestDocument(), equalTo(expectedResult.getIngestDocument())); + assertIngestDocument(resultList.get(4).getIngestDocument(), expectedResult.getIngestDocument()); assertThat(resultList.get(4).getFailure(), nullValue()); assertThat(resultList.get(4).getProcessorTag(), nullValue()); } @@ -746,14 +747,17 @@ public void testActualPipelineProcessorRepeatedInvocation() throws Exception { assertNull(resultList.get(0).getConditionalWithResult()); assertThat(resultList.get(0).getType(), equalTo("pipeline")); - assertThat(resultList.get(1).getIngestDocument(), not(equalTo(expectedResult.getIngestDocument()))); + assertThat( + resultList.get(1).getIngestDocument().getFieldValue(key1, Integer.class), + not(equalTo(expectedResult.getIngestDocument().getFieldValue(key1, Integer.class))) + ); assertThat(resultList.get(1).getFailure(), nullValue()); assertThat(resultList.get(1).getProcessorTag(), nullValue()); assertNull(resultList.get(2).getConditionalWithResult()); assertThat(resultList.get(2).getType(), equalTo("pipeline")); - assertThat(resultList.get(3).getIngestDocument(), equalTo(expectedResult.getIngestDocument())); + assertIngestDocument(resultList.get(3).getIngestDocument(), expectedResult.getIngestDocument()); assertThat(resultList.get(3).getFailure(), nullValue()); assertThat(resultList.get(3).getProcessorTag(), nullValue()); diff --git a/test/framework/src/main/java/org/elasticsearch/ingest/IngestDocumentMatcher.java b/test/framework/src/main/java/org/elasticsearch/ingest/IngestDocumentMatcher.java index 7257253ec2707..adeac905dee53 100644 --- a/test/framework/src/main/java/org/elasticsearch/ingest/IngestDocumentMatcher.java +++ b/test/framework/src/main/java/org/elasticsearch/ingest/IngestDocumentMatcher.java @@ -12,17 +12,32 @@ import java.util.Map; import java.util.Objects; -public class IngestDocumentMatcher { +public final class IngestDocumentMatcher { + + private IngestDocumentMatcher() { + // utility class + } + /** * Helper method to assert the equivalence between two IngestDocuments. * - * @param docA first document to compare - * @param docB second document to compare + * @param expected first document to compare + * @param actual second document to compare */ - public static void assertIngestDocument(IngestDocument docA, IngestDocument docB) { - if ((deepEquals(docA.getIngestMetadata(), docB.getIngestMetadata(), true) - && deepEquals(docA.getSourceAndMetadata(), docB.getSourceAndMetadata(), false)) == false) { - throw new AssertionError("Expected [" + docA + "] but received [" + docB + "]."); + public static void assertIngestDocument(IngestDocument expected, IngestDocument actual) { + // trivially true: if they're both null, then all is well + if (expected == null && actual == null) { + return; + } + + // if only one is null, however, then that's not okay + if ((expected == null || actual == null)) { + throw new AssertionError("Expected [" + expected + "] but received [" + actual + "]."); + } + + if ((deepEquals(expected.getIngestMetadata(), actual.getIngestMetadata(), true) + && deepEquals(expected.getSourceAndMetadata(), actual.getSourceAndMetadata(), false)) == false) { + throw new AssertionError("Expected [" + expected + "] but received [" + actual + "]."); } } diff --git a/test/framework/src/test/java/org/elasticsearch/ingest/IngestDocumentMatcherTests.java b/test/framework/src/test/java/org/elasticsearch/ingest/IngestDocumentMatcherTests.java index 8d1452e996860..1b850f9361dcc 100644 --- a/test/framework/src/test/java/org/elasticsearch/ingest/IngestDocumentMatcherTests.java +++ b/test/framework/src/test/java/org/elasticsearch/ingest/IngestDocumentMatcherTests.java @@ -64,7 +64,26 @@ public void testOnTypeConflict() { assertThrowsOnComparision(document1, document2); } + public void testNestedMapArrayEquivalence() { + IngestDocument ingestDocument = new IngestDocument(new HashMap<>(), new HashMap<>()); + // Test that equality still works when the ingest document uses primitive arrays, + // since normal .equals() methods would not work for Maps containing these arrays. + byte[] numbers = new byte[] { 0, 1, 2 }; + ingestDocument.setFieldValue("some.nested.array", numbers); + IngestDocument copy = new IngestDocument(ingestDocument); + byte[] copiedNumbers = copy.getFieldValue("some.nested.array", byte[].class); + assertArrayEquals(numbers, copiedNumbers); + assertNotEquals(numbers, copiedNumbers); + assertIngestDocument(ingestDocument, copy); + } + + public void testNullsAreEqual() { + assertIngestDocument(null, null); + } + private static void assertThrowsOnComparision(IngestDocument document1, IngestDocument document2) { + expectThrows(AssertionError.class, () -> assertIngestDocument(document1, null)); + expectThrows(AssertionError.class, () -> assertIngestDocument(null, document2)); expectThrows(AssertionError.class, () -> assertIngestDocument(document1, document2)); expectThrows(AssertionError.class, () -> assertIngestDocument(document2, document1)); }