Add pagination to v1 image-list

* Use recursive generator function to make subsequent requests
  to the v1 detailed images resource
* 'limit' continues to act as the absolute limit of images to return
  from a list call
* 'page_size' indicates how many images to ask for in each subsequent
  pagination request
* Expose --page-size through the cli
* Convert v1 images tests to use strict url comparison
* Drop strict_url_check from FakeAPI kwargs - now the functionality
  is always active and tests must directly match fixture urls
* Fix bug 1024614

Change-Id: Ifa7874d88360e03b5c8aa95bfb9d5e6dc6dc927e

5 files changed, 151 insertions, 38 deletions

diff --git a/glanceclient/v1/images.py b/glanceclient/v1/images.py
index 4263baf..027ab23 100644
--- a/glanceclient/v1/images.py
+++ b/glanceclient/v1/images.py
@@ -27,6 +27,8 @@ UPDATE_PARAMS = ('name', 'disk_format', 'container_format', 'min_disk',
 
 CREATE_PARAMS = UPDATE_PARAMS + ('id',)
 
+DEFAULT_PAGE_SIZE = 20
+
 
 class Image(base.Resource):
     def __repr__(self):
@@ -85,25 +87,45 @@ class ImageManager(base.Manager):
         resp, body = self.api.raw_request('GET', '/v1/images/%s' % image_id)
         return body
 
-    def list(self, limit=None, marker=None, filters=None):
+    def list(self, **kwargs):
         """Get a list of images.
 
-        :param limit: maximum number of images to return. Used for pagination.
-        :param marker: id of image last seen by caller. Used for pagination.
+        :param page_size: number of items to request in each paginated request
+        :param limit: maximum number of images to return
+        :param marker: begin returning images that appear later in the image
+                       list than that represented by this image id
+        :param filters: dict of direct comparison filters that mimics the
+                        structure of an image object
         :rtype: list of :class:`Image`
         """
-        params = {}
-        if limit:
-            params['limit'] = int(limit)
-        if marker:
-            params['marker'] = marker
-        if filters:
-            properties = filters.pop('properties', {})
-            for key, value in properties.items():
-                params['property-%s' % key] = value
-            params.update(filters)
-        query = '?%s' % urllib.urlencode(params) if params else ''
-        return self._list('/v1/images/detail%s' % query, "images")
+        limit = kwargs.get('limit')
+
+        def paginate(qp, seen=0):
+            url = '/v1/images/detail?%s' % urllib.urlencode(qp)
+            images = self._list(url, "images")
+            for image in images:
+                seen += 1
+                yield image
+
+            page_size = qp.get('limit')
+            if (page_size and len(images) == page_size and
+                    (limit is None or 0 < seen < limit)):
+                qp['marker'] = image.id
+                for image in paginate(qp, seen):
+                    yield image
+
+        params = {'limit': kwargs.get('page_size', DEFAULT_PAGE_SIZE)}
+
+        if 'marker' in kwargs:
+            params['marker'] = kwargs['marker']
+
+        filters = kwargs.get('filters', {})
+        properties = filters.pop('properties', {})
+        for key, value in properties.items():
+            params['property-%s' % key] = value
+        params.update(filters)
+
+        return paginate(params)
 
     def delete(self, image):
         """Delete an image."""
diff --git a/glanceclient/v1/shell.py b/glanceclient/v1/shell.py
index b3a973a..9639724 100644
--- a/glanceclient/v1/shell.py
+++ b/glanceclient/v1/shell.py
@@ -36,6 +36,8 @@ import glanceclient.v1.images
 @utils.arg('--property-filter', metavar='<KEY=VALUE>',
            help="Filter images by a user-defined image property.",
             action='append', dest='properties', default=[])
+@utils.arg('--page-size', metavar='<SIZE>', default=None, type=int,
+           help='Number of images to request in each paginated request.')
 def do_image_list(gc, args):
     """List images."""
     filter_keys = ['name', 'status', 'container_format', 'disk_format',
@@ -47,7 +49,10 @@ def do_image_list(gc, args):
         property_filter_items = [p.split('=', 1) for p in args.properties]
         filters['properties'] = dict(property_filter_items)
 
-    images = gc.images.list(filters=filters)
+    kwargs = {'filters': filters}
+    if args.page_size is not None:
+        kwargs['page_size'] = args.page_size
+    images = gc.images.list(**kwargs)
     columns = ['ID', 'Name', 'Disk Format', 'Container Format',
                'Size', 'Status']
     utils.print_list(images, columns)
diff --git a/tests/utils.py b/tests/utils.py
index 81435bd..be27a0b 100644
--- a/tests/utils.py
+++ b/tests/utils.py
@@ -15,16 +15,13 @@
 
 
 class FakeAPI(object):
-    def __init__(self, fixtures, strict_url_check=False):
+    def __init__(self, fixtures):
         self.fixtures = fixtures
         self.calls = []
-        self.strict_url_check = strict_url_check
 
     def _request(self, method, url, headers=None, body=None):
         call = (method, url, headers or {}, body)
         self.calls.append(call)
-        if not self.strict_url_check:
-            url = url.split('?', 1)[0]
         return self.fixtures[url][method]
 
     def raw_request(self, *args, **kwargs):
diff --git a/tests/v1/test_images.py b/tests/v1/test_images.py
index dc2f2c1..bc3a1f2 100644
--- a/tests/v1/test_images.py
+++ b/tests/v1/test_images.py
@@ -42,7 +42,87 @@ fixtures = {
             ),
         ),
     },
-    '/v1/images/detail': {
+    '/v1/images/detail?limit=20': {
+        'GET': (
+            {},
+            {'images': [
+                {
+                    'id': 'a',
+                    'name': 'image-1',
+                    'properties': {'arch': 'x86_64'},
+                },
+                {
+                    'id': 'b',
+                    'name': 'image-2',
+                    'properties': {'arch': 'x86_64'},
+                },
+            ]},
+        ),
+    },
+    '/v1/images/detail?marker=a&limit=20': {
+        'GET': (
+            {},
+            {'images': [
+                {
+                    'id': 'b',
+                    'name': 'image-1',
+                    'properties': {'arch': 'x86_64'},
+                },
+                {
+                    'id': 'c',
+                    'name': 'image-2',
+                    'properties': {'arch': 'x86_64'},
+                },
+            ]},
+        ),
+    },
+    '/v1/images/detail?limit=2': {
+        'GET': (
+            {},
+            {'images': [
+                {
+                    'id': 'a',
+                    'name': 'image-1',
+                    'properties': {'arch': 'x86_64'},
+                },
+                {
+                    'id': 'b',
+                    'name': 'image-2',
+                    'properties': {'arch': 'x86_64'},
+                },
+            ]},
+        ),
+    },
+    '/v1/images/detail?marker=b&limit=2': {
+        'GET': (
+            {},
+            {'images': [
+                {
+                    'id': 'c',
+                    'name': 'image-3',
+                    'properties': {'arch': 'x86_64'},
+                },
+            ]},
+        ),
+    },
+    '/v1/images/detail?limit=20&name=foo': {
+        'GET': (
+            {},
+            {'images': [
+                {
+                    'id': 'a',
+                    'name': 'image-1',
+                    'properties': {'arch': 'x86_64'},
+                },
+                {
+                    'id': 'b',
+                    'name': 'image-2',
+                    'properties': {'arch': 'x86_64'},
+                },
+            ]},
+        ),
+    },
+    '/v1/images/detail?property-ping=pong&limit=20': {
         'GET': (
             {},
             {'images': [
@@ -94,33 +174,42 @@ class ImageManagerTest(unittest.TestCase):
         self.api = utils.FakeAPI(fixtures)
         self.mgr = glanceclient.v1.images.ImageManager(self.api)
 
-    def test_list(self):
-        images = self.mgr.list()
-        expect = [('GET', '/v1/images/detail', {}, None)]
+    def test_paginated_list(self):
+        images = list(self.mgr.list(page_size=2))
+        expect = [
+            ('GET', '/v1/images/detail?limit=2', {}, None),
+            ('GET', '/v1/images/detail?marker=b&limit=2', {}, None),
+        ]
+        self.assertEqual(self.api.calls, expect)
+        self.assertEqual(len(images), 3)
+        self.assertEqual(images[0].id, 'a')
+        self.assertEqual(images[1].id, 'b')
+        self.assertEqual(images[2].id, 'c')
+
+    def test_list_with_limit_less_than_page_size(self):
+        list(self.mgr.list(page_size=20, limit=10))
+        expect = [('GET', '/v1/images/detail?limit=20', {}, None)]
         self.assertEqual(self.api.calls, expect)
-        self.assertEqual(len(images), 1)
-        self.assertEqual(images[0].id, '1')
-        self.assertEqual(images[0].name, 'image-1')
-        self.assertEqual(images[0].properties, {'arch': 'x86_64'})
 
-    def test_list_with_limit(self):
-        self.mgr.list(limit=10)
-        expect = [('GET', '/v1/images/detail?limit=10', {}, None)]
+    def test_list_with_limit_greater_than_page_size(self):
+        list(self.mgr.list(page_size=20, limit=30))
+        expect = [('GET', '/v1/images/detail?limit=20', {}, None)]
         self.assertEqual(self.api.calls, expect)
 
     def test_list_with_marker(self):
-        self.mgr.list(marker=20)
-        expect = [('GET', '/v1/images/detail?marker=20', {}, None)]
+        list(self.mgr.list(marker='a'))
+        expect = [('GET', '/v1/images/detail?marker=a&limit=20', {}, None)]
         self.assertEqual(self.api.calls, expect)
 
     def test_list_with_filter(self):
-        self.mgr.list(filters={'name': "foo"})
-        expect = [('GET', '/v1/images/detail?name=foo', {}, None)]
+        list(self.mgr.list(filters={'name': "foo"}))
+        expect = [('GET', '/v1/images/detail?limit=20&name=foo', {}, None)]
         self.assertEqual(self.api.calls, expect)
 
     def test_list_with_property_filters(self):
-        self.mgr.list(filters={'properties': {'ping': 'pong'}})
-        expect = [('GET', '/v1/images/detail?property-ping=pong', {}, None)]
+        list(self.mgr.list(filters={'properties': {'ping': 'pong'}}))
+        url = '/v1/images/detail?property-ping=pong&limit=20'
+        expect = [('GET', url, {}, None)]
         self.assertEqual(self.api.calls, expect)
 
     def test_get(self):
diff --git a/tests/v2/test_images.py b/tests/v2/test_images.py
index d6d6d17..9bdb702 100644
--- a/tests/v2/test_images.py
+++ b/tests/v2/test_images.py
@@ -84,7 +84,7 @@ FakeModel = warlock.model_factory(fake_schema)
 class TestController(unittest.TestCase):
     def setUp(self):
         super(TestController, self).setUp()
-        self.api = utils.FakeAPI(fixtures, strict_url_check=True)
+        self.api = utils.FakeAPI(fixtures)
         self.controller = images.Controller(self.api, FakeModel)
 
     def test_list_images(self):