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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Mar 4, 2020
Merged

BoolQueryBuilder uses ObjectParser #52880

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
278daaa
BoolQueryBuilder uses ObjectParser
romseygeek Feb 27, 2020
ec59f97
msm can be string or number
romseygeek Feb 27, 2020
f0f9b15
assert on wrapped exception as well
romseygeek Mar 2, 2020
02dbd64
Merge remote-tracking branch 'origin/master' into refactor/boolqueryp…
romseygeek Mar 2, 2020
ba4f065
Merge branch 'master' into refactor/boolqueryparser
elasticmachine Mar 2, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
124 changes: 28 additions & 96 deletions server/src/main/java/org/elasticsearch/index/query/BoolQueryBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import org.elasticsearch.common.io.stream.StreamInput;
import org.elasticsearch.common.io.stream.StreamOutput;
import org.elasticsearch.common.lucene.search.Queries;
import org.elasticsearch.common.xcontent.ObjectParser;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.common.xcontent.XContentParser;

Expand All @@ -51,13 +52,11 @@ public class BoolQueryBuilder extends AbstractQueryBuilder<BoolQueryBuilder> {

public static final boolean ADJUST_PURE_NEGATIVE_DEFAULT = true;

private static final String MUSTNOT = "mustNot";
private static final String MUST_NOT = "must_not";
private static final String FILTER = "filter";
private static final String SHOULD = "should";
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?
Copy link
Member

Choose a reason for hiding this comment

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

I think so!

Copy link
Contributor Author

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.

private static final ParseField MUST_NOT = new ParseField("must_not");
private static final ParseField FILTER = new ParseField("filter");
private static final ParseField SHOULD = new ParseField("should");
private static final ParseField MUST = new ParseField("must");
private static final ParseField MINIMUM_SHOULD_MATCH = new ParseField("minimum_should_match");
private static final ParseField ADJUST_PURE_NEGATIVE = new ParseField("adjust_pure_negative");

Expand Down Expand Up @@ -260,106 +259,39 @@ protected void doXContent(XContentBuilder builder, Params params) throws IOExcep
builder.endObject();
}

private static void doXArrayContent(String field, List<QueryBuilder> clauses, XContentBuilder builder, Params params)
private static void doXArrayContent(ParseField field, List<QueryBuilder> clauses, XContentBuilder builder, Params params)
throws IOException {
if (clauses.isEmpty()) {
return;
}
builder.startArray(field);
builder.startArray(field.getPreferredName());
for (QueryBuilder clause : clauses) {
clause.toXContent(builder, params);
}
builder.endArray();
}

private static final ObjectParser<BoolQueryBuilder, Void> PARSER = new ObjectParser<>("bool", BoolQueryBuilder::new);
static {
PARSER.declareObjectArray((builder, clauses) -> clauses.forEach(builder::must), (p, c) -> parseInnerQueryBuilder(p),
MUST);
PARSER.declareObjectArray((builder, clauses) -> clauses.forEach(builder::should), (p, c) -> parseInnerQueryBuilder(p),
SHOULD);
PARSER.declareObjectArray((builder, clauses) -> clauses.forEach(builder::mustNot), (p, c) -> parseInnerQueryBuilder(p),
MUST_NOT);
PARSER.declareObjectArray((builder, clauses) -> clauses.forEach(builder::mustNot), (p, c) -> parseInnerQueryBuilder(p),
MUSTNOT); // TODO should we deprecate this version?
PARSER.declareObjectArray((builder, clauses) -> clauses.forEach(builder::filter), (p, c) -> parseInnerQueryBuilder(p),
FILTER);
PARSER.declareBoolean(BoolQueryBuilder::adjustPureNegative, ADJUST_PURE_NEGATIVE);
PARSER.declareField(BoolQueryBuilder::minimumShouldMatch, (p, c) -> p.text(),
MINIMUM_SHOULD_MATCH, ObjectParser.ValueType.VALUE);
PARSER.declareString(BoolQueryBuilder::queryName, NAME_FIELD);
PARSER.declareFloat(BoolQueryBuilder::boost, BOOST_FIELD);
}

public static BoolQueryBuilder fromXContent(XContentParser parser) throws IOException, ParsingException {
boolean adjustPureNegative = BoolQueryBuilder.ADJUST_PURE_NEGATIVE_DEFAULT;
float boost = DEFAULT_BOOST;
String minimumShouldMatch = null;

final List<QueryBuilder> mustClauses = new ArrayList<>();
final List<QueryBuilder> mustNotClauses = new ArrayList<>();
final List<QueryBuilder> shouldClauses = new ArrayList<>();
final List<QueryBuilder> filterClauses = new ArrayList<>();
String queryName = null;

String currentFieldName = null;
XContentParser.Token token;
while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) {
if (token == XContentParser.Token.FIELD_NAME) {
currentFieldName = parser.currentName();
} else if (token == XContentParser.Token.START_OBJECT) {
switch (currentFieldName) {
case MUST:
mustClauses.add(parseInnerQueryBuilder(parser));
break;
case SHOULD:
shouldClauses.add(parseInnerQueryBuilder(parser));
break;
case FILTER:
filterClauses.add(parseInnerQueryBuilder(parser));
break;
case MUST_NOT:
case MUSTNOT:
mustNotClauses.add(parseInnerQueryBuilder(parser));
break;
default:
throw new ParsingException(parser.getTokenLocation(), "[bool] query does not support [" + currentFieldName + "]");
}
} else if (token == XContentParser.Token.START_ARRAY) {
while ((token = parser.nextToken()) != XContentParser.Token.END_ARRAY) {
switch (currentFieldName) {
case MUST:
mustClauses.add(parseInnerQueryBuilder(parser));
break;
case SHOULD:
shouldClauses.add(parseInnerQueryBuilder(parser));
break;
case FILTER:
filterClauses.add(parseInnerQueryBuilder(parser));
break;
case MUST_NOT:
case MUSTNOT:
mustNotClauses.add(parseInnerQueryBuilder(parser));
break;
default:
throw new ParsingException(parser.getTokenLocation(), "bool query does not support [" + currentFieldName + "]");
}
}
} else if (token.isValue()) {
if (DISABLE_COORD_FIELD.match(currentFieldName, parser.getDeprecationHandler())) {
// ignore
} else if (MINIMUM_SHOULD_MATCH.match(currentFieldName, parser.getDeprecationHandler())) {
minimumShouldMatch = parser.textOrNull();
} else if (BOOST_FIELD.match(currentFieldName, parser.getDeprecationHandler())) {
boost = parser.floatValue();
} else if (ADJUST_PURE_NEGATIVE.match(currentFieldName, parser.getDeprecationHandler())) {
adjustPureNegative = parser.booleanValue();
} else if (NAME_FIELD.match(currentFieldName, parser.getDeprecationHandler())) {
queryName = parser.text();
} else {
throw new ParsingException(parser.getTokenLocation(), "[bool] query does not support [" + currentFieldName + "]");
}
}
}
BoolQueryBuilder boolQuery = new BoolQueryBuilder();
for (QueryBuilder queryBuilder : mustClauses) {
boolQuery.must(queryBuilder);
}
for (QueryBuilder queryBuilder : mustNotClauses) {
boolQuery.mustNot(queryBuilder);
}
for (QueryBuilder queryBuilder : shouldClauses) {
boolQuery.should(queryBuilder);
}
for (QueryBuilder queryBuilder : filterClauses) {
boolQuery.filter(queryBuilder);
}
boolQuery.boost(boost);
boolQuery.adjustPureNegative(adjustPureNegative);
boolQuery.minimumShouldMatch(minimumShouldMatch);
boolQuery.queryName(queryName);
return boolQuery;
return PARSER.parse(parser, null);
}

@Override
Expand Down
16 changes: 13 additions & 3 deletions server/src/test/java/org/elasticsearch/index/query/BoolQueryBuilderTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,9 @@
import org.apache.lucene.search.MatchAllDocsQuery;
import org.apache.lucene.search.MatchNoDocsQuery;
import org.apache.lucene.search.Query;
import org.elasticsearch.common.ParsingException;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.common.xcontent.XContentFactory;
import org.elasticsearch.common.xcontent.XContentParseException;
import org.elasticsearch.common.xcontent.XContentParser;
import org.elasticsearch.common.xcontent.XContentType;
import org.elasticsearch.test.AbstractQueryTestCase;
Expand All @@ -41,6 +41,7 @@

import static org.elasticsearch.index.query.QueryBuilders.boolQuery;
import static org.elasticsearch.index.query.QueryBuilders.termQuery;
import static org.hamcrest.CoreMatchers.containsString;
import static org.hamcrest.CoreMatchers.equalTo;
import static org.hamcrest.CoreMatchers.instanceOf;

Expand Down Expand Up @@ -277,14 +278,23 @@ public void testFromJson() throws IOException {
assertEquals(query, "kimchy", ((TermQueryBuilder)queryBuilder.must().get(0)).value());
}

public void testMinimumShouldMatchNumber() throws IOException {
String query = "{\"bool\" : {\"must\" : { \"term\" : { \"field\" : \"value\" } }, \"minimum_should_match\" : 1 } }";
BoolQueryBuilder builder = (BoolQueryBuilder) parseQuery(query);
assertEquals("1", builder.minimumShouldMatch());
}

/**
* test that unknown query names in the clauses throw an error
*/
public void testUnknownQueryName() throws IOException {
String query = "{\"bool\" : {\"must\" : { \"unknown_query\" : { } } } }";

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());
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

++ I pushed f0f9b15

Throwable e = ex.getCause();
assertThat(e.getMessage(), containsString("unknown query [unknown_query]"));

}

public void testRewrite() throws IOException {
Expand Down