Watcher: Remove unused local variable in doExecute #36655
Conversation
|
Pinging @elastic/es-core-features |
| 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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
#36848 is the params.ctx removal, I saw this comment and decided to just nuke it.
* elastic/master: (539 commits) SQL: documentation improvements and updates (elastic#36918) [DOCS] Merges list of discovery and cluster formation settings (elastic#36909) Only compress responses if request was compressed (elastic#36867) Remove duplicate paragraph (elastic#36942) Fix URI to cluster stats endpoint on specific nodes (elastic#36784) Fix typo in unitTest task (elastic#36930) RecoveryMonitor#lastSeenAccessTime should be volatile (elastic#36781) [CCR] Add `ccr.auto_follow_coordinator.wait_for_timeout` setting (elastic#36714) Scripting: Remove deprecated params.ctx (elastic#36848) Refactor the REST actions to clarify what endpoints are deprecated. (elastic#36869) Add JDK 12 to CI rotation (elastic#36915) Improve error message for 6.x style realm settings (elastic#36876) Send clear session as routable remote request (elastic#36805) [DOCS] Remove redundant ILM attributes (elastic#36808) SQL: Fix bug regarding histograms usage in scripting (elastic#36866) Update index mappings when ccr restore complete (elastic#36879) Docs: Bump version to alpha2 after release Enable IPv6 URIs in reindex from remote (elastic#36874) Watcher: Remove unused local variable in doExecute (elastic#36655) [DOCS] Synchs titles of X-Pack APIs ...
No description provided.