DO NOT MERGE. Prevent duplication of POP3 attachments
* For each attachment we add, check the DB for an existing attachment
with similar metadata (name, mime type, content id, etc.)
* Skip adding them if already held
* Unit tests
Originally fixed in 5b0a12c1991988 / CL I036f39c6
Fixes bug http://b/2084704
diff --git a/src/com/android/email/LegacyConversions.java b/src/com/android/email/LegacyConversions.java
index cd39aeb..4c1ccf8 100644
--- a/src/com/android/email/LegacyConversions.java
+++ b/src/com/android/email/LegacyConversions.java
@@ -38,6 +38,7 @@
import android.content.ContentUris;
import android.content.ContentValues;
import android.content.Context;
+import android.database.Cursor;
import android.net.Uri;
import android.util.Log;
@@ -50,6 +51,9 @@
public class LegacyConversions {
+ /** DO NOT CHECK IN "TRUE" */
+ private static final boolean DEBUG_ATTACHMENTS = false;
+
/**
* Values for HEADER_ANDROID_BODY_QUOTED_PART to tag body parts
*/
@@ -255,8 +259,14 @@
/**
* Add a single attachment part to the message
*
- * TODO: This will simply add to any existing attachments - could this ever happen? If so,
- * change it to find existing attachments and delete/merge them.
+ * This will skip adding attachments if they are already found in the attachments table.
+ * The heuristic for this will fail (false-positive) if two identical attachments are
+ * included in a single POP3 message.
+ * TODO: Fix that, by (elsewhere) simulating an mLocation value based on the attachments
+ * position within the list of multipart/mixed elements. This would make every POP3 attachment
+ * unique, and might also simplify the code (since we could just look at the positions, and
+ * ignore the filename, etc.)
+ *
* TODO: Take a closer look at encoding and deal with it if necessary.
*
* @param context a context for file operations
@@ -301,8 +311,44 @@
localAttachment.mLocation = partId;
localAttachment.mEncoding = "B"; // TODO - convert other known encodings
+ if (DEBUG_ATTACHMENTS) {
+ Log.d(Email.LOG_TAG, "Add attachment " + localAttachment);
+ }
+
+ // To prevent duplication - do we already have a matching attachment?
+ // The fields we'll check for equality are:
+ // mFileName, mMimeType, mContentId, mMessageKey, mLocation
+ // NOTE: This will false-positive if you attach the exact same file, twice, to a POP3
+ // message. We can live with that - you'll get one of the copies.
+ Uri uri = ContentUris.withAppendedId(Attachment.MESSAGE_ID_URI, localMessage.mId);
+ Cursor cursor = context.getContentResolver().query(uri, Attachment.CONTENT_PROJECTION,
+ null, null, null);
+ boolean attachmentFoundInDb = false;
+ try {
+ while (cursor.moveToNext()) {
+ Attachment dbAttachment = new Attachment().restore(cursor);
+ // We test each of the fields here (instead of in SQL) because they may be
+ // null, or may be strings.
+ if (stringNotEqual(dbAttachment.mFileName, localAttachment.mFileName)) continue;
+ if (stringNotEqual(dbAttachment.mMimeType, localAttachment.mMimeType)) continue;
+ if (stringNotEqual(dbAttachment.mContentId, localAttachment.mContentId)) continue;
+ if (stringNotEqual(dbAttachment.mLocation, localAttachment.mLocation)) continue;
+ // We found a match, so use the existing attachment id, and stop looking/looping
+ attachmentFoundInDb = true;
+ localAttachment.mId = dbAttachment.mId;
+ if (DEBUG_ATTACHMENTS) {
+ Log.d(Email.LOG_TAG, "Skipped, found db attachment " + dbAttachment);
+ }
+ break;
+ }
+ } finally {
+ cursor.close();
+ }
+
// Save the attachment (so far) in order to obtain an id
- localAttachment.save(context);
+ if (!attachmentFoundInDb) {
+ localAttachment.save(context);
+ }
// If an attachment body was actually provided, we need to write the file now
saveAttachmentBody(context, part, localAttachment, localMessage.mAccountKey);
@@ -315,6 +361,17 @@
}
/**
+ * Helper for addOneAttachment that compares two strings, deals with nulls, and treats
+ * nulls and empty strings as equal.
+ */
+ /* package */ static boolean stringNotEqual(String a, String b) {
+ if (a == null && b == null) return false; // fast exit for two null strings
+ if (a == null) a = "";
+ if (b == null) b = "";
+ return !a.equals(b);
+ }
+
+ /**
* Save the body part of a single attachment, to a file in the attachments directory.
*/
public static void saveAttachmentBody(Context context, Part part, Attachment localAttachment,
diff --git a/src/com/android/email/provider/EmailContent.java b/src/com/android/email/provider/EmailContent.java
index 65f6775..62c7c53 100644
--- a/src/com/android/email/provider/EmailContent.java
+++ b/src/com/android/email/provider/EmailContent.java
@@ -1657,6 +1657,12 @@
return new EmailContent.Attachment[size];
}
};
+
+ @Override
+ public String toString() {
+ return "[" + mFileName + ", " + mMimeType + ", " + mSize + ", " + mContentId + ", "
+ + mContentUri + ", " + mMessageKey + ", " + mLocation + ", " + mEncoding + "]";
+ }
}
public interface MailboxColumns {
diff --git a/tests/src/com/android/email/LegacyConversionsTests.java b/tests/src/com/android/email/LegacyConversionsTests.java
index 91674cc..3fceb32 100644
--- a/tests/src/com/android/email/LegacyConversionsTests.java
+++ b/tests/src/com/android/email/LegacyConversionsTests.java
@@ -203,31 +203,7 @@
"local-message", accountId, mailboxId, false, true, mProviderContext);
// Prepare a legacy message with attachments
- Part attachment1Part = MessageTestUtils.bodyPart("image/gif", null);
- attachment1Part.setHeader(MimeHeader.HEADER_CONTENT_TYPE,
- "image/gif;\n name=\"attachment1\"");
- attachment1Part.setHeader(MimeHeader.HEADER_CONTENT_TRANSFER_ENCODING, "base64");
- attachment1Part.setHeader(MimeHeader.HEADER_CONTENT_DISPOSITION,
- "attachment;\n filename=\"attachment1\";\n size=100");
- attachment1Part.setHeader(MimeHeader.HEADER_ANDROID_ATTACHMENT_STORE_DATA, "101");
-
- Part attachment2Part = MessageTestUtils.bodyPart("image/jpg", null);
- attachment2Part.setHeader(MimeHeader.HEADER_CONTENT_TYPE,
- "image/jpg;\n name=\"attachment2\"");
- attachment2Part.setHeader(MimeHeader.HEADER_CONTENT_TRANSFER_ENCODING, "base64");
- attachment2Part.setHeader(MimeHeader.HEADER_CONTENT_DISPOSITION,
- "attachment;\n filename=\"attachment2\";\n size=200");
- attachment2Part.setHeader(MimeHeader.HEADER_ANDROID_ATTACHMENT_STORE_DATA, "102");
-
- final Message legacyMessage = new MessageBuilder()
- .setBody(new MultipartBuilder("multipart/mixed")
- .addBodyPart(MessageTestUtils.bodyPart("text/html", null))
- .addBodyPart(new MultipartBuilder("multipart/mixed")
- .addBodyPart((BodyPart)attachment1Part)
- .addBodyPart((BodyPart)attachment2Part)
- .buildBodyPart())
- .build())
- .build();
+ final Message legacyMessage = prepareLegacyMessageWithAttachments(2);
// Now, convert from legacy to provider and see what happens
ArrayList<Part> viewables = new ArrayList<Part>();
@@ -244,9 +220,9 @@
while (c.moveToNext()) {
Attachment attachment = Attachment.getContent(c, Attachment.class);
if ("101".equals(attachment.mLocation)) {
- checkAttachment("attachment1Part", attachment1Part, attachment);
+ checkAttachment("attachment1Part", attachments.get(0), attachment);
} else if ("102".equals(attachment.mLocation)) {
- checkAttachment("attachment2Part", attachment2Part, attachment);
+ checkAttachment("attachment2Part", attachments.get(1), attachment);
} else {
fail("Unexpected attachment with location " + attachment.mLocation);
}
@@ -257,6 +233,93 @@
}
/**
+ * Test that attachments aren't re-added in the DB. This supports the "partial download"
+ * nature of POP messages.
+ */
+ public void testAddDuplicateAttachments() throws MessagingException, IOException {
+ // Prepare a local message to add the attachments to
+ final long accountId = 1;
+ final long mailboxId = 1;
+ final EmailContent.Message localMessage = ProviderTestUtils.setupMessage(
+ "local-message", accountId, mailboxId, false, true, mProviderContext);
+
+ // Prepare a legacy message with attachments
+ Message legacyMessage = prepareLegacyMessageWithAttachments(2);
+
+ // Now, convert from legacy to provider and see what happens
+ ArrayList<Part> viewables = new ArrayList<Part>();
+ ArrayList<Part> attachments = new ArrayList<Part>();
+ MimeUtility.collectParts(legacyMessage, viewables, attachments);
+ LegacyConversions.updateAttachments(mProviderContext, localMessage, attachments);
+
+ // Confirm two attachment objects created
+ Uri uri = ContentUris.withAppendedId(Attachment.MESSAGE_ID_URI, localMessage.mId);
+ assertEquals(2, EmailContent.count(mProviderContext, uri, null, null));
+
+ // Now add the attachments again and confirm there are still only two
+ LegacyConversions.updateAttachments(mProviderContext, localMessage, attachments);
+ assertEquals(2, EmailContent.count(mProviderContext, uri, null, null));
+
+ // Now add a 3rd & 4th attachment and make sure the total is 4, not 2 or 6
+ legacyMessage = prepareLegacyMessageWithAttachments(4);
+ viewables = new ArrayList<Part>();
+ attachments = new ArrayList<Part>();
+ MimeUtility.collectParts(legacyMessage, viewables, attachments);
+ LegacyConversions.updateAttachments(mProviderContext, localMessage, attachments);
+ assertEquals(4, EmailContent.count(mProviderContext, uri, null, null));
+ }
+
+ /**
+ * Prepare a legacy message with 1+ attachments
+ */
+ private static Message prepareLegacyMessageWithAttachments(int numAttachments)
+ throws MessagingException {
+ // First, build one or more attachment parts
+ MultipartBuilder mpBuilder = new MultipartBuilder("multipart/mixed");
+ for (int i = 1; i <= numAttachments; ++i) {
+ BodyPart attachmentPart = MessageTestUtils.bodyPart("image/jpg", null);
+
+ // name=attachmentN size=N00 location=10N
+ attachmentPart.setHeader(MimeHeader.HEADER_CONTENT_TYPE,
+ "image/jpg;\n name=\"attachment" + i + "\"");
+ attachmentPart.setHeader(MimeHeader.HEADER_CONTENT_TRANSFER_ENCODING, "base64");
+ attachmentPart.setHeader(MimeHeader.HEADER_CONTENT_DISPOSITION,
+ "attachment;\n filename=\"attachment2\";\n size=" + i + "00");
+ attachmentPart.setHeader(MimeHeader.HEADER_ANDROID_ATTACHMENT_STORE_DATA, "10" + i);
+
+ mpBuilder.addBodyPart(attachmentPart);
+ }
+
+ // Now build a message with them
+ final Message legacyMessage = new MessageBuilder()
+ .setBody(new MultipartBuilder("multipart/mixed")
+ .addBodyPart(MessageTestUtils.bodyPart("text/html", null))
+ .addBodyPart(mpBuilder.buildBodyPart())
+ .build())
+ .build();
+
+ return legacyMessage;
+ }
+
+ /**
+ * Test the stringInequal helper
+ */
+ public void testStringInequal() {
+ // Pairs that are "equal"
+ assertFalse(LegacyConversions.stringNotEqual(null, null));
+ assertFalse(LegacyConversions.stringNotEqual(null, ""));
+ assertFalse(LegacyConversions.stringNotEqual("", null));
+ assertFalse(LegacyConversions.stringNotEqual("", ""));
+ assertFalse(LegacyConversions.stringNotEqual("string-equal", "string-equal"));
+ // Pairs that are "inequal"
+ assertTrue(LegacyConversions.stringNotEqual(null, "string-inequal"));
+ assertTrue(LegacyConversions.stringNotEqual("", "string-inequal"));
+ assertTrue(LegacyConversions.stringNotEqual("string-inequal", null));
+ assertTrue(LegacyConversions.stringNotEqual("string-inequal", ""));
+ assertTrue(LegacyConversions.stringNotEqual("string-inequal-a", "string-inequal-b"));
+ }
+
+ /**
* Compare attachment that was converted from Part (expected) to Provider Attachment (actual)
*
* TODO content URI should only be set if we also saved a file
@@ -345,7 +408,7 @@
assertEquals(tag, expect.isSet(Flag.FLAGGED), actual.mFlagFavorite);
}
- /**
+ /**
* Check equality of a pair of converted messages
*/
private void checkLegacyMessage(String tag, EmailContent.Message expect, Message actual)
