From 4e6cbe0d50f832a4d47e12251770c2e25f27988b Mon Sep 17 00:00:00 2001 From: javanna Date: Sat, 19 Nov 2016 15:04:26 +0100 Subject: [PATCH] Remove 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 removes support for the type parameter from the REST layer and 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. --- .../shards/ClusterSearchShardsRequest.java | 44 ++++------ .../ClusterSearchShardsRequestBuilder.java | 9 --- .../RestClusterSearchShardsAction.java | 5 -- .../java/org/elasticsearch/VersionTests.java | 2 + .../ClusterSearchShardsRequestTests.java | 80 +++++++++++++++++++ .../migration/migrate_6_0/search.asciidoc | 6 ++ docs/reference/search/search-shards.asciidoc | 2 +- .../rest-api-spec/api/search_shards.json | 8 +- 8 files changed, 106 insertions(+), 50 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 8dc747474c4bd..4464543a62907 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,14 +30,15 @@ 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; + public static final Version V_5_1_0_UNRELEASED = Version.fromId(5010099); + 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(); @@ -57,14 +59,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; @@ -88,23 +85,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. */ @@ -154,7 +134,10 @@ public void readFrom(StreamInput in) throws IOException { routing = in.readOptionalString(); preference = in.readOptionalString(); - types = in.readStringArray(); + if (in.getVersion().onOrBefore(V_5_1_0_UNRELEASED)) { + //types + in.readStringArray(); + } indicesOptions = IndicesOptions.readIndicesOptions(in); } @@ -170,7 +153,10 @@ public void writeTo(StreamOutput out) throws IOException { out.writeOptionalString(routing); out.writeOptionalString(preference); - out.writeStringArray(types); + if (out.getVersion().onOrBefore(V_5_1_0_UNRELEASED)) { + //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 030d5db647258..7cb7ac1254c60 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 @@ -37,15 +37,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..541fb96f65b4a 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 @@ -45,8 +45,6 @@ public RestClusterSearchShardsAction(Settings settings, RestController controlle controller.registerHandler(POST, "/_search_shards", this); controller.registerHandler(GET, "/{index}/_search_shards", this); controller.registerHandler(POST, "/{index}/_search_shards", this); - controller.registerHandler(GET, "/{index}/{type}/_search_shards", this); - controller.registerHandler(POST, "/{index}/{type}/_search_shards", this); } @Override @@ -54,12 +52,9 @@ public RestChannelConsumer prepareRequest(final RestRequest request, final NodeC String[] indices = Strings.splitStringByCommaToArray(request.param("index")); final ClusterSearchShardsRequest clusterSearchShardsRequest = Requests.clusterSearchShardsRequest(indices); clusterSearchShardsRequest.local(request.paramAsBoolean("local", clusterSearchShardsRequest.local())); - - clusterSearchShardsRequest.types(Strings.splitStringByCommaToArray(request.param("type"))); 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/VersionTests.java b/core/src/test/java/org/elasticsearch/VersionTests.java index 1e87f7eeb3151..c745088ca63c1 100644 --- a/core/src/test/java/org/elasticsearch/VersionTests.java +++ b/core/src/test/java/org/elasticsearch/VersionTests.java @@ -19,6 +19,7 @@ package org.elasticsearch; +import org.elasticsearch.action.admin.cluster.shards.ClusterSearchShardsRequest; import org.elasticsearch.cluster.metadata.IndexMetaData; import org.elasticsearch.common.lucene.Lucene; import org.elasticsearch.common.settings.Settings; @@ -288,6 +289,7 @@ public void testUnknownVersions() { // once we released 5.0.0 and it's added to Version.java we need to remove this constant assertUnknownVersion(Script.V_5_1_0_UNRELEASED); // once we released 5.0.0 and it's added to Version.java we need to remove this constant + assertUnknownVersion(ClusterSearchShardsRequest.V_5_1_0_UNRELEASED); } public static void assertUnknownVersion(Version version) { 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/migration/migrate_6_0/search.asciidoc b/docs/reference/migration/migrate_6_0/search.asciidoc index fe72768e5907c..cd39141095de1 100644 --- a/docs/reference/migration/migrate_6_0/search.asciidoc +++ b/docs/reference/migration/migrate_6_0/search.asciidoc @@ -9,3 +9,9 @@ * Queries on boolean fields now strictly parse boolean-like values. This means only the strings `"true"` and `"false"` will be parsed into their boolean counterparts. Other strings will cause an error to be thrown. + + +==== Search shards API + +The search shards API no longer accepts the `type` url parameter, which didn't +have any effect in previous versions. diff --git a/docs/reference/search/search-shards.asciidoc b/docs/reference/search/search-shards.asciidoc index c07c3755a261a..917a50ae9a7fc 100644 --- a/docs/reference/search/search-shards.asciidoc +++ b/docs/reference/search/search-shards.asciidoc @@ -5,7 +5,7 @@ The search shards api returns the indices and shards that a search request would be executed against. This can give useful feedback for working out issues or planning optimizations with routing and shard preferences. -The `index` and `type` parameters may be single values, or comma-separated. +The `index` may be a single value, or comma-separated. [float] === Usage diff --git a/rest-api-spec/src/main/resources/rest-api-spec/api/search_shards.json b/rest-api-spec/src/main/resources/rest-api-spec/api/search_shards.json index e03ed8fef2fec..b3de107b79787 100644 --- a/rest-api-spec/src/main/resources/rest-api-spec/api/search_shards.json +++ b/rest-api-spec/src/main/resources/rest-api-spec/api/search_shards.json @@ -3,16 +3,12 @@ "documentation": "http://www.elastic.co/guide/en/elasticsearch/reference/master/search-shards.html", "methods": ["GET", "POST"], "url": { - "path": "/{index}/{type}/_search_shards", - "paths": ["/_search_shards", "/{index}/_search_shards", "/{index}/{type}/_search_shards"], + "path": "/{index}/_search_shards", + "paths": ["/_search_shards", "/{index}/_search_shards"], "parts": { "index": { "type" : "list", "description" : "A comma-separated list of index names to search; use `_all` or empty string to perform the operation on all indices" - }, - "type": { - "type" : "list", - "description" : "A comma-separated list of document types to search; leave empty to perform the operation on all types" } }, "params": {