From 72463768f506f854f0a9706de0a553d49f4b3e5d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20B=C3=BCscher?= Date: Thu, 22 Oct 2015 14:20:34 +0200 Subject: [PATCH] Query DSL: Parsers should throw exception on unknown object This PR adds a randomized test to the query test base class that mutates an otherwise correct query by adding an additional object into the query hierarchy. Doing so makes the query illegal and should trigger some kind of exception. The new test revelead that some query parsers quietly return queries when called with such an illegal query. Those are also fixed here. Relates to #10974 --- .../termvectors/TermVectorsRequest.java | 5 +-- .../index/query/CommonTermsQueryParser.java | 11 ++++-- .../index/query/ExistsQueryParser.java | 6 ++- .../index/query/GeoShapeQueryParser.java | 10 +++-- .../index/query/IdsQueryParser.java | 8 ++-- .../index/query/MatchAllQueryParser.java | 4 +- .../index/query/MatchNoneQueryParser.java | 2 + .../index/query/MatchQueryParser.java | 8 ++-- .../index/query/MissingQueryParser.java | 4 +- .../index/query/MultiMatchQueryParser.java | 4 +- .../index/query/QueryStringQueryParser.java | 10 +++-- .../index/query/SimpleQueryStringParser.java | 2 + .../index/query/TermsQueryParser.java | 12 ++++-- .../cache/query/terms/TermsLookup.java | 11 ++++-- .../index/query/AbstractQueryTestCase.java | 26 +++++++++++++ .../query/HasChildQueryBuilderTests.java | 39 ++++++++++++++++++- .../query/HasParentQueryBuilderTests.java | 38 ++++++++++++++++++ .../query/SimpleQueryStringBuilderTests.java | 4 +- .../index/query/TermsQueryBuilderTests.java | 13 ++++--- 19 files changed, 178 insertions(+), 39 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/action/termvectors/TermVectorsRequest.java b/core/src/main/java/org/elasticsearch/action/termvectors/TermVectorsRequest.java index ef998922d51..c13e44097bc 100644 --- a/core/src/main/java/org/elasticsearch/action/termvectors/TermVectorsRequest.java +++ b/core/src/main/java/org/elasticsearch/action/termvectors/TermVectorsRequest.java @@ -19,7 +19,6 @@ package org.elasticsearch.action.termvectors; -import org.elasticsearch.ElasticsearchException; import org.elasticsearch.ElasticsearchParseException; import org.elasticsearch.action.ActionRequestValidationException; import org.elasticsearch.action.DocumentRequest; @@ -211,7 +210,7 @@ public class TermVectorsRequest extends SingleShardRequest i public String id() { return id; } - + /** * Sets the id of document the term vector is requested for. */ @@ -651,7 +650,7 @@ public class TermVectorsRequest extends SingleShardRequest i if (e.getValue() instanceof String) { mapStrStr.put(e.getKey(), (String) e.getValue()); } else { - throw new ElasticsearchException("expecting the analyzer at [{}] to be a String, but found [{}] instead", e.getKey(), e.getValue().getClass()); + throw new ElasticsearchParseException("expecting the analyzer at [{}] to be a String, but found [{}] instead", e.getKey(), e.getValue().getClass()); } } return mapStrStr; diff --git a/core/src/main/java/org/elasticsearch/index/query/CommonTermsQueryParser.java b/core/src/main/java/org/elasticsearch/index/query/CommonTermsQueryParser.java index 86de4e31129..3ecbd11320e 100644 --- a/core/src/main/java/org/elasticsearch/index/query/CommonTermsQueryParser.java +++ b/core/src/main/java/org/elasticsearch/index/query/CommonTermsQueryParser.java @@ -39,7 +39,7 @@ public class CommonTermsQueryParser implements QueryParser { } else if ("boost".equals(currentFieldName)) { boost = parser.floatValue(); } else { - throw new ParsingException(parser.getTokenLocation(), "[exists] query does not support [" + currentFieldName + "]"); + throw new ParsingException(parser.getTokenLocation(), "[" + ExistsQueryBuilder.NAME + "] query does not support [" + currentFieldName + "]"); } + } else { + throw new ParsingException(parser.getTokenLocation(), "[" + ExistsQueryBuilder.NAME + "] unknown token [" + token + "] after [" + currentFieldName + "]"); } } if (fieldPattern == null) { - throw new ParsingException(parser.getTokenLocation(), "exists must be provided with a [field]"); + throw new ParsingException(parser.getTokenLocation(), "[" + ExistsQueryBuilder.NAME + "] must be provided with a [field]"); } ExistsQueryBuilder builder = new ExistsQueryBuilder(fieldPattern); diff --git a/core/src/main/java/org/elasticsearch/index/query/GeoShapeQueryParser.java b/core/src/main/java/org/elasticsearch/index/query/GeoShapeQueryParser.java index e5198952c13..768600c3b38 100644 --- a/core/src/main/java/org/elasticsearch/index/query/GeoShapeQueryParser.java +++ b/core/src/main/java/org/elasticsearch/index/query/GeoShapeQueryParser.java @@ -70,8 +70,10 @@ public class GeoShapeQueryParser implements QueryParser { if (token == XContentParser.Token.FIELD_NAME) { currentFieldName = parser.currentName(); } else if (token == XContentParser.Token.START_OBJECT) { + if (fieldName != null) { + throw new ParsingException(parser.getTokenLocation(), "[" + GeoShapeQueryBuilder.NAME + "] point specified twice. [" + currentFieldName + "]"); + } fieldName = currentFieldName; - while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) { if (token == XContentParser.Token.FIELD_NAME) { currentFieldName = parser.currentName(); @@ -104,10 +106,12 @@ public class GeoShapeQueryParser implements QueryParser { } else if (parseContext.parseFieldMatcher().match(currentFieldName, SHAPE_PATH_FIELD)) { shapePath = parser.text(); } + } else { + throw new ParsingException(parser.getTokenLocation(), "[" + GeoShapeQueryBuilder.NAME + "] unknown token [" + token + "] after [" + currentFieldName + "]"); } } } else { - throw new ParsingException(parser.getTokenLocation(), "[geo_shape] query does not support [" + currentFieldName + "]"); + throw new ParsingException(parser.getTokenLocation(), "[" + GeoShapeQueryBuilder.NAME + "] query does not support [" + currentFieldName + "]"); } } } @@ -117,7 +121,7 @@ public class GeoShapeQueryParser implements QueryParser { } else if (parseContext.parseFieldMatcher().match(currentFieldName, AbstractQueryBuilder.NAME_FIELD)) { queryName = parser.text(); } else { - throw new ParsingException(parser.getTokenLocation(), "[geo_shape] query does not support [" + currentFieldName + "]"); + throw new ParsingException(parser.getTokenLocation(), "[" + GeoShapeQueryBuilder.NAME + "] query does not support [" + currentFieldName + "]"); } } } diff --git a/core/src/main/java/org/elasticsearch/index/query/IdsQueryParser.java b/core/src/main/java/org/elasticsearch/index/query/IdsQueryParser.java index 0ffd31644e5..0496a690f5f 100644 --- a/core/src/main/java/org/elasticsearch/index/query/IdsQueryParser.java +++ b/core/src/main/java/org/elasticsearch/index/query/IdsQueryParser.java @@ -79,7 +79,7 @@ public class IdsQueryParser implements QueryParser { types.add(value); } } else { - throw new ParsingException(parser.getTokenLocation(), "[ids] query does not support [" + currentFieldName + "]"); + throw new ParsingException(parser.getTokenLocation(), "[" + IdsQueryBuilder.NAME + "] query does not support [" + currentFieldName + "]"); } } else if (token.isValue()) { if ("type".equals(currentFieldName) || "_type".equals(currentFieldName)) { @@ -89,12 +89,14 @@ public class IdsQueryParser implements QueryParser { } else if ("_name".equals(currentFieldName)) { queryName = parser.text(); } else { - throw new ParsingException(parser.getTokenLocation(), "[ids] query does not support [" + currentFieldName + "]"); + throw new ParsingException(parser.getTokenLocation(), "[" + IdsQueryBuilder.NAME + "] query does not support [" + currentFieldName + "]"); } + } else { + throw new ParsingException(parser.getTokenLocation(), "[" + IdsQueryBuilder.NAME + "] unknown token [" + token + "] after [" + currentFieldName + "]"); } } if (!idsProvided) { - throw new ParsingException(parser.getTokenLocation(), "[ids] query, no ids values provided"); + throw new ParsingException(parser.getTokenLocation(), "[" + IdsQueryBuilder.NAME + "] query, no ids values provided"); } IdsQueryBuilder query = new IdsQueryBuilder(types.toArray(new String[types.size()])); diff --git a/core/src/main/java/org/elasticsearch/index/query/MatchAllQueryParser.java b/core/src/main/java/org/elasticsearch/index/query/MatchAllQueryParser.java index 770f9c3dd67..6cab98838e0 100644 --- a/core/src/main/java/org/elasticsearch/index/query/MatchAllQueryParser.java +++ b/core/src/main/java/org/elasticsearch/index/query/MatchAllQueryParser.java @@ -52,8 +52,10 @@ public class MatchAllQueryParser implements QueryParser { } else if ("boost".equals(currentFieldName)) { boost = parser.floatValue(); } else { - throw new ParsingException(parser.getTokenLocation(), "[match_all] query does not support [" + currentFieldName + "]"); + throw new ParsingException(parser.getTokenLocation(), "[" + MatchAllQueryBuilder.NAME + "] query does not support [" + currentFieldName + "]"); } + } else { + throw new ParsingException(parser.getTokenLocation(), "[" + MatchAllQueryBuilder.NAME + "] unknown token [" + token + "] after [" + currentFieldName + "]"); } } MatchAllQueryBuilder queryBuilder = new MatchAllQueryBuilder(); diff --git a/core/src/main/java/org/elasticsearch/index/query/MatchNoneQueryParser.java b/core/src/main/java/org/elasticsearch/index/query/MatchNoneQueryParser.java index 96b33f545f8..449824e72c9 100644 --- a/core/src/main/java/org/elasticsearch/index/query/MatchNoneQueryParser.java +++ b/core/src/main/java/org/elasticsearch/index/query/MatchNoneQueryParser.java @@ -51,6 +51,8 @@ public class MatchNoneQueryParser implements QueryParser } else { throw new ParsingException(parser.getTokenLocation(), "["+MatchNoneQueryBuilder.NAME+"] query does not support [" + currentFieldName + "]"); } + } else { + throw new ParsingException(parser.getTokenLocation(), "[" + MatchNoneQueryBuilder.NAME + "] unknown token [" + token + "] after [" + currentFieldName + "]"); } } diff --git a/core/src/main/java/org/elasticsearch/index/query/MatchQueryParser.java b/core/src/main/java/org/elasticsearch/index/query/MatchQueryParser.java index afcf25ca2a7..c50256480a0 100644 --- a/core/src/main/java/org/elasticsearch/index/query/MatchQueryParser.java +++ b/core/src/main/java/org/elasticsearch/index/query/MatchQueryParser.java @@ -55,7 +55,7 @@ public class MatchQueryParser implements QueryParser { XContentParser.Token token = parser.nextToken(); if (token != XContentParser.Token.FIELD_NAME) { - throw new ParsingException(parser.getTokenLocation(), "[match] query malformed, no field"); + throw new ParsingException(parser.getTokenLocation(), "[" + MatchQueryBuilder.NAME + "] query malformed, no field"); } String fieldName = parser.currentName(); @@ -93,7 +93,7 @@ public class MatchQueryParser implements QueryParser { } else if ("phrase_prefix".equals(tStr) || "phrasePrefix".equals(currentFieldName)) { type = MatchQuery.Type.PHRASE_PREFIX; } else { - throw new ParsingException(parser.getTokenLocation(), "[match] query does not support type " + tStr); + throw new ParsingException(parser.getTokenLocation(), "[" + MatchQueryBuilder.NAME + "] query does not support type " + tStr); } } else if ("analyzer".equals(currentFieldName)) { analyzer = parser.text(); @@ -131,8 +131,10 @@ public class MatchQueryParser implements QueryParser { } else if ("_name".equals(currentFieldName)) { queryName = parser.text(); } else { - throw new ParsingException(parser.getTokenLocation(), "[match] query does not support [" + currentFieldName + "]"); + throw new ParsingException(parser.getTokenLocation(), "[" + MatchQueryBuilder.NAME + "] query does not support [" + currentFieldName + "]"); } + } else { + throw new ParsingException(parser.getTokenLocation(), "[" + MatchQueryBuilder.NAME + "] unknown token [" + token + "] after [" + currentFieldName + "]"); } } parser.nextToken(); diff --git a/core/src/main/java/org/elasticsearch/index/query/MissingQueryParser.java b/core/src/main/java/org/elasticsearch/index/query/MissingQueryParser.java index 8d8c5aec01f..3a696dad38f 100644 --- a/core/src/main/java/org/elasticsearch/index/query/MissingQueryParser.java +++ b/core/src/main/java/org/elasticsearch/index/query/MissingQueryParser.java @@ -61,8 +61,10 @@ public class MissingQueryParser implements QueryParser { } else if ("boost".equals(currentFieldName)) { boost = parser.floatValue(); } else { - throw new ParsingException(parser.getTokenLocation(), "[missing] query does not support [" + currentFieldName + "]"); + throw new ParsingException(parser.getTokenLocation(), "[" + MissingQueryBuilder.NAME + "] query does not support [" + currentFieldName + "]"); } + } else { + throw new ParsingException(parser.getTokenLocation(), "[" + MissingQueryBuilder.NAME + "] unknown token [" + token + "] after [" + currentFieldName + "]"); } } diff --git a/core/src/main/java/org/elasticsearch/index/query/MultiMatchQueryParser.java b/core/src/main/java/org/elasticsearch/index/query/MultiMatchQueryParser.java index c86d0295577..d52f6707aa1 100644 --- a/core/src/main/java/org/elasticsearch/index/query/MultiMatchQueryParser.java +++ b/core/src/main/java/org/elasticsearch/index/query/MultiMatchQueryParser.java @@ -122,8 +122,10 @@ public class MultiMatchQueryParser implements QueryParser, ToXContent { public TermsLookup(String index, String type, String id, String path) { if (id == null) { - throw new IllegalArgumentException("[terms] query lookup element requires specifying the id."); + throw new IllegalArgumentException("[" + TermsQueryBuilder.NAME + "] query lookup element requires specifying the id."); } if (type == null) { - throw new IllegalArgumentException("[terms] query lookup element requires specifying the type."); + throw new IllegalArgumentException("[" + TermsQueryBuilder.NAME + "] query lookup element requires specifying the type."); } if (path == null) { - throw new IllegalArgumentException("[terms] query lookup element requires specifying the path."); + throw new IllegalArgumentException("[" + TermsQueryBuilder.NAME + "] query lookup element requires specifying the path."); } this.index = index; this.type = type; @@ -122,9 +123,11 @@ public class TermsLookup implements Writeable, ToXContent { path = parser.text(); break; default: - throw new ParsingException(parser.getTokenLocation(), "[terms] query does not support [" + currentFieldName + throw new ParsingException(parser.getTokenLocation(), "[" + TermsQueryBuilder.NAME + "] query does not support [" + currentFieldName + "] within lookup element"); } + } else { + throw new ParsingException(parser.getTokenLocation(), "[" + TermsQueryBuilder.NAME + "] unknown token [" + token + "] after [" + currentFieldName + "]"); } } return new TermsLookup(index, type, id, path).routing(routing); 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 212dd457171..c553e94e29f 100644 --- a/core/src/test/java/org/elasticsearch/index/query/AbstractQueryTestCase.java +++ b/core/src/test/java/org/elasticsearch/index/query/AbstractQueryTestCase.java @@ -20,10 +20,12 @@ package org.elasticsearch.index.query; import com.carrotsearch.randomizedtesting.generators.CodepointSetGenerator; +import com.fasterxml.jackson.core.JsonParseException; import com.fasterxml.jackson.core.io.JsonStringEncoder; import org.apache.lucene.search.Query; import org.elasticsearch.ElasticsearchException; +import org.elasticsearch.ElasticsearchParseException; import org.elasticsearch.Version; import org.elasticsearch.action.admin.indices.mapping.put.PutMappingRequest; import org.elasticsearch.action.get.GetRequest; @@ -76,6 +78,7 @@ import org.elasticsearch.indices.IndicesWarmer; import org.elasticsearch.indices.breaker.CircuitBreakerService; import org.elasticsearch.indices.breaker.NoneCircuitBreakerService; import org.elasticsearch.script.*; +import org.elasticsearch.script.Script.ScriptParseException; import org.elasticsearch.script.mustache.MustacheScriptEngineService; import org.elasticsearch.search.internal.SearchContext; import org.elasticsearch.test.ESTestCase; @@ -344,6 +347,29 @@ public abstract class AbstractQueryTestCase> // we'd like to see the offending field name here assertThat(e.getMessage(), containsString("bogusField")); } + } + + /** + * Test that adding additional object into otherwise correct query string + * should always trigger some kind of Parsing Exception. + */ + public void testUnknownObjectException() throws IOException { + String validQuery = createTestQueryBuilder().toString(); + assertThat(validQuery, containsString("{")); + for (int insertionPosition = 0; insertionPosition < validQuery.length(); insertionPosition++) { + if (validQuery.charAt(insertionPosition) == '{') { + String testQuery = validQuery.substring(0, insertionPosition) + "{ \"newField\" : " + validQuery.substring(insertionPosition) + "}"; + try { + parseQuery(testQuery); + fail("some parsing exception expected for query: " + testQuery); + } catch (ParsingException | ScriptParseException | ElasticsearchParseException e) { + // different kinds of exception wordings depending on location + // of mutation, so no simple asserts possible here + } catch (JsonParseException e) { + // mutation produced invalid json + } + } + } } /** diff --git a/core/src/test/java/org/elasticsearch/index/query/HasChildQueryBuilderTests.java b/core/src/test/java/org/elasticsearch/index/query/HasChildQueryBuilderTests.java index c567ba3a0f3..a6c63f83c96 100644 --- a/core/src/test/java/org/elasticsearch/index/query/HasChildQueryBuilderTests.java +++ b/core/src/test/java/org/elasticsearch/index/query/HasChildQueryBuilderTests.java @@ -20,11 +20,15 @@ package org.elasticsearch.index.query; import com.carrotsearch.randomizedtesting.generators.RandomPicks; +import com.fasterxml.jackson.core.JsonParseException; + import org.apache.lucene.queries.TermsQuery; import org.apache.lucene.search.*; import org.apache.lucene.search.join.ScoreMode; import org.apache.lucene.util.BytesRef; +import org.elasticsearch.ElasticsearchParseException; import org.elasticsearch.action.admin.indices.mapping.put.PutMappingRequest; +import org.elasticsearch.common.ParsingException; import org.elasticsearch.common.compress.CompressedXContent; import org.elasticsearch.common.xcontent.ToXContent; import org.elasticsearch.common.xcontent.XContentBuilder; @@ -35,6 +39,7 @@ import org.elasticsearch.index.mapper.Uid; import org.elasticsearch.index.mapper.internal.TypeFieldMapper; import org.elasticsearch.index.mapper.internal.UidFieldMapper; import org.elasticsearch.index.query.support.QueryInnerHits; +import org.elasticsearch.script.Script.ScriptParseException; import org.elasticsearch.search.fetch.innerhits.InnerHitsBuilder; import org.elasticsearch.search.fetch.innerhits.InnerHitsContext; import org.elasticsearch.search.internal.SearchContext; @@ -44,7 +49,8 @@ import org.elasticsearch.test.TestSearchContext; import java.io.IOException; import java.util.Collections; -import static org.elasticsearch.test.StreamsUtils.copyToStringFromClasspath; +import static org.hamcrest.Matchers.containsString; + import static org.hamcrest.CoreMatchers.equalTo; import static org.hamcrest.CoreMatchers.instanceOf; @@ -52,6 +58,7 @@ public class HasChildQueryBuilderTests extends AbstractQueryTestCase { private List randomTerms; @@ -195,9 +198,9 @@ public class TermsQueryBuilderTests extends AbstractQueryTestCase