feat: add raw requestor for calling arbitrary endpoints #252
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 Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis PR introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client as OpenFgaClient
participant ApiClient as ApiClient
participant HTTP as HTTP Layer
participant Response as Response Parser
Client->>Client: raw_request(method, path, params...)
Client->>Client: Validate operation_name (required)
Client->>Client: Resolve path params (auto-fill store_id)
Client->>Client: URL-encode path params
Client->>Client: Build query string
Client->>Client: Serialize body (JSON if dict/list)
Client->>Client: Set default headers (Content-Type, Accept)
Client->>ApiClient: call_api(method, path, body, headers, query_params)
ApiClient->>HTTP: Execute HTTP request
HTTP->>ApiClient: Return HTTP response
ApiClient->>Response: Parse response body
Response->>Response: Try JSON decode, fallback to raw
Response->>Client: Return RawResponse(status, headers, body)
Client->>Client: Return RawResponse to caller
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
|
Related Documentation Checked 6 published document(s) in 1 knowledge base(s). No updates required. How did I do? Any feedback? |
SoulPancake
changed the title
kcbiradar
force-pushed
the
feat/add-raw-request-method
branch
from
840bc79 to
f4dec14
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
4-4: Malformed badge URL - missing closing parenthesis.The Socket badge URL appears to be malformed. It's missing a closing parenthesis before the opening bracket for the link URL.
🔧 Suggested fix
-[ +[](https://socket.dev/pypi/package/openfga-sdk)
🤖 Fix all issues with AI agents
In `@openfga_sdk/sync/client/client.py`:
- Around line 1233-1234: The except block "except ApiException as e: raise" is
redundant and should be removed; locate the handler in
openfga_sdk/sync/client/client.py where ApiException is caught and simply delete
that except clause so the original ApiException will propagate naturally (leave
surrounding try/except structure and other exception handlers unchanged and
ensure indentation remains valid).
🧹 Nitpick comments (6)
openfga_sdk/client/models/raw_response.py (1)
49-63: Consider movingjsonimport to module level.The
jsonmodule is imported inside the method body (lines 50 and 58). Sincejsonis part of Python's standard library and always available, importing it at the module level (line 8-9 area) would be more idiomatic and slightly more efficient for repeated calls.♻️ Suggested refactor
Add import at the top of the file:
from dataclasses import dataclass from typing import Any +import jsonThen remove the inline imports in
json()andtext()methods.openfga_sdk/sync/client/client.py (2)
1139-1141: Consider validatingoptions["headers"]type before merging.The code assumes
options["headers"]is a dict when callingupdate(). For consistency with theset_heading_if_not_sethelper (lines 102-103) which handles non-dict headers values, consider adding a type check.♻️ Suggested defensive check
request_headers = dict(headers) if headers else {} if options and options.get("headers"): - request_headers.update(options["headers"]) + opt_headers = options.get("headers") + if isinstance(opt_headers, dict): + request_headers.update(opt_headers)
1196-1199: Header checks are case-sensitive but HTTP headers are case-insensitive.The checks
"Content-Type" not in request_headerswon't match if a user provides"content-type"(lowercase). This could result in duplicate headers with different casing.♻️ Suggested case-insensitive check
+ # Case-insensitive header check helper + request_headers_lower = {k.lower(): k for k in request_headers} + - if "Content-Type" not in request_headers: + if "content-type" not in request_headers_lower: request_headers["Content-Type"] = "application/json" - if "Accept" not in request_headers: + if "accept" not in request_headers_lower: request_headers["Accept"] = "application/json"test/sync/client/client_test.py (1)
4151-4454: Good raw_request coverage; consider 2 small edge-case tests.
- Assert that custom headers passed to
raw_request(..., headers=...)are actually forwarded (e.g., verifyX-Experimental-Featureis present in the mocked call headers).- Add a non-JSON response body case (e.g.,
content-type: text/plain) to validateRawResponse.text()/json()behavior when JSON parsing should fail.openfga_sdk/client/client.py (2)
1140-1143: Define header precedence + coerce header values to strings.Right now
options["headers"]overrides the explicitheadersargument (Line 1140-1143), and non-string values can slip in (your tests cover integer header values elsewhere). It’d be good to (a) document precedence, and (b) normalize values tostrbefore sending.
1209-1234: Tighten telemetry + response typing.
TelemetryAttributeimport inside the method is unused (Line 1211).TelemetryAttributes.fga_client_request_methodis set tooperation_name.lower()(Line 1213-1214). Please confirm this attribute is meant to hold the operation name vs the HTTP verb (since you already havemethod).- Response parsing should allow top-level JSON arrays too (currently annotated as
dict[str, Any], butjson.loads(...)can returnlist[Any]). Consider widening the type and aligningRawResponse.bodyaccordingly.Also applies to: 1242-1265
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
README.mdopenfga_sdk/api_client.pyopenfga_sdk/client/client.pyopenfga_sdk/client/models/__init__.pyopenfga_sdk/client/models/raw_response.pyopenfga_sdk/sync/api_client.pyopenfga_sdk/sync/client/client.pyopenfga_sdk/telemetry/attributes.pytest/client/client_test.pytest/sync/client/client_test.py
🧰 Additional context used
🧬 Code graph analysis (6)
openfga_sdk/client/models/__init__.py (1)
openfga_sdk/client/models/raw_response.py (1)
RawResponse(13-90)
openfga_sdk/sync/api_client.py (3)
openfga_sdk/rest.py (1)
getheaders(111-115)openfga_sdk/sync/rest.py (1)
getheaders(111-115)test/error_handling_test.py (1)
getheaders(31-32)
openfga_sdk/api_client.py (3)
openfga_sdk/rest.py (1)
getheaders(111-115)openfga_sdk/sync/rest.py (1)
getheaders(111-115)test/error_handling_test.py (1)
getheaders(31-32)
test/client/client_test.py (1)
openfga_sdk/client/models/raw_response.py (3)
RawResponse(13-90)json(33-65)text(67-90)
test/sync/client/client_test.py (2)
openfga_sdk/client/models/raw_response.py (3)
RawResponse(13-90)json(33-65)text(67-90)openfga_sdk/rest.py (6)
RESTClientObject(128-461)response(56-60)response(63-67)status(84-88)status(91-95)close(162-166)
openfga_sdk/client/client.py (6)
openfga_sdk/client/models/raw_response.py (2)
json(33-65)RawResponse(13-90)openfga_sdk/exceptions.py (2)
ApiException(118-298)FgaValidationException(36-61)openfga_sdk/sync/rest.py (5)
RESTResponse(27-125)data(70-74)data(77-81)status(84-88)status(91-95)openfga_sdk/telemetry/attributes.py (3)
get(97-104)TelemetryAttribute(14-16)TelemetryAttributes(19-369)openfga_sdk/api_client.py (2)
get_store_id(937-941)update_params_for_auth(771-810)openfga_sdk/sync/api_client.py (2)
get_store_id(937-941)update_params_for_auth(771-810)
🔇 Additional comments (17)
openfga_sdk/client/models/__init__.py (1)
26-26: LGTM!The
RawResponseimport and export follow the established pattern in this module. This correctly exposes the new model for downstream usage.Also applies to: 49-49
openfga_sdk/api_client.py (1)
420-423: LGTM!The change from
response_data.headerstoresponse_data.getheaders()ensures consistent header access and returns a properdict[str, str]that aligns with the newRawResponsemodel's expected header format.openfga_sdk/sync/api_client.py (1)
420-423: LGTM!Consistent with the async client change, ensuring both sync and async variants use the same
getheaders()method for header access.openfga_sdk/telemetry/attributes.py (1)
298-302: Good defensive fix for handling non-dict response bodies.The added
isinstance(response.body, dict)check properly guards againstAttributeErrorwhenbodyisbytes,str, orNone- which is possible with the newRawResponsemodel where body type isbytes | str | dict[str, Any] | None.README.md (2)
51-51: LGTM!Good addition to the table of contents for the new feature section.
1264-1400: Comprehensive documentation for the newraw_requestfeature.The documentation is well-structured with clear use cases and multiple examples covering:
- Basic POST requests
- Response handling (JSON parsing, accessing headers/status)
- GET requests with query parameters
- Path parameter substitution (explicit and automatic store_id)
The examples follow the existing documentation patterns in this README.
openfga_sdk/client/models/raw_response.py (1)
33-90: LGTM! The helper methods are well-implemented.The
json()andtext()methods handle all expected body types (dict, str, bytes, None) with appropriate error handling and fallbacks. The UnicodeDecodeError handling witherrors="replace"intext()is a good defensive pattern.openfga_sdk/sync/client/client.py (3)
1-4: LGTM! Imports are appropriate for the new functionality.The new imports (
json,urllib.parse,RawResponse,ApiException,RESTResponse) are all necessary for theraw_requestmethod implementation.Also applies to: 38-38, 46-46, 77-77
1146-1167: LGTM! Path parameter handling is well-implemented.The logic correctly:
- Auto-fills
store_idfrom configuration when not provided inpath_params- URL-encodes path parameter values for safety
- Validates that all placeholders are resolved before making the request
1243-1265: LGTM! Response body parsing handles all expected formats.The parsing logic correctly handles JSON responses (both string and bytes) with appropriate fallbacks for non-JSON content. The final
RawResponseconstruction properly captures status, headers, and body.test/client/client_test.py (5)
28-28: LGTM! Import is correctly placed.The
RawResponseimport is appropriately added alongside other model imports.
4170-4218: LGTM! Comprehensive test for POST request with body.This test verifies:
- RawResponse creation and properties (status, headers, body)
json()andtext()helper methods work correctly- The API is called with correct method, path, body, query params, and headers
4330-4379: LGTM! Validation error tests cover required scenarios.The tests properly verify that
FgaValidationExceptionis raised for:
- Missing
operation_name(required parameter)- Missing
store_idwhen path contains{store_id}- Missing path parameters when placeholders exist
4381-4413: LGTM! List query parameter expansion test.This test verifies that list values in
query_paramsare correctly expanded into multiple key-value pairs (e.g.,{"ids": ["id1", "id2", "id3"]}becomes[("ids", "id1"), ("ids", "id2"), ("ids", "id3")]).
4444-4475: LGTM! URL encoding test verifies special character handling.The test confirms that path parameters containing spaces and special characters (
value with spaces & special chars) are properly URL-encoded tovalue%20with%20spaces%20%26%20special%20chars.test/sync/client/client_test.py (1)
26-26: RawResponse import looks fine.openfga_sdk/client/client.py (1)
3-6: No issues with the new imports.Also applies to: 39-40, 46-51, 76-76
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
@rhamzeh @SoulPancake, please review and suggest if any changes are required. |
There was a problem hiding this comment.
Pull request overview
This PR adds a raw_request method to the OpenFGA Python SDK, enabling developers to call arbitrary FGA endpoints that may not yet be wrapped by the SDK. This is useful for calling experimental features, custom endpoints, or new API features in older SDK versions.
Changes:
- Added
RawResponsedataclass with status, headers, body, and helper methods (json(),text()) - Implemented
raw_request()method in both async and sync clients with path parameter substitution, query parameter handling, and automatic store_id resolution - Fixed API client header handling to use
getheaders()for proper dict conversion
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| openfga_sdk/client/models/raw_response.py | New RawResponse model with status, headers, body, and utility methods |
| openfga_sdk/client/client.py | Async raw_request implementation with URL encoding, query params, and telemetry |
| openfga_sdk/sync/client/client.py | Sync version of raw_request with identical logic |
| openfga_sdk/api_client.py | Fixed header handling to return dict from getheaders() |
| openfga_sdk/sync/api_client.py | Fixed header handling in sync client |
| openfga_sdk/telemetry/attributes.py | Added type check for response.body before dict operations |
| openfga_sdk/client/models/init.py | Exported RawResponse in module |
| test/client/client_test.py | Comprehensive async tests for raw_request |
| test/sync/client/client_test.py | Comprehensive sync tests for raw_request |
| README.md | Documentation with usage examples |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Thanks for your PR @kcbiradar |
kcbiradar
force-pushed
the
feat/add-raw-request-method
branch
from
b1adb8c to
3f85dfa
Compare
|
@SoulPancake Thanks for reviewing, updated failing testcases. Please can you re-review once? |
Codecov Report❌ Patch coverage is
❌ Your project status has failed because the head coverage (71.22%) is below the target coverage (80.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #252 +/- ##
==========================================
+ Coverage 71.17% 71.22% +0.05%
==========================================
Files 137 138 +1
Lines 11100 11289 +189
==========================================
+ Hits 7900 8041 +141
- Misses 3200 3248 +48 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@SoulPancake, any changes needed here? |
openfga_sdk/sync/api_client.py
Outdated
| return return_data | ||
| else: | ||
| return (return_data, response_data.status, response_data.headers) | ||
| return (return_data, response_data.status, response_data.getheaders()) |
There was a problem hiding this comment.
Is this a required change for the functionality
openfga_sdk/api_client.py
Outdated
| return return_data | ||
| else: | ||
| return (return_data, response_data.status, response_data.headers) | ||
| return (return_data, response_data.status, response_data.getheaders()) |
There was a problem hiding this comment.
| authorization_model_id, api_request_body, **kwargs | ||
| ) | ||
| return api_response | ||
|
|
There was a problem hiding this comment.
this already parses JSON
if isinstance(rest_response.data, str):
try:
response_body = json.loads(rest_response.data) # here
except (json.JSONDecodeError, ValueError):
response_body = rest_response.data
Then in RawResponse.json() it is parsed again
def json(self) -> dict[str, Any] | None:
if isinstance(self.body, str):
import json
try:
return json.loads(self.body) ## here again
Is that needed
| return self.body.decode("utf-8", errors="replace") | ||
|
|
||
| if isinstance(self.body, dict): | ||
| import json |
There was a problem hiding this comment.
Let's move the imports to the top level
|
@SoulPancake Thanks for reviewing, the requested changes are updated |
|
@SoulPancake please re-review once |
| ) | ||
|
|
||
| if response.body is not None: | ||
| if response.body is not None and isinstance(response.body, dict): |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| request_headers = dict(headers) if headers else {} | ||
| if options and options.get("headers"): | ||
| request_headers.update(options["headers"]) |
There was a problem hiding this comment.
The code assumes that options["headers"] is a dict when present, but the type annotation for options allows headers to be int | str | dict. If options.get("headers") returns a non-dict value (like an int or string), calling request_headers.update() will raise a TypeError at runtime. Add type checking to ensure options["headers"] is a dict before attempting to update, similar to how it's done elsewhere in the codebase (see line 104).
| request_headers.update(options["headers"]) | |
| if isinstance(options["headers"], dict): | |
| request_headers.update(options["headers"]) |
| f"Failed to get response from API client for {method.upper()} " | ||
| f"request to '{resource_path}'" | ||
| f"{f' (operation: {operation_name})' if operation_name else ''}. " |
There was a problem hiding this comment.
The f-string contains a nested f-string that will always evaluate to a non-empty string. The expression f' (operation: {operation_name})' is always truthy even when operation_name is None or empty. Change the conditional to check operation_name directly instead of the formatted string.
| raise RuntimeError( | ||
| f"Failed to get response from API client for {method.upper()} " | ||
| f"request to '{resource_path}'" | ||
| f"{f' (operation: {operation_name})' if operation_name else ''}. " |
There was a problem hiding this comment.
The f-string contains a nested f-string that will always evaluate to a non-empty string. The expression f' (operation: {operation_name})' is always truthy even when operation_name is None or empty. Change the conditional to check operation_name directly instead of the formatted string.
| raise RuntimeError( | |
| f"Failed to get response from API client for {method.upper()} " | |
| f"request to '{resource_path}'" | |
| f"{f' (operation: {operation_name})' if operation_name else ''}. " | |
| operation_suffix = f" (operation: {operation_name})" if operation_name else "" | |
| raise RuntimeError( | |
| f"Failed to get response from API client for {method.upper()} " | |
| f"request to '{resource_path}'{operation_suffix}. " |
| if options and options.get("headers"): | ||
| request_headers.update(options["headers"]) |
There was a problem hiding this comment.
The code assumes that options["headers"] is a dict when present, but the type annotation for options allows headers to be int | str | dict. If options.get("headers") returns a non-dict value (like an int or string), calling request_headers.update() will raise a TypeError at runtime. Add type checking to ensure options["headers"] is a dict before attempting to update, similar to how it's done elsewhere in the codebase (see line 104).
| if options and options.get("headers"): | |
| request_headers.update(options["headers"]) | |
| if options: | |
| options_headers = options.get("headers") | |
| if isinstance(options_headers, dict): | |
| request_headers.update(options_headers) |
3 participants
This adds the capability to call any FGA endpoint.
What problem is being solved?
How is it being solved?
What changes are made to solve it?
References
closes
Review Checklist
mainSummary by CodeRabbit
Release Notes
New Features
raw_requestmethod to OpenFgaClient for making custom HTTP requests to OpenFGA endpointsBug Fixes
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.