8 files changed, 85 insertions, 69 deletions

diff --git a/src/mailman/rest/docs/lists.rst b/src/mailman/rest/docs/lists.rst
index ce113c9f1..295e8c0b7 100644
--- a/src/mailman/rest/docs/lists.rst
+++ b/src/mailman/rest/docs/lists.rst
@@ -53,9 +53,11 @@ You can also query for lists from a particular domain.
 Paginating over list records
 ----------------------------
 
-List records, as well as user and member records can be requested in page
-chunks that are defined with the GET params ``count`` and ``page``, with 
-``count`` defining the length of the collection to be returned.
+Instead of returning all the list records at once, it's possible to return
+them in pages by adding the GET parameters ``count`` and ``page`` to the
+request URI.  Page 1 is the first page and ``count`` defines the size of the
+page.
+::
 
     >>> mlist = create_list('bird@example.com')
     >>> transaction.commit()
diff --git a/src/mailman/rest/docs/membership.rst b/src/mailman/rest/docs/membership.rst
index 7f0ffae9d..6deed7c48 100644
--- a/src/mailman/rest/docs/membership.rst
+++ b/src/mailman/rest/docs/membership.rst
@@ -206,8 +206,10 @@ We can also get just the members of a single mailing list.
 Paginating over member records
 ------------------------------
 
-Instead of returning all member records at once, it's possible to return
-the as pages.
+Instead of returning all the member records at once, it's possible to return
+them in pages by adding the GET parameters ``count`` and ``page`` to the
+request URI.  Page 1 is the first page and ``count`` defines the size of the
+page.
 
     >>> dump_json(
     ...     'http://localhost:9001/3.0/lists/ant@example.com/roster/member'
diff --git a/src/mailman/rest/docs/users.rst b/src/mailman/rest/docs/users.rst
index 93ae30d48..04533f578 100644
--- a/src/mailman/rest/docs/users.rst
+++ b/src/mailman/rest/docs/users.rst
@@ -65,8 +65,11 @@ returned in the REST API.
 Paginating over user records
 ----------------------------
 
-It's possible to return pagable chunks of all user records by adding 
-the GET params ``count`` and ``page`` to the request URI.
+Instead of returning all the user records at once, it's possible to return
+them in pages by adding the GET parameters ``count`` and ``page`` to the
+request URI.  Page 1 is the first page and ``count`` defines the size of the
+page.
+::
 
     >>> dump_json('http://localhost:9001/3.0/users?count=1&page=1')
     entry 0:
diff --git a/src/mailman/rest/helpers.py b/src/mailman/rest/helpers.py
index 3b64dc0dc..3a548cb10 100644
--- a/src/mailman/rest/helpers.py
+++ b/src/mailman/rest/helpers.py
@@ -41,7 +41,6 @@ from lazr.config import as_boolean
 from restish import http
 from restish.http import Response
 from restish.resource import MethodDecorator
-from urllib2 import HTTPError
 from webob.multidict import MultiDict
 
 from mailman.config import config
@@ -103,6 +102,7 @@ def etag(resource):
     """
     assert 'http_etag' not in resource, 'Resource already etagged'
     etag = hashlib.sha1(repr(resource)).hexdigest()
+
     resource['http_etag'] = '"{0}"'.format(etag)
     return json.dumps(resource, cls=ExtendedEncoder)
 
@@ -110,39 +110,32 @@ def etag(resource):
 def paginate(method):
     """Method decorator to paginate through collection result lists.
 
-    Use this to return only a slice of a collection, specified either
-    in the request itself or by the ``default_count`` argument.
-    ``default_count=None`` will return the whole collection if the request
-    contains no count/page parameters.
+    Use this to return only a slice of a collection, specified in the request
+    itself.  The request should use query parameters `count` and `page` to
+    specify the slice they want.  The slice will start at index
+    ``(page - 1) * count`` and end (exclusive) at ``(page * count)``.
 
-    :param default_count: The default page length if no count is specified.
-    :type default_count: int
-    :returns: Decorator function.
+    Decorated methods must take ``self`` and ``request`` as the first two
+    arguments.
     """
-    def wrapper(*args, **kwargs):
-        # args[0] is self.
-        # restish Request object is expected to be the second arg.
-        request = args[1]
-        # get the result
-        result = method(*args, **kwargs)
+    def wrapper(self, request, *args, **kwargs):
         try:
             count = int(request.GET['count'])
             page = int(request.GET['page'])
-            # Set indices
-            list_start = 0
-            list_end = None
-            # slice list only if count is not None
-            if count is not None:
-                list_start = int((page - 1) * count)
-                list_end = int(page * count)
-                return result[list_start:list_end]
+            if count < 0 or page < 0:
+                return http.bad_request([], b'Invalid parameters')
         # Wrong parameter types or no GET attribute in request object.
         except (AttributeError, ValueError, TypeError):
             return http.bad_request([], b'Invalid parameters')
-        # No count/page params
+        # No count/page params.
         except KeyError:
-            pass
-        return result
+            count = page = None
+        result = method(self, request, *args, **kwargs)
+        if count is None and page is None:
+            return result
+        list_start = int((page - 1) * count)
+        list_end = int(page * count)
+        return result[list_start:list_end]
     return wrapper
 
 
diff --git a/src/mailman/rest/lists.py b/src/mailman/rest/lists.py
index 328472794..2a3c72bfd 100644
--- a/src/mailman/rest/lists.py
+++ b/src/mailman/rest/lists.py
@@ -40,7 +40,7 @@ from mailman.interfaces.member import MemberRole
 from mailman.interfaces.subscriptions import ISubscriptionService
 from mailman.rest.configuration import ListConfiguration
 from mailman.rest.helpers import (
-    CollectionMixin, etag, no_content, path_to, restish_matcher, paginate)
+    CollectionMixin, etag, no_content, paginate, path_to, restish_matcher)
 from mailman.rest.members import AMember, MemberCollection
 from mailman.rest.moderation import HeldMessages, SubscriptionRequests
 from mailman.rest.validator import Validator
diff --git a/src/mailman/rest/members.py b/src/mailman/rest/members.py
index bf2cdf82b..76b2ed1a7 100644
--- a/src/mailman/rest/members.py
+++ b/src/mailman/rest/members.py
@@ -43,7 +43,7 @@ from mailman.interfaces.subscriptions import ISubscriptionService
 from mailman.interfaces.user import UnverifiedAddressError
 from mailman.interfaces.usermanager import IUserManager
 from mailman.rest.helpers import (
-    CollectionMixin, PATCH, etag, no_content, path_to, paginate)
+    CollectionMixin, PATCH, etag, no_content, paginate, path_to)
 from mailman.rest.preferences import Preferences, ReadOnlyPreferences
 from mailman.rest.validator import (
     Validator, enum_validator, subscriber_validator)
diff --git a/src/mailman/rest/tests/test_paginate.py b/src/mailman/rest/tests/test_paginate.py
index ae73125b5..ab39d0996 100644
--- a/src/mailman/rest/tests/test_paginate.py
+++ b/src/mailman/rest/tests/test_paginate.py
@@ -1,4 +1,4 @@
-# Copyright (C) 2011-2013 by the Free Software Foundation, Inc.
+# Copyright (C) 2013 by the Free Software Foundation, Inc.
 #
 # This file is part of GNU Mailman.
 #
@@ -27,18 +27,14 @@ __all__ = [
 
 import unittest
 
-from urllib2 import HTTPError
-from zope.component import getUtility
-
 from mailman.app.lifecycle import create_list
 from mailman.database.transaction import transaction
-from mailman.interfaces.listmanager import IListManager
 from mailman.rest.helpers import paginate
-from mailman.testing.helpers import call_api
 from mailman.testing.layers import RESTLayer
 
 
-class FakeRequest:
+
+class _FakeRequest:
     """Fake restish.http.Request object."""
 
     def __init__(self, count=None, page=None):
@@ -49,7 +45,10 @@ class FakeRequest:
             self.GET['page'] = page
 
 
+
 class TestPaginateHelper(unittest.TestCase):
+    """Test the @paginate decorator."""
+
     layer = RESTLayer
 
     def setUp(self):
@@ -57,73 +56,90 @@ class TestPaginateHelper(unittest.TestCase):
             self._mlist = create_list('test@example.com')
 
     def test_no_pagination(self):
-        # No pagination params in request
-        # Collection with 5 items.
+        # When there is no pagination params in the request, all 5 items in
+        # the collection are returned.
         @paginate
         def get_collection(self, request):
             return ['one', 'two', 'three', 'four', 'five']
         # Expect 5 items
-        page = get_collection(None, FakeRequest())
+        page = get_collection(None, _FakeRequest())
         self.assertEqual(page, ['one', 'two', 'three', 'four', 'five'])
 
     def test_valid_pagination_request_page_one(self):
-        # ?count=2&page=1 is a valid GET query string.
-        # Collection with 5 items.
+        # ?count=2&page=1 returns the first page, with two items in it.
         @paginate
         def get_collection(self, request):
             return ['one', 'two', 'three', 'four', 'five']
-        # Expect 2 items
-        page = get_collection(None, FakeRequest(2, 1))
+        page = get_collection(None, _FakeRequest(2, 1))
         self.assertEqual(page, ['one', 'two'])
 
     def test_valid_pagination_request_page_two(self):
-        # ?count=2&page=2 is a valid GET query string.
-        # Collection with 5 items.
+        # ?count=2&page=2 returns the second page, where a page has two items
+        # in it.
         @paginate
         def get_collection(self, request):
             return ['one', 'two', 'three', 'four', 'five']
-        # Expect 2 items
-        page = get_collection(None, FakeRequest(2, 2))
+        page = get_collection(None, _FakeRequest(2, 2))
         self.assertEqual(page, ['three', 'four'])
 
     def test_2nd_index_larger_than_total(self):
-        # ?count=2&page=3 is a valid GET query string.
-        # Collection with 5 items.
+        # ?count=2&page=3 returns the third page with page size 2, but the
+        # last page only has one item in it.
         @paginate
         def get_collection(self, request):
             return ['one', 'two', 'three', 'four', 'five']
-        # Expect last item
-        page = get_collection(None, FakeRequest(2, 3))
+        page = get_collection(None, _FakeRequest(2, 3))
         self.assertEqual(page, ['five'])
 
     def test_out_of_range_returns_empty_list(self):
-        # ?count=2&page=3 is a valid GET query string.
-        # Collection with 5 items.
+        # ?count=2&page=4 returns the fourth page, which doesn't exist, so an
+        # empty collection is returned.
         @paginate
         def get_collection(self, request):
             return ['one', 'two', 'three', 'four', 'five']
-        # Expect empty list
-        page = get_collection(None, FakeRequest(2, 4))
+        page = get_collection(None, _FakeRequest(2, 4))
         self.assertEqual(page, [])
 
     def test_count_as_string_returns_bad_request(self):
-        # ?count=two&page=2 are not valid values.
+        # ?count=two&page=2 are not valid values, so a bad request occurs.
         @paginate
         def get_collection(self, request):
             return []
-        # Expect Bad Request
-        response = get_collection(None, FakeRequest('two', 1))
+        response = get_collection(None, _FakeRequest('two', 1))
         self.assertEqual(response.status, '400 Bad Request')
 
     def test_no_get_attr_returns_bad_request(self):
-        # ?count=two&page=2 are not valid values.
+        # ?count=two&page=2 are not valid values so a bad request is returned.
         @paginate
         def get_collection(self, request):
             return []
-        request = FakeRequest()
+        request = _FakeRequest()
         del request.GET
-        # Assert request obj has no GET attr.
-        self.assertTrue(getattr(request, 'GET', None) is None)
-        # Expect Bad Request
+        # The request object has no GET attribute.
+        self.assertIsNone(getattr(request, 'GET', None))
         response = get_collection(None, request)
         self.assertEqual(response.status, '400 Bad Request')
+
+    def test_negative_count(self):
+        # ?count=-1&page=1
+        @paginate
+        def get_collection(self, request):
+            return ['one', 'two', 'three', 'four', 'five']
+        response = get_collection(None, _FakeRequest(-1, 1))
+        self.assertEqual(response.status, '400 Bad Request')
+
+    def test_negative_page(self):
+        # ?count=1&page=-1
+        @paginate
+        def get_collection(self, request):
+            return ['one', 'two', 'three', 'four', 'five']
+        response = get_collection(None, _FakeRequest(1, -1))
+        self.assertEqual(response.status, '400 Bad Request')
+
+    def test_negative_page_and_count(self):
+        # ?count=1&page=-1
+        @paginate
+        def get_collection(self, request):
+            return ['one', 'two', 'three', 'four', 'five']
+        response = get_collection(None, _FakeRequest(-1, -1))
+        self.assertEqual(response.status, '400 Bad Request')
diff --git a/src/mailman/rest/users.py b/src/mailman/rest/users.py
index 21bcad202..6826c92b6 100644
--- a/src/mailman/rest/users.py
+++ b/src/mailman/rest/users.py
@@ -38,7 +38,7 @@ from mailman.interfaces.address import ExistingAddressError
 from mailman.interfaces.usermanager import IUserManager
 from mailman.rest.addresses import UserAddresses
 from mailman.rest.helpers import (
-    CollectionMixin, GetterSetter, PATCH, etag, no_content, path_to, paginate)
+    CollectionMixin, GetterSetter, PATCH, etag, no_content, paginate, path_to)
 from mailman.rest.preferences import Preferences
 from mailman.rest.validator import PatchValidator, Validator