Skip to content
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.

Sign up for GitHub

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

Scripting: Remove deprecated params.ctx #36848

Merged
merged 11 commits into from
Dec 21, 2018
Merged

Scripting: Remove deprecated params.ctx #36848

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
35ed56d
Watcher: Remove deprecated params.ctx
hub-cap Dec 19, 2018
f5ff9e2
Merge remote-tracking branch 'upstream/master' into watcher_remove_pa…
hub-cap Dec 20, 2018
58e223d
Remove a redundant semicolon
hub-cap Dec 20, 2018
0671a66
Also remove update scripts params.ctx
hub-cap Dec 20, 2018
9b0c5c1
precommit
hub-cap Dec 20, 2018
c96f5a4
Merge remote-tracking branch 'upstream/master' into watcher_remove_pa…
hub-cap Dec 20, 2018
f07518d
Adding back PARAMETERS, needed by painless
hub-cap Dec 20, 2018
4d28432
thanks intellij
hub-cap Dec 20, 2018
bce6fc4
Merge remote-tracking branch 'upstream/master' into watcher_remove_pa…
hub-cap Dec 20, 2018
4d37548
Remove tests that are not relevant anymore
hub-cap Dec 20, 2018
f8e629a
doh, precommit
hub-cap Dec 21, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 1 addition & 16 deletions server/src/main/java/org/elasticsearch/script/UpdateScript.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,6 @@

package org.elasticsearch.script;

import java.util.Collections;
import java.util.HashMap;
import java.util.Map;

/**
Expand All @@ -31,17 +29,6 @@ public abstract class UpdateScript {

public static final String[] PARAMETERS = { };

private static final Map<String, String> DEPRECATIONS;
static {
Map<String, String> deprecations = new HashMap<>();
deprecations.put(
"ctx",
"Accessing variable [ctx] via [params.ctx] from within a update script " +
"is deprecated in favor of directly accessing [ctx]."
);
DEPRECATIONS = Collections.unmodifiableMap(deprecations);
}

/** The context used to compile {@link UpdateScript} factories. */
public static final ScriptContext<Factory> CONTEXT = new ScriptContext<>("update", Factory.class);

Expand All @@ -52,9 +39,7 @@ public abstract class UpdateScript {
private final Map<String, Object> ctx;

public UpdateScript(Map<String, Object> params, Map<String, Object> ctx) {
Map<String, Object> paramsWithCtx = new HashMap<>(params);
paramsWithCtx.put("ctx", ctx);
this.params = new ParameterMap(paramsWithCtx, DEPRECATIONS);
this.params = params;
this.ctx = ctx;
}

Expand Down
23 changes: 3 additions & 20 deletions ...tcher/src/main/java/org/elasticsearch/xpack/watcher/condition/WatcherConditionScript.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,43 +5,26 @@
*/
package org.elasticsearch.xpack.watcher.condition;

import org.elasticsearch.script.ParameterMap;
import org.elasticsearch.script.ScriptContext;
import org.elasticsearch.xpack.core.watcher.execution.WatchExecutionContext;
import org.elasticsearch.xpack.watcher.support.Variables;

import java.util.Collections;
import java.util.HashMap;
import java.util.Map;

/**
* A script to determine whether a watch should be run.
*/
public abstract class WatcherConditionScript {
public static final String[] PARAMETERS = {};

private static final Map<String, String> DEPRECATIONS;

static {
Map<String, String> deprecations = new HashMap<>();
deprecations.put(
"ctx",
"Accessing variable [ctx] via [params.ctx] from within a watcher_condition script " +
"is deprecated in favor of directly accessing [ctx]."
);
DEPRECATIONS = Collections.unmodifiableMap(deprecations);
}
public static final String[] PARAMETERS = {};

private final Map<String, Object> params;
// TODO: ctx should have its members extracted into execute parameters, but it needs to be a member for bwc access in params
private final Map<String, Object> ctx;

public WatcherConditionScript(Map<String, Object> params, WatchExecutionContext watcherContext) {
Map<String, Object> paramsWithCtx = new HashMap<>(params);
Map<String, Object> ctx = Variables.createCtx(watcherContext, watcherContext.payload());
paramsWithCtx.put("ctx", ctx);
this.params = new ParameterMap(Collections.unmodifiableMap(paramsWithCtx), DEPRECATIONS);
this.ctx = ctx;
this.params = params;
this.ctx = Variables.createCtx(watcherContext, watcherContext.payload());
}

public abstract boolean execute();
Expand Down
23 changes: 3 additions & 20 deletions ...rc/main/java/org/elasticsearch/xpack/watcher/transform/script/WatcherTransformScript.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,44 +5,27 @@
*/
package org.elasticsearch.xpack.watcher.transform.script;

import org.elasticsearch.script.ParameterMap;
import org.elasticsearch.script.ScriptContext;
import org.elasticsearch.xpack.core.watcher.execution.WatchExecutionContext;
import org.elasticsearch.xpack.core.watcher.watch.Payload;
import org.elasticsearch.xpack.watcher.support.Variables;

import java.util.Collections;
import java.util.HashMap;
import java.util.Map;

/**
* A script to transform the results of a watch execution.
*/
public abstract class WatcherTransformScript {
public static final String[] PARAMETERS = {};

private static final Map<String, String> DEPRECATIONS;

static {
Map<String, String> deprecations = new HashMap<>();
deprecations.put(
"ctx",
"Accessing variable [ctx] via [params.ctx] from within a watcher_transform script " +
"is deprecated in favor of directly accessing [ctx]."
);
DEPRECATIONS = Collections.unmodifiableMap(deprecations);
}
public static final String[] PARAMETERS = {};

private final Map<String, Object> params;
// TODO: ctx should have its members extracted into execute parameters, but it needs to be a member bwc access in params
private final Map<String, Object> ctx;

public WatcherTransformScript(Map<String, Object> params, WatchExecutionContext watcherContext, Payload payload) {
Map<String, Object> paramsWithCtx = new HashMap<>(params);
Map<String, Object> ctx = Variables.createCtx(watcherContext, payload);
paramsWithCtx.put("ctx", ctx);
this.params = new ParameterMap(Collections.unmodifiableMap(paramsWithCtx), DEPRECATIONS);
this.ctx = ctx;
this.params = params;
this.ctx = Variables.createCtx(watcherContext, payload);
}

public abstract Object execute();
Expand Down
8 changes: 3 additions & 5 deletions ...watcher/src/test/java/org/elasticsearch/xpack/watcher/condition/ScriptConditionTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,7 @@ public void testScriptConditionAccessCtx() throws Exception {
assertThat(condition.execute(ctx).met(), is(true));
}

public void testParamsCtxDeprecated() throws Exception {
public void testParamsCtxNull() throws Exception {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this test (and the one below) still make sense ?

The test is based on the absence of something that used to be there. I would be +1 to simply remove the deprecation tests (instead of changing them to null tests)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think its fair to remove these 2 tests (this and the other one mentioned i forgot to rename).

WatchExecutionContext watcherContext = mock(WatchExecutionContext.class);
when(watcherContext.id()).thenReturn(mock(Wid.class));
when(watcherContext.watch()).thenReturn(mock(Watch.class));
Expand All @@ -216,13 +216,11 @@ public void testParamsCtxDeprecated() throws Exception {
WatcherConditionScript watcherScript = new WatcherConditionScript(Collections.emptyMap(), watcherContext) {
@Override
public boolean execute() {
assertThat(getParams().get("ctx"), is(getCtx()));
assertNull(getParams().get("ctx"));
return true;
}
};
watcherScript.execute();
assertWarnings("Accessing variable [ctx] via [params.ctx] from within a watcher_condition script " +
"is deprecated in favor of directly accessing [ctx].");
assertTrue(watcherScript.execute());
}

private static XContentBuilder createConditionContent(String script, String scriptLang, ScriptType scriptType) throws IOException {
Expand Down
4 changes: 1 addition & 3 deletions .../src/test/java/org/elasticsearch/xpack/watcher/transform/script/ScriptTransformTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,9 +202,7 @@ public Object execute() {
return getParams().get("ctx");
}
};
assertThat(watcherScript.execute(), is(watcherScript.getCtx()));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we keep this test we should change the name accordingly. (similar as above)

assertWarnings("Accessing variable [ctx] via [params.ctx] from within a watcher_transform script " +
"is deprecated in favor of directly accessing [ctx].");
assertNull(watcherScript.execute());
}

static String scriptTypeField(ScriptType type) {
Expand Down