From 9dee07be3711c17ee9c39ee2cc388f52636deeb7 Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Tue, 5 Apr 2016 14:55:43 -0400 Subject: [PATCH] Fixup --- .../index/query/AbstractQueryBuilder.java | 4 +- .../index/query/GeohashCellQuery.java | 12 ++-- .../index/query/MatchQueryBuilder.java | 6 +- .../index/query/MultiMatchQueryBuilder.java | 33 ++++++----- .../index/query/QueryBuilder.java | 59 ------------------- .../functionscore/DecayFunctionBuilder.java | 6 +- .../FunctionScoreQueryBuilder.java | 28 ++++----- .../elasticsearch/search/SearchModule.java | 5 +- .../index/query/AbstractQueryTestCase.java | 49 +-------------- .../search/SearchModuleTests.java | 9 +-- 10 files changed, 48 insertions(+), 163 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 7f38712a6829f..39c835e4b0949 100644 --- a/core/src/main/java/org/elasticsearch/index/query/AbstractQueryBuilder.java +++ b/core/src/main/java/org/elasticsearch/index/query/AbstractQueryBuilder.java @@ -34,18 +34,16 @@ import java.io.IOException; import java.util.ArrayList; -import java.util.Arrays; import java.util.Collection; import java.util.List; import java.util.Objects; -import static java.util.Collections.unmodifiableList; - /** * Base class for all classes producing lucene queries. * Supports conversion to BytesReference and creation of lucene Query objects. */ public abstract class AbstractQueryBuilder> extends ToXContentToBytes implements QueryBuilder { + /** Default for boost to apply to resulting Lucene query. Defaults to 1.0*/ public static final float DEFAULT_BOOST = 1.0f; public static final ParseField NAME_FIELD = new ParseField("_name"); diff --git a/core/src/main/java/org/elasticsearch/index/query/GeohashCellQuery.java b/core/src/main/java/org/elasticsearch/index/query/GeohashCellQuery.java index badd54f1202cc..1f5aa537c0622 100644 --- a/core/src/main/java/org/elasticsearch/index/query/GeohashCellQuery.java +++ b/core/src/main/java/org/elasticsearch/index/query/GeohashCellQuery.java @@ -60,6 +60,7 @@ */ public class GeohashCellQuery { + public static final String NAME = "geohash_cell"; public static final ParseField NEIGHBORS_FIELD = new ParseField("neighbors"); public static final ParseField PRECISION_FIELD = new ParseField("precision"); public static final boolean DEFAULT_NEIGHBORS = false; @@ -95,8 +96,6 @@ public static Query create(QueryShardContext context, BaseGeoPointFieldMapper.Ge * false. */ public static class Builder extends AbstractQueryBuilder { - public static final String NAME = "geohash_cell"; - // we need to store the geohash rather than the corresponding point, // because a transformation from a geohash to a point an back to the // geohash will extend the accuracy of the hash to max precision @@ -272,7 +271,7 @@ public String getWriteableName() { public static class Parser implements QueryParser { - public static final ParseField QUERY_NAME_FIELD = new ParseField(Builder.NAME); + public static final ParseField QUERY_NAME_FIELD = new ParseField(NAME); @Inject public Parser() { @@ -291,8 +290,7 @@ public Builder fromXContent(QueryParseContext parseContext) throws IOException { XContentParser.Token token; if ((token = parser.currentToken()) != Token.START_OBJECT) { - throw new ElasticsearchParseException("failed to parse [{}] query. expected an object but found [{}] instead", Builder.NAME, - token); + throw new ElasticsearchParseException("failed to parse [{}] query. expected an object but found [{}] instead", NAME, token); } while ((token = parser.nextToken()) != Token.END_OBJECT) { @@ -336,12 +334,12 @@ public Builder fromXContent(QueryParseContext parseContext) throws IOException { geohash = GeoUtils.parseGeoPoint(parser).geohash(); } } else { - throw new ParsingException(parser.getTokenLocation(), "[" + Builder.NAME + + throw new ParsingException(parser.getTokenLocation(), "[" + NAME + "] field name already set to [" + fieldName + "] but found [" + field + "]"); } } } else { - throw new ElasticsearchParseException("failed to parse [{}] query. unexpected token [{}]", Builder.NAME, token); + throw new ElasticsearchParseException("failed to parse [{}] query. unexpected token [{}]", NAME, token); } } Builder builder = new Builder(fieldName, geohash); diff --git a/core/src/main/java/org/elasticsearch/index/query/MatchQueryBuilder.java b/core/src/main/java/org/elasticsearch/index/query/MatchQueryBuilder.java index 7f34131af742f..7c73d9f72c75d 100644 --- a/core/src/main/java/org/elasticsearch/index/query/MatchQueryBuilder.java +++ b/core/src/main/java/org/elasticsearch/index/query/MatchQueryBuilder.java @@ -525,8 +525,7 @@ public static MatchQueryBuilder fromXContent(QueryParseContext parseContext) thr XContentParser.Token token = parser.nextToken(); if (token != XContentParser.Token.FIELD_NAME) { - throw new ParsingException(parser.getTokenLocation(), - "[" + MatchQueryBuilder.NAME + "] query malformed, no field"); + throw new ParsingException(parser.getTokenLocation(), "[" + MatchQueryBuilder.NAME + "] query malformed, no field"); } String fieldName = parser.currentName(); @@ -565,8 +564,7 @@ public static MatchQueryBuilder fromXContent(QueryParseContext parseContext) thr } else if ("phrase_prefix".equals(tStr) || ("phrasePrefix".equals(tStr))) { type = MatchQuery.Type.PHRASE_PREFIX; } else { - throw new ParsingException(parser.getTokenLocation(), - "[" + NAME + "] query does not support type " + tStr); + throw new ParsingException(parser.getTokenLocation(), "[" + NAME + "] query does not support type " + tStr); } } else if (parseContext.parseFieldMatcher().match(currentFieldName, ANALYZER_FIELD)) { analyzer = parser.text(); diff --git a/core/src/main/java/org/elasticsearch/index/query/MultiMatchQueryBuilder.java b/core/src/main/java/org/elasticsearch/index/query/MultiMatchQueryBuilder.java index 621e539baa041..f89e0f22de21c 100644 --- a/core/src/main/java/org/elasticsearch/index/query/MultiMatchQueryBuilder.java +++ b/core/src/main/java/org/elasticsearch/index/query/MultiMatchQueryBuilder.java @@ -49,8 +49,7 @@ * Same as {@link MatchQueryBuilder} but supports multiple fields. */ public class MultiMatchQueryBuilder extends AbstractQueryBuilder { - public static final Names NAMES = new Names("multi_match", "multiMatch"); - public static final MultiMatchQueryBuilder PROTOTYPE = new MultiMatchQueryBuilder(""); + public static final String NAME = "multi_match"; public static final MultiMatchQueryBuilder.Type DEFAULT_TYPE = MultiMatchQueryBuilder.Type.BEST_FIELDS; public static final Operator DEFAULT_OPERATOR = Operator.OR; @@ -60,7 +59,7 @@ public class MultiMatchQueryBuilder extends AbstractQueryBuilder { /** @@ -161,7 +162,7 @@ public static Type parse(String value, ParseFieldMatcher parseFieldMatcher) { } } if (type == null) { - throw new ElasticsearchParseException("failed to parse [{}] query type [{}]. unknown type.", NAMES.primary(), value); + throw new ElasticsearchParseException("failed to parse [{}] query type [{}]. unknown type.", NAME, value); } return type; } @@ -193,10 +194,10 @@ public MultiMatchQueryBuilder.Type getType() { */ public MultiMatchQueryBuilder(Object value, String... fields) { if (value == null) { - throw new IllegalArgumentException("[" + NAMES.primary() + "] requires query value"); + throw new IllegalArgumentException("[" + NAME + "] requires query value"); } if (fields == null) { - throw new IllegalArgumentException("[" + NAMES.primary() + "] requires fields at initialization time"); + throw new IllegalArgumentException("[" + NAME + "] requires fields at initialization time"); } this.value = value; this.fieldsBoosts = new TreeMap<>(); @@ -248,7 +249,7 @@ public Map fields() { */ public MultiMatchQueryBuilder type(MultiMatchQueryBuilder.Type type) { if (type == null) { - throw new IllegalArgumentException("[" + NAMES.primary() + "] requires type to be non-null"); + throw new IllegalArgumentException("[" + NAME + "] requires type to be non-null"); } this.type = type; return this; @@ -259,7 +260,7 @@ public MultiMatchQueryBuilder type(MultiMatchQueryBuilder.Type type) { */ public MultiMatchQueryBuilder type(Object type) { if (type == null) { - throw new IllegalArgumentException("[" + NAMES.primary() + "] requires type to be non-null"); + throw new IllegalArgumentException("[" + NAME + "] requires type to be non-null"); } this.type = Type.parse(type.toString().toLowerCase(Locale.ROOT), ParseFieldMatcher.EMPTY); return this; @@ -274,7 +275,7 @@ public Type type() { */ public MultiMatchQueryBuilder operator(Operator operator) { if (operator == null) { - throw new IllegalArgumentException("[" + NAMES.primary() + "] requires operator to be non-null"); + throw new IllegalArgumentException("[" + NAME + "] requires operator to be non-null"); } this.operator = operator; return this; @@ -462,7 +463,7 @@ public Float cutoffFrequency() { public MultiMatchQueryBuilder zeroTermsQuery(MatchQuery.ZeroTermsQuery zeroTermsQuery) { if (zeroTermsQuery == null) { - throw new IllegalArgumentException("[" + NAMES.primary() + "] requires zero terms query to be non-null"); + throw new IllegalArgumentException("[" + NAME + "] requires zero terms query to be non-null"); } this.zeroTermsQuery = zeroTermsQuery; return this; @@ -474,7 +475,7 @@ public MatchQuery.ZeroTermsQuery zeroTermsQuery() { @Override public void doXContent(XContentBuilder builder, Params params) throws IOException { - builder.startObject(NAMES.primary()); + builder.startObject(NAME); builder.field(QUERY_FIELD.getPreferredName(), value); builder.startArray(FIELDS_FIELD.getPreferredName()); for (Map.Entry fieldEntry : this.fieldsBoosts.entrySet()) { @@ -550,7 +551,7 @@ public static MultiMatchQueryBuilder fromXContent(QueryParseContext parseContext parseFieldAndBoost(parser, fieldsBoosts); } else { throw new ParsingException(parser.getTokenLocation(), - "[" + NAMES.primary() + "] query does not support [" + currentFieldName + "]"); + "[" + NAME + "] query does not support [" + currentFieldName + "]"); } } else if (token.isValue()) { if (parseContext.parseFieldMatcher().match(currentFieldName, QUERY_FIELD)) { @@ -596,11 +597,11 @@ public static MultiMatchQueryBuilder fromXContent(QueryParseContext parseContext queryName = parser.text(); } else { throw new ParsingException(parser.getTokenLocation(), - "[" + NAMES.primary() + "] query does not support [" + currentFieldName + "]"); + "[" + NAME + "] query does not support [" + currentFieldName + "]"); } } else { throw new ParsingException(parser.getTokenLocation(), - "[" + NAMES.primary() + "] unknown token [" + token + "] after [" + currentFieldName + "]"); + "[" + NAME + "] unknown token [" + token + "] after [" + currentFieldName + "]"); } } @@ -653,7 +654,7 @@ private static void parseFieldAndBoost(XContentParser parser, Map @Override public String getWriteableName() { - return NAMES.primary(); + return NAME; } @Override @@ -661,7 +662,7 @@ protected Query doToQuery(QueryShardContext context) throws IOException { MultiMatchQuery multiMatchQuery = new MultiMatchQuery(context); if (analyzer != null) { if (context.getAnalysisService().analyzer(analyzer) == null) { - throw new QueryShardException(context, "[" + NAMES.primary() + "] analyzer [" + analyzer + "] not found"); + throw new QueryShardException(context, "[" + NAME + "] analyzer [" + analyzer + "] not found"); } multiMatchQuery.setAnalyzer(analyzer); } diff --git a/core/src/main/java/org/elasticsearch/index/query/QueryBuilder.java b/core/src/main/java/org/elasticsearch/index/query/QueryBuilder.java index 8cd726b7952ef..f8010e7b523c7 100644 --- a/core/src/main/java/org/elasticsearch/index/query/QueryBuilder.java +++ b/core/src/main/java/org/elasticsearch/index/query/QueryBuilder.java @@ -21,18 +21,9 @@ import org.apache.lucene.search.Query; import org.elasticsearch.common.io.stream.NamedWriteable; -import org.elasticsearch.common.util.iterable.Iterables; import org.elasticsearch.common.xcontent.ToXContent; -import org.elasticsearch.common.xcontent.XContentBuilder; import java.io.IOException; -import java.util.Arrays; -import java.util.HashSet; -import java.util.Set; - -import static java.util.Collections.singleton; -import static java.util.Collections.unmodifiableList; -import static java.util.Collections.unmodifiableSet; public interface QueryBuilder> extends NamedWriteable, ToXContent { @@ -105,54 +96,4 @@ static QueryBuilder rewriteQuery(QueryBuilder original, QueryRewriteContex return builder; } - /** - * Names under which the QueryBuilder is registered. QueryBuilders should have a public static final NAMES member declaring all the - * names that the query uses. - */ - public static class Names { - private final String primary; - private final Set all; - - /** - * Names under which the QueryBuilder is registered. - * - * @param primary this is the name that the query builder outputs with - * {@link QueryBuilder#toXContent(XContentBuilder, org.elasticsearch.common.xcontent.ToXContent.Params)} and that it uses as - * it's {@link QueryBuilder#getWriteableName()}. - * @param alternatives names that can be used for the query in XContent. Sometimes these are just deprecated names for the same - * query like multiMatch is a deprecated name for multi_match. Sometimes these alternate forms that - * effect parsing like match_phrase is just a match query with some settings tweaked. - */ - public Names(String primary, String... alternatives) { - this.primary = primary; - Set all = new HashSet<>(alternatives.length + 1); - all.add(primary); - for (String alternative: alternatives) { - boolean added = all.add(alternative); - if (false == added) { - throw new IllegalArgumentException( - "Alternative name [" + alternative + "] is listed twice or is the same as a primary name"); - } - } - this.all = unmodifiableSet(all); - } - - /** - * Fetch the primary name of this query. This is the name that the query builder outputs with - * {@link QueryBuilder#toXContent(XContentBuilder, org.elasticsearch.common.xcontent.ToXContent.Params)} and that it - * uses as it's {@link QueryBuilder#getWriteableName()}. - */ - public String primary() { - return primary; - } - - /** - * All names the might refer to this query builder in XContent. Sometimes these are just deprecated names for the same query like - * multiMatch is a deprecated name for multi_match. Sometimes these alternate forms that effect parsing - * like match_phrase is just a match query with some settings tweaked. - */ - public Set all() { - return all; - } - } } diff --git a/core/src/main/java/org/elasticsearch/index/query/functionscore/DecayFunctionBuilder.java b/core/src/main/java/org/elasticsearch/index/query/functionscore/DecayFunctionBuilder.java index 7a23a26918354..7ef44c3279e67 100644 --- a/core/src/main/java/org/elasticsearch/index/query/functionscore/DecayFunctionBuilder.java +++ b/core/src/main/java/org/elasticsearch/index/query/functionscore/DecayFunctionBuilder.java @@ -483,15 +483,15 @@ public AbstractDistanceScoreFunction(double userSuppiedScale, double decay, doub super(CombineFunction.MULTIPLY); this.mode = mode; if (userSuppiedScale <= 0.0) { - throw new IllegalArgumentException(FunctionScoreQueryBuilder.NAMES.primary() + " : scale must be > 0.0."); + throw new IllegalArgumentException(FunctionScoreQueryBuilder.NAME + " : scale must be > 0.0."); } if (decay <= 0.0 || decay >= 1.0) { - throw new IllegalArgumentException(FunctionScoreQueryBuilder.NAMES.primary() + " : decay must be in the range [0..1]."); + throw new IllegalArgumentException(FunctionScoreQueryBuilder.NAME + " : decay must be in the range [0..1]."); } this.scale = func.processScale(userSuppiedScale, decay); this.func = func; if (offset < 0.0d) { - throw new IllegalArgumentException(FunctionScoreQueryBuilder.NAMES.primary() + " : offset must be > 0.0"); + throw new IllegalArgumentException(FunctionScoreQueryBuilder.NAME + " : offset must be > 0.0"); } this.offset = offset; } diff --git a/core/src/main/java/org/elasticsearch/index/query/functionscore/FunctionScoreQueryBuilder.java b/core/src/main/java/org/elasticsearch/index/query/functionscore/FunctionScoreQueryBuilder.java index f69e3f51b8821..95683fb200aaa 100644 --- a/core/src/main/java/org/elasticsearch/index/query/functionscore/FunctionScoreQueryBuilder.java +++ b/core/src/main/java/org/elasticsearch/index/query/functionscore/FunctionScoreQueryBuilder.java @@ -61,7 +61,7 @@ public class FunctionScoreQueryBuilder extends AbstractQueryBuilder> void registerQuery(Writeable.Reader rea /** * Register a query via it's parser's prototype. - * TODO remove this in favor of registerQuery - * TODO remove AbstractQueryTestCase's getSupportedNames leniency when we remove this method + * TODO remove this in favor of registerQuery and merge innerRegisterQueryParser into registerQuery */ public void registerQueryParser(QueryParser queryParser, ParseField queryName) { innerRegisterQueryParser(queryParser, 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 4b333a4e70223..3004cce404480 100644 --- a/core/src/test/java/org/elasticsearch/index/query/AbstractQueryTestCase.java +++ b/core/src/test/java/org/elasticsearch/index/query/AbstractQueryTestCase.java @@ -20,7 +20,6 @@ package org.elasticsearch.index.query; import com.carrotsearch.randomizedtesting.generators.CodepointSetGenerator; -import com.carrotsearch.randomizedtesting.generators.RandomPicks; import com.fasterxml.jackson.core.JsonParseException; import com.fasterxml.jackson.core.io.JsonStringEncoder; @@ -63,7 +62,6 @@ import org.elasticsearch.common.xcontent.ToXContent; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentFactory; -import org.elasticsearch.common.xcontent.XContentHelper; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.common.xcontent.XContentType; import org.elasticsearch.env.Environment; @@ -116,7 +114,6 @@ import java.lang.reflect.Method; import java.lang.reflect.Proxy; import java.util.ArrayList; -import java.util.Collection; import java.util.Collections; import java.util.HashSet; import java.util.List; @@ -125,7 +122,6 @@ import java.util.Set; import java.util.concurrent.ExecutionException; -import static java.util.Collections.singleton; import static org.elasticsearch.cluster.service.ClusterServiceUtils.createClusterService; import static org.elasticsearch.cluster.service.ClusterServiceUtils.setState; import static org.hamcrest.Matchers.containsString; @@ -386,8 +382,8 @@ public void testFromXContent() throws IOException { for (int runs = 0; runs < NUMBER_OF_TESTQUERIES; runs++) { QB testQuery = createTestQueryBuilder(); XContentBuilder builder = toXContent(testQuery, randomFrom(XContentType.values())); - builder = randomizeName(shuffleXContent(builder, shuffleProtectedFields()), testQuery); - assertParsedQuery(builder.bytes(), testQuery); + XContentBuilder shuffled = shuffleXContent(builder, shuffleProtectedFields()); + assertParsedQuery(shuffled.bytes(), testQuery); for (Map.Entry alternateVersion : getAlternateVersions().entrySet()) { String queryAsString = alternateVersion.getKey(); assertParsedQuery(new BytesArray(queryAsString), alternateVersion.getValue(), ParseFieldMatcher.EMPTY); @@ -412,47 +408,6 @@ protected static XContentBuilder toXContent(QueryBuilder query, XContentType return builder; } - /** - * Randomize the name of a query builder. Note that this can change the meaning of the query builder. Like match can turn - * into match_phrase. - */ - private XContentBuilder randomizeName(XContentBuilder builder, QueryBuilder query) throws IOException { - String newName = RandomPicks.randomFrom(random(), getSupportedNames(query)); - if (newName.equals(query.getName())) { - return builder; - } - Tuple> t = XContentHelper.convertToMap(builder.bytes(), true); - Object queryAsMap = t.v2().remove(query.getName()); - if (queryAsMap == null) { - throw new IllegalArgumentException( - "Test query claimed to have name [" + query.getName() + "] but instead wrote [" + t.v2() + "]"); - } - t.v2().put(newName, queryAsMap); - builder = XContentFactory.contentBuilder(t.v1()); - builder.map(t.v2()); - logger.debug("Switched from [{}] to [{}]", query.getWriteableName(), newName); - return builder; - } - - /** - * Get a list of all supported names for a query builder class. Uses reflection and naming conventions so it is brittle. - */ - protected Collection getSupportedNames(QueryBuilder query) { - try { - return ((QueryBuilder.Names) query.getClass().getField("NAMES").get(null)).all(); - } catch (Exception pluralException) { - // TODO remove this backup when we remove support for registering by prototype - try { - return singleton((String) query.getClass().getField("NAME").get(null)); - } catch (Exception singularException) { - IllegalArgumentException e = new IllegalArgumentException("Expected a public static void NAMES field"); - e.addSuppressed(pluralException); - throw e; - } - } - } - - /** * Test that unknown field trigger ParsingException. * To find the right position in the root query, we add a marker as `queryName` which diff --git a/core/src/test/java/org/elasticsearch/search/SearchModuleTests.java b/core/src/test/java/org/elasticsearch/search/SearchModuleTests.java index 54543dc2c28a2..87e51000fb289 100644 --- a/core/src/test/java/org/elasticsearch/search/SearchModuleTests.java +++ b/core/src/test/java/org/elasticsearch/search/SearchModuleTests.java @@ -80,12 +80,9 @@ public void testRegisterHighlighter() { public void testRegisterQueryParserDuplicate() { SearchModule module = new SearchModule(Settings.EMPTY, new NamedWriteableRegistry()); - try { - module.registerQueryParser(new TermQueryParser(), TermQueryParser.QUERY_NAME_FIELD); - } catch (IllegalArgumentException e) { - assertThat(e.getMessage(), - containsString("already registered for name [term] while trying to register [org.elasticsearch.index.")); - } + IllegalArgumentException e = expectThrows(IllegalArgumentException.class, + () -> module.registerQueryParser(new TermQueryParser(), TermQueryParser.QUERY_NAME_FIELD)); + assertThat(e.getMessage(), containsString("already registered for name [term] while trying to register [org.elasticsearch.index.")); } public void testRegisteredQueries() {