From a4bbc7c4fcea5596ab9f5a3d82983ddcf6d25909 Mon Sep 17 00:00:00 2001 From: Barry Warsaw Date: Wed, 6 Jan 2016 22:43:04 -0500 Subject: [PATCH] Additional refactoring to use the QuerySequence wrapper, so that we can still use len() and slicing on SQLAlchemy query results. We also don't need to list()-ify the results in the tests. This isn't perfect, but at least it doesn't introduce yet another layer violation. --- src/mailman/app/subscriptions.py | 11 ++++++-- src/mailman/app/tests/test_subscriptions.py | 16 +++++------ src/mailman/commands/eml_membership.py | 2 +- src/mailman/rest/helpers.py | 17 +++++------- src/mailman/rest/lists.py | 13 ++++----- src/mailman/rest/members.py | 2 +- src/mailman/rest/post_moderation.py | 3 ++- src/mailman/runners/tests/test_join.py | 2 +- src/mailman/utilities/queries.py | 42 +++++++++++++++++++++++++++++ 9 files changed, 75 insertions(+), 33 deletions(-) create mode 100644 src/mailman/utilities/queries.py diff --git a/src/mailman/app/subscriptions.py b/src/mailman/app/subscriptions.py index 861a12ec0..8636e399a 100644 --- a/src/mailman/app/subscriptions.py +++ b/src/mailman/app/subscriptions.py @@ -52,7 +52,9 @@ from mailman.model.member import Member from mailman.model.user import User from mailman.utilities.datetime import now from mailman.utilities.i18n import make +from mailman.utilities.queries import QuerySequence from operator import attrgetter +from sqlalchemy.orm.exc import NoResultFound, MultipleResultsFound from zope.component import getUtility from zope.event import notify from zope.interface import implementer @@ -407,8 +409,13 @@ class SubscriptionService: q_address = q_address.filter(Member.role == role) q_user = q_user.filter(Member.role == role) # Do a UNION of the two queries, sort the result and generate Members. - query = q_address.union(q_user).order_by(*order).from_self(Member) - return query + try: + query = q_address.union(q_user).order_by(*order).from_self(Member) + except NoResultFound: + query = None + except MultipleResultsFound: + raise AssertionError('Too many matches') + return QuerySequence(query) def __iter__(self): for member in self.get_members(): diff --git a/src/mailman/app/tests/test_subscriptions.py b/src/mailman/app/tests/test_subscriptions.py index 0eefbd62c..229c85aa8 100644 --- a/src/mailman/app/tests/test_subscriptions.py +++ b/src/mailman/app/tests/test_subscriptions.py @@ -658,7 +658,7 @@ class TestSubscriptionService(unittest.TestCase): address = self._user_manager.create_address( 'anne@example.com', 'Anne Address') self._mlist.subscribe(address) - members = list(self._service.find_members('anne@example.com')) + members = self._service.find_members('anne@example.com') self.assertEqual(len(members), 1) self.assertEqual(members[0].address, address) @@ -671,7 +671,7 @@ class TestSubscriptionService(unittest.TestCase): user.preferred_address = address # Subscribe the address. self._mlist.subscribe(address) - members = list(self._service.find_members('anne@example.com')) + members = self._service.find_members('anne@example.com') self.assertEqual(len(members), 1) self.assertEqual(members[0].user, user) @@ -684,7 +684,7 @@ class TestSubscriptionService(unittest.TestCase): user.preferred_address = address # Subscribe the user. self._mlist.subscribe(user) - members = list(self._service.find_members('anne@example.com')) + members = self._service.find_members('anne@example.com') self.assertEqual(len(members), 1) self.assertEqual(members[0].user, user) @@ -702,7 +702,7 @@ class TestSubscriptionService(unittest.TestCase): # Subscribe the user. self._mlist.subscribe(user) # Search for the secondary address. - members = list(self._service.find_members('anne2@example.com')) + members = self._service.find_members('anne2@example.com') self.assertEqual(len(members), 1) self.assertEqual(members[0].user, user) @@ -723,7 +723,7 @@ class TestSubscriptionService(unittest.TestCase): # Subscribe the secondary address. self._mlist.subscribe(address_2) # Search for the primary address. - members = list(self._service.find_members('anne@example.com')) + members = self._service.find_members('anne@example.com') self.assertEqual(len(members), 0) def test_find_member_user_id(self): @@ -735,7 +735,7 @@ class TestSubscriptionService(unittest.TestCase): user.preferred_address = address # Subscribe the user. self._mlist.subscribe(user) - members = list(self._service.find_members(user.user_id)) + members = self._service.find_members(user.user_id) self.assertEqual(len(members), 1) self.assertEqual(members[0].user, user) @@ -759,7 +759,7 @@ class TestSubscriptionService(unittest.TestCase): address_3.user = user # Subscribe the secondary address only. self._mlist.subscribe(address_2) - members = list(self._service.find_members(user.user_id)) + members = self._service.find_members(user.user_id) self.assertEqual(len(members), 1) self.assertEqual(members[0].address, address_2) @@ -812,7 +812,7 @@ class TestSubscriptionService(unittest.TestCase): mlist1.subscribe(address_3, MemberRole.owner) # The results should be sorted first by list id, then by address, then # by member role. - members = list(self._service.find_members(user.user_id)) + members = self._service.find_members(user.user_id) self.assertEqual(len(members), 21) self.assertListEqual( [(m.list_id.partition('.')[0], diff --git a/src/mailman/commands/eml_membership.py b/src/mailman/commands/eml_membership.py index a9dfe56e2..49af269e4 100644 --- a/src/mailman/commands/eml_membership.py +++ b/src/mailman/commands/eml_membership.py @@ -105,7 +105,7 @@ used. # matching memberships. members = getUtility(ISubscriptionService).find_members( email, mlist.list_id, MemberRole.member) - if members.count() > 0: + if len(members) > 0: print(_('$person is already a member'), file=results) return ContinueProcessing.yes subscriber = match_subscriber(email, display_name) diff --git a/src/mailman/rest/helpers.py b/src/mailman/rest/helpers.py index a7bbc2bae..6a1408988 100644 --- a/src/mailman/rest/helpers.py +++ b/src/mailman/rest/helpers.py @@ -139,16 +139,14 @@ class CollectionMixin: return etag(resource) def _get_collection(self, request): - """Return the collection as a list-like object. + """Return the collection as a sequence. - The returned value must support iteration and slicing. It can for - example be a concrete list or an SQLAlchemy request. - - This method must be implemented by subclasses. + The returned value must support the collections.abc.Sequence + API. This method must be implemented by subclasses. :param request: An http request. :return: The collection - :rtype: list + :rtype: collections.abc.Sequence """ raise NotImplementedError @@ -165,12 +163,9 @@ class CollectionMixin: # get turned into HTTP 400 errors. count = request.get_param_as_int('count', min=0) page = request.get_param_as_int('page', min=1) - try: - total_size = collection.count() - except TypeError: - total_size = len(collection) + total_size = len(collection) if count is None and page is None: - return 0, total_size, list(collection) + return 0, total_size, collection list_start = (page - 1) * count list_end = page * count return list_start, total_size, collection[list_start:list_end] diff --git a/src/mailman/rest/lists.py b/src/mailman/rest/lists.py index 70f100f25..a4d2da56a 100644 --- a/src/mailman/rest/lists.py +++ b/src/mailman/rest/lists.py @@ -47,7 +47,6 @@ from mailman.rest.members import AMember, MemberCollection from mailman.rest.post_moderation import HeldMessages from mailman.rest.sub_moderation import SubscriptionRequests from mailman.rest.validator import Validator -from sqlalchemy.orm.exc import NoResultFound, MultipleResultsFound from zope.component import getUtility @@ -152,14 +151,12 @@ class AList(_ListBase): """Return a single member representation.""" if self._mlist is None: return NotFound(), [] - try: - the_member = getUtility(ISubscriptionService).find_members( - email, self._mlist.list_id, role).one() - except NoResultFound: + members = getUtility(ISubscriptionService).find_members( + email, self._mlist.list_id, role) + if len(members) == 0: return NotFound(), [] - except MultipleResultsFound: - raise AssertionError('Too many matches') - return AMember(request.context['api_version'], the_member.member_id) + assert len(members) == 1, 'Too many matches' + return AMember(request.context['api_version'], members[0].member_id) @child(roster_matcher) def roster(self, request, segments, role): diff --git a/src/mailman/rest/members.py b/src/mailman/rest/members.py index b41afe18a..12a413f42 100644 --- a/src/mailman/rest/members.py +++ b/src/mailman/rest/members.py @@ -31,7 +31,7 @@ from mailman.interfaces.address import IAddress, InvalidEmailAddressError from mailman.interfaces.listmanager import IListManager from mailman.interfaces.member import ( AlreadySubscribedError, DeliveryMode, MemberRole, MembershipError, - MembershipIsBannedError, MissingPreferredAddressError, NotAMemberError) + MembershipIsBannedError, MissingPreferredAddressError) from mailman.interfaces.registrar import IRegistrar from mailman.interfaces.subscriptions import ( ISubscriptionService, RequestRecord, TokenOwner) diff --git a/src/mailman/rest/post_moderation.py b/src/mailman/rest/post_moderation.py index ca3e228ba..746503ba9 100644 --- a/src/mailman/rest/post_moderation.py +++ b/src/mailman/rest/post_moderation.py @@ -31,6 +31,7 @@ from mailman.rest.helpers import ( CollectionMixin, bad_request, child, etag, no_content, not_found, okay, path_to) from mailman.rest.validator import Validator, enum_validator +from mailman.utilities.queries import QuerySequence from zope.component import getUtility @@ -148,7 +149,7 @@ class HeldMessages(_HeldMessageBase, CollectionMixin): def _get_collection(self, request): requests = IListRequests(self._mlist) - return requests.of_type(RequestType.held_message) + return QuerySequence(requests.of_type(RequestType.held_message)) def on_get(self, request, response): """/lists/listname/held""" diff --git a/src/mailman/runners/tests/test_join.py b/src/mailman/runners/tests/test_join.py index 72bf97b23..239d8e2ef 100644 --- a/src/mailman/runners/tests/test_join.py +++ b/src/mailman/runners/tests/test_join.py @@ -167,7 +167,7 @@ class TestJoinWithDigests(unittest.TestCase): # digest deliveries. members = getUtility(ISubscriptionService).find_members( 'anne@example.org') - self.assertEqual(members.count(), 1) + self.assertEqual(len(members), 1) self.assertEqual(rmember, members[0]) return rmember diff --git a/src/mailman/utilities/queries.py b/src/mailman/utilities/queries.py new file mode 100644 index 000000000..6f4bc6b4d --- /dev/null +++ b/src/mailman/utilities/queries.py @@ -0,0 +1,42 @@ +# Copyright (C) 2016 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 . + +"""Some helpers for queries.""" + +__all__ = [ + 'QuerySequence', + ] + + +from collections.abc import Sequence + + +class QuerySequence(Sequence): + def __init__(self, query=None): + super().__init__() + self._query = query + self._cached_results = None + + def __len__(self): + return (0 if self._query is None else self._query.count()) + + def __getitem__(self, index): + if self._query is None: + raise IndexError('index out of range') + if self._cached_results is None: + self._cached_results = list(self._query) + return self._cached_results[index] -- 2.11.4.GIT