feat: use common streaming utils, reverse sync to template #261
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 generic streaming API infrastructure for the OpenFGA Java SDK. It adds a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Areas requiring extra attention:
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
|
Related Documentation Checked 6 published document(s) in 1 knowledge base(s). No updates required. How did I do? Any feedback? |
Codecov Report❌ Patch coverage is ❌ Your project status has failed because the head coverage (36.81%) 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 #261 +/- ##
============================================
- Coverage 36.89% 36.81% -0.09%
- Complexity 1187 1191 +4
============================================
Files 192 194 +2
Lines 7410 7440 +30
Branches 854 856 +2
============================================
+ Hits 2734 2739 +5
- Misses 4553 4578 +25
Partials 123 123 ☔ 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/test/java/dev/openfga/sdk/api/StreamingApiTest.java (3)
51-65: Consider closingMockitoAnnotations.openMocks()to prevent resource leak.
MockitoAnnotations.openMocks(this)returns anAutoCloseablethat should be closed after tests complete to release mock resources. Consider using@ExtendWith(MockitoExtension.class)instead, which handles lifecycle automatically.+import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.junit.jupiter.MockitoExtension; ... +@ExtendWith(MockitoExtension.class) class StreamingApiTest { ... + private AutoCloseable mockitoCloseable; + @BeforeEach void setUp() { - MockitoAnnotations.openMocks(this); + mockitoCloseable = MockitoAnnotations.openMocks(this); ... + @AfterEach + void tearDown() throws Exception { + mockitoCloseable.close(); + }Alternatively, simply use
@ExtendWith(MockitoExtension.class)and remove the manualopenMockscall entirely.
69-72: Variable name should follow camelCase convention.
Responseshould beresponseto follow Java naming conventions. Same issue appears at line 106.- String Response = "{\"result\":{\"object\":\"document:1\"}}\n" + String response = "{\"result\":{\"object\":\"document:1\"}}\n" + "{\"result\":{\"object\":\"document:2\"}}\n" + "{\"result\":{\"object\":\"document:3\"}}\n"; - Stream<String> lineStream = Response.lines(); + Stream<String> lineStream = response.lines();
235-253: Consider strengthening deserialization assertions.The test only verifies
assertNotNullbut doesn't validate the deserialized values. Consider verifying thatresultanderrorfields are correctly populated.Object streamResult = mapper.readValue(jsonWithResult, resultType); assertNotNull(streamResult); + @SuppressWarnings("unchecked") + var typedResult = (dev.openfga.sdk.api.model.StreamResult<StreamedListObjectsResponse>) streamResult; + assertNotNull(typedResult.getResult()); + assertEquals("document:1", typedResult.getResult().getObject()); + assertNull(typedResult.getError()); // Test with error - code should be an integer String jsonWithError = "{\"error\":{\"code\":400,\"message\":\"Error occurred\"}}"; Object streamResultWithError = mapper.readValue(jsonWithError, resultType); assertNotNull(streamResultWithError); + @SuppressWarnings("unchecked") + var typedError = (dev.openfga.sdk.api.model.StreamResult<StreamedListObjectsResponse>) streamResultWithError; + assertNotNull(typedError.getError()); + assertNull(typedError.getResult());src/main/java/dev/openfga/sdk/api/StreamedListObjectsApi.java (1)
143-144: Redundant.toString()call on String parameter.
storeIdis already aString, so.toString()is unnecessary.String path = "/stores/{store_id}/streamed-list-objects" - .replace("{store_id}", StringUtil.urlEncode(storeId.toString())); + .replace("{store_id}", StringUtil.urlEncode(storeId));src/main/java/dev/openfga/sdk/api/BaseStreamingApi.java (3)
86-95: Stream processing blocks the HTTP client executor thread.The
lines.forEach(...)call is synchronous and will block until all lines are consumed. For long-running streams, this ties up a thread from the HTTP client's executor pool for the entire duration. This is an acceptable trade-off for simplicity, but consider documenting this behavior for users who may want to understand resource utilization.
140-146: Redundant null check.The
result != nullcheck on line 143 is redundant sincestreamResult.getResult() != nullwas already verified on line 140.} else if (streamResult.getResult() != null) { // Deliver the response object to the consumer T result = streamResult.getResult(); - if (result != null) { - consumer.accept(result); - } + consumer.accept(result); }
147-151: Exceptions are silently swallowed if errorConsumer is null.When
errorConsumeris null, any parsing exceptions are caught and discarded without logging or propagation. This could make debugging difficult when malformed lines appear in the stream.Consider logging the exception at debug/trace level even when no errorConsumer is provided:
} catch (Exception e) { if (errorConsumer != null) { errorConsumer.accept(e); + } else { + // Log at debug level to aid troubleshooting + // logger.debug("Failed to process stream line", e); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.openapi-generator/FILES(2 hunks)examples/streamed-list-objects/Makefile(1 hunks)src/main/java/dev/openfga/sdk/api/BaseStreamingApi.java(1 hunks)src/main/java/dev/openfga/sdk/api/StreamedListObjectsApi.java(2 hunks)src/main/java/dev/openfga/sdk/api/model/StreamResult.java(1 hunks)src/test/java/dev/openfga/sdk/api/StreamingApiTest.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/main/java/dev/openfga/sdk/api/StreamedListObjectsApi.java (3)
src/main/java/dev/openfga/sdk/util/Validation.java (1)
Validation(7-15)src/main/java/dev/openfga/sdk/errors/FgaInvalidParameterException.java (1)
FgaInvalidParameterException(3-19)src/main/java/dev/openfga/sdk/util/StringUtil.java (1)
StringUtil(12-51)
⏰ 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). (6)
- GitHub Check: Agent
- GitHub Check: CodeQL analysis (java)
- GitHub Check: Test and Build OpenFGA (21)
- GitHub Check: Test and Build OpenFGA (17)
- GitHub Check: Test and Build OpenFGA (11)
- GitHub Check: Analyze (java)
🔇 Additional comments (10)
examples/streamed-list-objects/Makefile (1)
8-12: LGTM!The Gradle wrapper path update to
../../gradlewcorrectly aligns with the repository structure where examples reside in a subdirectory..openapi-generator/FILES (2)
100-102: LGTM!The FILES manifest correctly registers the new streaming infrastructure components (
BaseStreamingApi.java, updatedStreamedListObjectsApi.java).
162-162: LGTM!
StreamResult.javais properly registered in the generated files manifest.src/main/java/dev/openfga/sdk/api/model/StreamResult.java (2)
28-40: LGTM - Clean generic wrapper design.The
StreamResult<T>model correctly handles the streaming response pattern where each line contains either aresultor anerror. The generic type parameter enables reuse across different streaming endpoints.
81-96: LGTM!The
equalsandhashCodeimplementations correctly handle the generic type with wildcard casting and useObjects.equals/Objects.hashfor null-safe comparisons.src/main/java/dev/openfga/sdk/api/StreamedListObjectsApi.java (2)
36-40: LGTM - Good refactoring to generic base class.The class now correctly extends
BaseStreamingApi<StreamedListObjectsResponse>and properly wires theTypeReferencefor deserialization. This enables code reuse for other streaming endpoints.
146-154: LGTM - Error handling is appropriate.The try-catch properly notifies the error consumer before returning a failed future, ensuring both error propagation paths are covered.
src/main/java/dev/openfga/sdk/api/BaseStreamingApi.java (3)
38-57: LGTM!Clean base class design with proper encapsulation. The use of
TypeReferencefor generic deserialization and final fields for immutability are appropriate patterns.
97-116: Dual error reporting: errors are delivered to both errorConsumer and the CompletableFuture.The current design calls
errorConsumer.accept()AND re-throws the exception, causing theCompletableFutureto complete exceptionally. This means callers who provide both anerrorConsumerand handle.exceptionally()or.whenComplete()on the returned future will observe the same error twice.If this is intentional, consider documenting the behavior in the Javadoc. Otherwise, you may want to choose one reporting mechanism.
165-181: LGTM!Request building logic is clean with proper exception wrapping and interceptor support.
There was a problem hiding this comment.
Pull request overview
This PR introduces a generic streaming utility framework to the OpenFGA Java SDK. It refactors the existing StreamedListObjectsApi implementation to use a new BaseStreamingApi<T> base class, which provides reusable streaming functionality for any endpoint that returns line-delimited JSON (NDJSON) format. The changes include a new generic StreamResult<T> model class to replace type-specific stream result models, comprehensive test coverage, and a minor fix to example Makefile paths.
Key Changes
- Introduces
BaseStreamingApi<T>as a reusable base class for streaming endpoints with consumer-callback pattern - Adds generic
StreamResult<T>model to handle streaming responses uniformly across different endpoint types - Refactors
StreamedListObjectsApito extendBaseStreamingApiinstead of implementing streaming logic directly
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/main/java/dev/openfga/sdk/api/BaseStreamingApi.java |
New abstract base class providing generic streaming implementation with async HTTP client, line-by-line processing, and error handling |
src/main/java/dev/openfga/sdk/api/model/StreamResult.java |
New generic wrapper model for streaming results that can contain either a result or error |
src/main/java/dev/openfga/sdk/api/StreamedListObjectsApi.java |
Refactored to extend BaseStreamingApi<StreamedListObjectsResponse>, removing duplicate streaming logic |
src/test/java/dev/openfga/sdk/api/StreamingApiTest.java |
New comprehensive test suite covering successful streams, errors in streams, HTTP errors, empty streams, and large streams |
examples/streamed-list-objects/Makefile |
Fixed gradle wrapper paths from ./gradlew to ../../gradlew |
.openapi-generator/FILES |
Updated to include new source files |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Generated from openfga/sdk-generator#668
Description
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
Tests
✏️ Tip: You can customize this high-level summary in your review settings.