Don't pass XContentParser to ParseFieldRegistry#lookup (#17794)

Don't pass XContentParser to ParseFieldRegistry#lookup

Passing the parser in is not good because we are not parsing anything in the lookup methods, we only need it to retrieve the xcontent location from it so that in case there is an error we emit where we were with the parsing. It is better to pass in the XContentLocation although calling getTokenLocation needs to create a new object at each call. The workaround of passing the Parser in is worse than the original problem.

core/src/main/java/org/elasticsearch/cluster/routing/allocation/command/AllocationCommands.java

@@ -159,7 +159,7 @@ public class AllocationCommands {
                 token = parser.nextToken();
                 String commandName = parser.currentName();
                 token = parser.nextToken();
-                commands.add(registry.lookup(commandName, parser, parseFieldMatcher).fromXContent(parser));
+                commands.add(registry.lookup(commandName, parseFieldMatcher, parser.getTokenLocation()).fromXContent(parser));
                 // move to the end object one
                 if (parser.nextToken() != XContentParser.Token.END_OBJECT) {
                     throw new ElasticsearchParseException("allocation command is malformed, done parsing a command, but didn't get END_OBJECT, got [{}] instead", token);

core/src/main/java/org/elasticsearch/common/xcontent/ParseFieldRegistry.java

@@ -68,15 +68,14 @@ public class ParseFieldRegistry<T> {
      * Lookup a value from the registry by name while checking that the name matches the ParseField.
      *
      * @param name The name of the thing to look up.
-     * @param parser The parser from which the name was looked up.
      * @param parseFieldMatcher to build nice error messages.
      * @return The value being looked up. Never null.
      * @throws ParsingException if the named thing isn't in the registry or the name was deprecated and deprecated names aren't supported.
      */
-    public T lookup(String name, XContentParser parser, ParseFieldMatcher parseFieldMatcher) {
+    public T lookup(String name, ParseFieldMatcher parseFieldMatcher, XContentLocation xContentLocation) {
         T value = lookupReturningNullIfNotFound(name, parseFieldMatcher);
         if (value == null) {
-            throw new ParsingException(parser.getTokenLocation(), "no [" + registryName + "] registered for [" + name + "]");
+            throw new ParsingException(xContentLocation, "no [" + registryName + "] registered for [" + name + "]");
         }
         return value;
     }

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

@@ -19,9 +19,6 @@
 
 package org.elasticsearch.index.query;
 
-import java.io.IOException;
-import java.util.Objects;
-
 import org.elasticsearch.common.ParseField;
 import org.elasticsearch.common.ParseFieldMatcher;
 import org.elasticsearch.common.ParseFieldMatcherSupplier;
@@ -29,6 +26,9 @@ import org.elasticsearch.common.ParsingException;
 import org.elasticsearch.common.xcontent.XContentParser;
 import org.elasticsearch.indices.query.IndicesQueriesRegistry;
 
+import java.io.IOException;
+import java.util.Objects;
+
 public class QueryParseContext implements ParseFieldMatcherSupplier {
 
     private static final ParseField CACHE = new ParseField("_cache").withAllDeprecated("Elasticsearch makes its own caching decisions");
@@ -105,7 +105,7 @@ public class QueryParseContext implements ParseFieldMatcherSupplier {
         if (token != XContentParser.Token.START_OBJECT && token != XContentParser.Token.START_ARRAY) {
             throw new ParsingException(parser.getTokenLocation(), "[_na] query malformed, no field after start_object");
         }
-        QueryBuilder<?> result = indicesQueriesRegistry.lookup(queryName, parser, parseFieldMatcher).fromXContent(this);
+        QueryBuilder<?> result = indicesQueriesRegistry.lookup(queryName, parseFieldMatcher, parser.getTokenLocation()).fromXContent(this);
         if (parser.currentToken() == XContentParser.Token.END_OBJECT || parser.currentToken() == XContentParser.Token.END_ARRAY) {
             // if we are at END_OBJECT, move to the next one...
             parser.nextToken();

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

@@ -477,7 +477,7 @@ public class FunctionScoreQueryBuilder extends AbstractQueryBuilder<FunctionScor
                     // we try to parse a score function. If there is no score function for the current field name,
                     // getScoreFunction will throw.
                     ScoreFunctionBuilder<?> scoreFunction = scoreFunctionsRegistry
-                            .lookup(currentFieldName, parseContext.parser(), parseContext.getParseFieldMatcher())
+                            .lookup(currentFieldName, parseContext.getParseFieldMatcher(), parser.getTokenLocation())
                             .fromXContent(parseContext);
                     filterFunctionBuilders.add(new FunctionScoreQueryBuilder.FilterFunctionBuilder(scoreFunction));
                 }
@@ -582,8 +582,8 @@ public class FunctionScoreQueryBuilder extends AbstractQueryBuilder<FunctionScor
                                         "failed to parse function_score functions. already found [{}], now encountering [{}].",
                                         scoreFunction.getName(), currentFieldName);
                             }
-                            scoreFunction = scoreFunctionsRegistry.lookup(currentFieldName, parser, parseContext.getParseFieldMatcher())
+                            scoreFunction = scoreFunctionsRegistry.lookup(currentFieldName, parseContext.getParseFieldMatcher(),
-                                    .fromXContent(parseContext);
+                                    parser.getTokenLocation()).fromXContent(parseContext);
                         }
                     } else if (token.isValue()) {
                         if (parseContext.getParseFieldMatcher().match(currentFieldName, WEIGHT_FIELD)) {

core/src/main/java/org/elasticsearch/search/aggregations/pipeline/movavg/MovAvgPipelineAggregatorBuilder.java

@@ -395,8 +395,8 @@ public class MovAvgPipelineAggregatorBuilder extends PipelineAggregatorBuilder<M
             factory.predict(predict);
         }
         if (model != null) {
-            MovAvgModel.AbstractModelParser modelParser = movingAverageMdelParserRegistry.lookup(model, parser,
+            MovAvgModel.AbstractModelParser modelParser = movingAverageMdelParserRegistry.lookup(model, context.getParseFieldMatcher(),
-                    context.getParseFieldMatcher());
+                    parser.getTokenLocation());
             MovAvgModel movAvgModel;
             try {
                 movAvgModel = modelParser.parse(settings, pipelineAggregatorName, factory.window(), context.getParseFieldMatcher());

core/src/test/java/org/elasticsearch/search/SearchModuleTests.java

@@ -99,16 +99,16 @@ public class SearchModuleTests extends ModuleTestCase {
         IndicesQueriesRegistry indicesQueriesRegistry = module.getQueryParserRegistry();
         XContentParser dummyParser = XContentHelper.createParser(new BytesArray("{}"));
         for (String queryName : supportedQueries) {
-            indicesQueriesRegistry.lookup(queryName, dummyParser, ParseFieldMatcher.EMPTY);
+            indicesQueriesRegistry.lookup(queryName, ParseFieldMatcher.EMPTY, dummyParser.getTokenLocation());
         }
 
         for (String queryName : NON_DEPRECATED_QUERIES) {
-            QueryParser<?> queryParser = indicesQueriesRegistry.lookup(queryName, dummyParser, ParseFieldMatcher.STRICT);
+            QueryParser<?> queryParser = indicesQueriesRegistry.lookup(queryName, ParseFieldMatcher.STRICT, dummyParser.getTokenLocation());
             assertThat(queryParser, notNullValue());
         }
         for (String queryName : DEPRECATED_QUERIES) {
             try {
-                indicesQueriesRegistry.lookup(queryName, dummyParser, ParseFieldMatcher.STRICT);
+                indicesQueriesRegistry.lookup(queryName, ParseFieldMatcher.STRICT, dummyParser.getTokenLocation());
                 fail("query is deprecated, getQueryParser should have failed in strict mode");
             } catch(IllegalArgumentException e) {
                 assertThat(e.getMessage(), containsString("Deprecated field [" + queryName + "] used"));