Deprecate ignored type parameter in search_shards api (#21730)

The `type` parameter has always been accepted by the search_shards api, probably to make the api and its urls the same as search. Truth is that the type never had any effect, it's been ignored from day one while accepting it may make users think that we actually do something with it.

This commit deprecate support for the type parameter from the REST layer and removes it from the Java API. Backwards compatibility is maintained on the transport layer though.

The new added serialization test also uncovered a bug in the java API where the `ClusterSearchShardsRequest` could be created with no arguments, but the indices were required to be not null otherwise the request couldn't be serialized as `writeTo` would throw NPE. Fixed by setting a default value (empty array) for indices.

Relates to #21688

Author: javanna

core/src/main/java/org/elasticsearch/action/admin/cluster/shards/ClusterSearchShardsRequest.java

@@ -19,6 +19,7 @@
 
 package org.elasticsearch.action.admin.cluster.shards;
 
+import org.elasticsearch.Version;
 import org.elasticsearch.action.ActionRequestValidationException;
 import org.elasticsearch.action.IndicesRequest;
 import org.elasticsearch.action.support.IndicesOptions;
@@ -29,16 +30,16 @@
 import org.elasticsearch.common.io.stream.StreamOutput;
 
 import java.io.IOException;
+import java.util.Objects;
 
 /**
  */
 public class ClusterSearchShardsRequest extends MasterNodeReadRequest<ClusterSearchShardsRequest> implements IndicesRequest.Replaceable {
-    private String[] indices;
+    private String[] indices = Strings.EMPTY_ARRAY;
     @Nullable
     private String routing;
     @Nullable
     private String preference;
-    private String[] types = Strings.EMPTY_ARRAY;
     private IndicesOptions indicesOptions = IndicesOptions.lenientExpandOpen();
 
 
@@ -59,14 +60,9 @@ public ActionRequestValidationException validate() {
      */
     @Override
     public ClusterSearchShardsRequest indices(String... indices) {
-        if (indices == null) {
-            throw new IllegalArgumentException("indices must not be null");
-        } else {
-            for (int i = 0; i < indices.length; i++) {
-                if (indices[i] == null) {
-                    throw new IllegalArgumentException("indices[" + i + "] must not be null");
-                }
-            }
+        Objects.requireNonNull(indices, "indices must not be null");
+        for (int i = 0; i < indices.length; i++) {
+            Objects.requireNonNull(indices[i], "indices[" + i + "] must not be null");
         }
         this.indices = indices;
         return this;
@@ -90,23 +86,6 @@ public ClusterSearchShardsRequest indicesOptions(IndicesOptions indicesOptions)
         return this;
     }
 
-    /**
-     * The document types to execute the search against. Defaults to be executed against
-     * all types.
-     */
-    public String[] types() {
-        return types;
-    }
-
-    /**
-     * The document types to execute the search against. Defaults to be executed against
-     * all types.
-     */
-    public ClusterSearchShardsRequest types(String... types) {
-        this.types = types;
-        return this;
-    }
-
     /**
      * A comma separated list of routing values to control the shards the search will be executed on.
      */
@@ -156,7 +135,10 @@ public void readFrom(StreamInput in) throws IOException {
         routing = in.readOptionalString();
         preference = in.readOptionalString();
 
-        types = in.readStringArray();
+        if (in.getVersion().onOrBefore(Version.V_5_1_0)) {
+            //types
+            in.readStringArray();
+        }
         indicesOptions = IndicesOptions.readIndicesOptions(in);
     }
 
@@ -172,7 +154,10 @@ public void writeTo(StreamOutput out) throws IOException {
         out.writeOptionalString(routing);
         out.writeOptionalString(preference);
 
-        out.writeStringArray(types);
+        if (out.getVersion().onOrBefore(Version.V_5_1_0)) {
+            //types
+            out.writeStringArray(Strings.EMPTY_ARRAY);
+        }
         indicesOptions.writeIndicesOptions(out);
     }
 

core/src/main/java/org/elasticsearch/action/admin/cluster/shards/ClusterSearchShardsRequestBuilder.java

@@ -39,15 +39,6 @@ public ClusterSearchShardsRequestBuilder setIndices(String... indices) {
         return this;
     }
 
-    /**
-     * The document types to execute the search against. Defaults to be executed against
-     * all types.
-     */
-    public ClusterSearchShardsRequestBuilder setTypes(String... types) {
-        request.types(types);
-        return this;
-    }
-
     /**
      * A comma separated list of routing values to control the shards the search will be executed on.
      */

core/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestClusterSearchShardsAction.java

@@ -55,11 +55,14 @@ public RestChannelConsumer prepareRequest(final RestRequest request, final NodeC
         final ClusterSearchShardsRequest clusterSearchShardsRequest = Requests.clusterSearchShardsRequest(indices);
         clusterSearchShardsRequest.local(request.paramAsBoolean("local", clusterSearchShardsRequest.local()));
 
-        clusterSearchShardsRequest.types(Strings.splitStringByCommaToArray(request.param("type")));
+        if (request.hasParam("type")) {
+            String type = request.param("type");
+            deprecationLogger.deprecated("type [" + type + "] doesn't have any effect in the search shards api, " +
+                    "it should be rather omitted");
+        }
         clusterSearchShardsRequest.routing(request.param("routing"));
         clusterSearchShardsRequest.preference(request.param("preference"));
         clusterSearchShardsRequest.indicesOptions(IndicesOptions.fromRequest(request, clusterSearchShardsRequest.indicesOptions()));
-
         return channel -> client.admin().cluster().searchShards(clusterSearchShardsRequest, new RestToXContentListener<>(channel));
     }
 }

core/src/test/java/org/elasticsearch/action/admin/cluster/shards/ClusterSearchShardsRequestTests.java

@@ -0,0 +1,80 @@
+/*
+ * Licensed to Elasticsearch under one or more contributor
+ * license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright
+ * ownership. Elasticsearch licenses this file to you under
+ * the Apache License, Version 2.0 (the "License"); you may
+ * not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.elasticsearch.action.admin.cluster.shards;
+
+import org.elasticsearch.Version;
+import org.elasticsearch.action.support.IndicesOptions;
+import org.elasticsearch.common.io.stream.BytesStreamOutput;
+import org.elasticsearch.common.io.stream.StreamInput;
+import org.elasticsearch.test.ESTestCase;
+import org.elasticsearch.test.VersionUtils;
+
+public class ClusterSearchShardsRequestTests extends ESTestCase {
+
+    public void testSerialization() throws Exception {
+        ClusterSearchShardsRequest request = new ClusterSearchShardsRequest();
+        if (randomBoolean()) {
+            int numIndices = randomIntBetween(1, 5);
+            String[] indices = new String[numIndices];
+            for (int i = 0; i < numIndices; i++) {
+                indices[i] = randomAsciiOfLengthBetween(3, 10);
+            }
+            request.indices(indices);
+        }
+        if (randomBoolean()) {
+            request.indicesOptions(
+                    IndicesOptions.fromOptions(randomBoolean(), randomBoolean(), randomBoolean(), randomBoolean()));
+        }
+        if (randomBoolean()) {
+            request.preference(randomAsciiOfLengthBetween(3, 10));
+        }
+        if (randomBoolean()) {
+            int numRoutings = randomIntBetween(1, 3);
+            String[] routings = new String[numRoutings];
+            for (int i = 0; i < numRoutings; i++) {
+                routings[i] = randomAsciiOfLengthBetween(3, 10);
+            }
+            request.routing(routings);
+        }
+
+        Version version = VersionUtils.randomVersion(random());
+        try (BytesStreamOutput out = new BytesStreamOutput()) {
+            out.setVersion(version);
+            request.writeTo(out);
+            try (StreamInput in = out.bytes().streamInput()) {
+                in.setVersion(version);
+                ClusterSearchShardsRequest deserialized = new ClusterSearchShardsRequest();
+                deserialized.readFrom(in);
+                assertArrayEquals(request.indices(), deserialized.indices());
+                assertSame(request.indicesOptions(), deserialized.indicesOptions());
+                assertEquals(request.routing(), deserialized.routing());
+                assertEquals(request.preference(), deserialized.preference());
+            }
+        }
+    }
+
+    public void testIndicesMustNotBeNull() {
+        ClusterSearchShardsRequest request = new ClusterSearchShardsRequest();
+        assertNotNull(request.indices());
+        expectThrows(NullPointerException.class, () -> request.indices((String[])null));
+        expectThrows(NullPointerException.class, () -> request.indices((String)null));
+        expectThrows(NullPointerException.class, () -> request.indices(new String[]{"index1", null, "index3"}));
+    }
+}

docs/reference/search/search-shards.asciidoc

@@ -7,6 +7,8 @@ planning optimizations with routing and shard preferences.
 
 The `index` and `type` parameters may be single values, or comma-separated.
 
+The `type` parameter is deprecated deprecated[5.1.0,was ignored in previous versions].
+
 [float]
 === Usage
 

rest-api-spec/src/main/resources/rest-api-spec/test/search_shards/10_basic.yaml

@@ -10,3 +10,23 @@
         routing: foo
 
   - match: { shards.0.0.index: test_1 }
+
+---
+"Search shards with type":
+  - skip:
+    features: "warnings"
+    version: " - 5.0.99"
+    reason: type was deprecated in 5.1.0
+
+  - do:
+      indices.create:
+        index: test_1
+
+  - do:
+      warnings:
+        - type [whatever] doesn't have any effect in the search shards api, it should be rather omitted
+      search_shards:
+        index:  test_1
+        type: whatever
+
+  - match: { shards.0.0.index: test_1 }