From cbe4ae9291160877f6664289e3713d5ef792bca5 Mon Sep 17 00:00:00 2001 From: Makoto Onuki Date: Tue, 18 May 2010 17:22:28 -0700 Subject: [PATCH] More tests for IMAP, clean up, and a few bug fixes. - A few new tests in ImapStoreUnitTests. - Added TODOs to ImapStoreUnitTests (for mainly NO response handling) - Renamed ImapStore.releaseConnection to poolConnection. - Fixed a bug in getConnection where it'd return a closed connection. - Now getConnection() hanles BYE response for NOOP correctly and treat the connection as closed. Change-Id: I48e5b89049338f7d4f1ac77cd7ac7243945a9575 --- .../android/email/mail/store/ImapStore.java | 85 +++++++----- .../email/mail/store/ImapStoreUnitTests.java | 131 ++++++++++++++++-- 2 files changed, 174 insertions(+), 42 deletions(-) diff --git a/src/com/android/email/mail/store/ImapStore.java b/src/com/android/email/mail/store/ImapStore.java index 13e8f6d0f..5019cc90f 100644 --- a/src/com/android/email/mail/store/ImapStore.java +++ b/src/com/android/email/mail/store/ImapStore.java @@ -61,11 +61,13 @@ import java.nio.charset.Charset; import java.security.MessageDigest; import java.security.NoSuchAlgorithmException; import java.util.ArrayList; +import java.util.Collection; import java.util.Date; import java.util.HashMap; import java.util.LinkedHashSet; -import java.util.LinkedList; import java.util.List; +import java.util.concurrent.ConcurrentLinkedQueue; +import java.util.concurrent.atomic.AtomicInteger; import java.util.regex.Pattern; import javax.net.ssl.SSLException; @@ -101,8 +103,8 @@ public class ImapStore extends Store { private String mIdPhrase = null; private static String sImapId = null; - private final LinkedList mConnections = - new LinkedList(); + private final ConcurrentLinkedQueue mConnectionPool = + new ConcurrentLinkedQueue(); /** * Charset used for converting folder names to and from UTF-7 as defined by RFC 3501. @@ -118,6 +120,14 @@ public class ImapStore extends Store { */ private HashMap mFolderCache = new HashMap(); + /** + * Next tag to use. All connections associated to the same ImapStore instance share the same + * counter to make tests simpler. + * (Some of the tests involve multiple connections but only have a single counter to track the + * tag.) + */ + private final AtomicInteger mNextCommandTag = new AtomicInteger(0); + /** * Static named constructor. */ @@ -182,6 +192,10 @@ public class ImapStore extends Store { } } + /* package */ Collection getConnectionPoolForTest() { + return mConnectionPool; + } + /** * For testing only. Injects a different root transport (it will be copied using * newInstanceWithConfiguration() each time IMAP sets up a new channel). The transport @@ -389,7 +403,7 @@ public class ImapStore extends Store { connection.close(); throw new MessagingException("Unable to get folder list.", ioe); } finally { - releaseConnection(connection); + poolConnection(connection); } } @@ -406,30 +420,35 @@ public class ImapStore extends Store { } /** - * Gets a connection if one is available for reuse, or creates a new one if not. - * @return + * Gets a connection if one is available from the pool, or creates a new one if not. */ - private ImapConnection getConnection() throws MessagingException { - synchronized (mConnections) { - ImapConnection connection = null; - while ((connection = mConnections.poll()) != null) { - try { - connection.executeSimpleCommand("NOOP"); - break; - } - catch (IOException ioe) { - connection.close(); - } + /* package */ ImapConnection getConnection() { + ImapConnection connection = null; + while ((connection = mConnectionPool.poll()) != null) { + try { + connection.executeSimpleCommand("NOOP"); + break; + } catch (MessagingException e) { + // Fall through + } catch (IOException e) { + // Fall through } - if (connection == null) { - connection = new ImapConnection(); - } - return connection; + connection.close(); + connection = null; } + if (connection == null) { + connection = new ImapConnection(); + } + return connection; } - private void releaseConnection(ImapConnection connection) { - mConnections.offer(connection); + /** + * Save a {@link ImapConnection} in the pool for reuse. + */ + /* package */ void poolConnection(ImapConnection connection) { + if (connection != null) { + mConnectionPool.add(connection); + } } /* package */ static String encodeFolderName(String name) { @@ -559,7 +578,7 @@ public class ImapStore extends Store { // TODO implement expunge mMessageCount = -1; synchronized (this) { - releaseConnection(mConnection); + poolConnection(mConnection); mConnection = null; } } @@ -602,7 +621,7 @@ public class ImapStore extends Store { } finally { if (mConnection == null) { - releaseConnection(connection); + poolConnection(connection); } } } @@ -642,7 +661,7 @@ public class ImapStore extends Store { } finally { if (mConnection == null) { - releaseConnection(connection); + poolConnection(connection); } } } @@ -1326,7 +1345,6 @@ public class ImapStore extends Store { private static final String IMAP_DEDACTED_LOG = "[IMAP command redacted]"; private Transport mTransport; private ImapResponseParser mParser; - private int mNextCommandTag; /** # of command/response lines to log upon crash. */ private static final int DISCOURSE_LOGGER_SIZE = 64; private final DiscourseLogger mDiscourse = new DiscourseLogger(DISCOURSE_LOGGER_SIZE); @@ -1336,8 +1354,6 @@ public class ImapStore extends Store { return; } - mNextCommandTag = 1; - try { // copy configuration into a clean transport, if necessary if (mTransport == null) { @@ -1441,9 +1457,14 @@ public class ImapStore extends Store { // } if (mTransport != null) { mTransport.close(); + mTransport = null; } } + /* package */ boolean isTransportOpenForTest() { + return mTransport != null ? mTransport.isOpen() : false; + } + public ImapResponse readResponse() throws IOException, MessagingException { return mParser.readResponse(); } @@ -1459,7 +1480,7 @@ public class ImapStore extends Store { public String sendCommand(String command, boolean sensitive) throws MessagingException, IOException { open(); - String tag = Integer.toString(mNextCommandTag++); + String tag = Integer.toString(mNextCommandTag.incrementAndGet()); String commandToSend = tag + " " + command; mTransport.writeLine(commandToSend, sensitive ? IMAP_DEDACTED_LOG : null); mDiscourse.addSentCommand(sensitive ? IMAP_DEDACTED_LOG : commandToSend); @@ -1467,12 +1488,12 @@ public class ImapStore extends Store { } public List executeSimpleCommand(String command) throws IOException, - ImapException, MessagingException { + MessagingException { return executeSimpleCommand(command, false); } public List executeSimpleCommand(String command, boolean sensitive) - throws IOException, ImapException, MessagingException { + throws IOException, MessagingException { String tag = sendCommand(command, sensitive); ArrayList responses = new ArrayList(); ImapResponse response; diff --git a/tests/src/com/android/email/mail/store/ImapStoreUnitTests.java b/tests/src/com/android/email/mail/store/ImapStoreUnitTests.java index d9f72ca32..c6d260c9e 100644 --- a/tests/src/com/android/email/mail/store/ImapStoreUnitTests.java +++ b/tests/src/com/android/email/mail/store/ImapStoreUnitTests.java @@ -19,6 +19,7 @@ package com.android.email.mail.store; import com.android.email.Email; import com.android.email.Utility; import com.android.email.mail.Address; +import com.android.email.mail.AuthenticationFailedException; import com.android.email.mail.Body; import com.android.email.mail.FetchProfile; import com.android.email.mail.Flag; @@ -34,6 +35,7 @@ import com.android.email.mail.internet.MimeBodyPart; import com.android.email.mail.internet.MimeMultipart; import com.android.email.mail.internet.MimeUtility; import com.android.email.mail.internet.TextBody; +import com.android.email.mail.store.ImapStore.ImapConnection; import com.android.email.mail.store.ImapStore.ImapMessage; import com.android.email.mail.transport.MockTransport; @@ -54,6 +56,8 @@ import java.util.HashMap; * $ runtest -c com.android.email.mail.store.ImapStoreUnitTests email * * TODO Check if callback is really called + * TODO test for BAD response in various places? + * TODO test for BYE response in various places? */ @SmallTest public class ImapStoreUnitTests extends AndroidTestCase { @@ -87,6 +91,7 @@ public class ImapStoreUnitTests extends AndroidTestCase { mStore = (ImapStore) ImapStore.newInstance("imap://user:password@server:999", getContext(), null); mFolder = (ImapStore.ImapFolder) mStore.getFolder(FOLDER_NAME); + mNextTag = 1; } /** @@ -103,6 +108,17 @@ public class ImapStoreUnitTests extends AndroidTestCase { // TODO: inject specific facts in the initial folder SELECT and check them here } + public void testLoginFailure() throws Exception { + MockTransport mockTransport = openAndInjectMockTransport(); + expectLogin(mockTransport, new String[] {"* ID NIL", "OK"}, "NO authentication failed"); + + try { + mStore.getConnection().open(); + fail("Didn't throw AuthenticationFailedException"); + } catch (AuthenticationFailedException expected) { + } + } + /** * TODO: Test with SSL negotiation (faked) * TODO: Test with SSL required but not supported @@ -384,19 +400,29 @@ public class ImapStoreUnitTests extends AndroidTestCase { } private void expectLogin(MockTransport mockTransport, String[] imapIdResponse) { - // Fix the tag # of the ID response - String last = imapIdResponse[imapIdResponse.length-1]; - last = "2 " + last; - imapIdResponse[imapIdResponse.length-1] = last; + expectLogin(mockTransport, imapIdResponse, "OK user authenticated (Success)"); + } + + private void expectLogin(MockTransport mockTransport, String[] imapIdResponse, + String loginResponse) { // inject boilerplate commands that match our typical login mockTransport.expect(null, "* OK Imap 2000 Ready To Assist You"); - mockTransport.expect("1 CAPABILITY", new String[] { + + mockTransport.expect(getNextTag(false) + " CAPABILITY", new String[] { "* CAPABILITY IMAP4rev1 STARTTLS AUTH=GSSAPI LOGINDISABLED", - "1 OK CAPABILITY completed"}); - mockTransport.expect("2 ID \\(.*\\)", imapIdResponse); - mockTransport.expect("3 LOGIN user \"password\"", - "3 OK user authenticated (Success)"); - mNextTag = 4; + getNextTag(true) + " OK CAPABILITY completed"}); + + String expectedNextTag = getNextTag(false); + // Fix the tag # of the ID response + String last = imapIdResponse[imapIdResponse.length-1]; + last = expectedNextTag + " " + last; + imapIdResponse[imapIdResponse.length-1] = last; + mockTransport.expect(getNextTag(false) + " ID \\(.*\\)", imapIdResponse); + + getNextTag(true); // Advance the tag for ID response. + + mockTransport.expect(getNextTag(false) + " LOGIN user \"password\"", + getNextTag(true) + " " + loginResponse); } /** @@ -500,6 +526,8 @@ public class ImapStoreUnitTests extends AndroidTestCase { assertEquals("multipart/mixed; boundary=a00000000000000000000000000b", message.getHeader("Content-Type")[0]); assertTrue(message.isSet(Flag.SEEN)); + + // TODO: Test NO response. } public void testFetchBodyStructure() throws MessagingException { @@ -595,6 +623,8 @@ public class ImapStoreUnitTests extends AndroidTestCase { // TODO Test for quote: If a filename contains ", it should become %22, // which isn't implemented. + + // TODO: Test NO response. } public void testFetchBodySane() throws MessagingException { @@ -616,6 +646,8 @@ public class ImapStoreUnitTests extends AndroidTestCase { }); mFolder.fetch(new Message[] { message }, fp, null); assertEquals("a@b.com", message.getHeader("from")[0]); + + // TODO: Test NO response. } public void testFetchBody() throws MessagingException { @@ -637,6 +669,8 @@ public class ImapStoreUnitTests extends AndroidTestCase { }); mFolder.fetch(new Message[] { message }, fp, null); assertEquals("a@b.com", message.getHeader("from")[0]); + + // TODO: Test NO response. } public void testFetchAttachment() throws Exception { @@ -682,6 +716,8 @@ public class ImapStoreUnitTests extends AndroidTestCase { assertEquals("abc", Utility.fromUtf8(IOUtils.toByteArray(mimePart1.getBody().getInputStream()))); + + // TODO: Test NO response. } /** @@ -808,6 +844,9 @@ public class ImapStoreUnitTests extends AndroidTestCase { assertEquals("13", message.getUid()); assertEquals(7, mFolder.getMessageCount()); + + // TODO: Test append failure + // TODO: Test OK without APPENDUID } public void testGetPersonalNamespaces() throws Exception { @@ -834,6 +873,7 @@ public class ImapStoreUnitTests extends AndroidTestCase { ); // TODO test with path prefix + // TODO: Test NO response. } public void testEncodeFolderName() { @@ -852,6 +892,8 @@ public class ImapStoreUnitTests extends AndroidTestCase { assertEquals("!\u65E5\u672C\u8A9E!", ImapStore.decodeFolderName("!&ZeVnLIqe-!")); } + // TODO test folder open failure + public void testExists() throws Exception { MockTransport mock = openAndInjectMockTransport(); expectLogin(mock, new String[] {"* ID NIL", "OK"}); @@ -930,6 +972,8 @@ public class ImapStoreUnitTests extends AndroidTestCase { }); mFolder.copyMessages(messages, folderTo, null); + + // TODO: Test NO response. (src message not found) } public void testGetUnreadMessageCount() throws Exception { @@ -957,6 +1001,8 @@ public class ImapStoreUnitTests extends AndroidTestCase { }); mFolder.expunge(); + + // TODO: Test NO response. (permission denied) } public void testSetFlags() throws Exception { @@ -984,6 +1030,8 @@ public class ImapStoreUnitTests extends AndroidTestCase { getNextTag(true) + " OK success" }); mFolder.setFlags(messages, new Flag[] {Flag.DELETED}, false); + + // TODO: Test NO response. (src message not found) } public void testGetMessage() throws Exception { @@ -1078,4 +1126,67 @@ public class ImapStoreUnitTests extends AndroidTestCase { } MoreAsserts.assertEquals(expectedUids, list.toArray(new String[0]) ); } + + /** + * Test for {@link ImapStore#getConnection} and {@link ImapStore#keepConnectionForReuse} + */ + public void testGetConnection() throws Exception { + MockTransport mock = openAndInjectMockTransport(); + + // Start: No pooled connections. + assertEquals(0, mStore.getConnectionPoolForTest().size()); + + // Get 1st connection. + final ImapConnection con1 = mStore.getConnection(); + assertNotNull(con1); + assertEquals(0, mStore.getConnectionPoolForTest().size()); // Pool size not changed. + assertFalse(con1.isTransportOpenForTest()); // Transport not open yet. + + // Open con1 + expectLogin(mock, new String[] {"* ID NIL", "OK"}); + con1.open(); + assertTrue(con1.isTransportOpenForTest()); + + // Get 2nd connection. + final ImapConnection con2 = mStore.getConnection(); + assertNotNull(con2); + assertEquals(0, mStore.getConnectionPoolForTest().size()); // Pool size not changed. + assertFalse(con2.isTransportOpenForTest()); // Transport not open yet. + + // con1 != con2 + assertNotSame(con1, con2); + + // Open con2 + expectLogin(mock, new String[] {"* ID NIL", "OK"}); + con2.open(); + assertTrue(con1.isTransportOpenForTest()); + + // Now we have two open connections: con1 and con2 + + // Save con1 in the pool. + mStore.poolConnection(con1); + assertEquals(1, mStore.getConnectionPoolForTest().size()); + + // Get another connection. Should get con1, after verifying the connection. + mock.expect(getNextTag(false) + " NOOP", new String[] {getNextTag(true) + " OK success"}); + + final ImapConnection con1b = mStore.getConnection(); + assertEquals(0, mStore.getConnectionPoolForTest().size()); // No connections left in pool + assertSame(con1, con1b); + assertTrue(con1.isTransportOpenForTest()); // We opened it. + + // Save con2. + mStore.poolConnection(con2); + assertEquals(1, mStore.getConnectionPoolForTest().size()); + + // Try to get connection, but this time, connection gets closed. + mock.expect(getNextTag(false) + " NOOP", new String[] {getNextTag(true) + "* BYE bye"}); + final ImapConnection con3 = mStore.getConnection(); + assertNotNull(con3); + assertEquals(0, mStore.getConnectionPoolForTest().size()); // No connections left in pool + + // It should be a new connection. + assertNotSame(con1, con3); + assertNotSame(con2, con3); + } }