-
Notifications
You must be signed in to change notification settings - Fork 192
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 paginator bug where None
was returned immediately
#1083
Conversation
The escape hatch added by aws-sdk-rust#391 did not properly handle the case where the first response was `None` and _not_ the empty string. This diff: - Checks for emptiness for both maps and strings - Fixes the check so that an initial `None` input does not cause an incorrect paginator error
fd52ddc
to
0344ae9
Compare
A new doc preview is ready to view. |
A new generated diff is ready to view.
|
A new generated diff is ready to view.
|
A new doc preview is ready to view. |
|
||
/// See https://github.com/awslabs/aws-sdk-rust/issues/405 | ||
/// | ||
/// EC2 can also reply with the token truly unset which will be interpreted as `None` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What exactly does this mean? How many "kinds" of unset are there? I can think of
a. field returned but empty or set to a value signifying empty
b. no field returned
Were we only covering case a previously and now we're covering case b too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah it can either be <nextToken></nextToken>
, or as is the case here, <nextToken>
is not present in the response at all. In one case, we get Some("")
, in the other, we get None
. None
used to be handled properly, but regressed when I added the failsafe to avoid infinite loops
} else { | ||
return "$token.is_none()" | ||
} | ||
return "$token.map(|token|token.is_empty()).unwrap_or(true)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should prefer function pointers in place of closures where possible:
return "$token.map(|token|token.is_empty()).unwrap_or(true)" | |
return "$token.map(String::is_empty).unwrap_or(true)" |
It might not be possible depending on how type inference interprets this though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this can be either a string or a hashmap
Co-authored-by: Zelda Hessler <[email protected]>
A new doc preview is ready to view. |
A new generated diff is ready to view.
|
A new generated diff is ready to view.
|
A new doc preview is ready to view. |
Motivation and Context
Description
The escape hatch added by aws-sdk-rust#391 did not properly handle the case where the first response was
None
and not the empty string. This diff:None
input does not cause an incorrect paginator errorTesting
Checklist
CHANGELOG.next.toml
if I made changes to the smithy-rs codegen or runtime cratesCHANGELOG.next.toml
if I made changes to the AWS SDK, generated SDK code, or SDK runtime cratesBy submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.