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

core/src/main/java/org/elasticsearch/index/query/GeoDistanceQueryParser.java

@@ -130,8 +130,13 @@ public class GeoDistanceQueryParser implements QueryParser<GeoDistanceQueryBuild
                 } else if ("validation_method".equals(currentFieldName)) {
                     validationMethod = GeoValidationMethod.fromString(parser.text());
                 } else {
-                    point.resetFromString(parser.text());
-                    fieldName = currentFieldName;
+                    if (fieldName == null) {
+                        point.resetFromString(parser.text());
+                        fieldName = currentFieldName;
+                    } else {
+                        throw new ParsingException(parser.getTokenLocation(), "[" + GeoDistanceQueryBuilder.NAME +
+                                "] field name already set to [" + fieldName + "] but found [" + currentFieldName + "]");
+                    }
                 }
             }
         }

core/src/main/java/org/elasticsearch/index/query/GeoDistanceRangeQueryParser.java

@@ -20,6 +20,7 @@
 package org.elasticsearch.index.query;
 
 import org.elasticsearch.common.ParseField;
+import org.elasticsearch.common.ParsingException;
 import org.elasticsearch.common.geo.GeoDistance;
 import org.elasticsearch.common.geo.GeoPoint;
 import org.elasticsearch.common.geo.GeoUtils;
@@ -94,18 +95,28 @@ public class GeoDistanceRangeQueryParser implements QueryParser<GeoDistanceRange
             } else if (parseContext.isDeprecatedSetting(currentFieldName)) {
                 // skip
             } else if (token == XContentParser.Token.START_ARRAY) {
-                if (point == null) {
-                    point = new GeoPoint();
+                if (fieldName == null) {
+                    if (point == null) {
+                        point = new GeoPoint();
+                    }
+                    GeoUtils.parseGeoPoint(parser, point);
+                    fieldName = currentFieldName;
+                } else {
+                    throw new ParsingException(parser.getTokenLocation(), "[" + GeoDistanceRangeQueryBuilder.NAME +
+                            "] field name already set to [" + fieldName + "] but found [" + currentFieldName + "]");
                 }
-                GeoUtils.parseGeoPoint(parser, point);
-                fieldName = currentFieldName;
             } else if (token == XContentParser.Token.START_OBJECT) {
                 // the json in the format of -> 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<GeoDistanceRange
                 } else if (parseContext.parseFieldMatcher().match(currentFieldName, DISTANCE_TYPE_FIELD)) {
                     geoDistance = GeoDistance.fromString(parser.text());
                 } else if (currentFieldName.endsWith(GeoPointFieldMapper.Names.LAT_SUFFIX)) {
+                    String maybeFieldName = currentFieldName.substring(0, currentFieldName.length() - GeoPointFieldMapper.Names.LAT_SUFFIX.length());
+                    if (fieldName == null || fieldName.equals(maybeFieldName)) {
+                        fieldName = maybeFieldName;
+                    } else {
+                        throw new ParsingException(parser.getTokenLocation(), "[" + GeoDistanceRangeQueryBuilder.NAME +
+                                "] field name already set to [" + fieldName + "] but found [" + currentFieldName + "]");
+                    }
                     if (point == null) {
                         point = new GeoPoint();
                     }
                     point.resetLat(parser.doubleValue());
-                    fieldName = currentFieldName.substring(0, currentFieldName.length() - GeoPointFieldMapper.Names.LAT_SUFFIX.length());
                 } else if (currentFieldName.endsWith(GeoPointFieldMapper.Names.LON_SUFFIX)) {
+                    String maybeFieldName = currentFieldName.substring(0, currentFieldName.length() - GeoPointFieldMapper.Names.LON_SUFFIX.length());
+                    if (fieldName == null || fieldName.equals(maybeFieldName)) {
+                        fieldName = maybeFieldName;
+                    } else {
+                        throw new ParsingException(parser.getTokenLocation(), "[" + GeoDistanceRangeQueryBuilder.NAME +
+                                "] field name already set to [" + fieldName + "] but found [" + currentFieldName + "]");
+                    }
                     if (point == null) {
                         point = new GeoPoint();
                     }
                     point.resetLon(parser.doubleValue());
-                    fieldName = currentFieldName.substring(0, currentFieldName.length() - GeoPointFieldMapper.Names.LON_SUFFIX.length());
                 } else if (currentFieldName.endsWith(GeoPointFieldMapper.Names.GEOHASH_SUFFIX)) {
+                    String maybeFieldName = currentFieldName.substring(0, currentFieldName.length() - GeoPointFieldMapper.Names.GEOHASH_SUFFIX.length());
+                    if (fieldName == null || fieldName.equals(maybeFieldName)) {
+                        fieldName = maybeFieldName;
+                    } else {
+                        throw new ParsingException(parser.getTokenLocation(), "[" + GeoDistanceRangeQueryBuilder.NAME +
+                                "] field name already set to [" + fieldName + "] but found [" + currentFieldName + "]");
+                    }
                     point = GeoPoint.fromGeohash(parser.text());
-                    fieldName = currentFieldName.substring(0, currentFieldName.length() - GeoPointFieldMapper.Names.GEOHASH_SUFFIX.length());
                 } else if (parseContext.parseFieldMatcher().match(currentFieldName, NAME_FIELD)) {
                     queryName = parser.text();
                 } else if (parseContext.parseFieldMatcher().match(currentFieldName, BOOST_FIELD)) {
@@ -189,11 +218,16 @@ public class GeoDistanceRangeQueryParser implements QueryParser<GeoDistanceRange
                 } else if (parseContext.parseFieldMatcher().match(currentFieldName, VALIDATION_METHOD)) {
                     validationMethod = GeoValidationMethod.fromString(parser.text());
                 } else {
-                    if (point == null) {
-                        point = new GeoPoint();
+                    if (fieldName == null) {
+                        if (point == null) {
+                            point = new GeoPoint();
+                        }
+                        point.resetFromString(parser.text());
+                        fieldName = currentFieldName;
+                    } else {
+                        throw new ParsingException(parser.getTokenLocation(), "[" + GeoDistanceRangeQueryBuilder.NAME +
+                                "] field name already set to [" + fieldName + "] but found [" + currentFieldName + "]");
                     }
-                    point.resetFromString(parser.text());
-                    fieldName = currentFieldName;
                 }
             }
         }

core/src/main/java/org/elasticsearch/index/query/GeohashCellQuery.java

@@ -24,6 +24,7 @@ import org.apache.lucene.util.GeoHashUtils;
 import org.elasticsearch.ElasticsearchParseException;
 import org.elasticsearch.common.Nullable;
 import org.elasticsearch.common.ParseField;
+import org.elasticsearch.common.ParsingException;
 import org.elasticsearch.common.Strings;
 import org.elasticsearch.common.geo.GeoPoint;
 import org.elasticsearch.common.geo.GeoUtils;
@@ -318,19 +319,25 @@ public class GeohashCellQuery {
                         parser.nextToken();
                         boost = parser.floatValue();
                     } else {
-                        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();
+                        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 {

core/src/main/java/org/elasticsearch/index/query/MatchNoneQueryBuilder.java

@@ -39,6 +39,7 @@ public class MatchNoneQueryBuilder extends AbstractQueryBuilder<MatchNoneQueryBu
     @Override
     protected void doXContent(XContentBuilder builder, Params params) throws IOException {
         builder.startObject(NAME);
+        printBoostAndQueryName(builder);
         builder.endObject();
     }
 
@@ -47,11 +48,6 @@ public class MatchNoneQueryBuilder extends AbstractQueryBuilder<MatchNoneQueryBu
         return Queries.newMatchNoDocsQuery();
     }
 
-    @Override
-    protected void setFinalBoost(Query query) {
-        //no-op this query doesn't support boost
-    }
-
     @Override
     protected boolean doEquals(MatchNoneQueryBuilder other) {
         return true;

core/src/main/java/org/elasticsearch/index/query/MatchNoneQueryParser.java

@@ -36,12 +36,28 @@ public class MatchNoneQueryParser implements QueryParser<MatchNoneQueryBuilder>
     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

core/src/main/java/org/elasticsearch/index/query/TypeQueryParser.java

@@ -55,14 +55,16 @@ public class TypeQueryParser implements QueryParser<TypeQueryBuilder> {
                     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)

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();
 

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<QB extends AbstractQueryBuilder<QB>> extends ESTestCase {
 
@@ -321,6 +323,29 @@ public abstract class AbstractQueryTestCase<QB extends AbstractQueryBuilder<QB>>
         }
     }
 
+    /**
+     * 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<QB extends AbstractQueryBuilder<QB>>
         QueryParseContext context = createParseContext();
         context.reset(parser);
         context.parseFieldMatcher(matcher);
-        return context.parseInnerQueryBuilder();
+        QueryBuilder parseInnerQueryBuilder = context.parseInnerQueryBuilder();
+        assertTrue(parser.nextToken() == null);
+        return parseInnerQueryBuilder;
     }
 
     /**

core/src/test/java/org/elasticsearch/index/query/ConstantScoreQueryBuilderTests.java

@@ -71,4 +71,9 @@ public class ConstantScoreQueryBuilderTests extends AbstractQueryTestCase<Consta
             // expected
         }
     }
+
+    @Override
+    public void testUnknownField() throws IOException {
+        assumeTrue("test doesn't apply for query filter queries", false);
+    }
 }

core/src/test/java/org/elasticsearch/index/query/MatchNoneQueryBuilderTests.java

@@ -27,20 +27,15 @@ import java.io.IOException;
 import static org.hamcrest.CoreMatchers.equalTo;
 import static org.hamcrest.CoreMatchers.instanceOf;
 
-public class MatchNoneQueryBuilderTests extends AbstractQueryTestCase {
+public class MatchNoneQueryBuilderTests extends AbstractQueryTestCase<MatchNoneQueryBuilder> {
 
     @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));

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<TemplateQue
         //no-op boost is checked already above as part of doAssertLuceneQuery as we rely on lucene equals impl
     }
 
+    /**
+     * Override superclass test since template query doesn't support boost and queryName, so
+     * we need to mutate other existing field in the test query.
+     */
+    @Override
+    public void testUnknownField() throws IOException {
+        TemplateQueryBuilder testQuery = createTestQueryBuilder();
+        String queryAsString = testQuery.toString().replace("inline", "bogusField");
+        try {
+            parseQuery(queryAsString);
+            fail("ScriptParseException expected.");
+        } catch (ScriptParseException e) {
+            assertTrue(e.getMessage().contains("bogusField"));
+        }
+    }
+
     public void testJSONGeneration() throws IOException {
         Map<String, Object> vars = new HashMap<>();
         vars.put("template", "filled");
@@ -97,11 +114,6 @@ public class TemplateQueryBuilderTests extends AbstractQueryTestCase<TemplateQue
     }
 
     public void testRawTemplate() throws IOException {
-        XContentBuilder builder = XContentFactory.jsonBuilder();
-        builder.startObject();
-        builder.startObject("match_{{template}}");
-        builder.endObject();
-        builder.endObject();
         String expectedTemplateString = "{\"match_{{template}}\":{}}";
         String query = "{\"template\": {\"query\": {\"match_{{template}}\": {}},\"params\" : {\"template\" : \"all\"}}}";
         Map<String, Object> params = new HashMap<>();

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<WrapperQuery
             // expected
         }
     }
+
+    /**
+     * Replace the generic test from superclass, wrapper query only expects
+     * to find `query` field with nested query and should throw exception for
+     * anything else.
+     */
+    @Override
+    public void testUnknownField() throws IOException {
+        try {
+            parseQuery("{ \"" + WrapperQueryBuilder.NAME + "\" : {\"bogusField\" : \"someValue\"} }");
+            fail("ParsingException expected.");
+        } catch (ParsingException e) {
+            assertTrue(e.getMessage().contains("bogusField"));
+        }
+    }
 }