feat: reverse sync python sdk#696
Open
SoulPancake wants to merge 8 commits into
Open
Conversation
Co-authored-by: SoulPancake <70265851+SoulPancake@users.noreply.github.com>
Contributor
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThe PR refactors Python SDK generator templates to centralize HTTP request execution across async and sync API classes, introducing raw HTTP request capabilities (execute_api_request, execute_streamed_api_request), enhancing exception handling with operation context and classification helpers, and updating documentation to demonstrate the new raw endpoint calling patterns. ChangesPython SDK Request Execution Refactoring with Raw HTTP Support
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Comment |
SoulPancake
force-pushed
the
feat/reverse-sync-python-sdk
branch
from
3007e82 to
ebb8c1e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
config/clients/python/template/src/exceptions.py.mustache (2)
254-261: ⚡ Quick winConsider checking
isinstance(RateLimitExceededError)for consistency.Similar to the previous comment, this predicate could benefit from an
isinstancecheck to align with the pattern used inis_validation_errorandis_not_found_error.♻️ Suggested improvement
def is_rate_limit_error(self): """ Check if this is a rate limit (429) error. Returns: True if HTTP status is 429 or error code indicates rate limiting """ - return self.status == 429 or (self.code and "rate_limit" in self.code.lower()) + return isinstance(self, RateLimitExceededError) or self.status == 429 or (self.code and "rate_limit" in self.code.lower())🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@config/clients/python/template/src/exceptions.py.mustache` around lines 254 - 261, Update the is_rate_limit_error predicate to mirror the pattern used in is_validation_error and is_not_found_error by also checking for the specific exception class; change the return to include an isinstance(self, RateLimitExceededError) check in addition to the existing status == 429 and error code check so that RateLimitExceededError instances are recognized consistently.
245-252: ⚡ Quick winConsider checking
isinstance(AuthenticationError)for consistency.The predicate only checks
status == 401, butis_validation_errorandis_not_found_erroralso check their respective exception types usingisinstance. For consistency and to handle cases where anAuthenticationErroris raised without a status code, consider adding anisinstancecheck.♻️ Suggested improvement
def is_authentication_error(self): """ Check if this is an authentication (401) error. Returns: True if HTTP status is 401 """ - return self.status == 401 + return isinstance(self, AuthenticationError) or self.status == 401🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@config/clients/python/template/src/exceptions.py.mustache` around lines 245 - 252, The is_authentication_error method currently only checks status == 401; update it to also return True when the exception instance is of type AuthenticationError (i.e., use isinstance(self, AuthenticationError) || self.status == 401) to match the pattern used by is_validation_error and is_not_found_error (and to handle cases where status is missing); locate the method is_authentication_error and the AuthenticationError class to add the isinstance check and ensure import/definition visibility.config/clients/python/template/src/api.py.mustache (1)
105-118: 💤 Low valueStreaming branch of
async _executereturns an async generator without awaiting — works, but worth a clarifying comment.
execute_streamed_api_requestis anasync defcontainingyield, so it is an async generator function: calling it (withoutawait) immediately returns anAsyncGeneratorobject. Wrapping that return inasync def _executemeans callers seeawait self._execute(...)resolving to an async generator they must thenasync forover. This is correct (and matches{{operationId}}doingreturn await ..._with_http_info(...)), but it's easy to misread — a one-line comment on line 107 noting that the async generator is intentionally returned (not awaited) would save future maintainers a double-take.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@config/clients/python/template/src/api.py.mustache` around lines 105 - 118, The streaming branch in async _execute intentionally returns the AsyncGenerator produced by execute_streamed_api_request rather than awaiting it; add a concise inline comment above the return in the _execute function (near the execute_streamed_api_request call) stating that execute_streamed_api_request is an async generator and that returning it (not awaiting) is deliberate so callers can await _execute and then async for over the returned generator.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@config/clients/python/template/src/api.py.mustache`:
- Around line 245-273: The async RawResponse path in execute_api_request is racy
because it reads ApiClient.last_response after an await; instead capture the
call_api return value directly (like the typed-response path) instead of relying
on last_response and remove the getattr(last_response) usage; call
self.api_client.call_api(...) with the same full argument set used by the typed
path (include path_params, post_params, files, async_req, _request_timeout,
collection_formats, plus the existing
query_params_list/header_params/body/response_types_map/auth_settings/_preload_content/_retry_params/_oauth2_client/_telemetry_attributes/_streaming),
await its result into a variable (e.g. rest_response), then build and return
RawResponse(status=rest_response.status,
headers=dict(rest_response.getheaders()),
body=ResponseParser.parse_body(rest_response.data)).
In `@config/clients/python/template/src/sync/api.py.mustache`:
- Around line 96-105: Telemetry attribute
TelemetryAttributes.fga_client_request_method is populated with inconsistent
casing between _execute (uses operation_name as-is) and execute_api_request
(uses operation_name.lower()), causing fragmented telemetry; normalize to a
single canonical casing (e.g., always lower()) by converting operation_name to
the chosen form in both _execute and the execute_api_request defaulting branch
before assigning to telemetry_attributes and before calling
TelemetryAttributes.fromBody; update references in _execute and
execute_api_request so TelemetryAttributes.fga_client_request_method always
receives the same cased value.
- Around line 246-274: The public/RawResponse branch currently discards the
return value of self.api_client.call_api and reads self.api_client.last_response
(fragile) and also omits several arguments present in the typed branch; instead,
capture call_api's return (e.g., response = self.api_client.call_api(...)) and
build RawResponse from that return value rather than getattr(self.api_client,
"last_response"), and make the call_api invocation mirror the typed branch by
including path_params={}, post_params=[], files={}, async_req=...,
_request_timeout=_request_timeout, and collection_formats={} (and any other
kwargs present in lines 224–244) so call_api signatures are consistent and
thread-safe.
---
Nitpick comments:
In `@config/clients/python/template/src/api.py.mustache`:
- Around line 105-118: The streaming branch in async _execute intentionally
returns the AsyncGenerator produced by execute_streamed_api_request rather than
awaiting it; add a concise inline comment above the return in the _execute
function (near the execute_streamed_api_request call) stating that
execute_streamed_api_request is an async generator and that returning it (not
awaiting) is deliberate so callers can await _execute and then async for over
the returned generator.
In `@config/clients/python/template/src/exceptions.py.mustache`:
- Around line 254-261: Update the is_rate_limit_error predicate to mirror the
pattern used in is_validation_error and is_not_found_error by also checking for
the specific exception class; change the return to include an isinstance(self,
RateLimitExceededError) check in addition to the existing status == 429 and
error code check so that RateLimitExceededError instances are recognized
consistently.
- Around line 245-252: The is_authentication_error method currently only checks
status == 401; update it to also return True when the exception instance is of
type AuthenticationError (i.e., use isinstance(self, AuthenticationError) ||
self.status == 401) to match the pattern used by is_validation_error and
is_not_found_error (and to handle cases where status is missing); locate the
method is_authentication_error and the AuthenticationError class to add the
isinstance check and ensure import/definition visibility.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 44597864-173f-45f0-88ef-51d2e269ffcb
📒 Files selected for processing (13)
Makefileconfig/clients/python/.openapi-generator-ignoreconfig/clients/python/config.overrides.jsonconfig/clients/python/template/README_calling_api.mustacheconfig/clients/python/template/README_custom_badges.mustacheconfig/clients/python/template/README_initializing.mustacheconfig/clients/python/template/pyproject.toml.mustacheconfig/clients/python/template/src/__init__.py.mustacheconfig/clients/python/template/src/api.py.mustacheconfig/clients/python/template/src/constants.mustacheconfig/clients/python/template/src/exceptions.py.mustacheconfig/clients/python/template/src/sync/api.py.mustacheconfig/common/files/README.mustache
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the SDK generator to align Python SDK generation with downstream changes (referenced python-sdk PR #289), including documentation and template updates intended to support calling non-wrapped endpoints and improving generated client behavior.
Changes:
- Updates Python API templates to add
execute_api_request/execute_streamed_api_requestand refactors endpoint execution through a shared_execute. - Refreshes Python templates/docs (README sections, exception formatting, pytest config) and bumps the Python SDK version to
0.10.3. - Adjusts the Python build flow by removing the
open_fga_api.pypatch application step.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| Makefile | Removes application of the open_fga_api.py patch during Python client build. |
| config/common/files/README.mustache | Adds conditional TOC entry for “Calling Other Endpoints”. |
| config/clients/python/template/src/sync/api.py.mustache | Adds shared execution path + raw/stream execution helpers to sync API template. |
| config/clients/python/template/src/api.py.mustache | Adds shared execution path + raw/stream execution helpers to async API template. |
| config/clients/python/template/src/exceptions.py.mustache | Updates exception formatting and adds operation context helpers. |
| config/clients/python/template/src/constants.mustache | Annotates USER_AGENT for versioning automation. |
| config/clients/python/template/src/init.py.mustache | Reworks exports/version source; adds RawResponse export. |
| config/clients/python/template/README_initializing.mustache | Updates initialization examples and expands OAuth2 credential guidance. |
| config/clients/python/template/README_custom_badges.mustache | Adds release-please marker comment to the Socket badge line. |
| config/clients/python/template/README_calling_api.mustache | Adds documentation for calling other endpoints + streaming examples. |
| config/clients/python/template/pyproject.toml.mustache | Enables --strict-markers and declares integration marker. |
| config/clients/python/config.overrides.json | Bumps Python packageVersion and enables supportsCallingOtherEndpoints. |
| config/clients/python/.openapi-generator-ignore | Adds openfga_sdk/constants.py to ignored outputs. |
Comment on lines
+201
to
+208
| retry_params = ( | ||
| options.get("retry_params") or options.get("_retry_params") | ||
| if options | ||
| else None | ||
| ) | ||
| request_timeout = options.get("_request_timeout") if options else None | ||
| async_req = options.get("async_req") if options else None | ||
|
|
Comment on lines
+253
to
+255
| auth_settings=[], | ||
| _return_http_data_only=True, | ||
| _preload_content=True, |
Comment on lines
452
to
498
| :param async_req: Whether to execute the request asynchronously. | ||
| :type async_req: bool, optional | ||
| :param _return_http_data_only: response data without head status code | ||
| and headers | ||
| :type _return_http_data_only: bool, optional | ||
| :param _preload_content: if False, the urllib3.HTTPResponse object will | ||
| be returned without reading/decoding response | ||
| data. Default is True. | ||
| :type _preload_content: bool, optional | ||
| :param _request_timeout: timeout setting for this request. If one | ||
| number provided, it will be total request | ||
| timeout. It can also be a pair (tuple) of | ||
| (connection, read) timeouts. | ||
| :param _request_auth: set to override the auth_settings for an a single | ||
| request; this effectively ignores the authentication | ||
| in the spec for a single request. | ||
| :param _retry_param: if specified, override the retry parameters specified in configuration | ||
| :type _request_auth: dict, optional | ||
| :type _content_type: string, optional: force content-type for the request | ||
| :return: Returns the result object. | ||
| If the method is called asynchronously, | ||
| returns the request thread. | ||
| :rtype: {{#returnType}}tuple({{.}}, status_code(int), headers(HTTPHeaderDict)){{/returnType}}{{^returnType}}None{{/returnType}} | ||
| """ | ||
|
|
||
| {{#servers.0}} | ||
| local_var_hosts = [ | ||
| {{#servers}} | ||
| '{{{url}}}'{{^-last}},{{/-last}} | ||
| {{/servers}} | ||
| ] | ||
| local_var_host = local_var_hosts[0] | ||
| if kwargs.get('_host_index'): | ||
| _host_index = int(kwargs.get('_host_index')) | ||
| if _host_index < 0 or _host_index >= len(local_var_hosts): | ||
| raise ApiValueError( | ||
| "Invalid host index. Must be 0 <= index < %s" | ||
| % len(local_var_host) | ||
| ) | ||
| local_var_host = local_var_hosts[_host_index] | ||
| {{/servers.0}} | ||
| local_var_params = locals() | ||
|
|
||
| all_params = [ | ||
| {{#requiredParams}}{{^-first}} | ||
| '{{paramName}}'{{^-last}},{{/-last}} | ||
| {{/-first}}{{/requiredParams}} | ||
| {{#requiredParams}} | ||
| {{#isPathParam}} | ||
| {{^-first}} | ||
| "{{paramName}}", | ||
| {{/-first}} | ||
| {{/isPathParam}} | ||
| {{^isPathParam}} | ||
| "{{paramName}}", | ||
| {{/isPathParam}} | ||
| {{/requiredParams}} | ||
| {{#optionalParams}} | ||
| '{{paramName}}'{{^-last}},{{/-last}} | ||
| "{{paramName}}", | ||
| {{/optionalParams}} | ||
| ] | ||
| all_params.extend( | ||
| [ | ||
| 'async_req', | ||
| '_return_http_data_only', | ||
| '_preload_content', | ||
| '_request_timeout', | ||
| '_request_auth', | ||
| '_content_type', | ||
| '_headers', | ||
| '_retry_params', | ||
| '_streaming', | ||
| ] | ||
| ) | ||
| all_params.extend(self._COMMON_PARAMS) | ||
|
|
||
| for key, val in local_var_params['kwargs'].items(): | ||
| if key not in all_params{{#servers.0}} and key != "_host_index"{{/servers.0}}: | ||
| for key, val in local_var_params["kwargs"].items(): | ||
| if key not in all_params: | ||
| raise FgaValidationException( |
Comment on lines
+176
to
+181
| from {{packageName}}.client.execute_api_request_builder import ( | ||
| ExecuteApiRequestBuilder, | ||
| ResponseParser, | ||
| ) | ||
| from {{packageName}}.client.models.raw_response import RawResponse | ||
|
|
Comment on lines
+202
to
+208
| retry_params = ( | ||
| options.get("retry_params") or options.get("_retry_params") | ||
| if options | ||
| else None | ||
| ) | ||
| request_timeout = options.get("_request_timeout") if options else None | ||
| async_req = options.get("async_req") if options else None |
Comment on lines
452
to
498
| :param async_req: Whether to execute the request asynchronously. | ||
| :type async_req: bool, optional | ||
| :param _return_http_data_only: response data without head status code | ||
| and headers | ||
| :type _return_http_data_only: bool, optional | ||
| :param _preload_content: if False, the urllib3.HTTPResponse object will | ||
| be returned without reading/decoding response | ||
| data. Default is True. | ||
| :type _preload_content: bool, optional | ||
| :param _request_timeout: timeout setting for this request. If one | ||
| number provided, it will be total request | ||
| timeout. It can also be a pair (tuple) of | ||
| (connection, read) timeouts. | ||
| :param _request_auth: set to override the auth_settings for an a single | ||
| request; this effectively ignores the authentication | ||
| in the spec for a single request. | ||
| :param _retry_param: if specified, override the retry parameters specified in configuration | ||
| :type _request_auth: dict, optional | ||
| :type _content_type: string, optional: force content-type for the request | ||
| :return: Returns the result object. | ||
| If the method is called asynchronously, | ||
| returns the request thread. | ||
| :rtype: {{#returnType}}tuple({{.}}, status_code(int), headers(HTTPHeaderDict)){{/returnType}}{{^returnType}}None{{/returnType}} | ||
| """ | ||
|
|
||
| {{#servers.0}} | ||
| local_var_hosts = [ | ||
| {{#servers}} | ||
| '{{{url}}}'{{^-last}},{{/-last}} | ||
| {{/servers}} | ||
| ] | ||
| local_var_host = local_var_hosts[0] | ||
| if kwargs.get('_host_index'): | ||
| _host_index = int(kwargs.get('_host_index')) | ||
| if _host_index < 0 or _host_index >= len(local_var_hosts): | ||
| raise ApiValueError( | ||
| "Invalid host index. Must be 0 <= index < %s" | ||
| % len(local_var_host) | ||
| ) | ||
| local_var_host = local_var_hosts[_host_index] | ||
| {{/servers.0}} | ||
| local_var_params = locals() | ||
|
|
||
| all_params = [ | ||
| {{#requiredParams}}{{^-first}} | ||
| '{{paramName}}'{{^-last}},{{/-last}} | ||
| {{/-first}}{{/requiredParams}} | ||
| {{#requiredParams}} | ||
| {{#isPathParam}} | ||
| {{^-first}} | ||
| "{{paramName}}", | ||
| {{/-first}} | ||
| {{/isPathParam}} | ||
| {{^isPathParam}} | ||
| "{{paramName}}", | ||
| {{/isPathParam}} | ||
| {{/requiredParams}} | ||
| {{#optionalParams}} | ||
| '{{paramName}}'{{^-last}},{{/-last}} | ||
| "{{paramName}}", | ||
| {{/optionalParams}} | ||
| ] | ||
| all_params.extend( | ||
| [ | ||
| "async_req", | ||
| "_return_http_data_only", | ||
| "_preload_content", | ||
| "_request_timeout", | ||
| "_request_auth", | ||
| "_content_type", | ||
| "_headers", | ||
| "_retry_params", | ||
| "_streaming", | ||
| ] | ||
| ) | ||
| all_params.extend(self._COMMON_PARAMS) | ||
|
|
||
| for key, val in local_var_params['kwargs'].items(): | ||
| if key not in all_params{{#servers.0}} and key != "_host_index"{{/servers.0}}: | ||
| for key, val in local_var_params["kwargs"].items(): | ||
| if key not in all_params: | ||
| raise FgaValidationException( |
| @@ -144,8 +144,7 @@ build-client-python: | |||
|
|
|||
| sort -uo ${CLIENTS_OUTPUT_DIR}/fga-python-sdk/.openapi-generator/FILES{,} | |||
|
|
|||
10 tasks
rhamzeh
reviewed
May 14, 2026
| openfga_sdk/sync/open_fga_api.py | ||
| openfga_sdk/api_client.py | ||
| openfga_sdk/configuration.py | ||
| openfga_sdk/constants.py |
Member
Author
There was a problem hiding this comment.
Oh the reason I removed it was because of the sdk version and user agent driven from the generator
Since now we are using release please to handle and bump that, but you're right there's a lot of other common configs in this that are controlled from the generator.
Maybe we move the versions to a different file?
Or sync it back once in a while 🤔
Member
Author
There was a problem hiding this comment.
I have extracted it to a version file that is SDK maintained @rhamzeh
Member
Author
There was a problem hiding this comment.
Seems to fail in CI, investigating, maybe my sequencing is not correct
Member
Author
|
The check should pass after this is merged openfga/python-sdk#289 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Generates openfga/python-sdk#289
What problem is being solved?
How is it being solved?
What changes are made to solve it?
References
Review Checklist
mainSummary by CodeRabbit
Release Notes
New Features
Documentation
Improvements