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

Fix search scroll request with a plain text body #23183

Merged
merged 2 commits into from
Feb 15, 2017

Conversation

jaymode
Copy link
Member

@jaymode jaymode commented Feb 15, 2017

The backport of #22691 caused plain text bodies with a scroll id to fail with an IllegalStateException as the wrong method was being called. This commit adds tests to ensure plain text bodies work and fixes the search scroll action so that it properly handles a request with a plain text body.

The backport of elastic#22691 caused plain text bodies with a scroll id to fail with an IllegalStateException as the wrong
method was being called. This commit adds tests to ensure plain text bodies work and fixes the search scroll action so
that it properly handles a request with a plain text body.
Copy link
Member

@danielmitterdorfer danielmitterdorfer left a comment

Choose a reason for hiding this comment

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

LGTM

@jaymode jaymode merged commit 1dc250e into elastic:5.x Feb 15, 2017
jaymode added a commit that referenced this pull request Feb 15, 2017
The backport of #22691 caused plain text bodies with a scroll id to fail with an IllegalStateException as the wrong
method was being called. This commit adds tests to ensure plain text bodies work and fixes the search scroll action so
that it properly handles a request with a plain text body.
@jaymode
Copy link
Member Author

jaymode commented Feb 15, 2017

@spalger fyi

@jaymode jaymode deleted the fix_scroll_plaintext branch February 15, 2017 14:18
@clintongormley
Copy link
Contributor

/cc @elastic/es-clients

@jaymode would be good to have a rest test for this

@spalger
Copy link
Contributor

spalger commented Feb 15, 2017

Thanks!

@jaymode
Copy link
Member Author

jaymode commented Feb 15, 2017

@clintongormley unfortunately the rest test framework does not support plain text bodies. It expects to be able to parse the body into a map which it cannot really do here, so I think adding a rest test is more trouble than it is worth

@javanna
Copy link
Member

javanna commented Feb 17, 2017

@jaymode you could add an integration test that sends a request using the low level REST client, rather than an ordinary yaml test. Extending ESRestTestCase should do it.

@jaymode
Copy link
Member Author

jaymode commented Feb 17, 2017

@clintongormley did you want the rest test so it could be consumed by the clients?

@clintongormley
Copy link
Contributor

@jaymode yes - eg the perl client wouldn't have sent a plain text header in that case, and i think most clients won't.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Core/Infra/REST API REST infrastructure and utilities v5.3.0 v5.4.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants