From 15238cb5683eb9a0eab9dcd251f509a693a22451 Mon Sep 17 00:00:00 2001 From: Aurélien Bompard Date: Tue, 2 Feb 2016 11:49:00 +0100 Subject: 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. --- src/mailman/chains/tests/test_headers.py | 18 +-- src/mailman/config/configure.zcml | 4 +- .../versions/d4fbb4fd34ca_header_match_order.py | 40 +++++ src/mailman/database/tests/test_migrations.py | 6 +- src/mailman/interfaces/mailinglist.py | 59 ++++++- src/mailman/model/mailinglist.py | 129 +++++++++++++-- src/mailman/model/tests/test_mailinglist.py | 176 +++++++++++++++++---- src/mailman/rules/docs/header-matching.rst | 8 +- src/mailman/utilities/importer.py | 6 +- 9 files changed, 378 insertions(+), 68 deletions(-) create mode 100644 src/mailman/database/alembic/versions/d4fbb4fd34ca_header_match_order.py diff --git a/src/mailman/chains/tests/test_headers.py b/src/mailman/chains/tests/test_headers.py index ff42feb95..d42baa55e 100644 --- a/src/mailman/chains/tests/test_headers.py +++ b/src/mailman/chains/tests/test_headers.py @@ -30,7 +30,7 @@ from mailman.config import config from mailman.core.chains import process from mailman.email.message import Message from mailman.interfaces.chain import LinkAction, HoldEvent -from mailman.interfaces.mailinglist import IHeaderMatchSet +from mailman.interfaces.mailinglist import IHeaderMatchList from mailman.testing.helpers import ( LogFileMark, configuration, event_subscribers, specialized_message_from_string as mfs) @@ -146,8 +146,8 @@ class TestHeaderChain(unittest.TestCase): # Test that the header-match chain has the header checks from the # mailing-list configuration. chain = config.chains['header-match'] - header_matches = IHeaderMatchSet(self._mlist) - header_matches.add('Foo', 'a+') + header_matches = IHeaderMatchList(self._mlist) + header_matches.append('Foo', 'a+') links = [link for link in chain.get_links(self._mlist, Message(), {}) if link.rule.name != 'any'] self.assertEqual(len(links), 1) @@ -159,10 +159,10 @@ class TestHeaderChain(unittest.TestCase): # Test that the mailing-list header-match complex rules are read # properly. chain = config.chains['header-match'] - header_matches = IHeaderMatchSet(self._mlist) - header_matches.add('Foo', 'a+', 'reject') - header_matches.add('Bar', 'b+', 'discard') - header_matches.add('Baz', 'z+', 'accept') + header_matches = IHeaderMatchList(self._mlist) + header_matches.append('Foo', 'a+', 'reject') + header_matches.append('Bar', 'b+', 'discard') + header_matches.append('Baz', 'z+', 'accept') links = [link for link in chain.get_links(self._mlist, Message(), {}) if link.rule.name != 'any'] self.assertEqual(len(links), 3) @@ -192,8 +192,8 @@ MIME-Version: 1.0 A message body. """) msgdata = {} - header_matches = IHeaderMatchSet(self._mlist) - header_matches.add('Foo', 'foo', 'accept') + header_matches = IHeaderMatchList(self._mlist) + header_matches.append('Foo', 'foo', 'accept') # This event subscriber records the event that occurs when the message # is processed by the owner chain. events = [] diff --git a/src/mailman/config/configure.zcml b/src/mailman/config/configure.zcml index b717e125f..535cf729f 100644 --- a/src/mailman/config/configure.zcml +++ b/src/mailman/config/configure.zcml @@ -36,8 +36,8 @@ = 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] diff --git a/src/mailman/rules/docs/header-matching.rst b/src/mailman/rules/docs/header-matching.rst index 6618d9cc9..4e6c0853d 100644 --- a/src/mailman/rules/docs/header-matching.rst +++ b/src/mailman/rules/docs/header-matching.rst @@ -131,9 +131,9 @@ action. The list administrator wants to match not on four stars, but on three plus signs, but only for the current mailing list. - >>> from mailman.interfaces.mailinglist import IHeaderMatchSet - >>> header_matches = IHeaderMatchSet(mlist) - >>> header_matches.add('x-spam-score', '[+]{3,}') + >>> from mailman.interfaces.mailinglist import IHeaderMatchList + >>> header_matches = IHeaderMatchList(mlist) + >>> header_matches.append('x-spam-score', '[+]{3,}') A message with a spam score of two pluses does not match. @@ -178,7 +178,7 @@ Now, the list administrator wants to match on three plus signs, but wants those emails to be discarded instead of held. >>> header_matches.remove('x-spam-score', '[+]{3,}') - >>> header_matches.add('x-spam-score', '[+]{3,}', 'discard') + >>> header_matches.append('x-spam-score', '[+]{3,}', 'discard') A message with a spam score of three pluses will still match, and the message will be discarded. diff --git a/src/mailman/utilities/importer.py b/src/mailman/utilities/importer.py index 59f4255b2..52de967cf 100644 --- a/src/mailman/utilities/importer.py +++ b/src/mailman/utilities/importer.py @@ -41,7 +41,7 @@ from mailman.interfaces.bans import IBanManager from mailman.interfaces.bounce import UnrecognizedBounceDisposition from mailman.interfaces.digests import DigestFrequency from mailman.interfaces.languages import ILanguageManager -from mailman.interfaces.mailinglist import IAcceptableAliasSet, IHeaderMatchSet +from mailman.interfaces.mailinglist import IAcceptableAliasSet, IHeaderMatchList from mailman.interfaces.mailinglist import Personalization, ReplyToMunging from mailman.interfaces.mailinglist import SubscriptionPolicy from mailman.interfaces.member import DeliveryMode, DeliveryStatus, MemberRole @@ -333,7 +333,7 @@ def import_config_pck(mlist, config_dict): # expression. Make that explicit for MM3. alias_set.add('^' + address) # Handle header_filter_rules conversion to header_matches. - header_match_set = IHeaderMatchSet(mlist) + header_matches = IHeaderMatchList(mlist) header_filter_rules = config_dict.get('header_filter_rules', []) for line_patterns, action, _unused in header_filter_rules: try: @@ -374,7 +374,7 @@ def import_config_pck(mlist, config_dict): 'invalid regular expression: %r', line_pattern) continue try: - header_match_set.add(header, pattern, chain) + header_matches.append(header, pattern, chain) except ValueError: log.warning('Skipping duplicate header_filter rule: %r', line_pattern) -- cgit v1.3.1