diff --git a/CHANGELOG.md b/CHANGELOG.md index fb4eb8c5a5ec4..00a259cbc6fc3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -63,6 +63,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)) - Enabled mockTelemetryPlugin for IT and fixed OOM issues ([#13054](https://github.com/opensearch-project/OpenSearch/pull/13054)) +- Fix from and size parameter can be negative when searching ([#13047](https://github.com/opensearch-project/OpenSearch/pull/13047)) - 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)) - Improve the error messages for _stats with closed indices ([#13012](https://github.com/opensearch-project/OpenSearch/pull/13012)) 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 new file mode 100644 index 0000000000000..7f3fb77b86366 --- /dev/null +++ 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.13.99" + reason: "fixed in 2.14.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.13.99" + reason: "fixed in 2.14.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 index c2350313c48aa..7462faea6ed8d 100644 --- a/server/src/main/java/org/opensearch/action/search/SearchRequest.java +++ b/server/src/main/java/org/opensearch/action/search/SearchRequest.java @@ -516,7 +516,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 index 80dc34c4d5d68..3a6b45013e892 100644 --- 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 index 8129039f02d8a..f65baa7df5cd7 100644 --- a/server/src/main/java/org/opensearch/search/builder/SearchSourceBuilder.java +++ b/server/src/main/java/org/opensearch/search/builder/SearchSourceBuilder.java @@ -435,7 +435,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; @@ -1232,9 +1232,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 index eea7b1829e9b0..5b1035e24185d 100644 --- 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));