Skip to content

Commit ab7e914

Browse files
gaobinlonggaobinlong
authored
Fix from and size parameter can be negative when searching (opensearch-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>
1 parent f41db2c commit ab7e914

File tree

6 files changed

+161
-11
lines changed

6 files changed

+161
-11
lines changed

‎CHANGELOG.md

+1
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
6161
- Fix issue with feature flags where default value may not be honored ([#12849](https://github.com/opensearch-project/OpenSearch/pull/12849))
6262
- Fix UOE While building Exists query for nested search_as_you_type field ([#12048](https://github.com/opensearch-project/OpenSearch/pull/12048))
6363
- 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))
64+
- Fix from and size parameter can be negative when searching ([#13047](https://github.com/opensearch-project/OpenSearch/pull/13047))
6465
- Enabled mockTelemetryPlugin for IT and fixed OOM issues ([#13054](https://github.com/opensearch-project/OpenSearch/pull/13054))
6566
- Fix implement mark() and markSupported() in class FilterStreamInput ([#13098](https://github.com/opensearch-project/OpenSearch/pull/13098))
6667
- Fix snapshot _status API to return correct status for partial snapshots ([#12812](https://github.com/opensearch-project/OpenSearch/pull/12812))

‎rest-api-spec/src/main/resources/rest-api-spec/test/search/360_from_and_size.yml

+113
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,113 @@
1+
setup:
2+
- do:
3+
indices.create:
4+
index: test_1
5+
- do:
6+
index:
7+
index: test_1
8+
id: 1
9+
body: { foo: bar }
10+
- do:
11+
index:
12+
index: test_1
13+
id: 2
14+
body: { foo: bar }
15+
- do:
16+
index:
17+
index: test_1
18+
id: 3
19+
body: { foo: bar }
20+
21+
- do:
22+
index:
23+
index: test_1
24+
id: 4
25+
body: { foo: bar }
26+
- do:
27+
indices.refresh:
28+
index: [test_1]
29+
30+
---
31+
teardown:
32+
- do:
33+
indices.delete:
34+
index: test_1
35+
ignore: 404
36+
37+
---
38+
"Throws exception if from or size query parameter is negative":
39+
- skip:
40+
version: " - 2.99.99"
41+
reason: "fixed in 3.0.0"
42+
- do:
43+
catch: '/\[from\] parameter cannot be negative, found \[-5\]/'
44+
search:
45+
index: test_1
46+
from: -5
47+
size: 10
48+
body:
49+
query:
50+
match:
51+
foo: bar
52+
53+
- do:
54+
catch: '/\[size\] parameter cannot be negative, found \[-1\]/'
55+
search:
56+
index: test_1
57+
from: 0
58+
size: -1
59+
body:
60+
query:
61+
match:
62+
foo: bar
63+
64+
- do:
65+
search:
66+
index: test_1
67+
from: 0
68+
size: 10
69+
body:
70+
query:
71+
match:
72+
foo: bar
73+
74+
- match: {hits.total.value: 4}
75+
76+
---
77+
"Throws exception if from or size request body parameter is negative":
78+
- skip:
79+
version: " - 2.99.99"
80+
reason: "fixed in 3.0.0"
81+
- do:
82+
catch: '/\[from\] parameter cannot be negative, found \[-5\]/'
83+
search:
84+
index: test_1
85+
body:
86+
from: -5
87+
size: 10
88+
query:
89+
match:
90+
foo: bar
91+
92+
- do:
93+
catch: '/\[size\] parameter cannot be negative, found \[-1\]/'
94+
search:
95+
index: test_1
96+
body:
97+
from: 0
98+
size: -1
99+
query:
100+
match:
101+
foo: bar
102+
103+
- do:
104+
search:
105+
index: test_1
106+
body:
107+
from: 0
108+
size: 10
109+
query:
110+
match:
111+
foo: bar
112+
113+
- match: {hits.total.value: 4}

‎server/src/main/java/org/opensearch/action/search/SearchRequest.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -497,7 +497,7 @@ public PointInTimeBuilder pointInTimeBuilder() {
497497
}
498498

499499
/**
500-
* The tye of search to execute.
500+
* The type of search to execute.
501501
*/
502502
public SearchType searchType() {
503503
return searchType;

‎server/src/main/java/org/opensearch/rest/action/search/RestSearchAction.java

+6-6
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,7 @@
5050
import org.opensearch.rest.action.RestCancellableNodeClient;
5151
import org.opensearch.rest.action.RestStatusToXContentListener;
5252
import org.opensearch.search.Scroll;
53+
import org.opensearch.search.SearchService;
5354
import org.opensearch.search.builder.SearchSourceBuilder;
5455
import org.opensearch.search.fetch.StoredFieldsContext;
5556
import org.opensearch.search.fetch.subphase.FetchSourceContext;
@@ -235,13 +236,12 @@ private static void parseSearchSource(final SearchSourceBuilder searchSourceBuil
235236
searchSourceBuilder.query(queryBuilder);
236237
}
237238

238-
int from = request.paramAsInt("from", -1);
239-
if (from != -1) {
240-
searchSourceBuilder.from(from);
239+
if (request.hasParam("from")) {
240+
searchSourceBuilder.from(request.paramAsInt("from", SearchService.DEFAULT_FROM));
241241
}
242-
int size = request.paramAsInt("size", -1);
243-
if (size != -1) {
244-
setSize.accept(size);
242+
243+
if (request.hasParam("size")) {
244+
setSize.accept(request.paramAsInt("size", SearchService.DEFAULT_SIZE));
245245
}
246246

247247
if (request.hasParam("explain")) {

‎server/src/main/java/org/opensearch/search/builder/SearchSourceBuilder.java

+3-3
Original file line numberDiff line numberDiff line change
@@ -418,7 +418,7 @@ public QueryBuilder postFilter() {
418418
*/
419419
public SearchSourceBuilder from(int from) {
420420
if (from < 0) {
421-
throw new IllegalArgumentException("[from] parameter cannot be negative");
421+
throw new IllegalArgumentException("[from] parameter cannot be negative, found [" + from + "]");
422422
}
423423
this.from = from;
424424
return this;
@@ -1215,9 +1215,9 @@ public void parseXContent(XContentParser parser, boolean checkTrailingTokens) th
12151215
currentFieldName = parser.currentName();
12161216
} else if (token.isValue()) {
12171217
if (FROM_FIELD.match(currentFieldName, parser.getDeprecationHandler())) {
1218-
from = parser.intValue();
1218+
from(parser.intValue());
12191219
} else if (SIZE_FIELD.match(currentFieldName, parser.getDeprecationHandler())) {
1220-
size = parser.intValue();
1220+
size(parser.intValue());
12211221
} else if (TIMEOUT_FIELD.match(currentFieldName, parser.getDeprecationHandler())) {
12221222
timeout = TimeValue.parseTimeValue(parser.text(), null, TIMEOUT_FIELD.getPreferredName());
12231223
} else if (TERMINATE_AFTER_FIELD.match(currentFieldName, parser.getDeprecationHandler())) {

‎server/src/test/java/org/opensearch/search/builder/SearchSourceBuilderTests.java

+37-1
Original file line numberDiff line numberDiff line change
@@ -571,7 +571,7 @@ public void testParseIndicesBoost() throws IOException {
571571

572572
public void testNegativeFromErrors() {
573573
IllegalArgumentException expected = expectThrows(IllegalArgumentException.class, () -> new SearchSourceBuilder().from(-2));
574-
assertEquals("[from] parameter cannot be negative", expected.getMessage());
574+
assertEquals("[from] parameter cannot be negative, found [-2]", expected.getMessage());
575575
}
576576

577577
public void testNegativeSizeErrors() {
@@ -582,6 +582,42 @@ public void testNegativeSizeErrors() {
582582
assertEquals("[size] parameter cannot be negative, found [-1]", expected.getMessage());
583583
}
584584

585+
public void testParseFromAndSize() throws IOException {
586+
int negativeFrom = randomIntBetween(-100, -1);
587+
String restContent = " { \"from\": \"" + negativeFrom + "\"}";
588+
try (XContentParser parser = createParser(JsonXContent.jsonXContent, restContent)) {
589+
IllegalArgumentException expected = expectThrows(
590+
IllegalArgumentException.class,
591+
() -> SearchSourceBuilder.fromXContent(parser)
592+
);
593+
assertEquals("[from] parameter cannot be negative, found [" + negativeFrom + "]", expected.getMessage());
594+
}
595+
596+
int validFrom = randomIntBetween(0, 100);
597+
restContent = " { \"from\": \"" + validFrom + "\"}";
598+
try (XContentParser parser = createParser(JsonXContent.jsonXContent, restContent)) {
599+
SearchSourceBuilder searchSourceBuilder = SearchSourceBuilder.fromXContent(parser);
600+
assertEquals(validFrom, searchSourceBuilder.from());
601+
}
602+
603+
int negativeSize = randomIntBetween(-100, -1);
604+
restContent = " { \"size\": \"" + negativeSize + "\"}";
605+
try (XContentParser parser = createParser(JsonXContent.jsonXContent, restContent)) {
606+
IllegalArgumentException expected = expectThrows(
607+
IllegalArgumentException.class,
608+
() -> SearchSourceBuilder.fromXContent(parser)
609+
);
610+
assertEquals("[size] parameter cannot be negative, found [" + negativeSize + "]", expected.getMessage());
611+
}
612+
613+
int validSize = randomIntBetween(0, 100);
614+
restContent = " { \"size\": \"" + validSize + "\"}";
615+
try (XContentParser parser = createParser(JsonXContent.jsonXContent, restContent)) {
616+
SearchSourceBuilder searchSourceBuilder = SearchSourceBuilder.fromXContent(parser);
617+
assertEquals(validSize, searchSourceBuilder.size());
618+
}
619+
}
620+
585621
private void assertIndicesBoostParseErrorMessage(String restContent, String expectedErrorMessage) throws IOException {
586622
try (XContentParser parser = createParser(JsonXContent.jsonXContent, restContent)) {
587623
ParsingException e = expectThrows(ParsingException.class, () -> SearchSourceBuilder.fromXContent(parser));

0 commit comments

Comments
 (0)