[TEST] make AbstractQueryTestCase#testUnknownObjectException more accurate

testUnknownObjectException used to generate malformed json objects in some cases, due to the existence of arrays as it was not closing the injected object correctly. That is why the test was catching JsonParseException among the exception that are expected to be thrown. That is fixed by tracking where the new object is placed and placing its end object marker to the right level rather than always at the end.

Also introduced a mechanism to explicitly declare objects that won't cause any exception when they get additional objects injected, so that there is no need to override the method anymore as that caused copy pasting of the whole test method. This also makes sure that changes are reflected in tests, as those inner objects are not skipped but we actually check that what is declared is true (no exceptions get thrown when an additional object is added within them.

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<MoreLik
         return queryBuilder;
     }
 
+    @Override
+    protected Set<String> 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 {

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<ScriptQueryBu
         ScriptQueryBuilder parsed = (ScriptQueryBuilder) parseQuery(json);
         assertEquals(json, "5", parsed.script().getScript());
     }
+
+    @Override
+    protected Set<String> 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");
+    }
 }

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<Functi
         return functionScoreQueryBuilder;
     }
 
+    @Override
+    protected Set<String> 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.

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<PercolateQ
         assertThat(e.getMessage() , equalTo(expectedString));
     }
 
-    // overwrite this test, because adding bogus field to the document part is valid and that would make the test fail
-    // (the document part represents the document being percolated and any key value pair is allowed there)
     @Override
-    public void testUnknownObjectException() throws IOException {
+    protected Set<String> getObjectsHoldingArbitraryContent() {
-        String validQuery = createTestQueryBuilder().toString();
+        //document contains arbitrary content, no error expected when an object is added to it
-        int endPos = validQuery.indexOf("document");
+        return Collections.singleton("document");
-        if (endPos == -1) {
-            endPos = validQuery.length();
-        }
-        assertThat(validQuery, containsString("{"));
-        for (int insertionPosition = 0; insertionPosition < endPos; 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 | 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
-                }
-            }
-        }
     }
 
     public void testRequiredParameters() {

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,28 +312,106 @@ public abstract class AbstractQueryTestCase<QB extends AbstractQueryBuilder<QB>>
     }
 
     /**
-     * Test that adding additional object into otherwise correct query string
+     * Test that adding an additional object within each object of the otherwise correct query always triggers some kind of
-     * should always trigger some kind of Parsing Exception.
+     * 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) == '{') {
+            if (validQuery.charAt(insertionPosition) == '"') {
-                String testQuery = validQuery.substring(0, insertionPosition) + "{ \"newField\" : "
+                withinQuotes = withinQuotes == false;
-                        + validQuery.substring(insertionPosition) + "}";
+            } 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);
+                    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) {
+                    if (expectedException == false) {
-                    // mutation produced invalid json
+                        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<String> getObjectsHoldingArbitraryContent() {
+        return Collections.emptySet();
+    }
 
     /**
      * Test that wraps the randomly generated query into an array as follows: { "query_name" : [{}]}