Cleanup ScriptType #21179
Cleanup ScriptType #21179
Conversation
jdconrad
added
:Core/Infra/Scripting
| public static void writeTo(ScriptType scriptType, StreamOutput out) throws IOException{ | ||
| if (scriptType != null) { | ||
| out.writeVInt(scriptType.val); | ||
| if (FILE.id == id) { |
There was a problem hiding this comment.
maybe so switch(id) {? looks cleaner?
There was a problem hiding this comment.
This is a bit awkward because the id is not considered a compile-time constant since it's set through the enum constructor. One option would be to add compile time constants for each id and use that instead, but I think the if statement is probably simpler.
| * - 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 { |
There was a problem hiding this comment.
I wonder if we can implement Writeable here then we don't need the static writeTo method anymore.
There was a problem hiding this comment.
Doing this now. Will re-test and commit once the tests are good.
| /** | ||
| * 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 for security purposes. |
There was a problem hiding this comment.
They are turned off for groovy by default because it is insecure but they are on by default for painless and mustache.
| /** | ||
| * 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 for security purposes. |
| } else { | ||
| out.writeVInt(INLINE.val); //Default to inline | ||
| throw new IllegalStateException("Error reading ScriptType id [" + id + "] from stream, expected one of [" + |
| this.scriptType = scriptType; | ||
| this.defaultScriptEnabled = defaultScriptEnabled; | ||
| /** | ||
| * Writes an int to the output stream based on the id of the {@link ScriptType}. |
There was a problem hiding this comment.
I don't think you need to explain that wire format for the enum is an int.
There was a problem hiding this comment.
Removed. Should inherit the base class JavaDoc.
| */ | ||
| ScriptType(int id, String name, ParseField parseField, boolean defaultEnabled) { | ||
| this.id = id; | ||
| this.name = name; |
There was a problem hiding this comment.
The name is always the name in the ParseField so maybe we should just get it from there?
There was a problem hiding this comment.
Good idea. Forces the name of the ParseField to be the same as the name of the enum.
clintongormley
added
:Core/Infra/Scripting
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.