Merge lp:~toshio/mailman/maildir into lp:mailman

[Toshio Kuratomi]

Change the prototype archiver to save to maildir.

Add an archive_dir config path

[Barry Warsaw (barry)]

We did a face-to-face review explaining my strange import line sorting rules. Toshio is going to take a crack at adding a unittest (and the src/mailman/archiving/docs/common.rst test will need fixing). Otherwise, the branch looks good and I'll merge it when the updates are pushed. Thanks!

review: Needs Fixing

[lp:~toshio/mailman/maildir]

7108. By toshio <email address hidden> on 2012-03-13

Style cleanups for the prototype patch

7109. By toshio <email address hidden> on 2012-03-13

Update documentation for the prototype archiver

7110. By toshio <email address hidden> on 2012-03-13

Use 2.6+ syntax for assigning an exception

7111. By toshio <email address hidden> on 2012-03-14

Unittests for the prototype archiver

7112. By toshio <email address hidden> on 2012-03-14

Revert to using a try: except: finally: for locking as the context manager
fails when the lock is stolen out from under it (as from a thread)

7113. By toshio <email address hidden> on 2012-03-14

Merge with upstream

[Barry Warsaw (barry)]

Great branch, thanks Toshio. There are a few minor things I'd like to clean up on the branch, but I'm happy to do that and land this for you. I'll mention them here just for future reference.

In common.rst:
    >>> len (os.listdir(archivepath)) >= 1
    True

it's better to write it like the following (aside from the PEP 8 violation of spaces after the 'len' :) so that it's easier to debug:

    >>> len(os.listdir(archive_path))
    1

Also, there's no need to re-import os (since it's imported earlier in the file) or to import config, since the doctest framework injects that into the doctest globals.

Minor nit: two blank lines between major sections in rst and .py files.

In prototype.py:

hashlib and base64 modules are unused (pyflakes ftw! :). Very minor nit: I like using just `log` when there's only one logger in a module.

I prefer full sentences (e.g. capitalized and period at the end) for docstrings and comments. I generally prefer `error` as targets for except clauses. Abbreviations in general make for less readable code, though there are occasional exceptions <wink>.

I wonder if config.py's ensure_directories_exist() should create the maildir for the prototype archiver? This would avoid having the archiver do this every time a message is added to the archive, where it will almost always fail with EEXIST. It would have to be part of the IArchive interface though to keep things separated, possibly a `prepare()` method that gets called on startup. OTOH, it's possible that an archiver will have to do additional preparation specifically for the mailing list, and it's probably not a good idea to have to call this every time a mailing list is created. So I guess for now, having archive_message() create the directory is the best choice. We can't add an __init__() to the archiver though because it never gets instantiated (maybe that should change too). Just thinking out loud here.

I think I'll pull the os.path.join() out of the Lock() instantiation.

I'm kind of favoring .format() rather than % interpolation these days, but of course we have to use '{0}' instead of '{}' until we switch to Python 3 :). Python 2.6 doesn't support '{}'.

Do you think the lock timeout needs to be configurable?

I think I'm going to rewrite the comment above mailbox.add() and not store it in message_key. It makes pyflakes (since message_key) is unused, but the comment will preserve the fact that the return value could be useful in the future.

test_prototype:

Generally, I like sticking to PEP 8 for test class names, e.g. TestPrototypeArchiver. I've started to put these into the __all__ of the module. I'm not sure it matters, but it looks a bit better to me these days. ;)

I almost always prefer double-triple-quoted strings for multiline string delimiters.

For consistency, I generally use <email address hidden>, bart@, cris@, dave@, and so on. I'm a bit more schizo on message-ids. I think I'm converging on short alphabetical animal names, e.g. <ant>, <bee>, <cat>, <dog>, <eel>, and so on.

A better way to set tempdirs for tests, especially when they correspond to ini file variables, is to use the config.push() interface, which allows you to layer on...

review: Approve

[lp:~toshio/mailman/maildir]

7114. By toshio <email address hidden> on 2012-03-14

Python-2.6 compat fixes

=== modified file 'src/mailman/archiving/docs/common.rst'
--- src/mailman/archiving/docs/common.rst	2012-03-14 04:03:01 +0000
+++ src/mailman/archiving/docs/common.rst	2012-03-14 05:34:19 +0000
@@ -62,13 +62,17 @@
     >>> os.path.exists(pckpath)
     True
 
-Note however that the prototype archiver can't archive messages.
+The prototype archiver is available to simplistically archive messages.
+::
 
     >>> archivers['prototype'].archive_message(mlist, msg)
-    Traceback (most recent call last):
-    ...
-    NotImplementedError
 
+    >>> import os
+    >>> from mailman import config
+    >>> archivepath = os.path.join(config.ARCHIVE_DIR, 'prototype',
+    ...     mlist.fqdn_listname, 'new')
+    >>> len (os.listdir(archivepath)) >= 1
+    True
 
 The Mail-Archive.com
 ====================

=== modified file 'src/mailman/archiving/prototype.py'
--- src/mailman/archiving/prototype.py	2012-03-14 04:03:01 +0000
+++ src/mailman/archiving/prototype.py	2012-03-14 05:34:19 +0000
@@ -25,11 +25,23 @@
     ]
 
 
+import os
+import errno
+import hashlib
+import logging
+
+from base64 import b32encode
+from datetime import timedelta
+from mailbox import Maildir
 from urlparse import urljoin
+
+from flufl.lock import Lock, TimeOutError
 from zope.interface import implements
 
+from mailman.config import config
 from mailman.interfaces.archiver import IArchiver
 
+elog = logging.getLogger('mailman.error')
 
 

 class Prototype:
@@ -61,5 +73,39 @@
 
     @staticmethod
     def archive_message(mlist, message):
-        """See `IArchiver`."""
-        raise NotImplementedError
+        """See `IArchiver`.
+        
+        This sample archiver saves nmessages into a maildir
+        """
+        archive_dir = os.path.join(config.ARCHIVE_DIR, 'prototype')
+        try:
+            os.makedirs(archive_dir, 0775)
+        except OSError as e:
+            # If this already exists, then we're fine
+            if e.errno != errno.EEXIST:
+                raise
+
+        # Maildir will throw an error if the directories are partially created
+        # (for instance the toplevel exists but cur, new, or tmp do not)
+        # therefore we don't create the toplevel as we did above
+        list_dir = os.path.join(archive_dir, mlist.fqdn_listname)
+        mail_box = Maildir(list_dir, create=True, factory=None)
+
+        # Lock the maildir as Maildir.add() is not threadsafe
+        lock = Lock(os.path.join(config.LOCK_DIR, '%s-maildir.lock'
+            % mlist.fqdn_listname))
+        try:
+            lock.lock(timeout=timedelta(seconds=1))
+            # Add the message to the Maildir
+            # Message_key could be used to construct the file path if
+            # necessary::
+            #   os.path.join(archive_dir, mlist.fqdn_listname, 'new',
+            #           message_key)
+            message_key = mail_box.add(message)
+        except TimeOutError:
+            # log the error and go on
+            elog.error('Unable to lock archive for %s, discarded'
+                    ' message: %s' % (mlist.fqdn_listname, 
+                        message.get('message-id', '<unknown>')))
+        finally:
+            lock.unlock(unconditionally=True)

=== added directory 'src/mailman/archiving/tests'
=== added file 'src/mailman/archiving/tests/__init__.py'
=== added file 'src/mailman/archiving/tests/test_prototype.py'
--- src/mailman/archiving/tests/test_prototype.py	1970-01-01 00:00:00 +0000
+++ src/mailman/archiving/tests/test_prototype.py	2012-03-14 05:34:19 +0000
@@ -0,0 +1,151 @@
+# Copyright (C) 2012 by the Free Software Foundation, Inc.
+#
+# This file is part of GNU Mailman.
+#
+# GNU Mailman is free software: you can redistribute it and/or modify it under
+# the terms of the GNU General Public License as published by the Free
+# Software Foundation, either version 3 of the License, or (at your option)
+# any later version.
+#
+# GNU Mailman is distributed in the hope that it will be useful, but WITHOUT
+# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+# FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+# more details.
+#
+# You should have received a copy of the GNU General Public License along with
+# GNU Mailman.  If not, see <http://www.gnu.org/licenses/>.
+
+"""Test the prototype archiver."""
+
+from __future__ import absolute_import, print_function, unicode_literals
+
+__metaclass__ = type
+__all__ = [
+    ]
+
+import os
+import shutil
+import tempfile
+import unittest
+import threading
+
+from flufl.lock import Lock
+
+from mailman.app.lifecycle import create_list
+from mailman.archiving import prototype
+from mailman.config import config
+from mailman.testing.helpers import LogFileMark
+from mailman.testing.helpers import specialized_message_from_string as smfs
+from mailman.testing.layers import ConfigLayer
+
+
+class test_PrototypeArchiveMethod(unittest.TestCase):
+    layer = ConfigLayer
+
+    def setUp(self):
+        # Create a fake mailing list and message object
+        self.message = smfs('''\
+To: [e-mail]
+From: [e-mail]
+Subject: Testing the test list
+Message-ID: <[e-mail]>
+
+Tests are better than not tests
+but the water deserves to be swum
+                ''')
+
+        self.mlist = create_list('[e-mail]')
+        config.db.commit()
+
+        # Set some config directories where we won't bash real data
+        config.ARCHIVE_DIR = '%s%s' % (tempfile.mkdtemp(), os.path.sep)
+        config.LOCK_DIR = tempfile.mkdtemp()
+
+        # Structure of a maildir
+        self.expected_dir_structure = frozenset(
+                (os.path.join(config.ARCHIVE_DIR, f) for f in (
+                    '',
+                    'prototype',
+                    os.path.join('prototype', self.mlist.fqdn_listname),
+                    os.path.join('prototype', self.mlist.fqdn_listname, 'cur'),
+                    os.path.join('prototype', self.mlist.fqdn_listname, 'new'),
+                    os.path.join('prototype', self.mlist.fqdn_listname, 'tmp'),
+                    )
+                    )
+                )
+
+    def tearDown(self):
+        shutil.rmtree(config.ARCHIVE_DIR)
+        shutil.rmtree(config.LOCK_DIR)
+
+    def _find(self, path):
+        all_filenames = set()
+        for dirs in os.walk(path):
+            directory = dirs[0]
+            if not isinstance(directory, unicode):
+                directory = unicode(directory)
+            all_filenames.add(directory)
+            if dirs[2]:
+                for filename in dirs[2]:
+                    new_filename = os.path.join(dirs[0], filename)
+                    if not isinstance(new_filename, unicode):
+                        new_filename = unicode(new_filename)
+                    all_filenames.add(new_filename)
+        return all_filenames
+
+    def test_archive_maildir_created(self):
+        prototype.Prototype.archive_message(self.mlist, self.message)
+        all_filenames = self._find(config.ARCHIVE_DIR)
+        # Check that the directory structure has been created and we have one
+        # more file (the archived message) than expected directories
+        self.assertTrue(self.expected_dir_structure.issubset(all_filenames))
+        self.assertEqual(len(all_filenames), len(self.expected_dir_structure) + 1)
+
+    def test_archive_maildir_existance_does_not_raise(self):
+        os.makedirs(os.path.join(config.ARCHIVE_DIR, 'prototype',
+            self.mlist.fqdn_listname, 'cur'))
+        os.mkdir(os.path.join(config.ARCHIVE_DIR, 'prototype',
+            self.mlist.fqdn_listname, 'new'))
+        os.mkdir(os.path.join(config.ARCHIVE_DIR, 'prototype',
+            self.mlist.fqdn_listname, 'tmp'))
+
+        # Checking that no exception is raised in this circumstance because it
+        # will be the common case (adding a new message to an archive whose
+        # directories have alreay been created)
+        try:
+            prototype.Prototype.archive_message(self.mlist, self.message)
+        except:
+            self.assertTrue(False, 'Exception raised when the archive'
+                    ' directory structure already in place')
+
+    def test_archive_lock_used(self):
+        # Test that locking the maildir when adding works as a failure here
+        # could mean we lose mail
+        lock = Lock(os.path.join(config.LOCK_DIR, '%s-maildir.lock'
+            % self.mlist.fqdn_listname))
+        with lock:
+            # Take this lock.  Then make sure the archiver fails while that's
+            # working.
+            archive_thread = threading.Thread(
+                    target=prototype.Prototype.archive_message,
+                    args=(self.mlist, self.message))
+            mark = LogFileMark('mailman.error')
+            archive_thread.run()
+            # Test that the archiver output the correct error
+            line = mark.readline()
+            self.assertTrue(line.endswith('Unable to lock archive for %s,'
+                    ' discarded message: %s\n' % (self.mlist.fqdn_listname,
+                        self.message.get('message-id'))))
+
+        # Check that the file didn't get created
+        created_files = self._find(config.ARCHIVE_DIR)
+        self.assertEqual(self.expected_dir_structure, created_files)
+
+    def test_mail_added(self):
+        prototype.Prototype.archive_message(self.mlist, self.message)
+        for filename in os.listdir(os.path.join(config.ARCHIVE_DIR,
+                'prototype', self.mlist.fqdn_listname, 'new')):
+            # Check that the email has been added
+            email = open(os.path.join(config.ARCHIVE_DIR, 'prototype',
+                    self.mlist.fqdn_listname, 'new', filename))
+            self.assertTrue((repr(self.message)).endswith(email.read()))

=== modified file 'src/mailman/config/config.py'
--- src/mailman/config/config.py	2012-03-03 18:40:16 +0000
+++ src/mailman/config/config.py	2012-03-14 05:34:19 +0000
@@ -173,6 +173,7 @@
             lock_dir                = category.lock_dir,
             log_dir                 = category.log_dir,
             messages_dir            = category.messages_dir,
+            archive_dir             = category.archive_dir,
             pipermail_private_dir   = category.pipermail_private_dir,
             pipermail_public_dir    = category.pipermail_public_dir,
             queue_dir               = category.queue_dir,

=== modified file 'src/mailman/config/schema.cfg'
--- src/mailman/config/schema.cfg	2012-03-13 05:01:58 +0000
+++ src/mailman/config/schema.cfg	2012-03-14 05:34:19 +0000
@@ -113,6 +113,9 @@
 ext_dir: $var_dir/ext
 # Directory where the default IMessageStore puts its messages.
 messages_dir: $var_dir/messages
+# Directory for archive backends to store their archives in.
+# Archivers should create a subdirectory in here to store their files
+archive_dir: $var_dir/archives
 # Directory for public Pipermail archiver artifacts.
 pipermail_public_dir: $var_dir/archives/public
 # Directory for private Pipermail archiver artifacts.