Change Namespace for Stored Script to Only Use Id #22206

jdconrad · 2016-12-15T21:11:57Z

Currently, stored scripts use a namespace of (lang, id) to be put, get, deleted, and executed. This is not necessary since the lang is stored with the stored script. A user should only have to specify an id to use a stored script. This change makes that possible while keeping backwards compatibility with the previous namespace of (lang, id). Anywhere the previous namespace is used will log deprecation warnings. The goal is to backport this change to 5.2 and then remove the previous namespace altogether in 6.0 giving users the opportunity to change their scripts to the new namespace.

Why is this change so large?

This change seems huge, but it's not really. The main files changed are ScriptMetaData and ScriptService with StoredScriptSource being added. Secondarily, most of the Request files have changed a bit to support the new/old namespaces. Many lines that have changed are added JavaDocs and some new tests. Also maintaining back compat eats up some lines for XContent and the wire format.

What is the new behavior?

When a user specifies a stored script, that script will be stored under both the new namespace and old namespace.

Take for example script 'A' with lang 'L0' and data 'D0'. If we add script 'A' to the empty set, the scripts map will be ["A" -- D0, "A#L0" -- D0]. If a script 'A' with lang 'L1' and data 'D1' is then added, the scripts map will be ["A" -- D1, "A#L1" -- D1, "A#L0" -- D0].

When a user deletes a stored script, that script will be deleted from both the new namespace (if it exists) and the old namespace.

Take for example a scripts map with {"A" -- D1, "A#L1" -- D1, "A#L0" -- D0}. If a script is removed specified by an id 'A' and lang null then the scripts map will be {"A#L0" -- D0}. To remove the final script, the deprecated namespace must be used, so an id 'A' and lang 'L0' would need to be specified.

When a user gets/executes a stored script, if the new namespace is used then the script will be retrieved/executed using only 'id', and if the old namespace is used then the script will be retrieved/executed using 'id' and 'lang'

…space.

clintongormley · 2016-12-16T12:50:24Z

@jdconrad Have you added deprecation logging for when the old form (including lang) has been used?

jdconrad · 2016-12-16T17:24:49Z

@clintongormley Sorry about missing the deprecation tag! I think I always miss one. To answer your question I added deprecation logging everywhere user-input can come in from that uses lang.

martijnvg

In general looks good. A left a number of comments.

I think that we should add some bwc tests too. I think we can add some tests around stored scripts in the qa/rolling-upgrade module?

Also I think in a followup issue, the rest actions / specs should be cleanup so that only the new api remains. This change should only go into master.

martijnvg · 2016-12-21T10:20:32Z

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

+        // Version 5.2+ will output the contents of the scripts' Map using
+        // StoredScriptSource to stored the language, code, and options.
+        if (out.getVersion().onOrAfter(Version.V_5_2_0_UNRELEASED)) {
+            out.write(scripts.size());


this should be out.writeVInt(scripts.size());.

Otherwise we overflow after 127 stored scripts :)

Nice catch! :)

martijnvg · 2016-12-21T11:56:54Z

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

+         */
+        public Builder storeScript(String id, StoredScriptSource source) {
+            StoredScriptSource previous = scripts.put(id, source);
+            scripts.put(source.getLang() + "#" + id, source);


I wonder whether we only can add scripts under the new scheme. That way script with old scheme would over time slowly disappear. I think this should work as we already deal with old and new scheme in get and delete?

I think we only need to worry about the update case. I think that is covered if we change the method into this:

StoredScriptSource previous = scripts.remove(source.getLang() + "#" + id); if (previous != null && previous.getLang().equals(source.getLang()) == false) { DEPRECATION_LOGGER.deprecated("stored script [" + id + "] already exists using a different lang " + "[" + previous.getLang() + "], the new namespace for stored scripts will only use (id) instead of (lang, id)"); } scripts.put(id, source); return this;

I thought about this as well and it certainly makes sense, but the tricky part is the user has no choice for stored templates. I wanted to make sure that the user could still have a stored template with the same name as a script for this version so they have time to come up with an appropriate plan for the deprecation. With the correct planning the old scheme will simply disappear when loading the scripts from cluster state metadata because we can make it ignore (and print a warning) that an old script cannot be loaded similar to how file scripts work now for scripts that do not compile.

Also, right now this technically isn't a breaking change which I think supports the user a little better (though, hopefully no one is dependent on having scripts of the same name with different languages).

martijnvg · 2016-12-21T11:59:49Z

docs/reference/migration/migrate_6_0/scripting.asciidoc

 === Scripting changes

+
+


Accidental. Good catch!

martijnvg · 2016-12-21T12:20:36Z

modules/lang-painless/src/test/resources/rest-api-spec/test/painless/16_update2.yaml

@@ -1,14 +1,20 @@
 ---
 "Indexed script":


not part of this change, but this should be renamed to "Stored script" :)

martijnvg · 2016-12-21T12:23:25Z

modules/lang-painless/src/test/resources/rest-api-spec/test/painless/16_update2.yaml


  - do:
+     warnings:
+       - 'specifying lang [painless] as part of the url path is deprecated'


Also I think we should update the yaml specs to just describe the id and update the yaml tests accordingly.
So that the new behavior gets tested too.

We can add get_script_deprecated.json, delete_script_deprecated.json etc. to test that the old behaviour and that warnings get returned in the response.

Updated the tests to the new namespace.

martijnvg · 2016-12-21T12:30:45Z

.../src/main/java/org/elasticsearch/rest/action/admin/cluster/RestDeleteStoredScriptAction.java

-        return request.param("lang");
+        // Note {lang} is actually {id} in the first handler.  It appears
+        // parameters as part of the path must be of the same ordering relative
+        // to name or they will not work as expected.


true... I also don't see a way to make this nicer.

:(, but it will be removed in master anyway

martijnvg · 2016-12-21T12:36:53Z

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

+ * {@link StoredScriptSource} represents user-defined parameters for a script
+ * saved in the {@link ClusterState}.
+ */
+public class StoredScriptSource extends AbstractDiffable<StoredScriptSource> implements Writeable, ToXContent {


👍 much better than script as bytes

martijnvg · 2016-12-21T12:40:02Z

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

+        assertThat(rnfe.getMessage(), equalTo("stored script [test] does not exist and cannot be deleted"));
+    }
+
+    public void testSourceParsing() throws Exception {


@colings86 is adding base test classes for writeable and xcontent serialization / de-serialization via #22281

That should get merged quickly. When that is in can you make this test class extend from AbstractSerializingTestCase. That will add random serialization tests in addition that the serialization that exist already.

and maybe do the same for ScriptMetaDataTests?

martijnvg · 2016-12-21T12:49:49Z

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

+
+                    int split = id.indexOf('#');
+
+                    if (split == -1) {


I think we can move the split check to the place where we assign the id variable?

split is used in both the if and else blocks

jdconrad · 2016-12-21T15:43:13Z

@martijnvg Thank you so much for reviewing this! I agree that the old API should be removed in master -- the plan was to get this in, backport it to version 5.2 (although maybe 5.3 is more realistic given the upcoming holidays), and then remove the old API in master where we can.
I'll get started working on your other comments pretty soon.

jdconrad · 2017-01-19T23:58:08Z

Added tests for rolling upgrade. These will need to be updated when the code is backported to 5.3 and when 6.0 deletes the code for the old API.

jdconrad · 2017-01-20T01:59:08Z

@martijnvg I think this is ready for another round of review. I believe I addressed all the comments, and added several tests that you requested. Thanks!

jdconrad · 2017-01-23T21:31:31Z

@martijnvg I added the patch you sent me to address the bwc bugs you found with one minor change -- I decided to clean up the Script readFrom and writeTo a little bit to separate out each of the read and write versions into their own if/else block as I think that will help clear up potential future confusion. And thanks again so much for catching some of the bwc bugs! Ready for another round of review when you have the chance.

martijnvg

Thx Jack! LGTM

Maybe also open an issue to track to removal of the lang parameter from the script/template apis and internal classes for 6.0?

jdconrad · 2017-01-31T16:14:24Z

@martijnvg Thanks for all the reviews here! Added this as a follow up issue (#22887) I will commit this code once I get master integrated properly and all the tests passing.

Currently, stored scripts use a namespace of (lang, id) to be put, get, deleted, and executed. This is not necessary since the lang is stored with the stored script. A user should only have to specify an id to use a stored script. This change makes that possible while keeping backwards compatibility with the previous namespace of (lang, id). Anywhere the previous namespace is used will log deprecation warnings. The new behavior is the following: When a user specifies a stored script, that script will be stored under both the new namespace and old namespace. Take for example script 'A' with lang 'L0' and data 'D0'. If we add script 'A' to the empty set, the scripts map will be ["A" -- D0, "A#L0" -- D0]. If a script 'A' with lang 'L1' and data 'D1' is then added, the scripts map will be ["A" -- D1, "A#L1" -- D1, "A#L0" -- D0]. When a user deletes a stored script, that script will be deleted from both the new namespace (if it exists) and the old namespace. Take for example a scripts map with {"A" -- D1, "A#L1" -- D1, "A#L0" -- D0}. If a script is removed specified by an id 'A' and lang null then the scripts map will be {"A#L0" -- D0}. To remove the final script, the deprecated namespace must be used, so an id 'A' and lang 'L0' would need to be specified. When a user gets/executes a stored script, if the new namespace is used then the script will be retrieved/executed using only 'id', and if the old namespace is used then the script will be retrieved/executed using 'id' and 'lang'

clintongormley · 2017-02-06T10:17:45Z

@jdconrad in 6.0 i assume the namespace support will no longer be deprecated but will be removed? If so, please add a note to the migration docs for 6.0

jdconrad · 2017-02-06T16:43:03Z

@clintongormley Yes, it will be removed, but that requires another change to actually remove it from 6.0 that I'll try to follow up with this week. Right now, it's still technically deprecated there, but not removed. Once, I make the change I'll make sure to add that as a (breaking?) note to the migration docs.

Mpdreamz · 2017-03-22T15:07:09Z

@jdconrad should the spec for script api's in master be changed from:

      "path": "/_scripts/{lang}",
      "paths": [ "/_scripts/{lang}", "/_scripts/{lang}/{id}" ],

to

      "path": "/_scripts/{id}",
      "paths": [ "/_scripts/{id}" ],

Or has the removal not made it in yet to master ?

jdconrad and others added 14 commits December 15, 2016 11:51

Change stored script namespace to be (id) instead of (lang,id).

b2702d2

Fixed tests for stored scripts with new namespace.

ef65278

More test fixes.

323a95c

Updated script for deprecation of stored script namespace.

51d0a1f

Added documentation for the deprecation of the old stored script name…

75dcda5

…space.

More test changes.

3d81273

More test fixes.

17f3993

More test fixes.

240dcca

More test fixes.

6c3cedf

Test fix.

c67b2ed

Test fixes.

c64cedb

Added documentation.

24622e1

More documentation.

6e850a0

Added stored script tests.

162b9e9

jdconrad added :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache v5.2.0 v6.0.0-alpha1 labels Dec 15, 2016

Fix import.

160c201

clintongormley added the >deprecation label Dec 16, 2016

martijnvg requested changes Dec 21, 2016

View reviewed changes

Merge branch 'master' into stored4

ced00e6

jdconrad added v5.3.0 and removed v5.2.0 labels Jan 4, 2017

jdconrad added 4 commits January 4, 2017 17:28

Fixed some tests.

191c581

Merge branch 'master' into stored4

64cbec8

Merge branch 'master' into stored4

06b2dbd

Test fixes.

7ee0c07

jdconrad added 2 commits January 19, 2017 10:02

Merge branch 'master' into stored4

541cd0b

Added rolling upgrade tests.

4806ef9

jdconrad added 3 commits January 19, 2017 16:33

Added new serialization tests.

06d3afd

Merge branch 'master' into stored4

ebc3371

More test fixes.

8b58094

jdconrad added 2 commits January 23, 2017 10:02

Merge branch 'master' into stored4

8b3773a

Added requested tests from MVG and removed some bwc bugs.

21109b3

jdconrad added 3 commits January 24, 2017 08:14

Merge branch 'master' into stored4

7663f15

Merge branch 'master' into stored4

09e1ab2

Merge branch 'master' into stored4

81191e1

martijnvg approved these changes Jan 31, 2017

View reviewed changes

Merge branch 'master' into stored4

c0908b8

jdconrad mentioned this pull request Jan 31, 2017

Update Stored Scripts to Only Support the New Namespace #22887

Closed

jdconrad added 2 commits January 31, 2017 09:24

Fix tests.

7c925a4

Remove trace setting from rolling upgrade.

7696f94

jdconrad merged commit 3d2626c into elastic:master Jan 31, 2017

jdconrad deleted the stored4 branch February 21, 2017 18:40

jasontedor mentioned this pull request Apr 21, 2017

"POST’ing Search Template as REST API causes Elastic to fail on start" #24227

Closed

Change Namespace for Stored Script to Only Use Id #22206

Change Namespace for Stored Script to Only Use Id #22206

Uh oh!

Conversation

jdconrad commented Dec 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

clintongormley commented Dec 16, 2016

Uh oh!

jdconrad commented Dec 16, 2016

Uh oh!

martijnvg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jdconrad commented Dec 21, 2016

Uh oh!

jdconrad commented Jan 19, 2017

Uh oh!

jdconrad commented Jan 20, 2017

Uh oh!

jdconrad commented Jan 23, 2017

Uh oh!

martijnvg left a comment

Choose a reason for hiding this comment

Uh oh!

jdconrad commented Jan 31, 2017

Uh oh!

clintongormley commented Feb 6, 2017

Uh oh!

jdconrad commented Feb 6, 2017

Uh oh!

Mpdreamz commented Mar 22, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jdconrad commented Dec 15, 2016 •

edited

Loading