From 9c8e69c71efd7fd4a0ae75fe8347ccc1c5dd40d4 Mon Sep 17 00:00:00 2001 From: albendz Date: Tue, 12 Jun 2018 19:49:13 -0700 Subject: [PATCH 1/8] Make text message not required in constructor for slack --- .../slack/message/SlackMessage.java | 18 +++++++++++++++--- .../slack/message/SlackMessageTests.java | 10 +++++++++- 2 files changed, 24 insertions(+), 4 deletions(-) diff --git a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/slack/message/SlackMessage.java b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/slack/message/SlackMessage.java index 9f7cd36c9106a..fd5bcc9e8b9ef 100644 --- a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/slack/message/SlackMessage.java +++ b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/slack/message/SlackMessage.java @@ -26,8 +26,16 @@ public class SlackMessage implements MessageElement { final String from; final String[] to; final String icon; - final String text; - final Attachment[] attachments; + String text; + Attachment[] attachments; + + public SlackMessage(String from, String[] to, String icon) { + this(from, to, icon, null, null); // Empty text and empty attachments + } + + public SlackMessage(String from, String[] to, String icon, String text) { + this(from, to, icon, text, null); // Empty attachments + } public SlackMessage(String from, String[] to, String icon, String text, Attachment[] attachments) { this.from = from; @@ -57,6 +65,10 @@ public Attachment[] getAttachments() { return attachments; } + public void setText(String text) { this.text = text;} + + public void setAttachments(Attachment[] attachments) { this.attachments = attachments;} + @Override public boolean equals(Object o) { if (this == o) return true; @@ -202,7 +214,7 @@ public SlackMessage render(String watchId, String actionId, TextTemplateEngine e attachments.addAll(dynamicAttachments.render(engine, model, defaults.attachment)); } if (attachments == null) { - return new SlackMessage(from, to, icon, text, null); + return new SlackMessage(from, to, icon, text); } return new SlackMessage(from, to, icon, text, attachments.toArray(new Attachment[attachments.size()])); } diff --git a/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/notification/slack/message/SlackMessageTests.java b/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/notification/slack/message/SlackMessageTests.java index 4075bd3dadde4..0a0de3fdd479d 100644 --- a/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/notification/slack/message/SlackMessageTests.java +++ b/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/notification/slack/message/SlackMessageTests.java @@ -575,7 +575,7 @@ public void testUrlPathIsFiltered() throws Exception { HttpResponse response = new HttpResponse(500); String path = randomAlphaOfLength(20); HttpRequest request = HttpRequest.builder("localhost", 1234).path(path).build(); - SlackMessage slackMessage = new SlackMessage("from", new String[] {"to"}, "icon", "text", null); + SlackMessage slackMessage = new SlackMessage("from", new String[] {"to"}, "icon", "text"); SentMessages sentMessages = new SentMessages("foo", Arrays.asList(SentMessages.SentMessage.responded("recipient", slackMessage, request, response))); @@ -602,6 +602,14 @@ public void testUrlPathIsFiltered() throws Exception { } } + // Create template from null message or null attachment ok + // Can have message as null + + public void canHaveNullTextAndAttachment() throws Exception { + SlackMessage slackMessage = new SlackMessage("from", new String[] {"to"}, "icon"); + assertThat(slackMessage.getText(), is(null)); + assertThat(slackMessage.getAttachments(), is(null)); + } private static void writeFieldIfNotNull(XContentBuilder builder, String field, Object value) throws IOException { if (value != null) { builder.field(field, value); From b6876b4e92fa0451362ede9d122fe59e657900d9 Mon Sep 17 00:00:00 2001 From: albendz Date: Tue, 12 Jun 2018 19:54:09 -0700 Subject: [PATCH 2/8] Remove unnecessary comments in test file --- .../watcher/notification/slack/message/SlackMessageTests.java | 3 --- 1 file changed, 3 deletions(-) diff --git a/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/notification/slack/message/SlackMessageTests.java b/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/notification/slack/message/SlackMessageTests.java index 0a0de3fdd479d..9f546c55d8db1 100644 --- a/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/notification/slack/message/SlackMessageTests.java +++ b/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/notification/slack/message/SlackMessageTests.java @@ -602,9 +602,6 @@ public void testUrlPathIsFiltered() throws Exception { } } - // Create template from null message or null attachment ok - // Can have message as null - public void canHaveNullTextAndAttachment() throws Exception { SlackMessage slackMessage = new SlackMessage("from", new String[] {"to"}, "icon"); assertThat(slackMessage.getText(), is(null)); From 63bf29aca1c7bd58b9453debddae2391428544fc Mon Sep 17 00:00:00 2001 From: albendz Date: Wed, 5 Sep 2018 16:17:01 -0700 Subject: [PATCH 3/8] Throw exception when reduce or combine is not provided; update tests --- .../ScriptedMetricAggregationBuilder.java | 7 +- .../ScriptedMetricAggregatorTests.java | 70 +++++++++++++------ 2 files changed, 54 insertions(+), 23 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/scripted/ScriptedMetricAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/scripted/ScriptedMetricAggregationBuilder.java index 8b6d834184d73..b379335922341 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/scripted/ScriptedMetricAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/scripted/ScriptedMetricAggregationBuilder.java @@ -222,8 +222,11 @@ protected ScriptedMetricAggregatorFactory doBuild(SearchContext context, Aggrega ScriptedMetricAggContexts.CombineScript.CONTEXT); combineScriptParams = combineScript.getParams(); } else { - compiledCombineScript = (p, a) -> null; - combineScriptParams = Collections.emptyMap(); + throw new IllegalArgumentException("[combineScript] must not be null: [" + name + "]"); + } + + if(reduceScript == null) { + throw new IllegalArgumentException("[reduceScript] must not be null: [" + name + "]"); } return new ScriptedMetricAggregatorFactory(name, compiledMapScript, mapScriptParams, compiledInitScript, initScriptParams, compiledCombineScript, combineScriptParams, reduceScript, diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/scripted/ScriptedMetricAggregatorTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/scripted/ScriptedMetricAggregatorTests.java index 65e42556461a5..daf35ccee28d9 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/scripted/ScriptedMetricAggregatorTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/scripted/ScriptedMetricAggregatorTests.java @@ -54,6 +54,8 @@ public class ScriptedMetricAggregatorTests extends AggregatorTestCase { private static final Script MAP_SCRIPT = new Script(ScriptType.INLINE, MockScriptEngine.NAME, "mapScript", Collections.emptyMap()); private static final Script COMBINE_SCRIPT = new Script(ScriptType.INLINE, MockScriptEngine.NAME, "combineScript", Collections.emptyMap()); + private static final Script REDUCE_SCRIPT = new Script(ScriptType.INLINE, MockScriptEngine.NAME, "reduceScript", + Collections.emptyMap()); private static final Script INIT_SCRIPT_SCORE = new Script(ScriptType.INLINE, MockScriptEngine.NAME, "initScriptScore", Collections.emptyMap()); @@ -61,6 +63,8 @@ public class ScriptedMetricAggregatorTests extends AggregatorTestCase { Collections.emptyMap()); private static final Script COMBINE_SCRIPT_SCORE = new Script(ScriptType.INLINE, MockScriptEngine.NAME, "combineScriptScore", Collections.emptyMap()); + private static final Script COMBINE_SCRIPT_NOOP = new Script(ScriptType.INLINE, MockScriptEngine.NAME, "combineScriptNoop", + Collections.emptyMap()); private static final Script INIT_SCRIPT_PARAMS = new Script(ScriptType.INLINE, MockScriptEngine.NAME, "initScriptParams", Collections.singletonMap("initialValue", 24)); @@ -96,6 +100,14 @@ public static void initMockScripts() { Map state = (Map) params.get("state"); return ((List) state.get("collector")).stream().mapToInt(Integer::intValue).sum(); }); + SCRIPTS.put("combineScriptNoop", params -> { + Map state = (Map) params.get("state"); + return state; + }); + SCRIPTS.put("reduceScript", params -> { + Map state = (Map) params.get("state"); + return state; + }); SCRIPTS.put("initScriptScore", params -> { Map state = (Map) params.get("state"); @@ -160,7 +172,7 @@ public void testNoDocs() throws IOException { } try (IndexReader indexReader = DirectoryReader.open(directory)) { ScriptedMetricAggregationBuilder aggregationBuilder = new ScriptedMetricAggregationBuilder(AGG_NAME); - aggregationBuilder.mapScript(MAP_SCRIPT); // map script is mandatory, even if its not used in this case + aggregationBuilder.mapScript(MAP_SCRIPT).combineScript(COMBINE_SCRIPT_NOOP).reduceScript(REDUCE_SCRIPT); ScriptedMetric scriptedMetric = search(newSearcher(indexReader, true, true), new MatchAllDocsQuery(), aggregationBuilder); assertEquals(AGG_NAME, scriptedMetric.getName()); assertNotNull(scriptedMetric.aggregation()); @@ -169,9 +181,6 @@ public void testNoDocs() throws IOException { } } - /** - * without combine script, the "states" map should contain a list of the size of the number of documents matched - */ public void testScriptedMetricWithoutCombine() throws IOException { try (Directory directory = newDirectory()) { int numDocs = randomInt(100); @@ -182,15 +191,28 @@ public void testScriptedMetricWithoutCombine() throws IOException { } try (IndexReader indexReader = DirectoryReader.open(directory)) { ScriptedMetricAggregationBuilder aggregationBuilder = new ScriptedMetricAggregationBuilder(AGG_NAME); - aggregationBuilder.initScript(INIT_SCRIPT).mapScript(MAP_SCRIPT); - ScriptedMetric scriptedMetric = search(newSearcher(indexReader, true, true), new MatchAllDocsQuery(), aggregationBuilder); - assertEquals(AGG_NAME, scriptedMetric.getName()); - assertNotNull(scriptedMetric.aggregation()); - @SuppressWarnings("unchecked") - Map agg = (Map) scriptedMetric.aggregation(); - @SuppressWarnings("unchecked") - List list = (List) agg.get("collector"); - assertEquals(numDocs, list.size()); + aggregationBuilder.initScript(INIT_SCRIPT).mapScript(MAP_SCRIPT).reduceScript(REDUCE_SCRIPT); + IllegalArgumentException exception = expectThrows(IllegalArgumentException.class, + () -> search(newSearcher(indexReader, true, true), new MatchAllDocsQuery(), aggregationBuilder)); + assertEquals(exception.getMessage(), "[combineScript] must not be null: [scriptedMetric]"); + } + } + } + + public void testScriptedMetricWithoutReduce() throws IOException { + try (Directory directory = newDirectory()) { + int numDocs = randomInt(100); + try (RandomIndexWriter indexWriter = new RandomIndexWriter(random(), directory)) { + for (int i = 0; i < numDocs; i++) { + indexWriter.addDocument(singleton(new SortedNumericDocValuesField("number", i))); + } + } + try (IndexReader indexReader = DirectoryReader.open(directory)) { + ScriptedMetricAggregationBuilder aggregationBuilder = new ScriptedMetricAggregationBuilder(AGG_NAME); + aggregationBuilder.initScript(INIT_SCRIPT).mapScript(MAP_SCRIPT).combineScript(COMBINE_SCRIPT); + IllegalArgumentException exception = expectThrows(IllegalArgumentException.class, + () -> search(newSearcher(indexReader, true, true), new MatchAllDocsQuery(), aggregationBuilder)); + assertEquals(exception.getMessage(), "[reduceScript] must not be null: [scriptedMetric]"); } } } @@ -208,7 +230,8 @@ public void testScriptedMetricWithCombine() throws IOException { } try (IndexReader indexReader = DirectoryReader.open(directory)) { ScriptedMetricAggregationBuilder aggregationBuilder = new ScriptedMetricAggregationBuilder(AGG_NAME); - aggregationBuilder.initScript(INIT_SCRIPT).mapScript(MAP_SCRIPT).combineScript(COMBINE_SCRIPT); + aggregationBuilder.initScript(INIT_SCRIPT).mapScript(MAP_SCRIPT) + .combineScript(COMBINE_SCRIPT).reduceScript(REDUCE_SCRIPT); ScriptedMetric scriptedMetric = search(newSearcher(indexReader, true, true), new MatchAllDocsQuery(), aggregationBuilder); assertEquals(AGG_NAME, scriptedMetric.getName()); assertNotNull(scriptedMetric.aggregation()); @@ -230,7 +253,8 @@ public void testScriptedMetricWithCombineAccessesScores() throws IOException { } try (IndexReader indexReader = DirectoryReader.open(directory)) { ScriptedMetricAggregationBuilder aggregationBuilder = new ScriptedMetricAggregationBuilder(AGG_NAME); - aggregationBuilder.initScript(INIT_SCRIPT_SCORE).mapScript(MAP_SCRIPT_SCORE).combineScript(COMBINE_SCRIPT_SCORE); + aggregationBuilder.initScript(INIT_SCRIPT_SCORE).mapScript(MAP_SCRIPT_SCORE) + .combineScript(COMBINE_SCRIPT_SCORE).reduceScript(REDUCE_SCRIPT); ScriptedMetric scriptedMetric = search(newSearcher(indexReader, true, true), new MatchAllDocsQuery(), aggregationBuilder); assertEquals(AGG_NAME, scriptedMetric.getName()); assertNotNull(scriptedMetric.aggregation()); @@ -250,7 +274,8 @@ public void testScriptParamsPassedThrough() throws IOException { try (IndexReader indexReader = DirectoryReader.open(directory)) { ScriptedMetricAggregationBuilder aggregationBuilder = new ScriptedMetricAggregationBuilder(AGG_NAME); - aggregationBuilder.initScript(INIT_SCRIPT_PARAMS).mapScript(MAP_SCRIPT_PARAMS).combineScript(COMBINE_SCRIPT_PARAMS); + aggregationBuilder.initScript(INIT_SCRIPT_PARAMS).mapScript(MAP_SCRIPT_PARAMS) + .combineScript(COMBINE_SCRIPT_PARAMS).reduceScript(REDUCE_SCRIPT); ScriptedMetric scriptedMetric = search(newSearcher(indexReader, true, true), new MatchAllDocsQuery(), aggregationBuilder); // The result value depends on the script params. @@ -270,8 +295,8 @@ public void testConflictingAggAndScriptParams() throws IOException { try (IndexReader indexReader = DirectoryReader.open(directory)) { ScriptedMetricAggregationBuilder aggregationBuilder = new ScriptedMetricAggregationBuilder(AGG_NAME); Map aggParams = Collections.singletonMap(CONFLICTING_PARAM_NAME, "blah"); - aggregationBuilder.params(aggParams).initScript(INIT_SCRIPT_PARAMS).mapScript(MAP_SCRIPT_PARAMS). - combineScript(COMBINE_SCRIPT_PARAMS); + aggregationBuilder.params(aggParams).initScript(INIT_SCRIPT_PARAMS).mapScript(MAP_SCRIPT_PARAMS) + .combineScript(COMBINE_SCRIPT_PARAMS).reduceScript(REDUCE_SCRIPT); IllegalArgumentException ex = expectThrows(IllegalArgumentException.class, () -> search(newSearcher(indexReader, true, true), new MatchAllDocsQuery(), aggregationBuilder) @@ -289,7 +314,8 @@ public void testSelfReferencingAggStateAfterInit() throws IOException { } try (IndexReader indexReader = DirectoryReader.open(directory)) { ScriptedMetricAggregationBuilder aggregationBuilder = new ScriptedMetricAggregationBuilder(AGG_NAME); - aggregationBuilder.initScript(INIT_SCRIPT_SELF_REF).mapScript(MAP_SCRIPT); + aggregationBuilder.initScript(INIT_SCRIPT_SELF_REF).mapScript(MAP_SCRIPT) + .combineScript(COMBINE_SCRIPT_PARAMS).reduceScript(REDUCE_SCRIPT); IllegalArgumentException ex = expectThrows(IllegalArgumentException.class, () -> search(newSearcher(indexReader, true, true), new MatchAllDocsQuery(), aggregationBuilder) @@ -309,7 +335,8 @@ public void testSelfReferencingAggStateAfterMap() throws IOException { } try (IndexReader indexReader = DirectoryReader.open(directory)) { ScriptedMetricAggregationBuilder aggregationBuilder = new ScriptedMetricAggregationBuilder(AGG_NAME); - aggregationBuilder.initScript(INIT_SCRIPT).mapScript(MAP_SCRIPT_SELF_REF); + aggregationBuilder.initScript(INIT_SCRIPT).mapScript(MAP_SCRIPT_SELF_REF) + .combineScript(COMBINE_SCRIPT_PARAMS).reduceScript(REDUCE_SCRIPT); IllegalArgumentException ex = expectThrows(IllegalArgumentException.class, () -> search(newSearcher(indexReader, true, true), new MatchAllDocsQuery(), aggregationBuilder) @@ -326,7 +353,8 @@ public void testSelfReferencingAggStateAfterCombine() throws IOException { } try (IndexReader indexReader = DirectoryReader.open(directory)) { ScriptedMetricAggregationBuilder aggregationBuilder = new ScriptedMetricAggregationBuilder(AGG_NAME); - aggregationBuilder.initScript(INIT_SCRIPT).mapScript(MAP_SCRIPT).combineScript(COMBINE_SCRIPT_SELF_REF); + aggregationBuilder.initScript(INIT_SCRIPT).mapScript(MAP_SCRIPT) + .combineScript(COMBINE_SCRIPT_SELF_REF).reduceScript(REDUCE_SCRIPT); IllegalArgumentException ex = expectThrows(IllegalArgumentException.class, () -> search(newSearcher(indexReader, true, true), new MatchAllDocsQuery(), aggregationBuilder) From f68326e70cab4b9a1fdc20dea2ee2651ca5b9e5a Mon Sep 17 00:00:00 2001 From: albendz Date: Wed, 5 Sep 2018 20:43:23 -0700 Subject: [PATCH 4/8] Update integration tests for scripted metrics to always include reduce and combine --- .../metrics/ScriptedMetricIT.java | 38 ++++++++++++++++--- 1 file changed, 33 insertions(+), 5 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/ScriptedMetricIT.java b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/ScriptedMetricIT.java index f62598fa7c317..82368a163c1f6 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/ScriptedMetricIT.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/ScriptedMetricIT.java @@ -329,10 +329,14 @@ protected Path nodeConfigPath(int nodeOrdinal) { public void testMap() { Script mapScript = new Script(ScriptType.INLINE, CustomScriptPlugin.NAME, "state['count'] = 1", Collections.emptyMap()); + Script combineScript = + new Script(ScriptType.INLINE, CustomScriptPlugin.NAME, "sum state values as a new aggregation", Collections.emptyMap()); + Script reduceScript = new Script(ScriptType.INLINE, CustomScriptPlugin.NAME, + "sum all states (lists) values as a new aggregation", Collections.emptyMap()); SearchResponse response = client().prepareSearch("idx") .setQuery(matchAllQuery()) - .addAggregation(scriptedMetric("scripted").mapScript(mapScript)) + .addAggregation(scriptedMetric("scripted").mapScript(mapScript).combineScript(combineScript).reduceScript(reduceScript)) .get(); assertSearchResponse(response); assertThat(response.getHits().getTotalHits(), equalTo(numDocs)); @@ -370,10 +374,18 @@ public void testMapWithParams() { Map aggregationParams = Collections.singletonMap("param2", 1); Script mapScript = new Script(ScriptType.INLINE, CustomScriptPlugin.NAME, "state[param1] = param2", scriptParams); + Script combineScript = + new Script(ScriptType.INLINE, CustomScriptPlugin.NAME, "sum state values as a new aggregation", Collections.emptyMap()); + Script reduceScript = new Script(ScriptType.INLINE, CustomScriptPlugin.NAME, + "sum all states (lists) values as a new aggregation", Collections.emptyMap()); SearchResponse response = client().prepareSearch("idx") .setQuery(matchAllQuery()) - .addAggregation(scriptedMetric("scripted").params(aggregationParams).mapScript(mapScript)) + .addAggregation(scriptedMetric("scripted") + .params(aggregationParams) + .mapScript(mapScript) + .combineScript(combineScript) + .reduceScript(reduceScript)) .get(); assertSearchResponse(response); assertThat(response.getHits().getTotalHits(), equalTo(numDocs)); @@ -424,7 +436,11 @@ public void testInitMapWithParams() { .initScript( new Script(ScriptType.INLINE, CustomScriptPlugin.NAME, "vars.multiplier = 3", Collections.emptyMap())) .mapScript(new Script(ScriptType.INLINE, CustomScriptPlugin.NAME, - "state.list.add(vars.multiplier)", Collections.emptyMap()))) + "state.list.add(vars.multiplier)", Collections.emptyMap())) + .combineScript(new Script(ScriptType.INLINE, CustomScriptPlugin.NAME, + "sum state values as a new aggregation", Collections.emptyMap())) + .reduceScript(new Script(ScriptType.INLINE, CustomScriptPlugin.NAME, + "sum all states (lists) values as a new aggregation", Collections.emptyMap()))) .get(); assertSearchResponse(response); assertThat(response.getHits().getTotalHits(), equalTo(numDocs)); @@ -467,6 +483,8 @@ public void testMapCombineWithParams() { Script mapScript = new Script(ScriptType.INLINE, CustomScriptPlugin.NAME, "state.list.add(1)", Collections.emptyMap()); Script combineScript = new Script(ScriptType.INLINE, CustomScriptPlugin.NAME, "sum state values as a new aggregation", Collections.emptyMap()); + Script reduceScript = new Script(ScriptType.INLINE, CustomScriptPlugin.NAME, + "sum all states (lists) values as a new aggregation", Collections.emptyMap()); SearchResponse response = client() .prepareSearch("idx") @@ -475,7 +493,8 @@ public void testMapCombineWithParams() { scriptedMetric("scripted") .params(params) .mapScript(mapScript) - .combineScript(combineScript)) + .combineScript(combineScript) + .reduceScript(reduceScript)) .execute().actionGet(); assertSearchResponse(response); assertThat(response.getHits().getTotalHits(), equalTo(numDocs)); @@ -520,6 +539,8 @@ public void testInitMapCombineWithParams() { Collections.emptyMap()); Script combineScript = new Script(ScriptType.INLINE, CustomScriptPlugin.NAME, "sum state values as a new aggregation", Collections.emptyMap()); + Script reduceScript = new Script(ScriptType.INLINE, CustomScriptPlugin.NAME, + "sum all states (lists) values as a new aggregation", Collections.emptyMap()); SearchResponse response = client() .prepareSearch("idx") @@ -529,7 +550,8 @@ public void testInitMapCombineWithParams() { .params(params) .initScript(initScript) .mapScript(mapScript) - .combineScript(combineScript)) + .combineScript(combineScript) + .reduceScript(reduceScript)) .get(); assertSearchResponse(response); assertThat(response.getHits().getTotalHits(), equalTo(numDocs)); @@ -714,6 +736,8 @@ public void testInitMapReduceWithParams() { Script initScript = new Script(ScriptType.INLINE, CustomScriptPlugin.NAME, "vars.multiplier = 3", Collections.emptyMap()); Script mapScript = new Script(ScriptType.INLINE, CustomScriptPlugin.NAME, "state.list.add(vars.multiplier)", Collections.emptyMap()); + Script combineScript = + new Script(ScriptType.INLINE, CustomScriptPlugin.NAME, "sum state values as a new aggregation", Collections.emptyMap()); Script reduceScript = new Script(ScriptType.INLINE, CustomScriptPlugin.NAME, "sum all states' state.list values as a new aggregation", Collections.emptyMap()); @@ -725,6 +749,7 @@ public void testInitMapReduceWithParams() { .params(params) .initScript(initScript) .mapScript(mapScript) + .combineScript(combineScript) .reduceScript(reduceScript)) .get(); assertSearchResponse(response); @@ -753,6 +778,8 @@ public void testMapReduceWithParams() { Script mapScript = new Script(ScriptType.INLINE, CustomScriptPlugin.NAME, "state.list.add(vars.multiplier)", Collections.emptyMap()); + Script combineScript = + new Script(ScriptType.INLINE, CustomScriptPlugin.NAME, "sum state values as a new aggregation", Collections.emptyMap()); Script reduceScript = new Script(ScriptType.INLINE, CustomScriptPlugin.NAME, "sum all states' state.list values as a new aggregation", Collections.emptyMap()); @@ -763,6 +790,7 @@ public void testMapReduceWithParams() { scriptedMetric("scripted") .params(params) .mapScript(mapScript) + .combineScript(combineScript) .reduceScript(reduceScript)) .get(); assertSearchResponse(response); From 3da8fd598dfb70e25a2059d38b673fdd6af9d47a Mon Sep 17 00:00:00 2001 From: albendz Date: Wed, 5 Sep 2018 20:53:46 -0700 Subject: [PATCH 5/8] Remove some old changes from previous branches --- .../slack/message/SlackMessage.java | 18 +++--------------- .../slack/message/SlackMessageTests.java | 2 +- 2 files changed, 4 insertions(+), 16 deletions(-) diff --git a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/slack/message/SlackMessage.java b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/slack/message/SlackMessage.java index 24362f7df4a37..ffff28ce862bd 100644 --- a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/slack/message/SlackMessage.java +++ b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/slack/message/SlackMessage.java @@ -27,16 +27,8 @@ public class SlackMessage implements MessageElement { final String from; final String[] to; final String icon; - String text; - Attachment[] attachments; - - public SlackMessage(String from, String[] to, String icon) { - this(from, to, icon, null, null); // Empty text and empty attachments - } - - public SlackMessage(String from, String[] to, String icon, String text) { - this(from, to, icon, text, null); // Empty attachments - } + final String text; + final Attachment[] attachments; public SlackMessage(String from, String[] to, String icon, @Nullable String text, @Nullable Attachment[] attachments) { if(text == null && attachments == null) { @@ -70,10 +62,6 @@ public Attachment[] getAttachments() { return attachments; } - public void setText(String text) { this.text = text;} - - public void setAttachments(Attachment[] attachments) { this.attachments = attachments;} - @Override public boolean equals(Object o) { if (this == o) return true; @@ -219,7 +207,7 @@ public SlackMessage render(String watchId, String actionId, TextTemplateEngine e attachments.addAll(dynamicAttachments.render(engine, model, defaults.attachment)); } if (attachments == null) { - return new SlackMessage(from, to, icon, text); + return new SlackMessage(from, to, icon, text, null); } return new SlackMessage(from, to, icon, text, attachments.toArray(new Attachment[attachments.size()])); } diff --git a/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/notification/slack/message/SlackMessageTests.java b/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/notification/slack/message/SlackMessageTests.java index 3fe49ca871557..10544e464ace5 100644 --- a/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/notification/slack/message/SlackMessageTests.java +++ b/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/notification/slack/message/SlackMessageTests.java @@ -575,7 +575,7 @@ public void testUrlPathIsFiltered() throws Exception { HttpResponse response = new HttpResponse(500); String path = randomAlphaOfLength(20); HttpRequest request = HttpRequest.builder("localhost", 1234).path(path).build(); - SlackMessage slackMessage = new SlackMessage("from", new String[] {"to"}, "icon", "text"); + SlackMessage slackMessage = new SlackMessage("from", new String[] {"to"}, "icon", "text", null); SentMessages sentMessages = new SentMessages("foo", Arrays.asList(SentMessages.SentMessage.responded("recipient", slackMessage, request, response))); From 667557adffb030652741e054c6743df63da9ca73 Mon Sep 17 00:00:00 2001 From: albendz Date: Tue, 11 Sep 2018 22:10:01 -0700 Subject: [PATCH 6/8] Rearrange script presence checks to be earlier in build --- .../ScriptedMetricAggregationBuilder.java | 27 ++++++++++--------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ScriptedMetricAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ScriptedMetricAggregationBuilder.java index 8923860fc535b..9a7ec3b49f0b1 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ScriptedMetricAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ScriptedMetricAggregationBuilder.java @@ -198,6 +198,20 @@ protected ScriptedMetricAggregatorFactory doBuild(SearchContext context, Aggrega QueryShardContext queryShardContext = context.getQueryShardContext(); + ScriptedMetricAggContexts.CombineScript.Factory compiledCombineScript; + Map combineScriptParams; + if (combineScript != null) { + compiledCombineScript = queryShardContext.getScriptService().compile(combineScript, + ScriptedMetricAggContexts.CombineScript.CONTEXT); + combineScriptParams = combineScript.getParams(); + } else { + throw new IllegalArgumentException("[combineScript] must not be null: [" + name + "]"); + } + + if(reduceScript == null) { + throw new IllegalArgumentException("[reduceScript] must not be null: [" + name + "]"); + } + // Extract params from scripts and pass them along to ScriptedMetricAggregatorFactory, since it won't have // access to them for the scripts it's given precompiled. @@ -215,19 +229,6 @@ protected ScriptedMetricAggregatorFactory doBuild(SearchContext context, Aggrega ScriptedMetricAggContexts.MapScript.CONTEXT); Map mapScriptParams = mapScript.getParams(); - ScriptedMetricAggContexts.CombineScript.Factory compiledCombineScript; - Map combineScriptParams; - if (combineScript != null) { - compiledCombineScript = queryShardContext.getScriptService().compile(combineScript, - ScriptedMetricAggContexts.CombineScript.CONTEXT); - combineScriptParams = combineScript.getParams(); - } else { - throw new IllegalArgumentException("[combineScript] must not be null: [" + name + "]"); - } - - if(reduceScript == null) { - throw new IllegalArgumentException("[reduceScript] must not be null: [" + name + "]"); - } return new ScriptedMetricAggregatorFactory(name, compiledMapScript, mapScriptParams, compiledInitScript, initScriptParams, compiledCombineScript, combineScriptParams, reduceScript, params, queryShardContext.lookup(), context, parent, subfactoriesBuilder, metaData); From 1adb4edb982afeefbb914221c0369f2d3bfc3982 Mon Sep 17 00:00:00 2001 From: albendz Date: Sat, 15 Sep 2018 20:16:50 -0700 Subject: [PATCH 7/8] Change null check order in script builder for aggregated metrics; correct test scripts in IT --- .../ScriptedMetricAggregationBuilder.java | 22 +++++---- .../metrics/ScriptedMetricIT.java | 49 ++++++++++++++----- 2 files changed, 49 insertions(+), 22 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ScriptedMetricAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ScriptedMetricAggregationBuilder.java index 9a7ec3b49f0b1..25fcebc6aa52d 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ScriptedMetricAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ScriptedMetricAggregationBuilder.java @@ -196,22 +196,16 @@ public Map params() { protected ScriptedMetricAggregatorFactory doBuild(SearchContext context, AggregatorFactory parent, Builder subfactoriesBuilder) throws IOException { - QueryShardContext queryShardContext = context.getQueryShardContext(); - - ScriptedMetricAggContexts.CombineScript.Factory compiledCombineScript; - Map combineScriptParams; - if (combineScript != null) { - compiledCombineScript = queryShardContext.getScriptService().compile(combineScript, - ScriptedMetricAggContexts.CombineScript.CONTEXT); - combineScriptParams = combineScript.getParams(); - } else { + if (combineScript == null) { throw new IllegalArgumentException("[combineScript] must not be null: [" + name + "]"); } if(reduceScript == null) { throw new IllegalArgumentException("[reduceScript] must not be null: [" + name + "]"); } - + + QueryShardContext queryShardContext = context.getQueryShardContext(); + // Extract params from scripts and pass them along to ScriptedMetricAggregatorFactory, since it won't have // access to them for the scripts it's given precompiled. @@ -229,6 +223,14 @@ protected ScriptedMetricAggregatorFactory doBuild(SearchContext context, Aggrega ScriptedMetricAggContexts.MapScript.CONTEXT); Map mapScriptParams = mapScript.getParams(); + + ScriptedMetricAggContexts.CombineScript.Factory compiledCombineScript; + Map combineScriptParams; + + compiledCombineScript = queryShardContext.getScriptService().compile(combineScript, + ScriptedMetricAggContexts.CombineScript.CONTEXT); + combineScriptParams = combineScript.getParams(); + return new ScriptedMetricAggregatorFactory(name, compiledMapScript, mapScriptParams, compiledInitScript, initScriptParams, compiledCombineScript, combineScriptParams, reduceScript, params, queryShardContext.lookup(), context, parent, subfactoriesBuilder, metaData); diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/ScriptedMetricIT.java b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/ScriptedMetricIT.java index 20cdc892c2783..3c3fcddc32e8d 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/ScriptedMetricIT.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/ScriptedMetricIT.java @@ -153,6 +153,14 @@ protected Map, Object>> pluginScripts() { return newAggregation; }); + scripts.put("no-op aggregation", vars -> { + return (Map) vars.get("state"); + }); + + scripts.put("no-op list aggregation", vars -> { + return (List>) vars.get("states"); + }); + // Equivalent to: // // newaggregation = []; @@ -188,6 +196,11 @@ protected Map, Object>> pluginScripts() { Integer sum = 0; List> states = (List>) vars.get("states"); + + if(states == null) { + return newAggregation; + } + for (Map state : states) { List list = (List) state.get("list"); if (list != null) { @@ -329,9 +342,9 @@ protected Path nodeConfigPath(int nodeOrdinal) { public void testMap() { Script mapScript = new Script(ScriptType.INLINE, CustomScriptPlugin.NAME, "state['count'] = 1", Collections.emptyMap()); Script combineScript = - new Script(ScriptType.INLINE, CustomScriptPlugin.NAME, "sum state values as a new aggregation", Collections.emptyMap()); + new Script(ScriptType.INLINE, CustomScriptPlugin.NAME, "no-op aggregation", Collections.emptyMap()); Script reduceScript = new Script(ScriptType.INLINE, CustomScriptPlugin.NAME, - "sum all states (lists) values as a new aggregation", Collections.emptyMap()); + "no-op list aggregation", Collections.emptyMap()); SearchResponse response = client().prepareSearch("idx") .setQuery(matchAllQuery()) @@ -374,9 +387,9 @@ public void testMapWithParams() { Script mapScript = new Script(ScriptType.INLINE, CustomScriptPlugin.NAME, "state[param1] = param2", scriptParams); Script combineScript = - new Script(ScriptType.INLINE, CustomScriptPlugin.NAME, "sum state values as a new aggregation", Collections.emptyMap()); + new Script(ScriptType.INLINE, CustomScriptPlugin.NAME, "no-op aggregation", Collections.emptyMap()); Script reduceScript = new Script(ScriptType.INLINE, CustomScriptPlugin.NAME, - "sum all states (lists) values as a new aggregation", Collections.emptyMap()); + "no-op list aggregation", Collections.emptyMap()); SearchResponse response = client().prepareSearch("idx") .setQuery(matchAllQuery()) @@ -437,9 +450,9 @@ public void testInitMapWithParams() { .mapScript(new Script(ScriptType.INLINE, CustomScriptPlugin.NAME, "state.list.add(vars.multiplier)", Collections.emptyMap())) .combineScript(new Script(ScriptType.INLINE, CustomScriptPlugin.NAME, - "sum state values as a new aggregation", Collections.emptyMap())) + "no-op aggregation", Collections.emptyMap())) .reduceScript(new Script(ScriptType.INLINE, CustomScriptPlugin.NAME, - "sum all states (lists) values as a new aggregation", Collections.emptyMap()))) + "no-op list aggregation", Collections.emptyMap()))) .get(); assertSearchResponse(response); assertThat(response.getHits().getTotalHits(), equalTo(numDocs)); @@ -483,7 +496,7 @@ public void testMapCombineWithParams() { Script combineScript = new Script(ScriptType.INLINE, CustomScriptPlugin.NAME, "sum state values as a new aggregation", Collections.emptyMap()); Script reduceScript = new Script(ScriptType.INLINE, CustomScriptPlugin.NAME, - "sum all states (lists) values as a new aggregation", Collections.emptyMap()); + "no-op list aggregation", Collections.emptyMap()); SearchResponse response = client() .prepareSearch("idx") @@ -539,7 +552,7 @@ public void testInitMapCombineWithParams() { Script combineScript = new Script(ScriptType.INLINE, CustomScriptPlugin.NAME, "sum state values as a new aggregation", Collections.emptyMap()); Script reduceScript = new Script(ScriptType.INLINE, CustomScriptPlugin.NAME, - "sum all states (lists) values as a new aggregation", Collections.emptyMap()); + "no-op list aggregation", Collections.emptyMap()); SearchResponse response = client() .prepareSearch("idx") @@ -736,7 +749,7 @@ public void testInitMapReduceWithParams() { Script mapScript = new Script(ScriptType.INLINE, CustomScriptPlugin.NAME, "state.list.add(vars.multiplier)", Collections.emptyMap()); Script combineScript = - new Script(ScriptType.INLINE, CustomScriptPlugin.NAME, "sum state values as a new aggregation", Collections.emptyMap()); + new Script(ScriptType.INLINE, CustomScriptPlugin.NAME, "no-op aggregation", Collections.emptyMap()); Script reduceScript = new Script(ScriptType.INLINE, CustomScriptPlugin.NAME, "sum all states' state.list values as a new aggregation", Collections.emptyMap()); @@ -778,7 +791,7 @@ public void testMapReduceWithParams() { Script mapScript = new Script(ScriptType.INLINE, CustomScriptPlugin.NAME, "state.list.add(vars.multiplier)", Collections.emptyMap()); Script combineScript = - new Script(ScriptType.INLINE, CustomScriptPlugin.NAME, "sum state values as a new aggregation", Collections.emptyMap()); + new Script(ScriptType.INLINE, CustomScriptPlugin.NAME, "no-op aggregation", Collections.emptyMap()); Script reduceScript = new Script(ScriptType.INLINE, CustomScriptPlugin.NAME, "sum all states' state.list values as a new aggregation", Collections.emptyMap()); @@ -1008,6 +1021,11 @@ public void testEmptyAggregation() throws Exception { */ public void testDontCacheScripts() throws Exception { Script mapScript = new Script(ScriptType.INLINE, CustomScriptPlugin.NAME, "state['count'] = 1", Collections.emptyMap()); + Script combineScript = + new Script(ScriptType.INLINE, CustomScriptPlugin.NAME, "no-op aggregation", Collections.emptyMap()); + Script reduceScript = new Script(ScriptType.INLINE, CustomScriptPlugin.NAME, + "no-op list aggregation", Collections.emptyMap()); + assertAcked(prepareCreate("cache_test_idx").addMapping("type", "d", "type=long") .setSettings(Settings.builder().put("requests.cache.enable", true).put("number_of_shards", 1).put("number_of_replicas", 1)) .get()); @@ -1022,7 +1040,7 @@ public void testDontCacheScripts() throws Exception { // Test that a request using a script does not get cached SearchResponse r = client().prepareSearch("cache_test_idx").setSize(0) - .addAggregation(scriptedMetric("foo").mapScript(mapScript)).get(); + .addAggregation(scriptedMetric("foo").mapScript(mapScript).combineScript(combineScript).reduceScript(reduceScript)).get(); assertSearchResponse(r); assertThat(client().admin().indices().prepareStats("cache_test_idx").setRequestCache(true).get().getTotal().getRequestCache() @@ -1034,10 +1052,17 @@ public void testDontCacheScripts() throws Exception { public void testConflictingAggAndScriptParams() { Map params = Collections.singletonMap("param1", "12"); Script mapScript = new Script(ScriptType.INLINE, CustomScriptPlugin.NAME, "state.list.add(1)", params); + Script combineScript = + new Script(ScriptType.INLINE, CustomScriptPlugin.NAME, "no-op aggregation", Collections.emptyMap()); + Script reduceScript = new Script(ScriptType.INLINE, CustomScriptPlugin.NAME, + "no-op list aggregation", Collections.emptyMap()); SearchRequestBuilder builder = client().prepareSearch("idx") .setQuery(matchAllQuery()) - .addAggregation(scriptedMetric("scripted").params(params).mapScript(mapScript)); + .addAggregation(scriptedMetric("scripted") + .params(params).mapScript(mapScript) + .combineScript(combineScript) + .reduceScript(reduceScript)); SearchPhaseExecutionException ex = expectThrows(SearchPhaseExecutionException.class, builder::get); assertThat(ex.getCause().getMessage(), containsString("Parameter name \"param1\" used in both aggregation and script parameters")); From 120bab7a386b4f45c36f94c0df39ceec39d75a46 Mon Sep 17 00:00:00 2001 From: albendz Date: Tue, 2 Oct 2018 15:30:32 -0700 Subject: [PATCH 8/8] Add breaking change details to PR --- docs/reference/migration/migrate_7_0/aggregations.asciidoc | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/docs/reference/migration/migrate_7_0/aggregations.asciidoc b/docs/reference/migration/migrate_7_0/aggregations.asciidoc index b29f741dd8579..2b8c2ed9cb783 100644 --- a/docs/reference/migration/migrate_7_0/aggregations.asciidoc +++ b/docs/reference/migration/migrate_7_0/aggregations.asciidoc @@ -26,3 +26,9 @@ has been removed. `missing_bucket` should be used instead. The object used to share aggregation state between the scripts in a Scripted Metric Aggregation is now a variable called `state` available in the script context, rather than being provided via the `params` object as `params._agg`. + +[float] +==== Make metric aggregation script parameters `reduce_script` and `combine_script` mandatory + +The metric aggregation has been changed to require these two script parameters to ensure users are +explicitly defining how their data is processed.