From 0ca1e31392a621eaa88f76ede34a2ca747ac47d6 Mon Sep 17 00:00:00 2001 From: Britta Weber Date: Fri, 6 Mar 2015 19:23:41 +0100 Subject: [PATCH] Revert "[mappings] update dynamic fields in mapping on master even if parsing fails for the rest of doc" This reverts commit d9a15409488b0112eb11b5b39dab21b5c0096349. --- .../index/mapper/DocumentMapper.java | 10 ++-- .../index/mapper/MapperParsingException.java | 19 +------ .../mapper/StrictDynamicMappingException.java | 4 +- .../mapper/core/AbstractFieldMapper.java | 4 +- .../index/mapper/internal/IdFieldMapper.java | 4 +- .../index/mapper/object/ObjectMapper.java | 20 +++---- .../index/mapper/object/RootObjectMapper.java | 2 +- .../elasticsearch/index/shard/IndexShard.java | 4 +- .../mapper/dynamic/DynamicMappingTests.java | 53 ++----------------- 9 files changed, 29 insertions(+), 91 deletions(-) diff --git a/src/main/java/org/elasticsearch/index/mapper/DocumentMapper.java b/src/main/java/org/elasticsearch/index/mapper/DocumentMapper.java index f8a80b60690..b3b9de51985 100644 --- a/src/main/java/org/elasticsearch/index/mapper/DocumentMapper.java +++ b/src/main/java/org/elasticsearch/index/mapper/DocumentMapper.java @@ -437,7 +437,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 + "]", context.mappingsModified()); + throw new MapperParsingException("Type mismatch, provide type [" + source.type() + "] but mapper is of type [" + this.type + "]"); } source.type(this.type); @@ -455,7 +455,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", context.mappingsModified()); + throw new MapperParsingException("Malformed content, must start with an object"); } boolean emptyDoc = false; token = parser.nextToken(); @@ -463,7 +463,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", context.mappingsModified()); + throw new MapperParsingException("Malformed content, after first object, either the type field or the actual properties should exist"); } for (RootMapper rootMapper : rootMappersOrdered) { @@ -489,10 +489,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", context.mappingsModified()); + throw new MapperParsingException("failed to parse, document is empty"); } - throw new MapperParsingException("failed to parse", e, context.mappingsModified()); + throw new MapperParsingException("failed to parse", e); } finally { // only close the parser when its not provided externally if (source.parser() == null && parser != null) { diff --git a/src/main/java/org/elasticsearch/index/mapper/MapperParsingException.java b/src/main/java/org/elasticsearch/index/mapper/MapperParsingException.java index 9cac8a26ef9..9c180b82ae5 100644 --- a/src/main/java/org/elasticsearch/index/mapper/MapperParsingException.java +++ b/src/main/java/org/elasticsearch/index/mapper/MapperParsingException.java @@ -28,30 +28,13 @@ 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; diff --git a/src/main/java/org/elasticsearch/index/mapper/StrictDynamicMappingException.java b/src/main/java/org/elasticsearch/index/mapper/StrictDynamicMappingException.java index 098d5abf086..f675396369b 100644 --- a/src/main/java/org/elasticsearch/index/mapper/StrictDynamicMappingException.java +++ b/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, boolean mappingsModified) { - super("mapping set to strict, dynamic introduction of [" + fieldName + "] within [" + path + "] is not allowed", mappingsModified); + public StrictDynamicMappingException(String path, String fieldName) { + super("mapping set to strict, dynamic introduction of [" + fieldName + "] within [" + path + "] is not allowed"); } @Override diff --git a/src/main/java/org/elasticsearch/index/mapper/core/AbstractFieldMapper.java b/src/main/java/org/elasticsearch/index/mapper/core/AbstractFieldMapper.java index 806074914d0..ee0c66e9ccb 100644 --- a/src/main/java/org/elasticsearch/index/mapper/core/AbstractFieldMapper.java +++ b/src/main/java/org/elasticsearch/index/mapper/core/AbstractFieldMapper.java @@ -429,7 +429,7 @@ public abstract class AbstractFieldMapper implements FieldMapper { } } } catch (Exception e) { - throw new MapperParsingException("failed to parse [" + names.fullName() + "]", e, context.mappingsModified()); + throw new MapperParsingException("failed to parse [" + names.fullName() + "]", e); } multiFields.parse(this, context); if (copyTo != null) { @@ -1077,7 +1077,7 @@ public abstract class AbstractFieldMapper implements FieldMapper { 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 + "]", context.mappingsModified()); + throw new MapperParsingException("attempt to copy value to non-existing object [" + field + "]"); } context.path().add(objectPath); diff --git a/src/main/java/org/elasticsearch/index/mapper/internal/IdFieldMapper.java b/src/main/java/org/elasticsearch/index/mapper/internal/IdFieldMapper.java index 3d2d4c1c9ba..32e25a23caf 100644 --- a/src/main/java/org/elasticsearch/index/mapper/internal/IdFieldMapper.java +++ b/src/main/java/org/elasticsearch/index/mapper/internal/IdFieldMapper.java @@ -307,7 +307,7 @@ public class IdFieldMapper extends AbstractFieldMapper 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", context.mappingsModified()); + throw new MapperParsingException("No id found while parsing the content source"); } // it either get built in the preParse phase, or get parsed... } @@ -329,7 +329,7 @@ public class IdFieldMapper extends AbstractFieldMapper 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 + "]", context.mappingsModified()); + throw new MapperParsingException("Provided id [" + context.id() + "] does not match the content one [" + id + "]"); } context.id(id); } // else we are in the pre/post parse phase diff --git a/src/main/java/org/elasticsearch/index/mapper/object/ObjectMapper.java b/src/main/java/org/elasticsearch/index/mapper/object/ObjectMapper.java index 653f9f0804a..7f349e40479 100644 --- a/src/main/java/org/elasticsearch/index/mapper/object/ObjectMapper.java +++ b/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", context.mappingsModified()); + throw new MapperParsingException("object mapping for [" + name + "] tried to parse field [" + currentFieldName + "] as object, but found a concrete value"); } 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?", context.mappingsModified()); + 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?"); } 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 + "]", context.mappingsModified()); + throw new MapperParsingException("no object mapping found for null value in [" + lastFieldName + "]"); } } mapper.parse(context); } else if (dynamic == Dynamic.STRICT) { - throw new StrictDynamicMappingException(fullPath, lastFieldName, context.mappingsModified()); + throw new StrictDynamicMappingException(fullPath, lastFieldName); } } 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() + "]", context.mappingsModified()); + throw new MapperParsingException("object mapping [" + name + "] trying to serialize an object with no field associated with it, current value [" + context.parser().textOrNull() + "]"); } 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, context.mappingsModified()); + throw new StrictDynamicMappingException(fullPath, currentFieldName); } 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, context.mappingsModified()); + throw new StrictDynamicMappingException(fullPath, arrayFieldName); } 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?", context.mappingsModified()); + 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?"); } 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() + "]", context.mappingsModified()); + throw new MapperParsingException("object mapping [" + name + "] trying to serialize a value with no field associated with it, current value [" + context.parser().textOrNull() + "]"); } 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, context.mappingsModified()); + throw new StrictDynamicMappingException(fullPath, currentFieldName); } if (dynamic == Dynamic.FALSE) { return; diff --git a/src/main/java/org/elasticsearch/index/mapper/object/RootObjectMapper.java b/src/main/java/org/elasticsearch/index/mapper/object/RootObjectMapper.java index fb188ff0f8f..08938c72c31 100644 --- a/src/main/java/org/elasticsearch/index/mapper/object/RootObjectMapper.java +++ b/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 + "]", context.mappingsModified()); + throw new MapperParsingException("failed to find type parsed [" + mappingType + "] for [" + name + "]"); } return typeParser.parse(name, dynamicTemplate.mappingForName(name, dynamicType), parserContext); } diff --git a/src/main/java/org/elasticsearch/index/shard/IndexShard.java b/src/main/java/org/elasticsearch/index/shard/IndexShard.java index 0cbe1ea5e86..d8e9a332013 100644 --- a/src/main/java/org/elasticsearch/index/shard/IndexShard.java +++ b/src/main/java/org/elasticsearch/index/shard/IndexShard.java @@ -434,7 +434,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() || (t instanceof MapperParsingException && ((MapperParsingException)t).isMappingsModified())) { + if (docMapper.v2()) { throw new WriteFailureException(t, docMapper.v1().type()); } else { throw t; @@ -467,7 +467,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() || (t instanceof MapperParsingException && ((MapperParsingException)t).isMappingsModified())) { + if (docMapper.v2()) { throw new WriteFailureException(t, docMapper.v1().type()); } else { throw t; diff --git a/src/test/java/org/elasticsearch/index/mapper/dynamic/DynamicMappingTests.java b/src/test/java/org/elasticsearch/index/mapper/dynamic/DynamicMappingTests.java index 47c739d4a12..a1fa2c62e17 100644 --- a/src/test/java/org/elasticsearch/index/mapper/dynamic/DynamicMappingTests.java +++ b/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.*; +import org.elasticsearch.index.mapper.DocumentMapper; +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,49 +242,4 @@ 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 mappings = getMappingsResponse.getMappings().get("test").get("doc").getSourceAsMap(); - assertNotNull(((LinkedHashMap) mappings.get("properties")).get("a")); - assertNotNull(((LinkedHashMap) mappings.get("properties")).get("z")); - } } \ No newline at end of file