Skip to content

Fix search scroll request with a plain text body#23183

Merged
jaymode merged 2 commits intoelastic:5.xfrom
jaymode:fix_scroll_plaintext
Feb 15, 2017
Merged

Fix search scroll request with a plain text body#23183
jaymode merged 2 commits intoelastic:5.xfrom
jaymode:fix_scroll_plaintext

Conversation

@jaymode
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

/cc @elastic/es-clients

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

@spalger
Copy link
Copy Markdown
Contributor

spalger commented Feb 15, 2017

Thanks!

@jaymode
Copy link
Copy Markdown
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
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
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