Do not throw exception when flat_object field is explicitly null (#15375

)

It is valid for a flat_object field to have an explicit value of
null. (It's functionally the same as not specifying the field at
all.) Prior to this fix, though, we would erroneously advance the
context parser to the next token, violating the contract with
DocumentParser (which says that a call to parseCreateField with
a null value should complete with the parser still pointing at
the null value -- it is DocumentParser's responsibility to advance).

Signed-off-by: Michael Froh <froh@amazon.com>

* Fix unit test

Signed-off-by: Michael Froh <froh@amazon.com>

* Add changelog entry

Signed-off-by: Michael Froh <froh@amazon.com>

---------

Signed-off-by: Michael Froh <froh@amazon.com>

CHANGELOG.md

@@ -68,6 +68,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 - Fixed array field name omission in flat_object function for nested JSON ([#13620](https://github.com/opensearch-project/OpenSearch/pull/13620))
 - Fix range aggregation optimization ignoring top level queries ([#15194](https://github.com/opensearch-project/OpenSearch/pull/15194))
 - Fix incorrect parameter names in MinHash token filter configuration handling ([#15233](https://github.com/opensearch-project/OpenSearch/pull/15233))
+- Fix indexing error when flat_object field is explicitly null ([#15375](https://github.com/opensearch-project/OpenSearch/pull/15375))
 - Fix split response processor not included in allowlist ([#15393](https://github.com/opensearch-project/OpenSearch/pull/15393))
 - Fix unchecked cast in dynamic action map getter ([#15394](https://github.com/opensearch-project/OpenSearch/pull/15394))
 

rest-api-spec/src/main/resources/rest-api-spec/test/index/100_partial_flat_object.yml

@@ -88,7 +88,16 @@ setup:
             } ]
           }
         }
-
+  - do:
+      index:
+        index: test_partial_flat_object
+        id: 4
+        body: {
+          "issue": {
+            "number": 999,
+            "labels": null
+          }
+        }
   - do:
       indices.refresh:
         index: test_partial_flat_object
@@ -135,7 +144,7 @@ teardown:
           }
         }
 
-  - length: { hits.hits: 3 }
+  - length: { hits.hits: 4 }
 
   # Match Query with exact dot path.
   - do:

server/src/main/java/org/opensearch/index/mapper/FlatObjectFieldMapper.java

@@ -568,11 +568,7 @@ protected void parseCreateField(ParseContext context) throws IOException {
         if (context.externalValueSet()) {
             String value = context.externalValue().toString();
             parseValueAddFields(context, value, fieldType().name());
-        } else if (context.parser().currentToken() == XContentParser.Token.VALUE_NULL) {
-            context.parser().nextToken(); // This triggers an exception in DocumentParser.
-            // We could remove the above nextToken() call to skip the null value, but the existing
-            // behavior (since 2.7) throws the exception.
-        } else {
+        } else if (context.parser().currentToken() != XContentParser.Token.VALUE_NULL) {
             JsonToStringXContentParser jsonToStringParser = new JsonToStringXContentParser(
                 NamedXContentRegistry.EMPTY,
                 DeprecationHandler.IGNORE_DEPRECATIONS,

server/src/test/java/org/opensearch/index/mapper/FlatObjectFieldMapperTests.java

@@ -25,7 +25,6 @@
 
 import static org.hamcrest.Matchers.instanceOf;
 import static org.hamcrest.core.IsEqual.equalTo;
-import static org.hamcrest.core.StringContains.containsString;
 
 public class FlatObjectFieldMapperTests extends MapperTestCase {
     private static final String FIELD_TYPE = "flat_object";
@@ -133,9 +132,10 @@ public void testDefaults() throws Exception {
 
     public void testNullValue() throws IOException {
         DocumentMapper mapper = createDocumentMapper(fieldMapping(this::minimalMapping));
-        MapperParsingException e = expectThrows(MapperParsingException.class, () -> mapper.parse(source(b -> b.nullField("field"))));
-        assertThat(e.getMessage(), containsString("object mapping for [_doc] tried to parse field [field] as object"));
-
+        ParsedDocument parsedDocument = mapper.parse(source(b -> b.nullField("field")));
+        assertEquals(1, parsedDocument.docs().size());
+        IndexableField[] fields = parsedDocument.rootDoc().getFields("field");
+        assertEquals(0, fields.length);
     }
 
     @Override