Streamlining code, fixed regression bug where the attachment names ar…

…e not ordered anymore, breaking unit tests
bbottema · Aug 16, 2019 · df0df01 · df0df01
1 parent a675673
commit df0df01
Show file tree

Hide file tree

Showing 3 changed files with 40 additions and 23 deletions.
diff --git a/src/main/java/org/simplejavamail/converter/internal/mimemessage/MimeMessageParser.java b/src/main/java/org/simplejavamail/converter/internal/mimemessage/MimeMessageParser.java
@@ -1,5 +1,7 @@
 package org.simplejavamail.converter.internal.mimemessage;
 
+import org.simplejavamail.internal.util.NaturalEntryKeyComparator;
+
 import javax.activation.DataHandler;
 import javax.activation.DataSource;
 import javax.annotation.Nonnull;
@@ -26,6 +28,7 @@
 import java.io.InputStream;
 import java.io.UnsupportedEncodingException;
 import java.util.*;
+import java.util.AbstractMap.SimpleEntry;
 
 import static java.lang.String.format;
 import static org.simplejavamail.internal.util.MiscUtil.extractCID;
@@ -126,21 +129,13 @@ private static void parseMimePartTree(@Nonnull final MimePart currentPart, @Nonn
 			final DataSource ds = createDataSource(currentPart);
 			// if the diposition is not provided, for now the part should be treated as inline (later non-embedded inline attachments are moved)
 			if (Part.ATTACHMENT.equalsIgnoreCase(disposition)) {
-				String resourceName = parseResourceName(parseContentID(currentPart), parseFileName(currentPart));
-				if (valueNullOrEmpty(resourceName)) {
-					resourceName = "unnamed";
-				}
-				parsedComponents.attachmentList.add(new AbstractMap.SimpleEntry(resourceName, ds));
+				parsedComponents.attachmentList.add(new SimpleEntry<>(parseResourceNameOrUnnamed(parseContentID(currentPart), parseFileName(currentPart)), ds));
 			} else if (disposition == null || Part.INLINE.equalsIgnoreCase(disposition)) {
 				if (parseContentID(currentPart) != null) {
 					parsedComponents.cidMap.put(parseContentID(currentPart), ds);
 				} else {
 					// contentID missing -> treat as standard attachment
-					String resourceName = parseResourceName(null, parseFileName(currentPart));
-					if (valueNullOrEmpty(resourceName)) {
-						resourceName = "unnamed";
-					}
-					parsedComponents.attachmentList.add(new AbstractMap.SimpleEntry(resourceName, ds));
+					parsedComponents.attachmentList.add(new SimpleEntry<>(parseResourceNameOrUnnamed(null, parseFileName(currentPart)), ds));
 				}
 			} else {
 				throw new IllegalStateException("invalid attachment type");
@@ -220,7 +215,13 @@ public static String parseDisposition(@Nonnull final MimePart currentPart) {
 	}
 
 	@Nonnull
-	private static String parseResourceName(@Nullable final String possibleWrappedContentID, @Nonnull final String fileName) {
+	private static String parseResourceNameOrUnnamed(@Nullable final String possibleWrappedContentID, @Nonnull final String fileName) {
+		String resourceName = parseResourceName(possibleWrappedContentID, fileName);
+		return valueNullOrEmpty(resourceName) ? "unnamed" : resourceName;
+	}
+
+	@Nonnull
+	private static String parseResourceName(@Nullable String possibleWrappedContentID, @Nonnull String fileName) {
 		if (!valueNullOrEmpty(possibleWrappedContentID)) {
 			// https://regex101.com/r/46ulb2/1
 			String unwrappedContentID = possibleWrappedContentID.replaceAll("^<?(.*?)>?$", "$1");
@@ -232,7 +233,7 @@ private static String parseResourceName(@Nullable final String possibleWrappedCo
 			return fileName;
 		}
 	}
-
+	
 	@SuppressWarnings("WeakerAccess")
 	@Nonnull
 	public static List<Header> retrieveAllHeaders(@Nonnull final MimePart part) {
@@ -455,14 +456,15 @@ static void moveInvalidEmbeddedResourcesToAttachments(ParsedMimeMessageComponent
 			Map.Entry<String, DataSource> cidEntry = it.next();
 			String cid = extractCID(cidEntry.getKey());
 			if (htmlContent == null || !htmlContent.contains("cid:" + cid)) {
-				parsedComponents.attachmentList.add(new AbstractMap.SimpleEntry(cid, cidEntry.getValue()));
+				parsedComponents.attachmentList.add(new SimpleEntry<>(cid, cidEntry.getValue()));
 				it.remove();
 			}
 		}
 	}
 
 	public static class ParsedMimeMessageComponents {
-		final List<Map.Entry<String, DataSource>> attachmentList = new ArrayList<>();
+		@SuppressWarnings("unchecked")
+		final Set<Map.Entry<String, DataSource>> attachmentList = new TreeSet<>(NaturalEntryKeyComparator.INSTANCE);
 		final Map<String, DataSource> cidMap = new TreeMap<>();
 		private final Map<String, Object> headers = new HashMap<>();
 		private final List<InternetAddress> toAddresses = new ArrayList<>();
@@ -482,7 +484,7 @@ public String getMessageId() {
 			return messageId;
 		}
 
-		public List<Map.Entry<String, DataSource>> getAttachmentList() {
+		public Set<Map.Entry<String, DataSource>> getAttachmentList() {
 			return attachmentList;
 		}
 

diff --git a/src/main/java/org/simplejavamail/internal/util/NaturalEntryKeyComparator.java b/src/main/java/org/simplejavamail/internal/util/NaturalEntryKeyComparator.java
@@ -0,0 +1,19 @@
+package org.simplejavamail.internal.util;
+
+import java.util.Comparator;
+import java.util.Map;
+
+public class NaturalEntryKeyComparator<T extends Comparable<T>> implements Comparator<Map.Entry<T, Object>> {
+
+	public static final NaturalEntryKeyComparator INSTANCE = new NaturalEntryKeyComparator();
+
+	// TODO Lombok
+	private NaturalEntryKeyComparator(){
+	}
+
+
+	@Override
+	public int compare(Map.Entry<T, Object> o1, Map.Entry<T, Object> o2) {
+		return o1.getKey().compareTo(o2.getKey());
+	}
+}
diff --git a/src/test/java/org/simplejavamail/converter/internal/mimemessage/MimeMessageParserTest.java b/src/test/java/org/simplejavamail/converter/internal/mimemessage/MimeMessageParserTest.java
@@ -18,10 +18,9 @@ public void testMoveInvalidEmbeddedResourcesToAttachments_NoHtmlNoInvalid() thro
 		moveInvalidEmbeddedResourcesToAttachments(parsedComponents);
 
 		assertThat(parsedComponents.cidMap).isEmpty();
-		assertThat(parsedComponents.attachmentList.size()).isEqualTo(2);
-		assertThat(parsedComponents.attachmentList.get(0).getKey()).isEqualTo("moo1");
-		assertThat(parsedComponents.attachmentList.get(1).getKey()).isEqualTo("moo2");
+		assertThat(parsedComponents.attachmentList).extracting("key").containsOnly("moo1", "moo2");
 	}
+
 	@Test
 	public void testMoveInvalidEmbeddedResourcesToAttachments_HtmlButNoInvalid() throws IOException {
 		ParsedMimeMessageComponents parsedComponents = new ParsedMimeMessageComponents();
@@ -31,9 +30,7 @@ public void testMoveInvalidEmbeddedResourcesToAttachments_HtmlButNoInvalid() thr
 		moveInvalidEmbeddedResourcesToAttachments(parsedComponents);
 
 		assertThat(parsedComponents.cidMap).isEmpty();
-		assertThat(parsedComponents.attachmentList.size()).isEqualTo(2);
-		assertThat(parsedComponents.attachmentList.get(0).getKey()).isEqualTo("moo1");
-		assertThat(parsedComponents.attachmentList.get(1).getKey()).isEqualTo("moo2");
+		assertThat(parsedComponents.attachmentList).extracting("key").containsOnly("moo1", "moo2");
 	}
 
 	@Test
@@ -45,7 +42,6 @@ public void testMoveInvalidEmbeddedResourcesToAttachments_Invalid() throws IOExc
 		moveInvalidEmbeddedResourcesToAttachments(parsedComponents);
 
 		assertThat(parsedComponents.cidMap).containsOnlyKeys("moo1");
-		assertThat(parsedComponents.attachmentList.size()).isEqualTo(1);
-		assertThat(parsedComponents.attachmentList.get(0).getKey()).isEqualTo("moo2");
+		assertThat(parsedComponents.attachmentList).extracting("key").containsOnly("moo2");
 	}
 }