feat(python): support per-request headers #596
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughRenames helper function set_heading_if_not_set to set_header_if_not_set across Python client templates, updates all call sites and docstrings, and adds a changelog entry documenting per-request custom headers via options["headers"]. No behavioral changes beyond naming and header dict alignment. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes ✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (2)
config/clients/python/template/src/client/client.py.mustache (1)
65-85: Avoid mutating caller options and simplify header detectionCurrent implementation mutates the caller’s options dict and relies on type checks to decide whether to set a header. This can leak SDK-injected headers across subsequent calls when a user reuses the same options object and makes the logic brittle.
- Create a shallow copy of options and a fresh headers dict to avoid mutating the input.
- Check presence by key, not by value type.
- Always reassign the new headers dict back to the (copied) options.
Apply this diff:
def set_header_if_not_set( options: dict[str, int | str | dict[str, int | str]] | None, name: str, value: str, ) -> dict[str, int | str | dict[str, int | str]]: """ - Set header to the value if it is not set + Set header to the value if it is not set """ - _options: dict[str, int | str | dict[str, int | str]] = ( - options if options is not None else {} - ) - - if type(_options.get("headers")) is not dict: - _options["headers"] = {} - - if type(_options["headers"]) is dict: - if type(_options["headers"].get(name)) not in [int, str]: - _options["headers"][name] = value - - return _options + # copy options to avoid mutating the caller's dict (common when options are reused) + _options: dict[str, int | str | dict[str, int | str]] = dict(options) if options else {} + # copy headers to avoid mutating caller's nested dict + headers = dict(_options.get("headers") or {}) + if name not in headers: + headers[name] = value + _options["headers"] = headers + return _optionsconfig/clients/python/template/src/sync/client/client.py.mustache (1)
65-85: Avoid mutating caller options and simplify header detectionMirror the async client change: copy the options dict and headers sub-dict before injecting default headers, and decide by key presence rather than value type.
Apply this diff:
def set_header_if_not_set( options: dict[str, int | str | dict[str, int | str]] | None, name: str, value: str, ) -> dict[str, int | str | dict[str, int | str]]: """ - Set header to the value if it is not set + Set header to the value if it is not set """ - _options: dict[str, int | str | dict[str, int | str]] = ( - options if options is not None else {} - ) - - if type(_options.get("headers")) is not dict: - _options["headers"] = {} - - if type(_options["headers"]) is dict: - if type(_options["headers"].get(name)) not in [int, str]: - _options["headers"][name] = value - - return _options + _options: dict[str, int | str | dict[str, int | str]] = dict(options) if options else {} + headers = dict(_options.get("headers") or {}) + if name not in headers: + headers[name] = value + _options["headers"] = headers + return _options
🧹 Nitpick comments (2)
config/clients/python/CHANGELOG.md.mustache (1)
8-8: Changelog entry: add PR reference for traceabilityRecommend referencing the PR number for this feature to aid tracking and release notes consistency.
Apply this diff:
-- feat: allow per-request custom headers via `options["headers"]` +- feat: allow per-request custom headers via `options["headers"]` (#596)config/clients/python/template/src/client/client.py.mustache (1)
818-819: Fix typo in docstring ("request11")There's an extra "11" suffix in the consistency line.
Apply this diff:
- :param consistency(options) - The type of consistency preferred for the request11 + :param consistency(options) - The type of consistency preferred for the request
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
config/clients/python/CHANGELOG.md.mustache(1 hunks)config/clients/python/template/src/client/client.py.mustache(24 hunks)config/clients/python/template/src/sync/client/client.py.mustache(24 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-and-test-java-sdk
- GitHub Check: build-and-test-go-sdk
🔇 Additional comments (1)
config/clients/python/template/src/client/client.py.mustache (1)
65-69: Rename verified — no lingeringset_heading_if_not_setI searched the repository for the old helper name and found no matches. Only the new helper
set_header_if_not_setis present in the template files:
- config/clients/python/template/src/client/client.py.mustache (definition + multiple usages)
- config/clients/python/template/src/sync/client/client.py.mustache (definition + multiple usages)
No further action required.
| Return kwargs with continuation_token, page_size, headers and retry parameters | ||
| """ |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Docstrings: standardize option names to snake_case
Docstrings use camelCase retryParams, maxRetry, minWaitInMs, while code expects retry_params (and downstream likely max_retry, min_wait_in_ms). For Python, prefer snake_case in docs to match actual option keys and PEP 8.
To locate all occurrences to update across both async and sync templates:
Also applies to: 224-228, 239-243, 254-258, 269-272, 286-290, 301-305, 316-320, 332-336, 353-357, 377-382, 479-483, 505-509, 518-522, 540-544, 601-605, 667-671, 749-754, 780-785, 814-819, 848-853, 878-883, 910-914, 926-930
🏁 Script executed:
#!/bin/bash
rg -n -C2 -g 'config/clients/python/template/src/**/client.py.mustache' -P 'retryParams|minWaitInMs|maxRetry'Length of output: 46361
🏁 Script executed:
#!/bin/bash
# Find implementation(s) and usages of options_to_kwargs
rg -n --hidden --glob '!**/node_modules/**' -S "options_to_kwargs" || true
# If found, show surrounding context for the implementation (if any file path returned)
FILE=$(rg -n --hidden --glob '!**/node_modules/**' -S "def options_to_kwargs|options_to_kwargs =" -g '**' -l || true)
if [ -n "$FILE" ]; then
echo "== Showing matches in: $FILE =="
sed -n '1,240p' "$FILE"
else
echo "No explicit 'def options_to_kwargs' found; showing usages instead."
rg -n --hidden --glob '!**/node_modules/**' -S "options_to_kwargs" -C3 || true
fiLength of output: 5280
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILES=(
"config/clients/python/template/src/client/client.py.mustache"
"config/clients/python/template/src/sync/client/client.py.mustache"
)
for f in "${FILES[@]}"; do
if [ -f "$f" ]; then
echo "== $f : lines 1-200 =="
sed -n '1,200p' "$f"
echo
echo "== $f : lines 200-400 =="
sed -n '200,400p' "$f"
echo
else
echo "MISSING: $f"
fi
done
echo "== grep for retryParams / maxRetry / minWaitInMs in those files =="
rg -n --hidden -S 'retryParams|maxRetry|minWaitInMs|retry_params|max_retry|min_wait_in_ms' "${FILES[@]}" || trueLength of output: 61351
Docstrings: use snake_case for retry options in Python templates
options_to_kwargs expects snake_case keys (e.g. "retry_params"), but docstrings use camelCase (retryParams, maxRetry, minWaitInMs). Update the docstrings so they match the code/PEP8.
Files to update (multiple occurrences in each):
- config/clients/python/template/src/client/client.py.mustache
- config/clients/python/template/src/sync/client/client.py.mustache
Suggested replacements (apply to every docstring occurrence of these params):
- :param retryParams(options) - Override the retry parameters for this request
- :param retry_params(options) - Override the retry parameters for this request
- :param retryParams.maxRetry(options) - Override the max number of retries on each API request
- :param retry_params.max_retry(options) - Override the max number of retries on each API request
- :param retryParams.minWaitInMs(options) - Override the minimum wait before a retry is initiated
- :param retry_params.min_wait_in_ms(options) - Override the minimum wait before a retry is initiated
You can find all occurrences with:
rg -n -C2 -g 'config/clients/python/template/src/**/client.py.mustache' -P 'retryParams|minWaitInMs|maxRetry'
| options = set_header_if_not_set(options, CLIENT_METHOD_HEADER, "ReadLatestAuthorizationModel") | ||
| options["page_size"] = 1 |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Prevent stale X-OpenFGA-Client-Method header when reusing options dicts
Because options can be reused by callers, mutating it in-place can leave a previously injected method header in the user's dict, causing the next call to send the wrong method header. The refactor proposed for set_header_if_not_set (copy options and headers) avoids this cross-call contamination while still respecting user-provided headers.
Run this script to check if any other places mutate options directly (beyond temporary local copies), which could still leak state across calls:
Also applies to: 484-485, 490-491, 510-511, 524-525, 606-608, 672-674, 854-856
🏁 Script executed:
#!/bin/bash
# Look for direct mutations to options (assignments to options[...] = or options.pop)
rg -n -C2 --type=py.mustache -P '(?<!def\s)\soptions\[[' -g 'config/clients/python/template/src/**/client.py.mustache'
rg -n -C2 --type=py.mustache -P '\boptions\.pop\(' -g 'config/clients/python/template/src/**/client.py.mustache'Length of output: 358
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Searching for client.py.mustache files..."
rg --files -g 'config/clients/python/template/src/**/client.py.mustache' || true
echo
echo "Searching for assignments into options[...]"
rg -n -C2 -g 'config/clients/python/template/src/**/client.py.mustache' -P 'options\[[^\]]+\]\s*=' || true
echo
echo "Searching for options.pop(...)"
rg -n -C2 -g 'config/clients/python/template/src/**/client.py.mustache' '\boptions\.pop\(' || true
echo
echo "Searching for options.update(...)"
rg -n -C2 -g 'config/clients/python/template/src/**/client.py.mustache' '\boptions\.update\(' || true
echo
echo "Searching for del options[...]"
rg -n -C2 -g 'config/clients/python/template/src/**/client.py.mustache' '\bdel\s+options\[' || true
echo
echo "Searching for options.clear(...)"
rg -n -C2 -g 'config/clients/python/template/src/**/client.py.mustache' '\boptions\.clear\(' || true
echo
echo "Searching for options.setdefault(...)"
rg -n -C2 -g 'config/clients/python/template/src/**/client.py.mustache' '\boptions\.setdefault\(' || trueLength of output: 5503
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILES=(
config/clients/python/template/src/client/client.py.mustache
config/clients/python/template/src/sync/client/client.py.mustache
)
for f in "${FILES[@]}"; do
if [[ -f "$f" ]]; then
echo "==== $f ===="
echo "---- lines 1-140 ----"
sed -n '1,140p' "$f"
echo
echo "---- lines 320-420 ----"
sed -n '320,420p' "$f"
echo
echo "---- lines 360-400 ----"
sed -n '360,400p' "$f"
echo
else
echo "File not found: $f"
fi
done
echo "---- Search for set_header_if_not_set and options_to_kwargs ----"
rg -n 'set_header_if_not_set|options_to_kwargs' || true
echo
echo "---- Show any helper function definitions that mention 'headers' and '_options' ----"
rg -n 'def .*options|headers' || trueLength of output: 133444
Action required: stop mutating callers' options dict / headers in Python client templates
set_header_if_not_set currently mutates the caller's dict (it aliases options and headers). I inspected the template files and confirmed multiple call sites that either call set_header_if_not_set then mutate options (e.g. options["page_size"] = 1) or mutate options in-place via pop — this can leak stale X-OpenFGA-Client-Method headers across calls.
Locations to fix
- config/clients/python/template/src/client/client.py.mustache
- def set_header_if_not_set — around line 65
- read_latest_authorization_model — lines 337–338 (options = set_header_if_not_set(...); options["page_size"] = 1)
- read — ~lines 371–392 (options.pop("page_size"), options.pop("continuation_token"))
- Other set_header_if_not_set call sites: ~484–485, 490–491, 510–511, 523–525, 606–608, 672–674, 854–856
- config/clients/python/template/src/sync/client/client.py.mustache
- def set_header_if_not_set — around line 65
- read_latest_authorization_model — lines 328–329 (same pattern)
- read — ~lines 360–392 (options.pop(...) usages)
- Other set_header_if_not_set call sites found throughout the file (writes, batch check, list relations, etc.)
Recommended minimal change
- Make set_header_if_not_set return a shallow copy of the options and headers (do not mutate the incoming dict). Example replacement:
def set_header_if_not_set(options: dict | None, name: str, value: str) -> dict:
_options = dict(options) if options is not None else {}
existing_headers = _options.get("headers")
headers = dict(existing_headers) if isinstance(existing_headers, dict) else {}
if not isinstance(headers.get(name), (int, str)):
headers[name] = value
_options["headers"] = headers
return _options
- Update call sites that mutate options in-place (options.pop(...) or direct assignments) to operate on a local copy (e.g., _opts = dict(options) or use the dict returned from set_header_if_not_set) before popping/setting keys so the caller's dict is not modified.
Rationale: the template must avoid mutating user-supplied dicts so headers (and other per-request options) cannot leak between calls.
| Return kwargs with continuation_token, page_size, headers and retry parameters | ||
| """ |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Docstrings: standardize option names to snake_case
Align the docstrings with actual option keys: use retry_params, max_retry, min_wait_in_ms.
Find all occurrences to update:
Also applies to: 215-219, 230-234, 245-249, 259-263, 277-281, 292-296, 307-311, 323-327, 345-348, 368-373, 465-468, 491-494, 503-507, 521-525, 576-581, 635-639, 723-728, 754-759, 788-793, 822-827, 851-856, 884-888, 900-904
🏁 Script executed:
#!/bin/bash
rg -n -C2 -g 'config/clients/python/template/src/**/client.py.mustache' -P 'retryParams|minWaitInMs|maxRetry'Length of output: 46361
Docstrings: use snake_case keys — retry_params, max_retry, min_wait_in_ms
Docstrings in the Python client templates reference camelCase option keys (retryParams / maxRetry / minWaitInMs). Update them to snake_case to match the actual option keys.
Files / locations to change (multiple occurrences):
- config/clients/python/template/src/client/client.py.mustache — examples: lines 225-227, 240-242, 255-257, 269-271, 287-289, 302-304, 317-319, 333-335, 354-356, 378-380, 480-482, 506-508, 519-521, 540-542, 602-604, 668-670, 750-752, 781-783, 815-817, 849-851, 879-881, 911-913, 927-929
- config/clients/python/template/src/sync/client/client.py.mustache — examples: lines 216-218, 231-233, 246-248, 260-262, 278-280, 293-295, 308-310, 324-326, 345-347, 369-371, 465-467, 491-493, 504-506, 521-523, 577-579, 636-638, 724-726, 755-757, 789-791, 823-825, 852-854, 885-887, 901-903
Replacement (apply across the occurrences above):
Replace:
: param retryParams(options) - Override the retry parameters for this request
: param retryParams.maxRetry(options) - Override the max number of retries on each API request
: param retryParams.minWaitInMs(options) - Override the minimum wait before a retry is initiated
With:
: param retry_params(options) - Override the retry parameters for this request
: param retry_params.max_retry(options) - Override the max number of retries on each API request
: param retry_params.min_wait_in_ms(options) - Override the minimum wait before a retry is initiated
🤖 Prompt for AI Agents
config/clients/python/template/src/sync/client/client.py.mustache around lines
91-92 (and apply to the other occurrences listed in the review): docstrings
reference camelCase retry option keys (retryParams, maxRetry, minWaitInMs);
update them to snake_case across all listed locations by replacing the three
lines that document retryParams with the snake_case variants: ": param
retry_params(options) - Override the retry parameters for this request", ":
param retry_params.max_retry(options) - Override the max number of retries on
each API request", and ": param retry_params.min_wait_in_ms(options) - Override
the minimum wait before a retry is initiated".
| """ | ||
| options = set_heading_if_not_set(options, CLIENT_METHOD_HEADER, "ReadLatestAuthorizationModel") | ||
| options = set_header_if_not_set(options, CLIENT_METHOD_HEADER, "ReadLatestAuthorizationModel") | ||
| options["page_size"] = 1 |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Prevent stale X-OpenFGA-Client-Method header when reusing options dicts
Same rationale as the async client: copying options/headers inside set_header_if_not_set prevents SDK-injected headers from persisting in user-supplied options across calls.
Check for direct mutations of options that might still cause leakage:
Also applies to: 469-469, 475-476, 496-496, 508-509, 582-584, 640-642, 829-830
🏁 Script executed:
#!/bin/bash
rg -n -C2 --type=py.mustache -P '(?<!def\s)\soptions\[[' -g 'config/clients/python/template/src/**/client.py.mustache'
rg -n -C2 --type=py.mustache -P '\boptions\.pop\(' -g 'config/clients/python/template/src/**/client.py.mustache'Length of output: 358
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Finding client.py.mustache files..."
rg --files -g 'config/clients/python/template/src/**/client.py.mustache' || true
echo
echo "Searching for occurrences that mutate or access 'options'..."
rg -n -C2 -S -e 'options\[' -g 'config/clients/python/template/src/**/client.py.mustache' || true
rg -n -C2 -S -e 'options\.pop\(' -g 'config/clients/python/template/src/**/client.py.mustache' || true
rg -n -C2 -S -e 'options\.update\(' -g 'config/clients/python/template/src/**/client.py.mustache' || true
rg -n -C2 -S -e '^\s*options\s*=' -g 'config/clients/python/template/src/**/client.py.mustache' || true
rg -n -C2 -S -e 'options\.copy\(' -g 'config/clients/python/template/src/**/client.py.mustache' || true
rg -n -C2 -S -e 'headers' -g 'config/clients/python/template/src/**/client.py.mustache' || true
echo
echo "Printing matches with line numbers for manual inspection..."
for f in $(rg --files -g 'config/clients/python/template/src/**/client.py.mustache'); do
echo
echo "----- $f -----"
rg -n -C3 -S -e 'options\[' -g "$f" || true
rg -n -C3 -S -e 'options\.pop\(' -g "$f" || true
rg -n -C3 -S -e 'options\.update\(' -g "$f" || true
rg -n -C3 -S -e 'options\.copy\(' -g "$f" || true
rg -n -C3 -S -e 'headers' -g "$f" || true
doneLength of output: 142463
Copy options and headers in set_header_if_not_set — avoid mutating caller's dict
set_header_if_not_set currently mutates the passed options dict (and nested headers), so SDK-injected headers (e.g. X-OpenFGA-Client-Method) can persist in user-supplied options across calls. I inspected both sync and async client templates and found direct mutations after set_header_if_not_set and other places that mutate options.
Locations to fix (examples from templates):
- config/clients/python/template/src/sync/client/client.py.mustache
- 328: options = set_header_if_not_set(options, CLIENT_METHOD_HEADER, "ReadLatestAuthorizationModel")
- 329: options["page_size"] = 1
- 376–382: options.pop("page_size"), options.pop("continuation_token"), kwargs = options_to_kwargs(options)
- Other set_header_if_not_set usages: 469, 475–476, 495–497, 508–510, 582–584, 640–642, 828–830
- config/clients/python/template/src/client/client.py.mustache
- 337: options = set_header_if_not_set(options, CLIENT_METHOD_HEADER, "ReadLatestAuthorizationModel")
- 338: options["page_size"] = 1
- 385–392: options.pop("page_size"), options.pop("continuation_token"), kwargs = options_to_kwargs(options)
- Other set_header_if_not_set usages: 484–491, 506–512, 604–608, 670–674, 852–856
Recommended minimal fix:
- Make set_header_if_not_set create shallow copies of the options dict and the headers dict before mutating them. Example:
def set_header_if_not_set(options, name, value):
_options = dict(options) if options is not None else {}
headers = dict(_options.get("headers") or {})
if not isinstance(headers.get(name), (int, str)):
headers[name] = value
_options["headers"] = headers
return _options
- After this change, existing direct mutations (options["page_size"]=1, options.pop(...)) will act on the local copy and will not leak into the caller's dict.
Please update set_header_if_not_set (both sync and async templates) and run a quick grep for remaining direct mutations to confirm no other call-sites rely on mutating the caller's dict.
🤖 Prompt for AI Agents
config/clients/python/template/src/sync/client/client.py.mustache lines 329:
set_header_if_not_set currently mutates the passed-in options dict and its
headers which causes SDK-injected headers to leak into caller-supplied options;
change set_header_if_not_set to create shallow copies of options and headers
(e.g., copy options into a new dict if not None, copy headers into a new dict or
{}), modify the copies to set the header only if not already set, assign the
copied headers back into the copied options, and return the copied options;
after making this change, the existing inline mutations like
options["page_size"] = 1 and options.pop(...) will operate on the returned local
copy and not mutate the caller's dict — repeat the same change for the async
template variant and run a grep across templates for remaining direct mutations
after set_header_if_not_set to ensure no call-sites rely on mutating the
caller's dict.
| :param retryParams.minWaitInMs(options) - Override the minimum wait before a retry is initiated | ||
| """ | ||
| options = set_heading_if_not_set(options, CLIENT_METHOD_HEADER, "Writes") | ||
| options = set_header_if_not_set(options, CLIENT_METHOD_HEADER, "Writes") |
There was a problem hiding this comment.
Bug: wrong client method header value ("Writes" vs "Write")
The telemetry header should be consistent with the async client and previous naming ("Write"). "Writes" is likely a typo and will skew metrics.
Apply this diff:
- options = set_header_if_not_set(options, CLIENT_METHOD_HEADER, "Writes")
+ options = set_header_if_not_set(options, CLIENT_METHOD_HEADER, "Write")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| options = set_header_if_not_set(options, CLIENT_METHOD_HEADER, "Writes") | |
| options = set_header_if_not_set(options, CLIENT_METHOD_HEADER, "Write") |
🤖 Prompt for AI Agents
In config/clients/python/template/src/sync/client/client.py.mustache around line
469, the telemetry client method header is set to "Writes" (plural) which is
incorrect; change the header value to "Write" to match the async client and
existing naming. Update the call so options = set_header_if_not_set(options,
CLIENT_METHOD_HEADER, "Write") ensuring consistency across clients and
preventing skewed telemetry.
2 participants
Summary
options["headers"]headersoption on client methodsTesting
make test-client-python(fails: docker: not found)https://chatgpt.com/codex/tasks/task_e_689e4bfaadcc832291107ea947202073
Summary by CodeRabbit