Check for negative "from" values in search request body #54953
Conversation
Today we already disallow negative values for the "from" parameter in the search API when it is set as a request parameter and setting it on the SearchSourceBuilder, but it is still parsed without complaint from a search body, leading to differing exceptions later. This PR changes this behavior to be the same regardless of setting the value directly, as url parameter or in the search body. Values of -1 are still allowed as the current default, meaning an unset value, in order not to break existing searches and templates using this. Closes elastic#54897
cbuescher
added
>bug
:Search/Search
|
Pinging @elastic/es-search (:Search/Search) |
|
@elasticmachine update branch |
|
@elasticmachine run elasticsearch-ci/2 |
| search: | ||
| rest_total_hits_as_int: true | ||
| index: test_1 | ||
| # value of -1 should be accepted as unset value |
There was a problem hiding this comment.
Can we try to address this ? For instance we could set the default value directly rather than delaying this to the creation of the search context ?
There was a problem hiding this comment.
I'd be open to forbid users setting from to "-1" entirely on 8.0 but since we currently accept it in the request body (and treat it as 0 later) I think we should allow it at least on 7.x to not break existing queries there. Every other negative value should alread throw a different exception, so thats fine I think. We can log a warning on 7.x when using "-1" to prepare for the change in 8.0 if thats okay?
There was a problem hiding this comment.
The rest spec states that the default value for from is 0 and size is 10 so I don't think users rely on us accepting -1 as a valid value. As you said we never output the value if it's unset so we imo we should forbid in 7.x.
| */ | ||
| public SearchSourceBuilder from(int from) { | ||
| if (from < 0) { | ||
| throw new IllegalArgumentException("[from] parameter cannot be negative"); | ||
| // accept -1 as the default unset value without error |
There was a problem hiding this comment.
Can we set the default early so that we can refuse any negative values ?
There was a problem hiding this comment.
I think for SearchSourceBuilder I would prefer to keep the notion of an "unset" value and interpret that in any way necessary later. I think this class should represent the user request body, e.g. in innerToXContent we only output values that are explicitely set by the user. Maybe @javanna has some preferences there as well?
There was a problem hiding this comment.
I'd agree if "unset" had different semantics depending on the context but we always set it to the default value.
We could forbid setting negative value but keep the initial value to -1 ?
|
@jimczi I left some answers to the questions you raised in your review. I'm leaning towards forbidding users setting |
|
@jimczi thanks for the review, I updated the PR to reject |
|
@elasticmachine update branch |
Today we already disallow negative values for the "from" parameter in the search
API when it is set as a request parameter and setting it on the
SearchSourceBuilder, but it is still parsed without complaint from a search
body, leading to different, confusing exceptions later on. This PR changes
this behavior to be the same regardless of setting the value directly, as url
parameter or in the search body. Values of -1 are still allowed as the current
default, meaning an unset value, in order not to break existing searches and
templates using this.
Closes #54897