[Router] Thrift friendly router for vertx #112
Conversation
WalkthroughThis pull request introduces comprehensive changes across multiple files, focusing on enhancing dependency management, build configurations, and introducing new utility classes for API handling. The modifications span build scripts, service commons, and testing infrastructure, with a particular emphasis on improving type-safe REST API interactions and streamlining project configurations. Changes
Sequence DiagramsequenceDiagram
participant Client
participant RouteHandler
participant Transformer
participant Serializer
Client->>RouteHandler: HTTP Request
RouteHandler->>RouteHandler: Deserialize Parameters
RouteHandler->>Transformer: Transform Input
Transformer-->>RouteHandler: Return Output
RouteHandler->>Serializer: Serialize Response
Serializer-->>Client: HTTP Response
Possibly related PRs
Suggested Reviewers
Poem
Warning Review ran into problems🔥 ProblemsGitHub Actions: Resource not accessible by integration - https://docs.github.com/rest/actions/workflow-runs#list-workflow-runs-for-a-repository. Please grant the required permissions to the CodeRabbit GitHub App under the organization or repository settings. Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (5)
router/src/main/java/ai/chronon/router/RouteHandlerWrapper.java (1)
23-62: Use parameterized type references to improve type safetyThe code currently uses
Class<I>to represent the input class, which may lead to type erasure issues with generics.Consider using
TypeReference<I>or similar constructs to retain type information at runtime, enhancing type safety and reducing potential casting errors..github/workflows/test_scala_no_spark.yaml (1)
70-73: Eliminate redundant setting ofSBT_OPTSenvironment variableThe
SBT_OPTSenvironment variable is being set multiple times in different steps. This redundancy can be avoided by setting it once at the job or workflow level.Consider setting
SBT_OPTSin theenvsection of the job:jobs: no_spark_scala_tests: runs-on: ubuntu-latest env: SBT_OPTS: "-Xmx8G -Xms2G" steps: # Existing stepsrouter/src/test/java/ai/chronon/router/test/RouteHandlerWrapperTest.java (3)
145-147: Remove unnecessary debug statementsThe
System.out.printlnstatements used for debugging should be removed to keep the test output clean.Apply this diff:
- if(response.statusCode() != 200) { - System.out.println(response.bodyAsString()); - }
169-171: Remove unnecessary debug statementsThe debug statements in
testAllParameterTypesThriftare unnecessary in the final code and should be removed.Apply this diff:
- if(response.statusCode() != 200) { - System.out.println(response.bodyAsString()); - }
81-182: Refactor tests using parameterized tests to reduce duplicationMultiple test methods perform similar operations with different parameters. Using parameterized tests (e.g., JUnit 5's
@ParameterizedTest) can improve maintainability and reduce code duplication.Example:
@ParameterizedTest @MethodSource("provideTestInputs") void testParameterMapping(TestInput input, VertxTestContext testContext) { // Test logic using the input }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
.github/workflows/test_scala_no_spark.yaml(1 hunks)build.sbt(3 hunks)online/src/main/scala/ai/chronon/online/stats/DriftStore.scala(0 hunks)router/src/main/java/ai/chronon/router/RouteHandlerWrapper.java(1 hunks)router/src/test/java/ai/chronon/router/test/RouteHandlerWrapperTest.java(1 hunks)router/src/test/java/ai/chronon/router/test/TestInput.java(1 hunks)
💤 Files with no reviewable changes (1)
- online/src/main/scala/ai/chronon/online/stats/DriftStore.scala
🔇 Additional comments (1)
router/src/test/java/ai/chronon/router/test/TestInput.java (1)
4-26: Class structure is suitable for testing purposes
The TestInput class is well-defined with appropriate getters and setters, facilitating effective testing of parameter mapping.
router/src/main/java/ai/chronon/router/RouteHandlerWrapper.java
Outdated
Show resolved
Hide resolved
router/src/main/java/ai/chronon/router/RouteHandlerWrapper.java
Outdated
Show resolved
Hide resolved
router/src/main/java/ai/chronon/router/RouteHandlerWrapper.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (7)
router/src/main/java/ai/chronon/router/RouteHandlerWrapper.java (1)
49-51: Specify a more precise content typeThe content type should be more specific for JSON responses.
ctx.response() - .putHeader("content-type", "application/json") + .putHeader("content-type", "application/json; charset=utf-8") .end(JsonObject.mapFrom(output).encode());router/src/test/java/ai/chronon/router/test/TestInput.java (1)
5-34: Consider enhancing the test input classThe test input class could be improved for better testing support:
- Add validation in setters
- Implement builder pattern for easier test setup
- Add toString() for better test failure messages
public class TestInput { private String userId; private OrderStatus status; private UserRole role; private Accuracy accuracy; private int limit; private double amount; private boolean active; + public static class Builder { + private TestInput instance = new TestInput(); + + public Builder withUserId(String userId) { + instance.setUserId(userId); + return this; + } + // Add other builder methods + + public TestInput build() { + return instance; + } + } + + public static Builder builder() { + return new Builder(); + } public void setUserId(String userId) { + if (userId != null && userId.trim().isEmpty()) { + throw new IllegalArgumentException("userId cannot be empty"); + } this.userId = userId; } + @Override + public String toString() { + return String.format("TestInput(userId=%s, status=%s, ...)", userId, status); + }router/src/test/java/ai/chronon/router/test/RouteHandlerWrapperTest.java (4)
23-34: Consider making TestOutput class immutableThe fields can be made final since they are only set in the constructor.
class TestOutput { - private String userId; - private String summary; + private final String userId; + private final String summary;
40-40: Consider making the test port configurableThe hardcoded port number could cause conflicts in CI environments. Consider making it configurable through system properties or test configuration.
- private int testPort = 8888; + private final int testPort = Integer.getInteger("test.server.port", 8888);
209-211: Extract common error logging logicThe error logging pattern is duplicated across multiple test methods. Consider extracting it into a helper method.
+ private void logErrorIfFailed(io.vertx.ext.web.client.HttpResponse<?> response) { + if(response.statusCode() != 200) { + System.out.println(response.bodyAsString()); + } + }Then use it in the test methods:
- if(response.statusCode() != 200) { - System.out.println(response.bodyAsString()); - } + logErrorIfFailed(response);Also applies to: 233-235, 254-256
191-194: Use more specific assertions for default valuesThe assertions for default values could be more specific about the expected behavior.
Add comments explaining the default value expectations:
+ // Verify that optional parameters get appropriate default values: + // - Enums default to null + // - Numeric types default to 0 + // - Booleans default to false assertTrue(result.getString("summary").contains("Status: null")); assertTrue(result.getString("summary").contains("Limit: 0")); assertTrue(result.getString("summary").contains("Amount: 0.00")); assertTrue(result.getString("summary").contains("Active: false"));build.sbt (1)
289-296: Consider defining Vert.x version as a variableThe Vert.x version is repeated multiple times. Consider defining it as a variable like other versions.
+lazy val vertxVersion = "4.5.9" + lazy val router = project .dependsOn(api.%("compile->compile;test->test")) .settings( libraryDependencies ++= Seq( - "io.vertx" % "vertx-core" % "4.5.9", - "io.vertx" % "vertx-web" % "4.5.9", - "io.vertx" % "vertx-web-client" % "4.5.9", - "io.vertx" % "vertx-junit5" % "4.5.9", + "io.vertx" % "vertx-core" % vertxVersion, + "io.vertx" % "vertx-web" % vertxVersion, + "io.vertx" % "vertx-web-client" % vertxVersion, + "io.vertx" % "vertx-junit5" % vertxVersion,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
build.sbt(3 hunks)router/src/main/java/ai/chronon/router/RouteHandlerWrapper.java(1 hunks)router/src/test/java/ai/chronon/router/test/OrderStatus.java(1 hunks)router/src/test/java/ai/chronon/router/test/RouteHandlerWrapperTest.java(1 hunks)router/src/test/java/ai/chronon/router/test/TestInput.java(1 hunks)router/src/test/java/ai/chronon/router/test/UserRole.java(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- router/src/test/java/ai/chronon/router/test/UserRole.java
🔇 Additional comments (6)
router/src/main/java/ai/chronon/router/RouteHandlerWrapper.java (3)
53-60:
Improve error handling and logging
The current error handling exposes internal error messages to clients and lacks proper logging.
Apply this diff to enhance error handling:
} catch (Exception e) {
+ // Log the full exception for debugging
+ logger.error("Error processing request", e);
ctx.response()
.setStatusCode(400)
.putHeader("content-type", "application/json")
.end(new JsonObject()
- .put("error", e.getMessage())
+ .put("error", "Invalid request parameters")
+ .put("code", "BAD_REQUEST")
.encode());
}64-87:
Add input validation and sanitization
The current implementation lacks proper input validation and sanitization.
Consider implementing:
- Parameter name validation against a whitelist
- Input sanitization to prevent injection
- Maximum parameter count check
private static <I> I createInputFromParams(Map<String, String> params, Class<I> inputClass)
throws Exception {
+ // Validate parameter count
+ if (params.size() > MAX_PARAMS) {
+ throw new IllegalArgumentException("Too many parameters");
+ }
+
+ // Validate parameter names
+ Set<String> validFields = Arrays.stream(inputClass.getDeclaredFields())
+ .map(Field::getName)
+ .collect(Collectors.toSet());
+
+ for (String param : params.keySet()) {
+ if (!validFields.contains(param)) {
+ throw new IllegalArgumentException("Invalid parameter: " + param);
+ }
+ }
I input = inputClass.getDeclaredConstructor().newInstance();102-135: 🛠️ Refactor suggestion
Enhance type conversion robustness
The current type conversion implementation could be improved in several ways:
- Add support for additional types (e.g., BigDecimal, LocalDateTime)
- Implement stricter validation for numeric types
- Add bounds checking for numeric values
private static Object convertValue(String value, Class<?> targetType) {
+ if (value == null) {
+ throw new IllegalArgumentException("Value cannot be null");
+ }
if (targetType == String.class) {
return value;
} else if (targetType == Integer.class || targetType == int.class) {
+ try {
return Integer.parseInt(value);
+ } catch (NumberFormatException e) {
+ throw new IllegalArgumentException(
+ String.format("Invalid integer value: %s", value));
+ }
}
+ // Add support for BigDecimal
+ else if (targetType == BigDecimal.class) {
+ try {
+ return new BigDecimal(value);
+ } catch (NumberFormatException e) {
+ throw new IllegalArgumentException(
+ String.format("Invalid decimal value: %s", value));
+ }
+ }Likely invalid or redundant comment.
router/src/test/java/ai/chronon/router/test/RouteHandlerWrapperTest.java (1)
63-66: Add validation in thrift transformers
The thrift transformers (thriftTransformer and windowTransformer) directly return input without validation. Consider adding input validation to ensure data integrity.
build.sbt (2)
294-294: Ensure consistent Jackson library versions across projects
The Jackson version inconsistency could lead to runtime conflicts.
297-302: Conditionally include platform-specific dependencies
The macOS-specific Netty dependency is correctly handled conditionally.
router/src/main/java/ai/chronon/router/RouteHandlerWrapper.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
router/src/test/java/ai/chronon/router/test/RouteHandlerWrapperTest.java (4)
44-46: Use dynamic port allocation to prevent port conflictsUsing a fixed port (
testPort = 8888) may cause port conflicts if multiple instances of the tests run simultaneously or if the port is already in use. Consider using port0to let the system assign an available port dynamically.Apply this diff to modify the port allocation:
- private int testPort = 8888; + private int testPort; @BeforeEach void setUp(Vertx vertx, VertxTestContext testContext) { - client = WebClient.create(vertx, new WebClientOptions().setDefaultPort(testPort)); + // Client will be initialized after the server starts ... // Start server vertx.createHttpServer() .requestHandler(router) - .listen(testPort) + .listen(0) .onComplete(testContext.succeeding(server -> { + testPort = server.actualPort(); + client = WebClient.create(vertx, new WebClientOptions().setDefaultPort(testPort)); testContext.completeNow(); })); }Also applies to: 87-90
98-275: Refactor repetitive test code into helper methodsSeveral test methods have duplicate code for setting up HTTP requests and verifying responses. Refactoring common code into helper methods will improve maintainability and reduce redundancy.
For example, create helper methods for:
- Building requests with common parameters.
- Asserting response status codes and body contents.
219-221: Remove unnecessary debug statements from testsThe
System.out.println(response.bodyAsString());statement is used for debugging but should be removed or replaced with proper logging.Apply this diff to clean up the test code:
if(response.statusCode() != 200) { - System.out.println(response.bodyAsString()); }
110-117: Simplify JSON assertions using dedicated librariesConsider using JSON assertion libraries like
JSONAssertorAssertJto make JSON response assertions more concise and expressive.This can improve test readability and maintainability.
Also applies to: 146-151, 224-231
build.sbt (1)
286-303: Define version variables for Vert.x and Jackson dependenciesTo ensure consistency and ease future updates, define version variables for Vert.x and Jackson dependencies.
Apply this diff to add version variables and update the dependencies:
+lazy val vertxVersion = "4.5.9" +lazy val jacksonVersion = "2.15.2" lazy val router = project .dependsOn(api.%("compile->compile;test->test")) .settings( libraryDependencies ++= Seq( - "io.vertx" % "vertx-core" % "4.5.9", - "io.vertx" % "vertx-web" % "4.5.9", - "io.vertx" % "vertx-web-client" % "4.5.9", - "io.vertx" % "vertx-junit5" % "4.5.9", - "com.fasterxml.jackson.core" % "jackson-databind" % "2.15.2", // pinned from elsewhere + "io.vertx" % "vertx-core" % vertxVersion, + "io.vertx" % "vertx-web" % vertxVersion, + "io.vertx" % "vertx-web-client" % vertxVersion, + "io.vertx" % "vertx-junit5" % vertxVersion, + "com.fasterxml.jackson.core" % "jackson-databind" % jacksonVersion, // pinned from elsewhere "org.junit.jupiter" % "junit-jupiter-api" % "5.10.5" % Test ), libraryDependencies ++= {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
.github/workflows/test_scala_no_spark.yaml(1 hunks)build.sbt(3 hunks)online/src/main/scala/ai/chronon/online/stats/DriftStore.scala(0 hunks)router/src/main/java/ai/chronon/router/RouteHandlerWrapper.java(1 hunks)router/src/test/java/ai/chronon/router/test/OrderStatus.java(1 hunks)router/src/test/java/ai/chronon/router/test/RouteHandlerWrapperTest.java(1 hunks)router/src/test/java/ai/chronon/router/test/TestInput.java(1 hunks)router/src/test/java/ai/chronon/router/test/UserRole.java(1 hunks)
💤 Files with no reviewable changes (1)
- online/src/main/scala/ai/chronon/online/stats/DriftStore.scala
🚧 Files skipped from review as they are similar to previous changes (3)
- router/src/test/java/ai/chronon/router/test/UserRole.java
- router/src/test/java/ai/chronon/router/test/OrderStatus.java
- .github/workflows/test_scala_no_spark.yaml
🔇 Additional comments (5)
router/src/test/java/ai/chronon/router/test/TestInput.java (1)
8-42: LGTM!
The TestInput class is well-defined with appropriate getters and setters for all fields, facilitating flexible test scenarios.
router/src/main/java/ai/chronon/router/RouteHandlerWrapper.java (3)
102-167: Enhance convertValue method to support additional data types
The convertValue method currently supports a limited set of data types. Consider extending it to handle additional common types such as BigDecimal, UUID, and date/time types.
This will increase the versatility of the handler when mapping request parameters to input objects.
53-60:
Avoid exposing internal error details in HTTP responses
Returning e.getMessage() directly in the HTTP response may expose sensitive internal information to the client. Replace it with a generic error message and log the exception on the server side.
Apply this diff to fix the issue:
ctx.response()
.setStatusCode(400)
.putHeader("content-type", "application/json")
.end(new JsonObject()
- .put("error", e.getMessage())
+ .put("error", "An unexpected error occurred.")
.encode());
+ // Log the exception details
+ e.printStackTrace();64-87:
Validate and sanitize input parameters to prevent security risks
Using reflection to set properties from user input can introduce security vulnerabilities, such as setting unintended or sensitive fields. Implement validation to ensure that only expected fields are set.
Consider:
- Whitelisting allowed field names.
- Checking for specific annotations on setter methods.
- Ignoring or rejecting unexpected parameters.
build.sbt (1)
294-295:
Ensure consistent Jackson library versions across projects
The jackson-databind dependency in the router project is specified as version 2.15.2. Ensure that this version matches the Jackson version used in other projects to avoid runtime conflicts.
Consider defining a global jacksonVersion variable and using it across all projects.
router/src/test/java/ai/chronon/router/test/RouteHandlerWrapperTest.java
Show resolved
Hide resolved
router/src/main/java/ai/chronon/router/RouteHandlerWrapper.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
router/src/test/java/ai/chronon/router/test/RouteHandlerWrapperTest.java (5)
41-41: Consider making the test port configurableThe hardcoded test port (8888) could cause conflicts with other services during testing. Consider making it configurable through system properties or test configuration.
- private int testPort = 8888; + private int testPort = Integer.getInteger("test.server.port", 8888);
53-71: Extract transformer function to improve readabilityThe transformer function is complex and could be extracted into a separate method for better maintainability and readability.
+ private TestOutput transformTestInput(TestInput input) { + return new TestOutput( + input.getUserId(), + String.format("Status: %s, Role: %s, Accuracy: %s, Limit: %d, Amount: %.2f, Active: %b, Items: %s, Props: %s", + input.getStatus(), + input.getRole(), + input.getAccuracy(), + input.getLimit(), + input.getAmount(), + input.isActive(), + input.getItems() != null ? String.join(",", input.getItems()) : "null", + input.getProps() != null ? + input.getProps().entrySet() + .stream() + .map(entry -> entry.getKey() + ":" + entry.getValue()) + .collect(Collectors.joining(",")) + : "null" + ) + ); + } + - Function<TestInput, TestOutput> transformer = input -> - new TestOutput( - input.getUserId(), - String.format("Status: %s, Role: %s, Accuracy: %s, Limit: %d, Amount: %.2f, Active: %b, Items: %s, Props: %s", - input.getStatus(), - input.getRole(), - input.getAccuracy(), - input.getLimit(), - input.getAmount(), - input.isActive(), - input.getItems() != null ? String.join(",", input.getItems()) : "null", - input.getProps() != null ? - input.getProps().entrySet() - .stream() - .map(entry -> entry.getKey() + ":" + entry.getValue()) - .collect(Collectors.joining(",")) - : "null" - ) - ); + Function<TestInput, TestOutput> transformer = this::transformTestInput;
225-227: Extract duplicate error logging codeThe error logging code is duplicated across multiple test methods. Consider extracting it into a helper method.
+ private void logErrorResponse(HttpResponse<Buffer> response) { + if(response.statusCode() != 200) { + System.out.println(response.bodyAsString()); + } + } + - if(response.statusCode() != 200) { - System.out.println(response.bodyAsString()); - } + logErrorResponse(response);Also applies to: 249-251, 270-272
233-236: Consider using more specific assertionsThe current assertions using
containsare somewhat loose. Consider using more specific assertions to verify the exact format and values.- assertTrue(summary.contains("Status: PROCESSING")); - assertTrue(summary.contains("Limit: 5")); - assertTrue(summary.contains("Amount: 123.45")); - assertTrue(summary.contains("Active: true")); + assertEquals("Status: PROCESSING, Role: null, Accuracy: null, Limit: 5, Amount: 123.45, Active: true, Items: null, Props: null", + summary);
37-38: Add class-level documentationThe test class lacks JavaDoc documentation describing its purpose and the components being tested.
+/** + * Unit tests for RouteHandlerWrapper class. + * Tests the functionality of parameter mapping and transformation + * for both POJO and Thrift objects in HTTP requests. + */ @ExtendWith(VertxExtension.class) class RouteHandlerWrapperTest {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
router/src/main/java/ai/chronon/router/RouteHandlerWrapper.java(1 hunks)router/src/test/java/ai/chronon/router/test/RouteHandlerWrapperTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- router/src/main/java/ai/chronon/router/RouteHandlerWrapper.java
🔇 Additional comments (1)
router/src/test/java/ai/chronon/router/test/RouteHandlerWrapperTest.java (1)
191-192: Add null check for response body
Accessing the error message without checking if the response body is null may lead to a NullPointerException.
JsonObject error = response.bodyAsJsonObject();
+ assertNotNull(error, "Response body is null");
assertTrue(error.getString("error").contains("not-a-number"));
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
router/src/test/java/ai/chronon/router/test/RouteHandlerWrapperTest.java (3)
41-42: Consider using a dynamic port for the test serverDefining a fixed port (
8888) may lead to port conflicts if the port is already in use. Consider using an ephemeral port or retrying with different ports to avoid potential issues during the test execution.
49-67: Extract complex string formatting into a separate method for readabilityThe
transformerfunction contains a complexString.formatwith nested ternary operators and streams, which can impact readability. Extracting this logic into a separate method will improve maintainability and make the code easier to understand.Apply this refactor:
// Add a method to handle summary creation private String createSummary(TestInput input) { return String.format( "Status: %s, Role: %s, Accuracy: %s, Limit: %d, Amount: %.2f, Active: %b, Items: %s, Props: %s", input.getStatus(), input.getRole(), input.getAccuracy(), input.getLimit(), input.getAmount(), input.isActive(), input.getItems() != null ? String.join(",", input.getItems()) : "null", input.getProps() != null ? input.getProps().entrySet() .stream() .map(entry -> entry.getKey() + ":" + entry.getValue()) .collect(Collectors.joining(",")) : "null" ); } // Update the transformer function Function<TestInput, TestOutput> transformer = input -> new TestOutput(input.getUserId(), createSummary(input));
219-221: Use assertions instead of printing to System.outIn your test methods, instead of printing the response body when the status code is not
200, consider using assertion messages to provide more informative feedback upon failure. This approach integrates better with testing frameworks and avoids cluttering the console output.Apply this diff to update the assertions:
- if(response.statusCode() != 200) { - System.out.println(response.bodyAsString()); - } - assertEquals(200, response.statusCode()); + assertEquals(200, response.statusCode(), "Unexpected status code: " + response.statusCode() + ", Response body: " + response.bodyAsString());Also applies to: 243-245, 264-266
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
router/src/test/java/ai/chronon/router/test/RouteHandlerWrapperTest.java(1 hunks)
🔇 Additional comments (2)
router/src/test/java/ai/chronon/router/test/RouteHandlerWrapperTest.java (2)
129-130: Add null check to prevent potential NullPointerException
In testInvalidEnumValue, accessing the error message without checking if the response body is null may lead to a NullPointerException if the response does not contain a valid JSON body.
Apply this diff to add a null check:
JsonObject error = response.bodyAsJsonObject();
+assertNotNull(error, "Response body is null");
assertTrue(error.getString("error").contains("Invalid enum value"));185-186: Add null check to prevent potential NullPointerException
In testInvalidNumberParameter, accessing the error message without checking if the response body is null may lead to a NullPointerException if the response does not contain a valid JSON body.
Apply this diff to add a null check:
JsonObject error = response.bodyAsJsonObject();
+assertNotNull(error, "Response body is null");
assertTrue(error.getString("error").contains("not-a-number"));
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
router/src/main/java/ai/chronon/router/RouteHandlerWrapper.java (2)
22-22: Track the TODO for Thrift BinaryProtocol serialization supportThe TODO indicates missing support for Thrift BinaryProtocol serialization, which seems important given the PR's objective of creating a "Thrift friendly router".
Would you like me to help create a GitHub issue to track this TODO and propose an implementation approach?
116-181: Improve documentation and extensibility of type conversionThe
convertValuemethod handles various types but could benefit from:
- Detailed documentation of supported types and formats
- Extension points for custom type conversions
- Clear error messages for unsupported types
Consider adding:
- Javadoc explaining supported types and formats
- A mechanism for registering custom type converters
+/** + * Converts string values to target types. + * Supported types: + * - Primitive types and their wrappers + * - Enums (with optional fromString method) + * - Lists (comma-separated values) + * - Maps (comma-separated key:value pairs) + * + * @param value The string value to convert + * @param targetType The target type to convert to + * @return The converted value + * @throws IllegalArgumentException if conversion fails or type is unsupported + */ private static Object convertValue(String value, Type targetType) { + // Try custom converters first + Object result = CustomConverterRegistry.convert(value, targetType); + if (result != null) { + return result; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
router/src/main/java/ai/chronon/router/RouteHandlerWrapper.java(1 hunks)
🔇 Additional comments (3)
router/src/main/java/ai/chronon/router/RouteHandlerWrapper.java (3)
60-74: Avoid exposing internal error details in HTTP responses
Returning stack traces directly in the HTTP response exposes sensitive internal information to the client.
136-143: Handle potential null values when invoking custom fromString
The null check logic for custom enum conversion needs improvement.
78-101:
Enhance security for reflection-based field setting
The current implementation allows setting any field that has a public setter, which could be a security risk. Consider:
- Implementing a whitelist of allowed fields
- Adding annotations to mark fields as injectable
- Validating field access permissions
Here's a suggested improvement:
+@Target(ElementType.METHOD)
+@Retention(RetentionPolicy.RUNTIME)
+public @interface Injectable {}
private static <I> I createInputFromParams(Map<String, String> params, Class<I> inputClass)
throws Exception {
I input = inputClass.getDeclaredConstructor().newInstance();
Method[] methods = inputClass.getMethods();
for (Method method : methods) {
- if (isSetter(method)) {
+ if (isSetter(method) && method.isAnnotationPresent(Injectable.class)) {
String fieldName = getFieldNameFromSetter(method);
+ if (!ALLOWED_FIELDS.contains(fieldName)) {
+ continue;
+ }Likely invalid or redundant comment.
| Map<String, String> params = new HashMap<>(); | ||
|
|
||
| // Add path parameters | ||
| params.putAll(ctx.pathParams()); | ||
|
|
||
| // Add query parameters | ||
| for (Map.Entry<String, String> entry : ctx.queryParams().entries()) { | ||
| params.put(entry.getKey(), entry.getValue()); | ||
| } |
There was a problem hiding this comment.
Add input validation for request parameters
The current implementation accepts all parameters without validation. Consider:
- Adding size limits for parameter values
- Validating parameter names against expected patterns
- Implementing rate limiting for requests
Here's a suggested improvement:
Map<String, String> params = new HashMap<>();
+// Add validation for parameter count
+if (ctx.queryParams().size() > MAX_PARAMS) {
+ throw new IllegalArgumentException("Too many parameters");
+}
+
+// Validate parameter names and values
+for (Map.Entry<String, String> entry : ctx.queryParams().entries()) {
+ if (!isValidParamName(entry.getKey())) {
+ throw new IllegalArgumentException("Invalid parameter name: " + entry.getKey());
+ }
+ if (entry.getValue().length() > MAX_PARAM_LENGTH) {
+ throw new IllegalArgumentException("Parameter value too long: " + entry.getKey());
+ }
params.put(entry.getKey(), entry.getValue());
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (9)
router/src/test/java/ai/chronon/router/test/RouteHandlerWrapperTest.java (2)
43-92: Consider extracting test configurationThe setUp method contains hardcoded test configuration values. Consider extracting these to constants or configuration:
- Port number (8888)
- Test routes ("/api/users/:userId/test", etc.)
class RouteHandlerWrapperTest { + private static final int TEST_PORT = 8888; + private static final String TEST_USER_ROUTE = "/api/users/:userId/test"; + private static final String TEST_COLUMN_ROUTE = "/api/column/:column/slice/:slice"; + private static final String TEST_WINDOW_ROUTE = "/api/window/units/:timeUnit/"; + private WebClient client;
220-223: Remove debug print statementsDebug print statements should be removed from test code or replaced with proper logging.
- if(response.statusCode() != 200) { - System.out.println(response.bodyAsString()); - }Also applies to: 244-246, 265-267, 286-288
router/README.md (2)
11-12: Fix grammar in the introduction.Add the missing article "the" for better readability.
-This module adds ability to make REST api's type-safe and boilerplate free by mapping requests into thrift objects +This module adds the ability to make REST APIs type-safe and boilerplate free by mapping requests into thrift objects🧰 Tools
🪛 LanguageTool
[uncategorized] ~11-~11: You might be missing the article “the” here.
Context: ...non codebase already. This module adds ability to make REST api's type-safe and boiler...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
31-37: Enhance code examples with more context.The route declaration example would be more helpful with:
- Comments explaining the purpose of each parameter
- Error handling examples
- Response type expectations
Route declaration ```java +// Define a transformer function that processes TileKey objects Function<TileKey, TileKey> thriftTransformer = input -> input; // some dummy function +// Create a route that maps path parameters (:column, :slice) and query parameters to TileKey fields router.get("/thrift_api/column/:column/slice/:slice") - .handler(RouteHandlerWrapper.createHandler(thriftTransformer, TileKey.class)); + .handler(RouteHandlerWrapper.createHandler(thriftTransformer, TileKey.class)) + // Optional: Add error handling + .failureHandler(ctx -> { + ctx.response() + .setStatusCode(400) + .putHeader("content-type", "application/json") + .end(new JsonObject().put("error", "Invalid request").encode()); + });</blockquote></details> <details> <summary>router/src/main/java/ai/chronon/router/RouteHandlerWrapper.java (2)</summary><blockquote> `30-48`: **Consider using try-with-resources for ThreadLocal cleanup.** The ThreadLocal instances could potentially lead to memory leaks if not properly cleaned up. Consider implementing AutoCloseable and using try-with-resources pattern. ```diff +public class RouteHandlerWrapper implements AutoCloseable { private static final ThreadLocal<TSerializer> binarySerializer = ThreadLocal.withInitial(() -> { try { return new TSerializer(new TBinaryProtocol.Factory()); } catch (TTransportException e) { throw new RuntimeException(e); } }); + + @Override + public void close() { + binarySerializer.remove(); + binaryDeSerializer.remove(); + base64Encoder.remove(); + base64Decoder.remove(); + }
23-23: Remove completed TODO comment.The TODO comment about adding Thrift BinaryProtocol support can be removed as it has been implemented.
- * TODO: Add support for Thrift BinaryProtocol based serialization based on a special request query param. + * Supports both JSON and Thrift BinaryProtocol serialization based on the response-content-type header.router/src/main/java/ai/chronon/router/BinaryResponse.java (3)
1-3: Add class-level documentationPlease add Javadoc documentation to describe the purpose of this class, including:
- Its role in handling binary responses
- Expected format of the data (base64-encoded)
- Typical usage scenarios with Thrift serialization
package ai.chronon.router; +/** + * Represents a binary response containing base64-encoded data and its associated content type. + * This class is primarily used for wrapping Thrift-serialized binary data in API responses. + */ public class BinaryResponse {
4-5: Add null checks and content type validationThe fields are properly encapsulated and immutable, but consider adding validation:
- Null checks for both fields
- Content type format validation (MIME type)
public class BinaryResponse { private final String data; private final String contentType; public BinaryResponse(String data, String contentType) { + if (data == null) { + throw new IllegalArgumentException("Data cannot be null"); + } + if (contentType == null || !contentType.matches("[a-zA-Z0-9-]+/[a-zA-Z0-9-+.]+")) { + throw new IllegalArgumentException("Invalid content type format"); + } this.data = data; this.contentType = contentType; }
12-18: Add method documentationThe getter implementations are correct, but please add Javadoc for better API documentation.
+ /** + * Returns the base64-encoded binary data. + * @return the base64-encoded string + */ public String getData() { return data; } + /** + * Returns the MIME content type of the binary data. + * @return the content type string + */ public String getContentType() { return contentType; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
router/img.pngis excluded by!**/*.png
📒 Files selected for processing (5)
build.sbt(3 hunks)router/README.md(1 hunks)router/src/main/java/ai/chronon/router/BinaryResponse.java(1 hunks)router/src/main/java/ai/chronon/router/RouteHandlerWrapper.java(1 hunks)router/src/test/java/ai/chronon/router/test/RouteHandlerWrapperTest.java(1 hunks)
🧰 Additional context used
🪛 LanguageTool
router/README.md
[uncategorized] ~11-~11: You might be missing the article “the” here.
Context: ...non codebase already. This module adds ability to make REST api's type-safe and boiler...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
🔇 Additional comments (7)
build.sbt (2)
56-56: LGTM: Root project aggregation updated correctly
The router project is properly included in the root project's aggregation.
286-304: LGTM: Router project configuration
The router project is well-configured with:
- Proper dependency on the api project
- Essential Vert.x dependencies
- Consistent logging framework version with Spark
- Platform-specific DNS resolver for macOS
router/src/test/java/ai/chronon/router/test/RouteHandlerWrapperTest.java (2)
1-299: LGTM: Comprehensive test coverage
The test suite provides excellent coverage of:
- Parameter mapping
- Enum handling
- Error cases
- Thrift integration
- Content type handling
179-191:
Add null check for response body
The test could fail with NullPointerException if the response body is null.
router/src/main/java/ai/chronon/router/RouteHandlerWrapper.java (2)
109-123:
Avoid exposing stack traces in error responses.
The current error handling exposes internal details through stack traces. This is a security concern as it may reveal sensitive information about the application structure.
- StringWriter sw = new StringWriter();
- PrintWriter pw = new PrintWriter(sw);
- e.printStackTrace(pw);
- String stackTrace = sw.toString();
+ // Log the full stack trace for debugging
+ e.printStackTrace();
ctx.response()
.setStatusCode(400)
.putHeader("content-type", "application/json")
.end(new JsonObject()
- .put("error", stackTrace)
+ .put("error", "Invalid request format")
+ .put("message", e.getMessage())
.encode());71-81:
Add request size validation.
The current implementation accepts all parameters without size validation, which could lead to memory issues with large requests.
return ctx -> {
try {
+ // Validate request size
+ int totalParams = ctx.pathParams().size() + ctx.queryParams().size();
+ if (totalParams > 100) { // Adjust limit as needed
+ throw new IllegalArgumentException("Too many parameters");
+ }
+
// Create map with path parameters
Map<String, String> params = new HashMap<>(ctx.pathParams());Likely invalid or redundant comment.
router/src/main/java/ai/chronon/router/BinaryResponse.java (1)
1-19: Verify usage with Thrift serialization
Let's verify how this class is used in the context of Thrift serialization across the codebase.
✅ Verification successful
Based on the search results, I can now generate the final response:
BinaryResponse is correctly integrated with Thrift serialization
The class is properly used within the Thrift serialization context, specifically in RouteHandlerWrapper.java where it's used to handle binary protocol responses. The implementation shows:
- Integration with
TBinaryProtocolfor Thrift serialization - Proper handling of content types and binary data
- Thread-safe serializer implementation using
ThreadLocal
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for BinaryResponse usage patterns
echo "Checking BinaryResponse usage patterns..."
rg -l "BinaryResponse" --type java
# Look for Thrift serialization related code
echo "Checking Thrift serialization patterns..."
rg "TProtocol|TSerializer|Binary" --type java
# Check for potential content type constants or enums
echo "Checking content type definitions..."
rg "content[_-]?type.*=.*" --type java
Length of output: 32733
router/src/test/java/ai/chronon/router/test/RouteHandlerWrapperTest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
router/README.md (5)
11-12: Minor grammatical improvements needed.Consider these grammatical improvements:
-This module adds ability to make REST api's type-safe and boilerplate free by mapping requests into thrift objects -automagically (via reflection). Developers still have full control over url design - as they did with vert.x before. +This module adds the ability to make REST APIs type-safe and boilerplate-free by mapping requests into Thrift objects +automatically (via reflection). Developers still have full control over URL design - as they did with Vert.x before.🧰 Tools
🪛 LanguageTool
[uncategorized] ~11-~11: You might be missing the article “the” here.
Context: ...non codebase already. This module adds ability to make REST api's type-safe and boiler...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
23-29: Enhance the Thrift definition with field descriptions.Consider adding comments to describe the purpose and constraints of each field in the TileKey struct. This will help developers understand the expected values and usage.
struct TileKey { + // Column identifier for the tile + // Slice identifier within the column + // Custom name for the tile + // Size in milliseconds, must be positive }
34-37: Improve example with a realistic use case.The current example uses a dummy transformer that simply returns the input. Consider providing a more realistic example that demonstrates actual data transformation and error handling.
-Function<TileKey, TileKey> thriftTransformer = input -> input; // some dummy function +// Example: Transform TileKey into a response with validation +Function<TileKey, TileKey> thriftTransformer = input -> { + if (input.sizeMillis != null && input.sizeMillis <= 0) { + throw new IllegalArgumentException("sizeMillis must be positive"); + } + // Perform actual transformation here + return input; +};
40-53: Document error scenarios and status codes.The example only shows the successful response. Consider adding documentation for:
- Error responses and their corresponding HTTP status codes
- Validation failure scenarios
- Missing required parameter handling
Example addition:
Error Responses: - 400 Bad Request: Invalid parameters ```json {"error": "Invalid sizeMillis: must be positive"}
- 404 Not Found: Resource not found
- 500 Internal Server Error: Server-side processing errors
--- `58-59`: **Add a concrete deserialization example.** While the documentation mentions using the `read` method for deserialization, it would be helpful to include a complete example showing how to deserialize the base64 encoded response. Example addition: ```java // Deserialization example String base64Data = response.getJsonObject().getString("data"); byte[] binaryData = Base64.getDecoder().decode(base64Data); TTransport transport = new TMemoryBuffer(binaryData); TProtocol protocol = new TCompactProtocol(transport); TileKey result = new TileKey(); result.read(protocol);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
router/README.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
router/README.md
[uncategorized] ~11-~11: You might be missing the article “the” here.
Context: ...non codebase already. This module adds ability to make REST api's type-safe and boiler...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~75-~75: You might be missing the article “a” here.
Context: ...looks like below. The data key holds base64 encoded string of thrift binary compact...
(AI_EN_LECTOR_MISSING_DETERMINER_A)
🔇 Additional comments (1)
router/README.md (1)
6-6: Verify the image file existence and path.
The image reference img.png appears to be broken. Please ensure the image file exists in the correct location relative to the README.
✅ Verification successful
Image file exists at the correct location
The image file img.png exists in the router/ directory, which is the correct location relative to the README.md file. The image reference in the documentation is properly configured.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the image file exists
fd -t f "img.png" -d 1 router/
Length of output: 45
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (6)
router/README.md (6)
11-12: Minor grammar improvements needed.Consider these improvements for better readability:
-This module adds ability to make REST api's type-safe and boilerplate free by mapping requests into thrift objects -automagically (via reflection). Developers still have full control over url design - as they did with vert.x before. +This module adds the ability to make REST APIs type-safe and boilerplate-free by mapping requests into Thrift objects +automatically (via reflection). Developers still have full control over URL design - as they did with Vert.x before.🧰 Tools
🪛 LanguageTool
[uncategorized] ~11-~11: You might be missing the article “the” here.
Context: ...non codebase already. This module adds ability to make REST api's type-safe and boiler...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
17-17: Consider expanding the function transformation explanation.The transformation from
Func<Input, Output>toFunc<RequestContext, Response>could benefit from additional details about:
- The types of inputs/outputs supported
- How the transformation handles errors
- Any limitations or constraints
23-29: Consider adding field descriptions to the Thrift definition.The
TileKeystruct would benefit from documentation comments explaining:
- The purpose of each field
- Valid value ranges or constraints
- Whether these fields are required in specific contexts
34-37: Consider providing a more realistic example.The current example uses a dummy transformer that simply returns the input. Consider:
- Adding a more practical transformation example
- Showing error handling
- Demonstrating validation logic
42-53: Consider adding error response examples.The documentation would benefit from examples of:
- Invalid parameter handling
- Error response formats
- HTTP status codes for different scenarios
57-59: Consider adding payload size comparisons.The documentation mentions that "thrift over the wire would shrink the payload significantly." Consider:
- Adding concrete examples comparing JSON vs Thrift payload sizes
- Providing performance benchmarks
- Discussing scenarios where this optimization is most beneficial
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
router/README.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
router/README.md
[uncategorized] ~11-~11: You might be missing the article “the” here.
Context: ...non codebase already. This module adds ability to make REST api's type-safe and boiler...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~75-~75: You might be missing the article “a” here.
Context: ...looks like below. The data key holds base64 encoded string of thrift binary compact...
(AI_EN_LECTOR_MISSING_DETERMINER_A)
🔇 Additional comments (1)
router/README.md (1)
6-6: Verify the image reference and consider adding alt text.
The image reference img.png might be broken or missing. Additionally, consider adding meaningful alt text for better accessibility.
✅ Verification successful
Image file exists, but alt text is still recommended
Consider adding meaningful alt text to improve accessibility. Example: 
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the referenced image exists
fd -t f "img.png" router/
Length of output: 40
router/README.md
Outdated
| // returns a thrift protocol with the data ready to be read - type agnostic | ||
| function payloadToThriftProtocol(base64String) { | ||
| const binaryString = atob(base64String); | ||
| const bytes = new Uint8Array(binaryString.length); | ||
| for (let i = 0; i < binaryString.length; i++) { | ||
| bytes[i] = binaryString.charCodeAt(i); | ||
| } | ||
| const binaryData = base64ToBinary(bytes); | ||
| const transport = new TFramedTransport(binaryData); | ||
| const protocol = new TBinaryProtocol(transport); | ||
| return protocol | ||
| } | ||
| ``` | ||
|
|
||
| Second - use the helper to decode the payload | ||
| ```js | ||
| import { YourThriftObject } from './gen-js/your_thrift'; | ||
|
|
||
| async function handleThriftData(base64EncodedData) { | ||
| try { | ||
| const thriftObj = new YourThriftObject(); | ||
| const buffer = payloadToThriftProtocol(base64EncodedData) | ||
| thriftObj.read(buffer); | ||
|
|
||
| console.log('Decoded Thrift object:', thriftObj); | ||
| return decodedObject; | ||
| } catch (error) { | ||
| console.error('Failed to decode Thrift data:', error); | ||
| throw error; | ||
| } | ||
| } | ||
| ``` |
There was a problem hiding this comment.
Fix issues in the JavaScript implementation.
The JavaScript example contains several issues:
- The
base64ToBinaryfunction is used but not defined - The
decodedObjectvariable is returned but never defined - Error handling could be more specific
Consider this improvement:
function payloadToThriftProtocol(base64String) {
const binaryString = atob(base64String);
const bytes = new Uint8Array(binaryString.length);
for (let i = 0; i < binaryString.length; i++) {
bytes[i] = binaryString.charCodeAt(i);
}
- const binaryData = base64ToBinary(bytes);
+ const binaryData = bytes.buffer;
const transport = new TFramedTransport(binaryData);
const protocol = new TBinaryProtocol(transport);
return protocol
}
async function handleThriftData(base64EncodedData) {
try {
const thriftObj = new YourThriftObject();
const buffer = payloadToThriftProtocol(base64EncodedData)
thriftObj.read(buffer);
console.log('Decoded Thrift object:', thriftObj);
- return decodedObject;
+ return thriftObj;
} catch (error) {
- console.error('Failed to decode Thrift data:', error);
+ console.error('Failed to decode Thrift data:', error.message);
throw error;
}
}Also, consider removing the [Untested] tag once the implementation has been verified.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // returns a thrift protocol with the data ready to be read - type agnostic | |
| function payloadToThriftProtocol(base64String) { | |
| const binaryString = atob(base64String); | |
| const bytes = new Uint8Array(binaryString.length); | |
| for (let i = 0; i < binaryString.length; i++) { | |
| bytes[i] = binaryString.charCodeAt(i); | |
| } | |
| const binaryData = base64ToBinary(bytes); | |
| const transport = new TFramedTransport(binaryData); | |
| const protocol = new TBinaryProtocol(transport); | |
| return protocol | |
| } | |
| ``` | |
| Second - use the helper to decode the payload | |
| ```js | |
| import { YourThriftObject } from './gen-js/your_thrift'; | |
| async function handleThriftData(base64EncodedData) { | |
| try { | |
| const thriftObj = new YourThriftObject(); | |
| const buffer = payloadToThriftProtocol(base64EncodedData) | |
| thriftObj.read(buffer); | |
| console.log('Decoded Thrift object:', thriftObj); | |
| return decodedObject; | |
| } catch (error) { | |
| console.error('Failed to decode Thrift data:', error); | |
| throw error; | |
| } | |
| } | |
| ``` | |
| // returns a thrift protocol with the data ready to be read - type agnostic | |
| function payloadToThriftProtocol(base64String) { | |
| const binaryString = atob(base64String); | |
| const bytes = new Uint8Array(binaryString.length); | |
| for (let i = 0; i < binaryString.length; i++) { | |
| bytes[i] = binaryString.charCodeAt(i); | |
| } | |
| const binaryData = bytes.buffer; | |
| const transport = new TFramedTransport(binaryData); | |
| const protocol = new TBinaryProtocol(transport); | |
| return protocol | |
| } | |
| Second - use the helper to decode the payload |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
router/src/main/java/ai/chronon/router/RouteHandlerWrapper.java (3)
20-24: Remove outdated TODO commentThe TODO comment about adding Thrift BinaryProtocol support can be removed as the implementation is already present in the code.
27-28: Add documentation for public configuration fieldsThese public static fields should be documented with JavaDoc to explain their purpose and expected values.
+ /** + * Header name used to specify the desired response content type. + * Supported values are "application/json" (default) and "application/tbinary-b64". + */ public static String responseContentTypeHeaderName = "response-content-type"; + + /** + * Header value for requesting Thrift binary protocol responses encoded in base64. + */ public static String tbinaryB64TypeHeaderValue = "application/tbinary-b64";
147-215: Add support for custom type convertersConsider adding support for custom type converters to handle application-specific types or complex conversions.
+ /** + * Interface for custom type conversion + */ + public interface TypeConverter<T> { + T convert(String value) throws IllegalArgumentException; + } + + private static final Map<Type, TypeConverter<?>> customConverters = new ConcurrentHashMap<>(); + + /** + * Registers a custom converter for a specific type + */ + public static <T> void registerConverter(Class<T> type, TypeConverter<T> converter) { + customConverters.put(type, converter); + } private static Object convertValue(String value, Type targetType) { + // Check for custom converter first + TypeConverter<?> customConverter = customConverters.get(targetType); + if (customConverter != null) { + return customConverter.convert(value); + } + // Handle Class types if (targetType instanceof Class<?> targetClass) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
router/src/main/java/ai/chronon/router/RouteHandlerWrapper.java(1 hunks)
🔇 Additional comments (2)
router/src/main/java/ai/chronon/router/RouteHandlerWrapper.java (2)
100-109: Avoid exposing internal error details in HTTP responses
Returning stack traces directly in the HTTP response exposes sensitive internal information to the client. Use a generic error message instead and log the details server-side.
71-76: Add input validation for request parameters
The implementation accepts all parameters without validation. Consider adding size limits and parameter name validation.
| private static <I> I createInputFromParams(Map<String, String> params, Class<I> inputClass) throws Exception { | ||
| // Create new instance using no-args constructor | ||
| I input = inputClass.getDeclaredConstructor().newInstance(); | ||
|
|
||
| // Get all methods | ||
| Method[] methods = inputClass.getMethods(); | ||
|
|
||
| // Find and invoke setters for matching parameters | ||
| for (Method method : methods) { | ||
| if (isSetter(method)) { | ||
| String fieldName = getFieldNameFromSetter(method); | ||
| String paramValue = params.get(fieldName); | ||
|
|
||
| if (paramValue != null) { | ||
| Type paramType = method.getGenericParameterTypes()[0]; | ||
| Object convertedValue = convertValue(paramValue, paramType); | ||
| method.invoke(input, convertedValue); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return input; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize performance by caching reflection metadata
The current implementation performs reflection operations for every request. Consider caching the setter methods and their parameter types to improve performance.
+ private static final Map<Class<?>, Map<String, Method>> SETTER_CACHE = new ConcurrentHashMap<>();
+
private static <I> I createInputFromParams(Map<String, String> params, Class<I> inputClass) throws Exception {
I input = inputClass.getDeclaredConstructor().newInstance();
- Method[] methods = inputClass.getMethods();
- for (Method method : methods) {
- if (isSetter(method)) {
- String fieldName = getFieldNameFromSetter(method);
+ Map<String, Method> setters = SETTER_CACHE.computeIfAbsent(inputClass, cls -> {
+ return Arrays.stream(cls.getMethods())
+ .filter(RouteHandlerWrapper::isSetter)
+ .collect(Collectors.toMap(
+ RouteHandlerWrapper::getFieldNameFromSetter,
+ method -> method
+ ));
+ });
+
+ for (Map.Entry<String, String> param : params.entrySet()) {
+ Method setter = setters.get(param.getKey());
+ if (setter != null) {
String paramValue = params.get(fieldName);
if (paramValue != null) {
- Type paramType = method.getGenericParameterTypes()[0];
+ Type paramType = setter.getGenericParameterTypes()[0];
Object convertedValue = convertValue(paramValue, paramType);
- method.invoke(input, convertedValue);
+ setter.invoke(input, convertedValue);
}
}
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
router/src/main/java/ai/chronon/router/RouteHandlerWrapper.java (2)
21-25: Remove outdated TODO commentThe TODO comment about adding Thrift BinaryProtocol serialization support can be removed as this functionality has already been implemented in the code.
148-152: Improve robustness of field name extractionThe current implementation assumes the method name is longer than 3 characters and doesn't handle edge cases.
private static String getFieldNameFromSetter(Method method) { String methodName = method.getName(); + if (methodName.length() <= 3 || !methodName.startsWith("set")) { + throw new IllegalArgumentException("Invalid setter method name: " + methodName); + } String fieldName = methodName.substring(3); // Remove "set" + if (fieldName.isEmpty()) { + throw new IllegalArgumentException("Empty field name in setter: " + methodName); + } return fieldName.substring(0, 1).toLowerCase() + fieldName.substring(1); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
router/src/main/java/ai/chronon/router/RouteHandlerWrapper.java(1 hunks)
🔇 Additional comments (4)
router/src/main/java/ai/chronon/router/RouteHandlerWrapper.java (4)
171-185: 🛠️ Refactor suggestion
Improve enum conversion robustness
The enum conversion should handle null values more consistently and provide better error messages.
try {
// Try custom fromString method first
Method fromString = targetClass.getMethod("fromString", String.class);
+ if (!Modifier.isStatic(fromString.getModifiers())) {
+ throw new IllegalArgumentException("fromString method must be static");
+ }
Object result = fromString.invoke(null, value);
- if (value != null && result == null) {
+ if (result == null) {
throw new IllegalArgumentException(String.format("Invalid enum value %s for type %s", value, targetClass.getSimpleName()));
}
return result;
} catch (NoSuchMethodException e) {
// Fall back to standard enum valueOf
- return Enum.valueOf(targetClass.asSubclass(Enum.class), value.toUpperCase());
+ try {
+ return Enum.valueOf(targetClass.asSubclass(Enum.class), value.toUpperCase());
+ } catch (IllegalArgumentException ex) {
+ throw new IllegalArgumentException(String.format("Invalid enum value '%s'. Allowed values are: %s",
+ value, Arrays.toString(targetClass.getEnumConstants())));
+ }Likely invalid or redundant comment.
103-112:
Security: Do not expose stack traces in error responses
Exposing stack traces in error responses can reveal sensitive implementation details to potential attackers.
} catch (Exception e) {
- e.printStackTrace();
+ // Log the full stack trace
+ logger.error("Error processing request", e);
- StringWriter sw = new StringWriter();
- PrintWriter pw = new PrintWriter(sw);
- e.printStackTrace(pw);
- String stackTrace = sw.toString();
+ String errorMessage = "An error occurred while processing your request";
+ if (e instanceof IllegalArgumentException) {
+ errorMessage = e.getMessage();
+ }
ctx.response()
.setStatusCode(400)
.putHeader("content-type", "application/json")
- .end(new JsonObject().put("error", stackTrace).encode());
+ .end(new JsonObject().put("error", errorMessage).encode());Likely invalid or redundant comment.
74-79:
Add input validation for request parameters
The code should validate the number of parameters to prevent potential DoS attacks.
// Create map with path parameters
+ final int MAX_PARAMS = 100; // Adjust based on your needs
Map<String, String> params = new HashMap<>(ctx.pathParams());
// Add query parameters
+ if (ctx.queryParams().size() > MAX_PARAMS) {
+ throw new IllegalArgumentException("Too many query parameters");
+ }
for (Map.Entry<String, String> entry : ctx.queryParams().entries()) {
+ if (entry.getValue().length() > 1000) { // Adjust based on your needs
+ throw new IllegalArgumentException("Query parameter value too long: " + entry.getKey());
+ }
params.put(entry.getKey(), entry.getValue());
}Likely invalid or redundant comment.
51-56:
Add null checks and improve type safety
The method should validate input parameters and handle the unchecked cast more safely.
public static <T extends TBase> T deserializeTBinaryBase64(String base64Data, Class<? extends TBase> clazz) throws NoSuchMethodException, InvocationTargetException, InstantiationException, IllegalAccessException, TException {
+ if (base64Data == null || clazz == null) {
+ throw new IllegalArgumentException("base64Data and clazz cannot be null");
+ }
byte[] binaryData = base64Decoder.get().decode(base64Data);
+ if (binaryData.length == 0) {
+ throw new IllegalArgumentException("Decoded data is empty");
+ }
T tb = (T) clazz.getDeclaredConstructor().newInstance();
+ if (!clazz.isInstance(tb)) {
+ throw new IllegalArgumentException("Failed to create instance of " + clazz.getName());
+ }
binaryDeSerializer.get().deserialize(tb, binaryData);
return tb;
}Likely invalid or redundant comment.
router/src/main/java/ai/chronon/router/RouteHandlerWrapper.java
Outdated
Show resolved
Hide resolved
router/src/main/java/ai/chronon/router/RouteHandlerWrapper.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
router/README.md (2)
62-72: Enhance documentation with security considerations.When using base64 encoded binary data over HTTP:
- Document size limitations and potential impact on performance
- Add security considerations for handling binary data
- Include error handling examples
75-75: Fix grammar: Add missing article.Change "holds base64 encoded string" to "holds a base64 encoded string"
🧰 Tools
🪛 LanguageTool
[uncategorized] ~75-~75: You might be missing the article “a” here.
Context: ...looks like below. Thedatakey holds base64 encoded string of thrift binary protoco...(AI_EN_LECTOR_MISSING_DETERMINER_A)
router/src/main/java/ai/chronon/router/RouteHandlerWrapper.java (1)
21-25: Remove outdated TODO commentThe TODO comment about adding Thrift BinaryProtocol support is outdated as the implementation already exists.
Apply this diff:
/** * Wrapper class for creating Route handlers that map parameters to an Input object and transform it to Output * The wrapped handler produces a JSON response. - * TODO: Add support for Thrift BinaryProtocol based serialization based on a special request query param. */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
router/README.md(1 hunks)router/src/main/java/ai/chronon/router/RouteHandlerWrapper.java(1 hunks)router/src/test/java/ai/chronon/router/test/RouteHandlerWrapperTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- router/src/test/java/ai/chronon/router/test/RouteHandlerWrapperTest.java
🧰 Additional context used
🪛 LanguageTool
router/README.md
[uncategorized] ~75-~75: You might be missing the article “a” here.
Context: ...looks like below. The data key holds base64 encoded string of thrift binary protoco...
(AI_EN_LECTOR_MISSING_DETERMINER_A)
🔇 Additional comments (6)
router/README.md (1)
6-6: Verify the image accessibility and add alt text.
The referenced image img.png might not be accessible to all users. Consider:
- Adding descriptive alt text for accessibility
- Ensuring the image is committed to the repository
- Using a more descriptive filename
router/src/main/java/ai/chronon/router/RouteHandlerWrapper.java (5)
178-180: Improve null handling in enum conversion
The null check logic for enum conversion can be simplified and made more robust.
Apply this diff:
- if (value != null && result == null) {
+ if (result == null) {
throw new IllegalArgumentException(String.format(
"Invalid enum value %s for type %s", value, targetClass.getSimpleName()));
}28-29:
Make configuration constants final and consider using an enum for content types
The configuration fields should be immutable to prevent accidental modifications at runtime.
Apply this diff:
- public static String RESPONSE_CONTENT_TYPE_HEADER = "response-content-type";
- public static String TBINARY_B64_TYPE_VALUE = "application/tbinary-b64";
+ public static final String RESPONSE_CONTENT_TYPE_HEADER = "response-content-type";
+ public static final String TBINARY_B64_TYPE_VALUE = "application/tbinary-b64";51-56: 🛠️ Refactor suggestion
Improve type safety and documentation of deserializeTBinaryBase64
The method needs better documentation and type safety improvements.
Apply this diff:
+ /**
+ * Deserializes a base64 encoded Thrift binary protocol string into a Thrift object.
+ *
+ * @param base64Data The base64 encoded string containing serialized Thrift data
+ * @param clazz The target Thrift class to deserialize into
+ * @param <T> The type parameter extending TBase
+ * @return Deserialized Thrift object
+ * @throws TException if Thrift deserialization fails
+ * @throws ReflectiveOperationException if reflection operations fail
+ */
public static <T extends TBase> T deserializeTBinaryBase64(String base64Data, Class<? extends TBase> clazz) throws NoSuchMethodException, InvocationTargetException, InstantiationException, IllegalAccessException, TException {
byte[] binaryData = base64Decoder.get().decode(base64Data);
- T tb = (T) clazz.getDeclaredConstructor().newInstance();
+ @SuppressWarnings("unchecked")
+ T tb = (T) clazz.getDeclaredConstructor().newInstance();
+ if (!clazz.isInstance(tb)) {
+ throw new IllegalArgumentException("Invalid Thrift class type: " + clazz.getName());
+ }
binaryDeSerializer.get().deserialize(tb, binaryData);
return tb;
}106-115:
Avoid exposing internal error details in HTTP responses
Returning stack traces directly in the HTTP response exposes sensitive internal information.
Apply this diff:
} catch (Exception e) {
e.printStackTrace();
-
- StringWriter sw = new StringWriter();
- PrintWriter pw = new PrintWriter(sw);
- e.printStackTrace(pw);
- String stackTrace = sw.toString();
-
- ctx.response().setStatusCode(400).putHeader("content-type", "application/json").end(new JsonObject().put("error", stackTrace).encode());
+ // Log the full stack trace for debugging
+ LOGGER.error("Error processing request", e);
+
+ ctx.response()
+ .setStatusCode(400)
+ .putHeader("content-type", "application/json")
+ .end(new JsonObject()
+ .put("error", "An unexpected error occurred")
+ .put("code", "BAD_REQUEST")
+ .encode());
}131-142:
Add input validation for request parameters
The current implementation accepts parameters without validation, which could lead to security vulnerabilities.
Apply this diff:
// Find and invoke setters for matching parameters
+ final int MAX_PARAM_LENGTH = 1024; // Adjust based on requirements
for (Map.Entry<String, String> param : params.entrySet()) {
Method setter = setters.get(param.getKey());
if (setter != null) {
String paramValue = param.getValue();
if (paramValue != null) {
+ if (paramValue.length() > MAX_PARAM_LENGTH) {
+ throw new IllegalArgumentException(
+ String.format("Parameter '%s' exceeds maximum length of %d",
+ param.getKey(), MAX_PARAM_LENGTH));
+ }
+ if (!paramValue.matches("[a-zA-Z0-9,._:@\\-+]*")) {
+ throw new IllegalArgumentException(
+ String.format("Parameter '%s' contains invalid characters",
+ param.getKey()));
+ }
Type paramType = setter.getGenericParameterTypes()[0];
Object convertedValue = convertValue(paramValue, paramType);
setter.invoke(input, convertedValue);
piyush-zlai
left a comment
piyush-zlai
left a comment
There was a problem hiding this comment.
Largely looks good - had a few questions / comments
build.sbt
Outdated
| "io.vertx" % "vertx-web" % "4.5.9", | ||
| "io.vertx" % "vertx-web-client" % "4.5.9", | ||
| "io.vertx" % "vertx-junit5" % "4.5.9", | ||
| "org.slf4j" % "slf4j-api" % "1.7.36", // match with spark |
There was a problem hiding this comment.
we have a val declared for the slf4j version - could use that
build.sbt
Outdated
| .dependsOn(api.%("compile->compile;test->test")) | ||
| .settings( | ||
| libraryDependencies ++= Seq( | ||
| "io.vertx" % "vertx-core" % "4.5.9", |
There was a problem hiding this comment.
I pulled out the vertx deps into a collection in my PR to migrate play to vertx, so post rebase you could pull that collection in
| @@ -0,0 +1,83 @@ | |||
| # Why? | |||
There was a problem hiding this comment.
In the other related PR - #118, I pulled some of the common Vert.x code between Hub & fetcher in a module - service_commons, wdyt of folding router into that?
| @@ -0,0 +1,83 @@ | |||
| # Why? | |||
|
|
|||
| We have a lot of glue code that maps data into objects across system and language boundaries. | |||
There was a problem hiding this comment.
Thanks for writing up this Readme & documenting!
| ctx.response().putHeader("content-type", "application/json").end(JsonObject.mapFrom(output).encode()); | ||
| } else { | ||
| if (!responseFormat.equals(TBINARY_B64_TYPE_VALUE)) { | ||
| throw new IllegalArgumentException("Unsupported response-content-type: " + responseFormat + ". Supported values are: " + TBINARY_B64_TYPE_VALUE); |
There was a problem hiding this comment.
in the error - should we say supported values are application/json and the tbinary_b64_type_value?
| /** | ||
| * Creates a RoutingContext handler that maps parameters to an Input object and transforms it to Output | ||
| * | ||
| * @param transformer Function to convert from Input to Output |
There was a problem hiding this comment.
one thing to maybe call out explicitly is the transformer is responsible for wrapping the work in a vert.x future if its time consuming. Else we'll quickly exhaust event threads
|
|
||
| BinaryResponse binaryResponse = new BinaryResponse(responseBase64, TBINARY_B64_TYPE_VALUE); | ||
|
|
||
| ctx.response().putHeader("content-type", "application/json").end(JsonObject.mapFrom(binaryResponse).encode()); |
There was a problem hiding this comment.
confused, shouldn't the content type be TBINARY_B64_TYPE_VALUE?
There was a problem hiding this comment.
Yes made changes accordingly
| ctx.response().putHeader("content-type", "application/json").end(JsonObject.mapFrom(binaryResponse).encode()); | ||
| } | ||
| } catch (Exception e) { | ||
| e.printStackTrace(); |
There was a problem hiding this comment.
lets log here instead of printstack trace. Our logging configs will take care of dumping the st
| e.printStackTrace(pw); | ||
| String stackTrace = sw.toString(); | ||
|
|
||
| ctx.response().setStatusCode(400).putHeader("content-type", "application/json").end(new JsonObject().put("error", stackTrace).encode()); |
There was a problem hiding this comment.
5xx response here instead of 4xx?
| String responseFormat = ctx.request().getHeader(RESPONSE_CONTENT_TYPE_HEADER); | ||
| if (responseFormat == null || responseFormat.equals("application/json")) { | ||
| // Send json response | ||
| ctx.response().putHeader("content-type", "application/json").end(JsonObject.mapFrom(output).encode()); |
There was a problem hiding this comment.
in my queued up PR #118, I have some helper wrappers to return 200, 400, 404, 500 responses without the verbose ctx.response... stuff. Can use / extends those here
There was a problem hiding this comment.
those are written as scala implicit conversions which are not directly usable here. will address this in a separate PR to unblock the other work.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
service_commons/src/main/java/ai/chronon/service_common/RouteHandlerWrapper.java (1)
28-29: Declare constants asstatic finaland uppercaseApply this diff:
-public static String RESPONSE_CONTENT_TYPE_HEADER = "response-content-type"; -public static String TBINARY_B64_TYPE_VALUE = "application/tbinary-b64"; +public static final String RESPONSE_CONTENT_TYPE_HEADER = "response-content-type"; +public static final String TBINARY_B64_TYPE_VALUE = "application/tbinary-b64";service_commons/src/test/java/ai/chronon/service_common/test/OrderStatus.java (1)
9-15: Handle invalid input without returning nullReturning null can cause NullPointerExceptions. Throw an exception instead.
online/src/main/scala/ai/chronon/online/stats/DriftStore.scala (1)
219-219: Replace hard-coded breaks with configuration.The TODO comment suggests this is temporary. Consider moving the breaks to a configuration file for flexibility.
service_commons/src/test/java/ai/chronon/service_common/test/RouteHandlerWrapperTest.java (2)
220-222: Remove debug print statement.Use proper logging instead of System.out.println for debugging.
- if(response.statusCode() != 200) { - System.out.println(response.bodyAsString()); - }
244-246: Consolidate duplicate debug code.The same debug code is repeated in three test methods. Consider extracting to a helper method.
Also applies to: 265-267, 286-288
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (13)
build.sbt(4 hunks)hub/src/main/java/ai/chronon/hub/HubVerticle.java(1 hunks)online/src/main/scala/ai/chronon/online/stats/DriftStore.scala(1 hunks)service/src/main/java/ai/chronon/service/handlers/FetchHandler.java(0 hunks)service_commons/src/main/java/ai/chronon/service_common/ApiProvider.java(1 hunks)service_commons/src/main/java/ai/chronon/service_common/BinaryResponse.java(1 hunks)service_commons/src/main/java/ai/chronon/service_common/ChrononServiceLauncher.java(1 hunks)service_commons/src/main/java/ai/chronon/service_common/ConfigStore.java(1 hunks)service_commons/src/main/java/ai/chronon/service_common/RouteHandlerWrapper.java(1 hunks)service_commons/src/test/java/ai/chronon/service_common/test/OrderStatus.java(1 hunks)service_commons/src/test/java/ai/chronon/service_common/test/RouteHandlerWrapperTest.java(1 hunks)service_commons/src/test/java/ai/chronon/service_common/test/TestInput.java(1 hunks)service_commons/src/test/java/ai/chronon/service_common/test/UserRole.java(1 hunks)
💤 Files with no reviewable changes (1)
- service/src/main/java/ai/chronon/service/handlers/FetchHandler.java
✅ Files skipped from review due to trivial changes (5)
- service_commons/src/test/java/ai/chronon/service_common/test/UserRole.java
- service_commons/src/main/java/ai/chronon/service_common/ConfigStore.java
- service_commons/src/main/java/ai/chronon/service_common/ChrononServiceLauncher.java
- service_commons/src/main/java/ai/chronon/service_common/ApiProvider.java
- hub/src/main/java/ai/chronon/hub/HubVerticle.java
🚧 Files skipped from review as they are similar to previous changes (1)
- build.sbt
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: other_spark_tests
- GitHub Check: mutation_spark_tests
- GitHub Check: fetcher_spark_tests
- GitHub Check: no_spark_scala_tests
- GitHub Check: join_spark_tests
- GitHub Check: scala_compile_fmt_fix
🔇 Additional comments (3)
service_commons/src/test/java/ai/chronon/service_common/test/TestInput.java (1)
8-42: Code looks goodThe
TestInputclass is well-defined.service_commons/src/test/java/ai/chronon/service_common/test/RouteHandlerWrapperTest.java (1)
43-92: LGTM! Comprehensive test setup.Good job setting up multiple routes to test different scenarios.
service_commons/src/main/java/ai/chronon/service_common/BinaryResponse.java (1)
3-19: LGTM! Well-designed immutable class.Clean implementation with proper encapsulation.
| /** | ||
| * Wrapper class for creating Route handlers that map parameters to an Input object and transform it to Output | ||
| * The wrapped handler produces a JSON response. | ||
| * TODO: Add support for Thrift BinaryProtocol based serialization based on a special request query param. |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove obsolete TODO
Thrift BinaryProtocol support is already implemented.
| e.printStackTrace(); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid printStackTrace()
Use a logging framework instead.
| StringWriter sw = new StringWriter(); | ||
| PrintWriter pw = new PrintWriter(sw); | ||
| e.printStackTrace(pw); | ||
| String stackTrace = sw.toString(); | ||
|
|
||
| ctx.response().setStatusCode(400).putHeader("content-type", "application/json").end(new JsonObject().put("error", stackTrace).encode()); |
There was a problem hiding this comment.
Don't expose stack traces in responses
Return a generic error message to prevent information leakage.
Apply this diff:
- e.printStackTrace(pw);
- String stackTrace = sw.toString();
-
- ctx.response().setStatusCode(400).putHeader("content-type", "application/json").end(new JsonObject().put("error", stackTrace).encode());
+ ctx.response().setStatusCode(400).putHeader("content-type", "application/json").end(new JsonObject().put("error", "An internal error occurred").encode());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| StringWriter sw = new StringWriter(); | |
| PrintWriter pw = new PrintWriter(sw); | |
| e.printStackTrace(pw); | |
| String stackTrace = sw.toString(); | |
| ctx.response().setStatusCode(400).putHeader("content-type", "application/json").end(new JsonObject().put("error", stackTrace).encode()); | |
| StringWriter sw = new StringWriter(); | |
| PrintWriter pw = new PrintWriter(sw); | |
| ctx.response().setStatusCode(400).putHeader("content-type", "application/json").end(new JsonObject().put("error", "An internal error occurred").encode()); |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (11)
service_commons/src/main/java/ai/chronon/service/RouteHandlerWrapper.java (4)
24-24: Consider implementing the TODO.Adding support for Thrift BinaryProtocol based serialization would enhance functionality.
Do you want me to help implement this feature or open a GitHub issue?
109-110: Return more informative error messages.Providing detailed errors can aid in debugging client issues.
174-181: Simplify enum conversion logic.You can use
Enum.valueOfdirectly for enums without a customfromStringmethod.
223-224: Include type name in exceptions.Improves clarity when unsupported types are encountered.
service_commons/src/test/java/ai/chronon/service/test/OrderStatus.java (1)
9-15: Avoid returning null infromString.Consider throwing an exception or returning a default to prevent NPEs.
service_commons/src/test/java/ai/chronon/service/test/RouteHandlerWrapperTest.java (4)
25-36: Add equals/hashCode to TestOutput class.For better test assertions and object comparison.
class TestOutput { private final String userId; private final String summary; public TestOutput(String userId, String summary) { this.userId = userId; this.summary = summary; } public String getUserId() { return userId; } public String getSummary() { return summary; } + + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (!(o instanceof TestOutput)) return false; + TestOutput that = (TestOutput) o; + return Objects.equals(userId, that.userId) && + Objects.equals(summary, that.summary); + } + + @Override + public int hashCode() { + return Objects.hash(userId, summary); + } }
220-222: Replace System.out.println with proper logging.Use SLF4J or the logging framework of your choice for consistent error reporting.
- System.out.println(response.bodyAsString()); + log.debug("Response body: {}", response.bodyAsString());Also applies to: 244-246, 265-267, 286-288
237-257: Consider using parameterized tests.The test could be more maintainable with JUnit's @ParameterizedTest for different parameter combinations.
+ @ParameterizedTest + @CsvSource({ + "my_col,my_slice,my_name,5", + "col2,slice2,name2,10" + }) - void testAllParameterTypesThrift(VertxTestContext testContext) { + void testAllParameterTypesThrift(String column, String slice, String name, + String sizeMillis, VertxTestContext testContext) { - client.get("/api/column/my_col/slice/my_slice") - .addQueryParam("name", "my_name") - .addQueryParam("sizeMillis", "5") + client.get("/api/column/" + column + "/slice/" + slice) + .addQueryParam("name", name) + .addQueryParam("sizeMillis", sizeMillis)
178-191: Enhance error message assertions.Add specific assertions for error message content to ensure proper error handling.
JsonObject error = response.bodyAsJsonObject(); - assertTrue(error.getString("error").contains("not-a-number")); + assertEquals("Failed to parse 'not-a-number' as number for parameter 'limit'", + error.getString("error"));service_commons/README.md (2)
11-12: Fix grammar in the introduction.Add missing articles for better readability.
-This module adds ability to make REST api's type-safe and boilerplate free +This module adds the ability to make REST APIs type-safe and boilerplate-free🧰 Tools
🪛 LanguageTool
[uncategorized] ~11-~11: You might be missing the article “the” here.
Context: ...non codebase already. This module adds ability to make REST api's type-safe and boiler...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
75-76: Fix grammar in the response section.Add missing article and hyphenation.
-The `data` key holds base64 encoded string +The `data` key holds a base64-encoded string🧰 Tools
🪛 LanguageTool
[uncategorized] ~75-~75: You might be missing the article “a” here.
Context: ...looks like below. Thedatakey holds base64 encoded string of thrift binary protoco...(AI_EN_LECTOR_MISSING_DETERMINER_A)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (1)
service_commons/img.pngis excluded by!**/*.png
📒 Files selected for processing (9)
.tool-versions(1 hunks)build.sbt(4 hunks)service_commons/README.md(1 hunks)service_commons/src/main/java/ai/chronon/service/ApiProvider.java(0 hunks)service_commons/src/main/java/ai/chronon/service/RouteHandlerWrapper.java(1 hunks)service_commons/src/test/java/ai/chronon/service/test/OrderStatus.java(1 hunks)service_commons/src/test/java/ai/chronon/service/test/RouteHandlerWrapperTest.java(1 hunks)service_commons/src/test/java/ai/chronon/service/test/TestInput.java(1 hunks)service_commons/src/test/java/ai/chronon/service/test/UserRole.java(1 hunks)
💤 Files with no reviewable changes (1)
- service_commons/src/main/java/ai/chronon/service/ApiProvider.java
✅ Files skipped from review due to trivial changes (2)
- service_commons/src/test/java/ai/chronon/service/test/UserRole.java
- .tool-versions
🚧 Files skipped from review as they are similar to previous changes (1)
- build.sbt
🧰 Additional context used
🪛 LanguageTool
service_commons/README.md
[uncategorized] ~11-~11: You might be missing the article “the” here.
Context: ...non codebase already. This module adds ability to make REST api's type-safe and boiler...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~75-~75: You might be missing the article “a” here.
Context: ...looks like below. The data key holds base64 encoded string of thrift binary protoco...
(AI_EN_LECTOR_MISSING_DETERMINER_A)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: table_utils_delta_format_spark_tests
- GitHub Check: other_spark_tests
- GitHub Check: mutation_spark_tests
- GitHub Check: fetcher_spark_tests
- GitHub Check: scala_compile_fmt_fix
- GitHub Check: join_spark_tests
- GitHub Check: no_spark_scala_tests
🔇 Additional comments (1)
service_commons/src/test/java/ai/chronon/service/test/TestInput.java (1)
8-42: Code looks good.The class structure is clear and well-defined.
## Summary
Adding a router that can map query params and path params together into
a thrift object / any pojo with setters.
We should now be able to kill all the manual mapping code in both
backend and frontend - and make everything typesafe.
we are manually mapping in a few places
- in the server for the key
- in the server for the value (to massage objects into the shape that
front end expects)
- in the frontend to map the reponse back into a value
We basically translate a `Func<Input, Output>` -> `Func<RequestContext,
Response>` via reflection to achieve this
Thrift def
```c
struct TileKey {
1: optional string column
2: optional string slice
3: optional string name
4: optional i64 sizeMillis
}
```
Route declaration
```java
Function<TileKey, TileKey> thriftTransformer = input -> input; // some dummy function
router.get("/thrift_api/column/:column/slice/:slice")
.handler(RouteHandlerWrapper.createHandler(thriftTransformer, TileKey.class));
```
Requesting
```java
client.get("/thrift_api/column/my_col/slice/my_slice")
.addQueryParam("name", "my_name")
.addQueryParam("sizeMillis", "5")
.send()
```
Response
```json
{"column":"my_col","slice":"my_slice","name":"my_name","sizeMillis":5}
```
## Checklist
- [x] Added Unit Tests
- [ ] Covered by existing CI
- [ ] Integration tested
- [ ] Documentation update
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
Based on the comprehensive summary, here are the release notes:
- **New Features**
- Introduced a new `RouteHandlerWrapper` to enhance type-safe REST API
handling
- Added support for advanced parameter mapping and serialization in
service endpoints
- **Dependency Updates**
- Upgraded Java version to Corretto 17
- Updated project dependencies and build configurations
- Added Jackson and Vert.x libraries for improved functionality
- **Testing Improvements**
- Created new test utilities and test classes for service commons
- Added comprehensive test coverage for route handling and parameter
processing
- **Documentation**
- Enhanced README for service commons module with detailed API usage
guidelines
- **Chores**
- Cleaned up unused imports
- Simplified serialization logic in some components
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
---------
Co-authored-by: kumarteja <[email protected]>
Co-authored-by: Kumar Teja Chippala <[email protected]>
## Summary
Adding a router that can map query params and path params together into
a thrift object / any pojo with setters.
We should now be able to kill all the manual mapping code in both
backend and frontend - and make everything typesafe.
we are manually mapping in a few places
- in the server for the key
- in the server for the value (to massage objects into the shape that
front end expects)
- in the frontend to map the reponse back into a value
We basically translate a `Func<Input, Output>` -> `Func<RequestContext,
Response>` via reflection to achieve this
Thrift def
```c
struct TileKey {
1: optional string column
2: optional string slice
3: optional string name
4: optional i64 sizeMillis
}
```
Route declaration
```java
Function<TileKey, TileKey> thriftTransformer = input -> input; // some dummy function
router.get("/thrift_api/column/:column/slice/:slice")
.handler(RouteHandlerWrapper.createHandler(thriftTransformer, TileKey.class));
```
Requesting
```java
client.get("/thrift_api/column/my_col/slice/my_slice")
.addQueryParam("name", "my_name")
.addQueryParam("sizeMillis", "5")
.send()
```
Response
```json
{"column":"my_col","slice":"my_slice","name":"my_name","sizeMillis":5}
```
## Checklist
- [x] Added Unit Tests
- [ ] Covered by existing CI
- [ ] Integration tested
- [ ] Documentation update
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
Based on the comprehensive summary, here are the release notes:
- **New Features**
- Introduced a new `RouteHandlerWrapper` to enhance type-safe REST API
handling
- Added support for advanced parameter mapping and serialization in
service endpoints
- **Dependency Updates**
- Upgraded Java version to Corretto 17
- Updated project dependencies and build configurations
- Added Jackson and Vert.x libraries for improved functionality
- **Testing Improvements**
- Created new test utilities and test classes for service commons
- Added comprehensive test coverage for route handling and parameter
processing
- **Documentation**
- Enhanced README for service commons module with detailed API usage
guidelines
- **Chores**
- Cleaned up unused imports
- Simplified serialization logic in some components
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
---------
Co-authored-by: kumarteja <[email protected]>
Co-authored-by: Kumar Teja Chippala <[email protected]>
## Summary
Adding a router that can map query params and path params together into
a thrift object / any pojo with setters.
We should now be able to kill all the manual mapping code in both
backend and frontend - and make everything typesafe.
we are manually mapping in a few places
- in the server for the key
- in the server for the value (to massage objects into the shape that
front end expects)
- in the frontend to map the reponse back into a value
We basically translate a `Func<Input, Output>` -> `Func<RequestContext,
Response>` via reflection to achieve this
Thrift def
```c
struct TileKey {
1: optional string column
2: optional string slice
3: optional string name
4: optional i64 sizeMillis
}
```
Route declaration
```java
Function<TileKey, TileKey> thriftTransformer = input -> input; // some dummy function
router.get("/thrift_api/column/:column/slice/:slice")
.handler(RouteHandlerWrapper.createHandler(thriftTransformer, TileKey.class));
```
Requesting
```java
client.get("/thrift_api/column/my_col/slice/my_slice")
.addQueryParam("name", "my_name")
.addQueryParam("sizeMillis", "5")
.send()
```
Response
```json
{"column":"my_col","slice":"my_slice","name":"my_name","sizeMillis":5}
```
## Checklist
- [x] Added Unit Tests
- [ ] Covered by existing CI
- [ ] Integration tested
- [ ] Documentation update
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
Based on the comprehensive summary, here are the release notes:
- **New Features**
- Introduced a new `RouteHandlerWrapper` to enhance type-safe REST API
handling
- Added support for advanced parameter mapping and serialization in
service endpoints
- **Dependency Updates**
- Upgraded Java version to Corretto 17
- Updated project dependencies and build configurations
- Added Jackson and Vert.x libraries for improved functionality
- **Testing Improvements**
- Created new test utilities and test classes for service commons
- Added comprehensive test coverage for route handling and parameter
processing
- **Documentation**
- Enhanced README for service commons module with detailed API usage
guidelines
- **Chores**
- Cleaned up unused imports
- Simplified serialization logic in some components
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
---------
Co-authored-by: kumarteja <[email protected]>
Co-authored-by: Kumar Teja Chippala <[email protected]>
## Summary
Adding a router that can map query params and path params together into
a thrift object / any pojo with setters.
We should now be able to kill all the manual mapping code in both
backend and frontend - and make everything typesafe.
we are manually mapping in a few places
- in the server for the key
- in the server for the value (to massage objects into the shape that
front end expects)
- in the frontend to map the reponse back into a value
We basically translate a `Func<Input, Output>` -> `Func<RequestContext,
Response>` via reflection to achieve this
Thrift def
```c
struct TileKey {
1: optional string column
2: optional string slice
3: optional string name
4: optional i64 sizeMillis
}
```
Route declaration
```java
Function<TileKey, TileKey> thriftTransformer = input -> input; // some dummy function
router.get("/thrift_api/column/:column/slice/:slice")
.handler(RouteHandlerWrapper.createHandler(thriftTransformer, TileKey.class));
```
Requesting
```java
client.get("/thrift_api/column/my_col/slice/my_slice")
.addQueryParam("name", "my_name")
.addQueryParam("sizeMillis", "5")
.send()
```
Response
```json
{"column":"my_col","slice":"my_slice","name":"my_name","sizeMillis":5}
```
## Checklist
- [x] Added Unit Tests
- [ ] Covered by existing CI
- [ ] Integration tested
- [ ] Documentation update
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
Based on the comprehensive summary, here are the release notes:
- **New Features**
- Introduced a new `RouteHandlerWrapper` to enhance type-safe REST API
handling
- Added support for advanced parameter mapping and serialization in
service endpoints
- **Dependency Updates**
- Upgraded Java version to Corretto 17
- Updated project dependencies and build configurations
- Added Jackson and Vert.x libraries for improved functionality
- **Testing Improvements**
- Created new test utilities and test classes for service commons
- Added comprehensive test coverage for route handling and parameter
processing
- **Documentation**
- Enhanced README for service commons module with detailed API usage
guidelines
- **Chores**
- Cleaned up unused imports
- Simplified serialization logic in some components
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
---------
Co-authored-by: kumarteja <[email protected]>
Co-authored-by: Kumar Teja Chippala <[email protected]>
## Summary
Adding a router that can map query params and path params together into
a thrift object / any pojo with setters.
We should now be able to kill all the manual mapping code in both
baour clientsend and frontend - and make everything typesafe.
we are manually mapping in a few places
- in the server for the key
- in the server for the value (to massage objects into the shape that
front end expects)
- in the frontend to map the reponse baour clients into a value
We basically translate a `Func<Input, Output>` -> `Func<RequestContext,
Response>` via reflection to achieve this
Thrift def
```c
struct TileKey {
1: optional string column
2: optional string slice
3: optional string name
4: optional i64 sizeMillis
}
```
Route declaration
```java
Function<TileKey, TileKey> thriftTransformer = input -> input; // some dummy function
router.get("/thrift_api/column/:column/slice/:slice")
.handler(RouteHandlerWrapper.createHandler(thriftTransformer, TileKey.class));
```
Requesting
```java
client.get("/thrift_api/column/my_col/slice/my_slice")
.addQueryParam("name", "my_name")
.addQueryParam("sizeMillis", "5")
.send()
```
Response
```json
{"column":"my_col","slice":"my_slice","name":"my_name","sizeMillis":5}
```
## Cheour clientslist
- [x] Added Unit Tests
- [ ] Covered by existing CI
- [ ] Integration tested
- [ ] Documentation update
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
Based on the comprehensive summary, here are the release notes:
- **New Features**
- Introduced a new `RouteHandlerWrapper` to enhance type-safe REST API
handling
- Added support for advanced parameter mapping and serialization in
service endpoints
- **Dependency Updates**
- Upgraded Java version to Corretto 17
- Updated project dependencies and build configurations
- Added Jaour clientsson and Vert.x libraries for improved functionality
- **Testing Improvements**
- Created new test utilities and test classes for service commons
- Added comprehensive test coverage for route handling and parameter
processing
- **Documentation**
- Enhanced README for service commons module with detailed API usage
guidelines
- **Chores**
- Cleaned up unused imports
- Simplified serialization logic in some components
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
---------
Co-authored-by: kumarteja <[email protected]>
Co-authored-by: Kumar Teja Chippala <[email protected]>
Summary
Adding a router that can map query params and path params together into a thrift object / any pojo with setters.
We should now be able to kill all the manual mapping code in both backend and frontend - and make everything typesafe.
we are manually mapping in a few places
We basically translate a
Func<Input, Output>->Func<RequestContext, Response>via reflection to achieve thisThrift def
Route declaration
Requesting
Response
{"column":"my_col","slice":"my_slice","name":"my_name","sizeMillis":5}Checklist
Summary by CodeRabbit
Based on the comprehensive summary, here are the release notes:
New Features
RouteHandlerWrapperto enhance type-safe REST API handlingDependency Updates
Testing Improvements
Documentation
Chores