From a5d231715462834a5f45fbaca6eb81d1f423cb67 Mon Sep 17 00:00:00 2001 From: Przemyslaw Gomulka Date: Fri, 16 Jul 2021 12:46:05 +0200 Subject: [PATCH] [Rest Api Compatibility] Allow to use size -1 (#75342) previously disallowed in #70209. However since now the recommendation is to not set the field at all - and rely on a default - this would be a shape change for users prefering to set -1 so far. With request compatible with v7 they will still be allowed to do this and will get a warning. relates #69548 relates #51816 --- rest-api-spec/build.gradle | 1 - .../rest/action/search/RestSearchAction.java | 17 +++++++++---- .../search/builder/SearchSourceBuilder.java | 24 +++++++++++++------ .../builder/SearchSourceBuilderTests.java | 8 +++++++ 4 files changed, 38 insertions(+), 12 deletions(-) diff --git a/rest-api-spec/build.gradle b/rest-api-spec/build.gradle index 1a7251640334..56aabfc0092a 100644 --- a/rest-api-spec/build.gradle +++ b/rest-api-spec/build.gradle @@ -97,7 +97,6 @@ tasks.named("yamlRestCompatTest").configure { 'search.aggregation/200_top_hits_metric/top_hits aggregation with sequence numbers', 'search.aggregation/51_filter_with_types/Filter aggs with terms lookup and ensure it\'s cached', 'search/150_rewrite_on_coordinator/Ensure that we fetch the document only once', //terms_lookup - 'search/260_parameter_validation/test size=-1 is deprecated', //size=-1 change 'search/310_match_bool_prefix/multi_match multiple fields with cutoff_frequency throws exception', //cutoff_frequency 'search/340_type_query/type query', // type_query - probably should behave like match_all ] + v7compatibilityNotSupportedTests()) diff --git a/server/src/main/java/org/elasticsearch/rest/action/search/RestSearchAction.java b/server/src/main/java/org/elasticsearch/rest/action/search/RestSearchAction.java index 504038414159..5ee3debf0682 100644 --- a/server/src/main/java/org/elasticsearch/rest/action/search/RestSearchAction.java +++ b/server/src/main/java/org/elasticsearch/rest/action/search/RestSearchAction.java @@ -16,12 +16,12 @@ import org.elasticsearch.action.search.SearchRequest; import org.elasticsearch.action.support.IndicesOptions; import org.elasticsearch.client.node.NodeClient; -import org.elasticsearch.core.Booleans; -import org.elasticsearch.core.RestApiVersion; import org.elasticsearch.common.Strings; import org.elasticsearch.common.io.stream.NamedWriteableRegistry; import org.elasticsearch.common.logging.DeprecationLogger; import org.elasticsearch.common.xcontent.XContentParser; +import org.elasticsearch.core.Booleans; +import org.elasticsearch.core.RestApiVersion; import org.elasticsearch.index.query.QueryBuilder; import org.elasticsearch.rest.BaseRestHandler; import org.elasticsearch.rest.RestRequest; @@ -207,7 +207,16 @@ private static void parseSearchSource(final SearchSourceBuilder searchSourceBuil } if (request.hasParam("size")) { int size = request.paramAsInt("size", SearchService.DEFAULT_SIZE); - setSize.accept(size); + if (request.getRestApiVersion() == RestApiVersion.V_7 && size == -1) { + // we treat -1 as not-set, but deprecate it to be able to later remove this funny extra treatment + deprecationLogger.compatibleApiWarning( + "search-api-size-1", + "Using search size of -1 is deprecated and will be removed in future versions. " + + "Instead, don't use the `size` parameter if you don't want to set it explicitly." + ); + } else { + setSize.accept(size); + } } if (request.hasParam("explain")) { @@ -303,7 +312,7 @@ static void preparePointInTime(SearchRequest request, RestRequest restRequest, N assert request.pointInTimeBuilder() != null; ActionRequestValidationException validationException = null; if (request.indices().length > 0) { - validationException = addValidationError("[indices] cannot be used with point in time. Do " + + validationException = addValidationError("[indices] cannot be used with point in time. Do " + "not specify any index with point in time.", validationException); } if (request.indicesOptions().equals(DEFAULT_INDICES_OPTIONS) == false) { diff --git a/server/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java b/server/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java index 72889ef6a362..764cc1ffe466 100644 --- a/server/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java @@ -10,23 +10,23 @@ import org.elasticsearch.ElasticsearchException; import org.elasticsearch.Version; -import org.elasticsearch.common.logging.DeprecationLogger; -import org.elasticsearch.core.Booleans; -import org.elasticsearch.core.Nullable; -import org.elasticsearch.common.xcontent.ParseField; import org.elasticsearch.common.ParsingException; import org.elasticsearch.common.Strings; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.io.stream.Writeable; -import org.elasticsearch.core.RestApiVersion; -import org.elasticsearch.core.TimeValue; +import org.elasticsearch.common.logging.DeprecationLogger; +import org.elasticsearch.common.xcontent.ParseField; import org.elasticsearch.common.xcontent.ToXContentFragment; import org.elasticsearch.common.xcontent.ToXContentObject; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentHelper; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.common.xcontent.XContentType; +import org.elasticsearch.core.Booleans; +import org.elasticsearch.core.Nullable; +import org.elasticsearch.core.RestApiVersion; +import org.elasticsearch.core.TimeValue; import org.elasticsearch.index.query.QueryBuilder; import org.elasticsearch.index.query.QueryRewriteContext; import org.elasticsearch.index.query.Rewriteable; @@ -1121,7 +1121,17 @@ public void parseXContent(XContentParser parser, boolean checkTrailingTokens) th if (FROM_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { from(parser.intValue()); } else if (SIZE_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { - size(parser.intValue()); + int parsedSize = parser.intValue(); + if (parser.getRestApiVersion() == RestApiVersion.V_7 && parsedSize == -1) { + // we treat -1 as not-set, but deprecate it to be able to later remove this funny extra treatment + deprecationLogger.compatibleApiWarning( + "search-api-size-1", + "Using search size of -1 is deprecated and will be removed in future versions. " + + "Instead, don't use the `size` parameter if you don't want to set it explicitly." + ); + } else { + size(parsedSize); + } } else if (TIMEOUT_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { timeout = TimeValue.parseTimeValue(parser.text(), null, TIMEOUT_FIELD.getPreferredName()); } else if (TERMINATE_AFTER_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { diff --git a/server/src/test/java/org/elasticsearch/search/builder/SearchSourceBuilderTests.java b/server/src/test/java/org/elasticsearch/search/builder/SearchSourceBuilderTests.java index 825a4f8593a9..85150128ce1b 100644 --- a/server/src/test/java/org/elasticsearch/search/builder/SearchSourceBuilderTests.java +++ b/server/src/test/java/org/elasticsearch/search/builder/SearchSourceBuilderTests.java @@ -484,6 +484,14 @@ public void testNegativeSizeErrors() throws IOException { IllegalArgumentException ex = expectThrows(IllegalArgumentException.class, () -> SearchSourceBuilder.fromXContent(parser)); assertThat(ex.getMessage(), containsString(Integer.toString(randomSize))); } + + restContent = "{\"size\" : -1}"; + try (XContentParser parser = createParserWithCompatibilityFor(JsonXContent.jsonXContent, restContent, RestApiVersion.V_7)) { + SearchSourceBuilder searchSourceBuilder = SearchSourceBuilder.fromXContent(parser); + assertEquals(-1, searchSourceBuilder.size()); + } + assertWarnings("Using search size of -1 is deprecated and will be removed in future versions. Instead, don't use the `size` " + + "parameter if you don't want to set it explicitly."); } public void testNegativeTerminateAfter() throws IOException {