From 79dea076dbd5f35c0572b51dfcceeb5112772091 Mon Sep 17 00:00:00 2001 From: Steve Baker Date: Mon, 5 Aug 2013 15:29:16 +1200 Subject: [PATCH] Do not paginate stack list unless page_size is set Pagination on the server has not yet been implemented, however the client assumes it is, causing infinite recursion when paginate is called. With this change, no pagination is attempted unless page_size is specified. However the 'limit' parameter is still honored, so it is still possible to cap the total number of results returned. heatclient users should not set page_size until pagination has been implemented on the server. The stack list in Horizon currently does not attempt to paginate, so is not affected. Fixes: bug #1207839 Change-Id: I4087d3a8af48206d6ebe3edc441469464e4a401a --- heatclient/tests/fakes.py | 3 +- heatclient/tests/test_shell.py | 2 +- heatclient/tests/test_stacks.py | 231 +++++++++++++++++++++++++++++++- heatclient/v1/stacks.py | 10 +- 4 files changed, 234 insertions(+), 12 deletions(-) diff --git a/heatclient/tests/fakes.py b/heatclient/tests/fakes.py index 732bd1f8..199dad9f 100644 --- a/heatclient/tests/fakes.py +++ b/heatclient/tests/fakes.py @@ -45,8 +45,7 @@ def script_heat_list(): {'content-type': 'application/json'}, json.dumps(resp_dict)) v1client.Client.json_request('GET', - '/stacks?limit=20').AndReturn((resp, - resp_dict)) + '/stacks?').AndReturn((resp, resp_dict)) def script_heat_normal_error(): diff --git a/heatclient/tests/test_shell.py b/heatclient/tests/test_shell.py index 9a07e0c5..e28e8411 100644 --- a/heatclient/tests/test_shell.py +++ b/heatclient/tests/test_shell.py @@ -166,7 +166,7 @@ class ShellValidationTest(TestCase): self.m.StubOutWithMock(v1client.Client, 'json_request') fakes.script_keystone_client() v1client.Client.json_request( - 'GET', '/stacks?limit=20').AndRaise(exc.Unauthorized) + 'GET', '/stacks?').AndRaise(exc.Unauthorized) self.m.ReplayAll() fake_env = { diff --git a/heatclient/tests/test_stacks.py b/heatclient/tests/test_stacks.py index f2a4b961..fe0f60b7 100644 --- a/heatclient/tests/test_stacks.py +++ b/heatclient/tests/test_stacks.py @@ -14,16 +14,32 @@ # under the License. from heatclient.v1.stacks import Stack +from heatclient.v1.stacks import StackManager from mock import MagicMock import testscenarios from testscenarios.scenarios import multiply_scenarios import testtools - load_tests = testscenarios.load_tests_apply_scenarios +def mock_stack(manager, stack_name, stack_id): + return Stack(manager, { + "id": stack_id, + "stack_name": stack_name, + "links": [{ + "href": "http://192.0.2.1:8004/v1/1234/stacks/%s/%s" % ( + stack_name, stack_id), + "rel": "self"}], + "description": "No description", + "stack_status_reason": "Stack create completed successfully", + "creation_time": "2013-08-04T20:57:55Z", + "updated_time": "2013-08-04T20:57:55Z", + "stack_status": "CREATE_COMPLETE" + }) + + class StackStatusActionTest(testtools.TestCase): scenarios = multiply_scenarios([ @@ -41,7 +57,8 @@ class StackStatusActionTest(testtools.TestCase): def test_status_action(self): stack_status = '%s_%s' % (self.action, self.status) - stack = Stack(None, {'stack_status': stack_status}) + stack = mock_stack(None, 'stack_1', 'abcd1234') + stack.stack_status = stack_status self.assertEqual(self.action, stack.action) self.assertEqual(self.status, stack.status) @@ -50,12 +67,218 @@ class StackOperationsTest(testtools.TestCase): def test_delete_stack(self): manager = MagicMock() - stack = Stack(manager, {'id': 'abcd1234'}) + stack = mock_stack(manager, None, 'abcd1234') stack.delete() manager.delete.assert_called_once_with('abcd1234') def test_get_stack(self): manager = MagicMock() - stack = Stack(manager, {'id': 'abcd1234', 'stack_name': 'the_stack'}) + stack = mock_stack(manager, 'the_stack', 'abcd1234') stack.get() manager.get.assert_called_once_with('the_stack/abcd1234') + + +class StackManagerNoPaginationTest(testtools.TestCase): + + scenarios = [ + ('total_0', dict(total=0)), + ('total_1', dict(total=1)), + ('total_9', dict(total=9)), + ('total_1', dict(total=10)), + ('total_11', dict(total=11)), + ('total_19', dict(total=19)), + ('total_20', dict(total=20)), + ('total_21', dict(total=21)), + ('total_49', dict(total=49)), + ('total_50', dict(total=50)), + ('total_51', dict(total=51)), + ('total_95', dict(total=95)), + ] + + # absolute limit for results returned + limit = 50 + + def mock_manager(self): + manager = StackManager(None) + manager._list = MagicMock() + + def mock_list(*args, **kwargs): + def results(): + for i in range(0, self.total): + stack_name = 'stack_%s' % (i + 1) + stack_id = 'abcd1234-%s' % (i + 1) + yield mock_stack(manager, stack_name, stack_id) + + return list(results()) + + manager._list.side_effect = mock_list + return manager + + def test_stack_list_no_pagination(self): + manager = self.mock_manager() + results = list(manager.list(limit=self.limit)) + manager._list.assert_called_once_with( + '/stacks?', 'stacks') + + last_result = min(self.limit, self.total) + # paginate is not specified, so the total + # results (up to the limit) is always returned + self.assertEqual(last_result, len(results)) + + if last_result > 0: + self.assertEqual('stack_1', results[0].stack_name) + self.assertEqual('stack_%s' % last_result, results[-1].stack_name) + + def test_stack_list_no_pagination_no_limit(self): + manager = self.mock_manager() + + results = list(manager.list()) + manager._list.assert_called_once_with( + '/stacks?', 'stacks') + + # paginate is not specified, so the total + # results is always returned + self.assertEqual(self.total, len(results)) + + if self.total > 0: + self.assertEqual('stack_1', results[0].stack_name) + self.assertEqual('stack_%s' % self.total, results[-1].stack_name) + + +class StackManagerPaginationTest(testtools.TestCase): + + scenarios = [ + ('0_offset_0', dict( + page_size=10, + offset=0, + total=0, + results=((0, 0),) + )), + ('1_offset_0', dict( + page_size=10, + offset=0, + total=1, + results=((0, 1),) + )), + ('9_offset_0', dict( + page_size=10, + offset=0, + total=9, + results=((0, 9),) + )), + ('10_offset_0', dict( + page_size=10, + offset=0, + total=10, + results=((0, 10), (10, 10)) + )), + ('11_offset_0', dict( + page_size=10, + offset=0, + total=11, + results=((0, 10), (10, 11)) + )), + ('11_offset_10', dict( + page_size=10, + offset=10, + total=11, + results=((10, 11),) + )), + ('19_offset_10', dict( + page_size=10, + offset=10, + total=19, + results=((10, 19),) + )), + ('20_offset_10', dict( + page_size=10, + offset=10, + total=20, + results=((10, 20), (20, 20)) + )), + ('21_offset_10', dict( + page_size=10, + offset=10, + total=21, + results=((10, 20), (20, 21)) + )), + ('21_offset_0', dict( + page_size=10, + offset=0, + total=21, + results=((0, 10), (10, 20), (20, 21)) + )), + ('21_offset_20', dict( + page_size=10, + offset=20, + total=21, + results=((20, 21),) + )), + ('95_offset_90', dict( + page_size=10, + offset=90, + total=95, + results=((90, 95),) + )), + ] + + # absolute limit for results returned + limit = 50 + + def mock_manager(self): + manager = StackManager(None) + manager._list = MagicMock() + + def mock_list(arg_url, arg_response_key): + try: + result = self.results[self.result_index] + except IndexError: + return + self.result_index = self.result_index + 1 + + offset = result[0] + url = '/stacks?' + if offset > 0: + url += 'marker=abcd1234-%s&' % offset + url += 'limit=%s' % self.page_size + self.assertEqual(url, arg_url) + + def results(): + + for i in range(*result): + stack_name = 'stack_%s' % (i + 1) + stack_id = 'abcd1234-%s' % (i + 1) + yield mock_stack(manager, stack_name, stack_id) + + return list(results()) + + manager._list.side_effect = mock_list + return manager + + def test_stack_list_pagination(self): + manager = self.mock_manager() + + list_params = { + 'page_size': self.page_size, + 'limit': self.limit + } + + if self.offset > 0: + marker = 'abcd1234-%s' % self.offset + list_params['marker'] = marker + + self.result_index = 0 + results = list(manager.list(**list_params)) + + # assert that the list method has been called enough times + self.assertEqual(len(self.results), self.result_index) + + last_result = min(self.limit, self.total - self.offset) + # one or more list calls have been recomposed into a single list + self.assertEqual(last_result, len(results)) + + if last_result > 0: + self.assertEqual('stack_%s' % (self.offset + 1), + results[0].stack_name) + self.assertEqual('stack_%s' % (self.offset + last_result), + results[-1].stack_name) diff --git a/heatclient/v1/stacks.py b/heatclient/v1/stacks.py index b9e76fc4..906e57e4 100644 --- a/heatclient/v1/stacks.py +++ b/heatclient/v1/stacks.py @@ -17,8 +17,6 @@ import urllib from heatclient.common import base -DEFAULT_PAGE_SIZE = 20 - class Stack(base.Resource): def __repr__(self): @@ -86,10 +84,12 @@ class StackManager(base.Manager): if (page_size and len(stacks) == page_size and (absolute_limit is None or 0 < seen < absolute_limit)): qp['marker'] = stack.id - for image in paginate(qp, seen): - yield image + for stack in paginate(qp, seen): + yield stack - params = {'limit': kwargs.get('page_size', DEFAULT_PAGE_SIZE)} + params = {} + if 'page_size' in kwargs: + params['limit'] = kwargs['page_size'] if 'marker' in kwargs: params['marker'] = kwargs['marker']