From 15a09a04f2cc62650d54a20ae85c94e92d4d7de9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20B=C3=BCscher?= Date: Fri, 24 Jul 2015 17:05:59 +0200 Subject: [PATCH] Query Refactoring: Move null-checks from constructors and setters to validate Following up to #12427, this PR does same changes, moving null-checks from construtors and setters in query builder to the validate() method. PR against query-refactoring branch --- .../index/query/AbstractQueryBuilder.java | 6 ++- .../index/query/AndQueryBuilder.java | 4 +- .../index/query/BoolQueryBuilder.java | 8 ++-- .../index/query/BoostingQueryBuilder.java | 26 +++++------ .../query/ConstantScoreQueryBuilder.java | 17 +++---- .../index/query/DisMaxQueryBuilder.java | 2 +- .../index/query/FQueryFilterBuilder.java | 8 +--- .../query/FieldMaskingSpanQueryBuilder.java | 25 +++++----- .../index/query/NotQueryBuilder.java | 8 +--- .../index/query/OrQueryBuilder.java | 4 +- .../index/query/QueryFilterBuilder.java | 8 +--- .../index/query/SpanNearQueryBuilder.java | 9 +--- .../index/query/AndQueryBuilderTest.java | 16 ++----- .../index/query/BoolQueryBuilderTest.java | 46 +++++++++---------- .../index/query/BoostingQueryBuilderTest.java | 21 ++++----- .../query/ConstantScoreQueryBuilderTest.java | 11 ++--- .../index/query/DisMaxQueryBuilderTest.java | 11 ++--- .../index/query/FQueryFilterBuilderTest.java | 11 ++--- .../FieldMaskingSpanQueryBuilderTest.java | 22 ++++----- .../index/query/NotQueryBuilderTest.java | 11 ++--- .../index/query/OrQueryBuilderTest.java | 16 ++----- .../index/query/QueryFilterBuilderTest.java | 11 ++--- 22 files changed, 123 insertions(+), 178 deletions(-) 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 82e5a4b7f9f..738174b836b 100644 --- a/core/src/main/java/org/elasticsearch/index/query/AbstractQueryBuilder.java +++ b/core/src/main/java/org/elasticsearch/index/query/AbstractQueryBuilder.java @@ -233,7 +233,7 @@ public abstract class AbstractQueryBuilder exte return queries; } - protected static QueryValidationException validateInnerQueries(List queryBuilders, QueryValidationException initialValidationException) { + protected QueryValidationException validateInnerQueries(List queryBuilders, QueryValidationException initialValidationException) { QueryValidationException validationException = initialValidationException; for (QueryBuilder queryBuilder : queryBuilders) { validationException = validateInnerQuery(queryBuilder, validationException); @@ -241,13 +241,15 @@ public abstract class AbstractQueryBuilder exte return validationException; } - protected static QueryValidationException validateInnerQuery(QueryBuilder queryBuilder, QueryValidationException initialValidationException) { + protected QueryValidationException validateInnerQuery(QueryBuilder queryBuilder, QueryValidationException initialValidationException) { QueryValidationException validationException = initialValidationException; if (queryBuilder != null) { QueryValidationException queryValidationException = queryBuilder.validate(); if (queryValidationException != null) { validationException = QueryValidationException.addValidationErrors(queryValidationException.validationErrors(), validationException); } + } else { + validationException = addValidationError("inner query cannot be null", validationException); } return validationException; } 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 bdcf3d565f0..0bf145de14b 100644 --- a/core/src/main/java/org/elasticsearch/index/query/AndQueryBuilder.java +++ b/core/src/main/java/org/elasticsearch/index/query/AndQueryBuilder.java @@ -51,7 +51,7 @@ public class AndQueryBuilder extends AbstractQueryBuilder { */ public AndQueryBuilder(QueryBuilder... filters) { for (QueryBuilder filter : filters) { - this.filters.add(Objects.requireNonNull(filter)); + this.filters.add(filter); } } @@ -60,7 +60,7 @@ public class AndQueryBuilder extends AbstractQueryBuilder { * @param filterBuilder nested filter, no null value allowed */ public AndQueryBuilder add(QueryBuilder filterBuilder) { - filters.add(Objects.requireNonNull(filterBuilder)); + filters.add(filterBuilder); return this; } 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 2f960ab5255..99db83bd976 100644 --- a/core/src/main/java/org/elasticsearch/index/query/BoolQueryBuilder.java +++ b/core/src/main/java/org/elasticsearch/index/query/BoolQueryBuilder.java @@ -68,7 +68,7 @@ public class BoolQueryBuilder extends AbstractQueryBuilder { * contribute to scoring. No null value allowed. */ public BoolQueryBuilder must(QueryBuilder queryBuilder) { - mustClauses.add(Objects.requireNonNull(queryBuilder)); + mustClauses.add(queryBuilder); return this; } @@ -84,7 +84,7 @@ public class BoolQueryBuilder extends AbstractQueryBuilder { * not contribute to scoring. No null value allowed. */ public BoolQueryBuilder filter(QueryBuilder queryBuilder) { - filterClauses.add(Objects.requireNonNull(queryBuilder)); + filterClauses.add(queryBuilder); return this; } @@ -100,7 +100,7 @@ public class BoolQueryBuilder extends AbstractQueryBuilder { * No null value allowed. */ public BoolQueryBuilder mustNot(QueryBuilder queryBuilder) { - mustNotClauses.add(Objects.requireNonNull(queryBuilder)); + mustNotClauses.add(queryBuilder); return this; } @@ -119,7 +119,7 @@ public class BoolQueryBuilder extends AbstractQueryBuilder { * @see #minimumNumberShouldMatch(int) */ public BoolQueryBuilder should(QueryBuilder queryBuilder) { - shouldClauses.add(Objects.requireNonNull(queryBuilder)); + shouldClauses.add(queryBuilder); return this; } 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 c2f5d1f537d..4e543506840 100644 --- a/core/src/main/java/org/elasticsearch/index/query/BoostingQueryBuilder.java +++ b/core/src/main/java/org/elasticsearch/index/query/BoostingQueryBuilder.java @@ -50,15 +50,7 @@ public class BoostingQueryBuilder extends AbstractQueryBuilder * Add a sub-query to this disjunction. */ public DisMaxQueryBuilder add(QueryBuilder queryBuilder) { - queries.add(Objects.requireNonNull(queryBuilder)); + queries.add(queryBuilder); return this; } 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 08b7ced73af..c9467cb9a29 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(); + static final NotQueryBuilder PROTOTYPE = new NotQueryBuilder(null); public NotQueryBuilder(QueryBuilder filter) { - this.filter = Objects.requireNonNull(filter); - } - - private NotQueryBuilder() { - this.filter = null; + this.filter = filter; } /** 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 3838d9a5a20..7594822d133 100644 --- a/core/src/main/java/org/elasticsearch/index/query/OrQueryBuilder.java +++ b/core/src/main/java/org/elasticsearch/index/query/OrQueryBuilder.java @@ -48,7 +48,7 @@ public class OrQueryBuilder extends AbstractQueryBuilder { public OrQueryBuilder(QueryBuilder... filters) { for (QueryBuilder filter : filters) { - this.filters.add(Objects.requireNonNull(filter)); + this.filters.add(filter); } } @@ -57,7 +57,7 @@ public class OrQueryBuilder extends AbstractQueryBuilder { * No null value allowed. */ public OrQueryBuilder add(QueryBuilder filterBuilder) { - filters.add(Objects.requireNonNull(filterBuilder)); + filters.add(filterBuilder); return this; } 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 a7cd8d1c216..a2ef9e74834 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(); + static final QueryFilterBuilder PROTOTYPE = new QueryFilterBuilder(null); /** * A filter that simply wraps a query. @@ -48,11 +48,7 @@ public class QueryFilterBuilder extends AbstractQueryBuilder * @param queryBuilder The query to wrap as a filter */ public QueryFilterBuilder(QueryBuilder queryBuilder) { - this.queryBuilder = Objects.requireNonNull(queryBuilder); - } - - private QueryFilterBuilder() { - this.queryBuilder = null; + this.queryBuilder = queryBuilder; } /** diff --git a/core/src/main/java/org/elasticsearch/index/query/SpanNearQueryBuilder.java b/core/src/main/java/org/elasticsearch/index/query/SpanNearQueryBuilder.java index 46ce70ed5ce..b710c99c1bc 100644 --- a/core/src/main/java/org/elasticsearch/index/query/SpanNearQueryBuilder.java +++ b/core/src/main/java/org/elasticsearch/index/query/SpanNearQueryBuilder.java @@ -54,7 +54,7 @@ public class SpanNearQueryBuilder extends AbstractQueryBuilder { 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(); @@ -98,7 +88,11 @@ public class AndQueryBuilderTest extends BaseQueryTestCase { int totalExpectedErrors = 0; for (int i = 0; i < iters; i++) { if (randomBoolean()) { - andQuery.add(RandomQueryBuilder.createInvalidQuery(random())); + if (randomBoolean()) { + andQuery.add(RandomQueryBuilder.createInvalidQuery(random())); + } else { + andQuery.add(null); + } totalExpectedErrors++; } else { andQuery.add(RandomQueryBuilder.createQuery(random())); 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 bfa92e02140..de36d7e7a11 100644 --- a/core/src/test/java/org/elasticsearch/index/query/BoolQueryBuilderTest.java +++ b/core/src/test/java/org/elasticsearch/index/query/BoolQueryBuilderTest.java @@ -101,7 +101,11 @@ public class BoolQueryBuilderTest extends BaseQueryTestCase { int totalExpectedErrors = 0; for (int i = 0; i < iters; i++) { if (randomBoolean()) { - booleanQuery.must(RandomQueryBuilder.createInvalidQuery(random())); + if (randomBoolean()) { + booleanQuery.must(RandomQueryBuilder.createInvalidQuery(random())); + } else { + booleanQuery.must(null); + } totalExpectedErrors++; } else { booleanQuery.must(RandomQueryBuilder.createQuery(random())); @@ -110,23 +114,37 @@ public class BoolQueryBuilderTest extends BaseQueryTestCase { iters = randomIntBetween(0, 3); for (int i = 0; i < iters; i++) { if (randomBoolean()) { - booleanQuery.should(RandomQueryBuilder.createInvalidQuery(random())); + if (randomBoolean()) { + booleanQuery.should(RandomQueryBuilder.createInvalidQuery(random())); + } else { + booleanQuery.should(null); + } totalExpectedErrors++; } else { booleanQuery.should(RandomQueryBuilder.createQuery(random())); } } + iters = randomIntBetween(0, 3); for (int i = 0; i < iters; i++) { if (randomBoolean()) { - booleanQuery.mustNot(RandomQueryBuilder.createInvalidQuery(random())); + if (randomBoolean()) { + booleanQuery.mustNot(RandomQueryBuilder.createInvalidQuery(random())); + } else { + booleanQuery.mustNot(null); + } totalExpectedErrors++; } else { booleanQuery.mustNot(RandomQueryBuilder.createQuery(random())); } } + iters = randomIntBetween(0, 3); for (int i = 0; i < iters; i++) { if (randomBoolean()) { - booleanQuery.filter(RandomQueryBuilder.createInvalidQuery(random())); + if (randomBoolean()) { + booleanQuery.filter(RandomQueryBuilder.createInvalidQuery(random())); + } else { + booleanQuery.filter(null); + } totalExpectedErrors++; } else { booleanQuery.filter(RandomQueryBuilder.createQuery(random())); @@ -134,24 +152,4 @@ 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 8eee5cc1097..7d22ea6e3f9 100644 --- a/core/src/test/java/org/elasticsearch/index/query/BoostingQueryBuilderTest.java +++ b/core/src/test/java/org/elasticsearch/index/query/BoostingQueryBuilderTest.java @@ -47,16 +47,20 @@ public class BoostingQueryBuilderTest extends BaseQueryTestCase { return new NotQueryBuilder(RandomQueryBuilder.createQuery(random())); } - @Test(expected=NullPointerException.class) - public void testNotQueryBuilderNull() { - new NotQueryBuilder(null); - } - /** * @throws IOException */ @@ -65,10 +60,12 @@ public class NotQueryBuilderTest extends BaseQueryTestCase { @Test public void testValidate() { - QueryBuilder innerQuery; + QueryBuilder innerQuery = null; int totalExpectedErrors = 0; if (randomBoolean()) { - innerQuery = RandomQueryBuilder.createInvalidQuery(random()); + if (randomBoolean()) { + innerQuery = RandomQueryBuilder.createInvalidQuery(random()); + } totalExpectedErrors++; } else { innerQuery = RandomQueryBuilder.createQuery(random()); 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 5c15450b0d7..ed19a470ebd 100644 --- a/core/src/test/java/org/elasticsearch/index/query/OrQueryBuilderTest.java +++ b/core/src/test/java/org/elasticsearch/index/query/OrQueryBuilderTest.java @@ -89,7 +89,11 @@ public class OrQueryBuilderTest extends BaseQueryTestCase { int totalExpectedErrors = 0; for (int i = 0; i < iters; i++) { if (randomBoolean()) { - orQuery.add(RandomQueryBuilder.createInvalidQuery(random())); + if (randomBoolean()) { + orQuery.add(RandomQueryBuilder.createInvalidQuery(random())); + } else { + orQuery.add(null); + } totalExpectedErrors++; } else { orQuery.add(RandomQueryBuilder.createQuery(random())); @@ -97,14 +101,4 @@ 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 67b0f9dba87..f90238a6aaf 100644 --- a/core/src/test/java/org/elasticsearch/index/query/QueryFilterBuilderTest.java +++ b/core/src/test/java/org/elasticsearch/index/query/QueryFilterBuilderTest.java @@ -43,17 +43,14 @@ public class QueryFilterBuilderTest extends BaseQueryTestCase