From 2e50dab45d5e3a541404f542ad14e0e8382cd325 Mon Sep 17 00:00:00 2001 From: Russell Cohen Date: Mon, 10 Jan 2022 11:38:29 -0500 Subject: [PATCH] Fix bug where paginators looped forever on empty string next token (#1054) * Fix bug where paginators looped forever on empty string next token * Update changelogs --- CHANGELOG.next.toml | 14 ++++- aws/sdk/integration-tests/Cargo.toml | 1 + .../dynamodb/tests/paginators.rs | 53 ++++++++++++++++++- aws/sdk/integration-tests/ec2/Cargo.toml | 11 ++++ aws/sdk/integration-tests/ec2/src/lib.rs | 4 ++ .../integration-tests/ec2/tests/paginators.rs | 52 ++++++++++++++++++ .../smithy/generators/PaginatorGenerator.kt | 19 ++++++- .../generators/PaginatorGeneratorTest.kt | 2 +- 8 files changed, 151 insertions(+), 5 deletions(-) create mode 100644 aws/sdk/integration-tests/ec2/Cargo.toml create mode 100644 aws/sdk/integration-tests/ec2/src/lib.rs create mode 100644 aws/sdk/integration-tests/ec2/tests/paginators.rs diff --git a/CHANGELOG.next.toml b/CHANGELOG.next.toml index eb0eb594e..c5b4f4888 100644 --- a/CHANGELOG.next.toml +++ b/CHANGELOG.next.toml @@ -14,4 +14,16 @@ message = "Fix typos for X-Ray trace ID environment variable in aws_http::recursion_detection" references = ["smithy-rs#1050"] meta = { "breaking" = false, "tada" = false, "bug" = true } -author = "nmoutschen" \ No newline at end of file +author = "nmoutschen" + +[[aws-sdk-rust]] +message = "Fix critical paginator bug where an empty outputToken lead to a never ending stream." +references = ["smithy-rs#1054", "aws-sdk-rust#391"] +meta = { "breaking" = false, "tada" = false, "bug" = true } +author = "rcoh" + +[[smithy-rs]] +message = "Fix critical paginator bug where an empty outputToken lead to a never ending stream." +references = ["smithy-rs#1054", "aws-sdk-rust#391"] +meta = { "breaking" = false, "tada" = false, "bug" = true } +author = "rcoh" diff --git a/aws/sdk/integration-tests/Cargo.toml b/aws/sdk/integration-tests/Cargo.toml index 88afbc68b..ae426a1fb 100644 --- a/aws/sdk/integration-tests/Cargo.toml +++ b/aws/sdk/integration-tests/Cargo.toml @@ -3,6 +3,7 @@ [workspace] members = [ "dynamodb", + "ec2", "iam", "kms", "polly", diff --git a/aws/sdk/integration-tests/dynamodb/tests/paginators.rs b/aws/sdk/integration-tests/dynamodb/tests/paginators.rs index 0c4462862..4e8254b72 100644 --- a/aws/sdk/integration-tests/dynamodb/tests/paginators.rs +++ b/aws/sdk/integration-tests/dynamodb/tests/paginators.rs @@ -53,7 +53,7 @@ fn mk_response(body: &'static str) -> http::Response { http::Response::builder().body(SdkBody::from(body)).unwrap() } -#[tokio::test] +#[tokio::test(flavor = "current_thread")] async fn paginators_loop_until_completion() { let conn = TestConnection::new(vec![ ( @@ -169,3 +169,54 @@ async fn paginators_handle_errors() { rows.try_next().await.expect_err("failure"); assert_eq!(rows.try_next().await.expect("ok"), None); } + +#[tokio::test] +async fn paginators_error_on_repeated_token() { + let response = r#"{ + "Count": 1, + "Items": [{ + "PostedBy": { + "S": "joe@example.com" + } + }], + "LastEvaluatedKey": { + "PostedBy": { "S": "joe@example.com" } + } + }"#; + // send the same response twice with the same pagination token + let conn = TestConnection::new(vec![ + ( + mk_request(r#"{"TableName":"test-table","Limit":32}"#), + mk_response(response), + ), + ( + mk_request( + r#"{"TableName":"test-table","Limit":32,"ExclusiveStartKey":{"PostedBy":{"S":"joe@example.com"}}}"#, + ), + mk_response(response), + ), + ]); + let client = Client::from_conf_conn(stub_config(), conn.clone()); + let mut rows = client + .scan() + .table_name("test-table") + .into_paginator() + .page_size(32) + .items() + .send(); + assert_eq!( + rows.try_next() + .await + .expect("no error") + .expect("not EOS") + .get("PostedBy"), + Some(&AttributeValue::S("joe@example.com".to_string())) + ); + let err = rows.try_next().await.expect_err("failure"); + assert!( + format!("{}", err).contains("next token did not change"), + "{}", + err + ); + assert_eq!(rows.try_next().await.expect("ok"), None); +} diff --git a/aws/sdk/integration-tests/ec2/Cargo.toml b/aws/sdk/integration-tests/ec2/Cargo.toml new file mode 100644 index 000000000..c78e70e6d --- /dev/null +++ b/aws/sdk/integration-tests/ec2/Cargo.toml @@ -0,0 +1,11 @@ +[package] +name = "ec2-tests" +version = "0.1.0" +edition = "2018" + +[dev-dependencies] +aws-sdk-ec2 = { path = "../../build/aws-sdk/sdk/ec2" } +aws-smithy-client = { path = "../../build/aws-sdk/sdk/aws-smithy-client", features = ["test-util"]} +tokio = { version = "1", features = ["full"]} +http = "0.2.6" +tokio-stream = "0.1.8" diff --git a/aws/sdk/integration-tests/ec2/src/lib.rs b/aws/sdk/integration-tests/ec2/src/lib.rs new file mode 100644 index 000000000..c6d888f02 --- /dev/null +++ b/aws/sdk/integration-tests/ec2/src/lib.rs @@ -0,0 +1,4 @@ +/* + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + * SPDX-License-Identifier: Apache-2.0. + */ diff --git a/aws/sdk/integration-tests/ec2/tests/paginators.rs b/aws/sdk/integration-tests/ec2/tests/paginators.rs new file mode 100644 index 000000000..833de6337 --- /dev/null +++ b/aws/sdk/integration-tests/ec2/tests/paginators.rs @@ -0,0 +1,52 @@ +/* + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + * SPDX-License-Identifier: Apache-2.0. + */ + +use aws_sdk_ec2::{model::InstanceType, Client, Config, Credentials, Region}; +use aws_smithy_client::test_connection::TestConnection; +use tokio_stream::StreamExt; + +fn stub_config() -> Config { + Config::builder() + .region(Region::new("us-east-1")) + .credentials_provider(Credentials::new("akid", "secret", None, None, "test")) + .build() +} + +/// See https://github.com/awslabs/aws-sdk-rust/issues/391 +/// +/// EC2 replies with `` which our XML parser parses as empty string and not "none" +#[tokio::test] +async fn paginators_handle_empty_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 6531e3112..0507656b7 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,6 +9,7 @@ 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 @@ -173,8 +174,13 @@ class PaginatorGenerator private constructor( // If the input member is None or it was an error let done = match resp { Ok(ref resp) => { - input.$inputTokenMember = #{output_token}(resp).cloned(); - input.$inputTokenMember.is_none() + let new_token = #{output_token}(resp); + if 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")} }, Err(_) => true, }; @@ -269,6 +275,15 @@ 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()" + } + } + private fun pageSizeSetter() = writable { paginationInfo.pageSizeMember.orNull()?.also { val memberName = symbolProvider.toMemberName(it) diff --git a/codegen/src/test/kotlin/software/amazon/smithy/rust/codegen/smithy/generators/PaginatorGeneratorTest.kt b/codegen/src/test/kotlin/software/amazon/smithy/rust/codegen/smithy/generators/PaginatorGeneratorTest.kt index baf0a9c1a..8898a621b 100644 --- a/codegen/src/test/kotlin/software/amazon/smithy/rust/codegen/smithy/generators/PaginatorGeneratorTest.kt +++ b/codegen/src/test/kotlin/software/amazon/smithy/rust/codegen/smithy/generators/PaginatorGeneratorTest.kt @@ -67,7 +67,7 @@ internal class PaginatorGeneratorTest { """.asSmithyModel() @Test - fun `generate correct paginators`() { + fun `generate paginators that compile`() { val (ctx, testDir) = generatePluginContext(model) RustCodegenPlugin().execute(ctx) "cargo test".runCommand(testDir) -- GitLab