Skip to content

Commit

Permalink
[Backport 2.x] Fix from and size parameter can be negative when searc…
Browse files Browse the repository at this point in the history
…hing (#13047) (#13361)

* Fix from and size parameter can be negative when searching (#13047)

* Fix from and size parameter can be negative when searching

Signed-off-by: Gao Binlong <[email protected]>

* Add changelog

Signed-off-by: Gao Binlong <[email protected]>

* fix test failure

Signed-off-by: Gao Binlong <[email protected]>

* Fix test failure

Signed-off-by: Gao Binlong <[email protected]>

* Optimize the code

Signed-off-by: Gao Binlong <[email protected]>

---------

Signed-off-by: Gao Binlong <[email protected]>
(cherry picked from commit ab7e914)

* Update support version

Signed-off-by: Gao Binlong <[email protected]>

* Small change to rerun gradle check

Signed-off-by: Gao Binlong <[email protected]>

---------

Signed-off-by: Gao Binlong <[email protected]>
  • Loading branch information
gaobinlong authored Apr 25, 2024
1 parent 3d26a6f commit 4378edd
Show file tree
Hide file tree
Showing 6 changed files with 161 additions and 11 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down
Original file line number Diff line number Diff line change
@@ -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}
Original file line number Diff line number Diff line change
Expand Up @@ -516,7 +516,7 @@ public PointInTimeBuilder pointInTimeBuilder() {
}

/**
* The tye of search to execute.
* The type of search to execute.
*/
public SearchType searchType() {
return searchType;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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")) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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())) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand All @@ -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));
Expand Down

0 comments on commit 4378edd

Please sign in to comment.