The order of a mailing list's header matches is significant

Add a numerical index property to HeaderMatch objects, and change the
HeaderMatchSet manager to take the order into account.

Items can now be inserted and removed by index.

2 files changed, 264 insertions, 41 deletions

diff --git a/src/mailman/model/mailinglist.py b/src/mailman/model/mailinglist.py
index 4160f6cc7..29f4ce26d 100644
--- a/src/mailman/model/mailinglist.py
+++ b/src/mailman/model/mailinglist.py
@@ -37,7 +37,7 @@ from mailman.interfaces.digests import DigestFrequency
 from mailman.interfaces.domain import IDomainManager
 from mailman.interfaces.languages import ILanguageManager
 from mailman.interfaces.mailinglist import (
-    IAcceptableAlias, IAcceptableAliasSet, IHeaderMatch, IHeaderMatchSet,
+    IAcceptableAlias, IAcceptableAliasSet, IHeaderMatch, IHeaderMatchList,
     IListArchiver, IListArchiverSet, IMailingList, Personalization,
     ReplyToMunging, SubscriptionPolicy)
 from mailman.interfaces.member import (
@@ -188,6 +188,10 @@ class MailingList(Model):
     topics_bodylines_limit = Column(Integer)
     topics_enabled = Column(Boolean)
     welcome_message_uri = Column(Unicode)
+    # ORM relationships
+    header_matches = relationship(
+        'HeaderMatch', backref='mailing_list', cascade="all, delete-orphan",
+        order_by="HeaderMatch.index")
 
     def __init__(self, fqdn_listname):
         super().__init__()
@@ -631,30 +635,62 @@ class HeaderMatch(Model):
 
     id = Column(Integer, primary_key=True)
 
-    mailing_list_id = Column(Integer, ForeignKey('mailinglist.id'))
-    mailing_list = relationship('MailingList', backref='header_matches')
+    mailing_list_id = Column(
+        Integer, ForeignKey('mailinglist.id'),
+        index=True, nullable=False)
 
+    index = Column(Integer, index=True, default=0)
     header = Column(Unicode)
     pattern = Column(Unicode)
     chain = Column(Unicode, nullable=True)
 
+    @dbconnection
+    def move_to(self, store, index):
+        if index == self.index:
+            return # Nothing to do
+        elif index < self.index:
+            # Moving up: header matches between the new position and the
+            # current one must be moved down the list to make room. Those after
+            # the current position must not be changed.
+            for header_match in store.query(HeaderMatch).filter(
+                HeaderMatch.mailing_list == self.mailing_list,
+                HeaderMatch.index >= index,
+                HeaderMatch.index < self.index):
+                header_match.index = header_match.index + 1
+        elif index > self.index:
+            # Moving down: header matches between the current position and the
+            # new one must be moved up the list to make room. Those after
+            # the new position must not be changed.
+            for header_match in store.query(HeaderMatch).filter(
+                HeaderMatch.mailing_list == self.mailing_list,
+                HeaderMatch.index > self.index,
+                HeaderMatch.index <= index):
+                header_match.index = header_match.index - 1
+        self.index = index
+
 
 
-@implementer(IHeaderMatchSet)
-class HeaderMatchSet:
-    """See `IHeaderMatchSet`."""
+@implementer(IHeaderMatchList)
+class HeaderMatchList:
+    """See `IHeaderMatchList`.
+
+    All write operations must mark the mailing list's header_matches collection
+    as expired:
+    http://docs.sqlalchemy.org/en/latest/orm/session_state_management.html#refreshing-expiring
+    """
 
     def __init__(self, mailing_list):
         self._mailing_list = mailing_list
 
     @dbconnection
     def clear(self, store):
-        """See `IHeaderMatchSet`."""
+        """See `IHeaderMatchList`."""
         store.query(HeaderMatch).filter(
             HeaderMatch.mailing_list == self._mailing_list).delete()
+        store.expire(self._mailing_list, ['header_matches'])
 
     @dbconnection
-    def add(self, store, header, pattern, chain=None):
+    def append(self, store, header, pattern, chain=None):
         header = header.lower()
         existing = store.query(HeaderMatch).filter(
             HeaderMatch.mailing_list == self._mailing_list,
@@ -662,17 +698,35 @@ class HeaderMatchSet:
             HeaderMatch.pattern == pattern).count()
         if existing > 0:
             raise ValueError('Pattern already exists')
+        last_index = store.query(HeaderMatch.index).filter(
+            HeaderMatch.mailing_list == self._mailing_list
+            ).order_by(HeaderMatch.index.desc()).limit(1).scalar()
+        if last_index is None:
+            last_index = -1
         header_match = HeaderMatch(
             mailing_list=self._mailing_list,
-            header=header, pattern=pattern, chain=chain)
+            header=header, pattern=pattern, chain=chain,
+            index=last_index + 1)
         store.add(header_match)
+        store.expire(self._mailing_list, ['header_matches'])
+
+    @dbconnection
+    def insert(self, store, index, header, pattern, chain=None):
+        self.append(header, pattern, chain)
+        # Get the header match that was just added.
+        header_match = store.query(HeaderMatch).filter(
+            HeaderMatch.mailing_list == self._mailing_list,
+            HeaderMatch.header == header.lower(),
+            HeaderMatch.pattern == pattern,
+            HeaderMatch.chain == chain).one()
+        header_match.move_to(index)
+        store.expire(self._mailing_list, ['header_matches'])
 
     @dbconnection
     def remove(self, store, header, pattern):
         header = header.lower()
-        # Don't just filter and use delete(), or the MailingList.header_matches
-        # collection will not be updated:
-        # http://docs.sqlalchemy.org/en/rel_1_0/orm/collections.html#dynamic-relationship-loaders
+        # Query.delete() has many caveats, don't use it here:
+        # http://docs.sqlalchemy.org/en/rel_1_0/orm/query.html#sqlalchemy.orm.query.Query.delete
         try:
             existing = store.query(HeaderMatch).filter(
                 HeaderMatch.mailing_list == self._mailing_list,
@@ -681,9 +735,56 @@ class HeaderMatchSet:
         except NoResultFound:
             raise ValueError('Pattern does not exist')
         else:
-            self._mailing_list.header_matches.remove(existing)
+            store.delete(existing)
+        self._restore_index_sequence()
+        store.expire(self._mailing_list, ['header_matches'])
+
+    @dbconnection
+    def __getitem__(self, store, key):
+        if key < 0:
+            key = len(self) + key
+        try:
+            return store.query(HeaderMatch).filter(
+                HeaderMatch.mailing_list == self._mailing_list,
+                HeaderMatch.index == key).one()
+        except NoResultFound:
+            raise IndexError
+
+    @dbconnection
+    def __delitem__(self, store, key):
+        try:
+            existing = store.query(HeaderMatch).filter(
+                HeaderMatch.mailing_list == self._mailing_list,
+                HeaderMatch.index == key).one()
+        except NoResultFound:
+            raise IndexError
+        else:
+            store.delete(existing)
+        self._restore_index_sequence()
+        store.expire(self._mailing_list, ['header_matches'])
+
+    @dbconnection
+    def __len__(self, store):
+        return store.query(HeaderMatch).filter(
+            HeaderMatch.mailing_list == self._mailing_list).count()
 
     @dbconnection
     def __iter__(self, store):
         yield from store.query(HeaderMatch).filter(
-            HeaderMatch.mailing_list == self._mailing_list)
+            HeaderMatch.mailing_list == self._mailing_list
+            ).order_by(HeaderMatch.index)
+
+    @dbconnection
+    def _restore_index_sequence(self, store):
+        """Restore a continuous index sequence for this mailing list's header
+        matches.
+
+        The header match indexes may not be continuous after deleting an item.
+        It won't prevent this component from working properly, but it's cleaner
+        to restore a continuous sequence.
+        """
+        for index, header_match in enumerate(store.query(HeaderMatch).filter(
+            HeaderMatch.mailing_list == self._mailing_list
+            ).order_by(HeaderMatch.index)):
+            header_match.index = index
+        store.expire(self._mailing_list, ['header_matches'])
diff --git a/src/mailman/model/tests/test_mailinglist.py b/src/mailman/model/tests/test_mailinglist.py
index 4779382b6..ee8a724d5 100644
--- a/src/mailman/model/tests/test_mailinglist.py
+++ b/src/mailman/model/tests/test_mailinglist.py
@@ -32,7 +32,7 @@ from mailman.config import config
 from mailman.database.transaction import transaction
 from mailman.interfaces.listmanager import IListManager
 from mailman.interfaces.mailinglist import (
-    IAcceptableAliasSet, IHeaderMatchSet, IListArchiverSet)
+    IAcceptableAliasSet, IHeaderMatchList, IListArchiverSet)
 from mailman.interfaces.member import (
     AlreadySubscribedError, MemberRole, MissingPreferredAddressError)
 from mailman.interfaces.usermanager import IUserManager
@@ -200,56 +200,178 @@ class TestHeaderMatch(unittest.TestCase):
         self._mlist = create_list('ant@example.com')
 
     def test_lowercase_header(self):
-        header_matches = IHeaderMatchSet(self._mlist)
-        header_matches.add('Header', 'pattern')
+        header_matches = IHeaderMatchList(self._mlist)
+        header_matches.append('Header', 'pattern')
         self.assertEqual(len(self._mlist.header_matches), 1)
         self.assertEqual(self._mlist.header_matches[0].header, 'header')
 
     def test_chain_defaults_to_none(self):
-        header_matches = IHeaderMatchSet(self._mlist)
-        header_matches.add('header', 'pattern')
+        header_matches = IHeaderMatchList(self._mlist)
+        header_matches.append('header', 'pattern')
         self.assertEqual(len(self._mlist.header_matches), 1)
         self.assertEqual(self._mlist.header_matches[0].chain, None)
 
     def test_duplicate(self):
-        header_matches = IHeaderMatchSet(self._mlist)
-        header_matches.add('Header', 'pattern')
+        header_matches = IHeaderMatchList(self._mlist)
+        header_matches.append('Header', 'pattern')
         self.assertRaises(
-            ValueError, header_matches.add, 'Header', 'pattern')
+            ValueError, header_matches.append, 'Header', 'pattern')
         self.assertEqual(len(self._mlist.header_matches), 1)
 
     def test_remove_non_existent(self):
-        header_matches = IHeaderMatchSet(self._mlist)
+        header_matches = IHeaderMatchList(self._mlist)
         self.assertRaises(
             ValueError, header_matches.remove, 'header', 'pattern')
 
     def test_add_remove(self):
-        header_matches = IHeaderMatchSet(self._mlist)
-        header_matches.add('header', 'pattern')
+        header_matches = IHeaderMatchList(self._mlist)
+        header_matches.append('header1', 'pattern')
+        header_matches.append('header2', 'pattern')
+        self.assertEqual(len(self._mlist.header_matches), 2)
+        self.assertEqual(len(header_matches), 2)
+        header_matches.remove('header1', 'pattern')
         self.assertEqual(len(self._mlist.header_matches), 1)
-        header_matches.remove('header', 'pattern')
+        self.assertEqual(len(header_matches), 1)
+        del header_matches[0]
         self.assertEqual(len(self._mlist.header_matches), 0)
+        self.assertEqual(len(header_matches), 0)
 
     def test_iterator(self):
-        header_matches = IHeaderMatchSet(self._mlist)
-        header_matches.add('Header', 'pattern')
-        header_matches.add('Subject', 'patt.*')
-        header_matches.add('From', '.*@example.com', 'discard')
-        header_matches.add('From', '.*@example.org', 'accept')
-        matches = sorted((match.header, match.pattern, match.chain)
-                         for match in IHeaderMatchSet(self._mlist))
+        header_matches = IHeaderMatchList(self._mlist)
+        header_matches.append('Header', 'pattern')
+        header_matches.append('Subject', 'patt.*')
+        header_matches.append('From', '.*@example.com', 'discard')
+        header_matches.append('From', '.*@example.org', 'accept')
+        matches = [(match.header, match.pattern, match.chain)
+                   for match in IHeaderMatchList(self._mlist)]
         self.assertEqual(
-            matches,
-            [('from', '.*@example.com', 'discard'),
-             ('from', '.*@example.org', 'accept'),
-             ('header', 'pattern', None),
-             ('subject', 'patt.*', None),
-             ])
+            matches, [
+            ('header', 'pattern', None),
+            ('subject', 'patt.*', None),
+            ('from', '.*@example.com', 'discard'),
+            ('from', '.*@example.org', 'accept'),
+            ])
 
     def test_clear(self):
-        header_matches = IHeaderMatchSet(self._mlist)
-        header_matches.add('Header', 'pattern')
+        header_matches = IHeaderMatchList(self._mlist)
+        header_matches.append('Header', 'pattern')
         self.assertEqual(len(self._mlist.header_matches), 1)
         with transaction():
             header_matches.clear()
         self.assertEqual(len(self._mlist.header_matches), 0)
+
+    def test_get_by_index(self):
+        header_matches = IHeaderMatchList(self._mlist)
+        header_matches.append('header', 'pattern')
+        hm = header_matches[0]
+        self.assertEqual(hm.header, 'header')
+        self.assertEqual(hm.pattern, 'pattern')
+
+    def test_get_by_negative_index(self):
+        header_matches = IHeaderMatchList(self._mlist)
+        header_matches.append('header', 'pattern')
+        hm = header_matches[-1]
+        self.assertEqual(hm.header, 'header')
+        self.assertEqual(hm.pattern, 'pattern')
+
+    def test_get_non_existent_by_index(self):
+        header_matches = IHeaderMatchList(self._mlist)
+        with self.assertRaises(IndexError):
+            header_matches[0]
+
+    def test_move_up(self):
+        header_matches = IHeaderMatchList(self._mlist)
+        header_matches.append('header-0', 'pattern')
+        header_matches.append('header-1', 'pattern')
+        header_matches.append('header-2', 'pattern')
+        header_matches.append('header-3', 'pattern')
+        self.assertEqual(
+            [(match.header, match.index) for match in header_matches], [
+            ('header-0', 0),
+            ('header-1', 1),
+            ('header-2', 2),
+            ('header-3', 3),
+            ])
+        header_match_2 = self._mlist.header_matches[2]
+        self.assertEqual(header_match_2.index, 2)
+        header_match_2.move_to(1)
+        self.assertEqual(
+            [(match.header, match.index) for match in header_matches], [
+            ('header-0', 0),
+            ('header-2', 1),
+            ('header-1', 2),
+            ('header-3', 3),
+            ])
+
+    def test_move_down(self):
+        header_matches = IHeaderMatchList(self._mlist)
+        header_matches.append('header-0', 'pattern')
+        header_matches.append('header-1', 'pattern')
+        header_matches.append('header-2', 'pattern')
+        header_matches.append('header-3', 'pattern')
+        self.assertEqual(
+            [(match.header, match.index) for match in header_matches], [
+            ('header-0', 0),
+            ('header-1', 1),
+            ('header-2', 2),
+            ('header-3', 3),
+            ])
+        header_match_1 = self._mlist.header_matches[1]
+        self.assertEqual(header_match_1.index, 1)
+        header_match_1.move_to(2)
+        self.assertEqual(
+            [(match.header, match.index) for match in header_matches], [
+            ('header-0', 0),
+            ('header-2', 1),
+            ('header-1', 2),
+            ('header-3', 3),
+            ])
+
+    def test_move_identical(self):
+        header_matches = IHeaderMatchList(self._mlist)
+        header_matches.append('header-0', 'pattern')
+        header_matches.append('header-1', 'pattern')
+        header_matches.append('header-2', 'pattern')
+        self.assertEqual(
+            [(match.header, match.index) for match in header_matches],
+            [('header-0', 0), ('header-1', 1), ('header-2', 2)])
+        header_match_1 = self._mlist.header_matches[1]
+        self.assertEqual(header_match_1.index, 1)
+        header_match_1.move_to(1)
+        self.assertEqual(
+            [(match.header, match.index) for match in header_matches],
+            [('header-0', 0), ('header-1', 1), ('header-2', 2)])
+
+    def test_insert(self):
+        header_matches = IHeaderMatchList(self._mlist)
+        header_matches.append('header-0', 'pattern')
+        header_matches.append('header-1', 'pattern')
+        self.assertEqual(
+            [(match.header, match.index) for match in header_matches],
+            [('header-0', 0), ('header-1', 1)])
+        header_matches.insert(1, 'header-2', 'pattern')
+        self.assertEqual(
+            [(match.header, match.index) for match in header_matches],
+            [('header-0', 0), ('header-2', 1), ('header-1', 2)])
+
+    def test_rebuild_sequence_after_remove(self):
+        header_matches = IHeaderMatchList(self._mlist)
+        header_matches.append('header-0', 'pattern')
+        header_matches.append('header-1', 'pattern')
+        header_matches.append('header-2', 'pattern')
+        self.assertEqual(
+            [(match.header, match.index) for match in header_matches],
+            [('header-0', 0), ('header-1', 1), ('header-2', 2)])
+        del header_matches[0]
+        self.assertEqual(
+            [(match.header, match.index) for match in header_matches],
+            [('header-1', 0), ('header-2', 1)])
+        header_matches.remove('header-1', 'pattern')
+        self.assertEqual(
+            [(match.header, match.index) for match in header_matches],
+            [('header-2', 0)])
+
+    def test_remove_non_existent_by_index(self):
+        header_matches = IHeaderMatchList(self._mlist)
+        with self.assertRaises(IndexError):
+            del header_matches[0]