From 9ef6f645f57d869a600113f555389b5d5e368c21 Mon Sep 17 00:00:00 2001
From: Mihai Preda <preda@google.com>
Date: Sat, 17 Oct 2009 17:37:39 +0200
Subject: [PATCH] Controller: modify the test for "attachment already loaded".

Bug 2192510.

update unit test.
---
 src/com/android/email/Controller.java         |  6 +-
 .../android/email/MessagingController.java    | 27 ++++++---
 .../email/provider/AttachmentProvider.java    |  8 ++-
 .../provider/AttachmentProviderTests.java     | 58 ++++++++++++-------
 4 files changed, 68 insertions(+), 31 deletions(-)

diff --git a/src/com/android/email/Controller.java b/src/com/android/email/Controller.java
index d4e2cd7a8..cd55a0d88 100644
--- a/src/com/android/email/Controller.java
+++ b/src/com/android/email/Controller.java
@@ -699,7 +699,9 @@ public class Controller {
 
         File saveToFile = AttachmentProvider.getAttachmentFilename(mProviderContext,
                 accountId, attachmentId);
-        if (saveToFile.exists()) {
+        Attachment attachInfo = Attachment.restoreAttachmentWithId(mProviderContext, attachmentId);
+
+        if (saveToFile.exists() && attachInfo.mContentUri != null) {
             // The attachment has already been downloaded, so we will just "pretend" to download it
             synchronized (mListeners) {
                 for (Result listener : mListeners) {
@@ -712,8 +714,6 @@ public class Controller {
             return;
         }
 
-        Attachment attachInfo = Attachment.restoreAttachmentWithId(mProviderContext, attachmentId);
-
         // Split here for target type (Service or MessagingController)
         IEmailService service = getServiceForMessage(messageId);
         if (service != null) {
diff --git a/src/com/android/email/MessagingController.java b/src/com/android/email/MessagingController.java
index 6273f8ede..4b62539a7 100644
--- a/src/com/android/email/MessagingController.java
+++ b/src/com/android/email/MessagingController.java
@@ -1837,9 +1837,20 @@ public class MessagingController implements Runnable {
         put("loadAttachment", listener, new Runnable() {
             public void run() {
                 try {
-                    // 1.  Pruning.  Policy is to have one downloaded attachment at a time,
-                    // per account, to reduce disk storage pressure.
-                    pruneCachedAttachments(accountId);
+                    //1. Check if the attachment is already here and return early in that case
+                    File saveToFile = AttachmentProvider.getAttachmentFilename(mContext, accountId,
+                            attachmentId);
+                    Attachment attachment =
+                        Attachment.restoreAttachmentWithId(mContext, attachmentId);
+                    if (attachment == null) {
+                        mListeners.loadAttachmentFailed(accountId, messageId, attachmentId,
+                                "Attachment is null");
+                        return;
+                    }
+                    if (saveToFile.exists() && attachment.mContentUri != null) {
+                        mListeners.loadAttachmentFinished(accountId, messageId, attachmentId);
+                        return;
+                    }
 
                     // 2. Open the remote folder.
                     // TODO all of these could be narrower projections
@@ -1849,15 +1860,17 @@ public class MessagingController implements Runnable {
                         EmailContent.Mailbox.restoreMailboxWithId(mContext, mailboxId);
                     EmailContent.Message message =
                         EmailContent.Message.restoreMessageWithId(mContext, messageId);
-                    Attachment attachment =
-                        Attachment.restoreAttachmentWithId(mContext, attachmentId);
-                    if (account == null || mailbox == null || message == null
-                            || attachment == null) {
+
+                    if (account == null || mailbox == null || message == null) {
                         mListeners.loadAttachmentFailed(accountId, messageId, attachmentId,
                                 "Account, mailbox, message or attachment are null");
                         return;
                     }
 
+                    // Pruning.  Policy is to have one downloaded attachment at a time,
+                    // per account, to reduce disk storage pressure.
+                    pruneCachedAttachments(accountId);
+
                     Store remoteStore =
                         Store.getInstance(account.getStoreUri(mContext), mContext, null);
                     Folder remoteFolder = remoteStore.getFolder(mailbox.mDisplayName);
diff --git a/src/com/android/email/provider/AttachmentProvider.java b/src/com/android/email/provider/AttachmentProvider.java
index c38110fc6..e8221c4b0 100644
--- a/src/com/android/email/provider/AttachmentProvider.java
+++ b/src/com/android/email/provider/AttachmentProvider.java
@@ -16,6 +16,7 @@
 
 package com.android.email.provider;
 
+import com.android.email.Email;
 import com.android.email.mail.internet.MimeUtility;
 import com.android.email.provider.EmailContent.Attachment;
 import com.android.email.provider.EmailContent.AttachmentColumns;
@@ -345,7 +346,12 @@ public class AttachmentProvider extends ContentProvider {
         if (c != null) {
             try {
                 if (c.moveToFirst()) {
-                    return Uri.parse(c.getString(0));
+                    final String strUri = c.getString(0);
+                    if (strUri != null) {
+                        return Uri.parse(strUri);
+                    } else {
+                        Email.log("AttachmentProvider: attachment with null contentUri");
+                    }
                 }
             } finally {
                 c.close();
diff --git a/tests/src/com/android/email/provider/AttachmentProviderTests.java b/tests/src/com/android/email/provider/AttachmentProviderTests.java
index 67e8061b6..f566a5648 100644
--- a/tests/src/com/android/email/provider/AttachmentProviderTests.java
+++ b/tests/src/com/android/email/provider/AttachmentProviderTests.java
@@ -373,47 +373,65 @@ public class AttachmentProviderTests extends ProviderTestCase2<AttachmentProvide
         afd.close();
     }
 
+    private Uri createAttachment(Account account, long messageId, String contentUriStr) {
+        // Add an attachment entry.
+        Attachment newAttachment = ProviderTestUtils.setupAttachment(messageId, "file", 100,
+                false, mMockContext);
+        newAttachment.mContentUri = contentUriStr;
+        long attachmentId = addAttachmentToDb(account, newAttachment);
+        Uri attachmentUri = AttachmentProvider.getAttachmentUri(account.mId, attachmentId);
+        return attachmentUri;
+    }
+
     /**
      * test resolveAttachmentIdToContentUri()
+     *  - without DB
      *  - not in DB
-     *  - in DB
+     *  - in DB, with not-null contentUri
+     *  - in DB, with null contentUri
      */
     public void testResolveAttachmentIdToContentUri() throws MessagingException {
         Account account1 = ProviderTestUtils.setupAccount("attachment-query", false, mMockContext);
         account1.mCompatibilityUuid = "test-UUID";
         account1.save(mMockContext);
         final long message1Id = 1;
-        long attachment1Id = 1;
-
-        // Note:  There is an implicit assumption in this test sequence that the first
-        // attachment we add will be id=1 and the 2nd will have id=2.  This could fail on
-        // a legitimate implementation.  Asserts below will catch this and fail the test
-        // if necessary.
-        Uri attachment1Uri = AttachmentProvider.getAttachmentUri(account1.mId, attachment1Id);
+        // We use attachmentId == 1 but any other id would do
+        final long attachment1Id = 1;
+        final Uri attachment1Uri = AttachmentProvider.getAttachmentUri(account1.mId, attachment1Id);
 
         // Test with no attached database - should return input
         Uri result = AttachmentProvider.resolveAttachmentIdToContentUri(
-                    mMockResolver, attachment1Uri);
+                mMockResolver, attachment1Uri);
         assertEquals(attachment1Uri, result);
 
+        setupAttachmentDatabase(account1);
+
         // Test with an attached database, but no attachment found - should return input
-        setupAttachmentDatabase(account1);
+        // We know that the attachmentId 1 does not exist because there are no attachments
+        // created at this point
         result = AttachmentProvider.resolveAttachmentIdToContentUri(
                 mMockResolver, attachment1Uri);
         assertEquals(attachment1Uri, result);
 
-        // Add an attachment entry.  Note, resolveAttachmentIdToContentUri() just uses
+        // Test with existing attachement and contentUri != null
+        // Note, resolveAttachmentIdToContentUri() just uses
         // the DB, and does not sample the files, so we won't bother creating the files
-        Attachment newAttachment1 = ProviderTestUtils.setupAttachment(message1Id, "file1", 100,
-                false, mMockContext);
-        newAttachment1.mContentUri = "file:///path/to/file";
-        attachment1Id = addAttachmentToDb(account1, newAttachment1);
-        assertEquals("Broken test:  Unexpected id assignment", 1, attachment1Id);
+        {
+            Uri attachmentUri = createAttachment(account1, message1Id, "file:///path/to/file");
+            Uri contentUri = AttachmentProvider.resolveAttachmentIdToContentUri(mMockResolver, 
+                    attachmentUri);
+            // When the attachment is found, return the stored content_uri value
+            assertEquals("file:///path/to/file", contentUri.toString());
+        }
 
-        // When the attachment is found, return the stored content_uri value
-        result = AttachmentProvider.resolveAttachmentIdToContentUri(
-                mMockResolver, attachment1Uri);
-        assertEquals("file:///path/to/file", result.toString());
+        // Test with existing attachement and contentUri == null
+        {
+            Uri attachmentUri = createAttachment(account1, message1Id, null);
+            Uri contentUri = AttachmentProvider.resolveAttachmentIdToContentUri(mMockResolver, 
+                    attachmentUri);
+            // When contentUri is null should return input
+            assertEquals(attachmentUri, contentUri);
+        }
     }
 
     /**