Skip to content

Commit

Permalink
Remove IngestDocument equals/hashCode (#99598)
Browse files Browse the repository at this point in the history
  • Loading branch information
@joegallo
joegallo committed Sep 18, 2023
1 parent f361597 commit d87b93f
Show file tree
Hide file tree
Showing 14 changed files with 82 additions and 226 deletions.
4 changes: 2 additions & 2 deletions ...es/ingest-common/src/test/java/org/elasticsearch/ingest/common/ForEachProcessorTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -344,7 +344,7 @@ public void testAppendingToTheSameField() {
execProcessor(processor, ingestDocument, (result, e) -> {});
assertThat(testProcessor.getInvokedCounter(), equalTo(2));
ingestDocument.removeField("_ingest._value");
assertThat(ingestDocument, equalTo(originalIngestDocument));
assertIngestDocument(ingestDocument, originalIngestDocument);
}

public void testRemovingFromTheSameField() {
Expand All @@ -355,7 +355,7 @@ public void testRemovingFromTheSameField() {
execProcessor(processor, ingestDocument, (result, e) -> {});
assertThat(testProcessor.getInvokedCounter(), equalTo(2));
ingestDocument.removeField("_ingest._value");
assertThat(ingestDocument, equalTo(originalIngestDocument));
assertIngestDocument(ingestDocument, originalIngestDocument);
}

public void testMapIteration() {
Expand Down
2 changes: 1 addition & 1 deletion modules/ingest-common/src/test/java/org/elasticsearch/ingest/common/GrokProcessorTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ public void testMatchWithoutCaptures() throws Exception {
MatcherWatchdog.noop()
);
processor.execute(doc);
assertThat(doc, equalTo(originalDoc));
assertIngestDocument(doc, originalDoc);
}

public void testNullField() {
Expand Down
18 changes: 0 additions & 18 deletions server/src/main/java/org/elasticsearch/action/ingest/WriteableIngestDocument.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@
import java.io.IOException;
import java.time.ZonedDateTime;
import java.util.Map;
import java.util.Objects;

import static org.elasticsearch.xcontent.ConstructingObjectParser.constructorArg;
import static org.elasticsearch.xcontent.ConstructingObjectParser.optionalConstructorArg;
Expand Down Expand Up @@ -140,23 +139,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();
Expand Down
18 changes: 0 additions & 18 deletions server/src/main/java/org/elasticsearch/ingest/IngestDocument.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -911,24 +911,6 @@ public void doNoSelfReferencesCheck(boolean doNoSelfReferencesCheck) {
this.doNoSelfReferencesCheck = doNoSelfReferencesCheck;
}

@Override
public boolean equals(Object obj) {
if (obj == this) {
return true;
}
if (obj == null || getClass() != obj.getClass()) {
return false;
}

IngestDocument other = (IngestDocument) obj;
return Objects.equals(ctxMap, other.ctxMap) && Maps.deepEquals(ingestMetadata, other.ingestMetadata);
}

@Override
public int hashCode() {
return Objects.hash(ctxMap, ingestMetadata);
}

@Override
public String toString() {
return "IngestDocument{" + " sourceAndMetadata=" + ctxMap + ", ingestMetadata=" + ingestMetadata + '}';
Expand Down
5 changes: 2 additions & 3 deletions server/src/main/java/org/elasticsearch/script/CtxMap.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@

package org.elasticsearch.script;

import org.elasticsearch.common.util.Maps;
import org.elasticsearch.common.util.set.Sets;

import java.util.AbstractCollection;
Expand Down Expand Up @@ -337,9 +336,9 @@ public Object setValue(Object value) {
public boolean equals(Object o) {
if (this == o) return true;
if ((o instanceof CtxMap) == false) return false;
if (super.equals(o) == false) return false;
CtxMap<?> ctxMap = (CtxMap<?>) o;
if (Maps.deepEquals(this, ctxMap) == false) return false;
return Maps.deepEquals(source, ctxMap.source) && metadata.equals(ctxMap.metadata);
return source.equals(ctxMap.source) && metadata.equals(ctxMap.metadata);
}

@Override
Expand Down
2 changes: 1 addition & 1 deletion server/src/test/java/org/elasticsearch/action/ingest/SimulateDocumentBaseResultTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()));
Expand Down
2 changes: 1 addition & 1 deletion server/src/test/java/org/elasticsearch/action/ingest/SimulateExecutionServiceTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}

Expand Down
2 changes: 1 addition & 1 deletion server/src/test/java/org/elasticsearch/action/ingest/SimulateProcessorResultTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,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()));
Expand Down
78 changes: 9 additions & 69 deletions server/src/test/java/org/elasticsearch/action/ingest/WriteableIngestDocumentTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@

import java.io.IOException;
import java.util.Arrays;
import java.util.Collections;
import java.util.HashMap;
import java.util.Map;
import java.util.Set;
Expand All @@ -45,72 +44,14 @@

public class WriteableIngestDocumentTests extends AbstractXContentTestCase<WriteableIngestDocument> {

@AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/99403")
public void testEqualsAndHashcode() throws Exception {
Map<String, Object> sourceAndMetadata = RandomDocumentPicks.randomSource(random());
int numFields = randomIntBetween(1, IngestDocument.Metadata.values().length);
sourceAndMetadata.put(VERSION.getFieldName(), TestIngestDocument.randomVersion());
for (int i = 0; i < numFields; i++) {
Tuple<String, Object> metadata = TestIngestDocument.randomMetadata();
sourceAndMetadata.put(metadata.v1(), metadata.v2());
}
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());
otherSourceAndMetadata.put(VERSION.getFieldName(), TestIngestDocument.randomVersion());
changed = true;
} else {
otherSourceAndMetadata = new HashMap<>(sourceAndMetadata);
}
if (randomBoolean()) {
numFields = randomIntBetween(1, IngestDocument.Metadata.values().length);
for (int i = 0; i < numFields; i++) {
Tuple<String, Object> metadata = randomValueOtherThanMany(
t -> t.v2().equals(sourceAndMetadata.get(t.v1())),
TestIngestDocument::randomMetadata
);
otherSourceAndMetadata.put(metadata.v1(), metadata.v2());
}
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 {
Expand Down Expand Up @@ -166,8 +107,7 @@ public void testToXContent() throws IOException {
sourceAndMetadata.putAll(toXContentSource);
ingestDocument.getMetadata().keySet().forEach(k -> sourceAndMetadata.put(k, ingestDocument.getMetadata().get(k)));
IngestDocument serializedIngestDocument = new IngestDocument(sourceAndMetadata, toXContentIngestMetadata);
// TODO(stu): is this test correct? Comparing against ingestDocument fails due to incorrectly failed byte array comparisons
assertThat(serializedIngestDocument, equalTo(serializedIngestDocument));
assertIngestDocument(writeableIngestDocument.getIngestDocument(), serializedIngestDocument);
}

public void testXContentHashSetSerialization() throws Exception {
Expand All @@ -189,7 +129,7 @@ public void testCopiesTheIngestDocument() {
IngestDocument document = createRandomIngestDoc();
WriteableIngestDocument wid = new WriteableIngestDocument(document);

assertThat(wid.getIngestDocument(), equalTo(document));
assertIngestDocument(wid.getIngestDocument(), document);
assertThat(wid.getIngestDocument(), not(sameInstance(document)));
}

Expand Down
77 changes: 0 additions & 77 deletions server/src/test/java/org/elasticsearch/ingest/IngestDocumentTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@

package org.elasticsearch.ingest;

import org.elasticsearch.core.Tuple;
import org.elasticsearch.test.ESTestCase;
import org.hamcrest.Matchers;
import org.junit.Before;
Expand Down Expand Up @@ -958,70 +957,6 @@ public void testRemoveEmptyField() {
}
}

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++) {
Tuple<String, Object> metadata = TestIngestDocument.randomMetadata();
sourceAndMetadata.put(metadata.v1(), metadata.v2());
}
sourceAndMetadata.putIfAbsent("_version", TestIngestDocument.randomVersion());
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));
}
// this is testing equality so use the wire constructor
IngestDocument ingestDocument = new IngestDocument(sourceAndMetadata, ingestMetadata);

boolean changed = false;
Map<String, Object> otherSourceAndMetadata;
if (randomBoolean()) {
otherSourceAndMetadata = RandomDocumentPicks.randomSource(random());
otherSourceAndMetadata.putIfAbsent("_version", TestIngestDocument.randomVersion());
changed = true;
} else {
otherSourceAndMetadata = new HashMap<>(sourceAndMetadata);
}
if (randomBoolean()) {
numFields = randomIntBetween(1, IngestDocument.Metadata.values().length);
for (int i = 0; i < numFields; i++) {
Tuple<String, Object> metadata;
do {
metadata = TestIngestDocument.randomMetadata();
} while (metadata.v2().equals(sourceAndMetadata.get(metadata.v1()))); // must actually be a change
otherSourceAndMetadata.put(metadata.v1(), metadata.v2());
}
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 = Map.copyOf(ingestMetadata);
}

IngestDocument otherIngestDocument = 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()));
IngestDocument thirdIngestDocument = new IngestDocument(Map.copyOf(sourceAndMetadata), Map.copyOf(ingestMetadata));
assertThat(thirdIngestDocument, equalTo(ingestDocument));
assertThat(ingestDocument, equalTo(thirdIngestDocument));
assertThat(ingestDocument.hashCode(), equalTo(thirdIngestDocument.hashCode()));
}
}

public void testIngestMetadataTimestamp() throws Exception {
long before = System.currentTimeMillis();
IngestDocument ingestDocument = RandomDocumentPicks.randomIngestDocument(random());
Expand Down Expand Up @@ -1169,16 +1104,4 @@ public void testIndexHistory() {
assertFalse(ingestDocument.updateIndexHistory(index1));
assertThat(ingestDocument.getIndexHistory(), Matchers.contains(index1, index2));
}

public void testEqualsAndHashCodeWithArray() {
// 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);
assertThat(copy, equalTo(ingestDocument));
}
}
3 changes: 2 additions & 1 deletion server/src/test/java/org/elasticsearch/ingest/PipelineProcessorTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import java.util.concurrent.TimeUnit;
import java.util.function.LongSupplier;

import static org.elasticsearch.ingest.IngestDocumentMatcher.assertIngestDocument;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.notNullValue;
import static org.hamcrest.Matchers.nullValue;
Expand Down Expand Up @@ -60,7 +61,7 @@ public String getDescription() {
Map<String, Object> config = new HashMap<>();
config.put("name", pipelineId);
factory.create(Map.of(), null, null, config).execute(testIngestDocument, (result, e) -> {});
assertEquals(testIngestDocument, invoked.get());
assertIngestDocument(testIngestDocument, invoked.get());
}

public void testThrowsOnMissingPipeline() throws Exception {
Expand Down

0 comments on commit d87b93f

Please sign in to comment.