-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
BoolQueryBuilder uses ObjectParser #52880
BoolQueryBuilder uses ObjectParser #52880
Conversation
Pinging @elastic/es-search (:Search/Search) |
There's also a TODO here regarding |
private static final String MUST = "must"; | ||
private static final ParseField DISABLE_COORD_FIELD = new ParseField("disable_coord") | ||
.withAllDeprecated("disable_coord has been removed"); | ||
private static final ParseField MUSTNOT = new ParseField("mustNot"); // TODO deprecate? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'll do this in a followup to get more eyes on it.
ParsingException ex = expectThrows(ParsingException.class, () -> parseQuery(query)); | ||
assertEquals("unknown query [unknown_query]", ex.getMessage()); | ||
XContentParseException ex = expectThrows(XContentParseException.class, () -> parseQuery(query)); | ||
assertEquals("[1:41] [bool] failed to parse field [must]", ex.getMessage()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be worth asserting on the "inner" exception too. Or something. Because this message isn't nearly a nice as the other one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++ I pushed f0f9b15
@elasticmachine run elasticsearch-ci/2 |
1 similar comment
@elasticmachine run elasticsearch-ci/2 |
@elasticmachine update branch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This commit removes the hand-rolled x-content parsing logic from BoolQueryBuilder and instead uses an ObjectParser to handle parsing. It also removes the long-deprecated (since version 6) disable_coord parameter.
I know it's been deprecated since 6 but it seems bad practice to have removed it in a 7.x patch rather than waiting for 8.0. This change breaks working clients and prevents the server from being upgraded from 7.5.2 => 7.7.0. |
Your right. @romseygeek, I think we should have only applied the removal to master. I think it might be a bit late now though. |
Thanks, yes probably too late now. |
This commit removes the hand-rolled x-content parsing logic from BoolQueryBuilder
and instead uses an ObjectParser to handle parsing. It also removes the long-deprecated
(since version 6)
disable_coord
parameter.