Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
6 issues found across 28 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="modules/openapi-generator/src/main/resources/typescript-fetch/apis.mustache">
<violation number="1" location="modules/openapi-generator/src/main/resources/typescript-fetch/apis.mustache:214">
P1: Form-style exploded parameters whose schemas are declared objects still serialize as `query_object[property]=value` because this branch only recognizes maps. Include object models in the per-property loop, while retaining whole-object assignment for deepObject and non-exploded parameters.</violation>
<violation number="2" location="modules/openapi-generator/src/main/resources/typescript-fetch/apis.mustache:216">
P2: When an exploded map contains an own `__proto__` key, this assignment drops that query entry during stringification. Define the property explicitly or use a null-prototype query-parameter object.</violation>
</file>
<file name="modules/openapi-generator/src/test/java/org/openapitools/codegen/go/GoClientCodegenTest.java">
<violation number="1" location="modules/openapi-generator/src/test/java/org/openapitools/codegen/go/GoClientCodegenTest.java:172">
P1: These assertions expect the generated api_default.go to pass "form" to parameterAddToHeaderOrQuery for filter and typedFilter, but GoClientCodegen only sets codegenParameter.style when the OpenAPI document declares a style. The fixture leaves style unset for those two params (they default to form by spec, not by the codegen), so the generated calls carry an empty style (`"", ""`), as the regenerated sample does for every no-style query param (e.g. api_fake.go: `"query", r.query, "", ""`). The assertion won't match and the test fails. Because the new un-bracketing in go/client.mustache is gated on `if style == "form"`, passing an empty style also means the runtime never explodes these default-style objects, so the fix wouldn't take effect for them either. Declare `style: form` explicitly on filter and typedFilter in the fixture (and regenerate), or pass the style through so the empty default becomes "form".</violation>
</file>
<file name="samples/openapi3/client/petstore/python-aiohttp/petstore_api/api/fake_api.py">
<violation number="1" location="samples/openapi3/client/petstore/python-aiohttp/petstore_api/api/fake_api.py:9540">
P2: When an object property name contains a query delimiter, this loop uses it directly as the query key while the serializer only quotes values, producing a malformed or misparsed URL. Encode `_key` before appending it, including the required `urllib.parse.quote` import.</violation>
</file>
<file name="modules/openapi-generator/src/main/resources/python/api.mustache">
<violation number="1" location="modules/openapi-generator/src/main/resources/python/api.mustache:352">
P1: When a query object uses `deepObject`, this branch still sends the whole dict under `baseName`, so Python emits JSON instead of `deepFilter[key]=value`. Emit one query entry per item using `{{baseName}}[key]` names in this branch.</violation>
</file>
<file name="modules/openapi-generator/src/main/resources/go/client.mustache">
<violation number="1" location="modules/openapi-generator/src/main/resources/go/client.mustache:214">
P2: The `if style == "form"` branch explodes every form-style object to per-entry keys even when `explode: false` (e.g. flatFilter), because Go never passes `explode` to the runtime. This produces `key=value` entries for flatFilter instead of the single comma-joined `flatFilter=key,value` the new fixture documents. The new Go test checks only the call-string and not the wire output, so it does not catch the mismatch; either propagate explode into the runtime or make the test verify the actual serialized output.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| {{#isMap}} | ||
| for (let key of Object.keys(requestParameters['{{paramName}}'])) { | ||
| queryParameters[key] = requestParameters['{{paramName}}'][key]; | ||
| queryParameters[key] = (requestParameters['{{paramName}}'] as any)[key]; | ||
| } | ||
| {{/isContainer}} | ||
| {{^isContainer}} | ||
| {{/isMap}} | ||
| {{^isMap}} | ||
| {{>apisAssignQueryParam}} | ||
| {{/isContainer}} | ||
| {{/isMap}} |
There was a problem hiding this comment.
P1: Form-style exploded parameters whose schemas are declared objects still serialize as query_object[property]=value because this branch only recognizes maps. Include object models in the per-property loop, while retaining whole-object assignment for deepObject and non-exploded parameters.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At modules/openapi-generator/src/main/resources/typescript-fetch/apis.mustache, line 214:
<comment>Form-style exploded parameters whose schemas are declared objects still serialize as `query_object[property]=value` because this branch only recognizes maps. Include object models in the per-property loop, while retaining whole-object assignment for deepObject and non-exploded parameters.</comment>
<file context>
@@ -202,14 +202,24 @@ export class {{classname}} extends runtime.BaseAPI {
+{{>apisAssignQueryParam}}
+ {{/isDeepObject}}
+ {{^isDeepObject}}
+ {{#isMap}}
for (let key of Object.keys(requestParameters['{{paramName}}'])) {
- queryParameters[key] = requestParameters['{{paramName}}'][key];
</file context>
| {{#isMap}} | |
| for (let key of Object.keys(requestParameters['{{paramName}}'])) { | |
| queryParameters[key] = requestParameters['{{paramName}}'][key]; | |
| queryParameters[key] = (requestParameters['{{paramName}}'] as any)[key]; | |
| } | |
| {{/isContainer}} | |
| {{^isContainer}} | |
| {{/isMap}} | |
| {{^isMap}} | |
| {{>apisAssignQueryParam}} | |
| {{/isContainer}} | |
| {{/isMap}} | |
| {{#isMap}} | |
| for (let key of Object.keys(requestParameters['{{paramName}}'])) { | |
| queryParameters[key] = (requestParameters['{{paramName}}'] as any)[key]; | |
| } | |
| {{/isMap}} | |
| {{^isMap}} | |
| {{#isModel}} | |
| for (let key of Object.keys(requestParameters['{{paramName}}'])) { | |
| queryParameters[key] = (requestParameters['{{paramName}}'] as any)[key]; | |
| } | |
| {{/isModel}} | |
| {{^isModel}} | |
| {{>apisAssignQueryParam}} | |
| {{/isModel}} | |
| {{/isMap}} |
There was a problem hiding this comment.
The gap is real — a $refed object model is isModel, not isMap, so it does still go on the wire whole. But it is pre-existing, and the suggested fix would put the wrong names on the wire.
typescript-fetch interface properties are not the wire names; translating between the two is exactly what the ToJSON functions exist for. From samples/client/petstore/typescript-fetch/builds/default-v3.0/models/FormatTest.ts:
'float': value['_float'],
'pattern_with_digits': value['patternWithDigits'],
'dateTime': value['dateTime'] == null ? value['dateTime'] : serializeDateTime(value['dateTime']),So Object.keys(model) over that parameter emits _float=… and patternWithDigits=…, and hands the serializer a raw Date for dateTime. Applying the suggestion verbatim would trade one wrong serialization for another.
A correct fix has to route through {{dataType}}ToJSON(...) before iterating, which is a feature rather than a template branch, and it wants its own tests and sample churn. It is also outside what this PR set out to change — the fixture and the isContainer → isMap move are both about free-form objects and declared maps. I have noted it as a known gap in the description and am happy to open a separate issue for it.
| {{#isDeepObject}} | ||
| _query_params.append(('{{baseName}}', {{paramName}})) |
There was a problem hiding this comment.
P1: When a query object uses deepObject, this branch still sends the whole dict under baseName, so Python emits JSON instead of deepFilter[key]=value. Emit one query entry per item using {{baseName}}[key] names in this branch.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At modules/openapi-generator/src/main/resources/python/api.mustache, line 352:
<comment>When a query object uses `deepObject`, this branch still sends the whole dict under `baseName`, so Python emits JSON instead of `deepFilter[key]=value`. Emit one query entry per item using `{{baseName}}[key]` names in this branch.</comment>
<file context>
@@ -347,7 +347,25 @@ https://github.com/OpenAPITools/openapi-generator/blob/c84b949df1a9ec04ba75989cb
{{^isDateTime}}{{^isDate}}
+ {{#isMap}}
+ {{#isExplode}}
+ {{#isDeepObject}}
+ _query_params.append(('{{baseName}}', {{paramName}}))
+ {{/isDeepObject}}
</file context>
| {{#isDeepObject}} | |
| _query_params.append(('{{baseName}}', {{paramName}})) | |
| {{#isDeepObject}} | |
| for _key, _value in {{paramName}}.items(): | |
| _query_params.append((f'{{baseName}}[{_key}]', _value)) |
There was a problem hiding this comment.
Correct on the facts — python emits deepFilter={"category": "books"} where the spec asks for deepFilter[category]=books — but this is pre-existing behaviour rather than something this PR introduces. It is byte-for-byte what the template did before, which is why the description lists python's deepFilter row as "one json parameter" under unchanged.
This PR is scoped to one thing: making go, python and typescript-fetch agree with csharp, java and php on form-style objects. deepObject serialization in python is a separate defect with its own blast radius, and folding it in makes this harder to review and to revert.
I have written it up as a known gap in the description so it is not lost. It is a small change if a maintainer would rather have it here than in a follow-up — say the word and I will add it along with the fixture and test rows:
{{#isDeepObject}}
for _key, _value in {{paramName}}.items():
_query_params.append((f'{{baseName}}[{_key}]', _value))
{{/isDeepObject}}| // of which differ from the comma joined pairs the specification asks for. | ||
| Path api = Paths.get(output + "/api_default.go"); | ||
| TestUtils.assertFileContains(api, | ||
| "parameterAddToHeaderOrQuery(localVarQueryParams, \"filter\", r.filter, \"form\", \"\")", |
There was a problem hiding this comment.
P1: These assertions expect the generated api_default.go to pass "form" to parameterAddToHeaderOrQuery for filter and typedFilter, but GoClientCodegen only sets codegenParameter.style when the OpenAPI document declares a style. The fixture leaves style unset for those two params (they default to form by spec, not by the codegen), so the generated calls carry an empty style ("", ""), as the regenerated sample does for every no-style query param (e.g. api_fake.go: "query", r.query, "", ""). The assertion won't match and the test fails. Because the new un-bracketing in go/client.mustache is gated on if style == "form", passing an empty style also means the runtime never explodes these default-style objects, so the fix wouldn't take effect for them either. Declare style: form explicitly on filter and typedFilter in the fixture (and regenerate), or pass the style through so the empty default becomes "form".
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At modules/openapi-generator/src/test/java/org/openapitools/codegen/go/GoClientCodegenTest.java, line 172:
<comment>These assertions expect the generated api_default.go to pass "form" to parameterAddToHeaderOrQuery for filter and typedFilter, but GoClientCodegen only sets codegenParameter.style when the OpenAPI document declares a style. The fixture leaves style unset for those two params (they default to form by spec, not by the codegen), so the generated calls carry an empty style (`"", ""`), as the regenerated sample does for every no-style query param (e.g. api_fake.go: `"query", r.query, "", ""`). The assertion won't match and the test fails. Because the new un-bracketing in go/client.mustache is gated on `if style == "form"`, passing an empty style also means the runtime never explodes these default-style objects, so the fix wouldn't take effect for them either. Declare `style: form` explicitly on filter and typedFilter in the fixture (and regenerate), or pass the style through so the empty default becomes "form".</comment>
<file context>
@@ -141,6 +141,40 @@ public void testPrimitiveTypeInOneOf() throws IOException {
+ // of which differ from the comma joined pairs the specification asks for.
+ Path api = Paths.get(output + "/api_default.go");
+ TestUtils.assertFileContains(api,
+ "parameterAddToHeaderOrQuery(localVarQueryParams, \"filter\", r.filter, \"form\", \"\")",
+ "parameterAddToHeaderOrQuery(localVarQueryParams, \"typedFilter\", r.typedFilter, \"form\", \"\")",
+ "parameterAddToHeaderOrQuery(localVarQueryParams, \"deepFilter\", r.deepFilter, \"deepObject\", \"\")",
</file context>
There was a problem hiding this comment.
Not valid — the test passes.
[INFO] Tests run: 382, Failures: 0, Errors: 0, Skipped: 0
[INFO] BUILD SUCCESS
(GoClientCodegenTest, GoModelTest, GoClientOptionsTest, AbstractGoCodegenTest, PythonClientCodegenTest, PythonPydanticV1ClientCodegenTest, TypeScriptFetchClientCodegenTest, TypeScriptFetchModelTest, TypeScriptFetchClientOptionsTest, DefaultCodegenTest.)
The first step of the reasoning is right: DefaultCodegen.fromParameter only assigns codegenParameter.style inside if (parameter.getStyle() != null). The second step is where it goes wrong — swagger-parser has already applied the OpenAPI default by the time we get there, so getStyle() returns FORM for a query parameter that declares no style. It is never null, and the codegen does not need to supply the default itself.
The regenerated samples show it directly. testBodyWithQueryParams in petstore-with-fake-endpoints-models-for-testing.yaml:1095 declares no style:, and the generated client is:
parameterAddToHeaderOrQuery(localVarQueryParams, "query", r.query, "form", "")not the "", "" this comment predicts — there is no ", "", "" anywhere in the go samples. So the fixture does not need an explicit style: form, and the runtime does explode these default-style objects.
| # form style explodes an object into one parameter per entry, keyed by the | ||
| # property name alone | ||
| for _key, _value in language.items(): | ||
| _query_params.append((_key, _value)) |
There was a problem hiding this comment.
P2: When an object property name contains a query delimiter, this loop uses it directly as the query key while the serializer only quotes values, producing a malformed or misparsed URL. Encode _key before appending it, including the required urllib.parse.quote import.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At samples/openapi3/client/petstore/python-aiohttp/petstore_api/api/fake_api.py, line 9540:
<comment>When an object property name contains a query delimiter, this loop uses it directly as the query key while the serializer only quotes values, producing a malformed or misparsed URL. Encode `_key` before appending it, including the required `urllib.parse.quote` import.</comment>
<file context>
@@ -9534,7 +9534,10 @@ def _test_query_parameter_collection_format_serialize(
+ # form style explodes an object into one parameter per entry, keyed by the
+ # property name alone
+ for _key, _value in language.items():
+ _query_params.append((_key, _value))
if allow_empty is not None:
</file context>
There was a problem hiding this comment.
Valid — fixed in fbe9fdc, though not quite where you suggested.
ApiClient.parameters_to_url_query quotes values and never quotes names. That was harmless while every name came from baseName in the document, but this PR makes the name runtime data for the first time, so a property containing & or = would break the URL.
Rather than import quote into every generated api.mustache file for one call site, I fixed the asymmetry where the value encoding already lives — parameters_to_url_query now quotes the name too:
new_params.append((quote(str(k)), quote(str(v))))and likewise in the two collection_formats branches. That brings python in line with go, which encodes via url.Values.Encode(), and typescript-fetch, which encodes via encodeURIComponent. Spec-declared names are unaffected in practice — they are almost always already URL-safe.
Verified against the new serializer:
{"category": "books", "createdDate:gte": "2023-01-01"} -> category=books&createdDate%3Agte=2023-01-01
{"a&b=c": "v"} -> a%26b%3Dc=v
The first line is now byte-identical to what go and typescript-fetch put on the wire, so the table in the description is uniform across all three.
There was a problem hiding this comment.
Correction to the last sentence (the python change now lives in #24802): declared names are quoted too, so a declared name with reserved characters changes on the wire, e.g. page[size] now goes out as page%5Bsize%5D and ids[] as ids%5B%5D. Servers decode both forms to the same name; #24802's description now says so.
| {{#isMap}} | ||
| for (let key of Object.keys(requestParameters['{{paramName}}'])) { | ||
| queryParameters[key] = requestParameters['{{paramName}}'][key]; | ||
| queryParameters[key] = (requestParameters['{{paramName}}'] as any)[key]; |
There was a problem hiding this comment.
P2: When an exploded map contains an own __proto__ key, this assignment drops that query entry during stringification. Define the property explicitly or use a null-prototype query-parameter object.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At modules/openapi-generator/src/main/resources/typescript-fetch/apis.mustache, line 216:
<comment>When an exploded map contains an own `__proto__` key, this assignment drops that query entry during stringification. Define the property explicitly or use a null-prototype query-parameter object.</comment>
<file context>
@@ -202,14 +202,24 @@ export class {{classname}} extends runtime.BaseAPI {
+ {{#isMap}}
for (let key of Object.keys(requestParameters['{{paramName}}'])) {
- queryParameters[key] = requestParameters['{{paramName}}'][key];
+ queryParameters[key] = (requestParameters['{{paramName}}'] as any)[key];
}
- {{/isContainer}}
</file context>
| queryParameters[key] = (requestParameters['{{paramName}}'] as any)[key]; | |
| Object.defineProperty(queryParameters, key, { value: (requestParameters['{{paramName}}'] as any)[key], enumerable: true, configurable: true, writable: true }); |
There was a problem hiding this comment.
Accurate as a JS observation — queryParameters['__proto__'] = 'x' does hit the Object.prototype setter and silently no-op for a string value.
Not fixing it here, for two reasons. First, it is pre-existing: this is the same assignment the {{#isContainer}} branch already performed for every declared map before this PR. The diff moved the line, it did not introduce the behaviour, and the runtime's HTTPQuery handling has the same property throughout.
Second, the suggested Object.defineProperty(queryParameters, key, { value: …, enumerable: true, configurable: true, writable: true }) lands in every generated api method that takes an exploded object. That is a lot of noise in code people read, for a key that has to be literally __proto__.
If a maintainer does want this hardened, the right shape is a single null-prototype queryParameters object in apis.mustache, applied uniformly rather than per parameter — happy to do that as its own change.
There was a problem hiding this comment.
| // form style explodes an object into one parameter per entry, keyed by the | ||
| // property name alone. Only deepObject nests the property under the | ||
| // parameter name. | ||
| keyPrefixForMapEntry = k.String() |
There was a problem hiding this comment.
P2: The if style == "form" branch explodes every form-style object to per-entry keys even when explode: false (e.g. flatFilter), because Go never passes explode to the runtime. This produces key=value entries for flatFilter instead of the single comma-joined flatFilter=key,value the new fixture documents. The new Go test checks only the call-string and not the wire output, so it does not catch the mismatch; either propagate explode into the runtime or make the test verify the actual serialized output.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At modules/openapi-generator/src/main/resources/go/client.mustache, line 214:
<comment>The `if style == "form"` branch explodes every form-style object to per-entry keys even when `explode: false` (e.g. flatFilter), because Go never passes `explode` to the runtime. This produces `key=value` entries for flatFilter instead of the single comma-joined `flatFilter=key,value` the new fixture documents. The new Go test checks only the call-string and not the wire output, so it does not catch the mismatch; either propagate explode into the runtime or make the test verify the actual serialized output.</comment>
<file context>
@@ -206,7 +206,14 @@ func parameterAddToHeaderOrQuery(headerOrQueryParams interface{}, keyPrefix stri
+ // form style explodes an object into one parameter per entry, keyed by the
+ // property name alone. Only deepObject nests the property under the
+ // parameter name.
+ keyPrefixForMapEntry = k.String()
+ }
+ parameterAddToHeaderOrQuery(headerOrQueryParams, keyPrefixForMapEntry, v.Interface(), style, collectionType)
</file context>
There was a problem hiding this comment.
Known — this is the "Known gaps, called out deliberately" section of the description, and the same caveat is repeated in a comment in the go test.
The go templates pass only style to the runtime, never explode, so parameterAddToHeaderOrQuery genuinely cannot tell the two apart. For flatFilter the spec asks for flatFilter=category,books; go bracketed it (flatFilter[category]=books) before this change and explodes it (category=books) after. Both are wrong, so this is not a regression in correctness — but I agree it is not a fix either, which is why it is documented rather than claimed.
On the test: it asserts on the emitted source because there is no harness in the repo that exercises parameterAddToHeaderOrQuery at runtime — nothing under samples/**/*_test.go references it. Verifying actual wire output would mean adding that harness, which I am glad to do if it is wanted.
Plumbing explode properly changes parameterAddToHeaderOrQuery's signature and all 12 call sites in go/api.mustache. I offered that in the description and the offer stands — it just wants a maintainer's call, since it changes a runtime function signature that generated code has depended on for a long time.
There was a problem hiding this comment.
Two corrections: the explode: false caveat now lives only in the description (d5cf44b dropped the test comment), and TestQueryDeepObject in samples/openapi3/client/petstore/go/fake_api_test.go does exercise parameterAddToHeaderOrQuery at runtime; it just has no form-style map parameter to reach the new branch.
|
thanks for the PR what about splitting this PR into 3 targeting go, python, typescript-fetch separately for easier review? |
A query parameter whose schema is an object and whose style/explode are left at
their defaults — style: form, explode: true — must go on the wire as one
parameter per entry, keyed by the property name alone. The go client bracketed
it as filter[category]=books instead.
parameterAddToHeaderOrQuery in go/client.mustache bracketed every map whatever
style it was handed. The reflect.Slice case immediately above it already
branches on style == "deepObject", so the map case looks like an oversight. The
generated api passes the style correctly; nothing downstream read it.
The flattening applies to the top level only. The recursive call no longer
carries the style down, so anything nested inside an entry keeps the path it has
accumulated: {profile: {name: "x"}} is profile[name]=x, and two siblings holding
the same property name stay distinct — {a: {x: 1}, b: {x: 2}} is a[x]=1&b[x]=2
rather than a pair of colliding x= entries. Nested slices are unaffected, since
the slice branch only special cases deepObject and so behaves identically for an
empty style and for form.
fbe9fdc to
6d0e539
Compare
wiebren
changed the title
|
Split per language as requested. This PR is now go only — the
The three touch disjoint sets of files under Each branch was tested on its own after the split, not just as part of the original combined |
There was a problem hiding this comment.
All reported issues were addressed across 15 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
For anyone reading top to bottom: cubic's review above ran against P1, which is exactly what the test asserts, and it passes. The supporting evidence cited — So no fixture change is needed, and the fix does fire for default-style objects. P2, |
A form style query parameter whose value is an array of maps reached the map branch one level down with the style still set, so the entries were keyed by their property names alone and the accumulated parent path was dropped. Reset the style when descending into array elements, mirroring the reset the map branch already does for its entries: only a map that is the parameter's direct value is exploded to bare property names. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Go3AyndcGwv5tFTwo9aBfy
There was a problem hiding this comment.
1 issue found across 16 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="modules/openapi-generator/src/test/java/org/openapitools/codegen/go/GoClientCodegenTest.java">
<violation number="1" location="modules/openapi-generator/src/test/java/org/openapitools/codegen/go/GoClientCodegenTest.java:169">
P3: These new assertions only verify that the generic `parameterAddToHeaderOrQuery` slice branch (always emitted into every client.go) contains the `styleForElement` reset, so they hold regardless of the fixture's parameters. The `exploded-object-query-param.yaml` fixture has no array-typed query parameter, so the array-element flattening fix (470b7e0) these lines were added to protect is never actually exercised. Add an array-of-objects query param to the fixture (e.g. `[{a:1}]` under form style) so the test verifies the element keeps its accumulated path (`filter[a]=1`) rather than just matching static template text.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| // an array element does not inherit the form flattening: a map nested one | ||
| // level down keeps its accumulated path instead of being keyed by its | ||
| // property names alone | ||
| "var styleForElement = style", |
There was a problem hiding this comment.
P3: These new assertions only verify that the generic parameterAddToHeaderOrQuery slice branch (always emitted into every client.go) contains the styleForElement reset, so they hold regardless of the fixture's parameters. The exploded-object-query-param.yaml fixture has no array-typed query parameter, so the array-element flattening fix (470b7e0) these lines were added to protect is never actually exercised. Add an array-of-objects query param to the fixture (e.g. [{a:1}] under form style) so the test verifies the element keeps its accumulated path (filter[a]=1) rather than just matching static template text.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At modules/openapi-generator/src/test/java/org/openapitools/codegen/go/GoClientCodegenTest.java, line 169:
<comment>These new assertions only verify that the generic `parameterAddToHeaderOrQuery` slice branch (always emitted into every client.go) contains the `styleForElement` reset, so they hold regardless of the fixture's parameters. The `exploded-object-query-param.yaml` fixture has no array-typed query parameter, so the array-element flattening fix (470b7e0) these lines were added to protect is never actually exercised. Add an array-of-objects query param to the fixture (e.g. `[{a:1}]` under form style) so the test verifies the element keeps its accumulated path (`filter[a]=1`) rather than just matching static template text.</comment>
<file context>
@@ -162,7 +162,14 @@ public void testExplodedObjectQueryParameter() throws IOException {
+ // an array element does not inherit the form flattening: a map nested one
+ // level down keeps its accumulated path instead of being keyed by its
+ // property names alone
+ "var styleForElement = style",
+ "} else if style == \"form\" {",
+ "styleForElement = \"\"",
</file context>
There was a problem hiding this comment.
Good catch on the substance — the fixture had no array parameter at all, so those assertions were matching template text that every generated client.go carries regardless. Addressed in b26fefe.
One correction to the suggested remedy, though: an array-of-objects under form + explode: true would not have covered it either. The api template unrolls the slice itself in that case —
for i := 0; i < s.Len(); i++ {
parameterAddToHeaderOrQuery(localVarQueryParams, "arrayFilter", s.Index(i).Interface(), "form", "multi")
}— so each element arrives as a map and enters the map branch; the array branch the reset lives in is never reached. I generated it both ways to check. It is explode: false that hands the whole slice down as one value:
parameterAddToHeaderOrQuery(localVarQueryParams, "arrayFilter", r.arrayFilter, "form", "csv")so that is what the fixture now declares, and that line is what the test asserts — derived from the spec rather than from the template.
On verifying filter[a]=1 specifically: that is behaviour of the generated go, not of the generator, so a codegen test cannot reach it. I did confirm it directly by exercising the helper in a generated client:
value, style form |
with the reset | reset removed |
|---|---|---|
[]map[string]interface{}{{"a": 1}} |
filter[a]=1 |
a=1 |
map[string]interface{}{"a": 1} |
a=1 |
a=1 |
The second row is the flattening this PR is for, unaffected either way. If a permanent runtime test is wanted the natural home is the echo_api go sample, but its tests there are generated stubs that are all t.Skip-ed, so I left that alone.
mvn -pl modules/openapi-generator -Dtest=GoClientCodegenTest test: 28 tests, 0 failures.
There was a problem hiding this comment.
Update: arrayFilter has left the shared fixture again (d5cf44b). Its call-site assertion passed with or without the reset, so it did not pin anything, and it made go's fixture differ from the other five ports. The reset is pinned by the styleForElement = "" assertion; the wire table above still holds.
The fixture declared four object parameters and no array, so the assertions added for the element reset in 470b7e0 only matched template text that every generated client.go carries: they held whatever the fixture contained. Add the array of objects, and deliberately not exploded. That is the only shape that reaches the array branch of parameterAddToHeaderOrQuery: with explode: true the api template unrolls the slice itself and hands each element over as a map, which enters the map branch instead, so an exploded array would have left the branch as uncovered as no array at all. Not exploded, the whole slice arrives as one form style value: parameterAddToHeaderOrQuery(localVarQueryParams, "arrayFilter", r.arrayFilter, "form", "csv") which is asserted, and is derived from the spec rather than from the template. The behaviour itself lives in generated go rather than in the generator, so it is not reachable from a codegen test. Exercising the helper in a generated client directly, with a []map[string]interface{} under style form: filter[a]=1 with the reset, a=1 without it, while a map that is the parameter's own value stays a=1 either way. mvn -pl modules/openapi-generator -Dtest=GoClientCodegenTest test: 28 tests, 0 failures. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01894FEkx3sDA5wTqjeEo33D
|
please update the samples when you've time: https://github.com/OpenAPITools/openapi-generator/actions/runs/33404842867/job/99711423993?pr=24797 |
Running the go toolchain in the issue_20079_go_regex_wrongly_translated sample rewrote its go.mod and go.sum, and the rewrite was swept into the array element commit. bin/generate-samples.sh emits neither, so the samples up to date job reverted both files and failed on the dirty tree. Restore them to what the generator produces. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JHEXQ9zWNyGwVnWPJJnzSo
|
Thanks for the ping. That job was not stale samples — it was two files that should never have been in the PR. The samples were up to date; the only dirty paths after the rerun were: I had run the go toolchain inside that sample while checking the generated client, and the a60dc9e restores both to master's content — verified byte for byte against |
A nil entry of a form style exploded map went on the wire as the literal "null" (nullValue=null), and so did a nil element of a list an entry holds (anyList=x&anyList=null&anyList=2). Both are now left out, after unwrapping interfaces and pointers, matching the kotlin port. A typed nil pointer, such as (*string)(nil), panicked with "reflect.Value.Interface on zero Value" because the pointer branch called Elem().Interface() without a nil check. It is now left out as well; that branch is shared with deepObject, where it turns the same panic into an omitted value. deepObject is otherwise unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
1 existing issue remains and no new issues found across 15 files
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
Re-trigger cubic
The previous commit left nil entries out of a form style exploded map but not out of a deepObject one, so the same map went on the wire two ways: a nil entry was dropped under form and sent as filter[k]=null under deepObject. deepObject was also inconsistent with itself, because the nil check added to the pointer branch already dropped a typed nil pointer there. Move the entry check ahead of the form branch, so it applies whatever the style, and skip a nil element in the array branch as well. The elements that survive keep the index they had, so filter[l][0] and filter[l][2] stay where they are rather than closing the gap; the index names a position in the array the caller passed, and renumbering would quietly move the remaining values. Nothing changes for a non-nil value, and a nil parameter passed at the top level still goes out as key=null. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This reverts cd901bc. deepObject sending [key]=null for a nil entry is behaviour the petstore sample pins in TestQueryDeepObject, and it is the same on master, so it stays out of a change about form style. Form style keeps skipping nil entries, and the nil check in the pointer case stays. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
All reported issues were addressed across 14 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
The assertion on parameterValueIndirect alone also matched the reverted layout that skipped nil for every style. Pinning what follows the skip fails if it is ever moved ahead of the style branch again, which would break deepObject's [key]=null that TestQueryDeepObject expects. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The shipped comments shrink to one line each, parameterValueIndirect returns true once it has unwrapped a non-nil value, the codegen test keeps only the assertions that fail without each fix, and arrayFilter leaves the shared fixture so it is byte-identical across the ports again. The fixture's description now names the three combinations it covers. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
All reported issues were addressed across 15 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
The constant true reported an invalid reflect.Value as usable. No caller passes one today, but the check guards v.Interface() against a panic and costs nothing. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This branch has not been deployed
2 participants
Bug
A query parameter whose schema is an object, with
style/explodeleft at their defaults (form,true), must go on the wire as one parameter per entry, keyed by the property name alone. Thegoclient bracketed it instead.called with
{"category": "books", "createdDate:gte": "2023-01-01"}:category=books&createdDate%3Agte=2023-01-01filter[category]=books&filter[createdDate%3Agte]=2023-01-01Series
One of six per-language PRs for the same bug: #24797 go, #24802 python, #24803 typescript-fetch, #24867 kotlin, #24868 dart, #24869 ruby. No shared
main/code; each adds the same fixture,3_0/exploded-object-query-param.yaml.Fix
go/client.mustache,parameterAddToHeaderOrQuery:form: each entry becomes its own parameter, keyed by the property name. It used to bracket every map whatever style it was handed.{a: {x: 1}, b: {x: 2}}isa[x]=1&b[x]=2, not two collidingx=entries.form, so a map inside an array staysarrayFilter[a]=1.Nil handling
Under
form, a nil entry and a nil item of a list entry are left out rather than sent asnull, as the kotlin, python and dart ports do. A typed nil pointer, which panicked inElem().Interface()on master, is left out under any style.filter){"k": nil, "a": "b"}a=bfilter[a]=b&filter[k]=null(as on master){"k": (*string)(nil), "a": "b"}a=b(master: panic)filter[a]=b(master: panic){"l": ["x", nil, 2]}l=x&l=2filter[l][0]=x&filter[l][1]=null&filter[l][2]=2(as on master)deepObjectkeeps[k]=nullon purpose: the petstore sample'sTestQueryDeepObjectpinsinputOptions[F3]=null.Verified
A client generated from the fixture, against a server that echoes its raw query string:
filter(object, defaults)category=bookstypedFilter(map, defaults)category=booksdeepFilter(style: deepObject)deepFilter[category]=books(as on master)flatFilter(explode: false)category=books(see Known gaps)GoClientCodegenTest#testExplodedObjectQueryParameterfails without the fix. No sample operation has a form-style map query parameter, so no sample test reaches the new branch;TestQueryDeepObjectinsamples/openapi3/client/petstore/go/fake_api_test.gopasses unchanged.Known gaps
The go templates pass
styleto the runtime but notexplode, so a form-style object withexplode: falseis now exploded; it was bracketed before, and the spec wantsflatFilter=category,books. PassingexplodechangesparameterAddToHeaderOrQuery's signature and all 12 call sites ingo/api.mustache; happy to do that here if you'd rather.PR checklist
./bin/generate-samples.sh bin/configs/go*.yaml): 12 sampleclient.gofiles.Generated with Claude Code