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

Conversation

danielgtaylor
Copy link
Contributor

Fixes boto/botocore#243 by making sure the max items pagination param uses the same type as the parameter it is replacing.

$ aws route53 list-resource-record-sets --hosted-zone-id /hostedzone/ABCD --max-items 1

A client error (NoSuchHostedZone) occurred when calling the ListResourceRecordSets operation: No hosted zone found with ID: ABCD

@jamesls please review.

type_ = 'integer'
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.

@jamesls
Copy link
Member

jamesls commented Mar 4, 2014

Please add a test for this specific case you mention in the PR descriptions:

$ aws route53 list-resource-record-sets --hosted-zone-id /hostedzone/ABCD --max-items 1

@danielgtaylor
Copy link
Contributor Author

I believe my latest changes fix all the comments above. Please take another look 😄

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']._parse_type)
Copy link
Member

Choose a reason for hiding this comment

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

In general, we should not be testing through any internal APIs in a unittest unless there's really no other way to do so (just a general rule of thumb to keep in mind). You can use the .cli_type_name property to get this information.

@jamesls
Copy link
Member

jamesls commented Mar 5, 2014

Also, I'd say just make the s/._parse_type/.cli_type_name/ change and feel free to merge.

:shipit:

danielgtaylor added a commit that referenced this pull request Mar 5, 2014
Fix pagination with string params. Fixes #689.
@danielgtaylor danielgtaylor merged commit bf5e9b3 into aws:develop Mar 5, 2014
@danielgtaylor danielgtaylor deleted the page-string branch March 5, 2014 19:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Route53 param validation for MaxItems
2 participants