[Rest Api Compatibility] Allow to use size -1 (elastic#75342)

previously disallowed in elastic#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 elastic#69548 relates elastic#51816
ywangd · Jul 30, 2021 · a5d2317 · a5d2317
1 parent 4f0b4e9
commit a5d2317
Show file tree

Hide file tree

Showing 4 changed files with 38 additions and 12 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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 {