Skip to content

Commit

Permalink
BoolQueryBuilder uses ObjectParser (#52880)
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@romseygeek
romseygeek authored Mar 4, 2020
1 parent 2930b32 commit 0edb1d3
Show file tree
Hide file tree
Showing 2 changed files with 41 additions and 99 deletions.
124 changes: 28 additions & 96 deletions server/src/main/java/org/elasticsearch/index/query/BoolQueryBuilder.java
View file
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?
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
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());
Throwable e = ex.getCause();
assertThat(e.getMessage(), containsString("unknown query [unknown_query]"));

}

public void testRewrite() throws IOException {
Expand Down

0 comments on commit 0edb1d3

Please sign in to comment.