From 5b0a12c199198870161876996296b1262c17408e Mon Sep 17 00:00:00 2001 From: Andrew Stadler Date: Wed, 14 Oct 2009 08:20:59 -0700 Subject: [PATCH] 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 Fixes bug http://b/2084704 --- src/com/android/email/LegacyConversions.java | 63 +++++++++- .../android/email/provider/EmailContent.java | 6 + .../android/email/LegacyConversionsTests.java | 119 +++++++++++++----- 3 files changed, 157 insertions(+), 31 deletions(-) diff --git a/src/com/android/email/LegacyConversions.java b/src/com/android/email/LegacyConversions.java index cd39aeb26..4c1ccf855 100644 --- a/src/com/android/email/LegacyConversions.java +++ b/src/com/android/email/LegacyConversions.java @@ -38,6 +38,7 @@ import org.apache.commons.io.IOUtils; 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 @@ import java.util.Date; 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 @@ public class LegacyConversions { /** * 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 @@ public class LegacyConversions { 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); @@ -314,6 +360,17 @@ public class LegacyConversions { localMessage.mFlagAttachment = true; } + /** + * 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. */ diff --git a/src/com/android/email/provider/EmailContent.java b/src/com/android/email/provider/EmailContent.java index 65f677586..62c7c537e 100644 --- a/src/com/android/email/provider/EmailContent.java +++ b/src/com/android/email/provider/EmailContent.java @@ -1657,6 +1657,12 @@ public abstract class EmailContent { 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 91674cc96..3fceb32b3 100644 --- a/tests/src/com/android/email/LegacyConversionsTests.java +++ b/tests/src/com/android/email/LegacyConversionsTests.java @@ -203,31 +203,7 @@ public class LegacyConversionsTests extends ProviderTestCase2 { "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 viewables = new ArrayList(); @@ -244,9 +220,9 @@ public class LegacyConversionsTests extends ProviderTestCase2 { 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); } @@ -256,6 +232,93 @@ public class LegacyConversionsTests extends ProviderTestCase2 { } } + /** + * 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 viewables = new ArrayList(); + ArrayList attachments = new ArrayList(); + 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(); + attachments = new ArrayList(); + 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) * @@ -345,7 +408,7 @@ public class LegacyConversionsTests extends ProviderTestCase2 { 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)