Repository navigation
fix(python): merge request headers case-insensitively - #25163
danielpassy wants to merge 3 commits into
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 52 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="samples/openapi3/client/petstore/python-httpx2/tests/test_request_headers.py">
<violation number="1" location="samples/openapi3/client/petstore/python-httpx2/tests/test_request_headers.py:181">
P2: `python-httpx2` has no `add_pet_sync`, so this test is skipped on every run and provides no sync-wrapper regression coverage; move it to a sample with sync support enabled.</violation>
</file>
<file name="samples/openapi3/client/petstore/python-httpx/petstore_api/api/fake_classname_tags123_api.py">
<violation number="1" location="samples/openapi3/client/petstore/python-httpx/petstore_api/api/fake_classname_tags123_api.py:270">
P2: A caller-supplied case variant of `Accept` now disables the generated Accept fallback even when its value is `None`. `_merge_headers`/`_set_header` copy entries as-is (no `None` filtering), so `_headers={'accept': None}` yields `_header_params={'accept': None}`, the `any()` guard suppresses `select_header_accept(...)`, and `parameters_to_tuples` forwards `('accept', None)` unchanged to the transport, where the new `httpx.Headers(headers or {})` wrapping in `rest.py` expects string values. This hands callers a new way to produce an invalid header out of a case variant. Filter `None` values during the merge (or in `_set_header`) so absence of a usable value still allows the generated fallback.</violation>
</file>
<file name="samples/openapi3/client/petstore/python-httpx/petstore_api/api_client.py">
<violation number="1" location="samples/openapi3/client/petstore/python-httpx/petstore_api/api_client.py:122">
P3: The cookie-auth normalization loop makes a pre-existing empty `Cookie` value append with a leading `"; "` separator. With `_headers={'cookie': ''}` plus cookie auth, `_set_header` re-inserts `Cookie=''`, the `else` branch then produces `Cookie: ; session=two`, whereas when no `Cookie` case variant exists the `if` branch writes `Cookie: session=two` without the separator. Normalize the separator only when the selected value is non-empty before appending the auth cookie.</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:891">
P3: Copying the raw existing cookie value into a new `Cookie` entry before the `headers['Cookie'] += "; "` append makes a previously tolerated input crash. If a caller passes a non-string cookie through `_headers` or `default_headers` under a lowercase `cookie` key (e.g. an int), the old code started from `headers['Cookie'] = ""` and could never raise on `+=`; the new code now does `int += str` → `TypeError: unsupported operand type(s) for +=`. Header values elsewhere are stringified by `parameters_to_tuples`, so this branch is the lone path that assumes a string value.</violation>
</file>
Reply to a comment to ask cubic a question or push back. It learns from your replies.
Turn on auto-fix | Re-trigger cubic
| self.assertEqual(headers, {'content-type': 'multipart/form-data'}) | ||
|
|
||
| async def test_sync_calls_use_request_local_headers(self): | ||
| if not hasattr(self.api, 'add_pet_sync'): |
There was a problem hiding this comment.
P2: python-httpx2 has no add_pet_sync, so this test is skipped on every run and provides no sync-wrapper regression coverage; move it to a sample with sync support enabled.
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 samples/openapi3/client/petstore/python-httpx2/tests/test_request_headers.py, line 181:
<comment>`python-httpx2` has no `add_pet_sync`, so this test is skipped on every run and provides no sync-wrapper regression coverage; move it to a sample with sync support enabled.</comment>
<file context>
@@ -0,0 +1,204 @@
+ self.assertEqual(headers, {'content-type': 'multipart/form-data'})
+
+ async def test_sync_calls_use_request_local_headers(self):
+ if not hasattr(self.api, 'add_pet_sync'):
+ self.skipTest('Sample does not enable supportHttpxSync')
+ headers = {'content-type': 'application/json', 'authorization': 'caller'}
</file context>
There was a problem hiding this comment.
The same concurrency test runs in both python-httpx-sync and python-httpx2-sync, where sync wrappers are enabled. The skip is expected only in samples without sync support; both sync variants passed.
|
|
||
| # set the HTTP header `Accept` | ||
| if 'Accept' not in _header_params: | ||
| if not any(key.lower() == 'accept' for key in _header_params): |
There was a problem hiding this comment.
P2: A caller-supplied case variant of Accept now disables the generated Accept fallback even when its value is None. _merge_headers/_set_header copy entries as-is (no None filtering), so _headers={'accept': None} yields _header_params={'accept': None}, the any() guard suppresses select_header_accept(...), and parameters_to_tuples forwards ('accept', None) unchanged to the transport, where the new httpx.Headers(headers or {}) wrapping in rest.py expects string values. This hands callers a new way to produce an invalid header out of a case variant. Filter None values during the merge (or in _set_header) so absence of a usable value still allows the generated fallback.
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 samples/openapi3/client/petstore/python-httpx/petstore_api/api/fake_classname_tags123_api.py, line 270:
<comment>A caller-supplied case variant of `Accept` now disables the generated Accept fallback even when its value is `None`. `_merge_headers`/`_set_header` copy entries as-is (no `None` filtering), so `_headers={'accept': None}` yields `_header_params={'accept': None}`, the `any()` guard suppresses `select_header_accept(...)`, and `parameters_to_tuples` forwards `('accept', None)` unchanged to the transport, where the new `httpx.Headers(headers or {})` wrapping in `rest.py` expects string values. This hands callers a new way to produce an invalid header out of a case variant. Filter `None` values during the merge (or in `_set_header`) so absence of a usable value still allows the generated fallback.</comment>
<file context>
@@ -267,7 +267,7 @@ def _test_classname_serialize(
# set the HTTP header `Accept`
- if 'Accept' not in _header_params:
+ if not any(key.lower() == 'accept' for key in _header_params):
_header_params['Accept'] = self.api_client.select_header_accept(
[
</file context>
There was a problem hiding this comment.
HTTPX already rejected None header values before this change, including when the generated Accept field was also present. This PR preserves value validation; filtering None would change that behavior.
| @staticmethod | ||
| def _set_header(headers, name, value): | ||
| """Replace a header case-insensitively, retaining the winning spelling.""" | ||
| for key in list(headers): |
There was a problem hiding this comment.
P3: The cookie-auth normalization loop makes a pre-existing empty Cookie value append with a leading "; " separator. With _headers={'cookie': ''} plus cookie auth, _set_header re-inserts Cookie='', the else branch then produces Cookie: ; session=two, whereas when no Cookie case variant exists the if branch writes Cookie: session=two without the separator. Normalize the separator only when the selected value is non-empty before appending the auth cookie.
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 samples/openapi3/client/petstore/python-httpx/petstore_api/api_client.py, line 122:
<comment>The cookie-auth normalization loop makes a pre-existing empty `Cookie` value append with a leading `"; "` separator. With `_headers={'cookie': ''}` plus cookie auth, `_set_header` re-inserts `Cookie=''`, the `else` branch then produces `Cookie: ; session=two`, whereas when no `Cookie` case variant exists the `if` branch writes `Cookie: session=two` without the separator. Normalize the separator only when the selected value is non-empty before appending the auth cookie.</comment>
<file context>
@@ -116,6 +116,23 @@ def set_default_header(self, header_name, header_value):
+ @staticmethod
+ def _set_header(headers, name, value):
+ """Replace a header case-insensitively, retaining the winning spelling."""
+ for key in list(headers):
+ if key.lower() == name.lower():
+ del headers[key]
</file context>
There was a problem hiding this comment.
Fixed in 392ccfe: the separator is added only when the selected Cookie value is non-empty. Regression tests cover an empty cookie with cookie authentication.
| {{#httpxLibrary}} | ||
| for key in list(headers): | ||
| if key.lower() == 'cookie': | ||
| self._set_header(headers, 'Cookie', headers[key]) |
There was a problem hiding this comment.
P3: Copying the raw existing cookie value into a new Cookie entry before the headers['Cookie'] += "; " append makes a previously tolerated input crash. If a caller passes a non-string cookie through _headers or default_headers under a lowercase cookie key (e.g. an int), the old code started from headers['Cookie'] = "" and could never raise on +=; the new code now does int += str → TypeError: unsupported operand type(s) for +=. Header values elsewhere are stringified by parameters_to_tuples, so this branch is the lone path that assumes a string value.
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/python/api_client.mustache, line 891:
<comment>Copying the raw existing cookie value into a new `Cookie` entry before the `headers['Cookie'] += "; "` append makes a previously tolerated input crash. If a caller passes a non-string cookie through `_headers` or `default_headers` under a lowercase `cookie` key (e.g. an int), the old code started from `headers['Cookie'] = ""` and could never raise on `+=`; the new code now does `int += str` → `TypeError: unsupported operand type(s) for +=`. Header values elsewhere are stringified by `parameters_to_tuples`, so this branch is the lone path that assumes a string value.</comment>
<file context>
@@ -856,6 +885,12 @@ https://github.com/OpenAPITools/openapi-generator/blob/c84b949df1a9ec04ba75989cb
+{{#httpxLibrary}}
+ for key in list(headers):
+ if key.lower() == 'cookie':
+ self._set_header(headers, 'Cookie', headers[key])
+ break
+{{/httpxLibrary}}
</file context>
There was a problem hiding this comment.
HTTPX already rejected numeric Cookie values before this change, even alongside Cookie authentication. This PR preserves value validation. Could you provide a previously successful HTTPX request that now fails?
There was a problem hiding this comment.
No previously successful request is established here: the reply says HTTPX already rejected numeric Cookie values, including alongside Cookie authentication, and that this PR preserves that validation. On the supplied evidence, the parent comment’s claim of a new regression does not apply.
danielpassy
changed the title
There was a problem hiding this comment.
2 issues found across 107 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. 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="samples/openapi3/client/petstore/python/tests/test_request_headers.py">
<violation number="1" location="samples/openapi3/client/petstore/python/tests/test_request_headers.py:23">
P2: The urllib3 tests call `response_data.read()` synchronously, but this always installs an `AsyncMock`, creating an unawaited coroutine and a `RuntimeWarning` on each call. Use a regular `Mock` for the synchronous client.</violation>
</file>
<file name="samples/openapi3/client/petstore/python/petstore_api/api/fake_api.py">
<violation number="1" location="samples/openapi3/client/petstore/python/petstore_api/api/fake_api.py:785">
P3: The case-insensitive Accept check is inlined in every generated endpoint, duplicating the same case-folding logic that this PR already centralizes in `ApiClient._set_header`/`_merge_headers`. It appears roughly a hundred times across the five regenerated samples, so a future fix to the containment logic (e.g., a transport whose keys are not plain `str`) has to be applied to each copy. Add an `ApiClient._has_header(headers, name)` helper next to `_set_header` and emit `if not self.api_client._has_header(_header_params, 'Accept'):` from the template.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Turn on auto-fix | Re-trigger cubic
| api_key={'api_key': 'configured-key'}, | ||
| )) | ||
| response = Mock(status=200, reason='OK', data=b'{}', headers={}) | ||
| response.read = AsyncMock(return_value=b'{}') |
There was a problem hiding this comment.
P2: The urllib3 tests call response_data.read() synchronously, but this always installs an AsyncMock, creating an unawaited coroutine and a RuntimeWarning on each call. Use a regular Mock for the synchronous client.
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 samples/openapi3/client/petstore/python/tests/test_request_headers.py, line 23:
<comment>The urllib3 tests call `response_data.read()` synchronously, but this always installs an `AsyncMock`, creating an unawaited coroutine and a `RuntimeWarning` on each call. Use a regular `Mock` for the synchronous client.</comment>
<file context>
@@ -0,0 +1,233 @@
+ api_key={'api_key': 'configured-key'},
+ ))
+ response = Mock(status=200, reason='OK', data=b'{}', headers={})
+ response.read = AsyncMock(return_value=b'{}')
+
+ def respond(*args, **kwargs):
</file context>
There was a problem hiding this comment.
The urllib3 RESTResponse.read() reads self.response.data; it never calls the mocked response.read(). All six header regression suites passed with -W error::RuntimeWarning: 88 passed, 2 expected skips.
|
|
||
| # set the HTTP header `Accept` | ||
| if 'Accept' not in _header_params: | ||
| if not any(key.lower() == 'accept' for key in _header_params): |
There was a problem hiding this comment.
P3: The case-insensitive Accept check is inlined in every generated endpoint, duplicating the same case-folding logic that this PR already centralizes in ApiClient._set_header/_merge_headers. It appears roughly a hundred times across the five regenerated samples, so a future fix to the containment logic (e.g., a transport whose keys are not plain str) has to be applied to each copy. Add an ApiClient._has_header(headers, name) helper next to _set_header and emit if not self.api_client._has_header(_header_params, 'Accept'): from the template.
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 samples/openapi3/client/petstore/python/petstore_api/api/fake_api.py, line 785:
<comment>The case-insensitive Accept check is inlined in every generated endpoint, duplicating the same case-folding logic that this PR already centralizes in `ApiClient._set_header`/`_merge_headers`. It appears roughly a hundred times across the five regenerated samples, so a future fix to the containment logic (e.g., a transport whose keys are not plain `str`) has to be applied to each copy. Add an `ApiClient._has_header(headers, name)` helper next to `_set_header` and emit `if not self.api_client._has_header(_header_params, 'Accept'):` from the template.</comment>
<file context>
@@ -782,7 +782,7 @@ def _fake_health_get_serialize(
# set the HTTP header `Accept`
- if 'Accept' not in _header_params:
+ if not any(key.lower() == 'accept' for key in _header_params):
_header_params['Accept'] = self.api_client.select_header_accept(
[
</file context>
There was a problem hiding this comment.
This check has one source in api.mustache; the repeated copies are generated output. Header keys are validated as strings. A helper would be optional refactoring, so I kept this fix minimal.
|
@cubic-dev-ai review this PR Please review the latest commit, c66329a, and reassess the remaining findings using the thread replies. The User-Agent getter and Cookie documentation are fixed; regression tests and lint passed. |
@danielpassy I have started the AI code review. It will take a few minutes to complete. |
This branch has not been deployed
1 participant
Generated Python clients can merge
content-typeandContent-Typeas different keys. For example, a JSON operation called with_headers={"content-type": "application/json"}adds a second Content-Type field; HTTPX then exposesapplication/json, application/json. Direct serialization can also mutate the supplied header dict.This change applies one case-insensitive, request-local merging rule to the Python generator's urllib3, asyncio/aiohttp, httpx and httpx2 libraries, including HTTPX sync wrappers. Each transport uses its native case-insensitive header container internally, after SDK validation and serialization. Repeated default-header setter calls now replace case variants consistently.
Precedence is preserved for identically spelled names and extended to case variants: per-call headers → explicit OpenAPI headers/generated Content-Type (
_content_type) → client defaults → client cookie → authentication. Accept remains a fallback. Later entries in a dict win; setters use the latest call. A non-empty_request_authoverrides configured auth;{}retains it.Case-variant conflicts now produce one winning field instead of transmitting both. The public
_headersvalidation and value serialization remain unchanged. Comma-separated values and repeated fields supplied directly to transports are preserved; this is not general duplicate-value removal. Full precedence details are documented in generated READMEs.Validation: 98 Python generator Java tests; full Petstore suites across seven variants; Echo API and legacy-model suites; mypy and regression-test lint. Regressions cover conflicting casing/values, defaults, Authorization/API keys, preserved inputs, async/threaded concurrency, form/multipart routing and legitimate repeated values. The original urllib3/aiohttp transport failures were reproduced before testing the correction. Only samples generated by
generatorName: pythonwere regenerated; no other generators or dependency manifests changed. Global multi-language verification was not run.Closes #25162. Related references: TS/JS #6571 (open), Go #24766 and compatibility follow-up #24791 (merged). The Go follow-up informed preserving existing Python precedence and defining a deterministic winner. These other generators' issues are not closed by this PR.
Contribution guidelines and code of conduct read. Python technical committee: @wing328.