[mappings] update dynamic fields in mapping on master even if parsing fails for the rest of doc

The local DocumentMapper is updated while parsing and dynamic fields are added before
parsing has finished. If parsing fails after a dynamic field has been added already
then the field was not added to the cluster state but was present in the local mapper of this
node. New documents with the same field would not necessarily cause an update either and
after restarting the node the mapping for these fields were lost. Instead the new fields
should always be updated.

closes #9851
closes #9874

src/main/java/org/elasticsearch/index/mapper/DocumentMapper.java

@@ -438,7 +438,7 @@ public class DocumentMapper implements ToXContent {
         ParseContext.InternalParseContext context = cache.get();
 
         if (source.type() != null && !source.type().equals(this.type)) {
-            throw new MapperParsingException("Type mismatch, provide type [" + source.type() + "] but mapper is of type [" + this.type + "]");
+            throw new MapperParsingException("Type mismatch, provide type [" + source.type() + "] but mapper is of type [" + this.type + "]", context.mappingsModified());
         }
         source.type(this.type);
 
@@ -456,7 +456,7 @@ public class DocumentMapper implements ToXContent {
             int countDownTokens = 0;
             XContentParser.Token token = parser.nextToken();
             if (token != XContentParser.Token.START_OBJECT) {
-                throw new MapperParsingException("Malformed content, must start with an object");
+                throw new MapperParsingException("Malformed content, must start with an object", context.mappingsModified());
             }
             boolean emptyDoc = false;
             token = parser.nextToken();
@@ -464,7 +464,7 @@ public class DocumentMapper implements ToXContent {
                 // empty doc, we can handle it...
                 emptyDoc = true;
             } else if (token != XContentParser.Token.FIELD_NAME) {
-                throw new MapperParsingException("Malformed content, after first object, either the type field or the actual properties should exist");
+                throw new MapperParsingException("Malformed content, after first object, either the type field or the actual properties should exist", context.mappingsModified());
             }
 
             for (RootMapper rootMapper : rootMappersOrdered) {
@@ -490,10 +490,10 @@ public class DocumentMapper implements ToXContent {
 
             // Throw a more meaningful message if the document is empty.
             if (source.source() != null && source.source().length() == 0) {
-                throw new MapperParsingException("failed to parse, document is empty");
+                throw new MapperParsingException("failed to parse, document is empty", context.mappingsModified());
             }
 
-            throw new MapperParsingException("failed to parse", e);
+            throw new MapperParsingException("failed to parse", e, context.mappingsModified());
         } finally {
             // only close the parser when its not provided externally
             if (source.parser() == null && parser != null) {

src/main/java/org/elasticsearch/index/mapper/MapperParsingException.java

@@ -28,13 +28,30 @@ public class MapperParsingException extends MapperException {
 
     public MapperParsingException(String message) {
         super(message);
+        mappingsModified = false;
+    }
+
+    public boolean isMappingsModified() {
+        return mappingsModified;
+    }
+
+    private boolean mappingsModified = false;
+
+    public MapperParsingException(String message, boolean mappingsModified) {
+        super(message);
+        this.mappingsModified = mappingsModified;
+    }
+
+    public MapperParsingException(String message, Throwable cause, boolean mappingsModified) {
+        super(message, cause);
+        this.mappingsModified = mappingsModified;
     }
 
     public MapperParsingException(String message, Throwable cause) {
         super(message, cause);
+        this.mappingsModified = false;
     }
 
-
     @Override
     public RestStatus status() {
         return RestStatus.BAD_REQUEST;

src/main/java/org/elasticsearch/index/mapper/StrictDynamicMappingException.java

@@ -24,8 +24,8 @@ import org.elasticsearch.rest.RestStatus;
  */
 public class StrictDynamicMappingException extends MapperParsingException {
 
-    public StrictDynamicMappingException(String path, String fieldName) {
+    public StrictDynamicMappingException(String path, String fieldName, boolean mappingsModified) {
-        super("mapping set to strict, dynamic introduction of [" + fieldName + "] within [" + path + "] is not allowed");
+        super("mapping set to strict, dynamic introduction of [" + fieldName + "] within [" + path + "] is not allowed", mappingsModified);
     }
 
     @Override

src/main/java/org/elasticsearch/index/mapper/core/AbstractFieldMapper.java

@@ -429,7 +429,7 @@ public abstract class AbstractFieldMapper<T> implements FieldMapper<T> {
                 }
             }
         } catch (Exception e) {
-            throw new MapperParsingException("failed to parse [" + names.fullName() + "]", e);
+            throw new MapperParsingException("failed to parse [" + names.fullName() + "]", e, context.mappingsModified());
         }
         multiFields.parse(this, context);
         if (copyTo != null) {
@@ -1077,7 +1077,7 @@ public abstract class AbstractFieldMapper<T> implements FieldMapper<T> {
                     ObjectMapper mapper = context.docMapper().objectMappers().get(objectPath);
                     if (mapper == null) {
                         //TODO: Create an object dynamically?
-                        throw new MapperParsingException("attempt to copy value to non-existing object [" + field + "]");
+                        throw new MapperParsingException("attempt to copy value to non-existing object [" + field + "]", context.mappingsModified());
                     }
 
                     context.path().add(objectPath);

src/main/java/org/elasticsearch/index/mapper/internal/IdFieldMapper.java

@@ -307,7 +307,7 @@ public class IdFieldMapper extends AbstractFieldMapper<String> implements Intern
     @Override
     public void postParse(ParseContext context) throws IOException {
         if (context.id() == null && !context.sourceToParse().flyweight()) {
-            throw new MapperParsingException("No id found while parsing the content source");
+            throw new MapperParsingException("No id found while parsing the content source", context.mappingsModified());
         }
         // it either get built in the preParse phase, or get parsed...
     }
@@ -329,7 +329,7 @@ public class IdFieldMapper extends AbstractFieldMapper<String> implements Intern
             // we are in the parse Phase
             String id = parser.text();
             if (context.id() != null && !context.id().equals(id)) {
-                throw new MapperParsingException("Provided id [" + context.id() + "] does not match the content one [" + id + "]");
+                throw new MapperParsingException("Provided id [" + context.id() + "] does not match the content one [" + id + "]", context.mappingsModified());
             }
             context.id(id);
         } // else we are in the pre/post parse phase

src/main/java/org/elasticsearch/index/mapper/object/ObjectMapper.java

@@ -499,7 +499,7 @@ public class ObjectMapper implements Mapper, AllFieldMapper.IncludeInAll {
         if (token.isValue() && !allowValue()) {
             // if we are parsing an object but it is just a value, its only allowed on root level parsers with there
             // is a field name with the same name as the type
-            throw new MapperParsingException("object mapping for [" + name + "] tried to parse field [" + currentFieldName + "] as object, but found a concrete value");
+            throw new MapperParsingException("object mapping for [" + name + "] tried to parse field [" + currentFieldName + "] as object, but found a concrete value", context.mappingsModified());
         }
 
         if (nested.isNested()) {
@@ -543,7 +543,7 @@ public class ObjectMapper implements Mapper, AllFieldMapper.IncludeInAll {
             } else if (token == XContentParser.Token.VALUE_NULL) {
                 serializeNullValue(context, currentFieldName);
             } else if (token == null) {
-                throw new MapperParsingException("object mapping for [" + name + "] tried to parse field [" + currentFieldName + "] as object, but got EOF, has a concrete value been provided to it?");
+                throw new MapperParsingException("object mapping for [" + name + "] tried to parse field [" + currentFieldName + "] as object, but got EOF, has a concrete value been provided to it?", context.mappingsModified());
             } else if (token.isValue()) {
                 serializeValue(context, currentFieldName, token);
             }
@@ -585,18 +585,18 @@ public class ObjectMapper implements Mapper, AllFieldMapper.IncludeInAll {
         if (mapper != null) {
             if (mapper instanceof FieldMapper) {
                 if (!((FieldMapper) mapper).supportsNullValue()) {
-                    throw new MapperParsingException("no object mapping found for null value in [" + lastFieldName + "]");
+                    throw new MapperParsingException("no object mapping found for null value in [" + lastFieldName + "]", context.mappingsModified());
                 }
             }
             mapper.parse(context);
         } else if (dynamic == Dynamic.STRICT) {
-            throw new StrictDynamicMappingException(fullPath, lastFieldName);
+            throw new StrictDynamicMappingException(fullPath, lastFieldName, context.mappingsModified());
         }
     }
 
     private void serializeObject(final ParseContext context, String currentFieldName) throws IOException {
         if (currentFieldName == null) {
-            throw new MapperParsingException("object mapping [" + name + "] trying to serialize an object with no field associated with it, current value [" + context.parser().textOrNull() + "]");
+            throw new MapperParsingException("object mapping [" + name + "] trying to serialize an object with no field associated with it, current value [" + context.parser().textOrNull() + "]", context.mappingsModified());
         }
         context.path().add(currentFieldName);
 
@@ -609,7 +609,7 @@ public class ObjectMapper implements Mapper, AllFieldMapper.IncludeInAll {
                 dynamic = context.root().dynamic();
             }
             if (dynamic == Dynamic.STRICT) {
-                throw new StrictDynamicMappingException(fullPath, currentFieldName);
+                throw new StrictDynamicMappingException(fullPath, currentFieldName, context.mappingsModified());
             } else if (dynamic == Dynamic.TRUE) {
                 // we sync here just so we won't add it twice. Its not the end of the world
                 // to sync here since next operations will get it before
@@ -661,7 +661,7 @@ public class ObjectMapper implements Mapper, AllFieldMapper.IncludeInAll {
                 dynamic = context.root().dynamic();
             }
             if (dynamic == Dynamic.STRICT) {
-                throw new StrictDynamicMappingException(fullPath, arrayFieldName);
+                throw new StrictDynamicMappingException(fullPath, arrayFieldName, context.mappingsModified());
             } else if (dynamic == Dynamic.TRUE) {
                 // we sync here just so we won't add it twice. Its not the end of the world
                 // to sync here since next operations will get it before
@@ -741,7 +741,7 @@ public class ObjectMapper implements Mapper, AllFieldMapper.IncludeInAll {
             } else if (token == XContentParser.Token.VALUE_NULL) {
                 serializeNullValue(context, lastFieldName);
             } else if (token == null) {
-                throw new MapperParsingException("object mapping for [" + name + "] with array for [" + arrayFieldName + "] tried to parse as array, but got EOF, is there a mismatch in types for the same field?");
+                throw new MapperParsingException("object mapping for [" + name + "] with array for [" + arrayFieldName + "] tried to parse as array, but got EOF, is there a mismatch in types for the same field?", context.mappingsModified());
             } else {
                 serializeValue(context, lastFieldName, token);
             }
@@ -750,7 +750,7 @@ public class ObjectMapper implements Mapper, AllFieldMapper.IncludeInAll {
 
     private void serializeValue(final ParseContext context, String currentFieldName, XContentParser.Token token) throws IOException {
         if (currentFieldName == null) {
-            throw new MapperParsingException("object mapping [" + name + "] trying to serialize a value with no field associated with it, current value [" + context.parser().textOrNull() + "]");
+            throw new MapperParsingException("object mapping [" + name + "] trying to serialize a value with no field associated with it, current value [" + context.parser().textOrNull() + "]", context.mappingsModified());
         }
         Mapper mapper = mappers.get(currentFieldName);
         if (mapper != null) {
@@ -766,7 +766,7 @@ public class ObjectMapper implements Mapper, AllFieldMapper.IncludeInAll {
             dynamic = context.root().dynamic();
         }
         if (dynamic == Dynamic.STRICT) {
-            throw new StrictDynamicMappingException(fullPath, currentFieldName);
+            throw new StrictDynamicMappingException(fullPath, currentFieldName, context.mappingsModified());
         }
         if (dynamic == Dynamic.FALSE) {
             return;

src/main/java/org/elasticsearch/index/mapper/object/RootObjectMapper.java

@@ -231,7 +231,7 @@ public class RootObjectMapper extends ObjectMapper {
         String mappingType = dynamicTemplate.mappingType(dynamicType);
         Mapper.TypeParser typeParser = parserContext.typeParser(mappingType);
         if (typeParser == null) {
-            throw new MapperParsingException("failed to find type parsed [" + mappingType + "] for [" + name + "]");
+            throw new MapperParsingException("failed to find type parsed [" + mappingType + "] for [" + name + "]", context.mappingsModified());
         }
         return typeParser.parse(name, dynamicTemplate.mappingForName(name, dynamicType), parserContext);
     }

src/main/java/org/elasticsearch/index/shard/IndexShard.java

@@ -448,7 +448,7 @@ public class IndexShard extends AbstractIndexShardComponent {
             ParsedDocument doc = docMapper.v1().parse(source).setMappingsModified(docMapper);
             return new Engine.Create(docMapper.v1(), docMapper.v1().uidMapper().term(doc.uid().stringValue()), doc, version, versionType, origin, startTime, state != IndexShardState.STARTED || canHaveDuplicates, autoGeneratedId);
         } catch (Throwable t) {
-            if (docMapper.v2()) {
+            if (docMapper.v2() || (t instanceof  MapperParsingException && ((MapperParsingException)t).isMappingsModified())) {
                 throw new WriteFailureException(t, docMapper.v1().type());
             } else {
                 throw t;
@@ -481,7 +481,7 @@ public class IndexShard extends AbstractIndexShardComponent {
             ParsedDocument doc = docMapper.v1().parse(source).setMappingsModified(docMapper);
             return new Engine.Index(docMapper.v1(), docMapper.v1().uidMapper().term(doc.uid().stringValue()), doc, version, versionType, origin, startTime, state != IndexShardState.STARTED || canHaveDuplicates);
         } catch (Throwable t) {
-            if (docMapper.v2()) {
+            if (docMapper.v2() || (t instanceof  MapperParsingException && ((MapperParsingException)t).isMappingsModified())) {
                 throw new WriteFailureException(t, docMapper.v1().type());
             } else {
                 throw t;

src/test/java/org/elasticsearch/index/mapper/dynamic/DynamicMappingTests.java

@@ -20,18 +20,18 @@ package org.elasticsearch.index.mapper.dynamic;
 
 import com.google.common.base.Predicate;
 import org.elasticsearch.action.admin.indices.mapping.get.GetMappingsResponse;
+import org.elasticsearch.action.index.IndexRequestBuilder;
 import org.elasticsearch.common.settings.ImmutableSettings;
 import org.elasticsearch.common.xcontent.XContentBuilder;
 import org.elasticsearch.common.xcontent.XContentFactory;
-import org.elasticsearch.index.mapper.DocumentMapper;
+import org.elasticsearch.index.mapper.*;
-import org.elasticsearch.index.mapper.FieldMappers;
-import org.elasticsearch.index.mapper.ParsedDocument;
-import org.elasticsearch.index.mapper.StrictDynamicMappingException;
 import org.elasticsearch.index.IndexService;
 import org.elasticsearch.test.ElasticsearchSingleNodeTest;
 import org.junit.Test;
 
 import java.io.IOException;
+import java.util.LinkedHashMap;
+import java.util.Map;
 
 import static org.elasticsearch.common.xcontent.XContentFactory.jsonBuilder;
 import static org.hamcrest.Matchers.equalTo;
@@ -242,4 +242,49 @@ public class DynamicMappingTests extends ElasticsearchSingleNodeTest {
         getMappingsResponse = client().admin().indices().prepareGetMappings("test").get();
         assertNotNull(getMappingsResponse.getMappings().get("test").get("type"));
     }
+
+    @Test
+    public void testFieldsCreatedWithPartialParsing() throws IOException, InterruptedException {
+        XContentBuilder mapping = jsonBuilder().startObject().startObject("doc")
+                .startObject("properties")
+                .startObject("z")
+                .field("type", "long")
+                .endObject()
+                .endObject()
+                .endObject().endObject();
+
+        IndexService indexService = createIndex("test", ImmutableSettings.EMPTY, "doc", mapping);
+        boolean create = randomBoolean();
+        if (create == false) {
+            // we want to test sometimes create and sometimes index so sometimes add the document before and sometimes not
+            client().prepareIndex().setIndex("test").setType("doc").setId("1").setSource(jsonBuilder().startObject().field("z", 0).endObject()).get();
+        }
+        try {
+            IndexRequestBuilder indexRequest = client().prepareIndex().setIndex("test").setType("doc").setId("1").setSource(jsonBuilder().startObject().field("a", "string").field("z", "string").endObject());
+            indexRequest.setCreate(create);
+            indexRequest.get();
+            fail();
+        } catch (MapperParsingException e) {
+            // this should fail because the field z is of type long
+        }
+        //type should be in mapping
+        GetMappingsResponse getMappingsResponse = client().admin().indices().prepareGetMappings("test").get();
+        assertNotNull(getMappingsResponse.getMappings().get("test").get("doc"));
+
+        client().prepareIndex().setIndex("test").setType("doc").setId("1").setSource(jsonBuilder().startObject().field("a", "string").field("z", 0).endObject()).get();
+        client().admin().indices().prepareRefresh("test").get();
+        assertThat(client().prepareSearch("test").get().getHits().getTotalHits(), equalTo(1l));
+
+        // both fields should be in local mapper
+        DocumentMapper mapper = indexService.mapperService().documentMapper("doc");
+        assertNotNull(mapper.mappers().name("a"));
+        assertNotNull(mapper.mappers().name("z"));
+
+        // both fields should be in the cluster state
+        getMappingsResponse = client().admin().indices().prepareGetMappings("test").get();
+        assertNotNull(getMappingsResponse.getMappings().get("test").get("doc"));
+        Map<String, Object> mappings = getMappingsResponse.getMappings().get("test").get("doc").getSourceAsMap();
+        assertNotNull(((LinkedHashMap) mappings.get("properties")).get("a"));
+        assertNotNull(((LinkedHashMap) mappings.get("properties")).get("z"));
+    }
 }