Function Score Query: make parsing stricter

Function Score Query now checks the type of token that we are parsing, which makes parsing stricter and allows to throw useful errors in case the json is malformed. It also makes code more readable as in what gets parsed when.

Closes #16583

core/src/main/java/org/elasticsearch/index/query/functionscore/FunctionScoreQueryParser.java

@@ -19,10 +19,6 @@
 
 package org.elasticsearch.index.query.functionscore;
 
-import java.io.IOException;
-import java.util.ArrayList;
-import java.util.List;
-
 import org.elasticsearch.common.ParseField;
 import org.elasticsearch.common.ParsingException;
 import org.elasticsearch.common.Strings;
@@ -39,6 +35,10 @@ import org.elasticsearch.index.query.QueryParseContext;
 import org.elasticsearch.index.query.QueryParser;
 import org.elasticsearch.index.query.functionscore.weight.WeightBuilder;
 
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
+
 /**
  * Parser for function_score query
  */
@@ -86,27 +86,12 @@ public class FunctionScoreQueryParser implements QueryParser<FunctionScoreQueryB
         while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) {
             if (token == XContentParser.Token.FIELD_NAME) {
                 currentFieldName = parser.currentName();
-            } else if ("query".equals(currentFieldName)) {
+            } else if (token == XContentParser.Token.START_OBJECT) {
-                query = parseContext.parseInnerQueryBuilder();
+                if ("query".equals(currentFieldName)) {
-            } else if ("score_mode".equals(currentFieldName) || "scoreMode".equals(currentFieldName)) {
+                    if (query != null) {
-                scoreMode = FiltersFunctionScoreQuery.ScoreMode.fromString(parser.text());
+                        throw new ParsingException(parser.getTokenLocation(), "failed to parse [{}] query. [query] is already defined.", FunctionScoreQueryBuilder.NAME);
-            } else if ("boost_mode".equals(currentFieldName) || "boostMode".equals(currentFieldName)) {
-                combineFunction = CombineFunction.fromString(parser.text());
-            } else if ("max_boost".equals(currentFieldName) || "maxBoost".equals(currentFieldName)) {
-                maxBoost = parser.floatValue();
-            } else if ("boost".equals(currentFieldName)) {
-                boost = parser.floatValue();
-            } else if ("_name".equals(currentFieldName)) {
-                queryName = parser.text();
-            } else if ("min_score".equals(currentFieldName) || "minScore".equals(currentFieldName)) {
-                minScore = parser.floatValue();
-            } else if ("functions".equals(currentFieldName)) {
-                if (singleFunctionFound) {
-                    String errorString = "already found [" + singleFunctionName + "], now encountering [functions].";
-                    handleMisplacedFunctionsDeclaration(parser.getTokenLocation(), errorString);
                     }
-                functionArrayFound = true;
+                    query = parseContext.parseInnerQueryBuilder();
-                currentFieldName = parseFiltersAndFunctions(parseContext, parser, filterFunctionBuilders);
                 } else {
                     if (singleFunctionFound) {
                         throw new ParsingException(parser.getTokenLocation(), "failed to parse [{}] query. already found function [{}], now encountering [{}]. use [functions] array if you want to define several functions.", FunctionScoreQueryBuilder.NAME, singleFunctionName, currentFieldName);
@@ -118,17 +103,53 @@ public class FunctionScoreQueryParser implements QueryParser<FunctionScoreQueryB
                     singleFunctionFound = true;
                     singleFunctionName = currentFieldName;
 
-                ScoreFunctionBuilder<?> scoreFunction;
+                    // we try to parse a score function. If there is no score function for the current field name,
-                if (parseContext.parseFieldMatcher().match(currentFieldName, WEIGHT_FIELD)) {
+                    // functionParserMapper.get() may throw an Exception.
-                    scoreFunction = new WeightBuilder().setWeight(parser.floatValue());
+                    ScoreFunctionBuilder<?> scoreFunction = functionParserMapper.get(parser.getTokenLocation(), currentFieldName).fromXContent(parseContext, parser);
-                } else {
-                    // we try to parse a score function. If there is no score
-                    // function for the current field name,
-                    // functionParserMapper.get() will throw an Exception.
-                    scoreFunction = functionParserMapper.get(parser.getTokenLocation(), currentFieldName).fromXContent(parseContext, parser);
-                }
                     filterFunctionBuilders.add(new FunctionScoreQueryBuilder.FilterFunctionBuilder(scoreFunction));
                 }
+            } else if (token == XContentParser.Token.START_ARRAY) {
+                if ("functions".equals(currentFieldName)) {
+                    if (singleFunctionFound) {
+                        String errorString = "already found [" + singleFunctionName + "], now encountering [functions].";
+                        handleMisplacedFunctionsDeclaration(parser.getTokenLocation(), errorString);
+                    }
+                    functionArrayFound = true;
+                    currentFieldName = parseFiltersAndFunctions(parseContext, parser, filterFunctionBuilders);
+                } else {
+                    throw new ParsingException(parser.getTokenLocation(), "failed to parse [{}] query. array [{}] is not supported", FunctionScoreQueryBuilder.NAME, currentFieldName);
+                }
+
+            } else if (token.isValue()) {
+                if ("score_mode".equals(currentFieldName) || "scoreMode".equals(currentFieldName)) {
+                    scoreMode = FiltersFunctionScoreQuery.ScoreMode.fromString(parser.text());
+                } else if ("boost_mode".equals(currentFieldName) || "boostMode".equals(currentFieldName)) {
+                    combineFunction = CombineFunction.fromString(parser.text());
+                } else if ("max_boost".equals(currentFieldName) || "maxBoost".equals(currentFieldName)) {
+                    maxBoost = parser.floatValue();
+                } else if ("boost".equals(currentFieldName)) {
+                    boost = parser.floatValue();
+                } else if ("_name".equals(currentFieldName)) {
+                    queryName = parser.text();
+                } else if ("min_score".equals(currentFieldName) || "minScore".equals(currentFieldName)) {
+                    minScore = parser.floatValue();
+                } else {
+                    if (singleFunctionFound) {
+                        throw new ParsingException(parser.getTokenLocation(), "failed to parse [{}] query. already found function [{}], now encountering [{}]. use [functions] array if you want to define several functions.", FunctionScoreQueryBuilder.NAME, singleFunctionName, currentFieldName);
+                    }
+                    if (functionArrayFound) {
+                        String errorString = "already found [functions] array, now encountering [" + currentFieldName + "].";
+                        handleMisplacedFunctionsDeclaration(parser.getTokenLocation(), errorString);
+                    }
+                    if (parseContext.parseFieldMatcher().match(currentFieldName, WEIGHT_FIELD)) {
+                        filterFunctionBuilders.add(new FunctionScoreQueryBuilder.FilterFunctionBuilder(new WeightBuilder().setWeight(parser.floatValue())));
+                        singleFunctionFound = true;
+                        singleFunctionName = currentFieldName;
+                    } else {
+                        throw new ParsingException(parser.getTokenLocation(), "failed to parse [{}] query. field [{}] is not supported", FunctionScoreQueryBuilder.NAME, currentFieldName);
+                    }
+                }
+            }
         }
 
         if (query == null) {
@@ -167,21 +188,23 @@ public class FunctionScoreQueryParser implements QueryParser<FunctionScoreQueryB
                 while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) {
                     if (token == XContentParser.Token.FIELD_NAME) {
                         currentFieldName = parser.currentName();
-                    } else if (parseContext.parseFieldMatcher().match(currentFieldName, WEIGHT_FIELD)) {
+                    } else if (token == XContentParser.Token.START_OBJECT) {
-                        functionWeight = parser.floatValue();
-                    } else {
                         if ("filter".equals(currentFieldName)) {
                             filter = parseContext.parseInnerQueryBuilder();
                         } else {
                             if (scoreFunction != null) {
                                 throw new ParsingException(parser.getTokenLocation(), "failed to parse function_score functions. already found [{}], now encountering [{}].", scoreFunction.getName(), currentFieldName);
                             }
-                            // do not need to check null here,
+                            // do not need to check null here, functionParserMapper does it already
-                            // functionParserMapper throws exception if parser
-                            // non-existent
                             ScoreFunctionParser functionParser = functionParserMapper.get(parser.getTokenLocation(), currentFieldName);
                             scoreFunction = functionParser.fromXContent(parseContext, parser);
                         }
+                    } else if (token.isValue()) {
+                        if (parseContext.parseFieldMatcher().match(currentFieldName, WEIGHT_FIELD)) {
+                            functionWeight = parser.floatValue();
+                        } else {
+                            throw new ParsingException(parser.getTokenLocation(), "failed to parse [{}] query. field [{}] is not supported", FunctionScoreQueryBuilder.NAME, currentFieldName);
+                        }
                     }
                 }
                 if (functionWeight != null) {

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;
@@ -59,7 +58,6 @@ import java.util.Map;
 import static org.elasticsearch.common.xcontent.XContentFactory.jsonBuilder;
 import static org.elasticsearch.index.query.QueryBuilders.functionScoreQuery;
 import static org.elasticsearch.index.query.QueryBuilders.termQuery;
-import static org.elasticsearch.test.StreamsUtils.copyToStringFromClasspath;
 import static org.hamcrest.Matchers.closeTo;
 import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.either;
@@ -552,8 +550,22 @@ public class FunctionScoreQueryBuilderTests extends AbstractQueryTestCase<Functi
     }
 
     public void testMalformedThrowsException() throws IOException {
+        String json = "{\n" +
+            "    \"function_score\":{\n" +
+            "        \"query\":{\n" +
+            "            \"term\":{\n" +
+            "                \"name.last\":\"banon\"\n" +
+            "            }\n" +
+            "        },\n" +
+            "        \"functions\": [\n" +
+            "            {\n" +
+            "                {\n" +
+            "            }\n" +
+            "        ]\n" +
+            "    }\n" +
+            "}";
         try {
-            parseQuery(copyToStringFromClasspath("/org/elasticsearch/index/query/faulty-function-score-query.json"));
+            parseQuery(json);
             fail("Expected JsonParseException");
         } catch (JsonParseException e) {
             assertThat(e.getMessage(), containsString("Unexpected character ('{"));
@@ -630,4 +642,79 @@ public class FunctionScoreQueryBuilderTests extends AbstractQueryTestCase<Functi
         assertEquals(json, 100, parsed.maxBoost(), 0.00001);
         assertEquals(json, 1, parsed.getMinScore(), 0.0001);
     }
+
+    public void testQueryMalformedArrayNotSupported() throws IOException {
+        String json =
+            "{\n" +
+                "  \"function_score\" : {\n" +
+                "    \"not_supported\" : []\n" +
+                "  }\n" +
+                "}";
+
+        try {
+            parseQuery(json);
+            fail("parse should have failed");
+        } catch (ParsingException e) {
+            assertThat(e.getMessage(), containsString("array [not_supported] is not supported"));
+        }
+    }
+
+    public void testQueryMalformedFieldNotSupported() throws IOException {
+        String json =
+            "{\n" +
+                "  \"function_score\" : {\n" +
+                "    \"not_supported\" : \"value\"\n" +
+                "  }\n" +
+                "}";
+
+        try {
+            parseQuery(json);
+            fail("parse should have failed");
+        } catch (ParsingException e) {
+            assertThat(e.getMessage(), containsString("field [not_supported] is not supported"));
+        }
+    }
+
+    public void testMalformedQueryFunctionFieldNotSupported() throws IOException {
+        String json =
+            "{\n" +
+                "  \"function_score\" : {\n" +
+                "    \"functions\" : [ {\n" +
+                "      \"not_supported\" : 23.0\n" +
+                "    }\n" +
+                "  }\n" +
+                "}";
+
+        try {
+            parseQuery(json);
+            fail("parse should have failed");
+        } catch (ParsingException e) {
+            assertThat(e.getMessage(), containsString("field [not_supported] is not supported"));
+        }
+    }
+
+    public void testMalformedQuery() throws IOException {
+        //verify that an error is thrown rather than setting the query twice (https://github.com/elastic/elasticsearch/issues/16583)
+        String json =
+            "{\n" +
+                "    \"function_score\":{\n" +
+                "        \"query\":{\n" +
+                "            \"bool\":{\n" +
+                "                \"must\":{\"match\":{\"field\":\"value\"}}" +
+                "             },\n" +
+                "            \"ignored_field_name\": {\n" +
+                "                {\"match\":{\"field\":\"value\"}}\n" +
+                "            }\n" +
+                "            }\n" +
+                "        }\n" +
+                "    }\n" +
+                "}";
+
+        try {
+            parseQuery(json);
+            fail("parse should have failed");
+        } catch(ParsingException e) {
+            assertThat(e.getMessage(), containsString("[query] is already defined."));
+        }
+    }
 }

core/src/test/resources/org/elasticsearch/index/query/faulty-function-score-query.json

@@ -1,15 +0,0 @@
-{
-    "function_score":{
-        "query":{
-            "term":{
-                "name.last":"banon"
-            }
-        },
-        "functions": { 
-            {
-            "boost_factor" : 3
-            }
-            }
-        }
-    }
-}