Barry Warsaw pushed to branch release-3.0 at mailman / Mailman
Commits:
7aae8ca5 by Aurélien Bompard at 2015-12-02T22:36:06Z
Handle deleting nonexistent messages from the message store. Closes: #167
- - - - -
5 changed files:
- src/mailman/app/tests/test_moderation.py
- src/mailman/docs/NEWS.rst
- src/mailman/interfaces/messages.py
- src/mailman/model/messagestore.py
- src/mailman/model/tests/test_messagestore.py
Changes:
=====================================
src/mailman/app/tests/test_moderation.py
=====================================
--- a/src/mailman/app/tests/test_moderation.py
+++ b/src/mailman/app/tests/test_moderation.py
@@ -154,6 +154,15 @@ Message-ID: <alpha>
self.assertEqual(messages[0].msgdata['recipients'],
['z...@example.com'])
+ def test_survive_a_deleted_message(self):
+ # When the message that should be deleted is not found in the store,
+ # no error is raised.
+ request_id = hold_message(self._mlist, self._msg)
+ message_store = getUtility(IMessageStore)
+ message_store.delete_message('<alpha>')
+ handle_message(self._mlist, request_id, Action.discard)
+ self.assertEqual(self._request_db.count, 0)
+
class TestUnsubscription(unittest.TestCase):
=====================================
src/mailman/docs/NEWS.rst
=====================================
--- a/src/mailman/docs/NEWS.rst
+++ b/src/mailman/docs/NEWS.rst
@@ -20,6 +20,13 @@ Bugs
Given by Aurélien Bompard.
* Allow mailing lists to have localhost names with a suffix matching the
subcommand extensions. Given by Aurélien Bompard. (Closes: #168)
+ * Don't traceback if a nonexistent message-id is deleted from the message
+ store. Given by Aurélien Bompard, tweaked by Barry Warsaw. (Closes: #167)
+
+Interfaces
+----------
+ * ``IMessageStore.delete_message()`` no longer raises a ``LookupError`` when
+ you attempt to delete a nonexistent message from the message store.
REST
----
=====================================
src/mailman/interfaces/messages.py
=====================================
--- a/src/mailman/interfaces/messages.py
+++ b/src/mailman/interfaces/messages.py
@@ -89,8 +89,10 @@ class IMessageStore(Interface):
def delete_message(message_id):
"""Remove the given message from the store.
- :param message: The Message-ID of the mesage to delete from the store.
- :raises LookupError: if there is no such message.
+ If the referenced message is missing from the message store, the
+ operation is ignored.
+
+ :param message: The Message-ID of the message to delete from the store.
"""
messages = Attribute(
=====================================
src/mailman/model/messagestore.py
=====================================
--- a/src/mailman/model/messagestore.py
+++ b/src/mailman/model/messagestore.py
@@ -129,8 +129,12 @@ class MessageStore:
@dbconnection
def delete_message(self, store, message_id):
row = store.query(Message).filter_by(message_id=message_id).first()
- if row is None:
- raise LookupError(message_id)
- path = os.path.join(config.MESSAGES_DIR, row.path)
- os.remove(path)
- store.delete(row)
+ if row is not None:
+ path = os.path.join(config.MESSAGES_DIR, row.path)
+ # It's possible that a race condition caused the file system path
+ # to already be deleted.
+ try:
+ os.remove(path)
+ except FileNotFoundError:
+ pass
+ store.delete(row)
=====================================
src/mailman/model/tests/test_messagestore.py
=====================================
--- a/src/mailman/model/tests/test_messagestore.py
+++ b/src/mailman/model/tests/test_messagestore.py
@@ -22,9 +22,12 @@ __all__ = [
]
+import os
import unittest
+from mailman.config import config
from mailman.interfaces.messages import IMessageStore
+from mailman.model.message import Message
from mailman.testing.helpers import (
specialized_message_from_string as mfs)
from mailman.testing.layers import ConfigLayer
@@ -51,15 +54,15 @@ This message is very important.
def test_get_message_by_hash(self):
# Messages have an X-Message-ID-Hash header, the value of which can be
# used to look the message up in the message store.
- message = mfs("""\
+ msg = mfs("""\
Subject: An important message
Message-ID: <ant>
This message is very important.
""")
- add_message_hash(message)
- self._store.add(message)
- self.assertEqual(message['x-message-id-hash'],
+ add_message_hash(msg)
+ self._store.add(msg)
+ self.assertEqual(msg['x-message-id-hash'],
'V3YEHAFKE2WVJNK63Z7RFP4JMHISI2RG')
found = self._store.get_message_by_hash(
'V3YEHAFKE2WVJNK63Z7RFP4JMHISI2RG')
@@ -67,5 +70,33 @@ This message is very important.
self.assertEqual(found['x-message-id-hash'],
'V3YEHAFKE2WVJNK63Z7RFP4JMHISI2RG')
- def test_cannot_delete_missing_message(self):
- self.assertRaises(LookupError, self._store.delete_message, 'missing')
+ def test_can_delete_missing_message(self):
+ # Deleting a message which isn't in the store does not raise an
+ # exception.
+ msg = mfs("""\
+Message-ID: <ant>
+
+""")
+ self._store.add(msg)
+ self.assertEqual(len(list(self._store.messages)), 1)
+ self._store.delete_message('missing')
+ self.assertEqual(len(list(self._store.messages)), 1)
+
+ def test_can_survive_missing_message_path(self):
+ # Deleting a message which is in the store, but which doesn't appear
+ # on the file system does not raise an exception, but still removes
+ # the message from the store.
+ msg = mfs("""\
+Message-ID: <ant>
+
+""")
+ self._store.add(msg)
+ self.assertEqual(len(list(self._store.messages)), 1)
+ # We have to use the SQLAlchemy API because the .get_message_by_*()
+ # methods return email Message objects, not IMessages. The former
+ # does not give us the path to the object on the file system.
+ row = config.db.store.query(Message).filter_by(
+ message_id='<ant>').first()
+ os.remove(os.path.join(config.MESSAGES_DIR, row.path))
+ self._store.delete_message('<ant>')
+ self.assertEqual(len(list(self._store.messages)), 0)
View it on GitLab:
https://gitlab.com/mailman/mailman/commit/7aae8ca53fa037efe5629115ea815b4847f0141b
_______________________________________________
Mailman-checkins mailing list
Mailman-checkins@python.org
Unsubscribe:
https://mail.python.org/mailman/options/mailman-checkins/archive%40jab.org
