Commit 054b412a authored by Aurélien Bompard's avatar Aurélien Bompard Committed by Barry Warsaw

Handle deleting nonexistent messages from the message store. Closes: #167

parent 015a498f
Pipeline #372321 passed with stage
......@@ -153,6 +153,15 @@ Message-ID: <alpha>
['[email protected]'])
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)
handle_message(self._mlist, request_id, Action.discard)
self.assertEqual(self._request_db.count, 0)
class TestUnsubscription(unittest.TestCase):
......@@ -50,6 +50,8 @@ Bugs
addresses. 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)
......@@ -71,6 +73,8 @@ Interfaces
`X-Message-ID-Hash` although the latter is still included for backward
compatibility. Also be sure that all places which add the header use the
same algorithm. (Closes #118)
* ``IMessageStore.delete_message()`` no longer raises a ``LookupError`` when
you attempt to delete a nonexistent message from the message store.
Internal API
......@@ -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(
......@@ -125,8 +125,12 @@ class MessageStore:
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)
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.
except FileNotFoundError:
......@@ -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.
found = self._store.get_message_by_hash(
......@@ -67,8 +70,36 @@ This message is very important.
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.assertEqual(len(list(self._store.messages)), 1)
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.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 =
os.remove(os.path.join(config.MESSAGES_DIR, row.path))
self.assertEqual(len(list(self._store.messages)), 0)
def test_message_id_hash(self):
# The new specification calls for a Message-ID-Hash header,
Markdown is supported
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment