Made single shards APIs fail if routing is configured to be required …

…in the mapping. This change make single shard requests fail when no routing is specified and routing has been configured to be required in the mapping. Thi Closes elastic#4506
brusic · Jan 19, 2014 · 0bd6c7f · 0bd6c7f
1 parent d2c3f64
commit 0bd6c7f
Show file tree

Hide file tree

Showing 18 changed files with 248 additions and 20 deletions.
diff --git a/rest-api-spec/test/create/50_parent.yaml b/rest-api-spec/test/create/50_parent.yaml
@@ -41,7 +41,7 @@
  - match:   { fields._routing: "5"}
 
  - do:
-      catch: missing
+      catch: param
       get:
           index:   test_1
           type:    test

diff --git a/rest-api-spec/test/exists/30_parent.yaml b/rest-api-spec/test/exists/30_parent.yaml
@@ -33,6 +33,7 @@
  - is_true: ''
 
  - do:
+      catch:  param
       exists:
           index:   test_1
           type:    test

diff --git a/rest-api-spec/test/get/30_parent.yaml b/rest-api-spec/test/get/30_parent.yaml
@@ -33,7 +33,7 @@
  - match:   { fields._routing: 中文}
 
  - do:
-      catch: missing
+      catch: param
       get:
           index:   test_1
           type:    test

diff --git a/rest-api-spec/test/get_source/30_parent.yaml b/rest-api-spec/test/get_source/30_parent.yaml
@@ -32,7 +32,7 @@
  - match:   { '': {foo: bar}}
 
  - do:
-      catch: missing
+      catch: param
       get_source:
           index:   test_1
           type:    test

diff --git a/rest-api-spec/test/index/50_parent.yaml b/rest-api-spec/test/index/50_parent.yaml
@@ -41,7 +41,7 @@
  - match:   { fields._routing: "5"}
 
  - do:
-      catch: missing
+      catch: param
       get:
           index:   test_1
           type:    test

diff --git a/rest-api-spec/test/update/50_parent.yaml b/rest-api-spec/test/update/50_parent.yaml
@@ -44,7 +44,7 @@
  - match: { fields._routing: "5"}
 
  - do:
-      catch:      missing
+      catch:      param
       update:
           index:   test_1
           type:    test

diff --git a/src/main/java/org/elasticsearch/action/RoutingMissingException.java b/src/main/java/org/elasticsearch/action/RoutingMissingException.java
@@ -20,6 +20,7 @@
 package org.elasticsearch.action;
 
 import org.elasticsearch.ElasticSearchException;
+import org.elasticsearch.rest.RestStatus;
 
 /**
  *
@@ -50,4 +51,9 @@ public String type() {
     public String id() {
         return id;
     }
+
+    @Override
+    public RestStatus status() {
+        return RestStatus.BAD_REQUEST;
+    }
 }
diff --git a/src/main/java/org/elasticsearch/action/explain/TransportExplainAction.java b/src/main/java/org/elasticsearch/action/explain/TransportExplainAction.java
@@ -23,6 +23,7 @@
 import org.apache.lucene.search.Explanation;
 import org.elasticsearch.ElasticSearchException;
 import org.elasticsearch.action.ActionListener;
+import org.elasticsearch.action.RoutingMissingException;
 import org.elasticsearch.action.support.single.shard.TransportShardSingleOperationAction;
 import org.elasticsearch.cache.recycler.CacheRecycler;
 import org.elasticsearch.cluster.ClusterService;
@@ -91,6 +92,11 @@ protected void resolveRequest(ClusterState state, ExplainRequest request) {
         String concreteIndex = state.metaData().concreteIndex(request.index());
         request.filteringAlias(state.metaData().filteringAliases(concreteIndex, request.index()));
         request.index(state.metaData().concreteIndex(request.index()));
+
+        // Fail fast on the node that received the request.
+        if (request.routing() == null && state.getMetaData().routingRequired(request.index(), request.type())) {
+            throw new RoutingMissingException(request.index(), request.type(), request.id());
+        }
     }
 
     protected ExplainResponse shardOperation(ExplainRequest request, int shardId) throws ElasticSearchException {

diff --git a/src/main/java/org/elasticsearch/action/get/TransportGetAction.java b/src/main/java/org/elasticsearch/action/get/TransportGetAction.java
@@ -20,6 +20,7 @@
 package org.elasticsearch.action.get;
 
 import org.elasticsearch.ElasticSearchException;
+import org.elasticsearch.action.RoutingMissingException;
 import org.elasticsearch.action.support.single.shard.TransportShardSingleOperationAction;
 import org.elasticsearch.cluster.ClusterService;
 import org.elasticsearch.cluster.ClusterState;
@@ -89,6 +90,11 @@ protected void resolveRequest(ClusterState state, GetRequest request) {
         // update the routing (request#index here is possibly an alias)
         request.routing(state.metaData().resolveIndexRouting(request.routing(), request.index()));
         request.index(state.metaData().concreteIndex(request.index()));
+
+        // Fail fast on the node that received the request.
+        if (request.routing() == null && state.getMetaData().routingRequired(request.index(), request.type())) {
+            throw new RoutingMissingException(request.index(), request.type(), request.id());
+        }
     }
 
     @Override

diff --git a/src/main/java/org/elasticsearch/action/get/TransportMultiGetAction.java b/src/main/java/org/elasticsearch/action/get/TransportMultiGetAction.java
@@ -68,6 +68,10 @@ protected void doExecute(final MultiGetRequest request, final ActionListener<Mul
                 responses.set(i, new MultiGetItemResponse(null, new MultiGetResponse.Failure(item.index(), item.type(), item.id(), "[" + item.index() + "] missing")));
                 continue;
             }
+            if (item.routing() == null && clusterState.getMetaData().routingRequired(item.index(), item.type())) {
+                responses.set(i, new MultiGetItemResponse(null, new MultiGetResponse.Failure(item.index(), item.type(), item.id(), "routing is required, but hasn't been specified")));
+                continue;
+            }
 
             item.routing(clusterState.metaData().resolveIndexRouting(item.routing(), item.index()));
             item.index(clusterState.metaData().concreteIndex(item.index()));

diff --git a/src/main/java/org/elasticsearch/action/termvector/TermVectorRequest.java b/src/main/java/org/elasticsearch/action/termvector/TermVectorRequest.java
@@ -89,6 +89,14 @@ public EnumSet<Flag> getFlags() {
         return flagsEnum;
     }
 
+    /**
+     * Sets the type of document to get the term vector for.
+     */
+    public TermVectorRequest type(String type) {
+        this.type = type;
+        return this;
+    }
+
     /**
      * Returns the type of document to get the term vector for.
      */
@@ -106,8 +114,9 @@ public String id() {
     /**
      * Sets the id of document the term vector is requested for.
      */
-    public void id(String id) {
+    public TermVectorRequest id(String id) {
         this.id = id;
+        return this;
     }
 
     /**
@@ -117,8 +126,9 @@ public String routing() {
         return routing;
     }
 
-    public void routing(String routing) {
+    public TermVectorRequest routing(String routing) {
         this.routing = routing;
+        return this;
     }
 
     /**

diff --git a/src/main/java/org/elasticsearch/action/termvector/TransportMultiTermVectorsAction.java b/src/main/java/org/elasticsearch/action/termvector/TransportMultiTermVectorsAction.java
@@ -71,6 +71,11 @@ protected void doExecute(final MultiTermVectorsRequest request, final ActionList
                         termVectorRequest.type(), termVectorRequest.id(), "[" + termVectorRequest.index() + "] missing")));
                 continue;
             }
+            if (termVectorRequest.routing() == null && clusterState.getMetaData().routingRequired(termVectorRequest.index(), termVectorRequest.type())) {
+                responses.set(i, new MultiTermVectorsItemResponse(null, new MultiTermVectorsResponse.Failure(termVectorRequest.index(),
+                        termVectorRequest.type(), termVectorRequest.id(), "routing is required, but hasn't been specified")));
+                continue;
+            }
             termVectorRequest.index(clusterState.metaData().concreteIndex(termVectorRequest.index()));
             ShardId shardId = clusterService
                     .operationRouting()

diff --git a/src/main/java/org/elasticsearch/action/termvector/TransportSingleShardTermVectorAction.java b/src/main/java/org/elasticsearch/action/termvector/TransportSingleShardTermVectorAction.java
@@ -20,6 +20,7 @@
 package org.elasticsearch.action.termvector;
 
 import org.elasticsearch.ElasticSearchException;
+import org.elasticsearch.action.RoutingMissingException;
 import org.elasticsearch.action.support.single.shard.TransportShardSingleOperationAction;
 import org.elasticsearch.cluster.ClusterService;
 import org.elasticsearch.cluster.ClusterState;
@@ -80,6 +81,11 @@ protected void resolveRequest(ClusterState state, TermVectorRequest request) {
         // update the routing (request#index here is possibly an alias)
         request.routing(state.metaData().resolveIndexRouting(request.routing(), request.index()));
         request.index(state.metaData().concreteIndex(request.index()));
+
+        // Fail fast on the node that received the request.
+        if (request.routing() == null && state.getMetaData().routingRequired(request.index(), request.type())) {
+            throw new RoutingMissingException(request.index(), request.type(), request.id());
+        }
     }
 
     @Override

diff --git a/src/main/java/org/elasticsearch/action/update/TransportUpdateAction.java b/src/main/java/org/elasticsearch/action/update/TransportUpdateAction.java
@@ -24,6 +24,7 @@
 import org.elasticsearch.ElasticSearchIllegalStateException;
 import org.elasticsearch.ExceptionsHelper;
 import org.elasticsearch.action.ActionListener;
+import org.elasticsearch.action.RoutingMissingException;
 import org.elasticsearch.action.admin.indices.create.CreateIndexRequest;
 import org.elasticsearch.action.admin.indices.create.CreateIndexResponse;
 import org.elasticsearch.action.admin.indices.create.TransportCreateIndexAction;
@@ -121,6 +122,11 @@ protected boolean resolveRequest(ClusterState state, UpdateRequest request, Acti
         String aliasOrIndex = request.index();
         request.routing((metaData.resolveIndexRouting(request.routing(), aliasOrIndex)));
         request.index(metaData.concreteIndex(request.index()));
+
+        // Fail fast on the node that received the request, rather than failing when translating on the index or delete request.
+        if (request.routing() == null && state.getMetaData().routingRequired(request.index(), request.type())) {
+            throw new RoutingMissingException(request.index(), request.type(), request.id());
+        }
         return true;
     }
 

diff --git a/src/main/java/org/elasticsearch/cluster/metadata/MetaData.java b/src/main/java/org/elasticsearch/cluster/metadata/MetaData.java
@@ -931,6 +931,22 @@ public boolean isPatternMatchingAllIndices(String[] indicesOrAliases, String[] c
         return false;
     }
 
+    /**
+     * @param concreteIndex The concrete index to check if routing is required
+     * @param type          The type to check if routing is required
+     * @return Whether routing is required according to the mapping for the specified index and type
+     */
+    public boolean routingRequired(String concreteIndex, String type) {
+        IndexMetaData indexMetaData = indices.get(concreteIndex);
+        if (indexMetaData != null) {
+            MappingMetaData mappingMetaData = indexMetaData.getMappings().get(type);
+            if (mappingMetaData != null) {
+                return mappingMetaData.routing().required();
+            }
+        }
+        return false;
+    }
+
     @Override
     public UnmodifiableIterator<IndexMetaData> iterator() {
         return indices.valuesIt();

diff --git a/src/test/java/org/elasticsearch/mget/SimpleMgetTests.java b/src/test/java/org/elasticsearch/mget/SimpleMgetTests.java
@@ -93,8 +93,9 @@ public void testThatParentPerDocumentIsSupported() throws Exception {
         assertThat(mgetResponse.getResponses()[0].isFailed(), is(false));
         assertThat(mgetResponse.getResponses()[0].getResponse().isExists(), is(true));
 
-        assertThat(mgetResponse.getResponses()[1].isFailed(), is(false));
-        assertThat(mgetResponse.getResponses()[1].getResponse().isExists(), is(false));
+        assertThat(mgetResponse.getResponses()[1].isFailed(), is(true));
+        assertThat(mgetResponse.getResponses()[1].getResponse(), nullValue());
+        assertThat(mgetResponse.getResponses()[1].getFailure().getMessage(), equalTo("routing is required, but hasn't been specified"));
     }
 
     @SuppressWarnings("unchecked")

diff --git a/src/test/java/org/elasticsearch/routing/AliasRoutingTests.java b/src/test/java/org/elasticsearch/routing/AliasRoutingTests.java
@@ -355,7 +355,12 @@ public void testRequiredRoutingMappingWithAlias() throws Exception {
         logger.info("--> deleting with no routing, should broadcast the delete since _routing is required");
         client().prepareDelete("test", "type1", "1").setRefresh(true).execute().actionGet();
         for (int i = 0; i < 5; i++) {
-            assertThat(client().prepareGet("test", "type1", "1").execute().actionGet().isExists(), equalTo(false));
+            try {
+                client().prepareGet("test", "type1", "1").get();
+                assert false;
+            } catch (RoutingMissingException e) {
+                assertThat(e.getMessage(), equalTo("routing is required for [test]/[type1]/[1]"));
+            }
             assertThat(client().prepareGet("test", "type1", "1").setRouting("0").execute().actionGet().isExists(), equalTo(false));
         }
 
@@ -367,7 +372,12 @@ public void testRequiredRoutingMappingWithAlias() throws Exception {
         client().prepareBulk().add(Requests.deleteRequest("test").type("type1").id("1")).execute().actionGet();
         client().admin().indices().prepareRefresh().execute().actionGet();
         for (int i = 0; i < 5; i++) {
-            assertThat(client().prepareGet("test", "type1", "1").execute().actionGet().isExists(), equalTo(false));
+            try {
+                assertThat(client().prepareGet("test", "type1", "1").execute().actionGet().isExists(), equalTo(false));
+                assert false;
+            } catch (RoutingMissingException e) {
+                assertThat(e.getMessage(), equalTo("routing is required for [test]/[type1]/[1]"));
+            }
             assertThat(client().prepareGet("test", "type1", "1").setRouting("0").execute().actionGet().isExists(), equalTo(false));
         }
     }