Skip to content

Commit 112704b

Browse files
andrrossandrross
authored
Add missing data types to IngestDocument deep copy (opensearch-project#14380)
PR opensearch-project#11725 added a new deep copy in the ScriptProcessor flow. If a script uses a Short or Byte data type then this new deep copy introduced a regression. This commit fixes that regression. However, it appears there has been an existing bug where using a Character type in the same way will fail (this failed before PR 11725). The failure is different, and appears to be related to something deeping in the XContent serialization layer. For now, I have fixed the regression but not yet dug into the failure with the Character data type. I have added a test that expects this failure. Resolves opensearch-project#14379 Signed-off-by: Andrew Ross <andrross@amazon.com>
1 parent 1d14569 commit 112704b

File tree

4 files changed

+111
-15
lines changed

4 files changed

+111
-15
lines changed

‎CHANGELOG.md

+1
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
2525
### Removed
2626

2727
### Fixed
28+
- Fix handling of Short and Byte data types in ScriptProcessor ingest pipeline ([#14379](https://github.com/opensearch-project/OpenSearch/issues/14379))
2829

2930
### Security
3031

‎modules/ingest-common/src/yamlRestTest/resources/rest-api-spec/test/ingest/190_script_processor.yml

+75
Original file line numberDiff line numberDiff line change
@@ -278,3 +278,78 @@ teardown:
278278
body: {source_field: "fooBar", foo: {foo: "bar"}}
279279
- match: { error.root_cause.0.type: "illegal_argument_exception" }
280280
- match: { error.root_cause.0.reason: "Iterable object is self-referencing itself (ingest script)" }
281+
282+
---
283+
"Test painless data types":
284+
- do:
285+
ingest.put_pipeline:
286+
id: "my_pipeline"
287+
body: >
288+
{
289+
"description": "_description",
290+
"processors": [
291+
{
292+
"script" : {
293+
"source" : "ctx.byte = (byte)127;ctx.short = (short)32767;ctx.int = (int)2147483647;ctx.long = (long)9223372036854775807L;ctx.float = (float)0.1;ctx.double = (double)0.1;ctx.boolean = (boolean)true"
294+
}
295+
},
296+
{
297+
"script" : {
298+
"source" : "ctx.other_field = 'other_field'"
299+
}
300+
}
301+
]
302+
}
303+
- match: { acknowledged: true }
304+
305+
- do:
306+
index:
307+
index: test
308+
id: 1
309+
pipeline: "my_pipeline"
310+
body: {source_field: "FooBar"}
311+
312+
- do:
313+
get:
314+
index: test
315+
id: 1
316+
- match: { _source.byte: 127 }
317+
- match: { _source.int: 2147483647 }
318+
- match: { _source.long: 9223372036854775807 }
319+
- gt: { _source.float: 0.0 }
320+
- lt: { _source.float: 0.2 }
321+
- gt: { _source.double: 0.0 }
322+
- lt: { _source.double: 0.2 }
323+
- match: { _source.boolean: true }
324+
325+
---
326+
"Test char type fails":
327+
- do:
328+
ingest.put_pipeline:
329+
id: "my_pipeline"
330+
body: >
331+
{
332+
"description": "_description",
333+
"processors": [
334+
{
335+
"script" : {
336+
"source" : "ctx.char = (char)'a'"
337+
}
338+
},
339+
{
340+
"script" : {
341+
"source" : "ctx.other_field = 'other_field'"
342+
}
343+
}
344+
]
345+
}
346+
- match: { acknowledged: true }
347+
348+
- do:
349+
catch: bad_request
350+
index:
351+
index: test
352+
id: 1
353+
pipeline: "my_pipeline"
354+
body: {source_field: "FooBar"}
355+
- match: { error.root_cause.0.type: "illegal_argument_exception" }

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

+3
Original file line numberDiff line numberDiff line change
@@ -776,6 +776,9 @@ public static Object deepCopy(Object value) {
776776
byte[] bytes = (byte[]) value;
777777
return Arrays.copyOf(bytes, bytes.length);
778778
} else if (value == null
779+
|| value instanceof Byte
780+
|| value instanceof Character
781+
|| value instanceof Short
779782
|| value instanceof String
780783
|| value instanceof Integer
781784
|| value instanceof Long

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

+32-15
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,7 @@ public class IngestDocumentTests extends OpenSearchTestCase {
6262

6363
private static final ZonedDateTime BOGUS_TIMESTAMP = ZonedDateTime.of(2016, 10, 23, 0, 0, 0, 0, ZoneOffset.UTC);
6464
private IngestDocument ingestDocument;
65+
private int initialSourceAndMetadataSize;
6566

6667
@Before
6768
public void setTestIngestDocument() {
@@ -70,7 +71,6 @@ public void setTestIngestDocument() {
7071
ingestMap.put("timestamp", BOGUS_TIMESTAMP);
7172
document.put("_ingest", ingestMap);
7273
document.put("foo", "bar");
73-
document.put("int", 123);
7474
Map<String, Object> innerObject = new HashMap<>();
7575
innerObject.put("buzz", "hello world");
7676
innerObject.put("foo_null", null);
@@ -92,7 +92,17 @@ public void setTestIngestDocument() {
9292
list2.add("bar");
9393
list2.add("baz");
9494
document.put("list2", list2);
95+
document.put("byte", (byte) 1);
96+
document.put("short", (short) 2);
97+
document.put("int", Integer.MAX_VALUE);
98+
document.put("long", Long.MAX_VALUE);
99+
document.put("float", 0.1f);
100+
document.put("double", 0.1d);
101+
document.put("char", 'a');
102+
document.put("string", "A test string \uD83C\uDF89");
103+
document.put("datetime", ZonedDateTime.parse("2007-12-03T10:15:30+01:00[Europe/Paris]"));
95104
ingestDocument = new IngestDocument("index", "id", null, null, null, document);
105+
initialSourceAndMetadataSize = 16; // i.e. ingestDocument.getSourceAndMetadata().size()
96106
}
97107

98108
public void testSelfReferencingSource() {
@@ -101,11 +111,18 @@ public void testSelfReferencingSource() {
101111
expectThrows(IllegalArgumentException.class, () -> IngestDocument.deepCopyMap(value));
102112
}
103113

114+
public void testCopy() {
115+
final IngestDocument copy = new IngestDocument(ingestDocument);
116+
assertThat(copy, equalTo(ingestDocument));
117+
assertThat(copy.getSourceAndMetadata(), not(sameInstance(ingestDocument.getSourceAndMetadata())));
118+
assertThat(copy.getIngestMetadata(), not(sameInstance(ingestDocument.getIngestMetadata())));
119+
}
120+
104121
public void testSimpleGetFieldValue() {
105122
assertThat(ingestDocument.getFieldValue("foo", String.class), equalTo("bar"));
106-
assertThat(ingestDocument.getFieldValue("int", Integer.class), equalTo(123));
123+
assertThat(ingestDocument.getFieldValue("int", Integer.class), equalTo(Integer.MAX_VALUE));
107124
assertThat(ingestDocument.getFieldValue("_source.foo", String.class), equalTo("bar"));
108-
assertThat(ingestDocument.getFieldValue("_source.int", Integer.class), equalTo(123));
125+
assertThat(ingestDocument.getFieldValue("_source.int", Integer.class), equalTo(Integer.MAX_VALUE));
109126
assertThat(ingestDocument.getFieldValue("_index", String.class), equalTo("index"));
110127
assertThat(ingestDocument.getFieldValue("_id", String.class), equalTo("id"));
111128
assertThat(
@@ -578,7 +595,7 @@ public void testAppendFieldValueConvertIntegerToList() {
578595
@SuppressWarnings("unchecked")
579596
List<Object> list = (List<Object>) object;
580597
assertThat(list.size(), equalTo(2));
581-
assertThat(list.get(0), equalTo(123));
598+
assertThat(list.get(0), equalTo(Integer.MAX_VALUE));
582599
assertThat(list.get(1), equalTo(456));
583600
}
584601

@@ -589,7 +606,7 @@ public void testAppendFieldValuesConvertIntegerToList() {
589606
@SuppressWarnings("unchecked")
590607
List<Object> list = (List<Object>) object;
591608
assertThat(list.size(), equalTo(3));
592-
assertThat(list.get(0), equalTo(123));
609+
assertThat(list.get(0), equalTo(Integer.MAX_VALUE));
593610
assertThat(list.get(1), equalTo(456));
594611
assertThat(list.get(2), equalTo(789));
595612
}
@@ -812,23 +829,23 @@ public void testSetFieldValueEmptyName() {
812829

813830
public void testRemoveField() {
814831
ingestDocument.removeField("foo");
815-
assertThat(ingestDocument.getSourceAndMetadata().size(), equalTo(7));
832+
assertThat(ingestDocument.getSourceAndMetadata().size(), equalTo(initialSourceAndMetadataSize - 1));
816833
assertThat(ingestDocument.getSourceAndMetadata().containsKey("foo"), equalTo(false));
817834
ingestDocument.removeField("_index");
818-
assertThat(ingestDocument.getSourceAndMetadata().size(), equalTo(6));
835+
assertThat(ingestDocument.getSourceAndMetadata().size(), equalTo(initialSourceAndMetadataSize - 2));
819836
assertThat(ingestDocument.getSourceAndMetadata().containsKey("_index"), equalTo(false));
820837
ingestDocument.removeField("_source.fizz");
821-
assertThat(ingestDocument.getSourceAndMetadata().size(), equalTo(5));
838+
assertThat(ingestDocument.getSourceAndMetadata().size(), equalTo(initialSourceAndMetadataSize - 3));
822839
assertThat(ingestDocument.getSourceAndMetadata().containsKey("fizz"), equalTo(false));
823840
assertThat(ingestDocument.getIngestMetadata().size(), equalTo(1));
824841
ingestDocument.removeField("_ingest.timestamp");
825-
assertThat(ingestDocument.getSourceAndMetadata().size(), equalTo(5));
842+
assertThat(ingestDocument.getSourceAndMetadata().size(), equalTo(initialSourceAndMetadataSize - 3));
826843
assertThat(ingestDocument.getIngestMetadata().size(), equalTo(0));
827844
}
828845

829846
public void testRemoveInnerField() {
830847
ingestDocument.removeField("fizz.buzz");
831-
assertThat(ingestDocument.getSourceAndMetadata().size(), equalTo(8));
848+
assertThat(ingestDocument.getSourceAndMetadata().size(), equalTo(initialSourceAndMetadataSize));
832849
assertThat(ingestDocument.getSourceAndMetadata().get("fizz"), instanceOf(Map.class));
833850
@SuppressWarnings("unchecked")
834851
Map<String, Object> map = (Map<String, Object>) ingestDocument.getSourceAndMetadata().get("fizz");
@@ -837,17 +854,17 @@ public void testRemoveInnerField() {
837854

838855
ingestDocument.removeField("fizz.foo_null");
839856
assertThat(map.size(), equalTo(2));
840-
assertThat(ingestDocument.getSourceAndMetadata().size(), equalTo(8));
857+
assertThat(ingestDocument.getSourceAndMetadata().size(), equalTo(initialSourceAndMetadataSize));
841858
assertThat(ingestDocument.getSourceAndMetadata().containsKey("fizz"), equalTo(true));
842859

843860
ingestDocument.removeField("fizz.1");
844861
assertThat(map.size(), equalTo(1));
845-
assertThat(ingestDocument.getSourceAndMetadata().size(), equalTo(8));
862+
assertThat(ingestDocument.getSourceAndMetadata().size(), equalTo(initialSourceAndMetadataSize));
846863
assertThat(ingestDocument.getSourceAndMetadata().containsKey("fizz"), equalTo(true));
847864

848865
ingestDocument.removeField("fizz.list");
849866
assertThat(map.size(), equalTo(0));
850-
assertThat(ingestDocument.getSourceAndMetadata().size(), equalTo(8));
867+
assertThat(ingestDocument.getSourceAndMetadata().size(), equalTo(initialSourceAndMetadataSize));
851868
assertThat(ingestDocument.getSourceAndMetadata().containsKey("fizz"), equalTo(true));
852869
}
853870

@@ -883,7 +900,7 @@ public void testRemoveSourceObject() {
883900

884901
public void testRemoveIngestObject() {
885902
ingestDocument.removeField("_ingest");
886-
assertThat(ingestDocument.getSourceAndMetadata().size(), equalTo(7));
903+
assertThat(ingestDocument.getSourceAndMetadata().size(), equalTo(initialSourceAndMetadataSize - 1));
887904
assertThat(ingestDocument.getSourceAndMetadata().containsKey("_ingest"), equalTo(false));
888905
}
889906

@@ -905,7 +922,7 @@ public void testRemoveEmptyPathAfterStrippingOutPrefix() {
905922

906923
public void testListRemoveField() {
907924
ingestDocument.removeField("list.0.field");
908-
assertThat(ingestDocument.getSourceAndMetadata().size(), equalTo(8));
925+
assertThat(ingestDocument.getSourceAndMetadata().size(), equalTo(initialSourceAndMetadataSize));
909926
assertThat(ingestDocument.getSourceAndMetadata().containsKey("list"), equalTo(true));
910927
Object object = ingestDocument.getSourceAndMetadata().get("list");
911928
assertThat(object, instanceOf(List.class));

0 commit comments

Comments
 (0)