From fc2fa95c701bf4924850d723a88be13a0130b9d5 Mon Sep 17 00:00:00 2001 From: Russell Cohen Date: Fri, 10 Oct 2025 16:03:20 -0400 Subject: [PATCH 1/2] Fix bug where queryParams was ignored if there were no other params --- .changelog/1760126889.md | 12 +++++ AGENTS.md | 49 +++++++++++++++++++ .../http/RequestBindingGenerator.kt | 2 +- .../rest-json-extras.smithy | 44 +++++++++++++++++ .../integration-tests/Cargo.lock | 26 +++++----- gradle.properties | 2 +- 6 files changed, 120 insertions(+), 15 deletions(-) create mode 100644 .changelog/1760126889.md diff --git a/.changelog/1760126889.md b/.changelog/1760126889.md new file mode 100644 index 00000000000..6ee526d6217 --- /dev/null +++ b/.changelog/1760126889.md @@ -0,0 +1,12 @@ +--- +applies_to: + - client +authors: + - rcoh +references: [ "smithy-rs#4346" ] +breaking: false +new_feature: false +bug_fix: true +--- + +Fix bug where httpQueryParams were silently dropped when no other query parameters were modeled. diff --git a/AGENTS.md b/AGENTS.md index 9cd21de318d..2b33d2f052b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -10,6 +10,55 @@ Protocol files: `codegen-{core,server}/.../protocols/` +## Protocol Tests + +Protocol tests validate that generated code correctly implements Smithy protocols (like restJson1, awsJson1_1, etc.). + +### Adding Protocol Tests + +Protocol tests are defined in Smithy model files using `@httpRequestTests` and `@httpResponseTests` traits: + +```smithy +@http(uri: "/my-operation", method: "GET") +@httpRequestTests([ + { + id: "MyOperationRequest", + documentation: "Test description", + protocol: "aws.protocols#restJson1", + method: "GET", + uri: "/my-operation", + queryParams: ["param1=value1", "param2=value2"], + params: { + queryMap: { + "param1": "value1", + "param2": "value2" + } + }, + appliesTo: "client", + } +]) +operation MyOperation { + input: MyOperationInput, +} +``` + +### Key Protocol Test Locations + +- **`codegen-core/common-test-models/rest-json-extras.smithy`** - restJson1 protocol tests +- **`codegen-core/common-test-models/constraints.smithy`** - Constraint validation tests with restJson1 +- **`codegen-client-test/model/main.smithy`** - awsJson1_1 protocol tests + +### httpQueryParams Bug Investigation + +When investigating the `@httpQueryParams` bug (where query parameters weren't appearing in requests), the issue was in `RequestBindingGenerator.kt` line 173. The bug occurred when: + +1. An operation had ONLY `@httpQueryParams` (no regular `@httpQuery` parameters) +2. The condition `if (dynamicParams.isEmpty() && literalParams.isEmpty() && mapParams.isEmpty())` would skip generating the `uri_query` function + +The fix was to ensure `mapParams.isEmpty()` was included in the condition check. The current implementation correctly generates query parameters for `@httpQueryParams` even when no other query parameters exist. + +**Testing httpQueryParams**: Create operations with only `@httpQueryParams` to ensure they generate proper query strings in requests. + ## preludeScope: Rust Prelude Types **Always use `preludeScope` for Rust prelude types:** diff --git a/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/generators/http/RequestBindingGenerator.kt b/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/generators/http/RequestBindingGenerator.kt index f45d6255f79..d9e23715a97 100644 --- a/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/generators/http/RequestBindingGenerator.kt +++ b/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/generators/http/RequestBindingGenerator.kt @@ -170,7 +170,7 @@ class RequestBindingGenerator( val dynamicParams = index.getRequestBindings(operationShape, HttpBinding.Location.QUERY) val mapParams = index.getRequestBindings(operationShape, HttpBinding.Location.QUERY_PARAMS) val literalParams = httpTrait.uri.queryLiterals - if (dynamicParams.isEmpty() && literalParams.isEmpty()) { + if (dynamicParams.isEmpty() && literalParams.isEmpty() && mapParams.isEmpty()) { return false } val preloadedParams = literalParams.keys + dynamicParams.map { it.locationName } diff --git a/codegen-core/common-test-models/rest-json-extras.smithy b/codegen-core/common-test-models/rest-json-extras.smithy index 01f095756ab..9377e6089f1 100644 --- a/codegen-core/common-test-models/rest-json-extras.smithy +++ b/codegen-core/common-test-models/rest-json-extras.smithy @@ -66,6 +66,7 @@ service RestJsonExtras { CaseInsensitiveErrorOperation, EmptyStructWithContentOnWireOp, QueryPrecedence, + HttpQueryParamsOnlyOperation, ], errors: [ExtraError] } @@ -350,3 +351,46 @@ structure EmptyStructWithContentOnWireOpOutput { operation EmptyStructWithContentOnWireOp { output: EmptyStructWithContentOnWireOpOutput, } + +@http(uri: "/http-query-params-only", method: "GET") +@httpRequestTests([ + { + id: "HttpQueryParamsOnlyRequest", + documentation: "Test that httpQueryParams are included in request when no other query parameters exist", + protocol: "aws.protocols#restJson1", + method: "GET", + uri: "/http-query-params-only", + queryParams: ["shouldExpandRoles=true", "shouldShowOnlyAuthForThisDocument=false"], + params: { + queryMap: { + "shouldExpandRoles": "true", + "shouldShowOnlyAuthForThisDocument": "false" + } + }, + appliesTo: "client", + }, + { + id: "HttpQueryParamsOnlyEmptyRequest", + documentation: "Test that empty httpQueryParams map results in no query parameters", + protocol: "aws.protocols#restJson1", + method: "GET", + uri: "/http-query-params-only", + params: { + queryMap: {} + }, + appliesTo: "client", + } +]) +operation HttpQueryParamsOnlyOperation { + input: HttpQueryParamsOnlyInput, +} + +structure HttpQueryParamsOnlyInput { + @httpQueryParams + queryMap: QueryMap, +} + +map QueryMap { + key: String, + value: String, +} diff --git a/codegen-server-test/integration-tests/Cargo.lock b/codegen-server-test/integration-tests/Cargo.lock index 7e5bffb0f46..97ad7c6e578 100644 --- a/codegen-server-test/integration-tests/Cargo.lock +++ b/codegen-server-test/integration-tests/Cargo.lock @@ -50,7 +50,7 @@ checksum = "c08606f8c3cbf4ce6ec8e28fb0014a2c086708fe954eaa885384a6165172e7e8" [[package]] name = "aws-smithy-async" -version = "1.2.5" +version = "1.2.6" dependencies = [ "futures-util", "pin-project-lite", @@ -59,7 +59,7 @@ dependencies = [ [[package]] name = "aws-smithy-cbor" -version = "0.61.1" +version = "0.61.2" dependencies = [ "aws-smithy-types", "minicbor", @@ -67,7 +67,7 @@ dependencies = [ [[package]] name = "aws-smithy-eventstream" -version = "0.60.11" +version = "0.60.12" dependencies = [ "aws-smithy-types", "bytes", @@ -76,7 +76,7 @@ dependencies = [ [[package]] name = "aws-smithy-http" -version = "0.62.4" +version = "0.62.5" dependencies = [ "aws-smithy-eventstream", "aws-smithy-runtime-api", @@ -96,7 +96,7 @@ dependencies = [ [[package]] name = "aws-smithy-http-client" -version = "1.1.2" +version = "1.1.3" dependencies = [ "aws-smithy-async", "aws-smithy-protocol-test", @@ -120,7 +120,7 @@ dependencies = [ [[package]] name = "aws-smithy-http-server" -version = "0.65.6" +version = "0.65.7" dependencies = [ "aws-smithy-cbor", "aws-smithy-http", @@ -148,21 +148,21 @@ dependencies = [ [[package]] name = "aws-smithy-json" -version = "0.61.5" +version = "0.61.6" dependencies = [ "aws-smithy-types", ] [[package]] name = "aws-smithy-observability" -version = "0.1.3" +version = "0.1.4" dependencies = [ "aws-smithy-runtime-api", ] [[package]] name = "aws-smithy-protocol-test" -version = "0.63.4" +version = "0.63.5" dependencies = [ "assert-json-diff", "aws-smithy-runtime-api", @@ -179,7 +179,7 @@ dependencies = [ [[package]] name = "aws-smithy-runtime" -version = "1.9.2" +version = "1.9.3" dependencies = [ "aws-smithy-async", "aws-smithy-http", @@ -202,7 +202,7 @@ dependencies = [ [[package]] name = "aws-smithy-runtime-api" -version = "1.9.0" +version = "1.9.1" dependencies = [ "aws-smithy-async", "aws-smithy-types", @@ -216,7 +216,7 @@ dependencies = [ [[package]] name = "aws-smithy-types" -version = "1.3.2" +version = "1.3.3" dependencies = [ "base64-simd", "bytes", @@ -241,7 +241,7 @@ dependencies = [ [[package]] name = "aws-smithy-xml" -version = "0.60.10" +version = "0.60.11" dependencies = [ "xmlparser", ] diff --git a/gradle.properties b/gradle.properties index 20c04016098..a0939a55c13 100644 --- a/gradle.properties +++ b/gradle.properties @@ -17,4 +17,4 @@ allowLocalDeps=false # Avoid registering dependencies/plugins/tasks that are only used for testing purposes isTestingEnabled=true # codegen publication version -codegenVersion=0.1.4 +codegenVersion=0.1.5 From bf7f54ef6884fb870a22102399732eca957eb0ae Mon Sep 17 00:00:00 2001 From: Russell Cohen Date: Fri, 10 Oct 2025 19:59:22 -0400 Subject: [PATCH 2/2] Update gradle.properties --- gradle.properties | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gradle.properties b/gradle.properties index a0939a55c13..20c04016098 100644 --- a/gradle.properties +++ b/gradle.properties @@ -17,4 +17,4 @@ allowLocalDeps=false # Avoid registering dependencies/plugins/tasks that are only used for testing purposes isTestingEnabled=true # codegen publication version -codegenVersion=0.1.5 +codegenVersion=0.1.4