[python] fix: explode object query parameters - #24802
Conversation
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 python client JSON-encoded the whole object into a single parameter instead. python/api.mustache appended the object whole, and it was JSON encoded by ApiClient.parameters_to_url_query, which sees no style or explode. Exploded maps are now appended entry by entry. parameters_to_url_query's dict branch is left alone: it is shared by every call site and is still the right fallback for a parameter that is not exploded. python/api_client.mustache quoted values and never quoted names. That was harmless while every name came from baseName in the document, but an exploded object takes its names from the object, so the names became runtime data for the first time and a name carrying & or = would break the URL. Names are now quoted too.
|
Split per language as requested, out of #24797. This is the python half; the siblings are:
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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 17 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="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:9539">
P2: When `language` contains a property named `context` (or another collection-format parameter), the serializer treats that exploded property as the unrelated array parameter and corrupts its value. Serialize exploded-object entries separately from `_collection_formats` lookup so arbitrary map keys remain scalar query values.</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:366">
P1: When a query parameter is an object with declared properties rather than a map, this `isMap` gate leaves it on the single-parameter path, so the generator still JSON-encodes the whole object. Apply exploded form serialization to object models as well, or add the corresponding property iteration for their generated representation.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| _query_params.append(('{{baseName}}', {{paramName}})) | ||
| {{/isExplode}} | ||
| {{/isMap}} | ||
| {{^isMap}} |
There was a problem hiding this comment.
P1: When a query parameter is an object with declared properties rather than a map, this isMap gate leaves it on the single-parameter path, so the generator still JSON-encodes the whole object. Apply exploded form serialization to object models as well, or add the corresponding property iteration for their generated representation.
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 366:
<comment>When a query parameter is an object with declared properties rather than a map, this `isMap` gate leaves it on the single-parameter path, so the generator still JSON-encodes the whole object. Apply exploded form serialization to object models as well, or add the corresponding property iteration for their generated representation.</comment>
<file context>
@@ -347,7 +347,25 @@ https://github.com/OpenAPITools/openapi-generator/blob/c84b949df1a9ec04ba75989cb
+ _query_params.append(('{{baseName}}', {{paramName}}))
+ {{/isExplode}}
+ {{/isMap}}
+ {{^isMap}}
_query_params.append(('{{baseName}}', {{paramName}}{{#isEnumRef}}.value{{/isEnumRef}}))
+ {{/isMap}}
</file context>
| _query_params.append(('language', language)) | ||
| # form style explodes an object into one parameter per entry, keyed by the | ||
| # property name alone | ||
| for _key, _value in language.items(): |
There was a problem hiding this comment.
P2: When language contains a property named context (or another collection-format parameter), the serializer treats that exploded property as the unrelated array parameter and corrupts its value. Serialize exploded-object entries separately from _collection_formats lookup so arbitrary map keys remain scalar query values.
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 9539:
<comment>When `language` contains a property named `context` (or another collection-format parameter), the serializer treats that exploded property as the unrelated array parameter and corrupts its value. Serialize exploded-object entries separately from `_collection_formats` lookup so arbitrary map keys remain scalar query values.</comment>
<file context>
@@ -9534,7 +9534,10 @@ def _test_query_parameter_collection_format_serialize(
- _query_params.append(('language', language))
+ # 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))
</file context>
…entry
An exploded object query parameter takes its names from the object, so a
property name can collide with the name of a sibling array parameter that
declares a collection format. parameters_to_url_query looked the name up in
collection_formats without checking that the value was a collection, so the
scalar was iterated character by character.
In the petstore fixture, testQueryParameterCollectionFormat declares
context: multi alongside the exploded map language. Calling it with
language={"context": "en"} produced context=e&context=n, and a csv sibling
turned https://x.test into h,t,t,p,s,%3A,/,/,x,.,t,e,s,t.
The collection format now applies only when the value really is a list or a
tuple. Every genuine array parameter is unaffected — multi, csv, ssv and pipes
all serialize byte for byte as before.
Reported by cubic on OpenAPITools#24802.
|
Thanks cubic — one of these was a real regression and is now fixed in 82c90b6. P2, collection format collision — valid, fixed. This was introduced by this PR and I had
The fix gates the branch on the value actually being a collection: if k in collection_formats and isinstance(v, (list, tuple)):Checked all four collection formats against real array parameters — Worth noting your suggested framing — serialize exploded entries separately from the P1, declared object models — valid, but pre-existing and deliberately out of scope. The |
There was a problem hiding this comment.
3 issues found across 10 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/python/PythonClientCodegenTest.java">
<violation number="1" location="modules/openapi-generator/src/test/java/org/openapitools/codegen/python/PythonClientCodegenTest.java:766">
P3: The collision scenario this comment describes (an exploded object property named `context` alongside a `context: multi` array parameter) is not present in the fixture, so the assertion only checks that the guard line exists in the shared api_client.mustache output. That line is generated for every Python client regardless of the fixture, so the test would pass even if the collision behavior regressed. Add a colliding parameter/property pair to exploded-object-query-param.yaml and assert the generated wire behavior (e.g., that the exploded entry is not split by the sibling array's collection format) to actually exercise the fix.</violation>
</file>
<file name="samples/client/others/python-legacy-model-dictionaries/legacy_model_dict_client/api_client.py">
<violation number="1" location="samples/client/others/python-legacy-model-dictionaries/legacy_model_dict_client/api_client.py:653">
P2: The guard and name-quoting fixes were applied only to the `python` template; the `python-pydantic-v1` template variant still generates `if k in collection_formats:` without the `isinstance(v, (list, tuple))` guard and without quoting parameter names. Pydantic-v1 generated clients therefore still have the collection-format collision bug (and unquoted names) for exploded object query params. The test only exercises the `python` generator, so the pydantic-v1 gap is not covered. Mirror the change in python-pydantic-v1/api_client.mustache and regenerate the pydantic-v1 samples.</violation>
</file>
<file name="modules/openapi-generator/src/main/resources/python/api_client.mustache">
<violation number="1" location="modules/openapi-generator/src/main/resources/python/api_client.mustache:696">
P3: The isinstance check only prevents the collision for scalar values. If an exploded object property value is itself a list/tuple and its name collides with a sibling array parameter, the collection format is still applied, so the property is still joined or repeated as if it were the sibling's list. For example, `filter: {context: ['en', 'fr']}` alongside a `context` array parameter with `multi` format still emits `context=en&context=fr` from the object entry, which is exactly the behavior the comment says must not happen. Distinguishing declared array parameter names from runtime object property names (e.g., only applying collection formats to keys that are declared query parameters) would close the gap; the value-type check alone cannot.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| # collection. An exploded object query parameter takes its names from the | ||
| # object, so a property name that happens to match a sibling array parameter | ||
| # must not be joined or repeated as if it were that parameter's list. | ||
| if k in collection_formats and isinstance(v, (list, tuple)): |
There was a problem hiding this comment.
P2: The guard and name-quoting fixes were applied only to the python template; the python-pydantic-v1 template variant still generates if k in collection_formats: without the isinstance(v, (list, tuple)) guard and without quoting parameter names. Pydantic-v1 generated clients therefore still have the collection-format collision bug (and unquoted names) for exploded object query params. The test only exercises the python generator, so the pydantic-v1 gap is not covered. Mirror the change in python-pydantic-v1/api_client.mustache and regenerate the pydantic-v1 samples.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At samples/client/others/python-legacy-model-dictionaries/legacy_model_dict_client/api_client.py, line 653:
<comment>The guard and name-quoting fixes were applied only to the `python` template; the `python-pydantic-v1` template variant still generates `if k in collection_formats:` without the `isinstance(v, (list, tuple))` guard and without quoting parameter names. Pydantic-v1 generated clients therefore still have the collection-format collision bug (and unquoted names) for exploded object query params. The test only exercises the `python` generator, so the pydantic-v1 gap is not covered. Mirror the change in python-pydantic-v1/api_client.mustache and regenerate the pydantic-v1 samples.</comment>
<file context>
@@ -646,7 +646,11 @@ def parameters_to_url_query(self, params, collection_formats):
+ # collection. An exploded object query parameter takes its names from the
+ # object, so a property name that happens to match a sibling array parameter
+ # must not be joined or repeated as if it were that parameter's list.
+ if k in collection_formats and isinstance(v, (list, tuple)):
collection_format = collection_formats[k]
if collection_format == 'multi':
</file context>
There was a problem hiding this comment.
Not needed there — python-pydantic-v1 never got the behavior these fixes protect. This PR
touches only the python templates; the pydantic-v1 api.mustache has no explode branch,
so an object query parameter still goes out on the single-parameter path and no runtime
property name ever reaches its parameters_to_url_query. The unguarded if k in collection_formats: there only ever sees the declared parameter names it always saw, and
the quoting fix protects runtime data that template never produces. Extending the explode
behavior to pydantic-v1 would be its own change with its own samples churn; happy to file
it separately if maintainers want the template variants to converge.
| // to a value that actually is a collection, or "context": "en" alongside a | ||
| // context: multi array parameter would go on the wire as context=e&context=n. | ||
| Path apiClient = Paths.get(output.getAbsolutePath(), "openapi_client", "api_client.py"); | ||
| TestUtils.assertFileContains(apiClient, |
There was a problem hiding this comment.
P3: The collision scenario this comment describes (an exploded object property named context alongside a context: multi array parameter) is not present in the fixture, so the assertion only checks that the guard line exists in the shared api_client.mustache output. That line is generated for every Python client regardless of the fixture, so the test would pass even if the collision behavior regressed. Add a colliding parameter/property pair to exploded-object-query-param.yaml and assert the generated wire behavior (e.g., that the exploded entry is not split by the sibling array's collection format) to actually exercise the fix.
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/python/PythonClientCodegenTest.java, line 766:
<comment>The collision scenario this comment describes (an exploded object property named `context` alongside a `context: multi` array parameter) is not present in the fixture, so the assertion only checks that the guard line exists in the shared api_client.mustache output. That line is generated for every Python client regardless of the fixture, so the test would pass even if the collision behavior regressed. Add a colliding parameter/property pair to exploded-object-query-param.yaml and assert the generated wire behavior (e.g., that the exploded entry is not split by the sibling array's collection format) to actually exercise the fix.</comment>
<file context>
@@ -757,6 +757,14 @@ public void testExplodedObjectQueryParameter() throws IOException {
+ // to a value that actually is a collection, or "context": "en" alongside a
+ // context: multi array parameter would go on the wire as context=e&context=n.
+ Path apiClient = Paths.get(output.getAbsolutePath(), "openapi_client", "api_client.py");
+ TestUtils.assertFileContains(apiClient,
+ "if k in collection_formats and isinstance(v, (list, tuple)):");
}
</file context>
There was a problem hiding this comment.
Fair — the fixture asserted the guard's text, not its behavior. Added two runtime tests in
c3929d4, in the hand-maintained petstore sample tests (tests/test_api_client.py, which CI
executes): a scalar entry named like a declared multi array parameter goes out as itself
(language=nl&context=abc), and the declared array parameter itself still gets its
collection format (context=a&context=b). Both run against the generated
parameters_to_url_query, so the collision path is now exercised, not just present.
| # collection. An exploded object query parameter takes its names from the | ||
| # object, so a property name that happens to match a sibling array parameter | ||
| # must not be joined or repeated as if it were that parameter's list. | ||
| if k in collection_formats and isinstance(v, (list, tuple)): |
There was a problem hiding this comment.
P3: The isinstance check only prevents the collision for scalar values. If an exploded object property value is itself a list/tuple and its name collides with a sibling array parameter, the collection format is still applied, so the property is still joined or repeated as if it were the sibling's list. For example, filter: {context: ['en', 'fr']} alongside a context array parameter with multi format still emits context=en&context=fr from the object entry, which is exactly the behavior the comment says must not happen. Distinguishing declared array parameter names from runtime object property names (e.g., only applying collection formats to keys that are declared query parameters) would close the gap; the value-type check alone cannot.
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_client.mustache, line 696:
<comment>The isinstance check only prevents the collision for scalar values. If an exploded object property value is itself a list/tuple and its name collides with a sibling array parameter, the collection format is still applied, so the property is still joined or repeated as if it were the sibling's list. For example, `filter: {context: ['en', 'fr']}` alongside a `context` array parameter with `multi` format still emits `context=en&context=fr` from the object entry, which is exactly the behavior the comment says must not happen. Distinguishing declared array parameter names from runtime object property names (e.g., only applying collection formats to keys that are declared query parameters) would close the gap; the value-type check alone cannot.</comment>
<file context>
@@ -689,7 +689,11 @@ https://github.com/OpenAPITools/openapi-generator/blob/c84b949df1a9ec04ba75989cb
+ # collection. An exploded object query parameter takes its names from the
+ # object, so a property name that happens to match a sibling array parameter
+ # must not be joined or repeated as if it were that parameter's list.
+ if k in collection_formats and isinstance(v, (list, tuple)):
collection_format = collection_formats[k]
if collection_format == 'multi':
</file context>
There was a problem hiding this comment.
Accurate as a mechanism, and left as is deliberately. A list-valued property of an
exploded object has no defined serialization in OpenAPI at all (form/explode is specified
for objects with scalar-ish members; nested collections are undefined), so when such a name
also collides with a declared array parameter there is no "right answer" to restore — the
guard covers the case the spec does define, a scalar property shadowing an array parameter.
Distinguishing the two sources for nested lists would mean tagging exploded entries through
the whole _query_params pipeline, which is a larger restructure than this fix warrants.
The new runtime tests in c3929d4 pin the two defined behaviors either side of the guard.
Two runtime tests on parameters_to_url_query in the hand-maintained petstore sample tests: a scalar entry named like a declared array parameter goes out as itself, and the declared array parameter still gets its collection format. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Go3AyndcGwv5tFTwo9aBfy
1 participant
A query parameter whose schema is an object and whose
style/explodeare left at theirdefaults —
style: form,explode: true— must go on the wire as one parameter per entry,keyed by the property name alone. The
pythonclient JSON encoded the whole object into asingle parameter instead, while
csharp,javaandphpgot it right.Given:
called with
{"category": "books", "createdDate:gte": "2023-01-01"}:category=books&createdDate%3Agte=2023-01-01filter=%7B%22category%22:%20%22books%22,%20...%7DThree causes
1.
python/api.mustache— an object parameter was appended whole and json encoded byApiClient.parameters_to_url_query, which sees no style or explode. Exploded maps are nowappended entry by entry.
parameters_to_url_query's dict branch is left alone: it is sharedby every call site and is still the right fallback for a parameter that is not exploded.
2.
python/api_client.mustache—parameters_to_url_queryquoted values and neverquoted names. That was harmless while every name came from
baseNamein the document, butan exploded object takes its names from the object, so with (1) they became runtime data for
the first time and a name carrying
&or=would break the URL. Names are now quoted too,which is also what
goalready did viaurl.Values.Encodeandtypescript-fetchviaencodeURIComponent. Spec-declared names are unaffected — they are almost always alreadyURL-safe.
Verified on the wire, not just asserted
The client was generated from the new fixture and pointed at a server that echoes its own raw
query string back:
filter(object, defaults)category=books&createdDate%3Agte=2023-01-01typedFilter(map, defaults)deepFilter(style: deepObject)flatFilter(explode: false)The first two rows are the fixed behaviour; the last two are byte-for-byte what the generator
did before.
3.
python/api_client.mustache, collection formats —parameters_to_url_querylookedthe parameter name up in
collection_formatswithout checking that the value was acollection. That was safe while every name came from
baseName, but with (1) the names areruntime data, so a property name can collide with a sibling array parameter's name and the
scalar gets iterated character by character. In the petstore fixture,
testQueryParameterCollectionFormatdeclarescontext: multinext to the exploded maplanguage, solanguage={"context": "en"}went on the wire ascontext=e&context=n, and acsv sibling turned
https://x.testintoh,t,t,p,s,%3A,/,/,x,.,t,e,s,t. The collectionformat now applies only when the value really is a
listor atuple. Every genuine arrayparameter is unaffected —
multi,csv,ssvandpipesall serialize byte for byte asbefore. Thanks to cubic for catching this.
Known gaps, called out deliberately
Declared object models. The exploded branch is gated on
isMap, so a$refed objectmodel as a query parameter is
isModeland still goes on the wire whole. This ispre-existing — every object was json encoded before this change — and it is the same gap
typescript-fetchhas in #24803, where iterating the model would also put the interfaceproperty names on the wire rather than the wire names. Left for a follow-up.
deepObject. python emitsdeepFilter={"category": "books"}where the spec asks fordeepFilter[category]=books. That is a pre-existing defect with its own blast radius ratherthan something this change introduces — the
deepFilterrow above is byte-for-byte what thetemplate did before — so it is left for a follow-up to keep this reviewable. Say the word and
I will fold it in.
Tests
modules/openapi-generator/src/test/resources/3_0/exploded-object-query-param.yamlcoversthe four combinations that decide the wire format:
PythonClientCodegenTest#testExplodedObjectQueryParameterIt fails without the fix (verified by stashing only the
main/changes), and also assertsthe collection format guard from (3). 87 tests pass across
PythonClientCodegenTestandPythonPydanticV1ClientCodegenTest.PR checklist
./bin/generate-samples.shforbin/configs/python*.yaml;./bin/utils/export_docs_generators.shproduced no diff).13 sample files changed: 8
api_client.pyand 5fake_api.py. Thefake_api.pyonescome from the petstore fixture's
languageparameter, a declared map with the defaultstyle; the
api_client.pyones are the name quoting in (2) and the collection formatguard in (3). The three
python-pydantic-v1samples are untouched, since thatgenerator has its own
api_client.mustache.Generated with Claude Code