Watcher: Fail email action on attachment download issues

In case that a single email attachment cannot be downloaded, this ensures
that the whole action fails with a correct Action.Failure.

This also fixes an NPE that would occur otherwise.

Original commit: elastic/x-pack-elasticsearch@7bb042a719

elasticsearch/x-pack/watcher/src/main/java/org/elasticsearch/watcher/actions/email/ExecutableEmailAction.java

@@ -5,6 +5,7 @@
  */
 package org.elasticsearch.watcher.actions.email;
 
+import org.elasticsearch.ElasticsearchException;
 import org.elasticsearch.common.logging.ESLogger;
 import org.elasticsearch.watcher.actions.Action;
 import org.elasticsearch.watcher.actions.ExecutableAction;
@@ -52,8 +53,12 @@ public class ExecutableEmailAction extends ExecutableAction<EmailAction> {
         if (action.getAttachments() != null && action.getAttachments().getAttachments().size() > 0) {
             for (EmailAttachmentParser.EmailAttachment emailAttachment : action.getAttachments().getAttachments()) {
                 EmailAttachmentParser parser = emailAttachmentParsers.get(emailAttachment.type());
-                Attachment attachment = parser.toAttachment(ctx, payload, emailAttachment);
+                try {
-                attachments.put(attachment.id(), attachment);
+                    Attachment attachment = parser.toAttachment(ctx, payload, emailAttachment);
+                    attachments.put(attachment.id(), attachment);
+                } catch (ElasticsearchException e) {
+                    return new EmailAction.Result.Failure(action.type(), e.getMessage());
+                }
             }
         }
 

elasticsearch/x-pack/watcher/src/main/java/org/elasticsearch/watcher/actions/email/service/attachment/EmailAttachmentParser.java

@@ -5,6 +5,7 @@
  */
 package org.elasticsearch.watcher.actions.email.service.attachment;
 
+import org.elasticsearch.ElasticsearchException;
 import org.elasticsearch.common.xcontent.ToXContent;
 import org.elasticsearch.common.xcontent.XContentParser;
 import org.elasticsearch.watcher.actions.email.service.Attachment;
@@ -48,6 +49,6 @@ public interface EmailAttachmentParser<T extends EmailAttachmentParser.EmailAtta
      * @param attachment The typed attachment
      * @return An attachment that is ready to be used in a MimeMessage
      */
-    Attachment toAttachment(WatchExecutionContext context, Payload payload, T attachment);
+    Attachment toAttachment(WatchExecutionContext context, Payload payload, T attachment) throws ElasticsearchException;
 
 }

elasticsearch/x-pack/watcher/src/main/java/org/elasticsearch/watcher/actions/email/service/attachment/HttpEmailAttachementParser.java

@@ -5,6 +5,7 @@
  */
 package org.elasticsearch.watcher.actions.email.service.attachment;
 
+import org.elasticsearch.ElasticsearchException;
 import org.elasticsearch.ElasticsearchParseException;
 import org.elasticsearch.common.ParseField;
 import org.elasticsearch.common.ParseFieldMatcher;
@@ -81,7 +82,8 @@ public class HttpEmailAttachementParser implements EmailAttachmentParser<HttpReq
     }
 
     @Override
-    public Attachment toAttachment(WatchExecutionContext context, Payload payload, HttpRequestAttachment attachment) {
+    public Attachment toAttachment(WatchExecutionContext context, Payload payload,
+                                   HttpRequestAttachment attachment) throws ElasticsearchException {
         Map<String, Object> model = Variables.createCtxModel(context, payload);
         HttpRequest httpRequest = attachment.getRequestTemplate().render(templateEngine, model);
 
@@ -106,6 +108,7 @@ public class HttpEmailAttachementParser implements EmailAttachmentParser<HttpReq
                     httpRequest.method(), httpRequest.path(), e.getMessage());
         }
 
-        return null;
+        throw new ElasticsearchException("Unable to get attachment of type [{}] with id [{}] in watch [{}] aborting watch execution",
+                type(), attachment.getId(), context.watch().id());
     }
 }

elasticsearch/x-pack/watcher/src/test/java/org/elasticsearch/watcher/actions/email/EmailActionTests.java

@@ -513,6 +513,65 @@ public class EmailActionTests extends ESTestCase {
         assertThat(dataAttachment.contentType(), is("application/yaml"));
     }
 
+    public void testThatOneFailedEmailAttachmentResultsInActionFailure() throws Exception {
+        EmailService emailService = new AbstractWatcherIntegrationTestCase.NoopEmailService();
+        TextTemplateEngine engine = mock(TextTemplateEngine.class);
+        HtmlSanitizer htmlSanitizer = mock(HtmlSanitizer.class);
+        HttpClient httpClient = mock(HttpClient.class);
+
+        // setup mock response, second one is an error
+        Map<String, String[]> headers = new HashMap<>(1);
+        headers.put(HttpHeaders.Names.CONTENT_TYPE, new String[]{"plain/text"});
+        when(httpClient.execute(any(HttpRequest.class)))
+                .thenReturn(new HttpResponse(200, "body", headers))
+                .thenReturn(new HttpResponse(403));
+
+        // setup email attachment parsers
+        HttpRequestTemplate.Parser httpRequestTemplateParser = new HttpRequestTemplate.Parser(registry);
+        Map<String, EmailAttachmentParser> attachmentParsers = new HashMap<>();
+        attachmentParsers.put(HttpEmailAttachementParser.TYPE, new HttpEmailAttachementParser(httpClient, httpRequestTemplateParser,
+                engine));
+        EmailAttachmentsParser emailAttachmentsParser = new EmailAttachmentsParser(attachmentParsers);
+
+        XContentBuilder builder = jsonBuilder().startObject()
+                .startObject("attachments")
+                .startObject("first")
+                .startObject("http")
+                .startObject("request").field("url", "http://localhost/first").endObject()
+                .endObject()
+                .endObject()
+                .startObject("second")
+                .startObject("http")
+                .startObject("request").field("url", "http://localhost/second").endObject()
+                .endObject()
+                .endObject()
+                .endObject()
+                .endObject();
+        XContentParser parser = JsonXContent.jsonXContent.createParser(builder.bytes());
+        logger.info("JSON: {}", builder.string());
+
+        parser.nextToken();
+
+        ExecutableEmailAction executableEmailAction = new EmailActionFactory(Settings.EMPTY, emailService, engine, htmlSanitizer,
+                emailAttachmentsParser).parseExecutable(randomAsciiOfLength(3), randomAsciiOfLength(7), parser);
+
+        DateTime now = DateTime.now(DateTimeZone.UTC);
+        Wid wid = new Wid(randomAsciiOfLength(5), randomLong(), now);
+        Map<String, Object> metadata = MapBuilder.<String, Object>newMapBuilder().put("_key", "_val").map();
+        WatchExecutionContext ctx = mockExecutionContextBuilder("watch1")
+                .wid(wid)
+                .payload(new Payload.Simple())
+                .time("watch1", now)
+                .metadata(metadata)
+                .buildMock();
+
+        Action.Result result = executableEmailAction.execute("test", ctx, new Payload.Simple());
+        assertThat(result, instanceOf(EmailAction.Result.Failure.class));
+        EmailAction.Result.Failure failure = (EmailAction.Result.Failure) result;
+        assertThat(failure.reason(),
+                is("Unable to get attachment of type [http] with id [second] in watch [watch1] aborting watch execution"));
+    }
+
     private EmailActionFactory createEmailActionFactory() {
         EmailService emailService = new AbstractWatcherIntegrationTestCase.NoopEmailService();
         TextTemplateEngine engine = mock(TextTemplateEngine.class);