Disallow lang to be used with Stored Scripts

client/rest-high-level/src/test/java/org/elasticsearch/client/documentation/CRUDDocumentationIT.java

@@ -283,7 +283,7 @@ public void testUpdate() throws IOException {
             request = new UpdateRequest("posts", "doc", "1").fetchSource(true);
             //tag::update-request-with-stored-script
             Script stored =
-                    new Script(ScriptType.STORED, "painless", "increment-field", parameters);  // <1>
+                    new Script(ScriptType.STORED, null, "increment-field", parameters);  // <1>
             request.script(stored);  // <2>
             //end::update-request-with-stored-script
             updateResponse = client.update(request);

client/rest-high-level/src/test/java/org/elasticsearch/client/documentation/QueryDSLDocumentationTests.java

@@ -339,7 +339,7 @@ public void testScript() {
         parameters.put("param1", 5);
         scriptQuery(new Script(
                 ScriptType.STORED,                                   // <1>
-                "painless",                                          // <2>
+                null,                                          // <2>
                 "myscript",                                          // <3>
                 singletonMap("param1", 5)));                         // <4>
         // end::script_file

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

@@ -19,15 +19,12 @@
 
 package org.elasticsearch.script;
 
-import org.apache.logging.log4j.Logger;
 import org.elasticsearch.Version;
 import org.elasticsearch.common.ParseField;
 import org.elasticsearch.common.bytes.BytesArray;
 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.DeprecationLogger;
-import org.elasticsearch.common.logging.ESLoggerFactory;
 import org.elasticsearch.common.xcontent.ObjectParser;
 import org.elasticsearch.common.xcontent.ObjectParser.ValueType;
 import org.elasticsearch.common.xcontent.ToXContentObject;
@@ -66,8 +63,7 @@
  * <li> {@link ScriptType#STORED}
  * <ul>
  * <li> {@link Script#lang}     - the language will be specified when storing the script, so this should
- *                                be {@code null}; however, this can be specified to look up a stored
- *                                script as part of the deprecated API
+ *                                be {@code null}
  * <li> {@link Script#idOrCode} - specifies the id of the stored script to be looked up, must not be {@code null}
  * <li> {@link Script#options}  - compiler options will be specified when a stored script is stored,
  *                                so they have no meaning here and must be {@code null}
@@ -78,16 +74,6 @@
  */
 public final class Script implements ToXContentObject, Writeable {
 
-    /**
-     * Standard logger necessary for allocation of the deprecation logger.
-     */
-    private static final Logger LOGGER = ESLoggerFactory.getLogger(ScriptMetaData.class);
-
-    /**
-     * Deprecation logger necessary for namespace changes related to stored scripts.
-     */
-    private static final DeprecationLogger DEPRECATION_LOGGER = new DeprecationLogger(LOGGER);
-
     /**
      * The name of the of the default scripting language.
      */
@@ -233,7 +219,7 @@ private Script build(String defaultLang) {
 
                 if (idOrCode == null) {
                     throw new IllegalArgumentException(
-                        "must specify <id> for an [" + ScriptType.INLINE.getParseField().getPreferredName() + "] script");
+                        "must specify <id> for an inline script");
                 }
 
                 if (options.size() > 1 || options.size() == 1 && options.get(CONTENT_TYPE_OPTION) == null) {
@@ -242,26 +228,21 @@ private Script build(String defaultLang) {
                     throw new IllegalArgumentException("illegal compiler options [" + options + "] specified");
                 }
             } else if (type == ScriptType.STORED) {
-                // Only issue this deprecation warning if we aren't using a template.  Templates during
-                // this deprecation phase must always specify the default template language or they would
-                // possibly pick up a script in a different language as defined by the user under the new
-                // namespace unintentionally.
-                if (lang != null && lang.equals(DEFAULT_TEMPLATE_LANG) == false) {
-                    DEPRECATION_LOGGER.deprecated("specifying the field [" + LANG_PARSE_FIELD.getPreferredName() + "] " +
-                        "for executing " + ScriptType.STORED + " scripts is deprecated; use only the field " +
-                        "[" + ScriptType.STORED.getParseField().getPreferredName() + "] to specify an <id>");
+                if (lang != null) {
+                    throw new IllegalArgumentException(
+                        "illegally specified <lang> for a stored script");
                 }
 
                 if (idOrCode == null) {
                     throw new IllegalArgumentException(
-                        "must specify <code> for an [" + ScriptType.STORED.getParseField().getPreferredName() + "] script");
+                        "must specify <code> for a stored script");
                 }
 
                 if (options.isEmpty()) {
                     options = null;
                 } else {
                     throw new IllegalArgumentException("field [" + OPTIONS_PARSE_FIELD.getPreferredName() + "] " +
-                        "cannot be specified using a [" + ScriptType.STORED.getParseField().getPreferredName() + "] script");
+                        "cannot be specified using a stored script");
                 }
             }
 
@@ -423,11 +404,14 @@ public Script(ScriptType type, String lang, String idOrCode, Map<String, String>
             this.lang = Objects.requireNonNull(lang);
             this.options = Collections.unmodifiableMap(Objects.requireNonNull(options));
         } else if (type == ScriptType.STORED) {
-            this.lang = lang;
+            if (lang != null) {
+                throw new IllegalArgumentException("lang cannot be specified for stored scripts");
+            }
+
+            this.lang = null;
 
             if (options != null) {
-                throw new IllegalStateException(
-                    "options must be null for [" + ScriptType.STORED.getParseField().getPreferredName() + "] scripts");
+                throw new IllegalStateException("options cannot be specified for stored scripts");
             }
 
             this.options = null;
@@ -455,7 +439,8 @@ public Script(StreamInput in) throws IOException {
         // same order as the constructor.
         } else if (in.getVersion().onOrAfter(Version.V_5_1_1)) {
             this.type = ScriptType.readFrom(in);
-            this.lang = in.readString();
+            String lang = in.readString();
+            this.lang = this.type == ScriptType.STORED ? null : lang;
 
             this.idOrCode = in.readString();
             @SuppressWarnings("unchecked")
@@ -482,7 +467,7 @@ public Script(StreamInput in) throws IOException {
             String lang = in.readOptionalString();
 
             if (lang == null) {
-                this.lang = DEFAULT_SCRIPT_LANG;
+                this.lang = this.type == ScriptType.STORED ? null : DEFAULT_SCRIPT_LANG;
             } else {
                 this.lang = lang;
             }

core/src/main/java/org/elasticsearch/script/ScriptService.java

@@ -231,28 +231,11 @@ public <FactoryType> FactoryType compile(Script script, ScriptContext<FactoryTyp
 
         String id = idOrCode;
 
-        // lang may be null when looking up a stored script, so we must get the
-        // source to retrieve the lang before checking if the context is supported
         if (type == ScriptType.STORED) {
-            // search template requests can possibly pass in the entire path instead
-            // of just an id for looking up a stored script, so we parse the path and
-            // check for appropriate errors
-            String[] path = id.split("/");
-
-            if (path.length == 3) {
-                if (lang != null && lang.equals(path[1]) == false) {
-                    throw new IllegalStateException("conflicting script languages, found [" + path[1] + "] but expected [" + lang + "]");
-                }
-
-                id = path[2];
-
-                deprecationLogger.deprecated("use of </lang/id> [" + idOrCode + "] for looking up" +
-                    " stored scripts/templates has been deprecated, use only <id> [" + id + "] instead");
-            } else if (path.length != 1) {
-                throw new IllegalArgumentException("illegal stored script format [" + id + "] use only <id>");
-            }
-
-            // a stored script must be pulled from the cluster state every time in case
+            // * lang and options will both be null when looking up a stored script,
+            // so we must get the source to retrieve the them before checking if the
+            // context is supported
+            // * a stored script must be pulled from the cluster state every time in case
             // the script has been updated since the last compilation
             StoredScriptSource source = getScriptFromClusterState(id, lang);
             lang = source.getLang();
@@ -262,7 +245,7 @@ public <FactoryType> FactoryType compile(Script script, ScriptContext<FactoryTyp
 
         // TODO: fix this through some API or something, that's wrong
         // special exception to prevent expressions from compiling as update or mapping scripts
-        boolean expression = "expression".equals(script.getLang());
+        boolean expression = "expression".equals(lang);
         boolean notSupported = context.name.equals(ExecutableScript.UPDATE_CONTEXT.name);
         if (expression && notSupported) {
             throw new UnsupportedOperationException("scripts of type [" + script.getType() + "]," +
@@ -303,7 +286,6 @@ public <FactoryType> FactoryType compile(Script script, ScriptContext<FactoryTyp
                 try {
                     // Either an un-cached inline script or indexed script
                     // If the script type is inline the name will be the same as the code for identification in exceptions
-
                     // but give the script engine the chance to be better, give it separate name + source code
                     // for the inline case, then its anonymous: null.
                     if (logger.isTraceEnabled()) {
@@ -379,22 +361,16 @@ public boolean isAnyContextEnabled() {
     }
 
     StoredScriptSource getScriptFromClusterState(String id, String lang) {
-        if (lang != null && isLangSupported(lang) == false) {
-            throw new IllegalArgumentException("unable to get stored script with unsupported lang [" + lang + "]");
-        }
-
         ScriptMetaData scriptMetadata = clusterState.metaData().custom(ScriptMetaData.TYPE);
 
         if (scriptMetadata == null) {
-            throw new ResourceNotFoundException("unable to find script [" + id + "]" +
-                (lang == null ? "" : " using lang [" + lang + "]") + " in cluster state");
+            throw new ResourceNotFoundException("unable to find script [" + id + "] in cluster state");
         }
 
         StoredScriptSource source = scriptMetadata.getStoredScript(id, lang);
 
         if (source == null) {
-            throw new ResourceNotFoundException("unable to find script [" + id + "]" +
-                (lang == null ? "" : " using lang [" + lang + "]") + " in cluster state");
+            throw new ResourceNotFoundException("unable to find script [" + id + "] in cluster state");
         }
 
         return source;

core/src/test/java/org/elasticsearch/script/ScriptServiceTests.java

@@ -18,13 +18,6 @@
  */
 package org.elasticsearch.script;
 
-import java.io.IOException;
-import java.nio.file.Path;
-import java.util.Collections;
-import java.util.HashMap;
-import java.util.Map;
-import java.util.function.Function;
-
 import org.elasticsearch.ResourceNotFoundException;
 import org.elasticsearch.action.admin.cluster.storedscripts.GetStoredScriptRequest;
 import org.elasticsearch.cluster.ClusterName;
@@ -40,6 +33,12 @@
 import org.elasticsearch.test.ESTestCase;
 import org.junit.Before;
 
+import java.io.IOException;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.function.Function;
+
 import static org.hamcrest.CoreMatchers.containsString;
 import static org.hamcrest.Matchers.notNullValue;
 import static org.hamcrest.Matchers.sameInstance;
@@ -77,8 +76,11 @@ private void buildScriptService(Settings additionalSettings) throws IOException
         scriptService = new ScriptService(finalSettings, engines, contexts) {
             @Override
             StoredScriptSource getScriptFromClusterState(String id, String lang) {
+                if (lang != null) {
+                    throw new IllegalArgumentException("expected null lang in test");
+                }
                 //mock the script that gets retrieved from an index
-                return new StoredScriptSource(lang, "1+1", Collections.emptyMap());
+                return new StoredScriptSource("test", "1+1", Collections.emptyMap());
             }
         };
     }
@@ -128,7 +130,7 @@ public void testAllowAllScriptTypeSettings() throws IOException {
         buildScriptService(Settings.EMPTY);
 
         assertCompileAccepted("painless", "script", ScriptType.INLINE, SearchScript.CONTEXT);
-        assertCompileAccepted("painless", "script", ScriptType.STORED, SearchScript.CONTEXT);
+        assertCompileAccepted(null, "script", ScriptType.STORED, SearchScript.CONTEXT);
     }
 
     public void testAllowAllScriptContextSettings() throws IOException {
@@ -146,7 +148,7 @@ public void testAllowSomeScriptTypeSettings() throws IOException {
         buildScriptService(builder.build());
 
         assertCompileAccepted("painless", "script", ScriptType.INLINE, SearchScript.CONTEXT);
-        assertCompileRejected("painless", "script", ScriptType.STORED, SearchScript.CONTEXT);
+        assertCompileRejected(null, "script", ScriptType.STORED, SearchScript.CONTEXT);
     }
 
     public void testAllowSomeScriptContextSettings() throws IOException {
@@ -165,7 +167,7 @@ public void testAllowNoScriptTypeSettings() throws IOException {
         buildScriptService(builder.build());
 
         assertCompileRejected("painless", "script", ScriptType.INLINE, SearchScript.CONTEXT);
-        assertCompileRejected("painless", "script", ScriptType.STORED, SearchScript.CONTEXT);
+        assertCompileRejected(null, "script", ScriptType.STORED, SearchScript.CONTEXT);
     }
 
     public void testAllowNoScriptContextSettings() throws IOException {
@@ -183,7 +185,7 @@ public void testCompileNonRegisteredContext() throws IOException {
 
         String type = scriptEngine.getType();
         IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () ->
-            scriptService.compile(new Script(randomFrom(ScriptType.values()), type, "test", Collections.emptyMap()), ExecutableScript.INGEST_CONTEXT));
+            scriptService.compile(new Script(ScriptType.INLINE, type, "test", Collections.emptyMap()), ExecutableScript.INGEST_CONTEXT));
         assertThat(e.getMessage(), containsString("script context [" + ExecutableScript.INGEST_CONTEXT.name + "] not supported"));
     }
 
@@ -216,7 +218,7 @@ public void testCompilationStatsOnCacheHit() throws IOException {
 
     public void testIndexedScriptCountedInCompilationStats() throws IOException {
         buildScriptService(Settings.EMPTY);
-        scriptService.compile(new Script(ScriptType.STORED, "test", "script", Collections.emptyMap()), randomFrom(contexts.values()));
+        scriptService.compile(new Script(ScriptType.STORED, null, "script", Collections.emptyMap()), randomFrom(contexts.values()));
         assertEquals(1L, scriptService.stats().getCompilations());
     }
 

core/src/test/java/org/elasticsearch/search/aggregations/metrics/ScriptedMetricIT.java

@@ -791,13 +791,13 @@ public void testInitMapCombineReduceWithParamsStored() {
                         scriptedMetric("scripted")
                                 .params(params)
                                 .initScript(
-                                    new Script(ScriptType.STORED, CustomScriptPlugin.NAME, "initScript_stored", Collections.emptyMap()))
+                                    new Script(ScriptType.STORED, null, "initScript_stored", Collections.emptyMap()))
                                 .mapScript(
-                                    new Script(ScriptType.STORED, CustomScriptPlugin.NAME, "mapScript_stored", Collections.emptyMap()))
+                                    new Script(ScriptType.STORED, null, "mapScript_stored", Collections.emptyMap()))
                                 .combineScript(
-                                    new Script(ScriptType.STORED, CustomScriptPlugin.NAME, "combineScript_stored", Collections.emptyMap()))
+                                    new Script(ScriptType.STORED, null, "combineScript_stored", Collections.emptyMap()))
                                 .reduceScript(
-                                    new Script(ScriptType.STORED, CustomScriptPlugin.NAME, "reduceScript_stored", Collections.emptyMap())))
+                                    new Script(ScriptType.STORED, null, "reduceScript_stored", Collections.emptyMap())))
                 .get();
         assertSearchResponse(response);
         assertThat(response.getHits().getTotalHits(), equalTo(numDocs));

core/src/test/java/org/elasticsearch/search/aggregations/pipeline/BucketScriptIT.java

@@ -496,7 +496,7 @@ public void testStoredScript() {
                                 .subAggregation(sum("field4Sum").field(FIELD_4_NAME))
                                 .subAggregation(
                                         bucketScript("seriesArithmetic",
-                                                new Script(ScriptType.STORED, CustomScriptPlugin.NAME, "my_script", Collections.emptyMap()),
+                                                new Script(ScriptType.STORED, null, "my_script", Collections.emptyMap()),
                                                 "field2Sum", "field3Sum", "field4Sum"))).execute().actionGet();
 
         assertSearchResponse(response);

core/src/test/java/org/elasticsearch/search/aggregations/pipeline/BucketSelectorIT.java

@@ -438,7 +438,7 @@ public void testStoredScript() {
                 .setContent(new BytesArray("{ \"script\": \"Double.isNaN(_value0) ? false : (_value0 + _value1 > 100)\" }"),
                     XContentType.JSON));
 
-        Script script = new Script(ScriptType.STORED, CustomScriptPlugin.NAME, "my_script", Collections.emptyMap());
+        Script script = new Script(ScriptType.STORED, null, "my_script", Collections.emptyMap());
 
         SearchResponse response = client()
                 .prepareSearch("idx")

docs/reference/migration/migrate_6_0/scripting.asciidoc

@@ -26,3 +26,11 @@ The `_index` variable has been removed. If you used it for advanced scoring, con
 
 All of the existing scripting security settings have been removed.  Instead
 they are replaced with `script.allowed_types` and `script.allowed_contexts`.
+
+==== `lang` can no longer be specified when using a stored script as part of a request
+
+The `lang` variable can no longer be specified as part of a request that uses a stored
+script otherwise an error will occur.  Note that a request using a stored script is
+different from a request that puts a stored script.  The language of the script has
+already been stored as part of the cluster state and an `id` is sufficient to access
+all of the information necessary to execute a stored script.