5 files changed, 152 insertions, 75 deletions

diff --git a/src/mailman/rest/helpers.py b/src/mailman/rest/helpers.py
index 5b51bc436..ab1f5b76a 100644
--- a/src/mailman/rest/helpers.py
+++ b/src/mailman/rest/helpers.py
@@ -43,7 +43,6 @@ from cStringIO import StringIO
 from datetime import datetime, timedelta
 from enum import Enum
 from lazr.config import as_boolean
-from restish import http
 from restish.http import Response
 from restish.resource import MethodDecorator
 from webob.multidict import MultiDict
@@ -124,22 +123,15 @@ def paginate(method):
     arguments.
     """
     def wrapper(self, request, *args, **kwargs):
-        try:
-            count = int(request.GET['count'])
-            page = int(request.GET['page'])
-            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.
-        except KeyError:
-            count = page = None
+        # Allow falcon's HTTPBadRequest exceptions to percolate up.  They'll
+        # get turned into HTTP 400 errors.
+        count = request.get_param_as_int('count', min=0)
+        page = request.get_param_as_int('page', min=1)
         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)
+        list_start = (page - 1) * count
+        list_end = 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 7f0e6e797..da1a75bd1 100644
--- a/src/mailman/rest/lists.py
+++ b/src/mailman/rest/lists.py
@@ -26,6 +26,7 @@ __all__ = [
     'ListArchivers',
     'ListConfiguration',
     'ListsForDomain',
+    'Styles',
     ]
 
 
@@ -37,15 +38,17 @@ from restish import http, resource
 from zope.component import getUtility
 
 from mailman.app.lifecycle import create_list, remove_list
+from mailman.config import config
 from mailman.interfaces.domain import BadDomainSpecificationError
 from mailman.interfaces.listmanager import (
     IListManager, ListAlreadyExistsError)
 from mailman.interfaces.mailinglist import IListArchiverSet
 from mailman.interfaces.member import MemberRole
+from mailman.interfaces.styles import IStyleManager
 from mailman.interfaces.subscriptions import ISubscriptionService
 from mailman.rest.configuration import ListConfiguration
 from mailman.rest.helpers import (
-    CollectionMixin, GetterSetter, PATCH, etag, no_content, paginate, path_to,
+    CollectionMixin, GetterSetter, NotFound, child, etag, paginate, path_to,
     restish_matcher)
 from mailman.rest.members import AMember, MemberCollection
 from mailman.rest.moderation import HeldMessages, SubscriptionRequests
@@ -140,20 +143,21 @@ class AList(_ListBase):
         else:
             self._mlist = manager.get_by_list_id(list_identifier)
 
-    @resource.GET()
-    def mailing_list(self, request):
+    def on_get(self, request, response):
         """Return a single mailing list end-point."""
         if self._mlist is None:
-            return http.not_found()
-        return http.ok([], self._resource_as_json(self._mlist))
+            falcon.responders.path_not_found(request, response)
+        else:
+            response.status = falcon.HTTP_200
+            response.body = self._resource_as_json(self._mlist)
 
-    @resource.DELETE()
-    def delete_list(self, request):
+    def on_delete(self, request, response):
         """Delete the named mailing list."""
         if self._mlist is None:
-            return http.not_found()
-        remove_list(self._mlist)
-        return no_content()
+            falcon.responders.path_not_found(request, response)
+        else:
+            remove_list(self._mlist)
+            response.status = falcon.HTTP_204
 
     @resource.child(member_matcher)
     def member(self, request, segments, role, email):
@@ -195,11 +199,11 @@ class AList(_ListBase):
             return http.not_found()
         return SubscriptionRequests(self._mlist)
 
-    @resource.child()
+    @child()
     def archivers(self, request, segments):
         """Return a representation of mailing list archivers."""
         if self._mlist is None:
-            return http.not_found()
+            return NotFound(), []
         return ListArchivers(self._mlist)
 
 
@@ -229,11 +233,11 @@ class AllLists(_ListBase):
             response.status = falcon.HTTP_201
             response.location = location
 
-    @resource.GET()
-    def collection(self, request):
+    def on_get(self, request, response):
         """/lists"""
         resource = self._make_collection(request)
-        return http.ok([], etag(resource))
+        response.status = falcon.HTTP_200
+        response.body = etag(resource)
 
 
 
@@ -245,7 +249,7 @@ class MembersOfList(MemberCollection):
         self._mlist = mailing_list
         self._role = role
 
-    #@paginate
+    @paginate
     def _get_collection(self, request):
         """See `CollectionMixin`."""
         # Overrides _MemberBase._get_collection() because we only want to
@@ -267,7 +271,7 @@ class ListsForDomain(_ListBase):
         response.status = falcon.HTTP_200
         response.body = etag(resource)
 
-    #@paginate
+    @paginate
     def _get_collection(self, request):
         """See `CollectionMixin`."""
         return list(self._domain.mailing_lists)
@@ -291,21 +295,21 @@ class ArchiverGetterSetter(GetterSetter):
         archiver.is_enabled = as_boolean(value)
 
 
-class ListArchivers(resource.Resource):
+class ListArchivers:
     """The archivers for a list, with their enabled flags."""
 
     def __init__(self, mlist):
         self._mlist = mlist
 
-    @resource.GET()
-    def statuses(self, request):
+    def on_get(self, request, response):
         """Get all the archiver statuses."""
         archiver_set = IListArchiverSet(self._mlist)
         resource = {archiver.name: archiver.is_enabled
                     for archiver in archiver_set.archivers}
-        return http.ok([], etag(resource))
+        response.status = falcon.HTTP_200
+        response.body = etag(resource)
 
-    def patch_put(self, request, is_optional):
+    def patch_put(self, request, response, is_optional):
         archiver_set = IListArchiverSet(self._mlist)
         kws = {archiver.name: ArchiverGetterSetter(self._mlist)
                for archiver in archiver_set.archivers}
@@ -315,15 +319,30 @@ class ListArchivers(resource.Resource):
         try:
             Validator(**kws).update(self._mlist, request)
         except ValueError as error:
-            return http.bad_request([], str(error))
-        return no_content()
+            falcon.responders.bad_request(request, response, body=str(error))
+        else:
+            response.status = falcon.HTTP_204
 
-    @resource.PUT()
-    def put_statuses(self, request):
+    def on_put(self, request, response):
         """Update all the archiver statuses."""
-        return self.patch_put(request, is_optional=False)
+        self.patch_put(request, response, is_optional=False)
 
-    @PATCH()
-    def patch_statuses(self, request):
+    def on_patch(self, request, response):
         """Patch some archiver statueses."""
-        return self.patch_put(request, is_optional=True)
+        self.patch_put(request, response, is_optional=True)
+
+
+
+class Styles:
+    """Simple resource representing all list styles."""
+
+    def __init__(self):
+        manager = getUtility(IStyleManager)
+        style_names = sorted(style.name for style in manager.styles)
+        self._resource = dict(
+            style_names=style_names,
+            default=config.styles.default)
+
+    def on_get(self, request, response):
+        response.status = falcon.HTTP_200
+        response.body = etag(self._resource)
diff --git a/src/mailman/rest/root.py b/src/mailman/rest/root.py
index 9e9f5bfa5..c8dce0eca 100644
--- a/src/mailman/rest/root.py
+++ b/src/mailman/rest/root.py
@@ -35,11 +35,10 @@ from mailman.config import config
 from mailman.core.constants import system_preferences
 from mailman.core.system import system
 from mailman.interfaces.listmanager import IListManager
-from mailman.interfaces.styles import IStyleManager
 from mailman.rest.addresses import AllAddresses, AnAddress
 from mailman.rest.domains import ADomain, AllDomains
 from mailman.rest.helpers import BadRequest, NotFound, child, etag, path_to
-from mailman.rest.lists import AList, AllLists
+from mailman.rest.lists import AList, AllLists, Styles
 from mailman.rest.members import AMember, AllMembers, FindMembers
 from mailman.rest.preferences import ReadOnlyPreferences
 from mailman.rest.templates import TemplateFinder
@@ -139,11 +138,7 @@ class TopLevel:
         if len(segments) == 0:
             return AllLists()
         elif len(segments) == 1 and segments[0] == 'styles':
-            manager = getUtility(IStyleManager)
-            style_names = sorted(style.name for style in manager.styles)
-            resource = dict(style_names=style_names,
-                            default=config.styles.default)
-            return http.ok([], etag(resource))
+            return Styles(), []
         else:
             # list-id is preferred, but for backward compatibility,
             # fqdn_listname is also accepted.
diff --git a/src/mailman/rest/tests/test_lists.py b/src/mailman/rest/tests/test_lists.py
index f4cafa3f3..9426e7f27 100644
--- a/src/mailman/rest/tests/test_lists.py
+++ b/src/mailman/rest/tests/test_lists.py
@@ -22,6 +22,7 @@ from __future__ import absolute_import, print_function, unicode_literals
 __metaclass__ = type
 __all__ = [
     'TestListArchivers',
+    'TestListPagination',
     'TestLists',
     'TestListsMissing',
     ]
@@ -228,3 +229,84 @@ class TestListArchivers(unittest.TestCase):
                 method='PATCH')
         self.assertEqual(cm.exception.code, 400)
         self.assertEqual(cm.exception.reason, 'Invalid boolean value: sure')
+
+
+
+class TestListPagination(unittest.TestCase):
+    """Test mailing list pagination functionality.
+
+    We create a bunch of mailing lists within a domain.  When we want to
+    get all the lists in that domain via the REST API, we need to
+    paginate over them, otherwise there could be too many for display.
+    """
+
+    layer = RESTLayer
+
+    def setUp(self):
+        with transaction():
+            # Create a bunch of mailing lists in the example.com domain.
+            create_list('ant@example.com')
+            create_list('bee@example.com')
+            create_list('cat@example.com')
+            create_list('dog@example.com')
+            create_list('emu@example.com')
+            create_list('fly@example.com')
+
+    def test_first_page(self):
+        resource, response = call_api(
+            'http://localhost:9001/3.0/domains/example.com/lists'
+            '?count=1&page=1')
+        # There are 6 total lists, but only the first one in the page.
+        self.assertEqual(resource['total_size'], 1)
+        self.assertEqual(resource['start'], 0)
+        self.assertEqual(len(resource['entries']), 1)
+        entry = resource['entries'][0]
+        self.assertEqual(entry['fqdn_listname'], 'ant@example.com')
+
+    def test_second_page(self):
+        resource, response = call_api(
+            'http://localhost:9001/3.0/domains/example.com/lists'
+            '?count=1&page=2')
+        # There are 6 total lists, but only the first one in the page.
+        self.assertEqual(resource['total_size'], 1)
+        self.assertEqual(resource['start'], 0)
+        self.assertEqual(len(resource['entries']), 1)
+        entry = resource['entries'][0]
+        self.assertEqual(entry['fqdn_listname'], 'bee@example.com')
+
+    def test_last_page(self):
+        resource, response = call_api(
+            'http://localhost:9001/3.0/domains/example.com/lists'
+            '?count=1&page=6')
+        # There are 6 total lists, but only the first one in the page.
+        self.assertEqual(resource['total_size'], 1)
+        self.assertEqual(resource['start'], 0)
+        self.assertEqual(len(resource['entries']), 1)
+        entry = resource['entries'][0]
+        self.assertEqual(entry['fqdn_listname'], 'fly@example.com')
+
+    def test_zeroth_page(self):
+        # Page numbers start at one.
+        with self.assertRaises(HTTPError) as cm:
+            resource, response = call_api(
+                'http://localhost:9001/3.0/domains/example.com/lists'
+                '?count=1&page=0')
+        self.assertEqual(cm.exception.code, 400)
+
+    def test_negative_page(self):
+        # Negative pages are not allowed.
+        with self.assertRaises(HTTPError) as cm:
+            resource, response = call_api(
+                'http://localhost:9001/3.0/domains/example.com/lists'
+                '?count=1&page=-1')
+        self.assertEqual(cm.exception.code, 400)
+
+    def test_past_last_page(self):
+        # The 7th page doesn't exist so the collection is empty.
+        resource, response = call_api(
+            'http://localhost:9001/3.0/domains/example.com/lists'
+            '?count=1&page=7')
+        # There are 6 total lists, but only the first one in the page.
+        self.assertEqual(resource['total_size'], 0)
+        self.assertEqual(resource['start'], 0)
+        self.assertNotIn('entries', resource)
diff --git a/src/mailman/rest/tests/test_paginate.py b/src/mailman/rest/tests/test_paginate.py
index 0774125bb..3a166d9d2 100644
--- a/src/mailman/rest/tests/test_paginate.py
+++ b/src/mailman/rest/tests/test_paginate.py
@@ -27,6 +27,7 @@ __all__ = [
 
 import unittest
 
+from falcon import InvalidParam, Request
 from mailman.app.lifecycle import create_list
 from mailman.database.transaction import transaction
 from mailman.rest.helpers import paginate
@@ -34,15 +35,15 @@ from mailman.testing.layers import RESTLayer
 
 
 
-class _FakeRequest:
+class _FakeRequest(Request):
     """Fake restish.http.Request object."""
 
     def __init__(self, count=None, page=None):
-        self.GET = {}
+        self._params = {}
         if count is not None:
-            self.GET['count'] = count
+            self._params['count'] = count
         if page is not None:
-            self.GET['page'] = page
+            self._params['page'] = page
 
 
 
@@ -105,41 +106,29 @@ class TestPaginateHelper(unittest.TestCase):
         @paginate
         def get_collection(self, request):
             return []
-        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 so a bad request is returned.
-        @paginate
-        def get_collection(self, request):
-            return []
-        request = _FakeRequest()
-        del request.GET
-        # 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')
+        self.assertRaises(InvalidParam, get_collection,
+                          None, _FakeRequest('two', 1))
 
     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')
+        self.assertRaises(InvalidParam, get_collection,
+                          None, _FakeRequest(-1, 1))
 
     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')
+        self.assertRaises(InvalidParam, get_collection,
+                          None, _FakeRequest(1, -1))
 
     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')
+        self.assertRaises(InvalidParam, get_collection,
+                          None, _FakeRequest(-1, -1))