From eb7752bf695b2a93854e0bb89ddbbc2236bb9aea Mon Sep 17 00:00:00 2001 From: Andrew Stadler Date: Tue, 6 Oct 2009 14:20:09 -0700 Subject: [PATCH] Fix back-to-back message-id bugs * MessageCompose now adds message-id to new messages (it was previously done on its behalf by MimeMessage). * LegacyConversions.updateMessageFields() now handles missing message-id without error. * Unit tests for the LegacyConversions change These two issues were combining with a failure of comcast's SMTP server to insert message-id headers, to prevent delivery of a message between any two comcast accounts using this client. Bug # http://b/issue?id=2161478 --- src/com/android/email/LegacyConversions.java | 12 +- src/com/android/email/Utility.java | 16 +++ .../email/activity/MessageCompose.java | 4 + .../android/email/LegacyConversionsTests.java | 111 +++++++++++++++++- 4 files changed, 141 insertions(+), 2 deletions(-) diff --git a/src/com/android/email/LegacyConversions.java b/src/com/android/email/LegacyConversions.java index 6e2931bb0..b03364b46 100644 --- a/src/com/android/email/LegacyConversions.java +++ b/src/com/android/email/LegacyConversions.java @@ -104,7 +104,17 @@ public class LegacyConversions { localMessage.mServerTimeStamp = internalDate.getTime(); } // public String mClientId; - localMessage.mMessageId = ((MimeMessage)message).getMessageId(); + + // Absorb a MessagingException here in the case of messages that were delivered without + // a proper message-id. This is seen in some ISP's but it is non-fatal -- (we'll just use + // the locally-generated message-id.) + try { + localMessage.mMessageId = ((MimeMessage)message).getMessageId(); + } catch (MessagingException me) { + if (Email.DEBUG) { + Log.d(Email.LOG_TAG, "Missing message-id for UID=" + localMessage.mServerId); + } + } // public long mBodyKey; localMessage.mMailboxKey = mailboxId; diff --git a/src/com/android/email/Utility.java b/src/com/android/email/Utility.java index baba44e54..6e6012aa5 100644 --- a/src/com/android/email/Utility.java +++ b/src/com/android/email/Utility.java @@ -406,4 +406,20 @@ public class Utility { return null; } + + /** + * Generate a random message-id header for locally-generated messages. + */ + public static String generateMessageId() { + StringBuffer sb = new StringBuffer(); + sb.append("<"); + for (int i = 0; i < 24; i++) { + sb.append(Integer.toString((int)(Math.random() * 35), 36)); + } + sb.append("."); + sb.append(Long.toString(System.currentTimeMillis())); + sb.append("@email.android.com>"); + return sb.toString(); + } + } diff --git a/src/com/android/email/activity/MessageCompose.java b/src/com/android/email/activity/MessageCompose.java index 18d121589..b3906cc65 100644 --- a/src/com/android/email/activity/MessageCompose.java +++ b/src/com/android/email/activity/MessageCompose.java @@ -21,6 +21,7 @@ import com.android.email.Email; import com.android.email.EmailAddressAdapter; import com.android.email.EmailAddressValidator; import com.android.email.R; +import com.android.email.Utility; import com.android.email.mail.Address; import com.android.email.mail.MessagingException; import com.android.email.mail.internet.EmailHtmlUtil; @@ -703,6 +704,9 @@ public class MessageCompose extends Activity implements OnClickListener, OnFocus * @param bodyText the body text. */ private void updateMessage(Message message, Account account, boolean hasAttachments) { + if (message.mMessageId == null || message.mMessageId.length() == 0) { + message.mMessageId = Utility.generateMessageId(); + } message.mTimeStamp = System.currentTimeMillis(); message.mFrom = new Address(account.getEmailAddress(), account.getSenderName()).pack(); message.mTo = getPackedAddresses(mToView); diff --git a/tests/src/com/android/email/LegacyConversionsTests.java b/tests/src/com/android/email/LegacyConversionsTests.java index e22343f3d..49aaf4fee 100644 --- a/tests/src/com/android/email/LegacyConversionsTests.java +++ b/tests/src/com/android/email/LegacyConversionsTests.java @@ -29,6 +29,7 @@ import com.android.email.mail.MessageTestUtils.MultipartBuilder; import com.android.email.mail.internet.MimeHeader; import com.android.email.mail.internet.MimeMessage; import com.android.email.mail.internet.MimeUtility; +import com.android.email.mail.internet.TextBody; import com.android.email.provider.EmailContent; import com.android.email.provider.EmailProvider; import com.android.email.provider.ProviderTestUtils; @@ -42,6 +43,7 @@ import android.test.ProviderTestCase2; import java.io.IOException; import java.util.ArrayList; +import java.util.Date; /** * Tests of the Legacy Conversions code (used by MessagingController). @@ -55,6 +57,17 @@ import java.util.ArrayList; */ public class LegacyConversionsTests extends ProviderTestCase2 { + private static final String UID = "UID.12345678"; + private static final String SENDER = "sender@android.com"; + private static final String RECIPIENT_TO = "recipient-to@android.com"; + private static final String RECIPIENT_CC = "recipient-cc@android.com"; + private static final String RECIPIENT_BCC = "recipient-bcc@android.com"; + private static final String REPLY_TO = "reply-to@android.com"; + private static final String SUBJECT = "This is the subject"; + private static final String BODY = "This is the body. This is also the body."; + private static final String MESSAGE_ID = "Test-Message-ID"; + private static final String MESSAGE_ID_2 = "Test-Message-ID-Second"; + EmailProvider mProvider; Context mProviderContext; Context mContext; @@ -81,6 +94,83 @@ public class LegacyConversionsTests extends ProviderTestCase2 { * TODO: rainy day tests of all kinds */ + /** + * Test basic conversion from Store message to Provider message + * + * TODO: Not a complete test of all fields, and some fields need special tests (e.g. flags) + * TODO: There are many special cases in the tested function, that need to be + * tested here as well. + */ + public void testUpdateMessageFields() throws MessagingException { + MimeMessage message = buildTestMessage(RECIPIENT_TO, RECIPIENT_CC, RECIPIENT_BCC, + REPLY_TO, SENDER, SUBJECT, null); + EmailContent.Message localMessage = new EmailContent.Message(); + + boolean result = LegacyConversions.updateMessageFields(localMessage, message, 1, 1); + assertTrue(result); + checkProviderMessage("testUpdateMessageFields", message, localMessage); + } + + /** + * Test basic conversion from Store message to Provider message, when the provider message + * does not have a proper message-id. + */ + public void testUpdateMessageFieldsNoMessageId() throws MessagingException { + MimeMessage message = buildTestMessage(RECIPIENT_TO, RECIPIENT_CC, RECIPIENT_BCC, + REPLY_TO, SENDER, SUBJECT, null); + EmailContent.Message localMessage = new EmailContent.Message(); + + // If the source message-id is null, the target should be left as-is + localMessage.mMessageId = MESSAGE_ID_2; + message.removeHeader("Message-ID"); + + boolean result = LegacyConversions.updateMessageFields(localMessage, message, 1, 1); + assertTrue(result); + assertEquals(MESSAGE_ID_2, localMessage.mMessageId); + } + + /** + * Build a lightweight Store message with simple field population + */ + private MimeMessage buildTestMessage(String to, String cc, String bcc, String replyTo, + String sender, String subject, String content) throws MessagingException { + MimeMessage message = new MimeMessage(); + + if (to != null) { + Address[] addresses = Address.parse(to); + message.setRecipients(RecipientType.TO, addresses); + } + if (cc != null) { + Address[] addresses = Address.parse(cc); + message.setRecipients(RecipientType.CC, addresses); + } + if (bcc != null) { + Address[] addresses = Address.parse(bcc); + message.setRecipients(RecipientType.BCC, addresses); + } + if (replyTo != null) { + Address[] addresses = Address.parse(replyTo); + message.setReplyTo(addresses); + } + if (sender != null) { + Address[] addresses = Address.parse(sender); + message.setFrom(Address.parse(sender)[0]); + } + if (subject != null) { + message.setSubject(subject); + } + if (content != null) { + TextBody body = new TextBody(content); + message.setBody(body); + } + + message.setUid(UID); + message.setSentDate(new Date()); + message.setInternalDate(new Date()); + message.setMessageId(MESSAGE_ID); + return message; + } + /** * Sunny day test of adding attachments from an IMAP message. */ @@ -219,16 +309,35 @@ public class LegacyConversionsTests extends ProviderTestCase2 { } /** - * Check equality of a pair of converted message + * Check equality of a pair of converted messages + */ + private void checkProviderMessage(String tag, Message expect, EmailContent.Message actual) + throws MessagingException { + assertEquals(tag, expect.getUid(), actual.mServerId); + assertEquals(tag, expect.getSubject(), actual.mSubject); + assertEquals(tag, Address.pack(expect.getFrom()), actual.mFrom); + assertEquals(tag, expect.getSentDate().getTime(), actual.mTimeStamp); + assertEquals(tag, Address.pack(expect.getRecipients(RecipientType.TO)), actual.mTo); + assertEquals(tag, Address.pack(expect.getRecipients(RecipientType.CC)), actual.mCc); + assertEquals(tag, ((MimeMessage)expect).getMessageId(), actual.mMessageId); + assertEquals(tag, expect.isSet(Flag.SEEN), actual.mFlagRead); + 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) throws MessagingException { assertEquals(tag, expect.mServerId, actual.getUid()); + assertEquals(tag, expect.mServerTimeStamp, actual.getInternalDate().getTime()); assertEquals(tag, expect.mSubject, actual.getSubject()); assertEquals(tag, expect.mFrom, Address.pack(actual.getFrom())); assertEquals(tag, expect.mTimeStamp, actual.getSentDate().getTime()); assertEquals(tag, expect.mTo, Address.pack(actual.getRecipients(RecipientType.TO))); assertEquals(tag, expect.mCc, Address.pack(actual.getRecipients(RecipientType.CC))); + assertEquals(tag, expect.mBcc, Address.pack(actual.getRecipients(RecipientType.BCC))); + assertEquals(tag, expect.mReplyTo, Address.pack(actual.getReplyTo())); assertEquals(tag, expect.mMessageId, ((MimeMessage)actual).getMessageId()); // check flags assertEquals(tag, expect.mFlagRead, actual.isSet(Flag.SEEN));