Remove deprecated code from stored scripts (#78643)

jdconrad · web-flow · commit 2cf160f2c00f · 2021-10-05T10:41:39.000-07:00
This change removes several pieces of deprecated code from stored scripts.

Stored scripts/templates are no longer allowed to be an empty and will throw an exception when used 
with PutStoredScript.

ScriptMetadata will now drop any existing stored scripts that are empty with a deprecation warning in 
the case they have not been previously removed.

The code field is now only allowed as source as part of a PutStoredScript JSON blob.
diff --git a/docs/reference/migration/migrate_8_0/scripting.asciidoc b/docs/reference/migration/migrate_8_0/scripting.asciidoc
@@ -22,4 +22,31 @@ scripts. Any use of `getDayOfWeek` expecting a return value of `int` will result
 in a compilation error or runtime error and may not allow the upgraded node to
 start.
 ====
+
+.Stored scripts no longer support empty scripts or search templates.
+[%collapsible]
+====
+*Details* +
+The {ref}/create-stored-script-api.html[create or update stored script API]'s
+`source` parameter cannot be empty.
+
+*Impact* +
+Before upgrading, use the {ref}/delete-stored-script-api.html[delete stored
+script API] to delete any empty stored scripts or search templates.
+In 8.0, {es} will drop any empty stored scripts or empty search templates from
+the cluster state. Requests to create a stored script or search template with
+an empty `source` will return an error.
+====
+
+.The create or update stored script API's `code` parameter has been removed.
+[%collapsible]
+====
+*Details* +
+The {ref}/create-stored-script-api.html[create or update stored script API]'s
+`code` parameter has been removed. Use the `source` parameter instead.
+
+*Impact* +
+Discontinue use of the `code` parameter. Requests that include the parameter
+will return an error.
+====
 // end::notable-breaking-changes[]
diff --git a/server/src/internalClusterTest/java/org/elasticsearch/script/StoredScriptsIT.java b/server/src/internalClusterTest/java/org/elasticsearch/script/StoredScriptsIT.java
@@ -55,7 +55,7 @@ public void testBasics() {
 
         IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> client().admin().cluster().preparePutStoredScript()
                 .setId("id#")
-                .setContent(new BytesArray("{}"), XContentType.JSON)
+                .setContent(new BytesArray("{\"script\": {\"lang\": \"" + LANG + "\", \"source\": \"1\"} }"), XContentType.JSON)
                 .get());
         assertEquals("Validation Failed: 1: id cannot contain '#' for stored script;", e.getMessage());
     }
diff --git a/server/src/main/java/org/elasticsearch/script/ScriptMetadata.java b/server/src/main/java/org/elasticsearch/script/ScriptMetadata.java
@@ -7,6 +7,8 @@
  */
 package org.elasticsearch.script;
 
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
 import org.elasticsearch.ResourceNotFoundException;
 import org.elasticsearch.Version;
 import org.elasticsearch.cluster.ClusterState;
@@ -18,8 +20,6 @@
 import org.elasticsearch.common.io.stream.StreamInput;
 import org.elasticsearch.common.io.stream.StreamOutput;
 import org.elasticsearch.common.io.stream.Writeable;
-import org.elasticsearch.common.logging.DeprecationCategory;
-import org.elasticsearch.common.logging.DeprecationLogger;
 import org.elasticsearch.common.xcontent.ToXContentFragment;
 import org.elasticsearch.common.xcontent.XContentBuilder;
 import org.elasticsearch.common.xcontent.XContentParser;
@@ -38,9 +38,9 @@
 public final class ScriptMetadata implements Metadata.Custom, Writeable, ToXContentFragment {
 
     /**
-     * Standard deprecation logger for used to deprecate allowance of empty templates.
+     * Standard logger used to warn about dropped scripts.
      */
-    private static final DeprecationLogger deprecationLogger = DeprecationLogger.getLogger(ScriptMetadata.class);
+    private static final Logger logger = LogManager.getLogger(ScriptMetadata.class);
 
     /**
      * A builder used to modify the currently stored scripts data held within
@@ -162,16 +162,10 @@ static ScriptMetadata deleteStoredScript(ScriptMetadata previous, String id) {
      *     ...
      * }
      * }
-     *
-     * When loading from a source prior to 6.0, if multiple scripts
-     * using the old namespace id format of [lang#id] are found to have the
-     * same id but different languages an error will occur.
      */
     public static ScriptMetadata fromXContent(XContentParser parser) throws IOException {
         Map<String, StoredScriptSource> scripts = new HashMap<>();
         String id = null;
-        StoredScriptSource source;
-        StoredScriptSource exists;
 
         Token token = parser.currentToken();
 
@@ -189,84 +183,28 @@ public static ScriptMetadata fromXContent(XContentParser parser) throws IOExcept
             switch (token) {
                 case FIELD_NAME:
                     id = parser.currentName();
-                    break;
-                case VALUE_STRING:
-                    if (id == null) {
-                        throw new ParsingException(parser.getTokenLocation(),
-                            "unexpected token [" + token + "], expected [<id>, <code>, {]");
-                    }
-
-                    int split = id.indexOf('#');
-                    String lang;
-
-                    if (split == -1) {
-                        throw new IllegalArgumentException("illegal stored script id [" + id + "], does not contain lang");
-                    } else {
-                        lang = id.substring(0, split);
-                        id = id.substring(split + 1);
-                        source = new StoredScriptSource(lang, parser.text(), Collections.emptyMap());
-
-                        if (source.getSource().isEmpty()) {
-                            if (source.getLang().equals(Script.DEFAULT_TEMPLATE_LANG)) {
-                                deprecationLogger.critical(
-                                    DeprecationCategory.TEMPLATES,
-                                    "empty_templates",
-                                    "empty templates should no longer be used"
-                                );
-                            } else {
-                                deprecationLogger.critical(
-                                    DeprecationCategory.TEMPLATES,
-                                    "empty_scripts",
-                                    "empty scripts should no longer be used"
-                                );
-                            }
-                        }
-                    }
-
-                    exists = scripts.get(id);
-
-                    if (exists == null) {
-                        scripts.put(id, source);
-                    } else if (exists.getLang().equals(lang) == false) {
-                        throw new IllegalArgumentException("illegal stored script, id [" + id + "] used for multiple scripts with " +
-                            "different languages [" + exists.getLang() + "] and [" + lang + "]; scripts using the old namespace " +
-                            "of [lang#id] as a stored script id will have to be updated to use only the new namespace of [id]");
-                    }
-
-                    id = null;
-
                     break;
                 case START_OBJECT:
                     if (id == null) {
                         throw new ParsingException(parser.getTokenLocation(),
                             "unexpected token [" + token + "], expected [<id>, <code>, {]");
                     }
 
-                    exists = scripts.get(id);
-                    source = StoredScriptSource.fromXContent(parser, true);
-
-                    if (exists == null) {
-                        // due to a bug (https://github.com/elastic/elasticsearch/issues/47593)
-                        // scripts may have been retained during upgrade that include the old-style
-                        // id of lang#id; these scripts are unreachable after 7.0, so they are dropped
-                        if (id.contains("#") == false) {
-                            scripts.put(id, source);
+                    StoredScriptSource source = StoredScriptSource.fromXContent(parser, true);
+                    // as of 8.0 we drop scripts/templates with an empty source
+                    // this check should be removed for the next upgradable version after 8.0
+                    // since there is a guarantee no more empty scripts will exist
+                    if (source.getSource().isEmpty()) {
+                        if (Script.DEFAULT_TEMPLATE_LANG.equals(source.getLang())) {
+                            logger.warn("empty template [" + id + "] found and dropped");
+                        } else {
+                            logger.warn("empty script [" + id + "] found and dropped");
                         }
-                    } else if (exists.getLang().equals(source.getLang()) == false) {
-                        throw new IllegalArgumentException(
-                            "illegal stored script, id ["
-                                + id
-                                + "] used for multiple scripts with different languages ["
-                                + exists.getLang()
-                                + "] and ["
-                                + source.getLang()
-                                + "]; scripts using the old namespace of [lang#id] as a stored script id will have to be updated "
-                                + "to use only the new namespace of [id]"
-                        );
+                    } else {
+                        scripts.put(id, source);
                     }
 
                     id = null;
-
                     break;
                 default:
                     throw new ParsingException(parser.getTokenLocation(), "unexpected token [" + token + "], expected [<id>, <code>, {]");
@@ -318,8 +256,6 @@ public void writeTo(StreamOutput out) throws IOException {
         }
     }
 
-
-
     /**
      * This will write XContent from {@link ScriptMetadata}.  The following format will be written:
      *
diff --git a/server/src/main/java/org/elasticsearch/script/StoredScriptSource.java b/server/src/main/java/org/elasticsearch/script/StoredScriptSource.java
@@ -11,19 +11,18 @@
 import org.elasticsearch.cluster.AbstractDiffable;
 import org.elasticsearch.cluster.ClusterState;
 import org.elasticsearch.cluster.Diff;
-import org.elasticsearch.common.xcontent.ParseField;
 import org.elasticsearch.common.ParsingException;
 import org.elasticsearch.common.Strings;
 import org.elasticsearch.common.bytes.BytesReference;
 import org.elasticsearch.common.io.stream.StreamInput;
 import org.elasticsearch.common.io.stream.StreamOutput;
 import org.elasticsearch.common.io.stream.Writeable;
-import org.elasticsearch.common.logging.DeprecationCategory;
 import org.elasticsearch.common.logging.DeprecationLogger;
 import org.elasticsearch.common.xcontent.LoggingDeprecationHandler;
 import org.elasticsearch.common.xcontent.NamedXContentRegistry;
 import org.elasticsearch.common.xcontent.ObjectParser;
 import org.elasticsearch.common.xcontent.ObjectParser.ValueType;
+import org.elasticsearch.common.xcontent.ParseField;
 import org.elasticsearch.common.xcontent.ToXContentObject;
 import org.elasticsearch.common.xcontent.XContentBuilder;
 import org.elasticsearch.common.xcontent.XContentFactory;
@@ -63,7 +62,7 @@ public class StoredScriptSource extends AbstractDiffable<StoredScriptSource> imp
     /**
      * Standard {@link ParseField} for source on the inner level.
      */
-    public static final ParseField SOURCE_PARSE_FIELD = new ParseField("source", "code");
+    public static final ParseField SOURCE_PARSE_FIELD = new ParseField("source");
 
     /**
      * Standard {@link ParseField} for options on the inner level.
@@ -121,8 +120,7 @@ private void setOptions(Map<String, String> options) {
          * Validates the parameters and creates an {@link StoredScriptSource}.
          *
          * @param ignoreEmpty Specify as {@code true} to ignoreEmpty the empty source check.
-         *                    This allow empty templates to be loaded for backwards compatibility.
-         *                    This allow empty templates to be loaded for backwards compatibility.
+         *                    This allow empty templates to be dropped for backwards compatibility.
          */
         private StoredScriptSource build(boolean ignoreEmpty) {
             if (lang == null) {
@@ -132,29 +130,13 @@ private StoredScriptSource build(boolean ignoreEmpty) {
             }
 
             if (source == null) {
-                if (ignoreEmpty || Script.DEFAULT_TEMPLATE_LANG.equals(lang)) {
-                    if (Script.DEFAULT_TEMPLATE_LANG.equals(lang)) {
-                        deprecationLogger.critical(DeprecationCategory.TEMPLATES, "empty_templates",
-                            "empty templates should no longer be used");
-                    } else {
-                        deprecationLogger.critical(DeprecationCategory.TEMPLATES, "empty_scripts",
-                            "empty scripts should no longer be used");
-                    }
+                if (ignoreEmpty) {
+                    source = "";
                 } else {
                     throw new IllegalArgumentException("must specify source for stored script");
                 }
-            } else if (source.isEmpty()) {
-                if (ignoreEmpty || Script.DEFAULT_TEMPLATE_LANG.equals(lang)) {
-                    if (Script.DEFAULT_TEMPLATE_LANG.equals(lang)) {
-                        deprecationLogger.critical(DeprecationCategory.TEMPLATES, "empty_templates",
-                            "empty templates should no longer be used");
-                    } else {
-                        deprecationLogger.critical(DeprecationCategory.TEMPLATES, "empty_scripts",
-                            "empty scripts should no longer be used");
-                    }
-                } else {
-                    throw new IllegalArgumentException("source cannot be empty");
-                }
+            } else if (source.isEmpty() && ignoreEmpty == false) {
+                throw new IllegalArgumentException("source cannot be empty");
             }
 
             if (options.size() > 1 || options.size() == 1 && options.get(Script.CONTENT_TYPE_OPTION) == null) {
@@ -175,21 +157,9 @@ private StoredScriptSource build(boolean ignoreEmpty) {
     }
 
     /**
-     * This will parse XContent into a {@link StoredScriptSource}.  The following formats can be parsed:
-     *
-     * The simple script format with no compiler options or user-defined params:
-     *
-     * Example:
-     * {@code
-     * {"script": "return Math.log(doc.popularity) * 100;"}
-     * }
+     * This will parse XContent into a {@link StoredScriptSource}.
      *
-     * The above format requires the lang to be specified using the deprecated stored script namespace
-     * (as a url parameter during a put request).  See {@link ScriptMetadata} for more information about
-     * the stored script namespaces.
-     *
-     * The complex script format using the new stored script namespace
-     * where lang and source are required but options is optional:
+     * Examples of legal JSON:
      *
      * {@code
      * {
@@ -215,22 +185,6 @@ private StoredScriptSource build(boolean ignoreEmpty) {
      * }
      * }
      *
-     * The use of "source" may also be substituted with "code" for backcompat with 5.3 to 5.5 format. For example:
-     *
-     * {@code
-     * {
-     *     "script" : {
-     *         "lang" : "<lang>",
-     *         "code" : "<source>",
-     *         "options" : {
-     *             "option0" : "<option0>",
-     *             "option1" : "<option1>",
-     *             ...
-     *         }
-     *     }
-     * }
-     * }
-     *
      * Note that the "source" parameter can also handle template parsing including from
      * a complex JSON object.
      *
@@ -249,12 +203,6 @@ public static StoredScriptSource parse(BytesReference content, XContentType xCon
 
             token = parser.nextToken();
 
-            if (token == Token.END_OBJECT) {
-                deprecationLogger.critical(DeprecationCategory.TEMPLATES, "empty_templates", "empty templates should no longer be used");
-
-                return new StoredScriptSource(Script.DEFAULT_TEMPLATE_LANG, "", Collections.emptyMap());
-            }
-
             if (token != Token.FIELD_NAME) {
                 throw new ParsingException(parser.getTokenLocation(), "unexpected token [" + token + ", expected [" +
                     SCRIPT_PARSE_FIELD.getPreferredName() + "]");
diff --git a/server/src/test/java/org/elasticsearch/script/ScriptMetadataTests.java b/server/src/test/java/org/elasticsearch/script/ScriptMetadataTests.java
diff --git a/server/src/test/java/org/elasticsearch/script/StoredScriptTests.java b/server/src/test/java/org/elasticsearch/script/StoredScriptTests.java

Original file line number	Diff line number	Diff line change
`@@ -55,7 +55,7 @@ public void testBasics() {`
`55`	`55`
`56`	`56`	`IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> client().admin().cluster().preparePutStoredScript()`
`57`	`57`	`.setId("id#")`
`58`		`- .setContent(new BytesArray("{}"), XContentType.JSON)`
	`58`	`+ .setContent(new BytesArray("{\"script\": {\"lang\": \"" + LANG + "\", \"source\": \"1\"} }"), XContentType.JSON)`
`59`	`59`	`.get());`
`60`	`60`	`assertEquals("Validation Failed: 1: id cannot contain '#' for stored script;", e.getMessage());`
`61`	`61`	`}`