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

Throw exception in scroll requests using from #26235

Merged
merged 2 commits into from
Aug 21, 2017

Conversation

cbuescher
Copy link
Member

@cbuescher cbuescher commented Aug 16, 2017

According to the discussion in #9373, the from search parameter cannot really be used with scrolled searches.
This commit adds a check for this case to the SearchRequest#validate() method so we can reported it as an
error rather than silently ignoring it.

Closes #9373

The `from` search parameter cannot really be used in scrolled searches. This
commit adds a check for this case to the SearchRequest#validate() method so we
can reported it as an error rather than silently ignoring it.

Closes elastic#9373
Copy link
Contributor

@s1monw s1monw left a comment

Choose a reason for hiding this comment

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

LGTM - we should maybe add a note into the migration guide!

@cbuescher
Copy link
Member Author

@s1monw thanks, I also had to adapt a failing rest test that used "from" in a scroll. Maybe you or @nik9000 (who seems to have added it) can recheck that this change is okay.
@clintongormley do you think this should go into 6.0/6x as well? In that case, is it breaking, considering that we ignored the parameter already? I would put a note in the 5.0 migration docs in this case, otherwise I'd just add it to v7.

Copy link
Member

@nik9000 nik9000 left a comment

Choose a reason for hiding this comment

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

I'm cool with the change and the test change.

@cbuescher
Copy link
Member Author

@s1monw I'll merge this into 7.0 and add a migration note and add the breaking label in a separate PR for 6.0/6.x, is that okay with you?

@cbuescher cbuescher merged commit 4ff12c9 into elastic:master Aug 21, 2017
cbuescher added a commit that referenced this pull request Aug 21, 2017
The `from` search parameter cannot really be used in scrolled searches. This
commit adds a check for this case to the SearchRequest#validate() method so we
can reported it as an error rather than silently ignoring it.

Closes #9373
cbuescher added a commit that referenced this pull request Aug 21, 2017
The `from` search parameter cannot really be used in scrolled searches. This
commit adds a check for this case to the SearchRequest#validate() method so we
can reported it as an error rather than silently ignoring it.

Closes #9373
@lcawl lcawl removed the v6.1.0 label Dec 12, 2017
@clintongormley clintongormley added the :Search/Search Search-related issues that do not fall into other categories label Feb 14, 2018
hub-cap added a commit to hub-cap/elasticsearch that referenced this pull request Aug 28, 2019
This commit builds on elastic#26235, and adds the same validation to the
builder so that it will throw an exception when setting a scroll on a
request that has from set.

Relates elastic#9373
Closes elastic#44493
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>breaking >enhancement :Search/Search Search-related issues that do not fall into other categories v6.0.0-beta2 v7.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants