chore: refactor HttpMethods to enum #277
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
WalkthroughThe pull request replaces string-based HTTP method parameters with a type-safe HttpMethod enum throughout the codebase. A new HttpMethod enum is introduced with constants for standard HTTP methods (GET, POST, PUT, DELETE, PATCH, HEAD, OPTIONS). The ApiExecutorRequestBuilder factory method and constructor signatures are updated to accept HttpMethod instead of String, and all usages across documentation, examples, tests, and integration tests are updated to pass enum constants. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 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. 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.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 (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
docs/ApiExecutor.md
Outdated
| ApiExecutorRequestBuilder request = ApiExecutorRequestBuilder.builder(HttpMethod.GET, "/stores/{store_id}/feature") | ||
| ApiExecutorRequestBuilder request = ApiExecutorRequestBuilder.builder("GET", "/stores/{store_id}/feature") |
There was a problem hiding this comment.
Missing code block opening marker and incomplete refactoring. The code block starting at line 76 is missing the opening triple backticks (```java). Additionally, line 77 still uses the old String-based method signature ("GET") instead of the new HttpMethod enum, creating a duplicate variable declaration with line 76.
docs/ApiExecutor.md
Outdated
| ApiExecutorRequestBuilder request = ApiExecutorRequestBuilder.builder(HttpMethod.POST, "/stores/{store_id}/bulk-delete") | ||
| ApiExecutorRequestBuilder request = ApiExecutorRequestBuilder.builder("POST", "/stores/{store_id}/bulk-delete") |
There was a problem hiding this comment.
Missing code block opening marker and incomplete refactoring. The code block starting at line 86 is missing the opening triple backticks (```java). Additionally, line 87 still uses the old String-based method signature ("POST") instead of the new HttpMethod enum, creating a duplicate variable declaration with line 86.
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
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.