From 5fcc648047700464f5b5bafb546d252b1686159d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20B=C3=BCscher?= Date: Tue, 20 Oct 2015 22:53:26 +0200 Subject: [PATCH] Tests: Check exception from query parsers for unknown field Most query parsers throw a ParsingException when they trying to parse a field with an unknown name. This adds a generic check for this to the AbstractQueryTestCase so the behaviour gets tested for all query parsers. The test works by first making sure the test query has a `boost` field and then changing this to an unknown field name and checking for an error. There are exceptions to this for WrapperQueryBuilder and QueryFilterBuilder, because here the parser only expects the wrapped `query` element. MatchNoneQueryBuilder and MatchAllQueryBuilder so far had setters for boost() and queryName() but didn't render them, which is added here for consistency. GeoDistance, GeoDistanceRange and GeoHashCellQuery so far treat unknown field names in the json as the target field name of the center point of the query, which so far was handled by overwriting points previously specified in the query. This is changed here so that an attempt to use two different field names to specify the central point of the query throws a ParsingException Relates to #10974 --- .../index/query/GeoDistanceQueryParser.java | 9 ++- .../query/GeoDistanceRangeQueryParser.java | 64 ++++++++++++++----- .../index/query/GeohashCellQuery.java | 27 +++++--- .../index/query/MatchNoneQueryBuilder.java | 6 +- .../index/query/MatchNoneQueryParser.java | 24 +++++-- .../index/query/TypeQueryParser.java | 6 +- .../index/query/WrapperQueryParser.java | 2 +- .../index/query/AbstractQueryTestCase.java | 29 ++++++++- .../query/ConstantScoreQueryBuilderTests.java | 5 ++ .../query/MatchNoneQueryBuilderTests.java | 11 +--- .../query/TemplateQueryBuilderTests.java | 22 +++++-- .../index/query/WrapperQueryBuilderTests.java | 16 +++++ 12 files changed, 168 insertions(+), 53 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/index/query/GeoDistanceQueryParser.java b/core/src/main/java/org/elasticsearch/index/query/GeoDistanceQueryParser.java index da9a7c2f07e..3cf4426394e 100644 --- a/core/src/main/java/org/elasticsearch/index/query/GeoDistanceQueryParser.java +++ b/core/src/main/java/org/elasticsearch/index/query/GeoDistanceQueryParser.java @@ -130,8 +130,13 @@ public class GeoDistanceQueryParser implements QueryParser field : { lat : 30, lon : 12 } - fieldName = currentFieldName; - if (point == null) { - point = new GeoPoint(); + if (fieldName == null) { + fieldName = currentFieldName; + if (point == null) { + point = new GeoPoint(); + } + GeoUtils.parseGeoPoint(parser, point); + } else { + throw new ParsingException(parser.getTokenLocation(), "[" + GeoDistanceRangeQueryBuilder.NAME + + "] field name already set to [" + fieldName + "] but found [" + currentFieldName + "]"); } - GeoUtils.parseGeoPoint(parser, point); } else if (token.isValue()) { if (parseContext.parseFieldMatcher().match(currentFieldName, FROM_FIELD)) { if (token == XContentParser.Token.VALUE_NULL) { @@ -162,20 +173,38 @@ public class GeoDistanceRangeQueryParser implements QueryParser 0) { - geohash = GeoUtils.parseGeoPoint(parser).geohash(); + if (fieldName == null) { + fieldName = field; + token = parser.nextToken(); + if (token == Token.VALUE_STRING) { + // A string indicates either a geohash or a + // lat/lon + // string + String location = parser.text(); + if (location.indexOf(",") > 0) { + geohash = GeoUtils.parseGeoPoint(parser).geohash(); + } else { + geohash = location; + } } else { - geohash = location; + geohash = GeoUtils.parseGeoPoint(parser).geohash(); } } else { - geohash = GeoUtils.parseGeoPoint(parser).geohash(); + throw new ParsingException(parser.getTokenLocation(), "[" + NAME + + "] field name already set to [" + fieldName + "] but found [" + field + "]"); } } } else { diff --git a/core/src/main/java/org/elasticsearch/index/query/MatchNoneQueryBuilder.java b/core/src/main/java/org/elasticsearch/index/query/MatchNoneQueryBuilder.java index 0c466a43d10..4fc96f37f7a 100644 --- a/core/src/main/java/org/elasticsearch/index/query/MatchNoneQueryBuilder.java +++ b/core/src/main/java/org/elasticsearch/index/query/MatchNoneQueryBuilder.java @@ -39,6 +39,7 @@ public class MatchNoneQueryBuilder extends AbstractQueryBuilder public MatchNoneQueryBuilder fromXContent(QueryParseContext parseContext) throws IOException { XContentParser parser = parseContext.parser(); - XContentParser.Token token = parser.nextToken(); - if (token != XContentParser.Token.END_OBJECT) { - throw new ParsingException(parser.getTokenLocation(), "[match_none] query malformed"); + String currentFieldName = null; + XContentParser.Token token; + String queryName = null; + float boost = AbstractQueryBuilder.DEFAULT_BOOST; + while (((token = parser.nextToken()) != XContentParser.Token.END_OBJECT && token != XContentParser.Token.END_ARRAY)) { + if (token == XContentParser.Token.FIELD_NAME) { + currentFieldName = parser.currentName(); + } else if (token.isValue()) { + if ("_name".equals(currentFieldName)) { + queryName = parser.text(); + } else if ("boost".equals(currentFieldName)) { + boost = parser.floatValue(); + } else { + throw new ParsingException(parser.getTokenLocation(), "["+MatchNoneQueryBuilder.NAME+"] query does not support [" + currentFieldName + "]"); + } + } } - return new MatchNoneQueryBuilder(); + MatchNoneQueryBuilder matchNoneQueryBuilder = new MatchNoneQueryBuilder(); + matchNoneQueryBuilder.boost(boost); + matchNoneQueryBuilder.queryName(queryName); + return matchNoneQueryBuilder; } @Override diff --git a/core/src/main/java/org/elasticsearch/index/query/TypeQueryParser.java b/core/src/main/java/org/elasticsearch/index/query/TypeQueryParser.java index e2b4e13c65e..10adc0c7390 100644 --- a/core/src/main/java/org/elasticsearch/index/query/TypeQueryParser.java +++ b/core/src/main/java/org/elasticsearch/index/query/TypeQueryParser.java @@ -55,14 +55,16 @@ public class TypeQueryParser implements QueryParser { boost = parser.floatValue(); } else if ("value".equals(currentFieldName)) { type = parser.utf8Bytes(); + } else { + throw new ParsingException(parser.getTokenLocation(), "[" + TypeQueryBuilder.NAME + "] filter doesn't support [" + currentFieldName + "]"); } } else { - throw new ParsingException(parser.getTokenLocation(), "[type] filter doesn't support [" + currentFieldName + "]"); + throw new ParsingException(parser.getTokenLocation(), "[" + TypeQueryBuilder.NAME + "] filter doesn't support [" + currentFieldName + "]"); } } if (type == null) { - throw new ParsingException(parser.getTokenLocation(), "[type] filter needs to be provided with a value for the type"); + throw new ParsingException(parser.getTokenLocation(), "[" + TypeQueryBuilder.NAME + "] filter needs to be provided with a value for the type"); } return new TypeQueryBuilder(type) .boost(boost) diff --git a/core/src/main/java/org/elasticsearch/index/query/WrapperQueryParser.java b/core/src/main/java/org/elasticsearch/index/query/WrapperQueryParser.java index 59c570e97f9..ce5cdba883d 100644 --- a/core/src/main/java/org/elasticsearch/index/query/WrapperQueryParser.java +++ b/core/src/main/java/org/elasticsearch/index/query/WrapperQueryParser.java @@ -44,7 +44,7 @@ public class WrapperQueryParser implements QueryParser { } String fieldName = parser.currentName(); if (!fieldName.equals("query")) { - throw new ParsingException(parser.getTokenLocation(), "[wrapper] query malformed"); + throw new ParsingException(parser.getTokenLocation(), "[wrapper] query malformed, expected `query` but was" + fieldName); } parser.nextToken(); 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 677da47a2de..212dd457171 100644 --- a/core/src/test/java/org/elasticsearch/index/query/AbstractQueryTestCase.java +++ b/core/src/test/java/org/elasticsearch/index/query/AbstractQueryTestCase.java @@ -37,6 +37,7 @@ import org.elasticsearch.cluster.ClusterState; import org.elasticsearch.cluster.metadata.IndexMetaData; import org.elasticsearch.cluster.metadata.MetaData; import org.elasticsearch.common.ParseFieldMatcher; +import org.elasticsearch.common.ParsingException; import org.elasticsearch.common.bytes.BytesReference; import org.elasticsearch.common.collect.Tuple; import org.elasticsearch.common.compress.CompressedXContent; @@ -100,6 +101,7 @@ import java.util.concurrent.ExecutionException; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.not; +import static org.hamcrest.Matchers.containsString; public abstract class AbstractQueryTestCase> extends ESTestCase { @@ -321,6 +323,29 @@ public abstract class AbstractQueryTestCase> } } + /** + * Test that unknown field trigger ParsingException. + * To find the right position in the root query, we add a marker as `queryName` which + * all query builders support. The added bogus field after that should trigger the exception. + * Queries that allow arbitrary field names at this level need to override this test. + */ + public void testUnknownField() throws IOException { + String marker = "#marker#"; + QB testQuery; + do { + testQuery = createTestQueryBuilder(); + } while (testQuery.toString().contains(marker)); + testQuery.queryName(marker); // to find root query to add additional bogus field there + String queryAsString = testQuery.toString().replace("\"" + marker + "\"", "\"" + marker + "\", \"bogusField\" : \"someValue\""); + try { + parseQuery(queryAsString); + fail("ParsingException expected."); + } catch (ParsingException e) { + // we'd like to see the offending field name here + assertThat(e.getMessage(), containsString("bogusField")); + } + } + /** * Returns alternate string representation of the query that need to be tested as they are never used as output * of {@link QueryBuilder#toXContent(XContentBuilder, ToXContent.Params)}. By default there are no alternate versions. @@ -361,7 +386,9 @@ public abstract class AbstractQueryTestCase> QueryParseContext context = createParseContext(); context.reset(parser); context.parseFieldMatcher(matcher); - return context.parseInnerQueryBuilder(); + QueryBuilder parseInnerQueryBuilder = context.parseInnerQueryBuilder(); + assertTrue(parser.nextToken() == null); + return parseInnerQueryBuilder; } /** diff --git a/core/src/test/java/org/elasticsearch/index/query/ConstantScoreQueryBuilderTests.java b/core/src/test/java/org/elasticsearch/index/query/ConstantScoreQueryBuilderTests.java index 44bcbee27b2..f2a529cefd3 100644 --- a/core/src/test/java/org/elasticsearch/index/query/ConstantScoreQueryBuilderTests.java +++ b/core/src/test/java/org/elasticsearch/index/query/ConstantScoreQueryBuilderTests.java @@ -71,4 +71,9 @@ public class ConstantScoreQueryBuilderTests extends AbstractQueryTestCase { @Override - protected boolean supportsBoostAndQueryName() { - return false; - } - - @Override - protected AbstractQueryBuilder doCreateTestQueryBuilder() { + protected MatchNoneQueryBuilder doCreateTestQueryBuilder() { return new MatchNoneQueryBuilder(); } @Override - protected void doAssertLuceneQuery(AbstractQueryBuilder queryBuilder, Query query, QueryShardContext context) throws IOException { + protected void doAssertLuceneQuery(MatchNoneQueryBuilder queryBuilder, Query query, QueryShardContext context) throws IOException { assertThat(query, instanceOf(BooleanQuery.class)); BooleanQuery booleanQuery = (BooleanQuery) query; assertThat(booleanQuery.clauses().size(), equalTo(0)); diff --git a/core/src/test/java/org/elasticsearch/index/query/TemplateQueryBuilderTests.java b/core/src/test/java/org/elasticsearch/index/query/TemplateQueryBuilderTests.java index 3039cc11103..12ac7709bfd 100644 --- a/core/src/test/java/org/elasticsearch/index/query/TemplateQueryBuilderTests.java +++ b/core/src/test/java/org/elasticsearch/index/query/TemplateQueryBuilderTests.java @@ -23,6 +23,7 @@ import org.apache.lucene.search.Query; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentFactory; import org.elasticsearch.common.xcontent.XContentType; +import org.elasticsearch.script.Script.ScriptParseException; import org.elasticsearch.script.ScriptService.ScriptType; import org.elasticsearch.script.Template; import org.junit.BeforeClass; @@ -72,6 +73,22 @@ public class TemplateQueryBuilderTests extends AbstractQueryTestCase vars = new HashMap<>(); vars.put("template", "filled"); @@ -97,11 +114,6 @@ public class TemplateQueryBuilderTests extends AbstractQueryTestCase params = new HashMap<>(); diff --git a/core/src/test/java/org/elasticsearch/index/query/WrapperQueryBuilderTests.java b/core/src/test/java/org/elasticsearch/index/query/WrapperQueryBuilderTests.java index fb9bde7dce7..e09deca5fec 100644 --- a/core/src/test/java/org/elasticsearch/index/query/WrapperQueryBuilderTests.java +++ b/core/src/test/java/org/elasticsearch/index/query/WrapperQueryBuilderTests.java @@ -21,6 +21,7 @@ package org.elasticsearch.index.query; import org.apache.lucene.search.Query; import org.elasticsearch.action.support.ToXContentToBytes; +import org.elasticsearch.common.ParsingException; import org.elasticsearch.common.bytes.BytesArray; import org.elasticsearch.common.bytes.BytesReference; import org.elasticsearch.common.xcontent.XContentFactory; @@ -102,4 +103,19 @@ public class WrapperQueryBuilderTests extends AbstractQueryTestCase