Repository navigation
fix(kotlin-client): escape $ in wire-name string literals - #25625
khayashi4337 wants to merge 2 commits into
Conversation
Wire names containing `$` (OData-style `$filter`, `$top`) were emitted
verbatim into Kotlin string literals, where `$x` starts interpolation:
the generated key became the same-named parameter's value, or failed to
compile. Escape `$` to `\$` in wire-name string literals across the
kotlin-client libraries: query keys (incl. explode/deepObject and map
forms), path literals and {param} placeholders, headers, multipart field
names and apiKey names.
Overlapping form/multipart emission is left to the sibling work
(kotlin-form-implicit-it / kotlin-vertx-form-body); the retrofit2
exploded-property argument keeps its raw baseName identifier — only the
@query wire string is escaped, so generated argument names do not change.
Add KotlinClientCodegenApiTest.testDollarWireNamesAreEscaped covering all
eight kotlin libraries with a fixture exercising $param, $path and
$api_key wire names.
Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
1 issue found across 19 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="modules/openapi-generator/src/main/resources/kotlin-client/libraries/jvm-retrofit2/explodedQueryParam.mustache">
<violation number="1" location="modules/openapi-generator/src/main/resources/kotlin-client/libraries/jvm-retrofit2/explodedQueryParam.mustache:1">
P2: An exploded property with `$` in its wire name still becomes an invalid Kotlin argument identifier because this emits raw `baseName` after `@Query`. Use the sanitized `name` for the argument identifier and keep `baseName` only for the wire key.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Re-trigger cubic
| @@ -1 +1 @@ | |||
| @Query("{{baseName}}") {{{baseName}}}: {{#collectionFormat}}{{#isCollectionFormatMulti}}{{{dataType}}}{{/isCollectionFormatMulti}}{{^isCollectionFormatMulti}}{{{collectionFormat.toUpperCase}}}Params{{/isCollectionFormatMulti}}{{/collectionFormat}}{{^collectionFormat}}{{{dataType}}}{{/collectionFormat}}{{#required}}{{#defaultValue}} = {{^isNumber}}{{{defaultValue}}}{{/isNumber}}{{#isNumber}}{{{dataType}}}("{{{defaultValue}}}"){{/isNumber}}{{/defaultValue}}{{/required}}{{^required}}?{{#defaultValue}} = {{^isNumber}}{{{defaultValue}}}{{/isNumber}}{{#isNumber}}{{{dataType}}}("{{{defaultValue}}}"){{/isNumber}}{{/defaultValue}}{{^defaultValue}} = null{{/defaultValue}}{{/required}} No newline at end of file | |||
| @Query("{{#lambda.escapeDollar}}{{baseName}}{{/lambda.escapeDollar}}") {{{baseName}}}: {{#collectionFormat}}{{#isCollectionFormatMulti}}{{{dataType}}}{{/isCollectionFormatMulti}}{{^isCollectionFormatMulti}}{{{collectionFormat.toUpperCase}}}Params{{/isCollectionFormatMulti}}{{/collectionFormat}}{{^collectionFormat}}{{{dataType}}}{{/collectionFormat}}{{#required}}{{#defaultValue}} = {{^isNumber}}{{{defaultValue}}}{{/isNumber}}{{#isNumber}}{{{dataType}}}("{{{defaultValue}}}"){{/isNumber}}{{/defaultValue}}{{/required}}{{^required}}?{{#defaultValue}} = {{^isNumber}}{{{defaultValue}}}{{/isNumber}}{{#isNumber}}{{{dataType}}}("{{{defaultValue}}}"){{/isNumber}}{{/defaultValue}}{{^defaultValue}} = null{{/defaultValue}}{{/required}} No newline at end of file | |||
There was a problem hiding this comment.
P2: An exploded property with $ in its wire name still becomes an invalid Kotlin argument identifier because this emits raw baseName after @Query. Use the sanitized name for the argument identifier and keep baseName only for the wire key.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At modules/openapi-generator/src/main/resources/kotlin-client/libraries/jvm-retrofit2/explodedQueryParam.mustache, line 1:
<comment>An exploded property with `$` in its wire name still becomes an invalid Kotlin argument identifier because this emits raw `baseName` after `@Query`. Use the sanitized `name` for the argument identifier and keep `baseName` only for the wire key.</comment>
<file context>
@@ -1 +1 @@
-@Query("{{baseName}}") {{{baseName}}}: {{#collectionFormat}}{{#isCollectionFormatMulti}}{{{dataType}}}{{/isCollectionFormatMulti}}{{^isCollectionFormatMulti}}{{{collectionFormat.toUpperCase}}}Params{{/isCollectionFormatMulti}}{{/collectionFormat}}{{^collectionFormat}}{{{dataType}}}{{/collectionFormat}}{{#required}}{{#defaultValue}} = {{^isNumber}}{{{defaultValue}}}{{/isNumber}}{{#isNumber}}{{{dataType}}}("{{{defaultValue}}}"){{/isNumber}}{{/defaultValue}}{{/required}}{{^required}}?{{#defaultValue}} = {{^isNumber}}{{{defaultValue}}}{{/isNumber}}{{#isNumber}}{{{dataType}}}("{{{defaultValue}}}"){{/isNumber}}{{/defaultValue}}{{^defaultValue}} = null{{/defaultValue}}{{/required}}
\ No newline at end of file
+@Query("{{#lambda.escapeDollar}}{{baseName}}{{/lambda.escapeDollar}}") {{{baseName}}}: {{#collectionFormat}}{{#isCollectionFormatMulti}}{{{dataType}}}{{/isCollectionFormatMulti}}{{^isCollectionFormatMulti}}{{{collectionFormat.toUpperCase}}}Params{{/isCollectionFormatMulti}}{{/collectionFormat}}{{^collectionFormat}}{{{dataType}}}{{/collectionFormat}}{{#required}}{{#defaultValue}} = {{^isNumber}}{{{defaultValue}}}{{/isNumber}}{{#isNumber}}{{{dataType}}}("{{{defaultValue}}}"){{/isNumber}}{{/defaultValue}}{{/required}}{{^required}}?{{#defaultValue}} = {{^isNumber}}{{{defaultValue}}}{{/isNumber}}{{#isNumber}}{{{dataType}}}("{{{defaultValue}}}"){{/isNumber}}{{/defaultValue}}{{^defaultValue}} = null{{/defaultValue}}{{/required}}
\ No newline at end of file
</file context>
| @Query("{{#lambda.escapeDollar}}{{baseName}}{{/lambda.escapeDollar}}") {{{baseName}}}: {{#collectionFormat}}{{#isCollectionFormatMulti}}{{{dataType}}}{{/isCollectionFormatMulti}}{{^isCollectionFormatMulti}}{{{collectionFormat.toUpperCase}}}Params{{/isCollectionFormatMulti}}{{/collectionFormat}}{{^collectionFormat}}{{{dataType}}}{{/collectionFormat}}{{#required}}{{#defaultValue}} = {{^isNumber}}{{{defaultValue}}}{{/isNumber}}{{#isNumber}}{{{dataType}}}("{{{defaultValue}}}"){{/isNumber}}{{/defaultValue}}{{/required}}{{^required}}?{{#defaultValue}} = {{^isNumber}}{{{defaultValue}}}{{/isNumber}}{{#isNumber}}{{{dataType}}}("{{{defaultValue}}}"){{/isNumber}}{{/defaultValue}}{{^defaultValue}} = null{{/defaultValue}}{{/required}} | |
| @Query("{{#lambda.escapeDollar}}{{baseName}}{{/lambda.escapeDollar}}") {{{name}}}: {{#collectionFormat}}{{#isCollectionFormatMulti}}{{{dataType}}}{{/isCollectionFormatMulti}}{{^isCollectionFormatMulti}}{{{collectionFormat.toUpperCase}}}Params{{/isCollectionFormatMulti}}{{/collectionFormat}}{{^collectionFormat}}{{{dataType}}}{{/collectionFormat}}{{#required}}{{#defaultValue}} = {{^isNumber}}{{{defaultValue}}}{{/isNumber}}{{#isNumber}}{{{dataType}}}("{{{defaultValue}}}"){{/isNumber}}{{/defaultValue}}{{/required}}{{^required}}?{{#defaultValue}} = {{^isNumber}}{{{defaultValue}}}{{/isNumber}}{{#isNumber}}{{{dataType}}}("{{{defaultValue}}}"){{/isNumber}}{{/defaultValue}}{{^defaultValue}} = null{{/defaultValue}}{{/required}} |
There was a problem hiding this comment.
This is intentional. Switching the argument from the raw baseName to the sanitized name renames existing public arguments for everyone who passes them by name (e.g. values becomes propertyValues in the echo_api sample), and it can also collide with another query parameter that sanitizes to the same name. This PR only escapes the wire string in @Query("…"); the argument identifier is called out as unchanged in the PR description.
…e $-escape test The dedicated librariesWithDollarWireNames provider listed the same eight libraries as the existing clientLibraries provider; the duplication would drift the next time the shared list changes. Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
In the
kotlinclient, a$in a wire name (the name that goes on the wire: the operation path, a path placeholder, a query/header/multipart key or an API-key name) is written into a Kotlin string literal unescaped in most libraries. Kotlin reads$nameinside a string as a template for the variablename, so OData-style names such as$filter,$top,/odata/$countor{$id}either fail to compile or are sent under a different key.Reproducer
modules/openapi-generator/src/test/resources/3_0/kotlin/kotlin-dollar-wire-name.yaml(added by this PR) declares:$filter,$topandfilter, and a header$x-head;opts(each property is sent as its own key) with a property$a, and a deepObject parameter$dobjwith a property$f;$fprop, the paths/items/{$id}and/odata/$count, and an API key named$api_key.What master generates from it:
$top,$idand$counthave no variable of that name in scope. They are unresolved references, so the file does not compile.$filterresolves to thefilterparameter that the fixture also declares. Injvm-ktor,jvm-vertx,multiplatform,jvm-volleyand the twojvm-spring-*libraries that literal is an ordinary string, so it compiles and the query key becomes that parameter's value instead of$filter. (jvm-okhttpalready escaped its query keys.)jvm-retrofit2every wire name sits in an annotation argument (@GET,@Path,@Query), which must be a compile-time constant, so a template there is a compile error even for$filter.jvm-ktoron master:After this PR:
Fix
The wire names are wrapped in
{{#lambda.escapeDollar}}…{{/lambda.escapeDollar}}. The lambda already exists inAbstractKotlinCodegen; it turns$into\$and leaves an already escaped\$alone. Some templates already used it, for example the query keys injvm-okhttp. Only string literals are wrapped:jvm-vertxonly the{{{path}}}part, so the$basePathinterpolation is kept);keyParamName) in theApiClienttemplates ofjvm-okhttp,jvm-ktor,jvm-retrofit2andmultiplatform, and injvm-vertx/api.mustache.jvm-microprofilealready escaped these names on master and is not touched.Deliberately left out, to keep this PR from editing the same lines as the open sibling PRs #25623 and #25624:
jvm-ktor,multiplatformandjvm-vertx, and the header keys ofjvm-vertx;multiplatform, where fix(kotlin-client): avoid name shadowing in ktor/multiplatform form bodies and responses #25624 replaces the wire name with the parameter.I will follow up on these once those PRs are merged.
Also not changed: in
jvm-retrofit2, the Kotlin parameter generated for each property of an exploded object query parameter still uses the raw schema name (@Query("\$a") $a: …). Changing it would rename public arguments for everyone who calls them by name.Caveat for
jvm-retrofit2path placeholders: the generated code now compiles, but Retrofit itself only accepts@Pathnames matching[a-zA-Z][a-zA-Z0-9_-]*, so a placeholder such as{$id}is still rejected by Retrofit at runtime. Query keys like@Query("\$filter")are not affected by that check.Behavior change
For specs with a
$in a wire name, the escaped sites now send the literal name. Where the old code compiled only because$namehappened to match a variable ($filternext to afilterparameter), the key sent changes from that variable's value to the literal$filter.Tests
testDollarWireNamesAreEscapedjvm-ktor,jvm-okhttp4,jvm-retrofit2,multiplatform,jvm-vertx,jvm-volley,jvm-spring-webclient,jvm-spring-restclientodata/\$countand\$idare emitted, and no unescaped"$top"orodata/$count"literal. Per library, the escaped keys it emits, e.g.localVariableQuery["\$dobj[\$f]"]andlocalVariableHeaders["\$x-head"](jvm-ktor),@Query("\$a"),@Header("\$x-head"),@Path("\$id")and@Part("\$fprop")(jvm-retrofit2),apiKey["\$api_key"](jvm-vertx). For the four libraries that put the API key inApiClient.kt, that file contains\$api_key../mvnw -nsu -Denforcer.skip=true -pl modules/openapi-generator -am -Dtest=KotlinClientCodegenApiTest -Dsurefire.failIfNoSpecifiedTests=false testtestDollarWireNamesAreEscapedThe test checks generated text; it does not compile the generated Kotlin.
Samples
No sample changes. Of the 26 specs used by the kotlin-client configs, only
3_0/parameter-test-spec.yamlhas a$in a wire name (the query parameter$query-$dollar-sign). Its config,kotlin-jvm-okhttp-parameter-tests, usesjvm-okhttp4, which already escaped that key on master (put("\$query-\$dollar-sign", …)). TheSamples up-to-dateCI job checks the rest.This PR and its sibling PRs #25622, #25623 and #25624 all touch kotlin-client templates.
git merge-treeshows no template conflicts between this branch and any of them, and the new test is added at the end ofKotlinClientCodegenApiTestto stay clear of where the siblings add theirs.cc @wing328 and the Kotlin technical committee: @karismann @Zomzog @andrewemery @4brunu @yutaka0m @stefankoppier @e5l
PR checklist
./mvnw -pl modules/openapi-generator-cli -am package -DskipTests) and regenerated the affected kotlin client configs with./bin/generate-samples.sh; there is no sample diff.export_docs_generators.shnot run: no generator option or metadata changes.Commit all changed files.
This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
These must match the expectations made by your contribution.
You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example
./bin/generate-samples.sh bin/configs/java*.IMPORTANT: Do NOT purge/delete any folders/files (e.g. tests) when regenerating the samples as manually written tests may be removed.
🤖 Generated with Claude Code
Summary by cubic
Fixes the kotlin-client generators emitting wire names containing
$unescaped, so OData-style names like$filter,$top,/odata/$count, and{$id}either broke Kotlin string interpolation or silently used a same-named parameter's value. Wire-name string literals (operation paths, path placeholders, query/header/multipart keys, API-key names) now pass through the existingescapeDollarlambda, which turns$into\$, in eight generated libraries.Behavior change
$namepreviously matched a parameter, the generated key now goes out as the literal wire name (e.g.$filter) instead of that parameter's value.jvm-retrofit2annotations now compile, but Retrofit's runtime validation still rejects{$id}-style path placeholders; query and header keys are unaffected.jvm-ktor,multiplatform, andjvm-vertx, and the multipart file part inmultiplatform.Tests
testDollarWireNamesAreEscapedcovers all eight libraries via the sharedclientLibrariesprovider, using the newkotlin-dollar-wire-name.yamlfixture.Written for commit 8fab58d. Summary will update on new commits.