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

Watcher: Remove unused local variable in doExecute #36655

Merged
merged 5 commits into from
Dec 20, 2018
Merged

Watcher: Remove unused local variable in doExecute #36655

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1e8d264
Watcher: Remove unused local variable in doExecute
hub-cap Dec 14, 2018
9bbeb6d
Merge remote-tracking branch 'upstream/master' into watcher_remove_un…
hub-cap Dec 18, 2018
3a6ca71
Merge remote-tracking branch 'upstream/master' into watcher_remove_un…
hub-cap Dec 19, 2018
36cf2ee
Merge remote-tracking branch 'upstream/master' into watcher_remove_un…
hub-cap Dec 20, 2018
7a393a1
whoops, precommit!
hub-cap Dec 20, 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
4 changes: 0 additions & 4 deletions ...ugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/condition/ScriptCondition.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,10 +58,6 @@ public Result execute(WatchExecutionContext ctx) {
}

public Result doExecute(WatchExecutionContext ctx) {
Map<String, Object> parameters = Variables.createCtxParamsMap(ctx, ctx.payload());
if (script.getParams() != null && !script.getParams().isEmpty()) {
parameters.putAll(script.getParams());
}
WatcherConditionScript conditionScript = scriptFactory.newInstance(script.getParams(), ctx);
Copy link
Contributor

Choose a reason for hiding this comment

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

Without much context to this code, it appears that parameters was likely meant to be sent as the first parameter to newInstance. The code here checks script.getParams() for null, but it does not check it downstream and could result in NPE if script.params() is ever null.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you know what, i think thats right. It looks like AbstractCompareCondition does the same thing, passing in the createCtxParamsMap(). Whats interesting is that when this is actually executed, the WatcherConditionScript also has all of the values of the context in it. Ive found in a few spots where these contexts fail if you try to access any params that dont exist, due to it being null in a painless script. I think there might be some more code exploration I need to do before I feel safe with this. One other data point is that the ExecutableScriptTransform, the other place that painless contexts are created is also firing off a factory.newInstance, passing in just script.getParams() directly. I think this could use some overall cleanup as its somewhat convoluted.

Copy link
Contributor

Choose a reason for hiding this comment

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

We discussed this issue in person and came to conclusion that this dead code is indeed dead code, and wouldn't make a difference if we started to use the parameters. Also we should fully remove the deprecated params.ctx support for 7.x (issue not logged yet).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#36848 is the params.ctx removal, I saw this comment and decided to just nuke it.

return conditionScript.execute() ? MET : UNMET;
}
Expand Down