Remove RootMapper.validate and validate the routing key up-front.

RootMapper.validate was only used by the routing field mapper, which makes
buggy assumptions about how fields are indexed. For example, it assumes that
the index representation of a field is the same as its external representation.

Close #5844

src/main/java/org/elasticsearch/action/index/IndexRequest.java

@@ -36,6 +36,7 @@ import org.elasticsearch.common.io.stream.StreamOutput;
 import org.elasticsearch.common.lucene.uid.Versions;
 import org.elasticsearch.common.xcontent.*;
 import org.elasticsearch.index.VersionType;
+import org.elasticsearch.index.mapper.MapperParsingException;
 import org.elasticsearch.index.mapper.internal.TimestampFieldMapper;
 
 import java.io.IOException;
@@ -567,12 +568,17 @@ public class IndexRequest extends ShardReplicationOperationRequest<IndexRequest>
                         id = parseContext.id();
                     }
                     if (parseContext.shouldParseRouting()) {
+                        if (routing != null && !routing.equals(parseContext.routing())) {
+                            throw new MapperParsingException("The provided routing value [" + routing + "] doesn't match the routing key stored in the document: [" + parseContext.routing() + "]");
+                        }
                         routing = parseContext.routing();
                     }
                     if (parseContext.shouldParseTimestamp()) {
                         timestamp = parseContext.timestamp();
                         timestamp = MappingMetaData.Timestamp.parseStringTimestamp(timestamp, mappingMd.timestamp().dateTimeFormatter());
                     }
+                } catch (MapperParsingException e) {
+                    throw e;
                 } catch (Exception e) {
                     throw new ElasticsearchParseException("failed to parse doc to extract routing/timestamp/id", e);
                 } finally {

src/main/java/org/elasticsearch/cluster/metadata/MappingMetaData.java

@@ -418,9 +418,11 @@ public class MappingMetaData {
     }
 
     public ParseContext createParseContext(@Nullable String id, @Nullable String routing, @Nullable String timestamp) {
+        // We parse the routing even if there is already a routing key in the request in order to make sure that
+        // they are the same
         return new ParseContext(
                 id == null && id().hasPath(),
-                routing == null && routing().hasPath(),
+                routing().hasPath(),
                 timestamp == null && timestamp().hasPath()
         );
     }

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

@@ -522,10 +522,6 @@ public class DocumentMapper implements ToXContent {
             for (RootMapper rootMapper : rootMappersOrdered) {
                 rootMapper.postParse(context);
             }
-
-            for (RootMapper rootMapper : rootMappersOrdered) {
-                rootMapper.validate(context);
-            }
         } catch (Throwable e) {
             // if its already a mapper parsing exception, no need to wrap it...
             if (e instanceof MapperParsingException) {

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

@@ -31,8 +31,6 @@ public interface RootMapper extends Mapper {
 
     void postParse(ParseContext context) throws IOException;
 
-    void validate(ParseContext context) throws MapperParsingException;
-
     /**
      * Should the mapper be included in the root {@link org.elasticsearch.index.mapper.object.ObjectMapper}.
      */

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

@@ -199,10 +199,6 @@ public class AllFieldMapper extends AbstractFieldMapper<Void> implements Interna
         // we parse in post parse
     }
 
-    @Override
-    public void validate(ParseContext context) throws MapperParsingException {
-    }
-
     @Override
     public boolean includeInObject() {
         return true;

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

@@ -124,10 +124,6 @@ public class AnalyzerMapper implements Mapper, InternalMapper, RootMapper {
         context.analyzer(analyzer);
     }
 
-    @Override
-    public void validate(ParseContext context) throws MapperParsingException {
-    }
-
     @Override
     public boolean includeInObject() {
         return false;

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

@@ -236,10 +236,6 @@ public class BoostFieldMapper extends NumberFieldMapper<Float> implements Intern
     public void postParse(ParseContext context) throws IOException {
     }
 
-    @Override
-    public void validate(ParseContext context) throws MapperParsingException {
-    }
-
     @Override
     public boolean includeInObject() {
         return true;

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

@@ -292,10 +292,6 @@ public class IdFieldMapper extends AbstractFieldMapper<String> implements Intern
         super.parse(context);
     }
 
-    @Override
-    public void validate(ParseContext context) throws MapperParsingException {
-    }
-
     @Override
     public boolean includeInObject() {
         return true;

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

@@ -172,10 +172,6 @@ public class IndexFieldMapper extends AbstractFieldMapper<String> implements Int
 
     }
 
-    @Override
-    public void validate(ParseContext context) throws MapperParsingException {
-    }
-
     @Override
     public boolean includeInObject() {
         return false;

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

@@ -170,10 +170,6 @@ public class ParentFieldMapper extends AbstractFieldMapper<Uid> implements Inter
         parse(context);
     }
 
-    @Override
-    public void validate(ParseContext context) throws MapperParsingException {
-    }
-
     @Override
     public boolean includeInObject() {
         return true;

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

@@ -35,7 +35,6 @@ import org.elasticsearch.index.codec.postingsformat.PostingsFormatProvider;
 import org.elasticsearch.index.fielddata.FieldDataType;
 import org.elasticsearch.index.mapper.*;
 import org.elasticsearch.index.mapper.core.AbstractFieldMapper;
-import org.elasticsearch.index.mapper.core.NumberFieldMapper;
 
 import java.io.IOException;
 import java.util.List;
@@ -172,31 +171,6 @@ public class RoutingFieldMapper extends AbstractFieldMapper<String> implements I
         return value.toString();
     }
 
-    @Override
-    public void validate(ParseContext context) throws MapperParsingException {
-        String routing = context.sourceToParse().routing();
-        if (path != null && routing != null) {
-            // we have a path, check if we can validate we have the same routing value as the one in the doc...
-            String value = null;
-            Field field = (Field) context.doc().getField(path);
-            if (field != null) {
-                value = field.stringValue();
-                if (value == null) {
-                    // maybe its a numeric field...
-                    if (field instanceof NumberFieldMapper.CustomNumericField) {
-                        value = ((NumberFieldMapper.CustomNumericField) field).numericAsString();
-                    }
-                }
-            }
-            if (value == null) {
-                value = context.ignoredValue(path);
-            }
-            if (!routing.equals(value)) {
-                throw new MapperParsingException("External routing [" + routing + "] and document path routing [" + value + "] mismatch");
-            }
-        }
-    }
-
     @Override
     public void preParse(ParseContext context) throws IOException {
         super.parse(context);

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

@@ -121,10 +121,6 @@ public class SizeFieldMapper extends IntegerFieldMapper implements RootMapper {
         return this.enabledState.enabled;
     }
 
-    @Override
-    public void validate(ParseContext context) throws MapperParsingException {
-    }
-
     @Override
     public void preParse(ParseContext context) throws IOException {
     }

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

@@ -248,10 +248,6 @@ public class SourceFieldMapper extends AbstractFieldMapper<byte[]> implements In
         // nothing to do here, we will call it in pre parse
     }
 
-    @Override
-    public void validate(ParseContext context) throws MapperParsingException {
-    }
-
     @Override
     public boolean includeInObject() {
         return false;

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

@@ -164,10 +164,6 @@ public class TTLFieldMapper extends LongFieldMapper implements InternalMapper, R
         return expirationTime - System.currentTimeMillis();
     }
 
-    @Override
-    public void validate(ParseContext context) throws MapperParsingException {
-    }
-
     @Override
     public void preParse(ParseContext context) throws IOException {
     }

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

@@ -179,10 +179,6 @@ public class TimestampFieldMapper extends DateFieldMapper implements InternalMap
         return value(value);
     }
 
-    @Override
-    public void validate(ParseContext context) throws MapperParsingException {
-    }
-
     @Override
     public void preParse(ParseContext context) throws IOException {
         super.parse(context);

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

@@ -168,10 +168,6 @@ public class TypeFieldMapper extends AbstractFieldMapper<String> implements Inte
         // we parse in pre parse
     }
 
-    @Override
-    public void validate(ParseContext context) throws MapperParsingException {
-    }
-
     @Override
     public boolean includeInObject() {
         return false;

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

@@ -165,10 +165,6 @@ public class UidFieldMapper extends AbstractFieldMapper<Uid> implements Internal
         // nothing to do here, we either do it in post parse, or in pre parse.
     }
 
-    @Override
-    public void validate(ParseContext context) throws MapperParsingException {
-    }
-
     @Override
     public boolean includeInObject() {
         return false;

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

@@ -144,10 +144,6 @@ public class VersionFieldMapper extends AbstractFieldMapper<Long> implements Int
         }
     }
 
-    @Override
-    public void validate(ParseContext context) throws MapperParsingException {
-    }
-
     @Override
     public boolean includeInObject() {
         return false;

src/test/java/org/elasticsearch/cluster/metadata/MappingMetaDataParserTests.java

@@ -43,8 +43,8 @@ public class MappingMetaDataParserTests extends ElasticsearchTestCase {
         md.parse(XContentFactory.xContent(bytes).createParser(bytes), parseContext);
         assertThat(parseContext.id(), equalTo("id"));
         assertThat(parseContext.idResolved(), equalTo(true));
-        assertThat(parseContext.routing(), nullValue());
-        assertThat(parseContext.routingResolved(), equalTo(false));
+        assertThat(parseContext.routing(), equalTo("routing_value"));
+        assertThat(parseContext.routingResolved(), equalTo(true));
         assertThat(parseContext.timestamp(), nullValue());
         assertThat(parseContext.timestampResolved(), equalTo(false));
     }
@@ -106,8 +106,8 @@ public class MappingMetaDataParserTests extends ElasticsearchTestCase {
         md.parse(XContentFactory.xContent(bytes).createParser(bytes), parseContext);
         assertThat(parseContext.id(), nullValue());
         assertThat(parseContext.idResolved(), equalTo(false));
-        assertThat(parseContext.routing(), nullValue());
-        assertThat(parseContext.routingResolved(), equalTo(false));
+        assertThat(parseContext.routing(), equalTo("routing_value"));
+        assertThat(parseContext.routingResolved(), equalTo(true));
         assertThat(parseContext.timestamp(), equalTo("1"));
         assertThat(parseContext.timestampResolved(), equalTo(true));
     }
@@ -160,8 +160,8 @@ public class MappingMetaDataParserTests extends ElasticsearchTestCase {
         md.parse(XContentFactory.xContent(bytes).createParser(bytes), parseContext);
         assertThat(parseContext.id(), equalTo("id"));
         assertThat(parseContext.idResolved(), equalTo(true));
-        assertThat(parseContext.routing(), nullValue());
-        assertThat(parseContext.routingResolved(), equalTo(false));
+        assertThat(parseContext.routing(), equalTo("routing_value"));
+        assertThat(parseContext.routingResolved(), equalTo(true));
         assertThat(parseContext.timestamp(), nullValue());
         assertThat(parseContext.timestampResolved(), equalTo(false));
     }
@@ -202,8 +202,8 @@ public class MappingMetaDataParserTests extends ElasticsearchTestCase {
         md.parse(XContentFactory.xContent(bytes).createParser(bytes), parseContext);
         assertThat(parseContext.id(), nullValue());
         assertThat(parseContext.idResolved(), equalTo(false));
-        assertThat(parseContext.routing(), nullValue());
-        assertThat(parseContext.routingResolved(), equalTo(false));
+        assertThat(parseContext.routing(), equalTo("routing_value"));
+        assertThat(parseContext.routingResolved(), equalTo(true));
         assertThat(parseContext.timestamp(), equalTo("1"));
         assertThat(parseContext.timestampResolved(), equalTo(true));
     }

src/test/java/org/elasticsearch/routing/SimpleRoutingTests.java

@@ -235,6 +235,7 @@ public class SimpleRoutingTests extends ElasticsearchIntegrationTest {
         client().admin().indices().prepareCreate("test")
                 .addMapping("type1", XContentFactory.jsonBuilder().startObject().startObject("type1")
                         .startObject("_routing").field("required", true).field("path", "routing_field").endObject()
+                        .startObject("routing_field").field("type", "string").field("index", randomBoolean() ? "no" : "not_analyzed").field("doc_values", randomBoolean() ? "yes" : "no").endObject()
                         .endObject().endObject())
                 .execute().actionGet();
         ensureGreen();
@@ -303,12 +304,6 @@ public class SimpleRoutingTests extends ElasticsearchIntegrationTest {
         client().admin().indices().prepareCreate("test")
                 .addMapping("type1", XContentFactory.jsonBuilder().startObject().startObject("type1")
                         .startObject("_routing").field("required", true).field("path", "routing_field").endObject()
-                        .startObject("properties")
-                            .startObject("routing_field")
-                                .field("type", "long")
-                                .field("doc_values", false) // TODO this test fails with doc values https://github.com/elasticsearch/elasticsearch/pull/5858
-                            .endObject()
-                        .endObject()
                         .endObject().endObject())
                 .execute().actionGet();
         ensureGreen();