Fix from and size parameter can be negative when searching (opensearc…

…h-project#13047) * Fix from and size parameter can be negative when searching Signed-off-by: Gao Binlong <gbinlong@amazon.com> * Add changelog Signed-off-by: Gao Binlong <gbinlong@amazon.com> * fix test failure Signed-off-by: Gao Binlong <gbinlong@amazon.com> * Fix test failure Signed-off-by: Gao Binlong <gbinlong@amazon.com> * Optimize the code Signed-off-by: Gao Binlong <gbinlong@amazon.com> --------- Signed-off-by: Gao Binlong <gbinlong@amazon.com>
gaobinlong · Apr 23, 2024 · ab7e914 · ab7e914
1 parent f41db2c
commit ab7e914
Show file tree

Hide file tree

Showing 6 changed files with 161 additions and 11 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -61,6 +61,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 - Fix issue with feature flags where default value may not be honored ([#12849](https://github.com/opensearch-project/OpenSearch/pull/12849))
 - Fix UOE While building Exists query for nested search_as_you_type field ([#12048](https://github.com/opensearch-project/OpenSearch/pull/12048))
 - Client with Java 8 runtime and Apache HttpClient 5 Transport fails with java.lang.NoSuchMethodError: java.nio.ByteBuffer.flip()Ljava/nio/ByteBuffer ([#13100](https://github.com/opensearch-project/opensearch-java/pull/13100))
+- Fix from and size parameter can be negative when searching ([#13047](https://github.com/opensearch-project/OpenSearch/pull/13047))
 - Enabled mockTelemetryPlugin for IT and fixed OOM issues ([#13054](https://github.com/opensearch-project/OpenSearch/pull/13054))
 - Fix implement mark() and markSupported() in class FilterStreamInput ([#13098](https://github.com/opensearch-project/OpenSearch/pull/13098))
 - Fix snapshot _status API to return correct status for partial snapshots ([#12812](https://github.com/opensearch-project/OpenSearch/pull/12812))

diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/search/360_from_and_size.yml b/rest-api-spec/src/main/resources/rest-api-spec/test/search/360_from_and_size.yml
@@ -0,0 +1,113 @@
+setup:
+  - do:
+      indices.create:
+          index:  test_1
+  - do:
+      index:
+          index:  test_1
+          id:     1
+          body:   { foo: bar }
+  - do:
+      index:
+          index:  test_1
+          id:     2
+          body:   { foo: bar }
+  - do:
+      index:
+          index:  test_1
+          id:     3
+          body:   { foo: bar }
+
+  - do:
+      index:
+          index:  test_1
+          id:     4
+          body:   { foo: bar }
+  - do:
+      indices.refresh:
+          index: [test_1]
+
+---
+teardown:
+  - do:
+      indices.delete:
+        index: test_1
+        ignore: 404
+
+---
+"Throws exception if from or size query parameter is negative":
+  - skip:
+      version: " - 2.99.99"
+      reason: "fixed in 3.0.0"
+  - do:
+      catch: '/\[from\] parameter cannot be negative, found \[-5\]/'
+      search:
+        index: test_1
+        from: -5
+        size: 10
+        body:
+          query:
+            match:
+              foo: bar
+
+  - do:
+      catch: '/\[size\] parameter cannot be negative, found \[-1\]/'
+      search:
+        index: test_1
+        from: 0
+        size: -1
+        body:
+          query:
+            match:
+              foo: bar
+
+  - do:
+      search:
+        index: test_1
+        from: 0
+        size: 10
+        body:
+          query:
+            match:
+              foo: bar
+
+  - match: {hits.total.value: 4}
+
+---
+"Throws exception if from or size request body parameter is negative":
+  - skip:
+      version: " - 2.99.99"
+      reason: "fixed in 3.0.0"
+  - do:
+      catch: '/\[from\] parameter cannot be negative, found \[-5\]/'
+      search:
+        index: test_1
+        body:
+          from: -5
+          size: 10
+          query:
+            match:
+              foo: bar
+
+  - do:
+      catch: '/\[size\] parameter cannot be negative, found \[-1\]/'
+      search:
+        index: test_1
+        body:
+          from: 0
+          size: -1
+          query:
+            match:
+              foo: bar
+
+  - do:
+      search:
+        index: test_1
+        body:
+          from: 0
+          size: 10
+          query:
+            match:
+              foo: bar
+
+  - match: {hits.total.value: 4}
diff --git a/server/src/main/java/org/opensearch/action/search/SearchRequest.java b/server/src/main/java/org/opensearch/action/search/SearchRequest.java
@@ -497,7 +497,7 @@ public PointInTimeBuilder pointInTimeBuilder() {
     }
 
     /**
-     * The tye of search to execute.
+     * The type of search to execute.
      */
     public SearchType searchType() {
         return searchType;

diff --git a/server/src/main/java/org/opensearch/rest/action/search/RestSearchAction.java b/server/src/main/java/org/opensearch/rest/action/search/RestSearchAction.java
@@ -50,6 +50,7 @@
 import org.opensearch.rest.action.RestCancellableNodeClient;
 import org.opensearch.rest.action.RestStatusToXContentListener;
 import org.opensearch.search.Scroll;
+import org.opensearch.search.SearchService;
 import org.opensearch.search.builder.SearchSourceBuilder;
 import org.opensearch.search.fetch.StoredFieldsContext;
 import org.opensearch.search.fetch.subphase.FetchSourceContext;
@@ -235,13 +236,12 @@ private static void parseSearchSource(final SearchSourceBuilder searchSourceBuil
             searchSourceBuilder.query(queryBuilder);
         }
 
-        int from = request.paramAsInt("from", -1);
-        if (from != -1) {
-            searchSourceBuilder.from(from);
+        if (request.hasParam("from")) {
+            searchSourceBuilder.from(request.paramAsInt("from", SearchService.DEFAULT_FROM));
         }
-        int size = request.paramAsInt("size", -1);
-        if (size != -1) {
-            setSize.accept(size);
+
+        if (request.hasParam("size")) {
+            setSize.accept(request.paramAsInt("size", SearchService.DEFAULT_SIZE));
         }
 
         if (request.hasParam("explain")) {

diff --git a/server/src/main/java/org/opensearch/search/builder/SearchSourceBuilder.java b/server/src/main/java/org/opensearch/search/builder/SearchSourceBuilder.java
@@ -418,7 +418,7 @@ public QueryBuilder postFilter() {
      */
     public SearchSourceBuilder from(int from) {
         if (from < 0) {
-            throw new IllegalArgumentException("[from] parameter cannot be negative");
+            throw new IllegalArgumentException("[from] parameter cannot be negative, found [" + from + "]");
         }
         this.from = from;
         return this;
@@ -1215,9 +1215,9 @@ public void parseXContent(XContentParser parser, boolean checkTrailingTokens) th
                 currentFieldName = parser.currentName();
             } else if (token.isValue()) {
                 if (FROM_FIELD.match(currentFieldName, parser.getDeprecationHandler())) {
-                    from = parser.intValue();
+                    from(parser.intValue());
                 } else if (SIZE_FIELD.match(currentFieldName, parser.getDeprecationHandler())) {
-                    size = parser.intValue();
+                    size(parser.intValue());
                 } 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/opensearch/search/builder/SearchSourceBuilderTests.java b/server/src/test/java/org/opensearch/search/builder/SearchSourceBuilderTests.java
@@ -571,7 +571,7 @@ public void testParseIndicesBoost() throws IOException {
 
     public void testNegativeFromErrors() {
         IllegalArgumentException expected = expectThrows(IllegalArgumentException.class, () -> new SearchSourceBuilder().from(-2));
-        assertEquals("[from] parameter cannot be negative", expected.getMessage());
+        assertEquals("[from] parameter cannot be negative, found [-2]", expected.getMessage());
     }
 
     public void testNegativeSizeErrors() {
@@ -582,6 +582,42 @@ public void testNegativeSizeErrors() {
         assertEquals("[size] parameter cannot be negative, found [-1]", expected.getMessage());
     }
 
+    public void testParseFromAndSize() throws IOException {
+        int negativeFrom = randomIntBetween(-100, -1);
+        String restContent = " { \"from\": \"" + negativeFrom + "\"}";
+        try (XContentParser parser = createParser(JsonXContent.jsonXContent, restContent)) {
+            IllegalArgumentException expected = expectThrows(
+                IllegalArgumentException.class,
+                () -> SearchSourceBuilder.fromXContent(parser)
+            );
+            assertEquals("[from] parameter cannot be negative, found [" + negativeFrom + "]", expected.getMessage());
+        }
+
+        int validFrom = randomIntBetween(0, 100);
+        restContent = " { \"from\": \"" + validFrom + "\"}";
+        try (XContentParser parser = createParser(JsonXContent.jsonXContent, restContent)) {
+            SearchSourceBuilder searchSourceBuilder = SearchSourceBuilder.fromXContent(parser);
+            assertEquals(validFrom, searchSourceBuilder.from());
+        }
+
+        int negativeSize = randomIntBetween(-100, -1);
+        restContent = " { \"size\": \"" + negativeSize + "\"}";
+        try (XContentParser parser = createParser(JsonXContent.jsonXContent, restContent)) {
+            IllegalArgumentException expected = expectThrows(
+                IllegalArgumentException.class,
+                () -> SearchSourceBuilder.fromXContent(parser)
+            );
+            assertEquals("[size] parameter cannot be negative, found [" + negativeSize + "]", expected.getMessage());
+        }
+
+        int validSize = randomIntBetween(0, 100);
+        restContent = " { \"size\": \"" + validSize + "\"}";
+        try (XContentParser parser = createParser(JsonXContent.jsonXContent, restContent)) {
+            SearchSourceBuilder searchSourceBuilder = SearchSourceBuilder.fromXContent(parser);
+            assertEquals(validSize, searchSourceBuilder.size());
+        }
+    }
+
     private void assertIndicesBoostParseErrorMessage(String restContent, String expectedErrorMessage) throws IOException {
         try (XContentParser parser = createParser(JsonXContent.jsonXContent, restContent)) {
             ParsingException e = expectThrows(ParsingException.class, () -> SearchSourceBuilder.fromXContent(parser));