Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds check for negative search request size #25397

Merged
merged 7 commits into from Jul 4, 2017
Merged

Adds check for negative search request size #25397

merged 7 commits into from Jul 4, 2017

Conversation

colings86
Copy link
Contributor

This change adds a check to SearchSourceBuilder to throw and exception if the size set on it is set to a negative value.

Closes #22530

@colings86 colings86 added :Search/Search Search-related issues that do not fall into other categories >bug review v5.5.0 v5.6.0 v6.0.0 labels Jun 26, 2017
@colings86 colings86 self-assigned this Jun 26, 2017
@colings86 colings86 requested a review from cbuescher June 26, 2017 11:45
Copy link
Member

@cbuescher cbuescher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@colings86 thanks, this change makes a lot of sence. I left two smallish suggestions, but my main concern merging this to 5.x without marking it as breaking. Currently users who use "size" : -1 in their code will get the default of 10 hits, with this change we would error both on REST and on Java API layer.
I think this is sufficient to only merge this change to master and I would suggest adding some notes about this in the changes/migration docs or anywhere else that might be appropriate. If we need this on 5.x as well I'd ask to include -1 as valid argument for bwc that then default to 10 hits.

@@ -344,6 +344,9 @@ public int from() {
* The number of search hits to return. Defaults to <tt>10</tt>.
*/
public SearchSourceBuilder size(int size) {
if (size < 0) {
throw new IllegalArgumentException("[size] parameter cannot be negative");
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can set the size to the default of 10, that's what SearchService#createContext defaults to later on anyway if it finds the current -1 value. I just find it easier to reason about if the value is set to the default in this case.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's best to keep the default as-is here and let it be handled when creating the context as today so that we can distinguish between set and set from default.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, the set/unset distinction makes sense e.g. when using the builder on the client side, otherwise we would always render the size even if not set. What makes me not like this is the fact that we reject "-1" as illegal but still use it as a "legal" value meaning something internally. To me it feels a bit weird when you are just looking at this class.

@@ -365,6 +365,13 @@ public void testNegativeFromErrors() {
assertEquals("[from] parameter cannot be negative", expected.getMessage());
}

public void testNegativeSizeErrors() {
IllegalArgumentException expected = expectThrows(IllegalArgumentException.class, () -> new SearchSourceBuilder().size(-2));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: maybe you could test -1 and another random negative int instead?

@jasontedor
Copy link
Member

I think it's okay to make the change in 5.x, using -1 as the default is completely undocumented.

Copy link
Member

@jasontedor jasontedor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left one suggestion.

@@ -344,6 +344,9 @@ public int from() {
* The number of search hits to return. Defaults to <tt>10</tt>.
*/
public SearchSourceBuilder size(int size) {
if (size < 0) {
throw new IllegalArgumentException("[size] parameter cannot be negative");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe include the illegal value?

@colings86
Copy link
Contributor Author

@jasontedor @cbuescher thanks for the reviews, the change has a few implications for reindex which I am in the process of resolving so I may ping you for review again after thats done

@cbuescher
Copy link
Member

using -1 as the default is completely undocumented

Nevertheless it will break your application if you currently use -1 for whatever reason. I'm not against changes like this, but I wonder if it is necessary to do this on a 5 version.

This change adds a check to `SearchSourceBuilder` to throw and exception if the size set on it is set to a negative value.

Closes #22530
@colings86
Copy link
Contributor Author

@cbuescher @jasontedor @nik9000 I pushed an update to this to address the problems from re-index. #24344 will also help this since it will decouple re-index's size from the search request's size

@colings86 colings86 requested a review from nik9000 July 3, 2017 12:41
@@ -52,7 +50,7 @@ protected void parseInternalRequest(Request internal, RestRequest restRequest,

SearchRequest searchRequest = internal.getSearchRequest();
int scrollSize = searchRequest.source().size();
searchRequest.source().size(SIZE_ALL_MATCHES);
// searchRequest.source().size(SIZE_ALL_MATCHES);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should just remove this line entirely.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well spotted, I had meant to remove this

@cbuescher
Copy link
Member

LGTM, left one smallish test suggestion above from the first review round that hasn't been adressed, but I'm good either way.

@colings86 colings86 merged commit 43efcff into elastic:master Jul 4, 2017
@colings86 colings86 deleted the fix/22530 branch July 4, 2017 09:51
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Jul 4, 2017
* master:
  [Analysis] Support normalizer in request param (elastic#24767)
  Remove deprecated IdsQueryBuilder constructor (elastic#25529)
  Adds check for negative search request size (elastic#25397)
  test: also inspect the upgrade api response to check whether the upgrade really ran
  [DOCS] restructure java clients docs pages (elastic#25517)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Search/Search Search-related issues that do not fall into other categories v6.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants