Watcher: Fix pagerduty action to send context data (elastic/x-pack-elasticsearch#3185)

The pagerduty action allows to send contexts, which contains an array
of texts or images, each with a link.

The field of this data was named 'context' instead of 'contexts' and
thus those contects were never correctly parsed on the pagerduty side.

Unfortunately pagerduty accepts any JSON, thus this was not caught so
far.

This commit allows parsing of the old field name to retain BWC, but when
written out via toXContent, it will always use the 'contexts' field name.

relates elastic/x-pack-elasticsearch#3184

Original commit: elastic/x-pack-elasticsearch@50f0b65d56

docs/en/watcher/actions/pagerduty.asciidoc

@@ -45,7 +45,7 @@ payload as well as an array of contexts to the action.
       "attach_payload" : true,
       "client" : "/foo/bar/{{ctx.watch_id}}",
       "client_url" : "http://www.example.org/",
-      "context" : [
+      "contexts" : [
         {
           "type": "link",
           "href": "http://acme.pagerduty.com"

plugin/src/main/java/org/elasticsearch/xpack/watcher/notification/pagerduty/IncidentEvent.java

@@ -121,7 +121,7 @@ public class IncidentEvent implements ToXContentObject {
                             builder.endObject();
                         }
                         if (contexts != null && contexts.length > 0) {
-                            builder.startArray(Fields.CONTEXT.getPreferredName());
+                            builder.startArray(Fields.CONTEXTS.getPreferredName());
                             for (IncidentEventContext context : contexts) {
                                 context.toXContent(builder, params);
                             }
@@ -154,7 +154,7 @@ public class IncidentEvent implements ToXContentObject {
         }
         builder.field(Fields.ATTACH_PAYLOAD.getPreferredName(), attachPayload);
         if (contexts != null) {
-            builder.startArray(Fields.CONTEXT.getPreferredName());
+            builder.startArray(Fields.CONTEXTS.getPreferredName());
             for (IncidentEventContext context : contexts) {
                 context.toXContent(builder, params);
             }
@@ -265,7 +265,7 @@ public class IncidentEvent implements ToXContentObject {
                 proxy.toXContent(builder, params);
             }
             if (contexts != null) {
-                builder.startArray(Fields.CONTEXT.getPreferredName());
+                builder.startArray(Fields.CONTEXTS.getPreferredName());
                 for (IncidentEventContext.Template context : contexts) {
                     context.toXContent(builder, params);
                 }
@@ -341,7 +341,7 @@ public class IncidentEvent implements ToXContentObject {
                         throw new ElasticsearchParseException("could not parse pager duty event template. failed to parse field [{}], " +
                                 "expected a boolean value but found [{}] instead", Fields.ATTACH_PAYLOAD.getPreferredName(), token);
                     }
-                } else if (Fields.CONTEXT.match(currentFieldName)) {
+                } else if (Fields.CONTEXTS.match(currentFieldName) || Fields.CONTEXT_DEPRECATED.match(currentFieldName)) {
                     if (token == XContentParser.Token.START_ARRAY) {
                         List<IncidentEventContext.Template> list = new ArrayList<>();
                         while ((token = parser.nextToken()) != XContentParser.Token.END_ARRAY) {
@@ -349,7 +349,7 @@ public class IncidentEvent implements ToXContentObject {
                                 list.add(IncidentEventContext.Template.parse(parser));
                             } catch (ElasticsearchParseException e) {
                                 throw new ElasticsearchParseException("could not parse pager duty event template. failed to parse field " +
-                                        "[{}]", Fields.CONTEXT.getPreferredName());
+                                        "[{}]", parser.currentName());
                             }
                         }
                         contexts = list.toArray(new IncidentEventContext.Template[list.size()]);
@@ -438,7 +438,11 @@ public class IncidentEvent implements ToXContentObject {
         ParseField CLIENT = new ParseField("client");
         ParseField CLIENT_URL = new ParseField("client_url");
         ParseField ATTACH_PAYLOAD = new ParseField("attach_payload");
-        ParseField CONTEXT = new ParseField("context");
+        ParseField CONTEXTS = new ParseField("contexts");
+        // this field exists because in versions prior 6.0 we accidentally used context instead of contexts and thus the correct data
+        // was never picked up on the pagerduty side
+        // we need to keep this for BWC
+        ParseField CONTEXT_DEPRECATED = new ParseField("context");
 
         ParseField SERVICE_KEY = new ParseField("service_key");
         ParseField PAYLOAD = new ParseField("payload");

plugin/src/test/java/org/elasticsearch/xpack/watcher/actions/pagerduty/PagerDutyActionTests.java

@@ -14,20 +14,20 @@ import org.elasticsearch.common.xcontent.XContentBuilder;
 import org.elasticsearch.common.xcontent.XContentParser;
 import org.elasticsearch.common.xcontent.json.JsonXContent;
 import org.elasticsearch.test.ESTestCase;
+import org.elasticsearch.xpack.watcher.actions.Action;
 import org.elasticsearch.xpack.watcher.common.http.HttpProxy;
 import org.elasticsearch.xpack.watcher.common.http.HttpRequest;
 import org.elasticsearch.xpack.watcher.common.http.HttpResponse;
 import org.elasticsearch.xpack.watcher.common.text.TextTemplate;
 import org.elasticsearch.xpack.watcher.common.text.TextTemplateEngine;
+import org.elasticsearch.xpack.watcher.execution.WatchExecutionContext;
+import org.elasticsearch.xpack.watcher.execution.Wid;
 import org.elasticsearch.xpack.watcher.notification.pagerduty.IncidentEvent;
 import org.elasticsearch.xpack.watcher.notification.pagerduty.IncidentEventContext;
 import org.elasticsearch.xpack.watcher.notification.pagerduty.IncidentEventDefaults;
 import org.elasticsearch.xpack.watcher.notification.pagerduty.PagerDutyAccount;
 import org.elasticsearch.xpack.watcher.notification.pagerduty.PagerDutyService;
 import org.elasticsearch.xpack.watcher.notification.pagerduty.SentEvent;
-import org.elasticsearch.xpack.watcher.actions.Action;
-import org.elasticsearch.xpack.watcher.execution.WatchExecutionContext;
-import org.elasticsearch.xpack.watcher.execution.Wid;
 import org.elasticsearch.xpack.watcher.watch.Payload;
 import org.joda.time.DateTime;
 import org.joda.time.DateTimeZone;
@@ -176,7 +176,8 @@ public class PagerDutyActionTests extends ESTestCase {
                     IncidentEventContext.Template.link(new TextTemplate("_href"), new TextTemplate("_text")),
                     IncidentEventContext.Template.image(new TextTemplate("_src"), new TextTemplate("_href"), new TextTemplate("_alt"))
             };
-            builder.array("context", (Object) contexts);
+            String fieldName = randomBoolean() ? "contexts" : "context";
+            builder.array(fieldName, (Object) contexts);
         }
 
         builder.endObject();

plugin/src/test/java/org/elasticsearch/xpack/watcher/notification/pagerduty/PagerDutyAccountsTests.java

@@ -5,10 +5,12 @@
  */
 package org.elasticsearch.xpack.watcher.notification.pagerduty;
 
+import org.elasticsearch.common.bytes.BytesArray;
 import org.elasticsearch.common.settings.ClusterSettings;
 import org.elasticsearch.common.settings.Settings;
-import org.elasticsearch.common.settings.SettingsException;
+import org.elasticsearch.common.xcontent.json.JsonXContent;
 import org.elasticsearch.test.ESTestCase;
+import org.elasticsearch.test.rest.yaml.ObjectPath;
 import org.elasticsearch.xpack.watcher.common.http.HttpClient;
 import org.elasticsearch.xpack.watcher.common.http.HttpProxy;
 import org.elasticsearch.xpack.watcher.common.http.HttpRequest;
@@ -20,9 +22,7 @@ import org.mockito.ArgumentCaptor;
 
 import java.util.Collections;
 
-import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.is;
-import static org.hamcrest.Matchers.isOneOf;
 import static org.hamcrest.Matchers.notNullValue;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
@@ -36,102 +36,6 @@ public class PagerDutyAccountsTests extends ESTestCase {
         httpClient = mock(HttpClient.class);
     }
 
-    public void testSingleAccount() throws Exception {
-        Settings.Builder builder = Settings.builder()
-                .put("xpack.notification.pagerduty.default_account", "account1");
-        addAccountSettings("account1", builder);
-        PagerDutyService service = new PagerDutyService(builder.build(), httpClient, new ClusterSettings(Settings.EMPTY,
-                Collections.singleton(PagerDutyService.PAGERDUTY_ACCOUNT_SETTING)));
-        PagerDutyAccount account = service.getAccount("account1");
-        assertThat(account, notNullValue());
-        assertThat(account.name, equalTo("account1"));
-        account = service.getAccount(null); // falling back on the default
-        assertThat(account, notNullValue());
-        assertThat(account.name, equalTo("account1"));
-    }
-
-    public void testSingleAccountNoExplicitDefault() throws Exception {
-        Settings.Builder builder = Settings.builder();
-        addAccountSettings("account1", builder);
-
-        PagerDutyService service = new PagerDutyService(builder.build(), httpClient, new ClusterSettings(Settings.EMPTY,
-                Collections.singleton(PagerDutyService.PAGERDUTY_ACCOUNT_SETTING)));
-        PagerDutyAccount account = service.getAccount("account1");
-        assertThat(account, notNullValue());
-        assertThat(account.name, equalTo("account1"));
-        account = service.getAccount(null); // falling back on the default
-        assertThat(account, notNullValue());
-        assertThat(account.name, equalTo("account1"));
-    }
-
-    public void testMultipleAccounts() throws Exception {
-        Settings.Builder builder = Settings.builder()
-                .put("xpack.notification.pagerduty.default_account", "account1");
-        addAccountSettings("account1", builder);
-        addAccountSettings("account2", builder);
-
-        PagerDutyService service = new PagerDutyService(builder.build(), httpClient, new ClusterSettings(Settings.EMPTY,
-                Collections.singleton(PagerDutyService.PAGERDUTY_ACCOUNT_SETTING)));
-        PagerDutyAccount account = service.getAccount("account1");
-        assertThat(account, notNullValue());
-        assertThat(account.name, equalTo("account1"));
-        account = service.getAccount("account2");
-        assertThat(account, notNullValue());
-        assertThat(account.name, equalTo("account2"));
-        account = service.getAccount(null); // falling back on the default
-        assertThat(account, notNullValue());
-        assertThat(account.name, equalTo("account1"));
-    }
-
-    public void testMultipleAccounts_NoExplicitDefault() throws Exception {
-        Settings.Builder builder = Settings.builder()
-                .put("xpack.notification.pagerduty.default_account", "account1");
-        addAccountSettings("account1", builder);
-        addAccountSettings("account2", builder);
-
-        PagerDutyService service = new PagerDutyService(builder.build(), httpClient, new ClusterSettings(Settings.EMPTY,
-                Collections.singleton(PagerDutyService.PAGERDUTY_ACCOUNT_SETTING)));
-        PagerDutyAccount account = service.getAccount("account1");
-        assertThat(account, notNullValue());
-        assertThat(account.name, equalTo("account1"));
-        account = service.getAccount("account2");
-        assertThat(account, notNullValue());
-        assertThat(account.name, equalTo("account2"));
-        account = service.getAccount(null);
-        assertThat(account, notNullValue());
-        assertThat(account.name, isOneOf("account1", "account2"));
-    }
-
-    public void testMultipleAccounts_UnknownDefault() throws Exception {
-        expectThrows(SettingsException.class, () -> {
-            Settings.Builder builder = Settings.builder()
-                    .put("xpack.notification.pagerduty.default_account", "unknown");
-            addAccountSettings("account1", builder);
-            addAccountSettings("account2", builder);
-            new PagerDutyService(builder.build(), httpClient, new ClusterSettings(Settings.EMPTY,
-                    Collections.singleton(PagerDutyService.PAGERDUTY_ACCOUNT_SETTING)));
-        });
-    }
-
-    public void testNoAccount() throws Exception {
-        expectThrows(IllegalArgumentException.class, () -> {
-            Settings.Builder builder = Settings.builder();
-            PagerDutyService service = new PagerDutyService(builder.build(), httpClient, new ClusterSettings(Settings.EMPTY,
-                    Collections.singleton(PagerDutyService.PAGERDUTY_ACCOUNT_SETTING)));
-            service.getAccount(null);
-        });
-    }
-
-    public void testNoAccount_WithDefaultAccount() throws Exception {
-        try {
-            Settings.Builder builder = Settings.builder()
-                    .put("xpack.notification.pagerduty.default_account", "unknown");
-            new PagerDutyService(builder.build(), httpClient, new ClusterSettings(Settings.EMPTY,
-                    Collections.singleton(PagerDutyService.PAGERDUTY_ACCOUNT_SETTING)));
-            fail("Expected a SettingsException to happen");
-        } catch (SettingsException e) {}
-    }
-
     public void testProxy() throws Exception {
         Settings.Builder builder = Settings.builder().put("xpack.notification.pagerduty.default_account", "account1");
         addAccountSettings("account1", builder);
@@ -150,6 +54,31 @@ public class PagerDutyAccountsTests extends ESTestCase {
         assertThat(request.proxy(), is(proxy));
     }
 
+    // in earlier versions of the PD action the wrong JSON was sent, because the contexts field was named context
+    // the pagerduty API accepts any JSON, thus this was never caught
+    public void testContextIsSentCorrect() throws Exception {
+        Settings.Builder builder = Settings.builder().put("xpack.notification.pagerduty.default_account", "account1");
+        addAccountSettings("account1", builder);
+        PagerDutyService service = new PagerDutyService(builder.build(), httpClient, new ClusterSettings(Settings.EMPTY,
+                Collections.singleton(PagerDutyService.PAGERDUTY_ACCOUNT_SETTING)));
+        PagerDutyAccount account = service.getAccount("account1");
+
+        ArgumentCaptor<HttpRequest> argumentCaptor = ArgumentCaptor.forClass(HttpRequest.class);
+        when(httpClient.execute(argumentCaptor.capture())).thenReturn(new HttpResponse(200));
+
+        IncidentEventContext[] contexts = {
+                IncidentEventContext.link("https://www.elastic.co/products/x-pack/alerting", "Go to the Elastic.co Alerting website"),
+                IncidentEventContext.image("https://www.elastic.co/assets/blte5d899fd0b0e6808/icon-alerting-bb.svg",
+                        "https://www.elastic.co/products/x-pack/alerting", "X-Pack-Alerting website link with log")
+        };
+        IncidentEvent event = new IncidentEvent("foo", null, null, null, null, account.getName(), true, contexts, HttpProxy.NO_PROXY);
+        account.send(event, Payload.EMPTY);
+
+        HttpRequest request = argumentCaptor.getValue();
+        ObjectPath source = ObjectPath.createFromXContent(JsonXContent.jsonXContent, new BytesArray(request.body()));
+        assertThat(source.evaluate("contexts"), notNullValue());
+    }
+
     private void addAccountSettings(String name, Settings.Builder builder) {
         builder.put("xpack.notification.pagerduty.account." + name + ".service_api_key", randomAlphaOfLength(50));
         Settings defaults = SlackMessageDefaultsTests.randomSettings();

plugin/src/test/java/org/elasticsearch/xpack/watcher/test/integration/PagerDutyServiceTests.java

@@ -48,8 +48,9 @@ public class PagerDutyServiceTests extends AbstractWatcherIntegrationTestCase {
 
         IncidentEvent event = new IncidentEvent("#testIncidentEvent()", null, null, "PagerDutyServiceTests", "_client_url", "_account",
                 true, new IncidentEventContext[] {
-                IncidentEventContext.link("_href", "_text"),
+                IncidentEventContext.link("https://www.elastic.co/products/x-pack/alerting", "Go to the Elastic.co Alerting website"),
-                IncidentEventContext.image("_src", "_href", "_alt")
+                IncidentEventContext.image("https://www.elastic.co/assets/blte5d899fd0b0e6808/icon-alerting-bb.svg",
+                        "https://www.elastic.co/products/x-pack/alerting", "X-Pack-Alerting website link with log")
         }, null);
 
         Payload payload = new Payload.Simple("_key", "_val");