am 196f6187: Merge change I036f39c6 into eclair-mr2

Merge commit '196f6187bb25c7ed1b2f6f5f3178152294b3cc22' into eclair-mr2-plus-aosp

* commit '196f6187bb25c7ed1b2f6f5f3178152294b3cc22':
  Prevent duplication of POP3 attachments

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,
+     * This will skip adding attachments if they are already found in the attachments table.
-     * change it to find existing attachments and delete/merge them.
+     * 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
+        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.
      */

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 {

tests/src/com/android/email/LegacyConversionsTests.java

@@ -203,31 +203,7 @@ public class LegacyConversionsTests extends ProviderTestCase2<EmailProvider> {
                 "local-message", accountId, mailboxId, false, true, mProviderContext);
 
         // Prepare a legacy message with attachments
-        Part attachment1Part = MessageTestUtils.bodyPart("image/gif", null);
+        final Message legacyMessage = prepareLegacyMessageWithAttachments(2);
-        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();
 
         // Now, convert from legacy to provider and see what happens
         ArrayList<Part> viewables = new ArrayList<Part>();
@@ -244,9 +220,9 @@ public class LegacyConversionsTests extends ProviderTestCase2<EmailProvider> {
             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<EmailProvider> {
         }
     }
 
+    /**
+     * 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)
      *