Fix paginator bug where None was returned immediately (#1083)

* Fix paginator bug where `None` was returned immediately 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 * Update changelog * Apply suggestions from code review Co-authored-by: Zelda Hessler <zhessler@amazon.com> * rustfmt Co-authored-by: Zelda Hessler <zhessler@amazon.com>
smithy-lang · Jan 18, 2022 · 21ffa90 · 21ffa90
1 parent 5eb8d4c
commit 21ffa90
Show file tree

Hide file tree

Showing 4 changed files with 66 additions and 12 deletions.
diff --git a/CHANGELOG.next.toml b/CHANGELOG.next.toml
@@ -52,3 +52,15 @@ message = "Silence profile credential warnings in Lambda environment"
 references = ["smithy-rs#1065", "aws-sdk-rust#398"]
 meta = { "breaking" = false, "tada" = true, "bug" = true }
 author = "nmoutschen"
+
+[[aws-sdk-rust]]
+message = "Fixed paginator bug impacting EC2 describe VPCs (and others)"
+references = ["aws-sdk-rust#405", "smithy-rs#1083"]
+meta = { "breaking" = false, "tada" = false, "bug" = true }
+author = "rcoh"
+
+[[smithy-rs]]
+message = "Fixed paginator bug impacting EC2 describe VPCs (and others)"
+references = ["aws-sdk-rust#405", "smithy-rs#1083"]
+meta = { "breaking" = false, "tada" = false, "bug" = true }
+author = "rcoh"
diff --git a/aws/sdk/integration-tests/ec2/tests/paginators.rs b/aws/sdk/integration-tests/ec2/tests/paginators.rs
@@ -50,3 +50,39 @@ async fn paginators_handle_empty_tokens() {
     assert_eq!(first_item, None);
     conn.assert_requests_match(&[]);
 }
+
+/// 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`
+#[tokio::test]
+async fn paginators_handle_unset_tokens() {
+    let request= "Action=DescribeSpotPriceHistory&Version=2016-11-15&AvailabilityZone=eu-north-1a&InstanceType.1=g5.48xlarge&ProductDescription.1=Linux%2FUNIX";
+    let response = r#"<?xml version="1.0" encoding="UTF-8"?>
+        <DescribeSpotPriceHistoryResponse xmlns="http://ec2.amazonaws.com/doc/2016-11-15/">
+            <requestId>edf3e86c-4baf-47c1-9228-9a5ea09542e8</requestId>
+            <spotPriceHistorySet/>
+        </DescribeSpotPriceHistoryResponse>"#;
+    let conn = TestConnection::<&str>::new(vec![(
+        http::Request::builder()
+            .uri("https://ec2.us-east-1.amazonaws.com/")
+            .body(request.into())
+            .unwrap(),
+        http::Response::builder()
+            .status(200)
+            .body(response)
+            .unwrap(),
+    )]);
+    let client = Client::from_conf_conn(stub_config(), conn.clone());
+    let instance_type = InstanceType::from("g5.48xlarge");
+    let mut paginator = client
+        .describe_spot_price_history()
+        .instance_types(instance_type)
+        .product_descriptions("Linux/UNIX")
+        .availability_zone("eu-north-1a")
+        .into_paginator()
+        .items()
+        .send();
+    let first_item = paginator.try_next().await.expect("success");
+    assert_eq!(first_item, None);
+    conn.assert_requests_match(&[]);
+}
diff --git a/...c/main/kotlin/software/amazon/smithy/rust/codegen/smithy/generators/PaginatorGenerator.kt b/...c/main/kotlin/software/amazon/smithy/rust/codegen/smithy/generators/PaginatorGenerator.kt
@@ -9,7 +9,6 @@ import software.amazon.smithy.model.Model
 import software.amazon.smithy.model.knowledge.PaginatedIndex
 import software.amazon.smithy.model.shapes.OperationShape
 import software.amazon.smithy.model.shapes.ServiceShape
-import software.amazon.smithy.model.shapes.StringShape
 import software.amazon.smithy.model.traits.IdempotencyTokenTrait
 import software.amazon.smithy.model.traits.PaginatedTrait
 import software.amazon.smithy.rust.codegen.rustlang.CargoDependency
@@ -175,12 +174,13 @@ class PaginatorGenerator private constructor(
                             let done = match resp {
                                 Ok(ref resp) => {
                                     let new_token = #{output_token}(resp);
-                                    if new_token == input.$inputTokenMember.as_ref() {
+                                    let is_empty = ${nextTokenEmpty("new_token")};
+                                    if !is_empty && new_token == input.$inputTokenMember.as_ref() {
                                         let _ = tx.send(Err(#{SdkError}::ConstructionFailure("next token did not change, aborting paginator. This indicates an SDK or AWS service bug.".into()))).await;
                                         return;
                                     }
                                     input.$inputTokenMember = new_token.cloned();
-                                    ${nextTokenEmpty("input.$inputTokenMember")}
+                                    is_empty
                                 },
                                 Err(_) => true,
                             };
@@ -192,7 +192,6 @@ class PaginatorGenerator private constructor(
                                 return
                             }
                         }
-
                     }))
                 }
             }
@@ -276,12 +275,7 @@ class PaginatorGenerator private constructor(
     }
 
     private fun nextTokenEmpty(token: String): String {
-        val tokenType = model.expectShape(paginationInfo.outputTokenMemberPath.last().target)
-        if (tokenType is StringShape) {
-            return "$token.as_deref().unwrap_or_default().is_empty()"
-        } else {
-            return "$token.is_none()"
-        }
+        return "$token.map(|token|token.is_empty()).unwrap_or(true)"
     }
 
     private fun pageSizeSetter() = writable {

diff --git a/tools/ci-cdk/canary-lambda/src/paginator_canary.rs b/tools/ci-cdk/canary-lambda/src/paginator_canary.rs
@@ -40,9 +40,21 @@ pub async fn paginator_canary(client: ec2::Client, page_size: usize) -> anyhow::
         num_pages += 1;
     }
     if dbg!(num_pages) < 2 {
-        bail!("should be ~60 of pages of results")
+        bail!(
+            "expected 3+ pages containing ~60 results but got {} pages",
+            num_pages
+        )
     }
 
+    // https://github.com/awslabs/aws-sdk-rust/issues/405
+    let _ = client
+        .describe_vpcs()
+        .into_paginator()
+        .items()
+        .send()
+        .collect::<Result<Vec<_>, _>>()
+        .await?;
+
     Ok(())
 }
 
@@ -54,6 +66,6 @@ mod test {
     async fn test_paginator() {
         let conf = aws_config::load_from_env().await;
         let client = aws_sdk_ec2::Client::new(&conf);
-        paginator_canary(client).await.unwrap()
+        paginator_canary(client, 20).await.unwrap()
     }
 }