Skip to content

SuggestionBuilder doesn't need to extend ToXContentToBytes #22280

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

Merged

Conversation

cbuescher
Copy link
Member

This changes SuggestionBuilder from extending ToXContentToBytes to implementing the ToXContent interface only. The former could lead to unexpected behaviour when
trying to display instances of the object, since the inherited toString() method would create an error message because the SuggestionBuilders toXContent() methods only render json fragments (see #22264 as an example).

This changes the class from extending the abstract class to implementing the
ToXContent interface only. The former could lead to unexpected behaviour when
trying to display the object, since the "toString()" method inherited from
ToXContentToBytes would create an error message because the SuggestionBuilders
toXContent() methods don't render complete json objects.
Copy link
Member

@javanna javanna left a comment

Choose a reason for hiding this comment

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

LGTM

@cbuescher cbuescher merged commit bc22c86 into elastic:master Dec 20, 2016
@ndimiduk
Copy link

ndimiduk commented May 4, 2017

I'm seeing the same behavior with AggregationBuilder#toString() for a variety of subtypes, using 5.1.2. Has this been fixed across the board in v5.2.0?

@cbuescher
Copy link
Member Author

@ndimiduk thanks for your update on this, this PR only relates to SuggestionBuilder so if there are any issues with AggregationBuilder#toString() they might still be there. Would you mind opening a separate issue with a complete reproduction of your problem and which subtypes your are seeing this with? Since AggregationBuilders, like Suggestionbuilder, might only render fragments of the search request but currently relies on ToXContentToBytes#toString() to do this, there might still be issues.

@yuthura
Copy link

yuthura commented Jul 11, 2017

@cbuescher I just ran into this problem with a DateHistogramAggregationBuilder and created a new issue for it: #25648.

@howardtanwcm
Copy link

Hi, can you merge this fix to v2.4?

@cbuescher
Copy link
Member Author

can you merge this fix to v2.4?

Sorry, that version is long outdated and won't have any future releases, therefore it makes no sense merging anything there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants