From 33c156dce23ec8d8031648f3a16dfa4cfbb45688 Mon Sep 17 00:00:00 2001 From: javanna Date: Mon, 21 Nov 2016 23:06:47 +0100 Subject: [PATCH] Deprecate ignored type parameter in search_shards api 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 --- .../shards/ClusterSearchShardsRequest.java | 43 ++++------ .../ClusterSearchShardsRequestBuilder.java | 9 --- .../RestClusterSearchShardsAction.java | 7 +- .../ClusterSearchShardsRequestTests.java | 80 +++++++++++++++++++ docs/reference/search/search-shards.asciidoc | 2 + .../test/search_shards/10_basic.yaml | 20 +++++ 6 files changed, 121 insertions(+), 40 deletions(-) create mode 100644 core/src/test/java/org/elasticsearch/action/admin/cluster/shards/ClusterSearchShardsRequestTests.java diff --git a/core/src/main/java/org/elasticsearch/action/admin/cluster/shards/ClusterSearchShardsRequest.java b/core/src/main/java/org/elasticsearch/action/admin/cluster/shards/ClusterSearchShardsRequest.java index 21ecf8a4c4feb..8a4bb6683f6bf 100644 --- a/core/src/main/java/org/elasticsearch/action/admin/cluster/shards/ClusterSearchShardsRequest.java +++ b/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 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); } diff --git a/core/src/main/java/org/elasticsearch/action/admin/cluster/shards/ClusterSearchShardsRequestBuilder.java b/core/src/main/java/org/elasticsearch/action/admin/cluster/shards/ClusterSearchShardsRequestBuilder.java index 0b9c9d044e7b6..100c9b258f2cc 100644 --- a/core/src/main/java/org/elasticsearch/action/admin/cluster/shards/ClusterSearchShardsRequestBuilder.java +++ b/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. */ diff --git a/core/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestClusterSearchShardsAction.java b/core/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestClusterSearchShardsAction.java index 459ccf5fa4dc4..6de61c37d4556 100644 --- a/core/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestClusterSearchShardsAction.java +++ b/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)); } } diff --git a/core/src/test/java/org/elasticsearch/action/admin/cluster/shards/ClusterSearchShardsRequestTests.java b/core/src/test/java/org/elasticsearch/action/admin/cluster/shards/ClusterSearchShardsRequestTests.java new file mode 100644 index 0000000000000..7c38a7ff656a3 --- /dev/null +++ b/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"})); + } +} diff --git a/docs/reference/search/search-shards.asciidoc b/docs/reference/search/search-shards.asciidoc index c07c3755a261a..8933561f67f1a 100644 --- a/docs/reference/search/search-shards.asciidoc +++ b/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 diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/search_shards/10_basic.yaml b/rest-api-spec/src/main/resources/rest-api-spec/test/search_shards/10_basic.yaml index 4bfe6e953b250..00c8a2ce1d472 100644 --- a/rest-api-spec/src/main/resources/rest-api-spec/test/search_shards/10_basic.yaml +++ b/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 }