Skip to content

Commit c51cfbb

Browse files
cbueschercbuescher
committed
Query Refactoring: Make sure that parsing nested queries always returns an query builder
Currently parsing inner queries can return `null` which leads to unnecessary complicated null checks further up the query hierarchy. By introducing a special EmptyQueryBuilder that can be used to signal that this query clause should be ignored upstream where possible, we can avoid additional null checks in parent query builders and still allow for this query to be ignored when creating the lucene query later. This new builder returns `null` when calling its `toQuery()` method, so at this point we still need to handle it, but having an explicit object for the intermediate query representation makes it easier to differentiate between cases where inner query clauses are empty (legal) or are not set at all (illegal). Also added check for empty inner builder list to BoolQueryBuilder and OrQueryBuilder. Removed setters for positive/negatice query in favour of constructor with mandatory non-null arguments in BoostingQueryBuilder. Closes #11915
1 parent 6353063 commit c51cfbb

34 files changed

+338
-289
lines changed

‎core/src/main/java/org/elasticsearch/index/query/AbstractQueryBuilder.java‎

Lines changed: 0 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -233,22 +233,6 @@ protected static Collection<Query> toQueries(Collection<QueryBuilder> queryBuild
233233
return queries;
234234
}
235235

236-
/**
237-
* Utility method that converts inner query builders to xContent and
238-
* checks for null values, rendering out empty object in this case.
239-
*/
240-
protected static void doXContentInnerBuilder(XContentBuilder xContentBuilder, String fieldName,
241-
QueryBuilder queryBuilder, Params params) throws IOException {
242-
xContentBuilder.field(fieldName);
243-
if (queryBuilder != null) {
244-
queryBuilder.toXContent(xContentBuilder, params);
245-
} else {
246-
// we output an empty object, QueryParseContext#parseInnerQueryBuilder will parse this back to `null` value
247-
xContentBuilder.startObject();
248-
xContentBuilder.endObject();
249-
}
250-
}
251-
252236
protected static QueryValidationException validateInnerQueries(List<QueryBuilder> queryBuilders, QueryValidationException initialValidationException) {
253237
QueryValidationException validationException = initialValidationException;
254238
for (QueryBuilder queryBuilder : queryBuilders) {

‎core/src/main/java/org/elasticsearch/index/query/AndQueryBuilder.java‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,10 @@ protected Query doToQuery(QueryParseContext parseContext) throws IOException {
9898
query.add(innerQuery, Occur.MUST);
9999
}
100100
}
101+
if (query.clauses().isEmpty()) {
102+
// no inner lucene query exists, ignore upstream
103+
return null;
104+
}
101105
return query;
102106
}
103107

‎core/src/main/java/org/elasticsearch/index/query/AndQueryParser.java‎

Lines changed: 3 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -54,9 +54,7 @@ public QueryBuilder fromXContent(QueryParseContext parseContext) throws IOExcept
5454
while ((token = parser.nextToken()) != XContentParser.Token.END_ARRAY) {
5555
queriesFound = true;
5656
QueryBuilder filter = parseContext.parseInnerFilterToQueryBuilder();
57-
if (filter != null) {
58-
queries.add(filter);
59-
}
57+
queries.add(filter);
6058
}
6159
} else {
6260
while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) {
@@ -69,17 +67,13 @@ public QueryBuilder fromXContent(QueryParseContext parseContext) throws IOExcept
6967
queriesFound = true;
7068
while ((token = parser.nextToken()) != XContentParser.Token.END_ARRAY) {
7169
QueryBuilder filter = parseContext.parseInnerFilterToQueryBuilder();
72-
if (filter != null) {
73-
queries.add(filter);
74-
}
70+
queries.add(filter);
7571
}
7672
} else {
7773
queriesFound = true;
7874
while ((token = parser.nextToken()) != XContentParser.Token.END_ARRAY) {
7975
QueryBuilder filter = parseContext.parseInnerFilterToQueryBuilder();
80-
if (filter != null) {
81-
queries.add(filter);
82-
}
76+
queries.add(filter);
8377
}
8478
}
8579
} else if (token.isValue()) {

‎core/src/main/java/org/elasticsearch/index/query/BoolQueryBuilder.java‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -255,16 +255,16 @@ public String getName() {
255255

256256
@Override
257257
protected Query doToQuery(QueryParseContext parseContext) throws IOException {
258-
if (!hasClauses()) {
259-
return new MatchAllDocsQuery();
260-
}
261-
262258
BooleanQuery booleanQuery = new BooleanQuery(disableCoord);
263259
addBooleanClauses(parseContext, booleanQuery, mustClauses, BooleanClause.Occur.MUST);
264260
addBooleanClauses(parseContext, booleanQuery, mustNotClauses, BooleanClause.Occur.MUST_NOT);
265261
addBooleanClauses(parseContext, booleanQuery, shouldClauses, BooleanClause.Occur.SHOULD);
266262
addBooleanClauses(parseContext, booleanQuery, filterClauses, BooleanClause.Occur.FILTER);
267263

264+
if (booleanQuery.clauses().isEmpty()) {
265+
return new MatchAllDocsQuery();
266+
}
267+
268268
Queries.applyMinimumShouldMatch(booleanQuery, minimumShouldMatch);
269269
return adjustPureNegative ? fixNegativeQueryIfNeeded(booleanQuery) : booleanQuery;
270270
}

‎core/src/main/java/org/elasticsearch/index/query/BoolQueryParser.java‎

Lines changed: 12 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -71,31 +71,26 @@ public QueryBuilder fromXContent(QueryParseContext parseContext) throws IOExcept
7171
switch (currentFieldName) {
7272
case "must":
7373
query = parseContext.parseInnerQueryBuilder();
74-
if (query != null) {
75-
mustClauses.add(query);
76-
}
74+
mustClauses.add(query);
7775
break;
7876
case "should":
7977
query = parseContext.parseInnerQueryBuilder();
80-
if (query != null) {
81-
shouldClauses.add(query);
78+
shouldClauses.add(query);
79+
// EmptyQueryBuilder does not add lucene query later, skip setting minuminShouldMatch
80+
if (query != EmptyQueryBuilder.PROTOTYPE) {
8281
if (parseContext.isFilter() && minimumShouldMatch == null) {
8382
minimumShouldMatch = "1";
8483
}
8584
}
8685
break;
8786
case "filter":
8887
query = parseContext.parseInnerFilterToQueryBuilder();
89-
if (query != null) {
90-
filterClauses.add(query);
91-
}
88+
filterClauses.add(query);
9289
break;
9390
case "must_not":
9491
case "mustNot":
9592
query = parseContext.parseInnerFilterToQueryBuilder();
96-
if (query != null) {
97-
mustNotClauses.add(query);
98-
}
93+
mustNotClauses.add(query);
9994
break;
10095
default:
10196
throw new QueryParsingException(parseContext, "[bool] query does not support [" + currentFieldName + "]");
@@ -105,31 +100,26 @@ public QueryBuilder fromXContent(QueryParseContext parseContext) throws IOExcept
105100
switch (currentFieldName) {
106101
case "must":
107102
query = parseContext.parseInnerQueryBuilder();
108-
if (query != null) {
109-
mustClauses.add(query);
110-
}
103+
mustClauses.add(query);
111104
break;
112105
case "should":
113106
query = parseContext.parseInnerQueryBuilder();
114-
if (query != null) {
115-
shouldClauses.add(query);
107+
shouldClauses.add(query);
108+
// EmptyQueryBuilder does not add lucene query later, skip setting minuminShouldMatch
109+
if (query != EmptyQueryBuilder.PROTOTYPE) {
116110
if (parseContext.isFilter() && minimumShouldMatch == null) {
117111
minimumShouldMatch = "1";
118112
}
119113
}
120114
break;
121115
case "filter":
122116
query = parseContext.parseInnerFilterToQueryBuilder();
123-
if (query != null) {
124-
filterClauses.add(query);
125-
}
117+
filterClauses.add(query);
126118
break;
127119
case "must_not":
128120
case "mustNot":
129121
query = parseContext.parseInnerFilterToQueryBuilder();
130-
if (query != null) {
131-
mustNotClauses.add(query);
132-
}
122+
mustNotClauses.add(query);
133123
break;
134124
default:
135125
throw new QueryParsingException(parseContext, "bool query does not support [" + currentFieldName + "]");

‎core/src/main/java/org/elasticsearch/index/query/BoostingQueryBuilder.java‎

Lines changed: 22 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -44,23 +44,31 @@ public class BoostingQueryBuilder extends AbstractQueryBuilder<BoostingQueryBuil
4444

4545
public static final String NAME = "boosting";
4646

47-
private QueryBuilder positiveQuery;
47+
private final QueryBuilder positiveQuery;
4848

49-
private QueryBuilder negativeQuery;
49+
private final QueryBuilder negativeQuery;
5050

5151
private float negativeBoost = -1;
5252

5353
static final BoostingQueryBuilder PROTOTYPE = new BoostingQueryBuilder();
5454

55-
public BoostingQueryBuilder() {
55+
/**
56+
* this constructor only used for prototype
57+
*/
58+
private BoostingQueryBuilder() {
59+
this.positiveQuery = null;
60+
this.negativeQuery = null;
5661
}
5762

5863
/**
59-
* Add the positive query for this boosting query.
64+
* Create a new {@link BoostingQueryBuilder}
65+
*
66+
* @param positiveQuery the positive query for this boosting query.
67+
* @param negativeQuery the negative query for this boosting query.
6068
*/
61-
public BoostingQueryBuilder positive(QueryBuilder positiveQuery) {
62-
this.positiveQuery = positiveQuery;
63-
return this;
69+
public BoostingQueryBuilder(QueryBuilder positiveQuery, QueryBuilder negativeQuery) {
70+
this.positiveQuery = Objects.requireNonNull(positiveQuery);
71+
this.negativeQuery = Objects.requireNonNull(negativeQuery);
6472
}
6573

6674
/**
@@ -70,14 +78,6 @@ public QueryBuilder positive() {
7078
return this.positiveQuery;
7179
}
7280

73-
/**
74-
* Add the negative query for this boosting query.
75-
*/
76-
public BoostingQueryBuilder negative(QueryBuilder negativeQuery) {
77-
this.negativeQuery = negativeQuery;
78-
return this;
79-
}
80-
8181
/**
8282
* Get the negative query for this boosting query.
8383
*/
@@ -103,8 +103,10 @@ public float negativeBoost() {
103103
@Override
104104
protected void doXContent(XContentBuilder builder, Params params) throws IOException {
105105
builder.startObject(NAME);
106-
doXContentInnerBuilder(builder, "positive", positiveQuery, params);
107-
doXContentInnerBuilder(builder, "negative", negativeQuery, params);
106+
builder.field("positive");
107+
positiveQuery.toXContent(builder, params);
108+
builder.field("negative");
109+
negativeQuery.toXContent(builder, params);
108110
builder.field("negative_boost", negativeBoost);
109111
printBoostAndQueryName(builder);
110112
builder.endObject();
@@ -128,13 +130,10 @@ public String getName() {
128130

129131
@Override
130132
protected Query doToQuery(QueryParseContext parseContext) throws IOException {
131-
// make upstream queries ignore this query by returning `null`
132-
// if either inner query builder is null or returns null-Query
133-
if (positiveQuery == null || negativeQuery == null) {
134-
return null;
135-
}
136133
Query positive = positiveQuery.toQuery(parseContext);
137134
Query negative = negativeQuery.toQuery(parseContext);
135+
// make upstream queries ignore this query by returning `null`
136+
// if either inner query builder returns null
138137
if (positive == null || negative == null) {
139138
return null;
140139
}
@@ -158,9 +157,7 @@ protected boolean doEquals(BoostingQueryBuilder other) {
158157
protected BoostingQueryBuilder doReadFrom(StreamInput in) throws IOException {
159158
QueryBuilder positiveQuery = in.readNamedWriteable();
160159
QueryBuilder negativeQuery = in.readNamedWriteable();
161-
BoostingQueryBuilder boostingQuery = new BoostingQueryBuilder();
162-
boostingQuery.positive(positiveQuery);
163-
boostingQuery.negative(negativeQuery);
160+
BoostingQueryBuilder boostingQuery = new BoostingQueryBuilder(positiveQuery, negativeQuery);
164161
boostingQuery.negativeBoost = in.readFloat();
165162
return boostingQuery;
166163
}

‎core/src/main/java/org/elasticsearch/index/query/BoostingQueryParser.java‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -88,9 +88,7 @@ public QueryBuilder fromXContent(QueryParseContext parseContext) throws IOExcept
8888
throw new QueryParsingException(parseContext, "[boosting] query requires 'negative_boost' to be set to be a positive value'");
8989
}
9090

91-
BoostingQueryBuilder boostingQuery = new BoostingQueryBuilder();
92-
boostingQuery.positive(positiveQuery);
93-
boostingQuery.negative(negativeQuery);
91+
BoostingQueryBuilder boostingQuery = new BoostingQueryBuilder(positiveQuery, negativeQuery);
9492
boostingQuery.negativeBoost(negativeBoost);
9593
boostingQuery.boost(boost);
9694
boostingQuery.queryName(queryName);

‎core/src/main/java/org/elasticsearch/index/query/ConstantScoreQueryBuilder.java‎

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,12 @@ public class ConstantScoreQueryBuilder extends AbstractQueryBuilder<ConstantScor
3838

3939
private final QueryBuilder filterBuilder;
4040

41-
static final ConstantScoreQueryBuilder PROTOTYPE = new ConstantScoreQueryBuilder(null);
41+
static final ConstantScoreQueryBuilder PROTOTYPE = new ConstantScoreQueryBuilder();
42+
43+
// only used for prototype
44+
private ConstantScoreQueryBuilder() {
45+
this.filterBuilder = null;
46+
}
4247

4348
/**
4449
* A query that wraps another query and simply returns a constant score equal to the
@@ -47,7 +52,7 @@ public class ConstantScoreQueryBuilder extends AbstractQueryBuilder<ConstantScor
4752
* @param filterBuilder The query to wrap in a constant score query
4853
*/
4954
public ConstantScoreQueryBuilder(QueryBuilder filterBuilder) {
50-
this.filterBuilder = filterBuilder;
55+
this.filterBuilder = Objects.requireNonNull(filterBuilder);
5156
}
5257

5358
/**
@@ -60,24 +65,19 @@ public QueryBuilder query() {
6065
@Override
6166
protected void doXContent(XContentBuilder builder, Params params) throws IOException {
6267
builder.startObject(NAME);
63-
doXContentInnerBuilder(builder, "filter", filterBuilder, params);
68+
builder.field("filter");
69+
filterBuilder.toXContent(builder, params);
6470
printBoostAndQueryName(builder);
6571
builder.endObject();
6672
}
6773

6874
@Override
6975
protected Query doToQuery(QueryParseContext parseContext) throws IOException {
70-
// current DSL allows empty inner filter clauses, we ignore them
71-
if (filterBuilder == null) {
72-
return null;
73-
}
74-
7576
Query innerFilter = filterBuilder.toQuery(parseContext);
7677
if (innerFilter == null ) {
7778
// return null so that parent queries (e.g. bool) also ignore this
7879
return null;
7980
}
80-
8181
return new ConstantScoreQuery(filterBuilder.toQuery(parseContext));
8282
}
8383

‎core/src/main/java/org/elasticsearch/index/query/DisMaxQueryParser.java‎

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -62,9 +62,7 @@ public QueryBuilder fromXContent(QueryParseContext parseContext) throws IOExcept
6262
if ("queries".equals(currentFieldName)) {
6363
queriesFound = true;
6464
QueryBuilder query = parseContext.parseInnerQueryBuilder();
65-
if (query != null) {
66-
queries.add(query);
67-
}
65+
queries.add(query);
6866
} else {
6967
throw new QueryParsingException(parseContext, "[dis_max] query does not support [" + currentFieldName + "]");
7068
}
@@ -73,9 +71,7 @@ public QueryBuilder fromXContent(QueryParseContext parseContext) throws IOExcept
7371
queriesFound = true;
7472
while (token != XContentParser.Token.END_ARRAY) {
7573
QueryBuilder query = parseContext.parseInnerQueryBuilder();
76-
if (query != null) {
77-
queries.add(query);
78-
}
74+
queries.add(query);
7975
token = parser.nextToken();
8076
}
8177
} else {

0 commit comments

Comments
 (0)