From de3c9a23e96febc1989f220098e6e6282dc34810 Mon Sep 17 00:00:00 2001 From: Simon Willnauer Date: Thu, 10 Sep 2015 12:08:50 +0200 Subject: [PATCH] Cleanup code and always use strict parsing on query tests --- .../index/query/HasParentQueryBuilder.java | 7 +++ .../index/query/NotQueryParser.java | 2 +- .../index/query/TermsQueryBuilder.java | 42 +++++++++----- .../index/query/TermsQueryParser.java | 11 +--- .../index/query/AbstractQueryTestCase.java | 24 ++++---- .../query/HasParentQueryBuilderTests.java | 24 ++++---- .../index/query/NotQueryBuilderTests.java | 28 ++++++--- .../query/QueryStringQueryBuilderTests.java | 2 +- .../index/query/TermsQueryBuilderTests.java | 57 +++++++++++++++---- 9 files changed, 125 insertions(+), 72 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/index/query/HasParentQueryBuilder.java b/core/src/main/java/org/elasticsearch/index/query/HasParentQueryBuilder.java index 4576101d635..8bcdff970eb 100644 --- a/core/src/main/java/org/elasticsearch/index/query/HasParentQueryBuilder.java +++ b/core/src/main/java/org/elasticsearch/index/query/HasParentQueryBuilder.java @@ -99,6 +99,13 @@ public class HasParentQueryBuilder extends AbstractQueryBuilder { } if (!queryFound) { - throw new QueryParsingException(parseContext, "filter is required when using `not` query"); + throw new QueryParsingException(parseContext, "query is required when using `not` query"); } NotQueryBuilder notQueryBuilder = new NotQueryBuilder(query); diff --git a/core/src/main/java/org/elasticsearch/index/query/TermsQueryBuilder.java b/core/src/main/java/org/elasticsearch/index/query/TermsQueryBuilder.java index 3f0ce9169b2..18922209cd8 100644 --- a/core/src/main/java/org/elasticsearch/index/query/TermsQueryBuilder.java +++ b/core/src/main/java/org/elasticsearch/index/query/TermsQueryBuilder.java @@ -39,10 +39,7 @@ import org.elasticsearch.index.mapper.MappedFieldType; import org.elasticsearch.indices.cache.query.terms.TermsLookup; import java.io.IOException; -import java.util.ArrayList; -import java.util.Arrays; -import java.util.List; -import java.util.Objects; +import java.util.*; /** * A filter for a field based on several terms matching on any of them. @@ -51,16 +48,29 @@ public class TermsQueryBuilder extends AbstractQueryBuilder { public static final String NAME = "terms"; - static final TermsQueryBuilder PROTOTYPE = new TermsQueryBuilder(null); + static final TermsQueryBuilder PROTOTYPE = new TermsQueryBuilder(""); public static final boolean DEFAULT_DISABLE_COORD = false; private final String fieldName; - private List values; + private final List values; + @Deprecated private String minimumShouldMatch; + @Deprecated private boolean disableCoord = DEFAULT_DISABLE_COORD; private TermsLookup termsLookup; + TermsQueryBuilder(String fieldName, List values, String minimumShouldMatch, boolean disableCoord, TermsLookup termsLookup) { + this.fieldName = fieldName; + if (values == null && termsLookup == null) { + throw new IllegalArgumentException("No value specified for terms query"); + } + this.values = values; + this.disableCoord = disableCoord; + this.minimumShouldMatch = minimumShouldMatch; + this.termsLookup = termsLookup; + } + /** * A filter for a field based on several terms matching on any of them. * @@ -128,6 +138,7 @@ public class TermsQueryBuilder extends AbstractQueryBuilder { */ public TermsQueryBuilder(String fieldName) { this.fieldName = fieldName; + this.values = null; } /** @@ -176,7 +187,7 @@ public class TermsQueryBuilder extends AbstractQueryBuilder { return this; } - public boolean disableCoord() { + boolean disableCoord() { return this.disableCoord; } @@ -308,7 +319,9 @@ public class TermsQueryBuilder extends AbstractQueryBuilder { if (minimumShouldMatch != null) { builder.field("minimum_should_match", minimumShouldMatch); } - builder.field("disable_coord", disableCoord); + if (disableCoord != DEFAULT_DISABLE_COORD) { + builder.field("disable_coord", disableCoord); + } printBoostAndQueryName(builder); builder.endObject(); } @@ -391,14 +404,15 @@ public class TermsQueryBuilder extends AbstractQueryBuilder { @SuppressWarnings("unchecked") @Override protected TermsQueryBuilder doReadFrom(StreamInput in) throws IOException { - TermsQueryBuilder termsQueryBuilder = new TermsQueryBuilder(in.readString()); + String field = in.readString(); + TermsLookup lookup = null; if (in.readBoolean()) { - termsQueryBuilder.termsLookup = TermsLookup.readTermsLookupFrom(in); + lookup = TermsLookup.readTermsLookupFrom(in); } - termsQueryBuilder.values = ((List) in.readGenericValue()); - termsQueryBuilder.minimumShouldMatch = in.readOptionalString(); - termsQueryBuilder.disableCoord = in.readBoolean(); - return termsQueryBuilder; + List values = (List) in.readGenericValue(); + String minimumShouldMatch = in.readOptionalString(); + boolean disableCoord = in.readBoolean(); + return new TermsQueryBuilder(field, values, minimumShouldMatch, disableCoord, lookup); } @Override diff --git a/core/src/main/java/org/elasticsearch/index/query/TermsQueryParser.java b/core/src/main/java/org/elasticsearch/index/query/TermsQueryParser.java index 350672a81ea..b1f949c1868 100644 --- a/core/src/main/java/org/elasticsearch/index/query/TermsQueryParser.java +++ b/core/src/main/java/org/elasticsearch/index/query/TermsQueryParser.java @@ -104,16 +104,7 @@ public class TermsQueryParser extends BaseQueryParser { if (fieldName == null) { throw new QueryParsingException(parseContext, "terms query requires a field name, followed by array of terms or a document lookup specification"); } - TermsQueryBuilder termsQueryBuilder; - if (values == null) { - termsQueryBuilder = new TermsQueryBuilder(fieldName); - } else { - termsQueryBuilder = new TermsQueryBuilder(fieldName, values); - } - return termsQueryBuilder - .disableCoord(disableCoord) - .minimumShouldMatch(minShouldMatch) - .termsLookup(termsLookup) + return new TermsQueryBuilder(fieldName, values, minShouldMatch, disableCoord, termsLookup) .boost(boost) .queryName(queryName); } diff --git a/core/src/test/java/org/elasticsearch/index/query/AbstractQueryTestCase.java b/core/src/test/java/org/elasticsearch/index/query/AbstractQueryTestCase.java index 80a0e2bc077..98aa3b2f20e 100644 --- a/core/src/test/java/org/elasticsearch/index/query/AbstractQueryTestCase.java +++ b/core/src/test/java/org/elasticsearch/index/query/AbstractQueryTestCase.java @@ -254,7 +254,7 @@ public abstract class AbstractQueryTestCase> QB testQuery = createTestQueryBuilder(); assertParsedQuery(testQuery.toString(), testQuery); for (Map.Entry alternateVersion : getAlternateVersions().entrySet()) { - assertParsedQuery(alternateVersion.getKey(), alternateVersion.getValue(), ParseFieldMatcher.EMPTY); + assertParsedQuery(alternateVersion.getKey(), alternateVersion.getValue()); } } @@ -270,7 +270,7 @@ public abstract class AbstractQueryTestCase> * Parses the query provided as string argument and compares it with the expected result provided as argument as a {@link QueryBuilder} */ protected void assertParsedQuery(String queryAsString, QueryBuilder expectedQuery) throws IOException { - assertParsedQuery(queryAsString, expectedQuery, getDefaultParseFieldMatcher()); + assertParsedQuery(queryAsString, expectedQuery, ParseFieldMatcher.STRICT); } protected void assertParsedQuery(String queryAsString, QueryBuilder expectedQuery, ParseFieldMatcher matcher) throws IOException { @@ -281,7 +281,7 @@ public abstract class AbstractQueryTestCase> } protected QueryBuilder parseQuery(String queryAsString) throws IOException { - return parseQuery(queryAsString, getDefaultParseFieldMatcher()); + return parseQuery(queryAsString, ParseFieldMatcher.STRICT); } protected QueryBuilder parseQuery(String queryAsString, ParseFieldMatcher matcher) throws IOException { @@ -292,16 +292,6 @@ public abstract class AbstractQueryTestCase> return context.parseInnerQueryBuilder(); } - /** - * Returns the default {@link ParseFieldMatcher} used for parsing non-alternative XContent representations. - * The default is {@link ParseFieldMatcher#STRICT}. - * Note: Queries returned from {@link #getAlternateVersions()} are always parsed with {@link ParseFieldMatcher#EMPTY} as they might - * not be backwards compatible. - */ - protected ParseFieldMatcher getDefaultParseFieldMatcher() { - return ParseFieldMatcher.STRICT; - } - /** * Test creates the {@link Query} from the {@link QueryBuilder} under test and delegates the * assertions being made on the result to the implementing subclass. @@ -384,6 +374,13 @@ public abstract class AbstractQueryTestCase> @Test public void testSerialization() throws IOException { QB testQuery = createTestQueryBuilder(); + assertSerialization(testQuery); + } + + /** + * Serialize the given query builder and asserts that both are equal + */ + protected QB assertSerialization(QB testQuery) throws IOException { try (BytesStreamOutput output = new BytesStreamOutput()) { testQuery.writeTo(output); try (StreamInput in = new NamedWriteableAwareStreamInput(StreamInput.wrap(output.bytes()), namedWriteableRegistry)) { @@ -392,6 +389,7 @@ public abstract class AbstractQueryTestCase> assertEquals(deserializedQuery, testQuery); assertEquals(deserializedQuery.hashCode(), testQuery.hashCode()); assertNotSame(deserializedQuery, testQuery); + return (QB) deserializedQuery; } } } diff --git a/core/src/test/java/org/elasticsearch/index/query/HasParentQueryBuilderTests.java b/core/src/test/java/org/elasticsearch/index/query/HasParentQueryBuilderTests.java index ee9e46e88ea..1236c784909 100644 --- a/core/src/test/java/org/elasticsearch/index/query/HasParentQueryBuilderTests.java +++ b/core/src/test/java/org/elasticsearch/index/query/HasParentQueryBuilderTests.java @@ -160,37 +160,35 @@ public class HasParentQueryBuilderTests extends AbstractQueryTestCase @Override protected Map getAlternateVersions() { Map alternateVersions = new HashMap<>(); - - NotQueryBuilder testQuery1 = new NotQueryBuilder(createTestQueryBuilder().innerQuery()); - String contentString1 = "{\n" + - " \"not\" : {\n" + - " \"filter\" : " + testQuery1.innerQuery().toString() + "\n" + - " }\n" + - "}"; - alternateVersions.put(contentString1, testQuery1); - QueryBuilder innerQuery = createTestQueryBuilder().innerQuery(); //not doesn't support empty query when query/filter element is not specified if (innerQuery != EmptyQueryBuilder.PROTOTYPE) { @@ -90,6 +82,24 @@ public class NotQueryBuilderTests extends AbstractQueryTestCase return alternateVersions; } + + public void testDeprecatedXContent() throws IOException { + String deprecatedJson = "{\n" + + " \"not\" : {\n" + + " \"filter\" : " + EmptyQueryBuilder.PROTOTYPE.toString() + "\n" + + " }\n" + + "}"; + try { + parseQuery(deprecatedJson); + fail("filter is deprecated"); + } catch (IllegalArgumentException ex) { + assertEquals("Deprecated field [filter] used, expected [query] instead", ex.getMessage()); + } + + NotQueryBuilder queryBuilder = (NotQueryBuilder) parseQuery(deprecatedJson, ParseFieldMatcher.EMPTY); + assertEquals(EmptyQueryBuilder.PROTOTYPE, queryBuilder.innerQuery()); + } + @Test public void testValidate() { QueryBuilder innerQuery = null; diff --git a/core/src/test/java/org/elasticsearch/index/query/QueryStringQueryBuilderTests.java b/core/src/test/java/org/elasticsearch/index/query/QueryStringQueryBuilderTests.java index aeed66a76e3..287acd0f072 100644 --- a/core/src/test/java/org/elasticsearch/index/query/QueryStringQueryBuilderTests.java +++ b/core/src/test/java/org/elasticsearch/index/query/QueryStringQueryBuilderTests.java @@ -35,7 +35,7 @@ import static org.hamcrest.CoreMatchers.equalTo; import static org.hamcrest.CoreMatchers.instanceOf; import static org.hamcrest.Matchers.*; -public class QueryStringQueryBuilderTests extends BaseQueryTestCase { +public class QueryStringQueryBuilderTests extends AbstractQueryTestCase { @Override protected QueryStringQueryBuilder doCreateTestQueryBuilder() { diff --git a/core/src/test/java/org/elasticsearch/index/query/TermsQueryBuilderTests.java b/core/src/test/java/org/elasticsearch/index/query/TermsQueryBuilderTests.java index 9d9aeb536f3..ddd149176ce 100644 --- a/core/src/test/java/org/elasticsearch/index/query/TermsQueryBuilderTests.java +++ b/core/src/test/java/org/elasticsearch/index/query/TermsQueryBuilderTests.java @@ -19,6 +19,7 @@ package org.elasticsearch.index.query; +import com.carrotsearch.randomizedtesting.generators.RandomPicks; import org.apache.lucene.index.Term; import org.apache.lucene.search.BooleanClause; import org.apache.lucene.search.BooleanQuery; @@ -34,6 +35,7 @@ import org.junit.Test; import java.io.IOException; import java.util.ArrayList; +import java.util.Arrays; import java.util.Iterator; import java.util.List; @@ -49,11 +51,6 @@ public class TermsQueryBuilderTests extends AbstractQueryTestCase randomTerms = new ArrayList<>();