-
Notifications
You must be signed in to change notification settings - Fork 25.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove deprecated code from stored scripts #78643
Changes from 7 commits
e63d7f4
c65b267
defb1af
ea688c1
ff5cf0b
725d156
c3287e8
9caaee8
79f19e7
943bb15
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -22,4 +22,29 @@ 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. | ||||||||||||||||||||
==== | ||||||||||||||||||||
|
||||||||||||||||||||
.Empty scripts/templates are no longer allowed as stored scripts. | ||||||||||||||||||||
[%collapsible] | ||||||||||||||||||||
==== | ||||||||||||||||||||
*Details* + | ||||||||||||||||||||
Empty scripts/templates cannot be specified as the source of a | ||||||||||||||||||||
PutStoredScript action. | ||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a reason we reference the It doesn't look like it in the code, but the wording made me wonder if this only impacted the Java client. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No! Just a mistake on my part. |
||||||||||||||||||||
|
||||||||||||||||||||
*Impact* + | ||||||||||||||||||||
Before upgrading, remove stored scripts/templates that are empty. Otherwise, | ||||||||||||||||||||
these scripts will be dropped, and may not align with the scripts on older | ||||||||||||||||||||
nodes. | ||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added a link to the delete stored script API. I assume the user will use that API to remove the scripts.
Suggested change
My suggestion doesn't include this because I wasn't sure what it meant. Does it just mean that behavior will be different after the upgrade? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. Poor wording on my part. I'm adopting your suggestion as the entirety of the text here. |
||||||||||||||||||||
==== | ||||||||||||||||||||
|
||||||||||||||||||||
.The `code` field can no longer be specified as part of a stored script/template. | ||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed. |
||||||||||||||||||||
[%collapsible] | ||||||||||||||||||||
==== | ||||||||||||||||||||
*Details* + | ||||||||||||||||||||
The `code` field is replaced by the `source` field as the only way to specify | ||||||||||||||||||||
the source of a script. | ||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed. |
||||||||||||||||||||
|
||||||||||||||||||||
*Impact* + | ||||||||||||||||||||
Before upgrading, any calls to PutStoredScript should use `source` instead | ||||||||||||||||||||
of `code`. | ||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed. |
||||||||||||||||||||
==== | ||||||||||||||||||||
// end::notable-breaking-changes[] |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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,30 @@ 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())) { | ||
deprecationLogger.critical(DeprecationCategory.TEMPLATES, "empty_templates", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think these should be warning logs, not deprecations. The deprecation already happened, this is a warning that we are dropping something meaningless. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
"empty template found and dropped"); | ||
} else { | ||
deprecationLogger.critical(DeprecationCategory.TEMPLATES, "empty_scripts", | ||
"empty script 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 +258,6 @@ public void writeTo(StreamOutput out) throws IOException { | |
} | ||
} | ||
|
||
|
||
|
||
/** | ||
* This will write XContent from {@link ScriptMetadata}. The following format will be written: | ||
* | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.