Watcher: Fix correct setting of email attachment names

Fix to ensure that the email attachment has a correctly set filename, which is
also now explained in the documentation.

In addition there is a check now for email attachments, that a filename can only
be specified once, otherwise an exception is thrown.

Closes elastic/elasticsearch#1503

Original commit: elastic/x-pack-elasticsearch@2a399058b3

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

@@ -34,7 +34,7 @@ public enum DataAttachment implements ToXContent {
 
         @Override
         public Attachment create(String id, Map<String, Object> data) {
-            return new Attachment.XContent.Yaml(id, "data.yml", new Payload.Simple(data));
+            return new Attachment.XContent.Yaml(id, id, new Payload.Simple(data));
         }
 
         @Override
@@ -51,7 +51,7 @@ public enum DataAttachment implements ToXContent {
 
         @Override
         public Attachment create(String id, Map<String, Object> data) {
-            return new Attachment.XContent.Json(id, "data.json", new Payload.Simple(data));
+            return new Attachment.XContent.Json(id, id, new Payload.Simple(data));
         }
 
         @Override

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

@@ -32,7 +32,6 @@ public class DataAttachment implements EmailAttachmentParser.EmailAttachment {
         } else {
             builder.field("format", "json");
         }
-
         return builder.endObject().endObject();
     }
 

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

@@ -25,6 +25,11 @@ public interface EmailAttachmentParser<T extends EmailAttachmentParser.EmailAtta
          * @return A type to identify the email attachment, same as the parser identifier
          */
         String type();
+
+        /**
+         * @return The id of this attachment
+         */
+        String id();
     }
 
     /**

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

@@ -10,8 +10,8 @@ import org.elasticsearch.common.xcontent.ToXContent;
 import org.elasticsearch.common.xcontent.XContentBuilder;
 
 import java.io.IOException;
+import java.util.Collection;
 import java.util.Collections;
-import java.util.List;
 import java.util.Objects;
 
 public class EmailAttachments implements ToXContent {
@@ -23,13 +23,13 @@ public class EmailAttachments implements ToXContent {
         ParseField ATTACHMENTS = new ParseField("attachments");
     }
 
-    private final List<EmailAttachmentParser.EmailAttachment> attachments;
+    private final Collection<EmailAttachmentParser.EmailAttachment> attachments;
 
-    public EmailAttachments(List<EmailAttachmentParser.EmailAttachment> attachments) {
+    public EmailAttachments(Collection<EmailAttachmentParser.EmailAttachment> attachments) {
         this.attachments = attachments;
     }
 
-    public List<EmailAttachmentParser.EmailAttachment> getAttachments() {
+    public Collection<EmailAttachmentParser.EmailAttachment> getAttachments() {
         return attachments;
     }
 

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

@@ -12,7 +12,7 @@ import org.elasticsearch.common.xcontent.XContentParser;
 
 import java.io.IOException;
 import java.util.ArrayList;
-import java.util.List;
+import java.util.LinkedHashMap;
 import java.util.Map;
 
 public class EmailAttachmentsParser {
@@ -25,7 +25,7 @@ public class EmailAttachmentsParser {
     }
 
     public EmailAttachments parse(XContentParser parser) throws IOException {
-        List<EmailAttachmentParser.EmailAttachment> attachments = new ArrayList<>();
+        Map<String, EmailAttachmentParser.EmailAttachment> attachments = new LinkedHashMap<>();
         String currentFieldName = null;
         XContentParser.Token token;
         while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) {
@@ -41,17 +41,21 @@ public class EmailAttachmentsParser {
 
                     EmailAttachmentParser emailAttachmentParser = parsers.get(currentAttachmentType);
                     if (emailAttachmentParser == null) {
-                        throw new ElasticsearchParseException("Cannot parse attachment of type " + currentAttachmentType);
+                        throw new ElasticsearchParseException("Cannot parse attachment of type [{}]", currentAttachmentType);
                     }
                     EmailAttachmentParser.EmailAttachment emailAttachment = emailAttachmentParser.parse(currentFieldName, parser);
-                    attachments.add(emailAttachment);
+                    if (attachments.containsKey(emailAttachment.id())) {
+                        throw new ElasticsearchParseException("Attachment with id [{}] has already been created, must be renamed",
+                                emailAttachment.id());
+                    }
+                    attachments.put(emailAttachment.id(), emailAttachment);
                     // one further to skip the end_object from the attachment
                     parser.nextToken();
                 }
             }
         }
 
-        return new EmailAttachments(attachments);
+        return new EmailAttachments(new ArrayList<>(attachments.values()));
     }
 
     public Map<String, EmailAttachmentParser> getParsers() {

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

@@ -94,7 +94,7 @@ public class HttpEmailAttachementParser implements EmailAttachmentParser<HttpReq
                 if (response.hasContent()) {
                     String contentType = attachment.getContentType();
                     String attachmentContentType = Strings.hasLength(contentType) ? contentType : response.contentType();
-                    return new Attachment.Bytes(attachment.getId(), response.body().toBytes(), attachmentContentType);
+                    return new Attachment.Bytes(attachment.id(), response.body().toBytes(), attachmentContentType);
                 } else {
                     logger.error("Empty response body: [host[{}], port[{}], method[{}], path[{}]: response status [{}]", httpRequest.host(),
                             httpRequest.port(), httpRequest.method(), httpRequest.path(), response.status());
@@ -109,6 +109,6 @@ public class HttpEmailAttachementParser implements EmailAttachmentParser<HttpReq
         }
 
         throw new ElasticsearchException("Unable to get attachment of type [{}] with id [{}] in watch [{}] aborting watch execution",
-                type(), attachment.getId(), context.watch().id());
+                type(), attachment.id(), context.watch().id());
     }
 }

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

@@ -17,7 +17,7 @@ public class HttpRequestAttachment implements EmailAttachmentParser.EmailAttachm
 
     private final HttpRequestTemplate requestTemplate;
     private final String contentType;
-    private String id;
+    private final String id;
 
     public HttpRequestAttachment(String id, HttpRequestTemplate requestTemplate, @Nullable String contentType) {
         this.id = id;
@@ -33,7 +33,8 @@ public class HttpRequestAttachment implements EmailAttachmentParser.EmailAttachm
         return contentType;
     }
 
-    public String getId() {
+    @Override
+    public String id() {
         return id;
     }
 

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

@@ -17,10 +17,8 @@ import java.util.Map;
 import static java.util.Collections.singletonMap;
 import static org.hamcrest.Matchers.is;
 
-/**
- *
- */
 public class DataAttachmentTests extends ESTestCase {
+
     public void testCreateJson() throws Exception {
         Map<String, Object> data = singletonMap("key", "value");
         Attachment attachment = DataAttachment.JSON.create("data", data);

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

@@ -481,7 +481,7 @@ public class EmailActionTests extends ESTestCase {
     }
 
     public void testThatDataAttachmentGetsAttachedWithId() throws Exception {
-        String attachmentId = "my_attachment";
+        String attachmentId = randomAsciiOfLength(10) + ".yml";
 
         XContentBuilder builder = jsonBuilder().startObject()
                 .startObject("attachments")
@@ -506,9 +506,9 @@ public class EmailActionTests extends ESTestCase {
         EmailAction.Result.Success successResult = (EmailAction.Result.Success) result;
         Map<String, Attachment> attachments = successResult.email().attachments();
 
-        assertThat(attachments, hasKey("my_attachment"));
+        assertThat(attachments, hasKey(attachmentId));
-        Attachment dataAttachment = attachments.get("my_attachment");
+        Attachment dataAttachment = attachments.get(attachmentId);
-        assertThat(dataAttachment.name(), is("data.yml"));
+        assertThat(dataAttachment.name(), is(attachmentId));
         assertThat(dataAttachment.type(), is("yaml"));
         assertThat(dataAttachment.contentType(), is("application/yaml"));
     }

elasticsearch/x-pack/watcher/src/test/java/org/elasticsearch/watcher/actions/email/service/attachment/DataAttachmentParserTests.java

@@ -11,7 +11,9 @@ import org.elasticsearch.common.xcontent.XContentParser;
 import org.elasticsearch.common.xcontent.json.JsonXContent;
 import org.elasticsearch.test.ESTestCase;
 
+import java.util.ArrayList;
 import java.util.HashMap;
+import java.util.List;
 import java.util.Map;
 
 import static org.elasticsearch.common.xcontent.XContentFactory.jsonBuilder;
@@ -36,7 +38,8 @@ public class DataAttachmentParserTests extends ESTestCase {
         assertThat(emailAttachments.getAttachments(), hasSize(1));
 
         XContentBuilder toXcontentBuilder = jsonBuilder().startObject();
-        emailAttachments.getAttachments().get(0).toXContent(toXcontentBuilder, ToXContent.EMPTY_PARAMS);
+        List<EmailAttachmentParser.EmailAttachment> attachments = new ArrayList<>(emailAttachments.getAttachments());
+        attachments.get(0).toXContent(toXcontentBuilder, ToXContent.EMPTY_PARAMS);
         toXcontentBuilder.endObject();
         assertThat(toXcontentBuilder.string(), is(builder.string()));
     }

elasticsearch/x-pack/watcher/src/test/java/org/elasticsearch/watcher/actions/email/service/attachment/EmailAttachmentParsersTests.java

@@ -61,15 +61,15 @@ public class EmailAttachmentParsersTests extends ESTestCase {
         EmailAttachments attachments = parser.parse(xContentParser);
         assertThat(attachments.getAttachments(), hasSize(2));
 
-        EmailAttachmentParser.EmailAttachment emailAttachment = attachments.getAttachments().get(0);
+        List<EmailAttachmentParser.EmailAttachment> emailAttachments = new ArrayList<>(attachments.getAttachments());
+        EmailAttachmentParser.EmailAttachment emailAttachment = emailAttachments.get(0);
         assertThat(emailAttachment, instanceOf(TestEmailAttachment.class));
 
         Attachment attachment = parsers.get("test").toAttachment(ctx, new Payload.Simple(), emailAttachment);
         assertThat(attachment.name(), is("my-id"));
         assertThat(attachment.contentType(), is("personalContentType"));
 
-        assertThat(parsers.get("test").toAttachment(ctx, new Payload.Simple(),
+        assertThat(parsers.get("test").toAttachment(ctx, new Payload.Simple(), emailAttachments.get(1)).id(), is("my-other-id"));
-                attachments.getAttachments().get(1)).id(), is("my-other-id"));
     }
 
     public void testThatUnknownParserThrowsException() throws IOException {
@@ -84,13 +84,13 @@ public class EmailAttachmentParsersTests extends ESTestCase {
             parser.parse(xContentParser);
             fail("Expected random parser of type [" + type + "] to throw an exception");
         } catch (ElasticsearchParseException e) {
-            assertThat(e.getMessage(), containsString("Cannot parse attachment of type " + type));
+            assertThat(e.getMessage(), containsString("Cannot parse attachment of type [" + type + "]"));
         }
     }
 
     public void testThatToXContentSerializationWorks() throws Exception {
         List<EmailAttachmentParser.EmailAttachment> attachments = new ArrayList<>();
-        attachments.add(new DataAttachment("my-id", org.elasticsearch.watcher.actions.email.DataAttachment.JSON));
+        attachments.add(new DataAttachment("my-name.json", org.elasticsearch.watcher.actions.email.DataAttachment.JSON));
 
         HttpRequestTemplate requestTemplate = HttpRequestTemplate.builder("localhost", 80).scheme(Scheme.HTTP).path("/").build();
         HttpRequestAttachment httpRequestAttachment = new HttpRequestAttachment("other-id", requestTemplate, null);
@@ -100,13 +100,36 @@ public class EmailAttachmentParsersTests extends ESTestCase {
         XContentBuilder builder = jsonBuilder();
         emailAttachments.toXContent(builder, ToXContent.EMPTY_PARAMS);
         logger.info("JSON is: " + builder.string());
-        assertThat(builder.string(), containsString("my-id"));
+        assertThat(builder.string(), containsString("my-name.json"));
         assertThat(builder.string(), containsString("json"));
         assertThat(builder.string(), containsString("other-id"));
         assertThat(builder.string(), containsString("localhost"));
         assertThat(builder.string(), containsString("/"));
     }
 
+    public void testThatTwoAttachmentsWithTheSameIdThrowError() throws Exception {
+        Map<String, EmailAttachmentParser> parsers = new HashMap<>();
+        parsers.put("test", new TestEmailAttachmentParser());
+        EmailAttachmentsParser parser = new EmailAttachmentsParser(parsers);
+
+        List<EmailAttachmentParser.EmailAttachment> attachments = new ArrayList<>();
+        attachments.add(new TestEmailAttachment("my-name.json", "value"));
+        attachments.add(new TestEmailAttachment("my-name.json", "value"));
+
+        EmailAttachments emailAttachments = new EmailAttachments(attachments);
+        XContentBuilder builder = jsonBuilder();
+        emailAttachments.toXContent(builder, ToXContent.EMPTY_PARAMS);
+        logger.info("JSON is: " + builder.string());
+
+        XContentParser xContentParser = JsonXContent.jsonXContent.createParser(builder.bytes());
+        try {
+            parser.parse(xContentParser);
+            fail("Expected parser to fail but did not happen");
+        } catch (ElasticsearchParseException e) {
+            assertThat(e.getMessage(), is("Attachment with id [my-name.json] has already been created, must be renamed"));
+        }
+    }
+
     public class TestEmailAttachmentParser implements EmailAttachmentParser<TestEmailAttachment> {
 
         @Override
@@ -138,7 +161,7 @@ public class EmailAttachmentParsersTests extends ESTestCase {
 
         @Override
         public Attachment toAttachment(WatchExecutionContext ctx, Payload payload, TestEmailAttachment attachment) {
-            return new Attachment.Bytes(attachment.getId(), attachment.getValue().getBytes(Charsets.UTF_8), "personalContentType");
+            return new Attachment.Bytes(attachment.id(), attachment.getValue().getBytes(Charsets.UTF_8), "personalContentType");
         }
     }
 
@@ -165,7 +188,8 @@ public class EmailAttachmentParsersTests extends ESTestCase {
             return value;
         }
 
-        public String getId() {
+        @Override
+        public String id() {
             return id;
         }
 

elasticsearch/x-pack/watcher/src/test/java/org/elasticsearch/watcher/actions/email/service/attachment/HttpEmailAttachementParserTests.java

@@ -21,7 +21,9 @@ import org.elasticsearch.watcher.support.secret.SecretService;
 import org.elasticsearch.watcher.test.MockTextTemplateEngine;
 import org.junit.Before;
 
+import java.util.ArrayList;
 import java.util.HashMap;
+import java.util.List;
 import java.util.Map;
 
 import static java.nio.charset.StandardCharsets.UTF_8;
@@ -83,7 +85,8 @@ public class HttpEmailAttachementParserTests extends ESTestCase {
         assertThat(emailAttachments.getAttachments(), hasSize(1));
 
         XContentBuilder toXcontentBuilder = jsonBuilder().startObject();
-        emailAttachments.getAttachments().get(0).toXContent(toXcontentBuilder, ToXContent.EMPTY_PARAMS);
+        List<EmailAttachmentParser.EmailAttachment> attachments = new ArrayList<>(emailAttachments.getAttachments());
+        attachments.get(0).toXContent(toXcontentBuilder, ToXContent.EMPTY_PARAMS);
         toXcontentBuilder.endObject();
         assertThat(toXcontentBuilder.string(), is(builder.string()));
     }