Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix pagination with string params #689

Merged
merged 4 commits into from Mar 5, 2014
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 15 additions & 1 deletion awscli/customizations/paginate.py
Expand Up @@ -66,8 +66,22 @@ def unify_paging_params(argument_table, operation, event_name, **kwargs):
STARTING_TOKEN_HELP,
operation,
parse_type='string')
# Try to get the pagination parameter type
limit_param = None
if 'limit_key' in operation.pagination:
for param in operation.params:
if param.name == operation.pagination['limit_key']:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's a safe assumption that there's always limit_key, based on the code below that does a if 'limit_key' in operation.pagination. It would be worth double checking that this is the case. Either way we should be consistent in this module.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is line 71 not the same as 107? I'm not sure what you mean here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, sorry I misread the code. Disregard my previous comment.

limit_param = param
break

type_ = limit_param and limit_param.type or 'integer'
if limit_param and limit_param.type not in PageArgument.type_map:
raise TypeError(('Unsupported pagination type {0} for operation {1}'
' and parameter {2}').format(type_, operation.name,
limit_param.name))

argument_table['max-items'] = PageArgument('max-items', MAX_ITEMS_HELP,
operation, parse_type='integer')
operation, parse_type=type_)


def check_should_enable_pagination(input_tokens, parsed_args, parsed_globals, **kwargs):
Expand Down
57 changes: 56 additions & 1 deletion tests/unit/customizations/test_paginate.py
Expand Up @@ -17,16 +17,18 @@
from awscli.customizations import paginate


class TestArgumentTableModifications(unittest.TestCase):
class TestPaginateBase(unittest.TestCase):

def setUp(self):
self.operation = mock.Mock()
self.operation.can_paginate = True
self.foo_param = mock.Mock()
self.foo_param.cli_name = 'foo'
self.foo_param.name = 'Foo'
self.foo_param.type = 'string'
self.bar_param = mock.Mock()
self.bar_param.cli_name = 'bar'
self.bar_param.type = 'string'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a test for:

  1. integer
  2. Unknown type (the raising TypeError is not tested).

self.bar_param.name = 'Bar'
self.params = [self.foo_param, self.bar_param]
self.operation.pagination = {
Expand All @@ -35,6 +37,9 @@ def setUp(self):
}
self.operation.params = self.params


class TestArgumentTableModifications(TestPaginateBase):

def test_customize_arg_table(self):
argument_table = {
'foo': mock.Mock(),
Expand Down Expand Up @@ -66,3 +71,53 @@ def test_operation_with_no_paginate(self):
paginate.unify_paging_params(argument_table, self.operation,
'building-argument-table.foo.bar')
self.assertEqual(starting_table, argument_table)


class TestStringLimitKey(TestPaginateBase):

def setUp(self):
super(TestStringLimitKey, self).setUp()
self.bar_param.type = 'string'

def test_integer_limit_key(self):
argument_table = {
'foo': mock.Mock(),
'bar': mock.Mock(),
}
paginate.unify_paging_params(argument_table, self.operation,
'building-argument-table.foo.bar')
# Max items should be the same type as bar, which may not be an int
self.assertEqual('string', argument_table['max-items'].cli_type_name)


class TestIntegerLimitKey(TestPaginateBase):

def setUp(self):
super(TestIntegerLimitKey, self).setUp()
self.bar_param.type = 'integer'

def test_integer_limit_key(self):
argument_table = {
'foo': mock.Mock(),
'bar': mock.Mock(),
}
paginate.unify_paging_params(argument_table, self.operation,
'building-argument-table.foo.bar')
# Max items should be the same type as bar, which may not be an int
self.assertEqual('integer', argument_table['max-items'].cli_type_name)


class TestBadLimitKey(TestPaginateBase):

def setUp(self):
super(TestBadLimitKey, self).setUp()
self.bar_param.type = 'bad'

def test_integer_limit_key(self):
argument_table = {
'foo': mock.Mock(),
'bar': mock.Mock(),
}
with self.assertRaises(TypeError):
paginate.unify_paging_params(argument_table, self.operation,
'building-argument-table.foo.bar')
18 changes: 17 additions & 1 deletion tests/unit/route53/test_resource_id.py
Expand Up @@ -102,4 +102,20 @@ def test_short_resource_id(self):
self.assert_params_for_cmd(cmdline, result, expected_rc=0,
ignore_params=['payload'])[0]



class TestMaxItems(BaseAWSCommandParamsTest):

prefix = 'route53 list-resource-record-sets'

def test_full_resource_id(self):
args = ' --hosted-zone-id /hostedzone/ABCD --max-items 1'
cmdline = self.prefix + args
result = {
'uri_params': {
'HostedZoneId': 'ABCD',
'MaxItems': '1'
},
'headers': {}
}
self.assert_params_for_cmd(cmdline, result, expected_rc=0,
ignore_params=['payload'])[0]