Skip to content

Commit

Permalink
Add missing data types to IngestDocument deep copy (opensearch-projec…
Browse files Browse the repository at this point in the history 
…t#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 <[email protected]>
  • Loading branch information
@andrross
andrross authored and wangdongyu.danny committed Aug 22, 2024
1 parent fd5cf4c commit 54667c0
Show file tree
Hide file tree
Showing 4 changed files with 111 additions and 15 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
### Removed

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

### Security

Expand Down
75 changes: 75 additions & 0 deletions ...gest-common/src/yamlRestTest/resources/rest-api-spec/test/ingest/190_script_processor.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -278,3 +278,78 @@ teardown:
body: {source_field: "fooBar", foo: {foo: "bar"}}
- match: { error.root_cause.0.type: "illegal_argument_exception" }
- match: { error.root_cause.0.reason: "Iterable object is self-referencing itself (ingest script)" }

---
"Test painless data types":
- do:
ingest.put_pipeline:
id: "my_pipeline"
body: >
{
"description": "_description",
"processors": [
{
"script" : {
"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"
}
},
{
"script" : {
"source" : "ctx.other_field = 'other_field'"
}
}
]
}
- match: { acknowledged: true }

- do:
index:
index: test
id: 1
pipeline: "my_pipeline"
body: {source_field: "FooBar"}

- do:
get:
index: test
id: 1
- match: { _source.byte: 127 }
- match: { _source.int: 2147483647 }
- match: { _source.long: 9223372036854775807 }
- gt: { _source.float: 0.0 }
- lt: { _source.float: 0.2 }
- gt: { _source.double: 0.0 }
- lt: { _source.double: 0.2 }
- match: { _source.boolean: true }

---
"Test char type fails":
- do:
ingest.put_pipeline:
id: "my_pipeline"
body: >
{
"description": "_description",
"processors": [
{
"script" : {
"source" : "ctx.char = (char)'a'"
}
},
{
"script" : {
"source" : "ctx.other_field = 'other_field'"
}
}
]
}
- match: { acknowledged: true }

- do:
catch: bad_request
index:
index: test
id: 1
pipeline: "my_pipeline"
body: {source_field: "FooBar"}
- match: { error.root_cause.0.type: "illegal_argument_exception" }
3 changes: 3 additions & 0 deletions server/src/main/java/org/opensearch/ingest/IngestDocument.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -776,6 +776,9 @@ public static Object deepCopy(Object value) {
byte[] bytes = (byte[]) value;
return Arrays.copyOf(bytes, bytes.length);
} else if (value == null
|| value instanceof Byte
|| value instanceof Character
|| value instanceof Short
|| value instanceof String
|| value instanceof Integer
|| value instanceof Long
Expand Down
47 changes: 32 additions & 15 deletions server/src/test/java/org/opensearch/ingest/IngestDocumentTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ public class IngestDocumentTests extends OpenSearchTestCase {

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

@Before
public void setTestIngestDocument() {
Expand All @@ -70,7 +71,6 @@ public void setTestIngestDocument() {
ingestMap.put("timestamp", BOGUS_TIMESTAMP);
document.put("_ingest", ingestMap);
document.put("foo", "bar");
document.put("int", 123);
Map<String, Object> innerObject = new HashMap<>();
innerObject.put("buzz", "hello world");
innerObject.put("foo_null", null);
Expand All @@ -92,7 +92,17 @@ public void setTestIngestDocument() {
list2.add("bar");
list2.add("baz");
document.put("list2", list2);
document.put("byte", (byte) 1);
document.put("short", (short) 2);
document.put("int", Integer.MAX_VALUE);
document.put("long", Long.MAX_VALUE);
document.put("float", 0.1f);
document.put("double", 0.1d);
document.put("char", 'a');
document.put("string", "A test string \uD83C\uDF89");
document.put("datetime", ZonedDateTime.parse("2007-12-03T10:15:30+01:00[Europe/Paris]"));
ingestDocument = new IngestDocument("index", "id", null, null, null, document);
initialSourceAndMetadataSize = 16; // i.e. ingestDocument.getSourceAndMetadata().size()
}

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

public void testCopy() {
final IngestDocument copy = new IngestDocument(ingestDocument);
assertThat(copy, equalTo(ingestDocument));
assertThat(copy.getSourceAndMetadata(), not(sameInstance(ingestDocument.getSourceAndMetadata())));
assertThat(copy.getIngestMetadata(), not(sameInstance(ingestDocument.getIngestMetadata())));
}

public void testSimpleGetFieldValue() {
assertThat(ingestDocument.getFieldValue("foo", String.class), equalTo("bar"));
assertThat(ingestDocument.getFieldValue("int", Integer.class), equalTo(123));
assertThat(ingestDocument.getFieldValue("int", Integer.class), equalTo(Integer.MAX_VALUE));
assertThat(ingestDocument.getFieldValue("_source.foo", String.class), equalTo("bar"));
assertThat(ingestDocument.getFieldValue("_source.int", Integer.class), equalTo(123));
assertThat(ingestDocument.getFieldValue("_source.int", Integer.class), equalTo(Integer.MAX_VALUE));
assertThat(ingestDocument.getFieldValue("_index", String.class), equalTo("index"));
assertThat(ingestDocument.getFieldValue("_id", String.class), equalTo("id"));
assertThat(
Expand Down Expand Up @@ -578,7 +595,7 @@ public void testAppendFieldValueConvertIntegerToList() {
@SuppressWarnings("unchecked")
List<Object> list = (List<Object>) object;
assertThat(list.size(), equalTo(2));
assertThat(list.get(0), equalTo(123));
assertThat(list.get(0), equalTo(Integer.MAX_VALUE));
assertThat(list.get(1), equalTo(456));
}

Expand All @@ -589,7 +606,7 @@ public void testAppendFieldValuesConvertIntegerToList() {
@SuppressWarnings("unchecked")
List<Object> list = (List<Object>) object;
assertThat(list.size(), equalTo(3));
assertThat(list.get(0), equalTo(123));
assertThat(list.get(0), equalTo(Integer.MAX_VALUE));
assertThat(list.get(1), equalTo(456));
assertThat(list.get(2), equalTo(789));
}
Expand Down Expand Up @@ -812,23 +829,23 @@ public void testSetFieldValueEmptyName() {

public void testRemoveField() {
ingestDocument.removeField("foo");
assertThat(ingestDocument.getSourceAndMetadata().size(), equalTo(7));
assertThat(ingestDocument.getSourceAndMetadata().size(), equalTo(initialSourceAndMetadataSize - 1));
assertThat(ingestDocument.getSourceAndMetadata().containsKey("foo"), equalTo(false));
ingestDocument.removeField("_index");
assertThat(ingestDocument.getSourceAndMetadata().size(), equalTo(6));
assertThat(ingestDocument.getSourceAndMetadata().size(), equalTo(initialSourceAndMetadataSize - 2));
assertThat(ingestDocument.getSourceAndMetadata().containsKey("_index"), equalTo(false));
ingestDocument.removeField("_source.fizz");
assertThat(ingestDocument.getSourceAndMetadata().size(), equalTo(5));
assertThat(ingestDocument.getSourceAndMetadata().size(), equalTo(initialSourceAndMetadataSize - 3));
assertThat(ingestDocument.getSourceAndMetadata().containsKey("fizz"), equalTo(false));
assertThat(ingestDocument.getIngestMetadata().size(), equalTo(1));
ingestDocument.removeField("_ingest.timestamp");
assertThat(ingestDocument.getSourceAndMetadata().size(), equalTo(5));
assertThat(ingestDocument.getSourceAndMetadata().size(), equalTo(initialSourceAndMetadataSize - 3));
assertThat(ingestDocument.getIngestMetadata().size(), equalTo(0));
}

public void testRemoveInnerField() {
ingestDocument.removeField("fizz.buzz");
assertThat(ingestDocument.getSourceAndMetadata().size(), equalTo(8));
assertThat(ingestDocument.getSourceAndMetadata().size(), equalTo(initialSourceAndMetadataSize));
assertThat(ingestDocument.getSourceAndMetadata().get("fizz"), instanceOf(Map.class));
@SuppressWarnings("unchecked")
Map<String, Object> map = (Map<String, Object>) ingestDocument.getSourceAndMetadata().get("fizz");
Expand All @@ -837,17 +854,17 @@ public void testRemoveInnerField() {

ingestDocument.removeField("fizz.foo_null");
assertThat(map.size(), equalTo(2));
assertThat(ingestDocument.getSourceAndMetadata().size(), equalTo(8));
assertThat(ingestDocument.getSourceAndMetadata().size(), equalTo(initialSourceAndMetadataSize));
assertThat(ingestDocument.getSourceAndMetadata().containsKey("fizz"), equalTo(true));

ingestDocument.removeField("fizz.1");
assertThat(map.size(), equalTo(1));
assertThat(ingestDocument.getSourceAndMetadata().size(), equalTo(8));
assertThat(ingestDocument.getSourceAndMetadata().size(), equalTo(initialSourceAndMetadataSize));
assertThat(ingestDocument.getSourceAndMetadata().containsKey("fizz"), equalTo(true));

ingestDocument.removeField("fizz.list");
assertThat(map.size(), equalTo(0));
assertThat(ingestDocument.getSourceAndMetadata().size(), equalTo(8));
assertThat(ingestDocument.getSourceAndMetadata().size(), equalTo(initialSourceAndMetadataSize));
assertThat(ingestDocument.getSourceAndMetadata().containsKey("fizz"), equalTo(true));
}

Expand Down Expand Up @@ -883,7 +900,7 @@ public void testRemoveSourceObject() {

public void testRemoveIngestObject() {
ingestDocument.removeField("_ingest");
assertThat(ingestDocument.getSourceAndMetadata().size(), equalTo(7));
assertThat(ingestDocument.getSourceAndMetadata().size(), equalTo(initialSourceAndMetadataSize - 1));
assertThat(ingestDocument.getSourceAndMetadata().containsKey("_ingest"), equalTo(false));
}

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

public void testListRemoveField() {
ingestDocument.removeField("list.0.field");
assertThat(ingestDocument.getSourceAndMetadata().size(), equalTo(8));
assertThat(ingestDocument.getSourceAndMetadata().size(), equalTo(initialSourceAndMetadataSize));
assertThat(ingestDocument.getSourceAndMetadata().containsKey("list"), equalTo(true));
Object object = ingestDocument.getSourceAndMetadata().get("list");
assertThat(object, instanceOf(List.class));
Expand Down

0 comments on commit 54667c0

Please sign in to comment.