Adds check for negative search request size#25397
Adds check for negative search request size#25397colings86 merged 7 commits intoelastic:masterfrom colings86:fix/22530
Conversation
cbuescher
left a comment
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
nit: maybe you could test -1 and another random negative int instead?
|
I think it's okay to make the change in 5.x, using -1 as the default is completely undocumented. |
There was a problem hiding this comment.
Maybe include the illegal value?
|
@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 |
Nevertheless it will break your application if you currently use |
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
|
@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 |
| SearchRequest searchRequest = internal.getSearchRequest(); | ||
| int scrollSize = searchRequest.source().size(); | ||
| searchRequest.source().size(SIZE_ALL_MATCHES); | ||
| // searchRequest.source().size(SIZE_ALL_MATCHES); |
There was a problem hiding this comment.
I think you should just remove this line entirely.
There was a problem hiding this comment.
Well spotted, I had meant to remove this
|
LGTM, left one smallish test suggestion above from the first review round that hasn't been adressed, but I'm good either way. |
* 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)
This change adds a check to
SearchSourceBuilderto throw and exception if the size set on it is set to a negative value.Closes #22530