feat: improve error messaging #245
Conversation
|
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. WalkthroughThe changes enhance error handling across the SDK by adding operation context to API exceptions, improving error message formatting with structured details (status, code, message, request ID), introducing helper methods for error categorization (validation, not found, authentication, rate limit, retryable, client/server errors), and adding comprehensive documentation and tests. Both async and sync implementations are updated consistently. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant APIClient as API Client
participant Telemetry as Telemetry<br/>Attributes
participant Exception as ApiException
Client->>APIClient: Make API call
APIClient->>APIClient: __call_api()
alt HTTP Error Response
APIClient->>APIClient: Parse error response
APIClient->>Telemetry: Get fga_client_request_method
Telemetry-->>APIClient: operation method name
rect rgb(200, 220, 240)
note over APIClient,Exception: NEW: Attach operation context
APIClient->>Exception: exception.operation_name =<br/>method_name.lower()
end
APIClient->>APIClient: Raise exception
end
Exception-->>Client: Throw enhanced exception<br/>with operation context
Client->>Exception: Access properties:<br/>operation_name, code,<br/>error_message, request_id
Client->>Exception: Call helper methods:<br/>is_validation_error(),<br/>is_retryable(), etc.
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Areas requiring extra attention:
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #245 +/- ##
==========================================
+ Coverage 70.99% 71.17% +0.18%
==========================================
Files 137 137
Lines 11038 11100 +62
==========================================
+ Hits 7836 7900 +64
+ Misses 3202 3200 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This PR enhances error handling in the OpenFGA Python SDK by adding structured error messages with operation context, exposing error codes and request IDs, and providing convenient helper methods for error categorization. The changes make it easier for SDK users to understand, debug, and handle different types of errors programmatically.
Key changes:
- Enhanced error message formatting with operation context and structured details
- Added properties (
code,error_message,request_id) and helper methods (is_validation_error(),is_retryable(), etc.) to exception classes - Modified exception constructors to support an optional
operation_nameparameter - Updated API clients to automatically populate operation names from telemetry attributes
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| openfga_sdk/exceptions.py | Enhanced ApiException with new properties, helper methods, and improved __str__ formatting; updated all exception subclasses to accept operation_name parameter |
| openfga_sdk/api_client.py | Added logic to set operation_name on exceptions from telemetry attributes in async API client |
| openfga_sdk/sync/api_client.py | Added logic to set operation_name on exceptions from telemetry attributes in sync API client |
| test/error_handling_test.py | Comprehensive unit tests for error formatting, properties, and helper methods |
| test/error_integration_test.py | Integration tests for error handling with real OpenFGA server |
| README.md | Added documentation for error handling features with usage examples |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
README.md (1)
1410-1431: Align error-handling example with existing write API and namingTo keep the docs consistent and avoid confusion:
- Reuse the
fga_clientnaming from earlier examples.- Avoid using
tupleas a variable name (shadows built‑in).- Show a realistic write call (e.g., via
ClientWriteRequestandClientTuple) so the snippet is copy‑pasteable.For example:
-from openfga_sdk.exceptions import ApiException - -try: - await client.write([tuple]) +from openfga_sdk.exceptions import ApiException +from openfga_sdk.client.models import ClientTuple, ClientWriteRequest + +try: + await fga_client.write( + ClientWriteRequest( + writes=[ + ClientTuple( + user="user:anne", + relation="viewer", + object="document:readme", + ) + ] + ) + )The rest of the snippet (checking
is_validation_error()/is_retryable()) looks good.openfga_sdk/api_client.py (1)
319-326: Operation name propagation from telemetry into ApiException looks correctThe new blocks correctly:
- Pull
fga_client.request.methodfrom_telemetry_attributesand- Normalize it with
.lower()before assigninge.operation_name,so exceptions can render
[write],[check], etc. This matches the newApiException.__str__semantics.You could optionally:
- Drop the
isinstance(e, ApiException)checks here (the caught types are already ApiException subclasses), and/or- Extract a tiny helper (e.g.,
_set_operation_name_from_telemetry(e, _telemetry_attributes)) to avoid duplicating the same pattern across both except branches (and in the sync client).But as‑is, the logic is sound.
Also applies to: 359-366
openfga_sdk/sync/api_client.py (1)
317-326: Sync client error context is now consistent with async clientThe sync
ApiClientnow mirrors the async behaviour by settinge.operation_namefromTelemetryAttributes.fga_client_request_methodbefore re‑raising, which ensures consistent[method] ...error strings and helper behaviour across both clients.As with the async version, you might:
- Remove the redundant
isinstance(e, ApiException)checks, and/or- Factor this into a shared helper to avoid repeating the same pattern in two branches and two files.
Functionally, this looks good.
Also applies to: 359-367
test/error_integration_test.py (1)
57-96: Integration tests correctly validate enriched error context; consider closing async clientThe async and sync integration tests exercise the right behaviours:
- They confirm
operation_name(write,check,expand) is populated from telemetry.- They assert status, code,
error_message, helper predicates, and the formatted string across multiple failure modes.- Skip logic based on
FGA_API_URL//.dockerenvkeeps CI flexible.One small improvement for the async fixture:
- In
fga_client, you createOpenFgaClient(config)instances but neverawait client.close()or useasync with OpenFgaClient(...)in setup/teardown. Depending on the underlying HTTP stack, this can lead to unclosed client sessions and noisy warnings under pytest.You could, for example, ensure cleanup closes the client:
@@ async def fga_client(self): - client = OpenFgaClient(config) + client = OpenFgaClient(config) @@ - yield client - - # Cleanup: delete the store - try: - await client.delete_store() - except Exception: - pass # Ignore cleanup errors + try: + yield client + finally: + try: + await client.delete_store() + except Exception: + pass + await client.close()The sync fixture is fine as‑is; its
ApiClient.close()only manages the thread pool and doesn’t require async teardown.Also applies to: 425-463
openfga_sdk/exceptions.py (1)
233-297: Consider teaching the helper predicates about the specific subclassesThe helpers are handy, but they currently ignore the dedicated subclasses:
is_authentication_errordoesn’t checkAuthenticationError/UnauthorizedException.is_rate_limit_errordoesn’t checkRateLimitExceededError.If these subclasses are ever raised without a populated HTTP status/code, the helpers will return
False. You could make them a bit more robust with something like:def is_authentication_error(self): @@ - return self.status == 401 + return isinstance(self, (AuthenticationError, UnauthorizedException)) or self.status == 401 @@ def is_rate_limit_error(self): @@ - 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()) + )This keeps status/code checks intact while also honoring the semantic subclasses.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
README.md(1 hunks)openfga_sdk/api_client.py(2 hunks)openfga_sdk/exceptions.py(3 hunks)openfga_sdk/sync/api_client.py(2 hunks)test/error_handling_test.py(1 hunks)test/error_integration_test.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
test/error_integration_test.py (7)
openfga_sdk/client/configuration.py (1)
ClientConfiguration(14-78)openfga_sdk/exceptions.py (12)
NotFoundException(299-301)ValidationException(319-321)code(188-201)error_message(204-215)is_not_found_error(244-251)is_client_error(280-287)is_server_error(289-296)is_retryable(271-278)is_validation_error(233-242)is_authentication_error(253-260)is_rate_limit_error(262-269)request_id(218-231)openfga_sdk/models/create_store_request.py (1)
CreateStoreRequest(20-120)openfga_sdk/client/models/tuple.py (1)
ClientTuple(5-75)openfga_sdk/client/models/write_request.py (1)
ClientWriteRequest(6-107)openfga_sdk/client/models/check_request.py (1)
ClientCheckRequest(4-94)openfga_sdk/client/models/expand_request.py (1)
ClientExpandRequest(4-59)
openfga_sdk/api_client.py (3)
openfga_sdk/exceptions.py (1)
ApiException(118-296)openfga_sdk/telemetry/attributes.py (1)
TelemetryAttributes(19-369)openfga_sdk/telemetry/configuration.py (2)
fga_client_request_method(175-182)fga_client_request_method(185-193)
openfga_sdk/sync/api_client.py (2)
openfga_sdk/exceptions.py (1)
ApiException(118-296)openfga_sdk/telemetry/attributes.py (1)
TelemetryAttributes(19-369)
test/error_handling_test.py (3)
openfga_sdk/exceptions.py (17)
ApiException(118-296)NotFoundException(299-301)RateLimitExceededError(329-331)ServiceException(314-316)ValidationException(319-321)code(188-201)parsed_exception(174-178)parsed_exception(181-185)error_message(204-215)request_id(218-231)is_validation_error(233-242)is_client_error(280-287)is_server_error(289-296)is_retryable(271-278)is_not_found_error(244-251)is_authentication_error(253-260)is_rate_limit_error(262-269)openfga_sdk/models/validation_error_message_response.py (1)
ValidationErrorMessageResponse(20-143)openfga_sdk/models/error_code.py (1)
ErrorCode(20-212)
openfga_sdk/exceptions.py (5)
openfga_sdk/sync/rest.py (4)
status(84-88)status(91-95)reason(98-102)reason(105-109)openfga_sdk/rest.py (4)
status(84-88)status(91-95)reason(98-102)reason(105-109)openfga_sdk/models/forbidden_response.py (2)
code(54-61)code(64-72)openfga_sdk/models/internal_error_message_response.py (4)
code(54-61)code(64-72)message(75-82)message(85-93)openfga_sdk/models/validation_error_message_response.py (4)
code(54-61)code(64-72)message(75-82)message(85-93)
⏰ 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). (1)
- GitHub Check: Agent
🔇 Additional comments (5)
test/error_handling_test.py (1)
1-310: Thorough unit coverage of enhanced ApiException behaviourThis test module does a solid job of exercising:
- Formatted error strings (including operation name, HTTP status, code, message, request‑id),
- Property accessors (
code,error_message,request_id,operation_name),- All the helper predicates (validation/not-found/auth/rate‑limit/retryable/client/server),
- Subclass behaviours and enum‑backed codes.
The
MockHTTPResponse/MockHeadersshims match howApiExceptionconsumeshttp_resp, so this should remain stable even as the HTTP layer evolves.openfga_sdk/exceptions.py (4)
119-142: Operation name plumbing is straightforward and backward‑compatibleAppending
operation_nameat the end of the constructor and simply storing it onself.operation_namekeeps existing call sites working while enabling richer context; the rest of the init logic is unchanged and looks correct.
143-172: Structured__str__formatting looks good; watch for callers relying on old formatThe new composition
[operation] HTTP <status> <message> (<code>) [request-id: ...]is clear and uses the derived helpers appropriately. It does, however, change the exception’s string representation semantics, so just confirm there are no consumers parsing or snapshotting the previousstr(ApiException)format (or update them alongside this change).
187-232: Derived properties for code/message/request-id are well factoredUsing
parsed_exceptionas the primary source and falling back to HTTP reason/defaults is sensible, and the small enum handling incodeplus case-insensitive lookup inrequest_idmatch the surrounding models and header handling.
299-332: Subclass constructors correctly forwardoperation_nameAll specialized exceptions mirror the base signature and pass
operation_namethrough, so existing usages remain valid while gaining the extra context.
Description
What problem is being solved?
How is it being solved?
What changes are made to solve it?
References
Review Checklist
mainSummary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.