From aab87b503184619b5637d6b326ec7717e34dfb99 Mon Sep 17 00:00:00 2001 From: Kali Kaneko Date: Fri, 16 Jan 2015 19:20:37 -0400 Subject: lots of little fixes after meskio's review mostly having to do with poor, missing or outdated documentation, naming of confusing things and reordering of code blocks for improved readability. --- src/leap/mail/adaptors/soledad.py | 85 ++++++++++------------ src/leap/mail/imap/account.py | 82 ++++++++++++---------- src/leap/mail/imap/mailbox.py | 43 +++++++----- src/leap/mail/imap/messages.py | 28 +++++++- src/leap/mail/imap/server.py | 10 ++- src/leap/mail/imap/service/imap.py | 20 +++--- src/leap/mail/imap/tests/test_imap.py | 61 +--------------- src/leap/mail/mail.py | 70 ++++++++++++------- src/leap/mail/mailbox_indexer.py | 128 ++++++++++++++-------------------- src/leap/mail/walk.py | 4 +- 10 files changed, 255 insertions(+), 276 deletions(-) diff --git a/src/leap/mail/adaptors/soledad.py b/src/leap/mail/adaptors/soledad.py index 46dbe4c..9f0bb30 100644 --- a/src/leap/mail/adaptors/soledad.py +++ b/src/leap/mail/adaptors/soledad.py @@ -19,7 +19,6 @@ Soledadad MailAdaptor module. import re from collections import defaultdict from email import message_from_string -from functools import partial from pycryptopp.hash import sha256 from twisted.internet import defer @@ -72,6 +71,7 @@ class SoledadDocumentWrapper(models.DocumentWrapper): deletion. """ # TODO we could also use a _dirty flag (in models) + # TODO add a get_count() method ??? -- that is extended over u1db. # We keep a dictionary with DeferredLocks, that will be # unique to every subclass of SoledadDocumentWrapper. @@ -86,10 +86,9 @@ class SoledadDocumentWrapper(models.DocumentWrapper): """ return cls._k_locks[cls.__name__] - def __init__(self, **kwargs): - doc_id = kwargs.pop('doc_id', None) + def __init__(self, doc_id=None, future_doc_id=None, **kwargs): self._doc_id = doc_id - self._future_doc_id = kwargs.pop('future_doc_id', None) + self._future_doc_id = future_doc_id self._lock = defer.DeferredLock() super(SoledadDocumentWrapper, self).__init__(**kwargs) @@ -123,7 +122,7 @@ class SoledadDocumentWrapper(models.DocumentWrapper): def update_doc_id(doc): self._doc_id = doc.doc_id - self._future_doc_id = None + self.set_future_doc_id(None) return doc if self.future_doc_id is None: @@ -201,6 +200,7 @@ class SoledadDocumentWrapper(models.DocumentWrapper): @classmethod def _get_or_create(cls, store, index, value): + # TODO shorten this method. assert store is not None assert index is not None assert value is not None @@ -211,6 +211,7 @@ class SoledadDocumentWrapper(models.DocumentWrapper): except AttributeError: raise RuntimeError("The model is badly defined") + # TODO separate into another method? def try_to_get_doc_from_index(indexes): values = [] idx_def = dict(indexes)[index] @@ -323,9 +324,6 @@ class SoledadDocumentWrapper(models.DocumentWrapper): d.addCallback(wrap_docs) return d - # TODO - # [ ] get_count() ??? - def __repr__(self): try: idx = getattr(self, self.model.__meta__.index) @@ -442,29 +440,23 @@ class MessageWrapper(object): integers, beginning at one, and the values are dictionaries with the content of the content-docs. """ - if isinstance(mdoc, SoledadDocument): - mdoc_id = mdoc.doc_id - mdoc = mdoc.content - else: - mdoc_id = None - if not mdoc: - mdoc = {} - self.mdoc = MetaMsgDocWrapper(doc_id=mdoc_id, **mdoc) - - if isinstance(fdoc, SoledadDocument): - fdoc_id = fdoc.doc_id - fdoc = fdoc.content - else: - fdoc_id = None - self.fdoc = FlagsDocWrapper(doc_id=fdoc_id, **fdoc) + + def get_doc_wrapper(doc, cls): + if isinstance(doc, SoledadDocument): + doc_id = doc.doc_id + doc = doc.content + else: + doc_id = None + if not doc: + doc = {} + return cls(doc_id=doc_id, **doc) + + self.mdoc = get_doc_wrapper(mdoc, MetaMsgDocWrapper) + + self.fdoc = get_doc_wrapper(fdoc, FlagsDocWrapper) self.fdoc.set_future_doc_id(self.mdoc.fdoc) - if isinstance(hdoc, SoledadDocument): - hdoc_id = hdoc.doc_id - hdoc = hdoc.content - else: - hdoc_id = None - self.hdoc = HeaderDocWrapper(doc_id=hdoc_id, **hdoc) + self.hdoc = get_doc_wrapper(hdoc, HeaderDocWrapper) self.hdoc.set_future_doc_id(self.mdoc.hdoc) if cdocs is None: @@ -489,10 +481,6 @@ class MessageWrapper(object): "Cannot create: fdoc has a doc_id") # TODO check that the doc_ids in the mdoc are coherent - # TODO I think we need to tolerate the no hdoc.doc_id case, for when we - # are doing a copy to another mailbox. - # leap_assert(self.hdoc.doc_id is None, - # "Cannot create: hdoc has a doc_id") d = [] d.append(self.mdoc.create(store)) d.append(self.fdoc.create(store)) @@ -566,8 +554,9 @@ class MessageWrapper(object): def get_subpart_dict(self, index): """ - :param index: index, 1-indexed + :param index: the part to lookup, 1-indexed :type index: int + :rtype: dict """ return self.hdoc.part_map[str(index)] @@ -785,16 +774,6 @@ class SoledadMailAdaptor(SoledadIndexMixin): assert(MessageClass is not None) return MessageClass(MessageWrapper(mdoc, fdoc, hdoc, cdocs), uid=uid) - def _get_msg_from_variable_doc_list(self, doc_list, msg_class, uid=None): - if len(doc_list) == 3: - mdoc, fdoc, hdoc = doc_list - cdocs = None - elif len(doc_list) > 3: - fdoc, hdoc = doc_list[:3] - cdocs = dict(enumerate(doc_list[3:], 1)) - return self.get_msg_from_docs( - msg_class, mdoc, fdoc, hdoc, cdocs, uid=uid) - def get_msg_from_mdoc_id(self, MessageClass, store, mdoc_id, uid=None, get_cdocs=False): @@ -844,10 +823,21 @@ class SoledadMailAdaptor(SoledadIndexMixin): else: d = get_parts_doc_from_mdoc_id() - d.addCallback(partial(self._get_msg_from_variable_doc_list, - msg_class=MessageClass, uid=uid)) + d.addCallback(self._get_msg_from_variable_doc_list, + msg_class=MessageClass, uid=uid) return d + def _get_msg_from_variable_doc_list(self, doc_list, msg_class, uid=None): + if len(doc_list) == 3: + mdoc, fdoc, hdoc = doc_list + cdocs = None + elif len(doc_list) > 3: + # XXX is this case used? + mdoc, fdoc, hdoc = doc_list[:3] + cdocs = dict(enumerate(doc_list[3:], 1)) + return self.get_msg_from_docs( + msg_class, mdoc, fdoc, hdoc, cdocs, uid=uid) + def get_flags_from_mdoc_id(self, store, mdoc_id): """ # XXX stuff here... @@ -875,7 +865,6 @@ class SoledadMailAdaptor(SoledadIndexMixin): def create_msg(self, store, msg): """ :param store: an instance of soledad, or anything that behaves alike - :type store: :param msg: a Message object. :return: a Deferred that is fired when all the underlying documents @@ -889,8 +878,6 @@ class SoledadMailAdaptor(SoledadIndexMixin): """ :param msg: a Message object. :param store: an instance of soledad, or anything that behaves alike - :type store: - :param msg: a Message object. :return: a Deferred that is fired when all the underlying documents have been updated (actually, it's only the fdoc that's allowed to update). diff --git a/src/leap/mail/imap/account.py b/src/leap/mail/imap/account.py index 0cf583b..38df845 100644 --- a/src/leap/mail/imap/account.py +++ b/src/leap/mail/imap/account.py @@ -49,9 +49,6 @@ if PROFILE_CMD: # Soledad IMAP Account ####################################### -# XXX watchout, account needs to be ready... so we should maybe return -# a deferred to the IMAP service when it's initialized - class IMAPAccount(object): """ An implementation of an imap4 Account @@ -67,8 +64,14 @@ class IMAPAccount(object): """ Keeps track of the mailboxes and subscriptions handled by this account. - :param account: The name of the account (user id). - :type account: str + The account is not ready to be used, since the store needs to be + initialized and we also need to do some initialization routines. + You can either pass a deferred to this constructor, or use + `callWhenReady` method. + + :param user_id: The name of the account (user id, in the form + user@provider). + :type user_id: str :param store: a Soledad instance. :type store: Soledad @@ -87,11 +90,6 @@ class IMAPAccount(object): self.user_id = user_id self.account = Account(store, ready_cb=lambda: d.callback(self)) - def _return_mailbox_from_collection(self, collection, readwrite=1): - if collection is None: - return None - mbox = IMAPMailbox(collection, rw=readwrite) - return mbox def end_session(self): """ Used to mark when the session has closed, and we should not allow any @@ -103,6 +101,12 @@ class IMAPAccount(object): self.session_ended = True def callWhenReady(self, cb, *args, **kw): + """ + Execute callback when the account is ready to be used. + XXX note that this callback will be called with a first ignored + parameter. + """ + # TODO ignore the first parameter and change tests accordingly. d = self.account.callWhenReady(cb, *args, **kw) return d @@ -129,6 +133,12 @@ class IMAPAccount(object): d.addCallback(self._return_mailbox_from_collection) return d + def _return_mailbox_from_collection(self, collection, readwrite=1): + if collection is None: + return None + mbox = IMAPMailbox(collection, rw=readwrite) + return mbox + # # IAccount # @@ -146,7 +156,7 @@ class IMAPAccount(object): :type creation_ts: int :returns: a Deferred that will contain the document if successful. - :rtype: bool + :rtype: defer.Deferred """ name = normalize_mailbox(name) @@ -190,25 +200,15 @@ class IMAPAccount(object): :return: A deferred that will fire with a true value if the creation - succeeds. + succeeds. The deferred might fail with a MailboxException + if the mailbox cannot be added. :rtype: Deferred - :raise MailboxException: Raised if this mailbox cannot be added. """ - paths = filter(None, normalize_mailbox(pathspec).split('/')) - subs = [] - sep = '/' - def pass_on_collision(failure): failure.trap(imap4.MailboxCollision) return True - for accum in range(1, len(paths)): - partial_path = sep.join(paths[:accum]) - d = self.addMailbox(partial_path) - d.addErrback(pass_on_collision) - subs.append(d) - def handle_collision(failure): failure.trap(imap4.MailboxCollision) if not pathspec.endswith('/'): @@ -216,19 +216,26 @@ class IMAPAccount(object): else: return defer.succeed(True) + def all_good(result): + return all(result) + + paths = filter(None, normalize_mailbox(pathspec).split('/')) + subs = [] + sep = '/' + + for accum in range(1, len(paths)): + partial_path = sep.join(paths[:accum]) + d = self.addMailbox(partial_path) + d.addErrback(pass_on_collision) + subs.append(d) + df = self.addMailbox(sep.join(paths)) df.addErrback(handle_collision) subs.append(df) - def all_good(result): - return all(result) - - if subs: - d1 = defer.gatherResults(subs) - d1.addCallback(all_good) - return d1 - else: - return defer.succeed(False) + d1 = defer.gatherResults(subs) + d1.addCallback(all_good) + return d1 def select(self, name, readwrite=1): """ @@ -285,8 +292,7 @@ class IMAPAccount(object): global _mboxes _mboxes = mailboxes if name not in mailboxes: - err = imap4.MailboxException("No such mailbox: %r" % name) - return defer.fail(err) + raise imap4.MailboxException("No such mailbox: %r" % name) def get_mailbox(_): return self.getMailbox(name) @@ -303,10 +309,9 @@ class IMAPAccount(object): # as part of their root. for others in _mboxes: if others != name and others.startswith(name): - err = imap4.MailboxException( + raise imap4.MailboxException( "Hierarchically inferior mailboxes " "exist and \\Noselect is set") - return defer.fail(err) return mbox d = self.account.list_all_mailbox_names() @@ -324,8 +329,6 @@ class IMAPAccount(object): # if self._inferiorNames(name) > 1: # self._index.removeMailbox(name) - # TODO use mail.Account.rename_mailbox - # TODO finish conversion to deferreds def rename(self, oldname, newname): """ Renames a mailbox. @@ -460,6 +463,9 @@ class IMAPAccount(object): :type name: str :rtype: Deferred """ + # TODO should raise MailboxException if attempted to unsubscribe + # from a mailbox that is not currently subscribed. + # TODO factor out with subscribe method. name = normalize_mailbox(name) def set_unsubscribed(mbox): diff --git a/src/leap/mail/imap/mailbox.py b/src/leap/mail/imap/mailbox.py index 58fc514..52f4dd5 100644 --- a/src/leap/mail/imap/mailbox.py +++ b/src/leap/mail/imap/mailbox.py @@ -91,8 +91,9 @@ class IMAPMailbox(object): implements( imap4.IMailbox, imap4.IMailboxInfo, - #imap4.ICloseableMailbox, imap4.ISearchableMailbox, + # XXX I think we do not need to implement CloseableMailbox, do we? + # imap4.ICloseableMailbox imap4.IMessageCopier) init_flags = INIT_FLAGS @@ -108,8 +109,6 @@ class IMAPMailbox(object): def __init__(self, collection, rw=1): """ - SoledadMailbox constructor. - :param collection: instance of IMAPMessageCollection :type collection: IMAPMessageCollection @@ -117,14 +116,10 @@ class IMAPMailbox(object): :type rw: int """ self.rw = rw - self.closed = False self._uidvalidity = None self.collection = collection - if not self.getFlags(): - self.setFlags(self.init_flags) - @property def mbox_name(self): return self.collection.mbox_name @@ -201,6 +196,7 @@ class IMAPMailbox(object): "flags expected to be a tuple") return self.collection.set_mbox_attr("flags", flags) + # TODO - not used? @property def is_closed(self): """ @@ -211,6 +207,7 @@ class IMAPMailbox(object): """ return self.collection.get_mbox_attr("closed") + # TODO - not used? def set_closed(self, closed): """ Set the closed attribute for this mailbox. @@ -448,9 +445,6 @@ class IMAPMailbox(object): d.addCallback(remove_mbox) return d - def _close_cb(self, result): - self.closed = True - def expunge(self): """ Remove all messages flagged \\Deleted @@ -555,24 +549,23 @@ class IMAPMailbox(object): # for sequence numbers (uid = 0) if is_sequence: - logger.debug("Getting msg by index: INEFFICIENT call!") # TODO --- implement sequences in mailbox indexer raise NotImplementedError else: - d = self._get_sequence_of_messages(messages_asked) + d = self._get_messages_range(messages_asked) d.addCallback(get_imap_messages_for_sequence) # TODO -- call signal_to_ui # d.addCallback(self.cb_signal_unread_to_ui) return d - def _get_sequence_of_messages(self, messages_asked): - def get_sequence(messages_asked): + def _get_messages_range(self, messages_asked): + def get_range(messages_asked): return self._filter_msg_seq(messages_asked) d = defer.maybeDeferred(self._bound_seq, messages_asked) - d.addCallback(get_sequence) + d.addCallback(get_range) return d def fetch_flags(self, messages_asked, uid): @@ -599,6 +592,10 @@ class IMAPMailbox(object): MessagePart. :rtype: tuple """ + is_sequence = True if uid == 0 else False + if is_sequence: + raise NotImplementedError + d = defer.Deferred() reactor.callLater(0, self._do_fetch_flags, messages_asked, uid, d) if PROFILE_CMD: @@ -649,7 +646,7 @@ class IMAPMailbox(object): generator = (item for item in result) d.callback(generator) - d_seq = self._get_sequence_of_messages(messages_asked) + d_seq = self._get_messages_range(messages_asked) d_seq.addCallback(get_flags_for_seq) return d_seq @@ -677,7 +674,11 @@ class IMAPMailbox(object): MessagePart. :rtype: tuple """ + # TODO implement sequences # TODO how often is thunderbird doing this? + is_sequence = True if uid == 0 else False + if is_sequence: + raise NotImplementedError class headersPart(object): def __init__(self, uid, headers): @@ -753,6 +754,12 @@ class IMAPMailbox(object): :raise ReadOnlyMailbox: Raised if this mailbox is not open for read-write. """ + # TODO implement sequences + # TODO how often is thunderbird doing this? + is_sequence = True if uid == 0 else False + if is_sequence: + raise NotImplementedError + if not self.isWriteable(): log.msg('read only mailbox!') raise imap4.ReadOnlyMailbox @@ -777,7 +784,7 @@ class IMAPMailbox(object): done. :type observer: deferred """ - # XXX implement also sequence (uid = 0) + # TODO implement also sequence (uid = 0) # TODO we should prevent client from setting Recent flag leap_assert(not isinstance(flags, basestring), "flags cannot be a string") @@ -800,7 +807,7 @@ class IMAPMailbox(object): got_flags_setted.addCallback(return_result_dict) return got_flags_setted - d_seq = self._get_sequence_of_messages(messages_asked) + d_seq = self._get_messages_range(messages_asked) d_seq.addCallback(set_flags_for_seq) return d_seq diff --git a/src/leap/mail/imap/messages.py b/src/leap/mail/imap/messages.py index df50323..8f4c953 100644 --- a/src/leap/mail/imap/messages.py +++ b/src/leap/mail/imap/messages.py @@ -36,16 +36,38 @@ logger = logging.getLogger(__name__) class IMAPMessage(object): """ - The main representation of a message. + The main representation of a message as seen by the IMAP Server. + This class implements the semantics specific to IMAP specification. """ - implements(imap4.IMessage) def __init__(self, message, prefetch_body=True, store=None, d=defer.Deferred()): """ - Initializes a LeapMessage. + Get an IMAPMessage. A mail.Message is needed, since many of the methods + are proxied to that object. + + + If you do not need to prefetch the body of the message, you can set + `prefetch_body` to False, but the current imap server implementation + expect the getBodyFile method to return inmediately. + + When the prefetch_body option is used, a deferred is also expected as a + parameter, and this will fire when the deferred initialization has + taken place, with this instance of IMAPMessage as a parameter. + + :param message: the abstract message + :type message: mail.Message + :param prefetch_body: Whether to prefetch the content doc for the body. + :type prefetch_body: bool + :param store: an instance of soledad, or anything that behaves like it. + :param d: an optional deferred, that will be fired with the instance of + the IMAPMessage being initialized + :type d: defer.Deferred """ + # TODO substitute the use of the deferred initialization by a factory + # function, maybe. + self.message = message self.__body_fd = None self.store = store diff --git a/src/leap/mail/imap/server.py b/src/leap/mail/imap/server.py index 23ddefc..027fd7a 100644 --- a/src/leap/mail/imap/server.py +++ b/src/leap/mail/imap/server.py @@ -500,13 +500,18 @@ class LEAPIMAPServer(imap4.IMAP4Server): select_DELETE = auth_DELETE # Need to override the command table after patching - # arg_astring and arg_literal + # arg_astring and arg_literal, except on the methods that we are already + # overriding. + # TODO -------------------------------------------- + # Check if we really need to override these + # methods, or we can monkeypatch. # do_DELETE = imap4.IMAP4Server.do_DELETE # do_CREATE = imap4.IMAP4Server.do_CREATE # do_RENAME = imap4.IMAP4Server.do_RENAME # do_SUBSCRIBE = imap4.IMAP4Server.do_SUBSCRIBE # do_UNSUBSCRIBE = imap4.IMAP4Server.do_UNSUBSCRIBE + # ------------------------------------------------- do_LOGIN = imap4.IMAP4Server.do_LOGIN do_STATUS = imap4.IMAP4Server.do_STATUS do_APPEND = imap4.IMAP4Server.do_APPEND @@ -530,8 +535,11 @@ class LEAPIMAPServer(imap4.IMAP4Server): auth_EXAMINE = (_selectWork, arg_astring, 0, 'EXAMINE') select_EXAMINE = auth_EXAMINE + # TODO ----------------------------------------------- + # re-add if we stop overriding DELETE # auth_DELETE = (do_DELETE, arg_astring) # select_DELETE = auth_DELETE + # ---------------------------------------------------- auth_RENAME = (do_RENAME, arg_astring, arg_astring) select_RENAME = auth_RENAME diff --git a/src/leap/mail/imap/service/imap.py b/src/leap/mail/imap/service/imap.py index 93e4d62..cc76e3a 100644 --- a/src/leap/mail/imap/service/imap.py +++ b/src/leap/mail/imap/service/imap.py @@ -17,6 +17,7 @@ """ IMAP service initialization """ +# TODO: leave only an implementor of IService in here import logging import os @@ -29,10 +30,9 @@ from twisted.python import log logger = logging.getLogger(__name__) from leap.common import events as leap_events -from leap.common.check import leap_assert, leap_assert_type, leap_check +from leap.common.check import leap_assert_type, leap_check from leap.mail.imap.account import IMAPAccount from leap.mail.imap.server import LEAPIMAPServer -from leap.mail.incoming import IncomingMail from leap.soledad.client import Soledad from leap.common.events.events_pb2 import IMAP_SERVICE_STARTED @@ -113,6 +113,10 @@ class LeapIMAPFactory(ServerFactory): """ Stops imap service (fetcher, factory and port). """ + # mark account as unusable, so any imap command will fail + # with unauth state. + self.theAccount.end_session() + # TODO should wait for all the pending deferreds, # the twisted way! if DO_PROFILE: @@ -123,23 +127,23 @@ class LeapIMAPFactory(ServerFactory): return ServerFactory.doStop(self) -def run_service(*args, **kwargs): +def run_service(store, **kwargs): """ Main entry point to run the service from the client. + :param store: a soledad instance + :returns: the port as returned by the reactor when starts listening, and the factory for the protocol. """ - leap_assert(len(args) == 2) - soledad = args - leap_assert_type(soledad, Soledad) + leap_assert_type(store, Soledad) port = kwargs.get('port', IMAP_PORT) userid = kwargs.get('userid', None) leap_check(userid is not None, "need an user id") - uuid = soledad.uuid - factory = LeapIMAPFactory(uuid, userid, soledad) + uuid = store.uuid + factory = LeapIMAPFactory(uuid, userid, store) try: tport = reactor.listenTCP(port, factory, diff --git a/src/leap/mail/imap/tests/test_imap.py b/src/leap/mail/imap/tests/test_imap.py index 6be41cd..67a24cd 100644 --- a/src/leap/mail/imap/tests/test_imap.py +++ b/src/leap/mail/imap/tests/test_imap.py @@ -75,6 +75,7 @@ class TestRealm: # TestCases # +# TODO rename to IMAPMessageCollection class MessageCollectionTestCase(IMAP4HelperMixin): """ Tests for the MessageCollection class @@ -87,6 +88,7 @@ class MessageCollectionTestCase(IMAP4HelperMixin): We override mixin method since we are only testing MessageCollection interface in this particular TestCase """ + # FIXME -- return deferred super(MessageCollectionTestCase, self).setUp() # FIXME --- update initialization @@ -1090,64 +1092,6 @@ class LEAPIMAP4ServerTestCase(IMAP4HelperMixin): # Okay, that was much fun indeed - # skipping close test: we just need expunge for now. - #def testClose(self): - #""" - #Test closing the mailbox. We expect to get deleted all messages flagged - #as such. - #""" - #acc = self.server.theAccount - #mailbox_name = 'mailbox-close' -# - #def add_mailbox(): - #return acc.addMailbox(mailbox_name) -# - #def login(): - #return self.client.login(TEST_USER, TEST_PASSWD) -# - #def select(): - #return self.client.select(mailbox_name) -# - #def get_mailbox(): - #def _save_mbox(mailbox): - #self.mailbox = mailbox - #d = self.server.theAccount.getMailbox(mailbox_name) - #d.addCallback(_save_mbox) - #return d -# - #def add_messages(): - #d1 = self.mailbox.addMessage( - #'test 1', flags=('\\Deleted', 'AnotherFlag')) - #d2 = self.mailbox.addMessage( - #'test 2', flags=('AnotherFlag',)) - #d3 = self.mailbox.addMessage( - #'test 3', flags=('\\Deleted',)) - #d = defer.gatherResults([d1, d2, d3]) - #return d -# - #def close(): - #return self.client.close() -# - #d = self.connected.addCallback(strip(add_mailbox)) - #d.addCallback(strip(login)) - #d.addCallbacks(strip(select), self._ebGeneral) - #d.addCallback(strip(get_mailbox)) - #d.addCallbacks(strip(add_messages), self._ebGeneral) - #d.addCallbacks(strip(close), self._ebGeneral) - #d.addCallbacks(self._cbStopClient, self._ebGeneral) - #d2 = self.loopback() - #d1 = defer.gatherResults([d, d2]) - #d1.addCallback(lambda _: self.mailbox.getMessageCount()) - #d1.addCallback(self._cbTestClose) - #return d1 -# - #def _cbTestClose(self, count): - # TODO is this correct? count should not take into account those - # flagged as deleted??? - #self.assertEqual(count, 1) - # TODO --- assert flags are those of the message #2 - #self.failUnless(self.mailbox.closed) - def testExpunge(self): """ Test expunge command @@ -1209,6 +1153,7 @@ class LEAPIMAP4ServerTestCase(IMAP4HelperMixin): self.assertItemsEqual(self.results, [1, 3]) +# TODO -------- Fix this testcase class AccountTestCase(IMAP4HelperMixin): """ Test the Account. diff --git a/src/leap/mail/mail.py b/src/leap/mail/mail.py index 9b7a562..59fd57c 100644 --- a/src/leap/mail/mail.py +++ b/src/leap/mail/mail.py @@ -58,9 +58,16 @@ def _write_and_rewind(payload): class MessagePart(object): - # TODO pass cdocs in init - def __init__(self, part_map, cdocs={}): + """ + :param part_map: a dictionary mapping the subparts for + this MessagePart (1-indexed). + :type part_map: dict + :param cdoc: optional, a dict of content documents + """ + # TODO document the expected keys in the part_map dict. + # TODO add abstraction layer between the cdocs and this class. Only + # adaptor should know about the format of the cdocs. self._pmap = part_map self._cdocs = cdocs @@ -266,6 +273,10 @@ class MessageCollection(object): # [ ] To guarantee synchronicity of the documents sent together during a # sync, we could get hold of a deferredLock that inhibits # synchronization while we are updating (think more about this!) + # [ ] review the serveral count_ methods. I think it's better to patch + # server to accept deferreds. + # [ ] Use inheritance for the mailbox-collection instead of handling the + # special cases everywhere? # Account should provide an adaptor instance when creating this collection. adaptor = None @@ -285,9 +296,6 @@ class MessageCollection(object): self.mbox_indexer = mbox_indexer self.mbox_wrapper = mbox_wrapper - # TODO --- review this count shit. I think it's better to patch server - # to accept deferreds. - def is_mailbox_collection(self): """ Return True if this collection represents a Mailbox. @@ -297,22 +305,26 @@ class MessageCollection(object): @property def mbox_name(self): - wrapper = getattr(self, "mbox_wrapper", None) - if not wrapper: + # TODO raise instead? + if self.mbox_wrapper is None: return None - return wrapper.mbox + return self.mbox_wrapper.mbox @property def mbox_uuid(self): - wrapper = getattr(self, "mbox_wrapper", None) - if not wrapper: + # TODO raise instead? + if self.mbox_wrapper is None: return None - return wrapper.uuid + return self.mbox_wrapper.uuid def get_mbox_attr(self, attr): + if self.mbox_wrapper is None: + raise RuntimeError("This is not a mailbox collection") return getattr(self.mbox_wrapper, attr) def set_mbox_attr(self, attr, value): + if self.mbox_wrapper is None: + raise RuntimeError("This is not a mailbox collection") setattr(self.mbox_wrapper, attr, value) return self.mbox_wrapper.update(self.store) @@ -323,10 +335,10 @@ class MessageCollection(object): Retrieve a message by its content hash. :rtype: Deferred """ - if not self.is_mailbox_collection(): - # instead of getting the metamsg by chash, query by (meta) index - # or use the internal collection of pointers-to-docs. + # TODO instead of getting the metamsg by chash, in this case we + # should query by (meta) index or use the internal collection of + # pointers-to-docs. raise NotImplementedError() metamsg_id = _get_mdoc_id(self.mbox_name, chash) @@ -462,6 +474,9 @@ class MessageCollection(object): raise NotImplementedError() def insert_copied_mdoc_id(wrapper): + # TODO this needs to be implemented before the copy + # interface works. + newmailbox_uuid = get_mbox_uuid_from_msg_wrapper(wrapper) return self.mbox_indexer.insert_doc( newmailbox_uuid, wrapper.mdoc.doc_id) @@ -525,15 +540,6 @@ class MessageCollection(object): d.addCallback(del_all_uid) return d - def _update_flags_or_tags(self, old, new, mode): - if mode == Flagsmode.APPEND: - final = list((set(tuple(old) + new))) - elif mode == Flagsmode.REMOVE: - final = list(set(old).difference(set(new))) - elif mode == Flagsmode.SET: - final = new - return final - def update_flags(self, msg, flags, mode): """ Update flags for a given message. @@ -563,6 +569,15 @@ class MessageCollection(object): d.addCallback(newtags) return d + def _update_flags_or_tags(self, old, new, mode): + if mode == Flagsmode.APPEND: + final = list((set(tuple(old) + new))) + elif mode == Flagsmode.REMOVE: + final = list(set(old).difference(set(new))) + elif mode == Flagsmode.SET: + final = new + return final + class Account(object): """ @@ -573,7 +588,7 @@ class Account(object): basic collection handled by traditional MUAs, but it can also handle other types of Collections (tag based, for instance). - leap.mail.imap.SoledadBackedAccount partially proxies methods in this + leap.mail.imap.IMAPAccount partially proxies methods in this class. """ @@ -582,7 +597,6 @@ class Account(object): # the Account class. adaptor_class = SoledadMailAdaptor - store = None def __init__(self, store, ready_cb=None): self.store = store @@ -612,6 +626,12 @@ class Account(object): return d def callWhenReady(self, cb, *args, **kw): + """ + Execute the callback when the initialization of the Account is ready. + Note that the callback will receive a first meaningless parameter. + """ + # TODO this should ignore the first parameter explicitely + # lambda _: cb(*args, **kw) self.deferred_initialization.addCallback(cb, *args, **kw) return self.deferred_initialization diff --git a/src/leap/mail/mailbox_indexer.py b/src/leap/mail/mailbox_indexer.py index 43a1f60..4eb0fa8 100644 --- a/src/leap/mail/mailbox_indexer.py +++ b/src/leap/mail/mailbox_indexer.py @@ -38,23 +38,23 @@ class WrongMetaDocIDError(Exception): pass -def sanitize(mailbox_id): - return mailbox_id.replace("-", "_") +def sanitize(mailbox_uuid): + return mailbox_uuid.replace("-", "_") -def check_good_uuid(mailbox_id): +def check_good_uuid(mailbox_uuid): """ Check that the passed mailbox identifier is a valid UUID. - :param mailbox_id: the uuid to check - :type mailbox_id: str + :param mailbox_uuid: the uuid to check + :type mailbox_uuid: str :return: None :raises: AssertionError if a wrong uuid was passed. """ try: - uuid.UUID(str(mailbox_id)) + uuid.UUID(str(mailbox_uuid)) except (AttributeError, ValueError): raise AssertionError( - "the mbox_id is not a valid uuid: %s" % mailbox_id) + "the mbox_id is not a valid uuid: %s" % mailbox_uuid) class MailboxIndexer(object): @@ -88,51 +88,33 @@ class MailboxIndexer(object): assert self.store is not None return self.store.raw_sqlcipher_query(*args, **kw) - def create_table(self, mailbox_id): + def create_table(self, mailbox_uuid): """ Create the UID table for a given mailbox. :param mailbox: the mailbox identifier. :type mailbox: str :rtype: Deferred """ - check_good_uuid(mailbox_id) + check_good_uuid(mailbox_uuid) sql = ("CREATE TABLE if not exists {preffix}{name}( " "uid INTEGER PRIMARY KEY AUTOINCREMENT, " "hash TEXT UNIQUE NOT NULL)".format( - preffix=self.table_preffix, name=sanitize(mailbox_id))) + preffix=self.table_preffix, name=sanitize(mailbox_uuid))) return self._query(sql) - def delete_table(self, mailbox_id): + def delete_table(self, mailbox_uuid): """ Delete the UID table for a given mailbox. :param mailbox: the mailbox name :type mailbox: str :rtype: Deferred """ - check_good_uuid(mailbox_id) + check_good_uuid(mailbox_uuid) sql = ("DROP TABLE if exists {preffix}{name}".format( - preffix=self.table_preffix, name=sanitize(mailbox_id))) + preffix=self.table_preffix, name=sanitize(mailbox_uuid))) return self._query(sql) - def rename_table(self, oldmailbox, newmailbox): - """ - Delete the UID table for a given mailbox. - :param oldmailbox: the old mailbox name - :type oldmailbox: str - :param newmailbox: the new mailbox name - :type newmailbox: str - :rtype: Deferred - """ - assert oldmailbox - assert newmailbox - assert oldmailbox != newmailbox - sql = ("ALTER TABLE {preffix}{old} " - "RENAME TO {preffix}{new}".format( - preffix=self.table_preffix, - old=sanitize(oldmailbox), new=sanitize(newmailbox))) - return self._query(sql) - - def insert_doc(self, mailbox_id, doc_id): + def insert_doc(self, mailbox_uuid, doc_id): """ Insert the doc_id for a MetaMsg in the UID table for a given mailbox. @@ -148,11 +130,11 @@ class MailboxIndexer(object): document. :rtype: Deferred """ - check_good_uuid(mailbox_id) + check_good_uuid(mailbox_uuid) assert doc_id - mailbox_id = mailbox_id.replace('-', '_') + mailbox_uuid = mailbox_uuid.replace('-', '_') - if not re.findall(METAMSGID_RE.format(mbox_uuid=mailbox_id), doc_id): + if not re.findall(METAMSGID_RE.format(mbox_uuid=mailbox_uuid), doc_id): raise WrongMetaDocIDError("Wrong format for the MetaMsg doc_id") def get_rowid(result): @@ -160,12 +142,12 @@ class MailboxIndexer(object): sql = ("INSERT INTO {preffix}{name} VALUES (" "NULL, ?)".format( - preffix=self.table_preffix, name=sanitize(mailbox_id))) + preffix=self.table_preffix, name=sanitize(mailbox_uuid))) values = (doc_id,) sql_last = ("SELECT MAX(rowid) FROM {preffix}{name} " "LIMIT 1;").format( - preffix=self.table_preffix, name=sanitize(mailbox_id)) + preffix=self.table_preffix, name=sanitize(mailbox_uuid)) d = self._query(sql, values) d.addCallback(lambda _: self._query(sql_last)) @@ -173,25 +155,25 @@ class MailboxIndexer(object): d.addErrback(lambda f: f.printTraceback()) return d - def delete_doc_by_uid(self, mailbox_id, uid): + def delete_doc_by_uid(self, mailbox_uuid, uid): """ Delete the entry for a MetaMsg in the UID table for a given mailbox. - :param mailbox_id: the mailbox uuid + :param mailbox_uuid: the mailbox uuid :type mailbox: str :param uid: the UID of the message. :type uid: int :rtype: Deferred """ - check_good_uuid(mailbox_id) + check_good_uuid(mailbox_uuid) assert uid sql = ("DELETE FROM {preffix}{name} " "WHERE uid=?".format( - preffix=self.table_preffix, name=sanitize(mailbox_id))) + preffix=self.table_preffix, name=sanitize(mailbox_uuid))) values = (uid,) return self._query(sql, values) - def delete_doc_by_hash(self, mailbox_id, doc_id): + def delete_doc_by_hash(self, mailbox_uuid, doc_id): """ Delete the entry for a MetaMsg in the UID table for a given mailbox. @@ -199,7 +181,7 @@ class MailboxIndexer(object): M-- - :param mailbox_id: the mailbox uuid + :param mailbox_uuid: the mailbox uuid :type mailbox: str :param doc_id: the doc_id for the MetaMsg :type doc_id: str @@ -207,82 +189,80 @@ class MailboxIndexer(object): document. :rtype: Deferred """ - check_good_uuid(mailbox_id) + check_good_uuid(mailbox_uuid) assert doc_id sql = ("DELETE FROM {preffix}{name} " "WHERE hash=?".format( - preffix=self.table_preffix, name=sanitize(mailbox_id))) + preffix=self.table_preffix, name=sanitize(mailbox_uuid))) values = (doc_id,) return self._query(sql, values) - def get_doc_id_from_uid(self, mailbox_id, uid): + def get_doc_id_from_uid(self, mailbox_uuid, uid): """ Get the doc_id for a MetaMsg in the UID table for a given mailbox. - :param mailbox_id: the mailbox uuid + :param mailbox_uuid: the mailbox uuid :type mailbox: str :param uid: the uid for the MetaMsg for this mailbox :type uid: int :rtype: Deferred """ - check_good_uuid(mailbox_id) - mailbox_id = mailbox_id.replace('-', '_') + check_good_uuid(mailbox_uuid) + mailbox_uuid = mailbox_uuid.replace('-', '_') def get_hash(result): return _maybe_first_query_item(result) sql = ("SELECT hash from {preffix}{name} " "WHERE uid=?".format( - preffix=self.table_preffix, name=sanitize(mailbox_id))) + preffix=self.table_preffix, name=sanitize(mailbox_uuid))) values = (uid,) d = self._query(sql, values) d.addCallback(get_hash) return d - def get_uid_from_doc_id(self, mailbox_id, doc_id): - check_good_uuid(mailbox_id) - mailbox_id = mailbox_id.replace('-', '_') + def get_uid_from_doc_id(self, mailbox_uuid, doc_id): + check_good_uuid(mailbox_uuid) + mailbox_uuid = mailbox_uuid.replace('-', '_') def get_uid(result): return _maybe_first_query_item(result) sql = ("SELECT uid from {preffix}{name} " "WHERE hash=?".format( - preffix=self.table_preffix, name=sanitize(mailbox_id))) + preffix=self.table_preffix, name=sanitize(mailbox_uuid))) values = (doc_id,) d = self._query(sql, values) d.addCallback(get_uid) return d - - - def get_doc_ids_from_uids(self, mailbox_id, uids): + def get_doc_ids_from_uids(self, mailbox_uuid, uids): # For IMAP relative numbering /sequences. # XXX dereference the range (n,*) raise NotImplementedError() - def count(self, mailbox_id): + def count(self, mailbox_uuid): """ Get the number of entries in the UID table for a given mailbox. - :param mailbox_id: the mailbox uuid - :type mailbox_id: str + :param mailbox_uuid: the mailbox uuid + :type mailbox_uuid: str :return: a deferred that will fire with an integer returning the count. :rtype: Deferred """ - check_good_uuid(mailbox_id) + check_good_uuid(mailbox_uuid) def get_count(result): return _maybe_first_query_item(result) sql = ("SELECT Count(*) FROM {preffix}{name};".format( - preffix=self.table_preffix, name=sanitize(mailbox_id))) + preffix=self.table_preffix, name=sanitize(mailbox_uuid))) d = self._query(sql) d.addCallback(get_count) d.addErrback(lambda _: 0) return d - def get_next_uid(self, mailbox_id): + def get_next_uid(self, mailbox_uuid): """ Get the next integer beyond the highest UID count for a given mailbox. @@ -291,13 +271,13 @@ class MailboxIndexer(object): only thing that can be assured is that it will be equal or greater than the value returned. - :param mailbox_id: the mailbox uuid + :param mailbox_uuid: the mailbox uuid :type mailbox: str :return: a deferred that will fire with an integer returning the next uid. :rtype: Deferred """ - check_good_uuid(mailbox_id) + check_good_uuid(mailbox_uuid) def increment(result): uid = _maybe_first_query_item(result) @@ -305,18 +285,18 @@ class MailboxIndexer(object): return 1 return uid + 1 - d = self.get_last_uid(mailbox_id) + d = self.get_last_uid(mailbox_uuid) d.addCallback(increment) return d - def get_last_uid(self, mailbox_id): + def get_last_uid(self, mailbox_uuid): """ Get the highest UID for a given mailbox. """ - check_good_uuid(mailbox_id) + check_good_uuid(mailbox_uuid) sql = ("SELECT MAX(rowid) FROM {preffix}{name} " "LIMIT 1;").format( - preffix=self.table_preffix, name=sanitize(mailbox_id)) + preffix=self.table_preffix, name=sanitize(mailbox_uuid)) def getit(result): return _maybe_first_query_item(result) @@ -325,17 +305,17 @@ class MailboxIndexer(object): d.addCallback(getit) return d - def all_uid_iter(self, mailbox_id): + def all_uid_iter(self, mailbox_uuid): """ Get a sequence of all the uids in this mailbox. - :param mailbox_id: the mailbox uuid - :type mailbox_id: str + :param mailbox_uuid: the mailbox uuid + :type mailbox_uuid: str """ - check_good_uuid(mailbox_id) + check_good_uuid(mailbox_uuid) sql = ("SELECT uid from {preffix}{name} ").format( - preffix=self.table_preffix, name=sanitize(mailbox_id)) + preffix=self.table_preffix, name=sanitize(mailbox_uuid)) def get_results(result): return [x[0] for x in result] diff --git a/src/leap/mail/walk.py b/src/leap/mail/walk.py index 8653a5f..891abdc 100644 --- a/src/leap/mail/walk.py +++ b/src/leap/mail/walk.py @@ -122,7 +122,7 @@ def walk_msg_tree(parts, body_phash=None): documents that will be stored in Soledad. It walks down the subparts in the parsed message tree, and collapses - the leaf docuents into a wrapper document until no multipart submessages + the leaf documents into a wrapper document until no multipart submessages are left. To achieve this, it iteratively calculates a wrapper vector of all documents in the sequence that have more than one part and have unitary documents to their right. To collapse a multipart, take as many @@ -171,7 +171,7 @@ def walk_msg_tree(parts, body_phash=None): HEADERS: dict(parts[wind][HEADERS]) } - # remove subparts and substitue wrapper + # remove subparts and substitute wrapper map(lambda i: parts.remove(i), slic) parts[wind] = cwra -- cgit v1.2.3