From 29d36a493a3e20bcbf4deb849d5adb27f44fa6c5 Mon Sep 17 00:00:00 2001 From: Alex Lovell-Troy Date: Wed, 28 May 2014 20:39:13 -0600 Subject: [PATCH 1/5] Adding ability to limit messages by max size Adding a new setting for maximum message size Adding method to limit a set of message uids by size Switching to IMAP uids so that we can trust that subsequent commands will use the same ids --- django_mailbox/transports/imap.py | 59 +++++++++++++++++++++++++++---- 1 file changed, 52 insertions(+), 7 deletions(-) diff --git a/django_mailbox/transports/imap.py b/django_mailbox/transports/imap.py index 078392e..570788a 100644 --- a/django_mailbox/transports/imap.py +++ b/django_mailbox/transports/imap.py @@ -2,12 +2,20 @@ from imaplib import IMAP4, IMAP4_SSL from .base import EmailTransport, MessageParseError +from django.conf import settings + +MAX_MESSAGE_SIZE = getattr( + settings, + 'DJANGO_MAILBOX_MAX_MESSAGE_SIZE', + False +) class ImapTransport(EmailTransport): def __init__(self, hostname, port=None, ssl=False, archive=''): self.hostname = hostname self.port = port self.archive = archive + self.MAX_MSG_SIZE = MAX_MESSAGE_SIZE if ssl: self.transport = IMAP4_SSL if not self.port: @@ -22,10 +30,47 @@ class ImapTransport(EmailTransport): typ, msg = self.server.login(username, password) self.server.select() - def get_message(self): - typ, inbox = self.server.search(None, 'ALL') - if not inbox[0]: + def _get_all_message_ids(self): + # Fetch all the message uids + response, message_ids = self.server.uid('search', None, 'ALL', ) + return message_ids[0].split(' ') + + def _get_small_message_ids(self, message_ids): + # Using existing message uids, get the sizes and + # return only those that are under the size + # limit + safe_message_ids = [] + + status, data = self.server.uid( + 'fetch', + ','.join(message_ids), + '(BODY.PEEK[HEADER] RFC822.SIZE BODYSTRUCTURE)' + ) + + for each_msg in data: + if isinstance(each_msg, tuple): + try: + metadata, structure = each_msg[0].split(' BODYSTRUCTURE ') + uid = metadata.split('(')[1].split(' ')[1] + size = metadata.split('(')[1].split(' ')[3] + if int(size) <= int(self.MAX_MSG_SIZE): + safe_message_ids.append(uid) + except ValueError, e: + print "ValueError: %s working on %s" % (e, each_msg[0]) + print each_msg + pass + return safe_message_ids + + def get_message(self): + message_ids = self._get_all_message_ids() + + # Limit the uids to the small ones if we care about that + if self.MAX_MSG_SIZE: + message_ids = self._get_small_message_ids(message_ids) + + + if not message_ids: return if self.archive: @@ -34,17 +79,17 @@ class ImapTransport(EmailTransport): # If the archive folder does not exist, create it self.server.create(self.archive) - for key in inbox[0].split(): + for uid in message_ids: try: - typ, msg_contents = self.server.fetch(key, '(RFC822)') + typ, msg_contents = self.server.uid('fetch', uid, '(RFC822)') message = self.get_email_from_bytes(msg_contents[0][1]) yield message except MessageParseError: continue if self.archive: - self.server.copy(key, self.archive) + self.server.uid('copy', uid, self.archive) - self.server.store(key, "+FLAGS", "\\Deleted") + self.server.uid('store', uid, "+FLAGS", "\\Deleted") self.server.expunge() return From 58550c76c933b797e0487192d98ded1d7d583cb4 Mon Sep 17 00:00:00 2001 From: Alex Lovell-Troy Date: Wed, 28 May 2014 20:50:48 -0600 Subject: [PATCH 2/5] Teaching people how to use the settings --- docs/topics/settings.rst | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/docs/topics/settings.rst b/docs/topics/settings.rst index 6023046..9f4dfe7 100644 --- a/docs/topics/settings.rst +++ b/docs/topics/settings.rst @@ -58,3 +58,10 @@ Settings a message instance's ``get_email_object`` method being called. Value of this field is the primary key of the ``django_mailbox.MessageAttachment`` instance currently storing this payload component's contents. + +* ``DJANGO_MAILBOX_MAX_MESSAGE_SIZE`` + * Default: ``False`` + * Type: ``integer`` + * If this is set, it will be read as a number of + bytes. Any messages above that size will not be + downloaded. ``2000000`` is 2 Megabytes. From b2abec1d401787c841b968ad364ed3b797e20c57 Mon Sep 17 00:00:00 2001 From: Alex Lovell-Troy Date: Wed, 28 May 2014 20:53:44 -0600 Subject: [PATCH 3/5] fixing typo in the docs --- docs/topics/settings.rst | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/topics/settings.rst b/docs/topics/settings.rst index 9f4dfe7..290e884 100644 --- a/docs/topics/settings.rst +++ b/docs/topics/settings.rst @@ -60,6 +60,7 @@ Settings instance currently storing this payload component's contents. * ``DJANGO_MAILBOX_MAX_MESSAGE_SIZE`` + * Default: ``False`` * Type: ``integer`` * If this is set, it will be read as a number of From ec18a01880819d0aa2096a6085723b4eef31b59a Mon Sep 17 00:00:00 2001 From: Alex Lovell-Troy Date: Thu, 29 May 2014 23:41:11 -0600 Subject: [PATCH 4/5] Refactored the imap transport based on the tests Testing the switch to uid-based IMAP calls required better testing The better testing revealed some code that wasn't so pretty I fixed the tests and the code --- django_mailbox/tests/base.py | 10 ++ django_mailbox/tests/test_transports.py | 127 +++++++++++------------- django_mailbox/transports/imap.py | 39 ++++---- 3 files changed, 88 insertions(+), 88 deletions(-) diff --git a/django_mailbox/tests/base.py b/django_mailbox/tests/base.py index a559b9e..72c3249 100644 --- a/django_mailbox/tests/base.py +++ b/django_mailbox/tests/base.py @@ -8,6 +8,16 @@ from django.test import TestCase from django_mailbox import models from django_mailbox.models import Mailbox, Message +def get_email_as_text(name): + with open( + os.path.join( + os.path.dirname(__file__), + 'messages', + name, + ), + 'rb' + ) as f: + return f.read() class EmailMessageTestCase(TestCase): ALLOWED_EXTRA_HEADERS = [ diff --git a/django_mailbox/tests/test_transports.py b/django_mailbox/tests/test_transports.py index 7f044d4..b701eae 100644 --- a/django_mailbox/tests/test_transports.py +++ b/django_mailbox/tests/test_transports.py @@ -1,12 +1,42 @@ import mock -import six -from django_mailbox.tests.base import EmailMessageTestCase +from django.test.utils import override_settings + +from django_mailbox.tests.base import EmailMessageTestCase, get_email_as_text from django_mailbox.transports import ImapTransport, Pop3Transport +FAKE_UID_SEARCH_ANSWER = ('OK', ['18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44']) +FAKE_UID_FETCH_SIZES = ('OK', ['1 (UID 18 RFC822.SIZE 58070000000)', '2 (UID 19 RFC822.SIZE 2593)']) +FAKE_UID_FETCH_MSG = ('OK', [('1 (UID 18 RFC822 {5807}', get_email_as_text('generic_message.eml') ),]) +FAKE_UID_COPY_MSG = ('OK', ['[COPYUID 1 2 2] (Success)']) +FAKE_LIST_ARCHIVE_FOLDERS_ANSWERS = ('OK', ['(\\HasNoChildren \\All) "/" "[Gmail]/All Mail"']) -class TestImapTransport(EmailMessageTestCase): +class IMAPTestCase(EmailMessageTestCase): def setUp(self): + def imap_server_uid_method(*args): + cmd = args[0] + arg2 = args[2] + if cmd == 'search': + return FAKE_UID_SEARCH_ANSWER + if cmd == 'copy': + return FAKE_UID_COPY_MSG + if cmd == 'fetch': + if arg2 == '(RFC822.SIZE)': + return FAKE_UID_FETCH_SIZES + if arg2 == '(RFC822)': + return FAKE_UID_FETCH_MSG + def imap_server_list_method(pattern=None): + return FAKE_LIST_ARCHIVE_FOLDERS_ANSWERS + + self.imap_server = mock.Mock() + self.imap_server.uid = imap_server_uid_method + self.imap_server.list = imap_server_list_method + super(IMAPTestCase, self).setUp() + + +class TestImapTransport(IMAPTestCase): + def setUp(self): + super(TestImapTransport, self).setUp() self.arbitrary_hostname = 'one.two.three' self.arbitrary_port = 100 self.ssl = False @@ -15,42 +45,18 @@ class TestImapTransport(EmailMessageTestCase): self.arbitrary_port, self.ssl ) - self.transport.server = None - super(TestImapTransport, self).setUp() + self.transport.server = self.imap_server def test_get_email_message(self): - with mock.patch.object(self.transport, 'server') as server: - server.search.return_value = ( - 'OK', - [ - 'One', # This is totally arbitrary - ] - ) - server.fetch.return_value = ( - 'OK', - [ - [ - '1 (RFC822 {8190}', # Wat? - self._get_email_as_text('generic_message.eml') - ], - ')', - ] - ) - actual_messages = list(self.transport.get_message()) - - self.assertEqual(len(actual_messages), 1) - + actual_messages = list(self.transport.get_message()) + self.assertEqual(len(actual_messages), 27) actual_message = actual_messages[0] expected_message = self._get_email_object('generic_message.eml') - self.assertEqual(expected_message, actual_message) - -class TestImapArchivedTransport(EmailMessageTestCase): +class TestImapArchivedTransport(TestImapTransport): def setUp(self): - self.arbitrary_hostname = 'one.two.three' - self.arbitrary_port = 100 - self.ssl = False + super(TestImapArchivedTransport, self).setUp() self.archive = 'Archive' self.transport = ImapTransport( self.arbitrary_hostname, @@ -58,50 +64,34 @@ class TestImapArchivedTransport(EmailMessageTestCase): self.ssl, self.archive ) - self.transport.server = None - super(TestImapArchivedTransport, self).setUp() + self.transport.server = self.imap_server + +class TestMaxSizeImapTransport(TestImapTransport): + + @override_settings(DJANGO_MAILBOX_MAX_MESSAGE_SIZE=5807) + def setUp(self): + super(TestMaxSizeImapTransport, self).setUp() + + self.transport = ImapTransport( + self.arbitrary_hostname, + self.arbitrary_port, + self.ssl, + ) + self.transport.server = self.imap_server + + def test_size_limit(self): + all_message_ids = self.transport._get_all_message_ids() + small_message_ids = self.transport._get_small_message_ids(all_message_ids) + self.assertEqual(len(small_message_ids), 1) def test_get_email_message(self): - with mock.patch.object(self.transport, 'server') as server: - server.search.return_value = ( - 'OK', - [ - 'One', # This is totally arbitrary - ] - ) - server.fetch.return_value = ( - 'OK', - [ - [ - '1 (RFC822 {8190}', # Wat? - self._get_email_as_text('generic_message.eml') - ], - ')', - ] - ) - server.list.return_value = ( - 'OK', - [ - '(\\HasNoChildren) "/" "Archive"' - ] - ) - server.copy.return_value = ( - 'OK', - [ - '[COPYUID 1 2 2] (Success)' - ] - ) - actual_messages = list(self.transport.get_message()) - + actual_messages = list(self.transport.get_message()) self.assertEqual(len(actual_messages), 1) - actual_message = actual_messages[0] expected_message = self._get_email_object('generic_message.eml') - self.assertEqual(expected_message, actual_message) - class TestPop3Transport(EmailMessageTestCase): def setUp(self): self.arbitrary_hostname = 'one.two.three' @@ -139,3 +129,4 @@ class TestPop3Transport(EmailMessageTestCase): expected_message = self._get_email_object('generic_message.eml') self.assertEqual(expected_message, actual_message) + diff --git a/django_mailbox/transports/imap.py b/django_mailbox/transports/imap.py index 570788a..c40a647 100644 --- a/django_mailbox/transports/imap.py +++ b/django_mailbox/transports/imap.py @@ -1,17 +1,21 @@ +import logging + +logger = logging.getLogger(__name__) + from imaplib import IMAP4, IMAP4_SSL from .base import EmailTransport, MessageParseError from django.conf import settings -MAX_MESSAGE_SIZE = getattr( - settings, - 'DJANGO_MAILBOX_MAX_MESSAGE_SIZE', - False -) class ImapTransport(EmailTransport): def __init__(self, hostname, port=None, ssl=False, archive=''): + MAX_MESSAGE_SIZE = getattr( + settings, + 'DJANGO_MAILBOX_MAX_MESSAGE_SIZE', + False + ) self.hostname = hostname self.port = port self.archive = archive @@ -30,10 +34,9 @@ class ImapTransport(EmailTransport): typ, msg = self.server.login(username, password) self.server.select() - def _get_all_message_ids(self): # Fetch all the message uids - response, message_ids = self.server.uid('search', None, 'ALL', ) + response, message_ids = self.server.uid('search', None, 'ALL') return message_ids[0].split(' ') def _get_small_message_ids(self, message_ids): @@ -45,21 +48,18 @@ class ImapTransport(EmailTransport): status, data = self.server.uid( 'fetch', ','.join(message_ids), - '(BODY.PEEK[HEADER] RFC822.SIZE BODYSTRUCTURE)' + '(RFC822.SIZE)' ) for each_msg in data: - if isinstance(each_msg, tuple): - try: - metadata, structure = each_msg[0].split(' BODYSTRUCTURE ') - uid = metadata.split('(')[1].split(' ')[1] - size = metadata.split('(')[1].split(' ')[3] - if int(size) <= int(self.MAX_MSG_SIZE): - safe_message_ids.append(uid) - except ValueError, e: - print "ValueError: %s working on %s" % (e, each_msg[0]) - print each_msg - pass + try: + uid = each_msg.split(' ')[2] + size = each_msg.split(' ')[4].rstrip(')') + if int(size) <= int(self.MAX_MSG_SIZE): + safe_message_ids.append(uid) + except ValueError, e: + logger.warning("ValueError: %s working on %s" % (e, each_msg[0])) + pass return safe_message_ids def get_message(self): @@ -69,7 +69,6 @@ class ImapTransport(EmailTransport): if self.MAX_MSG_SIZE: message_ids = self._get_small_message_ids(message_ids) - if not message_ids: return From 1efa1140a689edd50aa843f6e69be87fe5b96523 Mon Sep 17 00:00:00 2001 From: Alex Lovell-Troy Date: Thu, 29 May 2014 23:47:48 -0600 Subject: [PATCH 5/5] I'll get the 2to3 changes right someday --- django_mailbox/transports/imap.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/django_mailbox/transports/imap.py b/django_mailbox/transports/imap.py index c40a647..b6255b1 100644 --- a/django_mailbox/transports/imap.py +++ b/django_mailbox/transports/imap.py @@ -57,7 +57,7 @@ class ImapTransport(EmailTransport): size = each_msg.split(' ')[4].rstrip(')') if int(size) <= int(self.MAX_MSG_SIZE): safe_message_ids.append(uid) - except ValueError, e: + except ValueError as e: logger.warning("ValueError: %s working on %s" % (e, each_msg[0])) pass return safe_message_ids