From 1ef246adabbf430372a1c38dd1be7510aeac6121 Mon Sep 17 00:00:00 2001 From: Alexander Reelsen Date: Thu, 14 Apr 2016 09:03:24 +0200 Subject: [PATCH] Watcher: Fall back on default format color in hipchat action Our documentation states that we have default attributes for message.format and message.color, which in fact we do not have as an NPE was triggered in that case. This commit falls back to unset defaults and allows for hipchat messages to be sent without having to configure color/format in the action or the account. Closes elastic/elasticsearch#1666 Original commit: elastic/x-pack-elasticsearch@bfb7e35112ce63edd906e6fbb0378b6e0997bfba --- .../messy/tests/HipChatServiceIT.java | 29 ++++++++++--- .../text/DefaultTextTemplateEngine.java | 4 ++ .../hipchat/service/UserAccountTests.java | 41 +++++++++++++++++++ .../support/text/TextTemplateTests.java | 5 +++ .../watcher/test/MockTextTemplateEngine.java | 4 ++ 5 files changed, 77 insertions(+), 6 deletions(-) diff --git a/elasticsearch/qa/messy-test-xpack-with-mustache/src/test/java/org/elasticsearch/messy/tests/HipChatServiceIT.java b/elasticsearch/qa/messy-test-xpack-with-mustache/src/test/java/org/elasticsearch/messy/tests/HipChatServiceIT.java index 1d518dd8417..cfd68f10af1 100644 --- a/elasticsearch/qa/messy-test-xpack-with-mustache/src/test/java/org/elasticsearch/messy/tests/HipChatServiceIT.java +++ b/elasticsearch/qa/messy-test-xpack-with-mustache/src/test/java/org/elasticsearch/messy/tests/HipChatServiceIT.java @@ -90,7 +90,7 @@ public class HipChatServiceIT extends AbstractWatcherIntegrationTestCase { public void testSendMessageV1Account() throws Exception { HipChatService service = getInstanceFromMaster(HipChatService.class); HipChatMessage hipChatMessage = new HipChatMessage( - "/code HipChatServiceTests#testSendMessage_V1Account", + "HipChatServiceTests#testSendMessage_V1Account", new String[] { "test-watcher", "test-watcher-2" }, null, // users are unsupported in v1 "watcher-tests", @@ -108,7 +108,7 @@ public class HipChatServiceIT extends AbstractWatcherIntegrationTestCase { HipChatService service = getInstanceFromMaster(HipChatService.class); HipChatMessage.Color color = randomFrom(HipChatMessage.Color.values()); HipChatMessage hipChatMessage = new HipChatMessage( - "/code HipChatServiceTests#testSendMessage_IntegrationAccount colored " + color.value(), + "HipChatServiceTests#testSendMessage_IntegrationAccount colored " + color.value(), null, // custom rooms are unsupported by integration profiles null, // users are unsupported by integration profiles null, // custom "from" is not supported by integration profiles @@ -126,7 +126,7 @@ public class HipChatServiceIT extends AbstractWatcherIntegrationTestCase { HipChatService service = getInstanceFromMaster(HipChatService.class); HipChatMessage.Color color = randomFrom(HipChatMessage.Color.values()); HipChatMessage hipChatMessage = new HipChatMessage( - "/code HipChatServiceTests#testSendMessage_UserAccount colored " + color.value(), + "HipChatServiceTests#testSendMessage_UserAccount colored " + color.value(), new String[] { "test-watcher", "test-watcher-2" }, new String[] { "watcher@elastic.co" }, null, // custom "from" is not supported by integration profiles @@ -148,7 +148,7 @@ public class HipChatServiceIT extends AbstractWatcherIntegrationTestCase { switch (profile) { case USER: account = "user_account"; - actionBuilder = hipchatAction(account, "/code {{ctx.payload.ref}}") + actionBuilder = hipchatAction(account, "{{ctx.payload.ref}}") .addRooms("test-watcher", "test-watcher-2") .addUsers("watcher@elastic.co") .setFormat(HipChatMessage.Format.TEXT) @@ -158,7 +158,7 @@ public class HipChatServiceIT extends AbstractWatcherIntegrationTestCase { case INTEGRATION: account = "integration_account"; - actionBuilder = hipchatAction(account, "/code {{ctx.payload.ref}}") + actionBuilder = hipchatAction(account, "{{ctx.payload.ref}}") .setFormat(HipChatMessage.Format.TEXT) .setColor(color) .setNotify(false); @@ -167,7 +167,7 @@ public class HipChatServiceIT extends AbstractWatcherIntegrationTestCase { default: assertThat(profile, is(HipChatAccount.Profile.V1)); account = "v1_account"; - actionBuilder = hipchatAction(account, "/code {{ctx.payload.ref}}") + actionBuilder = hipchatAction(account, "{{ctx.payload.ref}}") .addRooms("test-watcher", "test-watcher-2") .setFrom("watcher-test") .setFormat(HipChatMessage.Format.TEXT) @@ -201,6 +201,23 @@ public class HipChatServiceIT extends AbstractWatcherIntegrationTestCase { assertThat(response.getHits().getTotalHits(), is(1L)); } + public void testDefaultValuesForColorAndFormatWorks() { + HipChatService service = getInstanceFromMaster(HipChatService.class); + HipChatMessage hipChatMessage = new HipChatMessage( + "HipChatServiceTests#testSendMessage_UserAccount with default Color and text", + new String[] { "test-watcher" }, + new String[] { "watcher@elastic.co" }, + null, // custom "from" is not supported by integration profiles + null, + null, + false); + + HipChatAccount account = service.getAccount("user_account"); + assertThat(account, notNullValue()); + SentMessages messages = account.send(hipChatMessage); + assertSentMessagesAreValid(2, messages); + } + private void assertSentMessagesAreValid(int expectedMessageSize, SentMessages messages) { assertThat(messages.count(), is(expectedMessageSize)); for (SentMessages.SentMessage message : messages) { diff --git a/elasticsearch/x-pack/watcher/src/main/java/org/elasticsearch/watcher/support/text/DefaultTextTemplateEngine.java b/elasticsearch/x-pack/watcher/src/main/java/org/elasticsearch/watcher/support/text/DefaultTextTemplateEngine.java index cd1e934e9cf..c079e5da222 100644 --- a/elasticsearch/x-pack/watcher/src/main/java/org/elasticsearch/watcher/support/text/DefaultTextTemplateEngine.java +++ b/elasticsearch/x-pack/watcher/src/main/java/org/elasticsearch/watcher/support/text/DefaultTextTemplateEngine.java @@ -31,6 +31,10 @@ public class DefaultTextTemplateEngine extends AbstractComponent implements Text @Override public String render(TextTemplate template, Map model) { + if (template == null) { + return null; + } + XContentType contentType = detectContentType(template); Map compileParams = compileParams(contentType); template = trimContentType(template); diff --git a/elasticsearch/x-pack/watcher/src/test/java/org/elasticsearch/watcher/actions/hipchat/service/UserAccountTests.java b/elasticsearch/x-pack/watcher/src/test/java/org/elasticsearch/watcher/actions/hipchat/service/UserAccountTests.java index b4d54e5fe78..dd2c01ef294 100644 --- a/elasticsearch/x-pack/watcher/src/test/java/org/elasticsearch/watcher/actions/hipchat/service/UserAccountTests.java +++ b/elasticsearch/x-pack/watcher/src/test/java/org/elasticsearch/watcher/actions/hipchat/service/UserAccountTests.java @@ -17,8 +17,11 @@ import org.elasticsearch.watcher.support.http.HttpMethod; import org.elasticsearch.watcher.support.http.HttpRequest; import org.elasticsearch.watcher.support.http.HttpResponse; import org.elasticsearch.watcher.support.http.Scheme; +import org.elasticsearch.watcher.support.text.TextTemplate; +import org.elasticsearch.watcher.test.MockTextTemplateEngine; import java.io.IOException; +import java.util.HashMap; import static org.elasticsearch.common.xcontent.XContentFactory.jsonBuilder; import static org.hamcrest.Matchers.arrayContaining; @@ -33,6 +36,7 @@ import static org.mockito.Mockito.when; * */ public class UserAccountTests extends ESTestCase { + public void testSettings() throws Exception { String accountName = "_name"; @@ -246,4 +250,41 @@ public class UserAccountTests extends ESTestCase { verify(httpClient).execute(reqU2); verify(httpClient).execute(reqU2); } + + public void testColorIsOptional() throws Exception { + Settings settings = Settings.builder() + .put("user", "testuser") + .put("auth_token", "awesome-auth-token") + .build(); + UserAccount userAccount = createUserAccount(settings); + + TextTemplate body = TextTemplate.inline("body").build(); + TextTemplate[] rooms = new TextTemplate[] { TextTemplate.inline("room").build() }; + HipChatMessage.Template template = new HipChatMessage.Template(body, rooms, null, "sender", HipChatMessage.Format.TEXT, null, true); + + HipChatMessage message = userAccount.render("watchId", "actionId", new MockTextTemplateEngine(), template, new HashMap<>()); + assertThat(message.color, is(nullValue())); + } + + public void testFormatIsOptional() throws Exception { + Settings settings = Settings.builder() + .put("user", "testuser") + .put("auth_token", "awesome-auth-token") + .build(); + UserAccount userAccount = createUserAccount(settings); + + TextTemplate body = TextTemplate.inline("body").build(); + TextTemplate[] rooms = new TextTemplate[] { TextTemplate.inline("room").build() }; + HipChatMessage.Template template = new HipChatMessage.Template(body, rooms, null, "sender", null, + TextTemplate.inline("yellow").build(), true); + + HipChatMessage message = userAccount.render("watchId", "actionId", new MockTextTemplateEngine(), template, new HashMap<>()); + assertThat(message.format, is(nullValue())); + } + + private UserAccount createUserAccount(Settings settings) { + HipChatServer hipChatServer = mock(HipChatServer.class); + HttpClient httpClient = mock(HttpClient.class); + return new UserAccount("notify-monitoring", settings, hipChatServer, httpClient, logger); + } } diff --git a/elasticsearch/x-pack/watcher/src/test/java/org/elasticsearch/watcher/support/text/TextTemplateTests.java b/elasticsearch/x-pack/watcher/src/test/java/org/elasticsearch/watcher/support/text/TextTemplateTests.java index 481efc09365..2731fcd8501 100644 --- a/elasticsearch/x-pack/watcher/src/test/java/org/elasticsearch/watcher/support/text/TextTemplateTests.java +++ b/elasticsearch/x-pack/watcher/src/test/java/org/elasticsearch/watcher/support/text/TextTemplateTests.java @@ -34,6 +34,7 @@ import static org.elasticsearch.watcher.support.Exceptions.illegalArgument; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.notNullValue; +import static org.hamcrest.Matchers.nullValue; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -186,6 +187,10 @@ public class TextTemplateTests extends ESTestCase { } } + public void testNullObject() throws Exception { + assertThat(engine.render(null ,new HashMap<>()), is(nullValue())); + } + private TextTemplate.Builder templateBuilder(ScriptType type, String text) { switch (type) { case INLINE: return TextTemplate.inline(text); diff --git a/elasticsearch/x-pack/watcher/src/test/java/org/elasticsearch/watcher/test/MockTextTemplateEngine.java b/elasticsearch/x-pack/watcher/src/test/java/org/elasticsearch/watcher/test/MockTextTemplateEngine.java index c5236983706..933f9bd79e0 100644 --- a/elasticsearch/x-pack/watcher/src/test/java/org/elasticsearch/watcher/test/MockTextTemplateEngine.java +++ b/elasticsearch/x-pack/watcher/src/test/java/org/elasticsearch/watcher/test/MockTextTemplateEngine.java @@ -13,6 +13,10 @@ import java.util.Map; public class MockTextTemplateEngine implements TextTemplateEngine { @Override public String render(TextTemplate template, Map model) { + if (template == null ) { + return null; + } + return template.getTemplate(); } }