diff --git a/core/src/test/java/org/elasticsearch/index/query/MoreLikeThisQueryBuilderTests.java b/core/src/test/java/org/elasticsearch/index/query/MoreLikeThisQueryBuilderTests.java index 291bb9f0bd6..801ae051f70 100644 --- a/core/src/test/java/org/elasticsearch/index/query/MoreLikeThisQueryBuilderTests.java +++ b/core/src/test/java/org/elasticsearch/index/query/MoreLikeThisQueryBuilderTests.java @@ -45,9 +45,11 @@ import org.junit.Before; import java.io.IOException; import java.util.Arrays; +import java.util.Collections; import java.util.EnumSet; import java.util.HashMap; import java.util.Map; +import java.util.Set; import java.util.stream.Stream; import static org.elasticsearch.index.query.QueryBuilders.moreLikeThisQuery; @@ -197,6 +199,12 @@ public class MoreLikeThisQueryBuilderTests extends AbstractQueryTestCase getObjectsHoldingArbitraryContent() { + //doc contains arbitrary content, anything can be added to it and no exception will be thrown + return Collections.singleton("doc"); + } + @Override protected MultiTermVectorsResponse executeMultiTermVectors(MultiTermVectorsRequest mtvRequest) { try { diff --git a/core/src/test/java/org/elasticsearch/index/query/ScriptQueryBuilderTests.java b/core/src/test/java/org/elasticsearch/index/query/ScriptQueryBuilderTests.java index 920d2537935..c2a89e32fad 100644 --- a/core/src/test/java/org/elasticsearch/index/query/ScriptQueryBuilderTests.java +++ b/core/src/test/java/org/elasticsearch/index/query/ScriptQueryBuilderTests.java @@ -28,6 +28,7 @@ import org.elasticsearch.test.AbstractQueryTestCase; import java.io.IOException; import java.util.Collections; import java.util.Map; +import java.util.Set; import static org.hamcrest.Matchers.instanceOf; @@ -81,4 +82,11 @@ public class ScriptQueryBuilderTests extends AbstractQueryTestCase getObjectsHoldingArbitraryContent() { + //script_score.script.params can contain arbitrary parameters. no error is expected when + //adding additional objects within the params object. + return Collections.singleton("params"); + } } diff --git a/core/src/test/java/org/elasticsearch/index/query/functionscore/FunctionScoreQueryBuilderTests.java b/core/src/test/java/org/elasticsearch/index/query/functionscore/FunctionScoreQueryBuilderTests.java index 62f3cf4504e..b702042a322 100644 --- a/core/src/test/java/org/elasticsearch/index/query/functionscore/FunctionScoreQueryBuilderTests.java +++ b/core/src/test/java/org/elasticsearch/index/query/functionscore/FunctionScoreQueryBuilderTests.java @@ -20,7 +20,6 @@ package org.elasticsearch.index.query.functionscore; import com.fasterxml.jackson.core.JsonParseException; - import org.apache.lucene.index.Term; import org.apache.lucene.search.MatchAllDocsQuery; import org.apache.lucene.search.Query; @@ -57,10 +56,13 @@ import org.joda.time.DateTime; import org.joda.time.DateTimeZone; import java.io.IOException; +import java.util.Arrays; import java.util.Collection; import java.util.Collections; +import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Set; import static java.util.Collections.singletonList; import static org.elasticsearch.common.xcontent.XContentFactory.jsonBuilder; @@ -102,6 +104,13 @@ public class FunctionScoreQueryBuilderTests extends AbstractQueryTestCase getObjectsHoldingArbitraryContent() { + //script_score.script.params can contain arbitrary parameters. no error is expected when adding additional objects + //within the params object. Score functions get parsed in the data nodes, so they are not validated in the coord node. + return new HashSet<>(Arrays.asList("params", "exp", "linear", "gauss")); + } + /** * Creates a random function score query using only constructor params. The caller is responsible for randomizing fields set outside of * the constructor. diff --git a/modules/percolator/src/test/java/org/elasticsearch/percolator/PercolateQueryBuilderTests.java b/modules/percolator/src/test/java/org/elasticsearch/percolator/PercolateQueryBuilderTests.java index 6c8b345f543..55ef53e696c 100644 --- a/modules/percolator/src/test/java/org/elasticsearch/percolator/PercolateQueryBuilderTests.java +++ b/modules/percolator/src/test/java/org/elasticsearch/percolator/PercolateQueryBuilderTests.java @@ -19,7 +19,6 @@ package org.elasticsearch.percolator; -import com.fasterxml.jackson.core.JsonParseException; import org.apache.lucene.analysis.Analyzer; import org.apache.lucene.analysis.core.WhitespaceAnalyzer; import org.apache.lucene.search.BooleanClause; @@ -27,12 +26,10 @@ import org.apache.lucene.search.BooleanQuery; import org.apache.lucene.search.IndexSearcher; import org.apache.lucene.search.MatchAllDocsQuery; import org.apache.lucene.search.Query; -import org.elasticsearch.ElasticsearchParseException; import org.elasticsearch.ResourceNotFoundException; import org.elasticsearch.action.admin.indices.mapping.put.PutMappingRequest; import org.elasticsearch.action.get.GetRequest; import org.elasticsearch.action.get.GetResponse; -import org.elasticsearch.common.ParsingException; import org.elasticsearch.common.bytes.BytesArray; import org.elasticsearch.common.bytes.BytesReference; import org.elasticsearch.common.compress.CompressedXContent; @@ -54,8 +51,8 @@ import java.util.ArrayList; import java.util.Collection; import java.util.Collections; import java.util.List; +import java.util.Set; -import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.sameInstance; @@ -171,31 +168,10 @@ public class PercolateQueryBuilderTests extends AbstractQueryTestCase getObjectsHoldingArbitraryContent() { + //document contains arbitrary content, no error expected when an object is added to it + return Collections.singleton("document"); } public void testRequiredParameters() { diff --git a/test/framework/src/main/java/org/elasticsearch/test/AbstractQueryTestCase.java b/test/framework/src/main/java/org/elasticsearch/test/AbstractQueryTestCase.java index a884f45359d..842b14cf386 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/AbstractQueryTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/test/AbstractQueryTestCase.java @@ -19,7 +19,6 @@ package org.elasticsearch.test; -import com.fasterxml.jackson.core.JsonParseException; import com.fasterxml.jackson.core.io.JsonStringEncoder; import org.apache.lucene.search.BoostQuery; import org.apache.lucene.search.Query; @@ -117,6 +116,7 @@ import java.util.Collections; import java.util.List; import java.util.Locale; import java.util.Map; +import java.util.Set; import java.util.concurrent.ExecutionException; import static java.util.Collections.emptyList; @@ -312,29 +312,107 @@ public abstract class AbstractQueryTestCase> } /** - * Test that adding additional object into otherwise correct query string - * should always trigger some kind of Parsing Exception. + * Test that adding an additional object within each object of the otherwise correct query always triggers some kind of + * parse exception. Some specific objects do not cause any exception as they can hold arbitray content; they can be + * declared by overriding {@link #getObjectsHoldingArbitraryContent()} */ - public void testUnknownObjectException() throws IOException { + public final void testUnknownObjectException() throws IOException { + //TODO building json by concatenating strings makes the code unmaintainable, we should rewrite this test String validQuery = createTestQueryBuilder().toString(); assertThat(validQuery, containsString("{")); + int level = 0; + boolean withinQuotes = false; + boolean expectedException = true; + int objectHoldingArbitraryContentLevel = 0; for (int insertionPosition = 0; insertionPosition < validQuery.length(); insertionPosition++) { - if (validQuery.charAt(insertionPosition) == '{') { - String testQuery = validQuery.substring(0, insertionPosition) + "{ \"newField\" : " - + validQuery.substring(insertionPosition) + "}"; + if (validQuery.charAt(insertionPosition) == '"') { + withinQuotes = withinQuotes == false; + } else if (withinQuotes == false && validQuery.charAt(insertionPosition) == '}') { + level--; + if (expectedException == false) { + //track where we are within the object that holds arbitrary content + objectHoldingArbitraryContentLevel--; + } + if (objectHoldingArbitraryContentLevel == 0) { + //reset the flag once we have traversed the whole object that holds arbitrary content + expectedException = true; + } + } else if (withinQuotes == false && validQuery.charAt(insertionPosition) == '{') { + level++; + if (expectedException) { + int start = -1; + int end = -1; + for (int i = insertionPosition; i >= 0; i--) { + if (validQuery.charAt(i) == '}') { + break; + } else if (validQuery.charAt(i) == '"') { + if (end == -1) { + end = i; + } else if (start == -1) { + start = i + 1; + } else { + break; + } + } + } + if (start >= 0 && end > 0) { + String objectName = validQuery.substring(start, end); + expectedException = getObjectsHoldingArbitraryContent().contains(objectName) == false; + } + } + if (expectedException == false) { + objectHoldingArbitraryContentLevel++; + } + String testQuery = validQuery.substring(0, insertionPosition) + "{ \"newField\" : "; + String secondPart = validQuery.substring(insertionPosition); + int currentLevel = level; + boolean quotes = false; + for (int i = 0; i < secondPart.length(); i++) { + if (secondPart.charAt(i) == '"') { + quotes = quotes == false; + } else if (quotes == false && secondPart.charAt(i) == '{') { + currentLevel++; + } else if (quotes == false && secondPart.charAt(i) == '}') { + currentLevel--; + if (currentLevel == level) { + testQuery += secondPart.substring(0, i - 1) + "}" + secondPart.substring(i); + break; + } + } + } + try { parseQuery(testQuery); - fail("some parsing exception expected for query: " + testQuery); + if (expectedException) { + fail("some parsing exception expected for query: " + testQuery); + } } catch (ParsingException | 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 + if (expectedException == false) { + throw new AssertionError("unexpected exception when parsing query:\n" + testQuery, e); + } + } catch(IllegalArgumentException e) { + assertThat(e.getMessage(), containsString("unknown field [newField], parser not found")); + if (expectedException == false) { + throw new AssertionError("unexpected exception when parsing query:\n" + testQuery, e); + } } } } } + /** + * Returns a set of object names that won't trigger any exception (uncluding their children) when testing that unknown + * objects cause parse exceptions through {@link #testUnknownObjectException()}. Default is an empty set. Can be overridden + * by subclasses that test queries which contain objects that get parsed on the data nodes (e.g. score functions) or objects + * that can contain arbitrary content (e.g. documents for percolate or more like this query, params for scripts). In such + * cases no exception would get thrown. + */ + protected Set getObjectsHoldingArbitraryContent() { + return Collections.emptySet(); + } + /** * Test that wraps the randomly generated query into an array as follows: { "query_name" : [{}]} * This causes unexpected situations in parser code that may not be handled properly.