feat(python-sdk): api executor for calling arbitrary endpoints#682
Open
SoulPancake wants to merge 1 commit intomainfrom
Open
feat(python-sdk): api executor for calling arbitrary endpoints#682SoulPancake wants to merge 1 commit intomainfrom
SoulPancake wants to merge 1 commit intomainfrom
Conversation
Contributor
WalkthroughThis PR introduces Changes
Sequence DiagramsequenceDiagram
participant Client
participant API as API Client
participant Builder as ExecuteApiRequestBuilder
participant ApiClient
participant Telemetry as Retry/Telemetry
participant Response as RawResponse
Client->>API: execute_api_request(operation_name, method, path, ...)
API->>API: _execute_api_request_internal(...)
API->>Builder: new ExecuteApiRequestBuilder(...)
Builder->>Builder: build() - construct request
API->>API: assemble headers & query params
API->>Telemetry: apply retry params & telemetry attributes
API->>ApiClient: call_api(method, path, body, query_params, headers, streaming, ...)
ApiClient->>ApiClient: execute REST call
ApiClient-->>API: response
API->>Response: transform to RawResponse(body, headers, status)
Response-->>Client: RawResponse
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip You can customize the tone of the review comments and chat replies.Configure the |
4 tasks
![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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
config/clients/python/template/src/sync/api.py.mustache (1)
43-44:⚠️ Potential issue | 🔴 CriticalIncorrect
__exit__signature.The
__exit__method is missing required parameters for context manager protocol. This will cause runtime errors when usingwithstatement.Fix
- def __exit__(self): + def __exit__(self, exc_type, exc_val, exc_tb): self.close()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/clients/python/template/src/sync/api.py.mustache` around lines 43 - 44, The __exit__ method in the context manager has an incorrect signature; change the method definition for __exit__ in the template (currently "def __exit__(self):") to accept the required context-manager parameters (exc_type, exc_value, traceback), call self.close() inside, and ensure it returns None or False so exceptions are not suppressed; update the __exit__ signature wherever the context-manager class is defined to use def __exit__(self, exc_type, exc_value, traceback): self.close().config/clients/python/template/src/api.py.mustache (1)
55-56:⚠️ Potential issue | 🔴 CriticalIncorrect
__exit__signature (non-async path).Same issue as the sync template -
__exit__is missing required parameters for context manager protocol.Fix
- def __exit__(self): + def __exit__(self, exc_type, exc_val, exc_tb): self.close()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/clients/python/template/src/api.py.mustache` around lines 55 - 56, The __exit__ context-manager method currently has the wrong signature; update def __exit__(self): to the proper protocol def __exit__(self, exc_type, exc_val, exc_tb): and keep calling self.close(); return False (or None) so exceptions are not suppressed. This change should be made for the __exit__ method in the API client class where __exit__ is defined.
🧹 Nitpick comments (2)
config/clients/python/config.overrides.json (2)
5-5: Consider a version bump for the new public API.The addition of
execute_api_requestandexecute_streamed_api_requestintroduces new public API surface. Per coding guidelines, version changes should be reflected inpackageVersion. Consider bumping the version (e.g., to0.10.0for a new feature). As per coding guidelines: "Always update the packageVersion in each language-specific config.overrides.json when making version changes."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/clients/python/config.overrides.json` at line 5, Update the packageVersion value in config/clients/python/config.overrides.json to reflect the new public API surface introduced by execute_api_request and execute_streamed_api_request; specifically bump "packageVersion" from "0.9.9" to "0.10.0" (or the appropriate next semantic version) so the Python package version aligns with the added features.
14-15: Inconsistent value types for feature flags.
supportsStreamedListObjectsuses a string value ("streamed_list_objects") while the newsupportsExecuteApiRequestuses a boolean (true). This inconsistency may cause confusion when maintaining or extending these flags.Consider whether this is intentional (e.g., string provides method name context for template rendering) or if the types should be consistent.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/clients/python/config.overrides.json` around lines 14 - 15, Inconsistent feature-flag types: supportsStreamedListObjects is a string while supportsExecuteApiRequest is a boolean; decide which representation you want and make them consistent across the config and any template/consumer code—either change supportsStreamedListObjects to a boolean (true/false) if it’s just a flag, or change supportsExecuteApiRequest to a string (e.g., "execute_api_request") if the flags are meant to carry method names; update the value in config.overrides.json (keys: supportsStreamedListObjects, supportsExecuteApiRequest) and then search for usages in template rendering or code that expect a string vs boolean and adjust those consumers accordingly so types and semantics match.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config/clients/python/template/README_calling_other_endpoints.mustache`:
- Around line 30-32: The docs reference missing template modules/classes:
implement a RawResponse model and the execute_api_request_builder module
(containing ExecuteApiRequestBuilder and ResponseParser) used by
api.py.mustache; specifically add a RawResponse class (e.g., in
client.models.raw_response.RawResponse) that stores body and provides a json()
method returning the already-parsed body (or alternatively change docs to use
response.body), and add execute_api_request_builder with
ExecuteApiRequestBuilder and ResponseParser.parse_body so api.py.mustache can
instantiate RawResponse(body=ResponseParser.parse_body(rest_response.data))
without errors.
In `@config/clients/python/template/src/api.py.mustache`:
- Around line 262-273: The code reads response via self.api_client.last_response
(used in the block building RawResponse) which has the same
thread-safety/side-effect problem as the sync template; instead capture and use
the direct return value from the API call (the result of api_client.call_api /
the async equivalent) rather than relying on last_response. Modify the call site
that currently discards call_api's return to assign its return (e.g.,
rest_response = await/ self.api_client.call_api(...)) and then build RawResponse
from that object (status, getheaders(), data parsed by
ResponseParser.parse_body) so RawResponse is based on the immediate return value
and not the shared last_response attribute.
In `@config/clients/python/template/src/sync/api.py.mustache`:
- Around line 233-260: The code relies on the side-effect of
ApiClient.last_response instead of the call_api return value; update the call in
the method to capture call_api's return (e.g., response =
self.api_client.call_api(...)) and use that response to construct the
RawResponse (status, headers via response.getheaders(), body via
ResponseParser.parse_body(response.data)); retain a safe fallback to
getattr(self.api_client, "last_response", None) only if the call_api return is
None, and adjust variable names (rest_response/response) consistently so
RawResponse construction uses the captured response rather than always reading
last_response.
In `@config/common/files/README.mustache`:
- Around line 110-113: The conditional block using the mustache tag
supportsExecuteApiRequest and partial README_calling_other_endpoints can merge
headers when the condition is false; fix by ensuring the opening tag and header
are on separate lines and that there is a blank/newline after the closing tag so
the subsequent "### Retries" always starts on its own line, i.e. adjust the
block around {{`#supportsExecuteApiRequest`}}, the included partial
README_calling_other_endpoints, and {{/supportsExecuteApiRequest}} so both
surrounding headers are separated by newlines.
---
Outside diff comments:
In `@config/clients/python/template/src/api.py.mustache`:
- Around line 55-56: The __exit__ context-manager method currently has the wrong
signature; update def __exit__(self): to the proper protocol def __exit__(self,
exc_type, exc_val, exc_tb): and keep calling self.close(); return False (or
None) so exceptions are not suppressed. This change should be made for the
__exit__ method in the API client class where __exit__ is defined.
In `@config/clients/python/template/src/sync/api.py.mustache`:
- Around line 43-44: The __exit__ method in the context manager has an incorrect
signature; change the method definition for __exit__ in the template (currently
"def __exit__(self):") to accept the required context-manager parameters
(exc_type, exc_value, traceback), call self.close() inside, and ensure it
returns None or False so exceptions are not suppressed; update the __exit__
signature wherever the context-manager class is defined to use def
__exit__(self, exc_type, exc_value, traceback): self.close().
---
Nitpick comments:
In `@config/clients/python/config.overrides.json`:
- Line 5: Update the packageVersion value in
config/clients/python/config.overrides.json to reflect the new public API
surface introduced by execute_api_request and execute_streamed_api_request;
specifically bump "packageVersion" from "0.9.9" to "0.10.0" (or the appropriate
next semantic version) so the Python package version aligns with the added
features.
- Around line 14-15: Inconsistent feature-flag types:
supportsStreamedListObjects is a string while supportsExecuteApiRequest is a
boolean; decide which representation you want and make them consistent across
the config and any template/consumer code—either change
supportsStreamedListObjects to a boolean (true/false) if it’s just a flag, or
change supportsExecuteApiRequest to a string (e.g., "execute_api_request") if
the flags are meant to carry method names; update the value in
config.overrides.json (keys: supportsStreamedListObjects,
supportsExecuteApiRequest) and then search for usages in template rendering or
code that expect a string vs boolean and adjust those consumers accordingly so
types and semantics match.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d5dd0de9-9f4e-452d-ba02-c33af6bb7de7
📒 Files selected for processing (6)
config/clients/python/config.overrides.jsonconfig/clients/python/patches/open_fga_api.py.patchconfig/clients/python/template/README_calling_other_endpoints.mustacheconfig/clients/python/template/src/api.py.mustacheconfig/clients/python/template/src/sync/api.py.mustacheconfig/common/files/README.mustache
Comment on lines
+262
to
+273
| rest_response = getattr(self.api_client, "last_response", None) | ||
| if rest_response is None: | ||
| raise RuntimeError( | ||
| f"No response for {method.upper()} {resource_path} " | ||
| f"(operation: {operation_name})" | ||
| ) | ||
|
|
||
| return RawResponse( | ||
| status=rest_response.status, | ||
| headers=dict(rest_response.getheaders()), | ||
| body=ResponseParser.parse_body(rest_response.data), | ||
| ) |
Contributor
There was a problem hiding this comment.
Same last_response side-effect concern as sync implementation.
This mirrors the issue in the sync template - discarding call_api return value and fetching via last_response attribute is fragile and not thread-safe. Consider a consistent fix across both templates.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@config/clients/python/template/src/api.py.mustache` around lines 262 - 273,
The code reads response via self.api_client.last_response (used in the block
building RawResponse) which has the same thread-safety/side-effect problem as
the sync template; instead capture and use the direct return value from the API
call (the result of api_client.call_api / the async equivalent) rather than
relying on last_response. Modify the call site that currently discards
call_api's return to assign its return (e.g., rest_response = await/
self.api_client.call_api(...)) and then build RawResponse from that object
(status, getheaders(), data parsed by ResponseParser.parse_body) so RawResponse
is based on the immediate return value and not the shared last_response
attribute.
Comment on lines
+233
to
+260
| self.api_client.call_api( | ||
| resource_path=resource_path, | ||
| method=method.upper(), | ||
| query_params=query_params_list if query_params_list else None, | ||
| header_params=final_headers, | ||
| body=body, | ||
| response_types_map={}, | ||
| auth_settings=[], | ||
| _return_http_data_only=True, | ||
| _preload_content=True, | ||
| _retry_params=retry_params, | ||
| _oauth2_client=self._oauth2_client, | ||
| _telemetry_attributes=telemetry_attributes, | ||
| _streaming=streaming, | ||
| ) | ||
|
|
||
| rest_response = getattr(self.api_client, "last_response", None) | ||
| if rest_response is None: | ||
| raise RuntimeError( | ||
| f"No response for {method.upper()} {resource_path} " | ||
| f"(operation: {operation_name})" | ||
| ) | ||
|
|
||
| return RawResponse( | ||
| status=rest_response.status, | ||
| headers=dict(rest_response.getheaders()), | ||
| body=ResponseParser.parse_body(rest_response.data), | ||
| ) |
Contributor
There was a problem hiding this comment.
Relying on last_response side-effect is fragile.
The implementation discards the return value of call_api (line 233) and instead retrieves the response via getattr(self.api_client, "last_response", None) (line 249). This pattern:
- Creates a hidden dependency on
ApiClientsettinglast_responseas a side-effect - Is not thread-safe if
api_clientis shared across threads - May break if
call_apiimplementation changes
Consider capturing and using the return value from call_api directly, or document this coupling explicitly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@config/clients/python/template/src/sync/api.py.mustache` around lines 233 -
260, The code relies on the side-effect of ApiClient.last_response instead of
the call_api return value; update the call in the method to capture call_api's
return (e.g., response = self.api_client.call_api(...)) and use that response to
construct the RawResponse (status, headers via response.getheaders(), body via
ResponseParser.parse_body(response.data)); retain a safe fallback to
getattr(self.api_client, "last_response", None) only if the call_api return is
None, and adjust variable names (rest_response/response) consistently so
RawResponse construction uses the captured response rather than always reading
last_response.
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
1 participant
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#252
What problem is being solved?
How is it being solved?
What changes are made to solve it?
References
Review Checklist
mainSummary by CodeRabbit