From 10f80167e66901ce9b208b974d4d8ac2bdc6fa53 Mon Sep 17 00:00:00 2001 From: javanna Date: Fri, 26 Jun 2015 15:38:11 +0200 Subject: [PATCH] Query refactoring: validate inner queries whenever supported Added validation for all inner queries that any already refactored query may hold. Added also tests around this. At the end of the refactoring validate will be called by SearchRequest#validate during TransportSearchAction execution, which will call validate against the top level query builder that will need to go and validate its data plus all of its inner queries, and so on. Closes #11889 --- .../index/query/AbstractQueryBuilder.java | 23 ++++++++ .../index/query/AndQueryBuilder.java | 3 +- .../index/query/BaseTermQueryBuilder.java | 4 +- .../index/query/BoolQueryBuilder.java | 8 ++- .../index/query/BoostingQueryBuilder.java | 5 +- .../index/query/CommonTermsQueryBuilder.java | 4 +- .../query/ConstantScoreQueryBuilder.java | 3 +- .../index/query/DisMaxQueryBuilder.java | 3 +- .../index/query/FQueryFilterBuilder.java | 3 +- .../query/FieldMaskingSpanQueryBuilder.java | 16 +++--- .../query/FieldMaskingSpanQueryParser.java | 2 +- .../index/query/NotQueryBuilder.java | 3 +- .../index/query/OrQueryBuilder.java | 3 +- .../index/query/QueryFilterBuilder.java | 3 +- .../index/query/QueryValidationException.java | 27 +++++++++- .../index/query/RangeQueryBuilder.java | 6 +-- .../index/query/SimpleQueryStringBuilder.java | 4 +- .../index/query/AndQueryBuilderTest.java | 20 ++++++- .../index/query/BaseQueryTestCase.java | 10 ++++ .../index/query/BoolQueryBuilderTest.java | 54 ++++++++++++++++--- .../index/query/BoostingQueryBuilderTest.java | 31 ++++++++--- .../query/ConstantScoreQueryBuilderTest.java | 16 +++++- .../index/query/DisMaxQueryBuilderTest.java | 18 ++++++- .../index/query/FQueryFilterBuilderTest.java | 16 +++++- .../FieldMaskingSpanQueryBuilderTest.java | 34 ++++++------ .../index/query/NotQueryBuilderTest.java | 16 +++++- .../index/query/OrQueryBuilderTest.java | 20 ++++++- .../index/query/QueryFilterBuilderTest.java | 16 +++++- .../index/query/RandomQueryBuilder.java | 40 ++++++++++---- 29 files changed, 326 insertions(+), 85 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 124396a0cc5..7cd3c4e3b33 100644 --- a/core/src/main/java/org/elasticsearch/index/query/AbstractQueryBuilder.java +++ b/core/src/main/java/org/elasticsearch/index/query/AbstractQueryBuilder.java @@ -76,6 +76,10 @@ public abstract class AbstractQueryBuilder extends ToXC public void writeTo(StreamOutput out) throws IOException { } + protected final QueryValidationException addValidationError(String validationError, QueryValidationException validationException) { + return QueryValidationException.addValidationError(getName(), validationError, validationException); + } + @Override public final boolean equals(Object obj) { if (this == obj) { @@ -159,4 +163,23 @@ public abstract class AbstractQueryBuilder extends ToXC xContentBuilder.endObject(); } } + + protected static QueryValidationException validateInnerQueries(List queryBuilders, QueryValidationException initialValidationException) { + QueryValidationException validationException = initialValidationException; + for (QueryBuilder queryBuilder : queryBuilders) { + validationException = validateInnerQuery(queryBuilder, validationException); + } + return validationException; + } + + protected static 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); + } + } + 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 b0c8a572556..b37bd93de87 100644 --- a/core/src/main/java/org/elasticsearch/index/query/AndQueryBuilder.java +++ b/core/src/main/java/org/elasticsearch/index/query/AndQueryBuilder.java @@ -125,8 +125,7 @@ public class AndQueryBuilder extends AbstractQueryBuilder { @Override public QueryValidationException validate() { - // nothing to validate - return null; + return validateInnerQueries(filters, null); } @Override diff --git a/core/src/main/java/org/elasticsearch/index/query/BaseTermQueryBuilder.java b/core/src/main/java/org/elasticsearch/index/query/BaseTermQueryBuilder.java index 8b81d17b68f..3d5db49bb61 100644 --- a/core/src/main/java/org/elasticsearch/index/query/BaseTermQueryBuilder.java +++ b/core/src/main/java/org/elasticsearch/index/query/BaseTermQueryBuilder.java @@ -178,10 +178,10 @@ public abstract class BaseTermQueryBuilder> public QueryValidationException validate() { QueryValidationException validationException = null; if (fieldName == null || fieldName.isEmpty()) { - validationException = QueryValidationException.addValidationError("field name cannot be null or empty.", validationException); + validationException = addValidationError("field name cannot be null or empty.", validationException); } if (value == null) { - validationException = QueryValidationException.addValidationError("value cannot be null.", validationException); + validationException = addValidationError("value cannot be null.", validationException); } return validationException; } 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 2b482a7c7b3..397e5a473d2 100644 --- a/core/src/main/java/org/elasticsearch/index/query/BoolQueryBuilder.java +++ b/core/src/main/java/org/elasticsearch/index/query/BoolQueryBuilder.java @@ -315,8 +315,12 @@ public class BoolQueryBuilder extends AbstractQueryBuilder imp @Override public QueryValidationException validate() { - // nothing to validate, clauses are optional, see hasClauses(), other parameters have defaults - return null; + QueryValidationException validationException = null; + validationException = validateInnerQueries(mustClauses, validationException); + validationException = validateInnerQueries(shouldClauses, validationException); + validationException = validateInnerQueries(mustNotClauses, validationException); + validationException = validateInnerQueries(filterClauses, validationException); + return validationException; } private static void addBooleanClauses(QueryParseContext parseContext, BooleanQuery booleanQuery, List clauses, Occur occurs) 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 3e6aeea0901..70886f7225b 100644 --- a/core/src/main/java/org/elasticsearch/index/query/BoostingQueryBuilder.java +++ b/core/src/main/java/org/elasticsearch/index/query/BoostingQueryBuilder.java @@ -132,9 +132,10 @@ public class BoostingQueryBuilder extends AbstractQueryBuilder @Override public QueryValidationException validate() { - // nothing to validate, clauses are optional - return null; + return validateInnerQueries(queries, null); } @Override 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 9ecbc21dcbb..a5bd11f8dc6 100644 --- a/core/src/main/java/org/elasticsearch/index/query/FQueryFilterBuilder.java +++ b/core/src/main/java/org/elasticsearch/index/query/FQueryFilterBuilder.java @@ -105,8 +105,7 @@ public class FQueryFilterBuilder extends AbstractQueryBuilder { @Override public QueryValidationException validate() { - // nothing to validate. - return null; + return validateInnerQuery(filter, null); } @Override 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 d6f5d2fbdcd..5cbf71c09cb 100644 --- a/core/src/main/java/org/elasticsearch/index/query/OrQueryBuilder.java +++ b/core/src/main/java/org/elasticsearch/index/query/OrQueryBuilder.java @@ -122,8 +122,7 @@ public class OrQueryBuilder extends AbstractQueryBuilder { @Override public QueryValidationException validate() { - // nothing to validate - return null; + return validateInnerQueries(filters, null); } @Override 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 b308cdfc6e9..ee1837a55fc 100644 --- a/core/src/main/java/org/elasticsearch/index/query/QueryFilterBuilder.java +++ b/core/src/main/java/org/elasticsearch/index/query/QueryFilterBuilder.java @@ -78,8 +78,7 @@ public class QueryFilterBuilder extends AbstractQueryBuilder @Override public QueryValidationException validate() { - // nothing to validate - return null; + return validateInnerQuery(queryBuilder, null); } @Override diff --git a/core/src/main/java/org/elasticsearch/index/query/QueryValidationException.java b/core/src/main/java/org/elasticsearch/index/query/QueryValidationException.java index c6aa7f662a5..3761a8247fd 100644 --- a/core/src/main/java/org/elasticsearch/index/query/QueryValidationException.java +++ b/core/src/main/java/org/elasticsearch/index/query/QueryValidationException.java @@ -34,6 +34,11 @@ public class QueryValidationException extends IllegalArgumentException { validationErrors.add(error); } + public QueryValidationException(List errors) { + super("query validation failed"); + validationErrors.addAll(errors); + } + public void addValidationError(String error) { validationErrors.add(error); } @@ -62,16 +67,34 @@ public class QueryValidationException extends IllegalArgumentException { /** * Helper method than can be used to add error messages to an existing {@link QueryValidationException}. * When passing {@code null} as the initial exception, a new exception is created. + * + * @param queryId * @param validationError the error message to add to an initial exception * @param validationException an initial exception. Can be {@code null}, in which case a new exception is created. * @return a {@link QueryValidationException} with added validation error message */ - public static QueryValidationException addValidationError(String validationError, QueryValidationException validationException) { + public static QueryValidationException addValidationError(String queryId, String validationError, QueryValidationException validationException) { if (validationException == null) { - validationException = new QueryValidationException(validationError); + validationException = new QueryValidationException("[" + queryId + "] " + validationError); } else { validationException.addValidationError(validationError); } return validationException; } + + /** + * Helper method than can be used to add error messages to an existing {@link QueryValidationException}. + * When passing {@code null} as the initial exception, a new exception is created. + * @param validationErrors the error messages to add to an initial exception + * @param validationException an initial exception. Can be {@code null}, in which case a new exception is created. + * @return a {@link QueryValidationException} with added validation error message + */ + public static QueryValidationException addValidationErrors(List validationErrors, QueryValidationException validationException) { + if (validationException == null) { + validationException = new QueryValidationException(validationErrors); + } else { + validationException.addValidationErrors(validationErrors); + } + return validationException; + } } diff --git a/core/src/main/java/org/elasticsearch/index/query/RangeQueryBuilder.java b/core/src/main/java/org/elasticsearch/index/query/RangeQueryBuilder.java index f8d0e306271..c3b5c859dd5 100644 --- a/core/src/main/java/org/elasticsearch/index/query/RangeQueryBuilder.java +++ b/core/src/main/java/org/elasticsearch/index/query/RangeQueryBuilder.java @@ -326,13 +326,13 @@ public class RangeQueryBuilder extends MultiTermQueryBuilder public QueryValidationException validate() { QueryValidationException validationException = null; if (this.fieldName == null || this.fieldName.isEmpty()) { - validationException = QueryValidationException.addValidationError("field name cannot be null or empty.", validationException); + validationException = addValidationError("field name cannot be null or empty.", validationException); } if (this.timeZone != null) { try { DateTimeZone.forID(this.timeZone); } catch (Exception e) { - validationException = QueryValidationException.addValidationError("error parsing timezone." + e.getMessage(), + validationException = addValidationError("error parsing timezone." + e.getMessage(), validationException); } } @@ -340,7 +340,7 @@ public class RangeQueryBuilder extends MultiTermQueryBuilder try { Joda.forPattern(this.format); } catch (Exception e) { - validationException = QueryValidationException.addValidationError("error parsing format." + e.getMessage(), + validationException = addValidationError("error parsing format." + e.getMessage(), validationException); } } diff --git a/core/src/main/java/org/elasticsearch/index/query/SimpleQueryStringBuilder.java b/core/src/main/java/org/elasticsearch/index/query/SimpleQueryStringBuilder.java index ce883a396e1..f88aecd6d5b 100644 --- a/core/src/main/java/org/elasticsearch/index/query/SimpleQueryStringBuilder.java +++ b/core/src/main/java/org/elasticsearch/index/query/SimpleQueryStringBuilder.java @@ -277,11 +277,9 @@ public class SimpleQueryStringBuilder extends AbstractQueryBuilder { AndQueryBuilder query = new AndQueryBuilder(); int subQueries = randomIntBetween(1, 5); for (int i = 0; i < subQueries; i++ ) { - query.add(RandomQueryBuilder.create(random())); + query.add(RandomQueryBuilder.createQuery(random())); } if (randomBoolean()) { query.queryName(randomAsciiOfLengthBetween(1, 10)); @@ -90,4 +90,20 @@ public class AndQueryBuilderTest extends BaseQueryTestCase { assertQueryHeader(parser, AndQueryBuilder.PROTOTYPE.getName()); context.indexQueryParserService().queryParser(AndQueryBuilder.PROTOTYPE.getName()).fromXContent(context); } + + @Test + public void testValidate() { + AndQueryBuilder andQuery = new AndQueryBuilder(); + int iters = randomIntBetween(0, 5); + int totalExpectedErrors = 0; + for (int i = 0; i < iters; i++) { + if (randomBoolean()) { + andQuery.add(RandomQueryBuilder.createInvalidQuery(random())); + totalExpectedErrors++; + } else { + andQuery.add(RandomQueryBuilder.createQuery(random())); + } + } + assertValidate(andQuery, totalExpectedErrors); + } } 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 f10ba2799fe..f6d7f6a9df7 100644 --- a/core/src/test/java/org/elasticsearch/index/query/BaseQueryTestCase.java +++ b/core/src/test/java/org/elasticsearch/index/query/BaseQueryTestCase.java @@ -273,4 +273,14 @@ public abstract class BaseQueryTestCase> extends Ela assertThat(parser.currentName(), is(expectedParserName)); assertThat(parser.nextToken(), is(XContentParser.Token.START_OBJECT)); } + + protected static void assertValidate(QueryBuilder queryBuilder, int totalExpectedErrors) { + QueryValidationException queryValidationException = queryBuilder.validate(); + if (totalExpectedErrors > 0) { + assertThat(queryValidationException, notNullValue()); + assertThat(queryValidationException.validationErrors().size(), equalTo(totalExpectedErrors)); + } else { + assertThat(queryValidationException, nullValue()); + } + } } 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 b55c822185f..51f71bbee97 100644 --- a/core/src/test/java/org/elasticsearch/index/query/BoolQueryBuilderTest.java +++ b/core/src/test/java/org/elasticsearch/index/query/BoolQueryBuilderTest.java @@ -20,17 +20,18 @@ package org.elasticsearch.index.query; import org.apache.lucene.search.BooleanClause; +import org.apache.lucene.search.BooleanClause.Occur; import org.apache.lucene.search.BooleanQuery; import org.apache.lucene.search.MatchAllDocsQuery; import org.apache.lucene.search.Query; -import org.apache.lucene.search.BooleanClause.Occur; import org.elasticsearch.common.lucene.search.Queries; +import org.junit.Test; import java.io.IOException; import java.util.List; -import static org.hamcrest.Matchers.equalTo; import static org.elasticsearch.common.lucene.search.Queries.fixNegativeQueryIfNeeded; +import static org.hamcrest.Matchers.equalTo; public class BoolQueryBuilderTest extends BaseQueryTestCase { @@ -63,19 +64,19 @@ public class BoolQueryBuilderTest extends BaseQueryTestCase { } int mustClauses = randomIntBetween(0, 3); for (int i = 0; i < mustClauses; i++) { - query.must(RandomQueryBuilder.create(random())); + query.must(RandomQueryBuilder.createQuery(random())); } int mustNotClauses = randomIntBetween(0, 3); for (int i = 0; i < mustNotClauses; i++) { - query.mustNot(RandomQueryBuilder.create(random())); + query.mustNot(RandomQueryBuilder.createQuery(random())); } int shouldClauses = randomIntBetween(0, 3); for (int i = 0; i < shouldClauses; i++) { - query.should(RandomQueryBuilder.create(random())); + query.should(RandomQueryBuilder.createQuery(random())); } int filterClauses = randomIntBetween(0, 3); for (int i = 0; i < filterClauses; i++) { - query.filter(RandomQueryBuilder.create(random())); + query.filter(RandomQueryBuilder.createQuery(random())); } if (randomBoolean()) { query.queryName(randomUnicodeOfLengthBetween(3, 15)); @@ -107,4 +108,45 @@ public class BoolQueryBuilderTest extends BaseQueryTestCase { booleanQuery.add(new BooleanClause(query.toQuery(parseContext), occurs)); } } + + @Test + public void testValidate() { + BoolQueryBuilder booleanQuery = new BoolQueryBuilder(); + int iters = randomIntBetween(0, 3); + int totalExpectedErrors = 0; + for (int i = 0; i < iters; i++) { + if (randomBoolean()) { + booleanQuery.must(RandomQueryBuilder.createInvalidQuery(random())); + totalExpectedErrors++; + } else { + booleanQuery.must(RandomQueryBuilder.createQuery(random())); + } + } + iters = randomIntBetween(0, 3); + for (int i = 0; i < iters; i++) { + if (randomBoolean()) { + booleanQuery.should(RandomQueryBuilder.createInvalidQuery(random())); + totalExpectedErrors++; + } else { + booleanQuery.should(RandomQueryBuilder.createQuery(random())); + } + } + for (int i = 0; i < iters; i++) { + if (randomBoolean()) { + booleanQuery.mustNot(RandomQueryBuilder.createInvalidQuery(random())); + totalExpectedErrors++; + } else { + booleanQuery.mustNot(RandomQueryBuilder.createQuery(random())); + } + } + for (int i = 0; i < iters; i++) { + if (randomBoolean()) { + booleanQuery.filter(RandomQueryBuilder.createInvalidQuery(random())); + totalExpectedErrors++; + } else { + booleanQuery.filter(RandomQueryBuilder.createQuery(random())); + } + } + assertValidate(booleanQuery, totalExpectedErrors); + } } 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 95e40551f99..017cfde8505 100644 --- a/core/src/test/java/org/elasticsearch/index/query/BoostingQueryBuilderTest.java +++ b/core/src/test/java/org/elasticsearch/index/query/BoostingQueryBuilderTest.java @@ -28,15 +28,13 @@ import org.junit.Test; import java.io.IOException; -import static org.hamcrest.Matchers.is; - public class BoostingQueryBuilderTest extends BaseQueryTestCase { @Override protected BoostingQueryBuilder createTestQueryBuilder() { BoostingQueryBuilder query = new BoostingQueryBuilder(); - query.positive(RandomQueryBuilder.create(random())); - query.negative(RandomQueryBuilder.create(random())); + query.positive(RandomQueryBuilder.createQuery(random())); + query.negative(RandomQueryBuilder.createQuery(random())); query.negativeBoost(2.0f / randomIntBetween(1, 20)); if (randomBoolean()) { query.boost(2.0f / randomIntBetween(1, 20)); @@ -121,9 +119,26 @@ public class BoostingQueryBuilderTest extends BaseQueryTestCase { @@ -60,7 +58,7 @@ public class FieldMaskingSpanQueryBuilderTest extends BaseQueryTestCase { */ @Override protected NotQueryBuilder createTestQueryBuilder() { - NotQueryBuilder query = new NotQueryBuilder(RandomQueryBuilder.create(random())); + NotQueryBuilder query = new NotQueryBuilder(RandomQueryBuilder.createQuery(random())); if (randomBoolean()) { query.queryName(randomAsciiOfLengthBetween(1, 10)); } @@ -92,4 +92,18 @@ public class NotQueryBuilderTest extends BaseQueryTestCase { assertQueryHeader(parser, NotQueryBuilder.PROTOTYPE.getName()); context.indexQueryParserService().queryParser(NotQueryBuilder.PROTOTYPE.getName()).fromXContent(context); } + + @Test + public void testValidate() { + QueryBuilder innerQuery; + int totalExpectedErrors = 0; + if (randomBoolean()) { + innerQuery = RandomQueryBuilder.createInvalidQuery(random()); + totalExpectedErrors++; + } else { + innerQuery = RandomQueryBuilder.createQuery(random()); + } + NotQueryBuilder notQuery = new NotQueryBuilder(innerQuery); + assertValidate(notQuery, totalExpectedErrors); + } } 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 db9b7ba1b27..78feca5b37b 100644 --- a/core/src/test/java/org/elasticsearch/index/query/OrQueryBuilderTest.java +++ b/core/src/test/java/org/elasticsearch/index/query/OrQueryBuilderTest.java @@ -19,9 +19,9 @@ package org.elasticsearch.index.query; +import org.apache.lucene.search.BooleanClause.Occur; import org.apache.lucene.search.BooleanQuery; import org.apache.lucene.search.Query; -import org.apache.lucene.search.BooleanClause.Occur; import org.elasticsearch.common.xcontent.XContentFactory; import org.elasticsearch.common.xcontent.XContentParser; import org.junit.Test; @@ -57,7 +57,7 @@ public class OrQueryBuilderTest extends BaseQueryTestCase { OrQueryBuilder query = new OrQueryBuilder(); int subQueries = randomIntBetween(1, 5); for (int i = 0; i < subQueries; i++ ) { - query.add(RandomQueryBuilder.create(random())); + query.add(RandomQueryBuilder.createQuery(random())); } if (randomBoolean()) { query.queryName(randomAsciiOfLengthBetween(1, 10)); @@ -91,4 +91,20 @@ public class OrQueryBuilderTest extends BaseQueryTestCase { assertQueryHeader(parser, OrQueryBuilder.PROTOTYPE.getName()); context.indexQueryParserService().queryParser(OrQueryBuilder.PROTOTYPE.getName()).fromXContent(context); } + + @Test + public void testValidate() { + OrQueryBuilder orQuery = new OrQueryBuilder(); + int iters = randomIntBetween(0, 5); + int totalExpectedErrors = 0; + for (int i = 0; i < iters; i++) { + if (randomBoolean()) { + orQuery.add(RandomQueryBuilder.createInvalidQuery(random())); + totalExpectedErrors++; + } else { + orQuery.add(RandomQueryBuilder.createQuery(random())); + } + } + assertValidate(orQuery, totalExpectedErrors); + } } 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 93e1ca0b6df..85b3912da2e 100644 --- a/core/src/test/java/org/elasticsearch/index/query/QueryFilterBuilderTest.java +++ b/core/src/test/java/org/elasticsearch/index/query/QueryFilterBuilderTest.java @@ -40,7 +40,7 @@ public class QueryFilterBuilderTest extends BaseQueryTestCase