Consolidate script parsing from object (#59507)

The update by query action parses a script from an object (map or string). We will need to do the same for runtime fields as they are parsed as part of mappings (#59391).

This commit moves the existing parsing of a script from an object from RestUpdateByQueryAction to the Script class. It also adds tests and adjusts some error messages that are incorrect. Also, options were not parsed before and they are now. And unsupported fields are no longer silently ignored.

modules/reindex/src/main/java/org/elasticsearch/index/reindex/RestUpdateByQueryAction.java

@@ -19,23 +19,17 @@
 
 package org.elasticsearch.index.reindex;
 
-import org.elasticsearch.ElasticsearchParseException;
 import org.elasticsearch.client.node.NodeClient;
-import org.elasticsearch.common.xcontent.LoggingDeprecationHandler;
 import org.elasticsearch.rest.RestRequest;
 import org.elasticsearch.script.Script;
-import org.elasticsearch.script.ScriptType;
 
 import java.io.IOException;
-import java.util.Collections;
 import java.util.HashMap;
-import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
 import java.util.function.Consumer;
 
 import static org.elasticsearch.rest.RestRequest.Method.POST;
-import static org.elasticsearch.script.Script.DEFAULT_SCRIPT_LANG;
 
 public class RestUpdateByQueryAction extends AbstractBulkByQueryRestHandler<UpdateByQueryRequest, UpdateByQueryAction> {
 
@@ -69,7 +63,7 @@ public class RestUpdateByQueryAction extends AbstractBulkByQueryRestHandler<Upda
 
         Map<String, Consumer<Object>> consumers = new HashMap<>();
         consumers.put("conflicts", o -> internal.setConflicts((String) o));
-        consumers.put("script", o -> internal.setScript(parseScript(o)));
+        consumers.put("script", o -> internal.setScript(Script.parse(o)));
         consumers.put("max_docs", s -> setMaxDocsValidateIdentical(internal, ((Number) s).intValue()));
 
         parseInternalRequest(internal, request, consumers);
@@ -77,68 +71,4 @@ public class RestUpdateByQueryAction extends AbstractBulkByQueryRestHandler<Upda
         internal.setPipeline(request.param("pipeline"));
         return internal;
     }
-
-    @SuppressWarnings("unchecked")
-    private static Script parseScript(Object config) {
-        assert config != null : "Script should not be null";
-
-        if (config instanceof String) {
-            return new Script((String) config);
-        } else if (config instanceof Map) {
-            Map<String,Object> configMap = (Map<String, Object>) config;
-            String script = null;
-            ScriptType type = null;
-            String lang = null;
-            Map<String, Object> params = Collections.emptyMap();
-            for (Iterator<Map.Entry<String, Object>> itr = configMap.entrySet().iterator(); itr.hasNext();) {
-                Map.Entry<String, Object> entry = itr.next();
-                String parameterName = entry.getKey();
-                Object parameterValue = entry.getValue();
-                if (Script.LANG_PARSE_FIELD.match(parameterName, LoggingDeprecationHandler.INSTANCE)) {
-                    if (parameterValue instanceof String || parameterValue == null) {
-                        lang = (String) parameterValue;
-                    } else {
-                        throw new ElasticsearchParseException("Value must be of type String: [" + parameterName + "]");
-                    }
-                } else if (Script.PARAMS_PARSE_FIELD.match(parameterName, LoggingDeprecationHandler.INSTANCE)) {
-                    if (parameterValue instanceof Map || parameterValue == null) {
-                        params = (Map<String, Object>) parameterValue;
-                    } else {
-                        throw new ElasticsearchParseException("Value must be of type String: [" + parameterName + "]");
-                    }
-                } else if (ScriptType.INLINE.getParseField().match(parameterName, LoggingDeprecationHandler.INSTANCE)) {
-                    if (parameterValue instanceof String || parameterValue == null) {
-                        script = (String) parameterValue;
-                        type = ScriptType.INLINE;
-                    } else {
-                        throw new ElasticsearchParseException("Value must be of type String: [" + parameterName + "]");
-                    }
-                } else if (ScriptType.STORED.getParseField().match(parameterName, LoggingDeprecationHandler.INSTANCE)) {
-                    if (parameterValue instanceof String || parameterValue == null) {
-                        script = (String) parameterValue;
-                        type = ScriptType.STORED;
-                    } else {
-                        throw new ElasticsearchParseException("Value must be of type String: [" + parameterName + "]");
-                    }
-                }
-            }
-            if (script == null) {
-                throw new ElasticsearchParseException("expected one of [{}] or [{}] fields, but found none",
-                    ScriptType.INLINE.getParseField().getPreferredName(), ScriptType.STORED.getParseField().getPreferredName());
-            }
-            assert type != null : "if script is not null, type should definitely not be null";
-
-            if (type == ScriptType.STORED) {
-                if (lang != null) {
-                    throw new IllegalArgumentException("lang cannot be specified for stored scripts");
-                }
-
-                return new Script(type, null, script, null, params);
-            } else {
-                return new Script(type, lang == null ? DEFAULT_SCRIPT_LANG : lang, script, params);
-            }
-        } else {
-            throw new IllegalArgumentException("Script value should be a String or a Map");
-        }
-    }
 }

server/src/main/java/org/elasticsearch/script/Script.java

@@ -19,6 +19,7 @@
 
 package org.elasticsearch.script;
 
+import org.elasticsearch.ElasticsearchParseException;
 import org.elasticsearch.common.ParseField;
 import org.elasticsearch.common.Strings;
 import org.elasticsearch.common.bytes.BytesArray;
@@ -405,6 +406,84 @@ public final class Script implements ToXContentObject, Writeable {
         return PARSER.apply(parser, null).build(defaultLang);
     }
 
+    /**
+     * Parse a {@link Script} from an {@link Object}, that can either be a {@link String} or a {@link Map}.
+     * @see #parse(XContentParser, String)
+     * @param config  The object to parse the script from.
+     * @return        The parsed {@link Script}.
+     */
+    @SuppressWarnings("unchecked")
+    public static Script parse(Object config) {
+        Objects.requireNonNull(config, "Script must not be null");
+        if (config instanceof String) {
+            return new Script((String) config);
+        } else if (config instanceof Map) {
+            Map<String,Object> configMap = (Map<String, Object>) config;
+            String script = null;
+            ScriptType type = null;
+            String lang = null;
+            Map<String, Object> params = Collections.emptyMap();
+            Map<String, String> options = Collections.emptyMap();
+            for (Map.Entry<String, Object> entry : configMap.entrySet()) {
+                String parameterName = entry.getKey();
+                Object parameterValue = entry.getValue();
+                if (Script.LANG_PARSE_FIELD.match(parameterName, LoggingDeprecationHandler.INSTANCE)) {
+                    if (parameterValue instanceof String || parameterValue == null) {
+                        lang = (String) parameterValue;
+                    } else {
+                        throw new ElasticsearchParseException("Value must be of type String: [" + parameterName + "]");
+                    }
+                } else if (Script.PARAMS_PARSE_FIELD.match(parameterName, LoggingDeprecationHandler.INSTANCE)) {
+                    if (parameterValue instanceof Map || parameterValue == null) {
+                        params = (Map<String, Object>) parameterValue;
+                    } else {
+                        throw new ElasticsearchParseException("Value must be of type Map: [" + parameterName + "]");
+                    }
+                } else if (Script.OPTIONS_PARSE_FIELD.match(parameterName, LoggingDeprecationHandler.INSTANCE)) {
+                    if (parameterValue instanceof Map || parameterValue == null) {
+                        options = (Map<String, String>) parameterValue;
+                    } else {
+                        throw new ElasticsearchParseException("Value must be of type Map: [" + parameterName + "]");
+                    }
+                } else if (ScriptType.INLINE.getParseField().match(parameterName, LoggingDeprecationHandler.INSTANCE)) {
+                    if (parameterValue instanceof String || parameterValue == null) {
+                        script = (String) parameterValue;
+                        type = ScriptType.INLINE;
+                    } else {
+                        throw new ElasticsearchParseException("Value must be of type String: [" + parameterName + "]");
+                    }
+                } else if (ScriptType.STORED.getParseField().match(parameterName, LoggingDeprecationHandler.INSTANCE)) {
+                    if (parameterValue instanceof String || parameterValue == null) {
+                        script = (String) parameterValue;
+                        type = ScriptType.STORED;
+                    } else {
+                        throw new ElasticsearchParseException("Value must be of type String: [" + parameterName + "]");
+                    }
+                } else {
+                    throw new ElasticsearchParseException("Unsupported field [" + parameterName + "]");
+                }
+            }
+            if (script == null) {
+                throw new ElasticsearchParseException("Expected one of [{}] or [{}] fields, but found none",
+                    ScriptType.INLINE.getParseField().getPreferredName(), ScriptType.STORED.getParseField().getPreferredName());
+            }
+            assert type != null : "if script is not null, type should definitely not be null";
+
+            if (type == ScriptType.STORED) {
+                if (lang != null) {
+                    throw new IllegalArgumentException("[" + Script.LANG_PARSE_FIELD.getPreferredName() +
+                        "] cannot be specified for stored scripts");
+                }
+
+                return new Script(type, null, script, null, params);
+            } else {
+                return new Script(type, lang == null ? DEFAULT_SCRIPT_LANG : lang, script, options, params);
+            }
+        } else {
+            throw new IllegalArgumentException("Script value should be a String or a Map");
+        }
+    }
+
     private final ScriptType type;
     private final String lang;
     private final String idOrCode;

server/src/test/java/org/elasticsearch/script/ScriptTests.java

@@ -19,6 +19,7 @@
 
 package org.elasticsearch.script;
 
+import org.elasticsearch.ElasticsearchParseException;
 import org.elasticsearch.common.Strings;
 import org.elasticsearch.common.io.stream.InputStreamStreamInput;
 import org.elasticsearch.common.io.stream.OutputStreamStreamOutput;
@@ -26,6 +27,7 @@ import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.common.xcontent.ToXContent;
 import org.elasticsearch.common.xcontent.XContentBuilder;
 import org.elasticsearch.common.xcontent.XContentFactory;
+import org.elasticsearch.common.xcontent.XContentHelper;
 import org.elasticsearch.common.xcontent.XContentParser;
 import org.elasticsearch.common.xcontent.XContentType;
 import org.elasticsearch.test.ESTestCase;
@@ -34,6 +36,7 @@ import java.io.ByteArrayInputStream;
 import java.io.ByteArrayOutputStream;
 import java.io.IOException;
 import java.util.Collections;
+import java.util.HashMap;
 import java.util.Map;
 
 import static org.hamcrest.Matchers.equalTo;
@@ -96,4 +99,115 @@ public class ScriptTests extends ESTestCase {
             }
         }
     }
+
+    public void testParseFromObjectShortSyntax() {
+        Script script = Script.parse("doc['my_field']");
+        assertEquals(Script.DEFAULT_SCRIPT_LANG, script.getLang());
+        assertEquals("doc['my_field']", script.getIdOrCode());
+        assertEquals(0, script.getParams().size());
+        assertEquals(0, script.getOptions().size());
+        assertEquals(ScriptType.INLINE, script.getType());
+    }
+
+    public void testParseFromObject() {
+        Map<String, Object> map = new HashMap<>();
+        map.put("source", "doc['my_field']");
+        Map<String, Object> params = new HashMap<>();
+        int numParams = randomIntBetween(0, 3);
+        for (int i = 0; i < numParams; i++) {
+            params.put("param" + i, i);
+        }
+        map.put("params", params);
+        Map<String, String> options = new HashMap<>();
+        int numOptions = randomIntBetween(0, 3);
+        for (int i = 0; i < numOptions; i++) {
+            options.put("option" + i, Integer.toString(i));
+        }
+        map.put("options", options);
+        String lang = Script.DEFAULT_SCRIPT_LANG;;
+        if (randomBoolean()) {
+            map.put("lang", lang);
+        } else if(randomBoolean()) {
+            lang = "expression";
+            map.put("lang", lang);
+        }
+
+        Script script = Script.parse(map);
+        assertEquals(lang, script.getLang());
+        assertEquals("doc['my_field']", script.getIdOrCode());
+        assertEquals(ScriptType.INLINE, script.getType());
+        assertEquals(params, script.getParams());
+        assertEquals(options, script.getOptions());
+    }
+
+    public void testParseFromObjectFromScript() {
+        Map<String, Object> params = new HashMap<>();
+        int numParams = randomIntBetween(0, 3);
+        for (int i = 0; i < numParams; i++) {
+            params.put("param" + i, i);
+        }
+        Map<String, String> options = new HashMap<>();
+        int numOptions = randomIntBetween(0, 3);
+        for (int i = 0; i < numOptions; i++) {
+            options.put("option" + i, Integer.toString(i));
+        }
+        Script script = new Script(ScriptType.INLINE, Script.DEFAULT_SCRIPT_LANG, "doc['field']", options, params);
+        Map<String, Object> scriptObject = XContentHelper.convertToMap(XContentType.JSON.xContent(), Strings.toString(script), false);
+        Script parsedScript = Script.parse(scriptObject);
+        assertEquals(script, parsedScript);
+    }
+
+    public void testParseFromObjectWrongFormat() {
+        {
+            NullPointerException exc = expectThrows(
+                NullPointerException.class,
+                () -> Script.parse((Object)null)
+            );
+            assertEquals("Script must not be null", exc.getMessage());
+        }
+        {
+            IllegalArgumentException exc = expectThrows(
+                IllegalArgumentException.class,
+                () -> Script.parse(3)
+            );
+            assertEquals("Script value should be a String or a Map", exc.getMessage());
+        }
+        {
+            ElasticsearchParseException exc = expectThrows(
+                ElasticsearchParseException.class,
+                () -> Script.parse(Collections.emptyMap())
+            );
+            assertEquals("Expected one of [source] or [id] fields, but found none", exc.getMessage());
+        }
+    }
+
+    public void testParseFromObjectUnsupportedFields() {
+        ElasticsearchParseException exc = expectThrows(
+            ElasticsearchParseException.class,
+            () -> Script.parse(Map.of("source", "script", "unsupported", "value"))
+        );
+        assertEquals("Unsupported field [unsupported]", exc.getMessage());
+    }
+
+    public void testParseFromObjectWrongOptionsFormat() {
+        Map<String, Object> map = new HashMap<>();
+        map.put("source", "doc['my_field']");
+        map.put("options", 3);
+        ElasticsearchParseException exc = expectThrows(
+            ElasticsearchParseException.class,
+            () -> Script.parse(map)
+        );
+        assertEquals("Value must be of type Map: [options]", exc.getMessage());
+    }
+
+    public void testParseFromObjectWrongParamsFormat() {
+        Map<String, Object> map = new HashMap<>();
+        map.put("source", "doc['my_field']");
+        map.put("params", 3);
+        ElasticsearchParseException exc = expectThrows(
+            ElasticsearchParseException.class,
+            () -> Script.parse(map)
+        );
+        assertEquals("Value must be of type Map: [params]", exc.getMessage());
+    }
 }