Skip to content

Commit 2f8fa89

Browse files
joegallojoegallo
authored
Refactor WriteableIngestDocument (#99324) (#100224)
1 parent f1f2e8d commit 2f8fa89

File tree

13 files changed

+176
-115
lines changed

13 files changed

+176
-115
lines changed

‎server/src/main/java/org/elasticsearch/action/ingest/SimulateDocumentBaseResult.java‎

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -51,12 +51,17 @@ public final class SimulateDocumentBaseResult implements SimulateDocumentResult
5151
}
5252

5353
public SimulateDocumentBaseResult(IngestDocument ingestDocument) {
54+
Exception failure = null;
55+
WriteableIngestDocument wid = null;
5456
if (ingestDocument != null) {
55-
this.ingestDocument = new WriteableIngestDocument(ingestDocument);
56-
} else {
57-
this.ingestDocument = null;
57+
try {
58+
wid = new WriteableIngestDocument(ingestDocument);
59+
} catch (Exception ex) {
60+
failure = ex;
61+
}
5862
}
59-
this.failure = null;
63+
this.ingestDocument = wid;
64+
this.failure = failure;
6065
}
6166

6267
public SimulateDocumentBaseResult(Exception failure) {

‎server/src/main/java/org/elasticsearch/action/ingest/SimulateProcessorResult.java‎

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -126,7 +126,19 @@ public SimulateProcessorResult(
126126
) {
127127
this.processorTag = processorTag;
128128
this.description = description;
129-
this.ingestDocument = (ingestDocument == null) ? null : new WriteableIngestDocument(ingestDocument);
129+
WriteableIngestDocument wid = null;
130+
if (ingestDocument != null) {
131+
try {
132+
wid = new WriteableIngestDocument(ingestDocument);
133+
} catch (Exception ex) {
134+
// if there was a failure already, then track it as a suppressed exception
135+
if (failure != null) {
136+
ex.addSuppressed(failure);
137+
}
138+
failure = ex;
139+
}
140+
}
141+
this.ingestDocument = wid;
130142
this.failure = failure;
131143
this.conditionalWithResult = conditionalWithResult;
132144
this.type = type;

‎server/src/main/java/org/elasticsearch/action/ingest/WriteableIngestDocument.java‎

Lines changed: 17 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,6 @@
2626
import java.util.Date;
2727
import java.util.HashMap;
2828
import java.util.Map;
29-
import java.util.Objects;
3029

3130
import static org.elasticsearch.xcontent.ConstructingObjectParser.constructorArg;
3231
import static org.elasticsearch.xcontent.ConstructingObjectParser.optionalConstructorArg;
@@ -57,7 +56,7 @@ final class WriteableIngestDocument implements Writeable, ToXContentFragment {
5756
sourceAndMetadata.put(Metadata.VERSION_TYPE.getFieldName(), a[5]);
5857
}
5958
sourceAndMetadata.putAll((Map<String, Object>) a[6]);
60-
return new WriteableIngestDocument(new IngestDocument(sourceAndMetadata, (Map<String, Object>) a[7]));
59+
return new WriteableIngestDocument(sourceAndMetadata, (Map<String, Object>) a[7]);
6160
}
6261
);
6362
static {
@@ -84,9 +83,24 @@ final class WriteableIngestDocument implements Writeable, ToXContentFragment {
8483
PARSER.declareObject(constructorArg(), INGEST_DOC_PARSER, new ParseField(DOC_FIELD));
8584
}
8685

86+
/**
87+
* Builds a writeable ingest document that wraps a copy of the passed-in, non-null ingest document.
88+
*
89+
* @throws IllegalArgumentException if the passed-in ingest document references itself
90+
*/
8791
WriteableIngestDocument(IngestDocument ingestDocument) {
8892
assert ingestDocument != null;
89-
this.ingestDocument = ingestDocument;
93+
this.ingestDocument = new IngestDocument(ingestDocument); // internal defensive copy
94+
}
95+
96+
/**
97+
* Builds a writeable ingest document by constructing the wrapped ingest document from the passed-in maps.
98+
* <p>
99+
* This is intended for cases like deserialization, where we know the passed-in maps aren't self-referencing,
100+
* and where a defensive copy is unnecessary.
101+
*/
102+
private WriteableIngestDocument(Map<String, Object> sourceAndMetadata, Map<String, Object> ingestMetadata) {
103+
this.ingestDocument = new IngestDocument(sourceAndMetadata, ingestMetadata);
90104
}
91105

92106
WriteableIngestDocument(StreamInput in) throws IOException {
@@ -132,23 +146,6 @@ public static WriteableIngestDocument fromXContent(XContentParser parser) {
132146
return PARSER.apply(parser, null);
133147
}
134148

135-
@Override
136-
public boolean equals(Object o) {
137-
if (this == o) {
138-
return true;
139-
}
140-
if (o == null || getClass() != o.getClass()) {
141-
return false;
142-
}
143-
WriteableIngestDocument that = (WriteableIngestDocument) o;
144-
return Objects.equals(ingestDocument, that.ingestDocument);
145-
}
146-
147-
@Override
148-
public int hashCode() {
149-
return Objects.hash(ingestDocument);
150-
}
151-
152149
@Override
153150
public String toString() {
154151
return ingestDocument.toString();

‎server/src/main/java/org/elasticsearch/ingest/IngestDocument.java‎

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
package org.elasticsearch.ingest;
1010

1111
import org.elasticsearch.common.Strings;
12+
import org.elasticsearch.common.util.CollectionUtils;
1213
import org.elasticsearch.common.util.LazyMap;
1314
import org.elasticsearch.index.VersionType;
1415
import org.elasticsearch.index.mapper.IdFieldMapper;
@@ -82,16 +83,26 @@ public IngestDocument(
8283
}
8384

8485
/**
85-
* Copy constructor that creates a new {@link IngestDocument} which has exactly the same properties as the one provided as argument
86+
* Copy constructor that creates a new {@link IngestDocument} which has exactly the same properties as the one provided.
87+
*
88+
* @throws IllegalArgumentException if the passed-in ingest document references itself
8689
*/
8790
public IngestDocument(IngestDocument other) {
88-
this(deepCopyMap(other.sourceAndMetadata), deepCopyMap(other.ingestMetadata));
91+
this(deepCopyMap(ensureNoSelfReferences(other.sourceAndMetadata)), deepCopyMap(other.ingestMetadata));
92+
}
93+
94+
/**
95+
* Internal helper utility method to get around the issue that a {@code this(...) } constructor call must be the first statement
96+
* in a constructor. This is only for use in the {@link IngestDocument#IngestDocument(IngestDocument)} copy constructor, it's not a
97+
* general purpose method.
98+
*/
99+
private static Map<String, Object> ensureNoSelfReferences(Map<String, Object> source) {
100+
CollectionUtils.ensureNoSelfReferences(source, null);
101+
return source;
89102
}
90103

91104
/**
92-
* Constructor needed for testing that allows to create a new {@link IngestDocument} given the provided elasticsearch metadata,
93-
* source and ingest metadata. This is needed because the ingest metadata will be initialized with the current timestamp at
94-
* init time, which makes equality comparisons impossible in tests.
105+
* Constructor to create an IngestDocument from its constituent maps. The maps are shallow copied.
95106
*/
96107
public IngestDocument(Map<String, Object> sourceAndMetadata, Map<String, Object> ingestMetadata) {
97108
this.sourceAndMetadata = sourceAndMetadata;

‎server/src/main/java/org/elasticsearch/ingest/TrackingResultProcessor.java‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -99,7 +99,7 @@ public void execute(IngestDocument ingestDocument, BiConsumer<IngestDocument, Ex
9999
pipelineProcessor.getType(),
100100
pipelineProcessor.getTag(),
101101
pipelineProcessor.getDescription(),
102-
new IngestDocument(ingestDocument),
102+
ingestDocument,
103103
e,
104104
conditionalWithResult
105105
)
@@ -149,7 +149,7 @@ public void execute(IngestDocument ingestDocument, BiConsumer<IngestDocument, Ex
149149
actualProcessor.getType(),
150150
actualProcessor.getTag(),
151151
actualProcessor.getDescription(),
152-
new IngestDocument(ingestDocument),
152+
ingestDocument,
153153
e,
154154
conditionalWithResult
155155
)
@@ -173,7 +173,7 @@ public void execute(IngestDocument ingestDocument, BiConsumer<IngestDocument, Ex
173173
actualProcessor.getType(),
174174
actualProcessor.getTag(),
175175
actualProcessor.getDescription(),
176-
new IngestDocument(ingestDocument),
176+
ingestDocument,
177177
conditionalWithResult
178178
)
179179
);

‎server/src/test/java/org/elasticsearch/action/ingest/SimulateDocumentBaseResultTests.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,7 @@ protected Predicate<String> getRandomFieldsExcludeFilter() {
8888
}
8989

9090
public static void assertEqualDocs(SimulateDocumentBaseResult response, SimulateDocumentBaseResult parsedResponse) {
91-
assertEquals(response.getIngestDocument(), parsedResponse.getIngestDocument());
91+
assertIngestDocument(response.getIngestDocument(), parsedResponse.getIngestDocument());
9292
if (response.getFailure() != null) {
9393
assertNotNull(parsedResponse.getFailure());
9494
assertThat(parsedResponse.getFailure().getMessage(), containsString(response.getFailure().getMessage()));

‎server/src/test/java/org/elasticsearch/action/ingest/SimulateExecutionServiceTests.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -108,7 +108,7 @@ public void testExecuteItem() throws Exception {
108108
assertThat(processor.getInvokedCounter(), equalTo(2));
109109
assertThat(actualItemResponse, instanceOf(SimulateDocumentBaseResult.class));
110110
SimulateDocumentBaseResult simulateDocumentBaseResult = (SimulateDocumentBaseResult) actualItemResponse;
111-
assertThat(simulateDocumentBaseResult.getIngestDocument(), equalTo(ingestDocument));
111+
assertIngestDocument(simulateDocumentBaseResult.getIngestDocument(), ingestDocument);
112112
assertThat(simulateDocumentBaseResult.getFailure(), nullValue());
113113
}
114114

‎server/src/test/java/org/elasticsearch/action/ingest/SimulateProcessorResultTests.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -144,7 +144,7 @@ protected Predicate<String> getRandomFieldsExcludeFilter() {
144144

145145
static void assertEqualProcessorResults(SimulateProcessorResult response, SimulateProcessorResult parsedResponse) {
146146
assertEquals(response.getProcessorTag(), parsedResponse.getProcessorTag());
147-
assertEquals(response.getIngestDocument(), parsedResponse.getIngestDocument());
147+
assertIngestDocument(response.getIngestDocument(), parsedResponse.getIngestDocument());
148148
if (response.getFailure() != null) {
149149
assertNotNull(parsedResponse.getFailure());
150150
assertThat(parsedResponse.getFailure().getMessage(), containsString(response.getFailure().getMessage()));

‎server/src/test/java/org/elasticsearch/action/ingest/WriteableIngestDocumentTests.java‎

Lines changed: 17 additions & 59 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,6 @@
2424

2525
import java.io.IOException;
2626
import java.util.Arrays;
27-
import java.util.Collections;
2827
import java.util.HashMap;
2928
import java.util.Map;
3029
import java.util.StringJoiner;
@@ -36,67 +35,18 @@
3635
import static org.hamcrest.Matchers.instanceOf;
3736
import static org.hamcrest.Matchers.is;
3837
import static org.hamcrest.Matchers.not;
38+
import static org.hamcrest.Matchers.sameInstance;
3939

4040
public class WriteableIngestDocumentTests extends AbstractXContentTestCase<WriteableIngestDocument> {
4141

42-
public void testEqualsAndHashcode() throws Exception {
43-
Map<String, Object> sourceAndMetadata = RandomDocumentPicks.randomSource(random());
44-
int numFields = randomIntBetween(1, IngestDocument.Metadata.values().length);
45-
for (int i = 0; i < numFields; i++) {
46-
sourceAndMetadata.put(randomFrom(IngestDocument.Metadata.values()).getFieldName(), randomAlphaOfLengthBetween(5, 10));
47-
}
48-
Map<String, Object> ingestMetadata = new HashMap<>();
49-
numFields = randomIntBetween(1, 5);
50-
for (int i = 0; i < numFields; i++) {
51-
ingestMetadata.put(randomAlphaOfLengthBetween(5, 10), randomAlphaOfLengthBetween(5, 10));
52-
}
53-
WriteableIngestDocument ingestDocument = new WriteableIngestDocument(new IngestDocument(sourceAndMetadata, ingestMetadata));
54-
55-
boolean changed = false;
56-
Map<String, Object> otherSourceAndMetadata;
57-
if (randomBoolean()) {
58-
otherSourceAndMetadata = RandomDocumentPicks.randomSource(random());
59-
changed = true;
60-
} else {
61-
otherSourceAndMetadata = new HashMap<>(sourceAndMetadata);
62-
}
63-
if (randomBoolean()) {
64-
numFields = randomIntBetween(1, IngestDocument.Metadata.values().length);
65-
for (int i = 0; i < numFields; i++) {
66-
otherSourceAndMetadata.put(randomFrom(IngestDocument.Metadata.values()).getFieldName(), randomAlphaOfLengthBetween(5, 10));
67-
}
68-
changed = true;
69-
}
70-
71-
Map<String, Object> otherIngestMetadata;
72-
if (randomBoolean()) {
73-
otherIngestMetadata = new HashMap<>();
74-
numFields = randomIntBetween(1, 5);
75-
for (int i = 0; i < numFields; i++) {
76-
otherIngestMetadata.put(randomAlphaOfLengthBetween(5, 10), randomAlphaOfLengthBetween(5, 10));
77-
}
78-
changed = true;
79-
} else {
80-
otherIngestMetadata = Collections.unmodifiableMap(ingestMetadata);
81-
}
42+
@Override
43+
protected boolean assertToXContentEquivalence() {
44+
return false;
45+
}
8246

83-
WriteableIngestDocument otherIngestDocument = new WriteableIngestDocument(
84-
new IngestDocument(otherSourceAndMetadata, otherIngestMetadata)
85-
);
86-
if (changed) {
87-
assertThat(ingestDocument, not(equalTo(otherIngestDocument)));
88-
assertThat(otherIngestDocument, not(equalTo(ingestDocument)));
89-
} else {
90-
assertThat(ingestDocument, equalTo(otherIngestDocument));
91-
assertThat(otherIngestDocument, equalTo(ingestDocument));
92-
assertThat(ingestDocument.hashCode(), equalTo(otherIngestDocument.hashCode()));
93-
WriteableIngestDocument thirdIngestDocument = new WriteableIngestDocument(
94-
new IngestDocument(Collections.unmodifiableMap(sourceAndMetadata), Collections.unmodifiableMap(ingestMetadata))
95-
);
96-
assertThat(thirdIngestDocument, equalTo(ingestDocument));
97-
assertThat(ingestDocument, equalTo(thirdIngestDocument));
98-
assertThat(ingestDocument.hashCode(), equalTo(thirdIngestDocument.hashCode()));
99-
}
47+
@Override
48+
protected void assertEqualInstances(WriteableIngestDocument expectedInstance, WriteableIngestDocument newInstance) {
49+
assertIngestDocument(expectedInstance.getIngestDocument(), newInstance.getIngestDocument());
10050
}
10151

10252
public void testSerialization() throws IOException {
@@ -148,7 +98,7 @@ public void testToXContent() throws IOException {
14898
}
14999

150100
IngestDocument serializedIngestDocument = new IngestDocument(toXContentSource, toXContentIngestMetadata);
151-
assertThat(serializedIngestDocument, equalTo(serializedIngestDocument));
101+
assertIngestDocument(serializedIngestDocument, serializedIngestDocument);
152102
}
153103

154104
public void testXContentHashSetSerialization() throws Exception {
@@ -169,6 +119,14 @@ public void testXContentHashSetSerialization() throws Exception {
169119
}
170120
}
171121

122+
public void testCopiesTheIngestDocument() {
123+
IngestDocument document = createRandomIngestDoc();
124+
WriteableIngestDocument wid = new WriteableIngestDocument(document);
125+
126+
assertIngestDocument(wid.getIngestDocument(), document);
127+
assertThat(wid.getIngestDocument(), not(sameInstance(document)));
128+
}
129+
172130
static IngestDocument createRandomIngestDoc() {
173131
XContentType xContentType = randomFrom(XContentType.values());
174132
BytesReference sourceBytes = RandomObjects.randomSource(random(), xContentType);

‎server/src/test/java/org/elasticsearch/ingest/IngestDocumentTests.java‎

Lines changed: 44 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1016,10 +1016,50 @@ public void testIngestMetadataTimestamp() throws Exception {
10161016
}
10171017

10181018
public void testCopyConstructor() {
1019-
IngestDocument ingestDocument = RandomDocumentPicks.randomIngestDocument(random());
1020-
IngestDocument copy = new IngestDocument(ingestDocument);
1021-
assertThat(ingestDocument.getSourceAndMetadata(), not(sameInstance(copy.getSourceAndMetadata())));
1022-
assertIngestDocument(ingestDocument, copy);
1019+
{
1020+
// generic test with a random document and copy
1021+
IngestDocument ingestDocument = RandomDocumentPicks.randomIngestDocument(random());
1022+
IngestDocument copy = new IngestDocument(ingestDocument);
1023+
1024+
// these fields should not be the same instance
1025+
assertThat(ingestDocument.getSourceAndMetadata(), not(sameInstance(copy.getSourceAndMetadata())));
1026+
1027+
// but the two objects should be very much equal to each other
1028+
assertIngestDocument(ingestDocument, copy);
1029+
}
1030+
1031+
{
1032+
// manually punch in a few values
1033+
IngestDocument ingestDocument = RandomDocumentPicks.randomIngestDocument(random());
1034+
ingestDocument.setFieldValue("_index", "foo1");
1035+
ingestDocument.setFieldValue("_id", "bar1");
1036+
ingestDocument.setFieldValue("hello", "world1");
1037+
IngestDocument copy = new IngestDocument(ingestDocument);
1038+
1039+
// make sure the copy matches
1040+
assertIngestDocument(ingestDocument, copy);
1041+
1042+
// change the copy
1043+
copy.setFieldValue("_index", "foo2");
1044+
copy.setFieldValue("_id", "bar2");
1045+
copy.setFieldValue("hello", "world2");
1046+
1047+
// the original shouldn't have changed
1048+
assertThat(ingestDocument.getFieldValue("_index", String.class), equalTo("foo1"));
1049+
assertThat(ingestDocument.getFieldValue("_id", String.class), equalTo("bar1"));
1050+
assertThat(ingestDocument.getFieldValue("hello", String.class), equalTo("world1"));
1051+
}
1052+
1053+
{
1054+
// the copy constructor rejects self-references
1055+
IngestDocument ingestDocument = RandomDocumentPicks.randomIngestDocument(random());
1056+
List<Object> someList = new ArrayList<>();
1057+
someList.add("some string");
1058+
someList.add(someList); // the list contains itself
1059+
ingestDocument.setFieldValue("someList", someList);
1060+
Exception e = expectThrows(IllegalArgumentException.class, () -> new IngestDocument(ingestDocument));
1061+
assertThat(e.getMessage(), equalTo("Iterable object is self-referencing itself"));
1062+
}
10231063
}
10241064

10251065
public void testCopyConstructorWithZonedDateTime() {

0 commit comments

Comments
 (0)