From 340f344e47b545235deaf5222e350da3dbd06b36 Mon Sep 17 00:00:00 2001 From: Luca Cavanna Date: Tue, 14 Jul 2020 10:56:44 +0200 Subject: [PATCH 1/2] Consolidate script parsing from object 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. --- .../reindex/RestUpdateByQueryAction.java | 72 +----------- .../java/org/elasticsearch/script/Script.java | 79 +++++++++++++ .../org/elasticsearch/script/ScriptTests.java | 107 ++++++++++++++++++ 3 files changed, 187 insertions(+), 71 deletions(-) diff --git a/modules/reindex/src/main/java/org/elasticsearch/index/reindex/RestUpdateByQueryAction.java b/modules/reindex/src/main/java/org/elasticsearch/index/reindex/RestUpdateByQueryAction.java index e82df722bcdf1..f6d5b1647fef5 100644 --- a/modules/reindex/src/main/java/org/elasticsearch/index/reindex/RestUpdateByQueryAction.java +++ b/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 { @@ -69,7 +63,7 @@ protected UpdateByQueryRequest buildRequest(RestRequest request) throws IOExcept Map> 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 @@ protected UpdateByQueryRequest buildRequest(RestRequest request) throws IOExcept 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 configMap = (Map) config; - String script = null; - ScriptType type = null; - String lang = null; - Map params = Collections.emptyMap(); - for (Iterator> itr = configMap.entrySet().iterator(); itr.hasNext();) { - Map.Entry 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) 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"); - } - } } diff --git a/server/src/main/java/org/elasticsearch/script/Script.java b/server/src/main/java/org/elasticsearch/script/Script.java index a4b2dfb8b29a6..98c482f807c18 100644 --- a/server/src/main/java/org/elasticsearch/script/Script.java +++ b/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 static Script parse(XContentParser parser, String defaultLang) throws IOE 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 configMap = (Map) config; + String script = null; + ScriptType type = null; + String lang = null; + Map params = Collections.emptyMap(); + Map options = Collections.emptyMap(); + for (Map.Entry 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) 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) 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; diff --git a/server/src/test/java/org/elasticsearch/script/ScriptTests.java b/server/src/test/java/org/elasticsearch/script/ScriptTests.java index 8b66bb32c486e..614b212d603bc 100644 --- a/server/src/test/java/org/elasticsearch/script/ScriptTests.java +++ b/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.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.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,108 @@ public void testParse() throws IOException { } } } + + 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 map = new HashMap<>(); + map.put("source", "doc['my_field']"); + Map 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 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 params = new HashMap<>(); + int numParams = randomIntBetween(0, 3); + for (int i = 0; i < numParams; i++) { + params.put("param" + i, i); + } + Map 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 scriptObject = XContentHelper.convertToMap(XContentType.JSON.xContent(), Strings.toString(script), false); + Script parsedScript = Script.parse(scriptObject); + assertEquals(script, parsedScript); + } + + public void testParseFromObjectWrongFormat() { + { + 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 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 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()); + } } From ae05fb79eb6f79c5fda0d82e885790e40addc249 Mon Sep 17 00:00:00 2001 From: Luca Cavanna Date: Tue, 14 Jul 2020 11:06:22 +0200 Subject: [PATCH 2/2] iter --- server/src/main/java/org/elasticsearch/script/Script.java | 2 +- .../test/java/org/elasticsearch/script/ScriptTests.java | 7 +++++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/script/Script.java b/server/src/main/java/org/elasticsearch/script/Script.java index 98c482f807c18..1b5566c5ca1c5 100644 --- a/server/src/main/java/org/elasticsearch/script/Script.java +++ b/server/src/main/java/org/elasticsearch/script/Script.java @@ -414,7 +414,7 @@ public static Script parse(XContentParser parser, String defaultLang) throws IOE */ @SuppressWarnings("unchecked") public static Script parse(Object config) { - Objects.requireNonNull(config, "script must not be null"); + Objects.requireNonNull(config, "Script must not be null"); if (config instanceof String) { return new Script((String) config); } else if (config instanceof Map) { diff --git a/server/src/test/java/org/elasticsearch/script/ScriptTests.java b/server/src/test/java/org/elasticsearch/script/ScriptTests.java index 614b212d603bc..58560b1d1a85a 100644 --- a/server/src/test/java/org/elasticsearch/script/ScriptTests.java +++ b/server/src/test/java/org/elasticsearch/script/ScriptTests.java @@ -158,6 +158,13 @@ public void testParseFromObjectFromScript() { } 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,