From c51cfbb6d0729a1e6075db45b756ae96b7f9aeb1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20B=C3=BCscher?= Date: Fri, 26 Jun 2015 17:18:40 +0200 Subject: [PATCH] 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 --- .../index/query/AbstractQueryBuilder.java | 16 --- .../index/query/AndQueryBuilder.java | 4 + .../index/query/AndQueryParser.java | 12 +- .../index/query/BoolQueryBuilder.java | 8 +- .../index/query/BoolQueryParser.java | 34 ++---- .../index/query/BoostingQueryBuilder.java | 47 ++++---- .../index/query/BoostingQueryParser.java | 4 +- .../query/ConstantScoreQueryBuilder.java | 18 +-- .../index/query/DisMaxQueryParser.java | 8 +- .../index/query/EmptyQueryBuilder.java | 83 ++++++++++++++ .../index/query/FQueryFilterBuilder.java | 14 ++- .../index/query/FQueryFilterParser.java | 3 - .../query/FieldMaskingSpanQueryBuilder.java | 6 +- .../index/query/NotQueryBuilder.java | 14 ++- .../index/query/OrQueryBuilder.java | 4 + .../index/query/QueryBuilders.java | 4 +- .../index/query/QueryFilterBuilder.java | 14 ++- .../index/query/QueryParseContext.java | 2 +- .../indices/query/IndicesQueriesRegistry.java | 4 + .../index/query/AndQueryBuilderTest.java | 13 +++ .../index/query/BaseQueryTestCase.java | 16 ++- .../index/query/BoolQueryBuilderTest.java | 29 ++++- .../index/query/BoostingQueryBuilderTest.java | 103 ++++-------------- .../query/ConstantScoreQueryBuilderTest.java | 31 ++---- .../index/query/DisMaxQueryBuilderTest.java | 12 +- .../index/query/FQueryFilterBuilderTest.java | 15 ++- .../FieldMaskingSpanQueryBuilderTest.java | 12 +- .../index/query/NotQueryBuilderTest.java | 24 +--- .../index/query/OrQueryBuilderTest.java | 13 +++ .../index/query/QueryFilterBuilderTest.java | 36 +----- .../index/query/RandomQueryBuilder.java | 6 +- .../query/SimpleIndexQueryParserTests.java | 2 +- .../highlight/HighlighterSearchTests.java | 11 +- .../migrate_query_refactoring.asciidoc | 5 + 34 files changed, 338 insertions(+), 289 deletions(-) create mode 100644 core/src/main/java/org/elasticsearch/index/query/EmptyQueryBuilder.java diff --git a/core/src/main/java/org/elasticsearch/index/query/AbstractQueryBuilder.java b/core/src/main/java/org/elasticsearch/index/query/AbstractQueryBuilder.java index 3e2a6aebff8..82e5a4b7f9f 100644 --- a/core/src/main/java/org/elasticsearch/index/query/AbstractQueryBuilder.java +++ b/core/src/main/java/org/elasticsearch/index/query/AbstractQueryBuilder.java @@ -233,22 +233,6 @@ public abstract class AbstractQueryBuilder exte return queries; } - /** - * Utility method that converts inner query builders to xContent and - * checks for null values, rendering out empty object in this case. - */ - protected static void doXContentInnerBuilder(XContentBuilder xContentBuilder, String fieldName, - QueryBuilder queryBuilder, Params params) throws IOException { - xContentBuilder.field(fieldName); - if (queryBuilder != null) { - queryBuilder.toXContent(xContentBuilder, params); - } else { - // we output an empty object, QueryParseContext#parseInnerQueryBuilder will parse this back to `null` value - xContentBuilder.startObject(); - xContentBuilder.endObject(); - } - } - protected static QueryValidationException validateInnerQueries(List queryBuilders, QueryValidationException initialValidationException) { QueryValidationException validationException = initialValidationException; for (QueryBuilder queryBuilder : queryBuilders) { diff --git a/core/src/main/java/org/elasticsearch/index/query/AndQueryBuilder.java b/core/src/main/java/org/elasticsearch/index/query/AndQueryBuilder.java index 68a4799f1ac..bdcf3d565f0 100644 --- a/core/src/main/java/org/elasticsearch/index/query/AndQueryBuilder.java +++ b/core/src/main/java/org/elasticsearch/index/query/AndQueryBuilder.java @@ -98,6 +98,10 @@ public class AndQueryBuilder extends AbstractQueryBuilder { query.add(innerQuery, Occur.MUST); } } + if (query.clauses().isEmpty()) { + // no inner lucene query exists, ignore upstream + return null; + } return query; } diff --git a/core/src/main/java/org/elasticsearch/index/query/AndQueryParser.java b/core/src/main/java/org/elasticsearch/index/query/AndQueryParser.java index bad092c1d11..4f5c24dbefd 100644 --- a/core/src/main/java/org/elasticsearch/index/query/AndQueryParser.java +++ b/core/src/main/java/org/elasticsearch/index/query/AndQueryParser.java @@ -54,9 +54,7 @@ public class AndQueryParser extends BaseQueryParser { while ((token = parser.nextToken()) != XContentParser.Token.END_ARRAY) { queriesFound = true; QueryBuilder filter = parseContext.parseInnerFilterToQueryBuilder(); - if (filter != null) { - queries.add(filter); - } + queries.add(filter); } } else { while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) { @@ -69,17 +67,13 @@ public class AndQueryParser extends BaseQueryParser { queriesFound = true; while ((token = parser.nextToken()) != XContentParser.Token.END_ARRAY) { QueryBuilder filter = parseContext.parseInnerFilterToQueryBuilder(); - if (filter != null) { - queries.add(filter); - } + queries.add(filter); } } else { queriesFound = true; while ((token = parser.nextToken()) != XContentParser.Token.END_ARRAY) { QueryBuilder filter = parseContext.parseInnerFilterToQueryBuilder(); - if (filter != null) { - queries.add(filter); - } + queries.add(filter); } } } else if (token.isValue()) { diff --git a/core/src/main/java/org/elasticsearch/index/query/BoolQueryBuilder.java b/core/src/main/java/org/elasticsearch/index/query/BoolQueryBuilder.java index 1f604c23bbe..32b092dc48d 100644 --- a/core/src/main/java/org/elasticsearch/index/query/BoolQueryBuilder.java +++ b/core/src/main/java/org/elasticsearch/index/query/BoolQueryBuilder.java @@ -255,16 +255,16 @@ public class BoolQueryBuilder extends AbstractQueryBuilder { @Override protected Query doToQuery(QueryParseContext parseContext) throws IOException { - if (!hasClauses()) { - return new MatchAllDocsQuery(); - } - BooleanQuery booleanQuery = new BooleanQuery(disableCoord); addBooleanClauses(parseContext, booleanQuery, mustClauses, BooleanClause.Occur.MUST); addBooleanClauses(parseContext, booleanQuery, mustNotClauses, BooleanClause.Occur.MUST_NOT); addBooleanClauses(parseContext, booleanQuery, shouldClauses, BooleanClause.Occur.SHOULD); addBooleanClauses(parseContext, booleanQuery, filterClauses, BooleanClause.Occur.FILTER); + if (booleanQuery.clauses().isEmpty()) { + return new MatchAllDocsQuery(); + } + Queries.applyMinimumShouldMatch(booleanQuery, minimumShouldMatch); return adjustPureNegative ? fixNegativeQueryIfNeeded(booleanQuery) : booleanQuery; } diff --git a/core/src/main/java/org/elasticsearch/index/query/BoolQueryParser.java b/core/src/main/java/org/elasticsearch/index/query/BoolQueryParser.java index 5fb0f3c9059..ad919de1f0f 100644 --- a/core/src/main/java/org/elasticsearch/index/query/BoolQueryParser.java +++ b/core/src/main/java/org/elasticsearch/index/query/BoolQueryParser.java @@ -71,14 +71,13 @@ public class BoolQueryParser extends BaseQueryParser { switch (currentFieldName) { case "must": query = parseContext.parseInnerQueryBuilder(); - if (query != null) { - mustClauses.add(query); - } + mustClauses.add(query); break; case "should": query = parseContext.parseInnerQueryBuilder(); - if (query != null) { - shouldClauses.add(query); + shouldClauses.add(query); + // EmptyQueryBuilder does not add lucene query later, skip setting minuminShouldMatch + if (query != EmptyQueryBuilder.PROTOTYPE) { if (parseContext.isFilter() && minimumShouldMatch == null) { minimumShouldMatch = "1"; } @@ -86,16 +85,12 @@ public class BoolQueryParser extends BaseQueryParser { break; case "filter": query = parseContext.parseInnerFilterToQueryBuilder(); - if (query != null) { - filterClauses.add(query); - } + filterClauses.add(query); break; case "must_not": case "mustNot": query = parseContext.parseInnerFilterToQueryBuilder(); - if (query != null) { - mustNotClauses.add(query); - } + mustNotClauses.add(query); break; default: throw new QueryParsingException(parseContext, "[bool] query does not support [" + currentFieldName + "]"); @@ -105,14 +100,13 @@ public class BoolQueryParser extends BaseQueryParser { switch (currentFieldName) { case "must": query = parseContext.parseInnerQueryBuilder(); - if (query != null) { - mustClauses.add(query); - } + mustClauses.add(query); break; case "should": query = parseContext.parseInnerQueryBuilder(); - if (query != null) { - shouldClauses.add(query); + shouldClauses.add(query); + // EmptyQueryBuilder does not add lucene query later, skip setting minuminShouldMatch + if (query != EmptyQueryBuilder.PROTOTYPE) { if (parseContext.isFilter() && minimumShouldMatch == null) { minimumShouldMatch = "1"; } @@ -120,16 +114,12 @@ public class BoolQueryParser extends BaseQueryParser { break; case "filter": query = parseContext.parseInnerFilterToQueryBuilder(); - if (query != null) { - filterClauses.add(query); - } + filterClauses.add(query); break; case "must_not": case "mustNot": query = parseContext.parseInnerFilterToQueryBuilder(); - if (query != null) { - mustNotClauses.add(query); - } + mustNotClauses.add(query); break; default: throw new QueryParsingException(parseContext, "bool query does not support [" + currentFieldName + "]"); diff --git a/core/src/main/java/org/elasticsearch/index/query/BoostingQueryBuilder.java b/core/src/main/java/org/elasticsearch/index/query/BoostingQueryBuilder.java index 91d79f590e2..c2f5d1f537d 100644 --- a/core/src/main/java/org/elasticsearch/index/query/BoostingQueryBuilder.java +++ b/core/src/main/java/org/elasticsearch/index/query/BoostingQueryBuilder.java @@ -44,23 +44,31 @@ public class BoostingQueryBuilder extends AbstractQueryBuilder { + + public static final String NAME = "empty_query"; + + /** the one and only empty query builder */ + public static final EmptyQueryBuilder PROTOTYPE = new EmptyQueryBuilder(); + + private EmptyQueryBuilder() { + // prevent other constructors + } + + @Override + public String getName() { + return NAME; + } + + @Override + protected void doXContent(XContentBuilder builder, Params params) throws IOException { + // empty + } + + /** + * @returns null to signal to caller that this query should be ignored + * in the context of the DSL. + */ + @Override + public Query doToQuery(QueryParseContext parseContext) throws QueryParsingException, IOException { + return null; + } + + @Override + public QueryValidationException validate() { + // nothing to validate + return null; + } + + @Override + public EmptyQueryBuilder doReadFrom(StreamInput in) throws IOException { + return EmptyQueryBuilder.PROTOTYPE; + } + + @Override + public void doWriteTo(StreamOutput out) throws IOException { + // nothing to serialize + } +} diff --git a/core/src/main/java/org/elasticsearch/index/query/FQueryFilterBuilder.java b/core/src/main/java/org/elasticsearch/index/query/FQueryFilterBuilder.java index 64fffe75447..08b7ced73af 100644 --- a/core/src/main/java/org/elasticsearch/index/query/FQueryFilterBuilder.java +++ b/core/src/main/java/org/elasticsearch/index/query/FQueryFilterBuilder.java @@ -39,7 +39,7 @@ public class FQueryFilterBuilder extends AbstractQueryBuilder { private final QueryBuilder filter; - static final NotQueryBuilder PROTOTYPE = new NotQueryBuilder(null); + static final NotQueryBuilder PROTOTYPE = new NotQueryBuilder(); public NotQueryBuilder(QueryBuilder filter) { - this.filter = filter; + this.filter = Objects.requireNonNull(filter); + } + + private NotQueryBuilder() { + this.filter = null; } /** @@ -53,16 +57,14 @@ public class NotQueryBuilder extends AbstractQueryBuilder { @Override protected void doXContent(XContentBuilder builder, Params params) throws IOException { builder.startObject(NAME); - doXContentInnerBuilder(builder, "query", filter, params); + builder.field("query"); + filter.toXContent(builder, params); printBoostAndQueryName(builder); builder.endObject(); } @Override protected Query doToQuery(QueryParseContext parseContext) throws IOException { - if (filter == null) { - return null; - } Query luceneQuery = filter.toQuery(parseContext); if (luceneQuery == null) { return null; diff --git a/core/src/main/java/org/elasticsearch/index/query/OrQueryBuilder.java b/core/src/main/java/org/elasticsearch/index/query/OrQueryBuilder.java index 049821a5eca..3838d9a5a20 100644 --- a/core/src/main/java/org/elasticsearch/index/query/OrQueryBuilder.java +++ b/core/src/main/java/org/elasticsearch/index/query/OrQueryBuilder.java @@ -95,6 +95,10 @@ public class OrQueryBuilder extends AbstractQueryBuilder { query.add(innerQuery, Occur.SHOULD); } } + if (query.clauses().isEmpty()) { + // no inner lucene query exists, ignore upstream + return null; + } return query; } diff --git a/core/src/main/java/org/elasticsearch/index/query/QueryBuilders.java b/core/src/main/java/org/elasticsearch/index/query/QueryBuilders.java index 90b2efca348..55789b8df4d 100644 --- a/core/src/main/java/org/elasticsearch/index/query/QueryBuilders.java +++ b/core/src/main/java/org/elasticsearch/index/query/QueryBuilders.java @@ -277,8 +277,8 @@ public abstract class QueryBuilders { * Unlike the "NOT" clause, this still selects documents that contain undesirable terms, * but reduces their overall score: */ - public static BoostingQueryBuilder boostingQuery() { - return new BoostingQueryBuilder(); + public static BoostingQueryBuilder boostingQuery(QueryBuilder positiveQuery, QueryBuilder negativeQuery) { + return new BoostingQueryBuilder(positiveQuery, negativeQuery); } /** diff --git a/core/src/main/java/org/elasticsearch/index/query/QueryFilterBuilder.java b/core/src/main/java/org/elasticsearch/index/query/QueryFilterBuilder.java index 7fe48b32531..a7cd8d1c216 100644 --- a/core/src/main/java/org/elasticsearch/index/query/QueryFilterBuilder.java +++ b/core/src/main/java/org/elasticsearch/index/query/QueryFilterBuilder.java @@ -40,7 +40,7 @@ public class QueryFilterBuilder extends AbstractQueryBuilder private final QueryBuilder queryBuilder; - static final QueryFilterBuilder PROTOTYPE = new QueryFilterBuilder(null); + static final QueryFilterBuilder PROTOTYPE = new QueryFilterBuilder(); /** * A filter that simply wraps a query. @@ -48,7 +48,11 @@ public class QueryFilterBuilder extends AbstractQueryBuilder * @param queryBuilder The query to wrap as a filter */ public QueryFilterBuilder(QueryBuilder queryBuilder) { - this.queryBuilder = queryBuilder; + this.queryBuilder = Objects.requireNonNull(queryBuilder); + } + + private QueryFilterBuilder() { + this.queryBuilder = null; } /** @@ -72,15 +76,13 @@ public class QueryFilterBuilder extends AbstractQueryBuilder @Override protected void doXContent(XContentBuilder builder, Params params) throws IOException { - doXContentInnerBuilder(builder, NAME, queryBuilder, params); + builder.field(NAME); + queryBuilder.toXContent(builder, params); } @Override protected Query doToQuery(QueryParseContext parseContext) throws IOException { // inner query builder can potentially be `null`, in that case we ignore it - if (this.queryBuilder == null) { - return null; - } Query innerQuery = this.queryBuilder.toQuery(parseContext); if (innerQuery == null) { return null; diff --git a/core/src/main/java/org/elasticsearch/index/query/QueryParseContext.java b/core/src/main/java/org/elasticsearch/index/query/QueryParseContext.java index 22e63571a68..569e5679405 100644 --- a/core/src/main/java/org/elasticsearch/index/query/QueryParseContext.java +++ b/core/src/main/java/org/elasticsearch/index/query/QueryParseContext.java @@ -232,7 +232,7 @@ public class QueryParseContext { token = parser.nextToken(); if (token == XContentParser.Token.END_OBJECT) { // empty query - return null; + return EmptyQueryBuilder.PROTOTYPE; } if (token != XContentParser.Token.FIELD_NAME) { throw new QueryParsingException(this, "[_na] query malformed, no field after start_object"); diff --git a/core/src/main/java/org/elasticsearch/indices/query/IndicesQueriesRegistry.java b/core/src/main/java/org/elasticsearch/indices/query/IndicesQueriesRegistry.java index 3c099f5b35f..682daec4afa 100644 --- a/core/src/main/java/org/elasticsearch/indices/query/IndicesQueriesRegistry.java +++ b/core/src/main/java/org/elasticsearch/indices/query/IndicesQueriesRegistry.java @@ -26,6 +26,7 @@ import org.elasticsearch.common.component.AbstractComponent; import org.elasticsearch.common.inject.Inject; import org.elasticsearch.common.io.stream.NamedWriteableRegistry; import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.index.query.EmptyQueryBuilder; import org.elasticsearch.index.query.QueryParser; import java.util.Map; @@ -45,6 +46,9 @@ public class IndicesQueriesRegistry extends AbstractComponent { } namedWriteableRegistry.registerPrototype(queryParser.getBuilderPrototype()); } + // EmptyQueryBuilder is not registered as query parser but used internally. + // We need to register it with the NamedWriteableRegistry in order to serialize it + namedWriteableRegistry.registerPrototype(EmptyQueryBuilder.PROTOTYPE); this.queryParsers = ImmutableMap.copyOf(queryParsers); } diff --git a/core/src/test/java/org/elasticsearch/index/query/AndQueryBuilderTest.java b/core/src/test/java/org/elasticsearch/index/query/AndQueryBuilderTest.java index 5ac0d69b270..e134ef87db4 100644 --- a/core/src/test/java/org/elasticsearch/index/query/AndQueryBuilderTest.java +++ b/core/src/test/java/org/elasticsearch/index/query/AndQueryBuilderTest.java @@ -43,6 +43,9 @@ public class AndQueryBuilderTest extends BaseQueryTestCase { query.add(innerQuery, Occur.MUST); } } + if (query.clauses().isEmpty()) { + return null; + } return query; } @@ -78,6 +81,16 @@ public class AndQueryBuilderTest extends BaseQueryTestCase { context.indexQueryParserService().queryParser(AndQueryBuilder.PROTOTYPE.getName()).fromXContent(context); } + @Test(expected=NullPointerException.class) + public void testNullConstructor() { + new AndQueryBuilder(EmptyQueryBuilder.PROTOTYPE, null); + } + + @Test(expected=NullPointerException.class) + public void testAddNull() { + new AndQueryBuilder(EmptyQueryBuilder.PROTOTYPE).add(null); + } + @Test public void testValidate() { AndQueryBuilder andQuery = new AndQueryBuilder(); diff --git a/core/src/test/java/org/elasticsearch/index/query/BaseQueryTestCase.java b/core/src/test/java/org/elasticsearch/index/query/BaseQueryTestCase.java index b7647ea2f58..fdce80c33ed 100644 --- a/core/src/test/java/org/elasticsearch/index/query/BaseQueryTestCase.java +++ b/core/src/test/java/org/elasticsearch/index/query/BaseQueryTestCase.java @@ -234,14 +234,22 @@ public abstract class BaseQueryTestCase> ext Query expectedQuery = createExpectedQuery(testQuery, context); Query actualQuery = testQuery.toQuery(context); - assertThat(actualQuery, instanceOf(expectedQuery.getClass())); - assertThat(actualQuery, equalTo(expectedQuery)); - assertLuceneQuery(testQuery, actualQuery, context); + // expectedQuery can be null, e.g. in case of BoostingQueryBuilder + // with inner clause that returns null itself + if (expectedQuery == null) { + assertNull("Expected a null query, saw some object.", actualQuery); + } else { + assertThat(actualQuery, instanceOf(expectedQuery.getClass())); + assertThat(actualQuery, equalTo(expectedQuery)); + assertLuceneQuery(testQuery, actualQuery, context); + } } protected final Query createExpectedQuery(QB queryBuilder, QueryParseContext context) throws IOException { Query expectedQuery = doCreateExpectedQuery(queryBuilder, context); - expectedQuery.setBoost(queryBuilder.boost()); + if (expectedQuery != null) { + expectedQuery.setBoost(queryBuilder.boost()); + } return expectedQuery; } diff --git a/core/src/test/java/org/elasticsearch/index/query/BoolQueryBuilderTest.java b/core/src/test/java/org/elasticsearch/index/query/BoolQueryBuilderTest.java index bc379157736..bfa92e02140 100644 --- a/core/src/test/java/org/elasticsearch/index/query/BoolQueryBuilderTest.java +++ b/core/src/test/java/org/elasticsearch/index/query/BoolQueryBuilderTest.java @@ -72,12 +72,14 @@ public class BoolQueryBuilderTest extends BaseQueryTestCase { } BooleanQuery boolQuery = new BooleanQuery(queryBuilder.disableCoord()); - boolQuery.setBoost(queryBuilder.boost()); addBooleanClauses(context, boolQuery, queryBuilder.must(), BooleanClause.Occur.MUST); addBooleanClauses(context, boolQuery, queryBuilder.mustNot(), BooleanClause.Occur.MUST_NOT); addBooleanClauses(context, boolQuery, queryBuilder.should(), BooleanClause.Occur.SHOULD); addBooleanClauses(context, boolQuery, queryBuilder.filter(), BooleanClause.Occur.FILTER); + if (boolQuery.clauses().isEmpty()) { + return new MatchAllDocsQuery(); + } Queries.applyMinimumShouldMatch(boolQuery, queryBuilder.minimumNumberShouldMatch()); return queryBuilder.adjustPureNegative() ? fixNegativeQueryIfNeeded(boolQuery) : boolQuery; } @@ -85,7 +87,10 @@ public class BoolQueryBuilderTest extends BaseQueryTestCase { private static void addBooleanClauses(QueryParseContext parseContext, BooleanQuery booleanQuery, List clauses, Occur occurs) throws IOException { for (QueryBuilder query : clauses) { - booleanQuery.add(new BooleanClause(query.toQuery(parseContext), occurs)); + Query innerQuery = query.toQuery(parseContext); + if (innerQuery != null) { + booleanQuery.add(new BooleanClause(innerQuery, occurs)); + } } } @@ -129,4 +134,24 @@ public class BoolQueryBuilderTest extends BaseQueryTestCase { } assertValidate(booleanQuery, totalExpectedErrors); } + + @Test(expected=NullPointerException.class) + public void testAddNullMust() { + new BoolQueryBuilder().must(null); + } + + @Test(expected=NullPointerException.class) + public void testAddNullMustNot() { + new BoolQueryBuilder().mustNot(null); + } + + @Test(expected=NullPointerException.class) + public void testAddNullShould() { + new BoolQueryBuilder().should(null); + } + + @Test(expected=NullPointerException.class) + public void testAddNullFilter() { + new BoolQueryBuilder().filter(null); + } } diff --git a/core/src/test/java/org/elasticsearch/index/query/BoostingQueryBuilderTest.java b/core/src/test/java/org/elasticsearch/index/query/BoostingQueryBuilderTest.java index 1db0f7305c8..8eee5cc1097 100644 --- a/core/src/test/java/org/elasticsearch/index/query/BoostingQueryBuilderTest.java +++ b/core/src/test/java/org/elasticsearch/index/query/BoostingQueryBuilderTest.java @@ -21,9 +21,6 @@ package org.elasticsearch.index.query; import org.apache.lucene.queries.BoostingQuery; import org.apache.lucene.search.Query; -import org.elasticsearch.common.xcontent.XContentBuilder; -import org.elasticsearch.common.xcontent.XContentFactory; -import org.elasticsearch.common.xcontent.XContentParser; import org.junit.Test; import java.io.IOException; @@ -32,9 +29,7 @@ public class BoostingQueryBuilderTest extends BaseQueryTestCasenull - * if any of the inner query builder is not set - */ - @Test - public void testInnerQueryBuilderNull() throws IOException { - BoostingQueryBuilder boostingQueryBuilder = new BoostingQueryBuilder(); - if (randomBoolean()) { - boostingQueryBuilder.positive(new MatchAllQueryBuilder()).negative(null); - } else { - boostingQueryBuilder.positive(null).negative(new MatchAllQueryBuilder()); - } - assertNull(boostingQueryBuilder.toQuery(createContext())); - } - - @Test - public void testInnerQueryBuilderReturnsNull() throws IOException { - QueryBuilder noOpBuilder = new AbstractQueryBuilder() { - - @Override - public String getName() { - return "dummy"; - } - - @Override - protected void doXContent(XContentBuilder builder, Params params) throws IOException { - } - - @Override - protected Query doToQuery(QueryParseContext parseContext) throws IOException { - return null; - } - }; - BoostingQueryBuilder boostingQueryBuilder; - if (randomBoolean()) { - boostingQueryBuilder = new BoostingQueryBuilder().positive(new MatchAllQueryBuilder()).negative(noOpBuilder); - } else { - boostingQueryBuilder = new BoostingQueryBuilder().positive(noOpBuilder).negative(new MatchAllQueryBuilder()); - } - assertNull(boostingQueryBuilder.toQuery(createContext())); - } - @Test public void testValidate() { - BoostingQueryBuilder boostingQuery = new BoostingQueryBuilder(); int totalExpectedErrors = 0; - if (randomBoolean()) { - boostingQuery.negative(RandomQueryBuilder.createInvalidQuery(random())); + QueryBuilder positive; + QueryBuilder negative; + if (frequently()) { + negative = RandomQueryBuilder.createInvalidQuery(random()); totalExpectedErrors++; - } else if(rarely()) { - boostingQuery.negative(RandomQueryBuilder.createQuery(random())); + } else { + negative = RandomQueryBuilder.createQuery(random()); } - if (randomBoolean()) { - boostingQuery.positive(RandomQueryBuilder.createInvalidQuery(random())); + if (frequently()) { + positive = RandomQueryBuilder.createInvalidQuery(random()); totalExpectedErrors++; - } else if(rarely()) { - boostingQuery.positive(RandomQueryBuilder.createQuery(random())); + } else { + positive = RandomQueryBuilder.createQuery(random()); } + BoostingQueryBuilder boostingQuery = new BoostingQueryBuilder(positive, negative); if (frequently()) { boostingQuery.negativeBoost(0.5f); } else { @@ -134,4 +70,13 @@ public class BoostingQueryBuilderTest extends BaseQueryTestCase { @Override protected Query doCreateExpectedQuery(DisMaxQueryBuilder testBuilder, QueryParseContext context) throws QueryParsingException, IOException { - return new DisjunctionMaxQuery(AbstractQueryBuilder.toQueries(testBuilder.queries(), context), testBuilder.tieBreaker()); + Collection queries = AbstractQueryBuilder.toQueries(testBuilder.queries(), context); + if (queries.isEmpty()) { + return null; + } + return new DisjunctionMaxQuery(queries, testBuilder.tieBreaker()); } /** @@ -82,6 +87,11 @@ public class DisMaxQueryBuilderTest extends BaseQueryTestCase { @Override protected Query doCreateExpectedQuery(NotQueryBuilder queryBuilder, QueryParseContext context) throws QueryParsingException, IOException { - if (queryBuilder.filter() == null) { + if (queryBuilder.filter().toQuery(context) == null) { return null; } return Queries.not(queryBuilder.filter().toQuery(context)); @@ -45,25 +45,9 @@ public class NotQueryBuilderTest extends BaseQueryTestCase { return new NotQueryBuilder(RandomQueryBuilder.createQuery(random())); } - /** - * test corner case where no inner query exist - */ - @Test - public void testNoInnerQuerry() throws QueryParsingException, IOException { - NotQueryBuilder notQuery = new NotQueryBuilder(null); - assertNull(notQuery.toQuery(createContext())); - } - - /** - * Test corner case where inner queries returns null. - * This is legal, e.g. here {@link ConstantScoreQueryBuilder} can wrap a filter - * element that itself parses to null, e.g. - * '{ "constant_score" : "filter {} }' - */ - @Test - public void testInnerQuery() throws QueryParsingException, IOException { - NotQueryBuilder notQuery = new NotQueryBuilder(new ConstantScoreQueryBuilder(null)); - assertNull(notQuery.toQuery(createContext())); + @Test(expected=NullPointerException.class) + public void testNotQueryBuilderNull() { + new NotQueryBuilder(null); } /** diff --git a/core/src/test/java/org/elasticsearch/index/query/OrQueryBuilderTest.java b/core/src/test/java/org/elasticsearch/index/query/OrQueryBuilderTest.java index e9222576d8c..5c15450b0d7 100644 --- a/core/src/test/java/org/elasticsearch/index/query/OrQueryBuilderTest.java +++ b/core/src/test/java/org/elasticsearch/index/query/OrQueryBuilderTest.java @@ -44,6 +44,9 @@ public class OrQueryBuilderTest extends BaseQueryTestCase { query.add(innerQuery, Occur.SHOULD); } } + if (query.clauses().isEmpty()) { + return null; + } return query; } @@ -94,4 +97,14 @@ public class OrQueryBuilderTest extends BaseQueryTestCase { } assertValidate(orQuery, totalExpectedErrors); } + + @Test(expected=NullPointerException.class) + public void testNullConstructor() { + new OrQueryBuilder(EmptyQueryBuilder.PROTOTYPE, null); + } + + @Test(expected=NullPointerException.class) + public void testAddNull() { + new OrQueryBuilder().add(null); + } } diff --git a/core/src/test/java/org/elasticsearch/index/query/QueryFilterBuilderTest.java b/core/src/test/java/org/elasticsearch/index/query/QueryFilterBuilderTest.java index 696d61c35c0..67b0f9dba87 100644 --- a/core/src/test/java/org/elasticsearch/index/query/QueryFilterBuilderTest.java +++ b/core/src/test/java/org/elasticsearch/index/query/QueryFilterBuilderTest.java @@ -21,8 +21,6 @@ package org.elasticsearch.index.query; import org.apache.lucene.search.ConstantScoreQuery; import org.apache.lucene.search.Query; -import org.elasticsearch.common.xcontent.XContentFactory; -import org.elasticsearch.common.xcontent.XContentParser; import org.junit.Test; import java.io.IOException; @@ -32,11 +30,12 @@ public class QueryFilterBuilderTest extends BaseQueryTestCasenull to pass on upwards - */ - @Test - public void testInnerQueryReturnsNull() throws IOException { - QueryParseContext context = createContext(); - - // create inner filter - String queryString = "{ \"constant_score\" : { \"filter\" : {} }"; - XContentParser parser = XContentFactory.xContent(queryString).createParser(queryString); - context.reset(parser); - assertQueryHeader(parser, ConstantScoreQueryBuilder.PROTOTYPE.getName()); - QueryBuilder innerQuery = context.indexQueryParserService().queryParser(ConstantScoreQueryBuilder.PROTOTYPE.getName()).fromXContent(context); - - // check that when wrapping this filter, toQuery() returns null - QueryFilterBuilder queryFilterQuery = new QueryFilterBuilder(innerQuery); - assertNull(queryFilterQuery.toQuery(createContext())); + @Test(expected=NullPointerException.class) + public void testQueryFilterBuilderNull() { + new QueryFilterBuilder(null); } @Test diff --git a/core/src/test/java/org/elasticsearch/index/query/RandomQueryBuilder.java b/core/src/test/java/org/elasticsearch/index/query/RandomQueryBuilder.java index 7d647b93b71..e355d922618 100644 --- a/core/src/test/java/org/elasticsearch/index/query/RandomQueryBuilder.java +++ b/core/src/test/java/org/elasticsearch/index/query/RandomQueryBuilder.java @@ -36,13 +36,15 @@ public class RandomQueryBuilder { * @return a random {@link QueryBuilder} */ public static QueryBuilder createQuery(Random r) { - switch (RandomInts.randomIntBetween(r, 0, 2)) { + switch (RandomInts.randomIntBetween(r, 0, 3)) { case 0: return new MatchAllQueryBuilderTest().createTestQueryBuilder(); case 1: return new TermQueryBuilderTest().createTestQueryBuilder(); case 2: return new IdsQueryBuilderTest().createTestQueryBuilder(); + case 3: + return EmptyQueryBuilder.PROTOTYPE; default: throw new UnsupportedOperationException(); } @@ -59,7 +61,7 @@ public class RandomQueryBuilder { case 0: return new TermQueryBuilder("", "test"); case 1: - return new BoostingQueryBuilder().negativeBoost(-1f); + return new BoostingQueryBuilder(new MatchAllQueryBuilder(), new MatchAllQueryBuilder()).negativeBoost(-1f); case 2: return new CommonTermsQueryBuilder("", "text"); case 3: diff --git a/core/src/test/java/org/elasticsearch/index/query/SimpleIndexQueryParserTests.java b/core/src/test/java/org/elasticsearch/index/query/SimpleIndexQueryParserTests.java index 78a8adaf9b6..59c2ce3cb19 100644 --- a/core/src/test/java/org/elasticsearch/index/query/SimpleIndexQueryParserTests.java +++ b/core/src/test/java/org/elasticsearch/index/query/SimpleIndexQueryParserTests.java @@ -964,7 +964,7 @@ public class SimpleIndexQueryParserTests extends ElasticsearchSingleNodeTest { @Test public void testBoostingQueryBuilder() throws IOException { IndexQueryParserService queryParser = queryParser(); - Query parsedQuery = queryParser.parse(boostingQuery().positive(termQuery("field1", "value1")).negative(termQuery("field1", "value2")).negativeBoost(0.2f)).query(); + Query parsedQuery = queryParser.parse(boostingQuery(termQuery("field1", "value1"), termQuery("field1", "value2")).negativeBoost(0.2f)).query(); assertThat(parsedQuery, instanceOf(BoostingQuery.class)); } diff --git a/core/src/test/java/org/elasticsearch/search/highlight/HighlighterSearchTests.java b/core/src/test/java/org/elasticsearch/search/highlight/HighlighterSearchTests.java index e5ba8a34db2..7f498e6a127 100644 --- a/core/src/test/java/org/elasticsearch/search/highlight/HighlighterSearchTests.java +++ b/core/src/test/java/org/elasticsearch/search/highlight/HighlighterSearchTests.java @@ -50,7 +50,6 @@ import static org.elasticsearch.index.query.QueryBuilders.*; import static org.elasticsearch.search.builder.SearchSourceBuilder.highlight; import static org.elasticsearch.search.builder.SearchSourceBuilder.searchSource; import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.*; -import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertNoFailures; import static org.elasticsearch.test.hamcrest.RegexMatcher.matches; import static org.hamcrest.Matchers.*; @@ -1368,7 +1367,7 @@ public class HighlighterSearchTests extends ElasticsearchIntegrationTest { logger.info("--> highlighting and searching on field1"); SearchSourceBuilder source = searchSource() - .query(boostingQuery().positive(termQuery("field2", "brown")).negative(termQuery("field2", "foobar")).negativeBoost(0.5f)) + .query(boostingQuery(termQuery("field2", "brown"), termQuery("field2", "foobar")).negativeBoost(0.5f)) .highlight(highlight().field("field2").order("score").preTags("").postTags("")); SearchResponse searchResponse = client().prepareSearch("test").setSource(source.buildAsBytes()).get(); @@ -1386,7 +1385,7 @@ public class HighlighterSearchTests extends ElasticsearchIntegrationTest { logger.info("--> highlighting and searching on field1"); SearchSourceBuilder source = searchSource() - .query(boostingQuery().positive(termQuery("field2", "brown")).negative(termQuery("field2", "foobar")).negativeBoost(0.5f)) + .query(boostingQuery(termQuery("field2", "brown"), termQuery("field2", "foobar")).negativeBoost(0.5f)) .highlight(highlight().field("field2").order("score").preTags("").postTags("")); SearchResponse searchResponse = client().prepareSearch("test").setSource(source.buildAsBytes()).get(); @@ -2300,7 +2299,7 @@ public class HighlighterSearchTests extends ElasticsearchIntegrationTest { logger.info("--> highlighting and searching on field1"); SearchSourceBuilder source = searchSource() - .query(boostingQuery().positive(termQuery("field2", "brown")).negative(termQuery("field2", "foobar")).negativeBoost(0.5f)) + .query(boostingQuery(termQuery("field2", "brown"), termQuery("field2", "foobar")).negativeBoost(0.5f)) .highlight(highlight().field("field2").preTags("").postTags("")); SearchResponse searchResponse = client().search(searchRequest("test").source(source)).actionGet(); @@ -2603,10 +2602,10 @@ public class HighlighterSearchTests extends ElasticsearchIntegrationTest { assertHighlight(response, 0, "field1", 0, 1, highlightedMatcher); phrase.boost(1); // Try with a boosting query - response = search.setQuery(boostingQuery().positive(phrase).negative(terms).boost(boost).negativeBoost(1)).get(); + response = search.setQuery(boostingQuery(phrase, terms).boost(boost).negativeBoost(1)).get(); assertHighlight(response, 0, "field1", 0, 1, highlightedMatcher); // Try with a boosting query using a negative boost - response = search.setQuery(boostingQuery().positive(phrase).negative(terms).boost(1).negativeBoost(1/boost)).get(); + response = search.setQuery(boostingQuery(phrase, terms).boost(1).negativeBoost(1/boost)).get(); assertHighlight(response, 0, "field1", 0, 1, highlightedMatcher); } } diff --git a/docs/reference/migration/migrate_query_refactoring.asciidoc b/docs/reference/migration/migrate_query_refactoring.asciidoc index ba425d78fd5..464e43e1982 100644 --- a/docs/reference/migration/migrate_query_refactoring.asciidoc +++ b/docs/reference/migration/migrate_query_refactoring.asciidoc @@ -6,6 +6,11 @@ on the query-refactoring feature branch. === Java-API +==== BoostingQueryBuilder + +Removed setters for mandatory positive/negative query. Both arguments now have +to be supplied at construction time already and have to be non-null. + ==== QueryFilterBuilder Removed the setter `queryName(String queryName)` since this field is not supported