Cleanup ScriptType (#21179)

Refactored ScriptType to clean up some of the variable and method names. Added more documentation. Deprecated the 'in' ParseField in favor of 'stored' to match the indexed scripts being replaced by stored scripts.

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

@@ -109,7 +109,7 @@ public void writeTo(StreamOutput out) throws IOException {
         boolean hasType = type != null;
         out.writeBoolean(hasType);
         if (hasType) {
-            ScriptType.writeTo(type, out);
+            type.writeTo(out);
         }
         out.writeOptionalString(lang);
         out.writeMap(params);

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

@@ -72,7 +72,7 @@ static String operationKey(ScriptContext scriptContext) {
     }
 
     static String sourceKey(ScriptType scriptType) {
-        return SCRIPT_SETTINGS_PREFIX + "." + scriptType.getScriptType();
+        return SCRIPT_SETTINGS_PREFIX + "." + scriptType.getName();
     }
 
     static String getGlobalKey(String lang, ScriptType scriptType) {

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

@@ -50,7 +50,7 @@ public class ScriptSettings {
         for (ScriptType scriptType : ScriptType.values()) {
             scriptTypeSettingMap.put(scriptType, Setting.boolSetting(
                 ScriptModes.sourceKey(scriptType),
-                scriptType.getDefaultScriptEnabled(),
+                scriptType.isDefaultEnabled(),
                 Property.NodeScope));
         }
         SCRIPT_TYPE_SETTING_MAP = Collections.unmodifiableMap(scriptTypeSettingMap);
@@ -102,7 +102,7 @@ private static List<Setting<Boolean>> languageSettings(Map<ScriptType, Setting<B
                 boolean defaultLangAndType = defaultNonFileScriptMode;
                 // Files are treated differently because they are never default-deny
                 if (ScriptType.FILE == scriptType) {
-                    defaultLangAndType = ScriptType.FILE.getDefaultScriptEnabled();
+                    defaultLangAndType = ScriptType.FILE.isDefaultEnabled();
                 }
                 final boolean defaultIfNothingSet = defaultLangAndType;
 

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

@@ -22,68 +22,118 @@
 import org.elasticsearch.common.ParseField;
 import org.elasticsearch.common.io.stream.StreamInput;
 import org.elasticsearch.common.io.stream.StreamOutput;
+import org.elasticsearch.common.io.stream.Writeable;
 
 import java.io.IOException;
-import java.util.Locale;
 
 /**
- * The type of a script, more specifically where it gets loaded from:
- * - provided dynamically at request time
- * - loaded from an index
- * - loaded from file
+ * ScriptType represents the way a script is stored and retrieved from the {@link ScriptService}.
+ * It's also used to by {@link ScriptSettings} and {@link ScriptModes} to determine whether or not
+ * a {@link Script} is allowed to be executed based on both default and user-defined settings.
  */
-public enum ScriptType {
+public enum ScriptType implements Writeable {
 
-    INLINE(0, "inline", "inline", false),
-    STORED(1, "id", "stored", false),
-    FILE(2, "file", "file", true);
+    /**
+     * INLINE scripts are specified in numerous queries and compiled on-the-fly.
+     * They will be cached based on the lang and code of the script.
+     * They are turned off by default because most languages are insecure
+     * (Groovy and others), but can be overriden by the specific {@link ScriptEngineService}
+     * if the language is naturally secure (Painless, Mustache, and Expressions).
+     */
+    INLINE ( 0 , new ParseField("inline") , false ),
 
-    private final int val;
-    private final ParseField parseField;
-    private final String scriptType;
-    private final boolean defaultScriptEnabled;
+    /**
+     * STORED scripts are saved as part of the {@link org.elasticsearch.cluster.ClusterState}
+     * based on user requests.  They will be cached when they are first used in a query.
+     * They are turned off by default because most languages are insecure
+     * (Groovy and others), but can be overriden by the specific {@link ScriptEngineService}
+     * if the language is naturally secure (Painless, Mustache, and Expressions).
+     */
+    STORED ( 1 , new ParseField("stored", "id") , false ),
+
+    /**
+     * FILE scripts are loaded from disk either on start-up or on-the-fly depending on
+     * user-defined settings.  They will be compiled and cached as soon as they are loaded
+     * from disk.  They are turned on by default as they should always be safe to execute.
+     */
+    FILE ( 2 , new ParseField("file") , true  );
 
+    /**
+     * Reads an int from the input stream and converts it to a {@link ScriptType}.
+     * @return The ScriptType read from the stream. Throws an {@link IllegalStateException}
+     * if no ScriptType is found based on the id.
+     */
     public static ScriptType readFrom(StreamInput in) throws IOException {
-        int scriptTypeVal = in.readVInt();
-        for (ScriptType type : values()) {
-            if (type.val == scriptTypeVal) {
-                return type;
-            }
-        }
-        throw new IllegalArgumentException("Unexpected value read for ScriptType got [" + scriptTypeVal + "] expected one of ["
-                + INLINE.val + "," + FILE.val + "," + STORED.val + "]");
-    }
+        int id = in.readVInt();
 
-    public static void writeTo(ScriptType scriptType, StreamOutput out) throws IOException{
-        if (scriptType != null) {
-            out.writeVInt(scriptType.val);
+        if (FILE.id == id) {
+            return FILE;
+        } else if (STORED.id == id) {
+            return STORED;
+        } else if (INLINE.id == id) {
+            return INLINE;
         } else {
-            out.writeVInt(INLINE.val); //Default to inline
+            throw new IllegalStateException("Error reading ScriptType id [" + id + "] from stream, expected one of [" +
+                FILE.id + " [" + FILE.parseField.getPreferredName() + "], " +
+                STORED.id + " [" + STORED.parseField.getPreferredName() + "], " +
+                INLINE.id + " [" + INLINE.parseField.getPreferredName() + "]]");
         }
     }
 
-    ScriptType(int val, String name, String scriptType, boolean defaultScriptEnabled) {
-        this.val = val;
-        this.parseField = new ParseField(name);
-        this.scriptType = scriptType;
-        this.defaultScriptEnabled = defaultScriptEnabled;
+    private final int id;
+    private final ParseField parseField;
+    private final boolean defaultEnabled;
+
+    /**
+     * Standard constructor.
+     * @param id A unique identifier for a type that can be read/written to a stream.
+     * @param parseField Specifies the name used to parse input from queries.
+     * @param defaultEnabled Whether or not a {@link ScriptType} can be run by default.
+     */
+    ScriptType(int id, ParseField parseField, boolean defaultEnabled) {
+        this.id = id;
+        this.parseField = parseField;
+        this.defaultEnabled = defaultEnabled;
     }
 
-    public ParseField getParseField() {
-        return parseField;
+    public void writeTo(StreamOutput out) throws IOException {
+        out.writeVInt(id);
     }
 
-    public boolean getDefaultScriptEnabled() {
-        return defaultScriptEnabled;
+    /**
+     * @return The unique id for this {@link ScriptType}.
+     */
+    public int getId() {
+        return id;
     }
 
-    public String getScriptType() {
-        return scriptType;
+    /**
+     * @return The unique name for this {@link ScriptType} based on the {@link ParseField}.
+     */
+    public String getName() {
+        return parseField.getPreferredName();
     }
 
+    /**
+     * @return Specifies the name used to parse input from queries.
+     */
+    public ParseField getParseField() {
+        return parseField;
+    }
+
+    /**
+     * @return Whether or not a {@link ScriptType} can be run by default.  Note
+     * this can be potentially overriden by any {@link ScriptEngineService}.
+     */
+    public boolean isDefaultEnabled() {
+        return defaultEnabled;
+    }
+
+    /**
+     * @return The same as calling {@link #getName()}.
+     */
     @Override
     public String toString() {
-        return name().toLowerCase(Locale.ROOT);
+        return getName();
     }
-
 }

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

@@ -62,7 +62,7 @@ public void testFineGrainedSettingsDontAffectNativeScripts() throws IOException
         Settings.Builder builder = Settings.builder();
         if (randomBoolean()) {
             ScriptType scriptType = randomFrom(ScriptType.values());
-            builder.put("script" + "." + scriptType.getScriptType(), randomBoolean());
+            builder.put("script" + "." + scriptType.getName(), randomBoolean());
         } else {
             ScriptContext scriptContext = randomFrom(ScriptContext.Standard.values());
             builder.put("script" + "." + scriptContext.getKey(), randomBoolean());

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

@@ -125,7 +125,7 @@ public void testScriptTypeGenericSettings() {
         ScriptType[] randomScriptTypes = randomScriptTypesSet.toArray(new ScriptType[randomScriptTypesSet.size()]);
         Settings.Builder builder = Settings.builder();
         for (int i = 0; i < randomInt; i++) {
-            builder.put("script" + "." + randomScriptTypes[i].getScriptType(), randomScriptModes[i]);
+            builder.put("script" + "." + randomScriptTypes[i].getName(), randomScriptModes[i]);
         }
         this.scriptModes = new ScriptModes(scriptSettings, builder.build());
 

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

@@ -266,9 +266,9 @@ public void testFineGrainedSettings() throws IOException {
         Settings.Builder builder = Settings.builder();
         for (Map.Entry<ScriptType, Boolean> entry : scriptSourceSettings.entrySet()) {
             if (entry.getValue()) {
-                builder.put("script" + "." + entry.getKey().getScriptType(), "true");
+                builder.put("script" + "." + entry.getKey().getName(), "true");
             } else {
-                builder.put("script" + "." + entry.getKey().getScriptType(), "false");
+                builder.put("script" + "." + entry.getKey().getName(), "false");
             }
         }
         for (Map.Entry<ScriptContext, Boolean> entry : scriptContextSettings.entrySet()) {

docs/plugins/lang-javascript.asciidoc

@@ -120,7 +120,7 @@ GET test/_search
     "function_score": {
       "script_score": {
         "script": {
-          "id": "my_script", <2>
+          "stored": "my_script", <2>
           "lang": "javascript",
           "params": {
             "factor": 2

docs/plugins/lang-python.asciidoc

@@ -119,7 +119,7 @@ GET test/_search
     "function_score": {
       "script_score": {
         "script": {
-          "id": "my_script", <2>
+          "stored": "my_script", <2>
           "lang": "python",
           "params": {
             "factor": 2

docs/reference/modules/scripting/using.asciidoc

@@ -8,12 +8,12 @@ the same pattern:
 -------------------------------------
   "script": {
     "lang":   "...",  <1>
-    "inline" | "id" | "file": "...", <2>
+    "inline" | "stored" | "file": "...", <2>
     "params": { ... } <3>
   }
 -------------------------------------
 <1> The language the script is written in, which defaults to `painless`.
-<2> The script itself which may be specified as `inline`, `id`, or `file`.
+<2> The script itself which may be specified as `inline`, `stored`, or `file`.
 <3> Any named parameters that should be passed into the script.
 
 For example, the following script is used in a search request to return a
@@ -211,7 +211,7 @@ GET _scripts/groovy/calculate-score
 // CONSOLE
 // TEST[continued]
 
-Stored scripts can be used by specifying the `lang` and `id` parameters as follows:
+Stored scripts can be used by specifying the `lang` and `stored` parameters as follows:
 
 [source,js]
 --------------------------------------------------
@@ -221,7 +221,7 @@ GET _search
     "script": {
       "script": {
         "lang": "groovy",
-        "id":   "calculate-score",
+        "stored": "calculate-score",
         "params": {
           "my_modifier": 2
         }

docs/reference/query-dsl/template-query.asciidoc

@@ -108,7 +108,7 @@ GET /_search
 {
     "query": {
         "template": {
-            "id": "my_template", <1>
+            "stored": "my_template", <1>
             "params" : {
                 "query_string" : "all about search"
             }

modules/lang-mustache/src/main/java/org/elasticsearch/script/mustache/SearchTemplateRequest.java

@@ -157,7 +157,7 @@ public void writeTo(StreamOutput out) throws IOException {
         out.writeBoolean(simulate);
         out.writeBoolean(explain);
         out.writeBoolean(profile);
-        ScriptType.writeTo(scriptType, out);
+        scriptType.writeTo(out);
         out.writeOptionalString(script);
         boolean hasParams = scriptParams != null;
         out.writeBoolean(hasParams);

modules/lang-mustache/src/test/resources/rest-api-spec/test/lang_mustache/40_template_query.yaml

@@ -44,15 +44,15 @@
       warnings:
           - '[template] query is deprecated, use search template api instead'
       search:
-        body: { "query": { "template": { "id": "1", "params": { "my_value": "value1" } } } }
+        body: { "query": { "template": { "stored": "1", "params": { "my_value": "value1" } } } }
 
   - match: { hits.total: 1 }
 
   - do:
       warnings:
           - '[template] query is deprecated, use search template api instead'
       search:
-        body: { "query": { "template": { "id": "/mustache/1", "params": { "my_value": "value1" } } } }
+        body: { "query": { "template": { "stored": "/mustache/1", "params": { "my_value": "value1" } } } }
 
   - match: { hits.total: 1 }