From 21ffa905dbc048dde6dff654bf953ee01df478b5 Mon Sep 17 00:00:00 2001 From: Russell Cohen Date: Tue, 18 Jan 2022 12:46:53 -0500 Subject: [PATCH] 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 * rustfmt Co-authored-by: Zelda Hessler --- CHANGELOG.next.toml | 12 +++++++ .../integration-tests/ec2/tests/paginators.rs | 36 +++++++++++++++++++ .../smithy/generators/PaginatorGenerator.kt | 14 +++----- .../canary-lambda/src/paginator_canary.rs | 16 +++++++-- 4 files changed, 66 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.next.toml b/CHANGELOG.next.toml index b775bfc4cf..9eda0e2234 100644 --- 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 index 833de63379..b77fbb6eaf 100644 --- 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#" + + edf3e86c-4baf-47c1-9228-9a5ea09542e8 + + "#; + 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/codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/generators/PaginatorGenerator.kt b/codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/generators/PaginatorGenerator.kt index 0507656b78..0fec1ef84a 100644 --- a/codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/generators/PaginatorGenerator.kt +++ b/codegen/src/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 index 046763434e..44e487b07f 100644 --- 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::, _>>() + .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() } }