feat: Improve error messaging #260
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 enhances error handling in the OpenFGA Java SDK by parsing API error responses, exposing structured error details (code, message, operation name), capturing request IDs from response headers, and formatting error messages with operation context. Comprehensive integration tests validate various error scenarios across multiple operations. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SDK as FgaError.getError()
participant Parser as ObjectMapper
participant Error as FgaError instance
Client->>SDK: receive HTTP error response
SDK->>SDK: extract x-request-id header
SDK->>Parser: parse JSON error body
Parser-->>SDK: ApiErrorResponse (code, message)
SDK->>Error: populate apiErrorMessage
SDK->>Error: populate operationName
SDK->>Error: set requestId from header
Error->>Error: getMessage() formats<br/>[operationName] message(code)
SDK-->>Client: FgaError with structured details
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
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
❌ Your project status has failed because the head coverage (37.13%) 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 #260 +/- ##
============================================
+ Coverage 36.89% 37.13% +0.23%
- Complexity 1187 1195 +8
============================================
Files 192 192
Lines 7410 7470 +60
Branches 854 862 +8
============================================
+ Hits 2734 2774 +40
- Misses 4553 4566 +13
- Partials 123 130 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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 (7)
src/main/java/dev/openfga/sdk/errors/FgaError.java (3)
24-30: ApiErrorResponse visibility and new error-detail fields look good; consider tightening the public surfaceThe addition of
apiErrorMessage/operationName(with getters/setters) cleanly exposes structured error context to callers and aligns with the new integration tests’ expectations.The nested
ApiErrorResponsetype, however, is declaredpublicwith public mutable fields but is only consumed internally ingetError. That effectively leaks the backend’s wire-level error schema into the SDK’s public API surface without providing a first-class way to obtain instances of this type.If you don’t intend SDK consumers to rely on
ApiErrorResponsedirectly, consider making it an implementation detail:- public static class ApiErrorResponse { - public String code; - public String message; - } + private static class ApiErrorResponse { + public String code; + public String message; + }This preserves JSON binding while keeping the public API focused on
FgaError’s own fields (apiErrorCode,apiErrorMessage,operationName, etc.).Also applies to: 176-190
5-6: JSON parsing and request-id extraction are correct; consider reusing ObjectMapper and avoiding System.err in library codeThe flow to set
operationName, parse the JSON response body intoApiErrorResponse, and extract the"x-request-id"header is sound and matches what the new integration tests assert.Two refinements to consider:
Reuse
ObjectMapperinstead of allocating per errorCreating a new
ObjectMapperon each error is relatively heavy and unnecessary here:- if (body != null && !body.trim().isEmpty()) { - ObjectMapper mapper = new ObjectMapper(); + if (body != null && !body.trim().isEmpty()) { + // Reuse a single mapper instance for error parsing + ObjectMapper mapper = ERROR_OBJECT_MAPPER;with a cached mapper:
+ private static final ObjectMapper ERROR_OBJECT_MAPPER = new ObjectMapper();This keeps error handling lightweight, especially in high-error-rate scenarios.
Avoid
System.err.printlnin a reusable SDKPrinting directly to
System.errfrom library code can be noisy and hard to manage for consumers. It’s usually better to either:
- Use the project’s logging abstraction, or
- Silently ignore parse failures and rely on
super.getMessage()+getResponseData()for diagnostics.For example, if you have a logger available:
- } catch (JsonProcessingException e) { - // Fall back, do nothing - log the exception for debugging - System.err.println("Failed to parse API error response JSON: " + e.getMessage()); - } + } catch (JsonProcessingException e) { + // Fall back silently or log via the project logger if available + LOGGER.debug("Failed to parse API error response JSON", e); + }(Adjust to whatever logging facility the rest of the SDK uses.)
Also applies to: 87-107
102-107: Request-id extraction logic is fine; consider centralizing the header nameExtracting
"x-request-id"viaheaders.firstValue("x-request-id")and storing it on the error object is useful for debugging and matches the integration tests.To avoid scattering magic header strings and to keep things consistent with
FgaConstants.RETRY_AFTER_HEADER_NAME, consider introducing a constant for the request-id header (either here or inFgaConstants) and reusing it wherever needed in the future.src/test-integration/java/dev/openfga/sdk/errors/FgaErrorIntegrationTest.java (4)
37-40: Consider loading auth-model.json from the classpath instead of a hard-coded filesystem pathReading the model via:
authModelJson = Files.readString(Paths.get("src", "test-integration", "resources", "auth-model.json"));ties the tests to a specific working directory layout. Using a classpath resource tends to be more robust across build tools and IDEs. For example:
var url = getClass().getClassLoader().getResource("auth-model.json"); assertNotNull(url, "auth-model.json should be on the test classpath"); authModelJson = Files.readString(Paths.get(url.toURI()));and put
auth-model.jsonon the test (or integration-test) classpath.
42-58: Per-test client/store setup is correct; you may be able to amortize it if tests growThe
initializeApimethod correctly initializesOpenFgaClient, creates a store, and writes the authorization model before each test, ensuring isolation.If this suite expands or becomes slow, you could consider:
- Moving store creation + model write to a
@BeforeAllmethod and reusing the same store across tests (since tests only read/write invalid data), or- Sharing
OpenFgaClientand storeId at class level while resetting any mutable state as needed.Not urgent, but a possible optimization if CI times become an issue.
212-231: Not-found error assertions are reasonable; consider asserting on operationName directly
testNotFoundErrorcorrectly:
- Exercises a non-existent store path,
- Asserts the cause is an
FgaError(allowing forFgaApiNotFoundErroror other subclasses),- Verifies message presence and core error metadata.
Given that
operationNameis now explicitly tracked, you might make the test a bit more robust and less dependent on message formatting by also checking:assertEquals("getStore", exception.getOperationName());rather than relying solely on substrings in
getMessage().
251-278: Authentication/authorization error test is flexible; update comment to match behavior
testAuthenticationErrorintentionally accepts anyFgaErrorsubclass (not strictlyFgaApiAuthenticationError), acknowledging in the code comment that the backend may return either 404 or 403/401 depending on version, and then checks for a sensible message,apiErrorCode, andrequestId.The assertions are appropriate; the only small mismatch is the leading comment:
// Test FgaApiAuthenticationError when store doesn't exist (403/401 from API)Given the test is intentionally tolerant of both authentication and not-found cases, you might reword the comment to something like “Test that authentication/authorization related errors surface details when the store doesn’t exist” to better match the actual assertion strategy.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/dev/openfga/sdk/errors/FgaError.java(4 hunks)src/test-integration/java/dev/openfga/sdk/errors/FgaErrorIntegrationTest.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/test-integration/java/dev/openfga/sdk/errors/FgaErrorIntegrationTest.java (4)
src/main/java/dev/openfga/sdk/api/client/OpenFgaClient.java (1)
OpenFgaClient(23-1314)src/main/java/dev/openfga/sdk/api/client/model/ClientTupleKey.java (1)
ClientTupleKey(9-69)src/main/java/dev/openfga/sdk/api/client/model/ClientWriteRequest.java (1)
ClientWriteRequest(5-34)src/main/java/dev/openfga/sdk/api/configuration/ClientConfiguration.java (1)
ClientConfiguration(8-137)
⏰ 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: Agent
- GitHub Check: CodeQL analysis (java)
🔇 Additional comments (12)
src/main/java/dev/openfga/sdk/errors/FgaError.java (1)
192-200: getMessage override achieves the intended formatting; be aware of changed message shapeThe new
getMessage()implementation:if (apiErrorMessage != null && !apiErrorMessage.isEmpty() && operationName != null) { String codePart = (apiErrorCode != null && !apiErrorCode.isEmpty()) ? " (" + apiErrorCode + ")" : ""; return String.format("[%s] %s%s", operationName, apiErrorMessage, codePart); } else { return super.getMessage(); }correctly yields messages like
[write] type 'invalid_type' not found (validation_error), which the integration tests assert on, and still falls back tosuper.getMessage()if structured fields are missing.The only behavioral change to be mindful of is that, once
apiErrorMessageandoperationNameare set, the formatted message no longer includes any extra context that might have been present in the originalsuper.getMessage()(such as HTTP status or low-level detail). Callers that need that information can still accessgetCode(),getCause(), andgetResponseData(), so this is likely acceptable, but it’s worth acknowledging as an intentional change in how messages are presented.src/test-integration/java/dev/openfga/sdk/errors/FgaErrorIntegrationTest.java (11)
60-79: Validation error for invalid type is well-covered
testWriteValidationErrorcleanly verifies:
- The exception type (
FgaApiValidationError),- That the formatted message includes the backend message,
apiErrorCode,apiErrorMessage,operationName, andrequestId.This is a solid regression test for the new error-parsing behavior on write.
81-102: Validation error for invalid relation correctly asserts formatted message and metadata
testWriteValidationErrorWithInvalidRelationnicely checks:
- Message contains both the operation name and specific relation error text,
apiErrorCodeis populated,operationNameandrequestIdare set.Good coverage of a distinct validation failure mode for the same operation.
104-130: Message formatting test directly validates the new getMessage contract
testErrorMessageFormattingWithAllFieldsexplicitly asserts:
getMessage()format[operationName] apiErrorMessage (apiErrorCode),- Individual fields like
operationName,apiErrorCode,apiErrorMessage, andrequestId.This is an important high-level contract test for the overridden
getMessage()and the JSON parsing inFgaError.
132-152: Check operation wiring and error mapping are validated appropriately
testCheckValidationErrorensures that:
checkoperations surfaceFgaApiValidationError,operationNameis"check"and reflected in the message,apiErrorCode,apiErrorMessage, andrequestIdare set.This guards against mislabeling the operation context when mapping errors from the low-level API.
154-183: Stack-trace and message detail preservation test is effective
testErrorDetailsAreNotLostInStackTraceverifies that:
- The cause type remains
FgaApiValidationError,- Both
toString()andgetMessage()contain the backend error details and operation/code,getResponseData()is still populated.This is a good safeguard against future changes that might accidentally strip detail when wrapping exceptions.
185-210: Batch write validation error behavior is covered
testMultipleTupleErrorsShowDetailedMessageensures that, even when multiple tuples are invalid, the resultingFgaApiValidationErrorstill:
- Contains a useful message mentioning the operation,
- Exposes
apiErrorCode,apiErrorMessage, andrequestId.This is valuable coverage for non-singleton error scenarios.
233-249: Invalid-parameter behavior at the client layer is well tested
testInvalidParameterExceptionensures that:
- A write with missing
storeIdand an empty request fails fast withFgaInvalidParameterExceptionbefore any API call,- The message surfaces enough context about the bad parameter.
This is a good guardrail around client-side validation.
280-299: Read operation error mapping and context are validated correctly
testReadOperationErrorconfirms that:
- Invalid read requests result in
FgaApiValidationError,operationNameis"read"(or at least present in the message),apiErrorCode,apiErrorMessage, andrequestIdare set.This nicely mirrors the write/check coverage for the read path.
301-320: Expand operation error behavior is covered analogously to other operations
testExpandOperationErrorexercises theexpandpath and verifies:
FgaApiValidationErroris thrown,operationNameis"expand"(or present in the message),apiErrorCode,apiErrorMessage, andrequestIdare populated.Good symmetry across all major operations.
322-352: Different validation subcases sharing the same code are clearly distinguished
testDifferentErrorCodesAreSurfacedchecks that:
- Two different validation scenarios (invalid type vs invalid relation) both have the same
apiErrorCode,- But produce distinct
apiErrorMessagevalues.This is a helpful regression test to ensure the SDK does not accidentally collapse distinct backend messages when mapping errors.
354-380: Operation-context in error messages is validated across write and check
testErrorMessageContainsOperationContextverifies that:
getMessage()for write and check operations includes[write]and[check]respectively, andgetOperationName()differs between those two failures.This is a concise, high-level assertion that the operation context is reliably threaded through to the final exception messages.
There was a problem hiding this comment.
Pull Request Overview
This PR improves error messaging in the OpenFGA Java SDK by enhancing how error details are surfaced to developers. The key improvement is that error information (operation name, API error message, and error code) is now formatted into the exception message itself, making errors more discoverable without requiring developers to dig through stack traces.
- Adds structured error message formatting with format
[operationName] apiErrorMessage (apiErrorCode) - Extracts and parses API error responses to populate error details (code and message)
- Adds comprehensive integration tests to verify error messaging across different operations
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| src/main/java/dev/openfga/sdk/errors/FgaError.java | Adds JSON parsing of API error responses, new fields for operation name and API error message, and overrides getMessage() to format error details |
| src/test-integration/java/dev/openfga/sdk/errors/FgaErrorIntegrationTest.java | Adds comprehensive integration tests covering various error scenarios (validation errors, not found errors, different operations) to verify error message formatting |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/test-integration/java/dev/openfga/sdk/errors/FgaErrorIntegrationTest.java
Outdated
Show resolved
Hide resolved
src/test-integration/java/dev/openfga/sdk/errors/FgaErrorIntegrationTest.java
Outdated
Show resolved
Hide resolved
| error.setApiErrorMessage(resp.message); | ||
| } catch (JsonProcessingException e) { | ||
| // Fall back, do nothing - log the exception for debugging | ||
| System.err.println("Failed to parse API error response JSON: " + e.getMessage()); |
There was a problem hiding this comment.
Using System.err.println() for error logging in production code is not a best practice. Consider using a proper logging framework (like SLF4J) or, if intentional for debugging purposes, consider whether this should be silently swallowed instead. Library code should avoid writing directly to stderr as it interferes with application-level logging configuration.
There was a problem hiding this comment.
I think it's not a good idea to introduce a logging framework dependency within the SDK
I wonder if we should even log this but we shouldn't swallow exceptions either
@jimmyjames Would like your thoughts
There was a problem hiding this comment.
We should add a logger but not with this PR
There was a problem hiding this comment.
Is there any problem with silently swallowing the error given that we should fallback to what the current behaviour is?
There was a problem hiding this comment.
Is there any problem with silently swallowing the error given that we should fallback to what the current behaviour is?
There isn't but I was thinking more from a debugging perspective, But I guess it's not really required to log to do that when we need to.
src/test-integration/java/dev/openfga/sdk/errors/FgaErrorIntegrationTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
| // Fall back, do nothing - log the exception for debugging | ||
| System.err.println("Failed to parse API error response JSON: " + e.getMessage()); |
There was a problem hiding this comment.
Using System.err.println() for error logging is not suitable for production code. Consider using a proper logging framework (e.g., SLF4J) or removing the statement entirely if silent fallback is acceptable. Console output can interfere with application logs and is not configurable.
src/test-integration/java/dev/openfga/sdk/errors/FgaErrorIntegrationTest.java
Show resolved
Hide resolved
Co-authored-by: Copilot <[email protected]>
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
Tests