Replace Ingest ScriptContext with Custom Interface #32003
Conversation
original-brownbear
commented
Jul 12, 2018
•
original-brownbear
commented
- Resolving part of this todo: https://github.com/elastic/elasticsearch/pull/32003/files#diff-7baf7c80f050de6daa1d08ed27675188L50
- Removed mocking from script processor test to actually run the new code in the mock script service
- Adjusted IT to new API
original-brownbear
added
the
:Data Management/Ingest Node
|
Pinging @elastic/es-core-infra |
| @@ -58,7 +59,7 @@ public void testScripting() throws Exception { | |||
| doAnswer(invocationOnMock -> { | |||
| ingestDocument.setFieldValue("bytes_total", randomBytesTotal); | |||
There was a problem hiding this comment.
one thing we do not check here is that the Map passed in as the ctx is actually
IngestDocument#getSourceAndMetadata()
Do you think that would be useful here?
We do have checks in rest-api-spec/test/ingest/50_script_processor_using_painless.yml that we are actually modifying the document, so that may be sufficient. Seems like it would be nice to validate this from unit tests too
There was a problem hiding this comment.
@talevy makes sense + is trivial to do => sec adjusting :)
|
@talevy seems ITs are broken, gotta give this another look. |
|
@talevy should be all good now, just had to adjust the |
|
|
||
| import java.util.Map; | ||
|
|
||
| public abstract class IngestScript { |
There was a problem hiding this comment.
maybe add class level java docs here?
There was a problem hiding this comment.
Sounds good, added in 762d2f7 :)
| return params; | ||
| } | ||
|
|
||
| public abstract void execute(Map<String, Object> ctx); |
There was a problem hiding this comment.
While this will be needed for backcompat in 6.x, I think we want to expose IngestDocument as a variable directly here. But this can be done as a followup (along with deprecation control of the old ctx variable).
|
@rjernst thanks for the review! |
* master: Replace Ingest ScriptContext with Custom Interface (#32003) Mute failing tests
* Replace Ingest ScriptContext with Custom Interface * Make org.elasticsearch.ingest.common.ScriptProcessorTests#testScripting more precise * Don't mock script factory in ScriptProcessorTests * Adjust mock script plugin in IT for new API
* Replace Ingest ScriptContext with Custom Interface * Make org.elasticsearch.ingest.common.ScriptProcessorTests#testScripting more precise * Don't mock script factory in ScriptProcessorTests * Adjust mock script plugin in IT for new API
* es/6.x: Use correct formatting for links (#29460) Revert "Adds a new auto-interval date histogram (#28993)" Revert "fix typo" fix typo Adds a new auto-interval date histogram (#28993) [Rollup] Replace RollupIT with a ESRestTestCase version (#31977) [Rollup] Fix duplicate field names in test (#32075) [Tests] Fix failure due to changes exception message (#32036) [Test] Mute MlJobIT#testDeleteJobAfterMissingAliases Replace Ingest ScriptContext with Custom Interface (#32003) (#32060) Cleanup Duplication in `PainlessScriptEngine` (#31991) (#32061) HLRC: Add xpack usage api (#31975) Clean Up Snapshot Create Rest API (#31779)
