feat: Move from Jackson to Gson serialization#808
feat: Move from Jackson to Gson serialization#808kabir wants to merge 6 commits intoa2aproject:0.3.xfrom
Conversation
…ct#789) Replacing Jackson with Gson for json (de)serialization Fixes #<issue_number_goes_here> 🦕 --------- Signed-off-by: Emmanuel Hugonnet <ehugonne@redhat.com> Co-authored-by: Kabir Khan <kkhan@redhat.com>
* chore: updating the workflows Signed-off-by: Emmanuel Hugonnet <ehugonne@redhat.com> * chore: fixing javadoc Signed-off-by: Emmanuel Hugonnet <ehugonne@redhat.com> * chore: Updating kafka version Signed-off-by: Emmanuel Hugonnet <ehugonne@redhat.com> * fix; Fixing the last issues to be able to pass the TCK again Signed-off-by: Emmanuel Hugonnet <ehugonne@redhat.com> * fix: Fixing the missing id in the jsonrpc response Extract request id before jsonrpc validation so error responses include top-level id. Signed-off-by: Emmanuel Hugonnet <ehugonne@redhat.com> --------- Signed-off-by: Emmanuel Hugonnet <ehugonne@redhat.com>
There was a problem hiding this comment.
Code Review
This pull request migrates the SDK from Jackson to Gson, introducing a centralized JsonUtil utility and custom exceptions to remove the Jackson dependency. It also updates Kafka and Protobuf versions, adds new BOM files, and implements double-checked locking for agent card retrieval. Feedback identifies critical thread-safety issues in the locking logic due to missing volatile declarations and a circular dependency risk in JsonUtil's static initialization. Additionally, the review highlights a security vulnerability in reflection-based deserialization, potential precision loss for numeric IDs, and several areas where JSON parsing lacks robustness against malformed or non-object input structures.
9b24775 to
5517b6d
Compare
…pliance This commit addresses critical review feedback across security, data correctness, thread safety, and A2A spec compliance: **Security (CWE-470):** - Add whitelist validation before Class.forName() in ThrowableTypeAdapter to prevent arbitrary class loading vulnerability. Only allow java.lang.*, java.io.*, and io.a2a.* packages plus specific safe exception types. **Data Correctness:** - Fix JSON-RPC ID truncation: use getAsLong() instead of getAsInt() to handle IDs exceeding Integer.MAX_VALUE - Fix error data serialization: use GSON.toJson() instead of toString() to preserve JSON structure in error responses - Use shared JsonUtil.OBJECT_MAPPER in JSONRPCUtils for consistent type adapter support (OffsetDateTime, etc.) across all serialization paths **Thread Safety:** - Implement double-checked locking in getAgentCard() methods across Client, JSONRPCTransport, and RestTransport to prevent race conditions during lazy initialization - Declare agentCard and needsExtendedCard fields as volatile to ensure visibility across threads and prevent instruction reordering in double-checked locking pattern **A2A Spec Compliance:** - Fix intermittent null field serialization: JSON-RPC server now uses JsonUtil.toJson() instead of returning AgentCard object (which caused Quarkus to use Jackson, serializing nulls as "field": null instead of omitting them) - Remove leading slash from pushNotificationConfig resource name pattern to match REST API conventions **Code Quality:** - Improve error messages in JdkA2AHttpClient streaming handler (body not available in streaming context) - Add verification tests for AgentCard null field omission - Fix javadoc reference to use JsonUtil.OBJECT_MAPPER instead of Gson Resolves intermittent TCK failures related to null field serialization. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
5517b6d to
34fd3b4
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request migrates the project's JSON processing from Jackson to Gson, introducing a centralized JsonUtil utility and custom type adapters to handle A2A Protocol types and Java 17 reflection constraints. The changes span the SDK, client, and server modules, and include thread-safety enhancements for agent card initialization using volatile fields and double-checked locking. Feedback was provided regarding a redundant inner try-catch block in the JSON-RPC routing logic that should be removed to simplify the code and ensure that the outer JsonSyntaxException handler is reachable.
…ependency, precision, and robustness This commit addresses the remaining HIGH and MEDIUM priority issues from the PR a2aproject#808 review: **HIGH Priority - Circular Dependency (JsonUtil.java:333):** - Convert JSONRPCErrorTypeAdapter from TypeAdapter to TypeAdapterFactory to eliminate circular dependency during static initialization - The adapter now receives the Gson instance via the factory's create() method instead of referencing OBJECT_MAPPER during class initialization - Prevents potential NullPointerException if the adapter is invoked during initialization **MEDIUM Priority - ID Precision Loss (JsonUtil.java:1002):** - Fix JSON-RPC ID serialization to preserve fractional values (e.g., 1.5 remains 1.5) - Check number type: use longValue() for integer types (Long, Integer, Short, Byte), but preserve full precision for Double/Float by passing Number directly to JsonWriter - Prevents data loss when clients use fractional numbers as JSON-RPC IDs **MEDIUM Priority - JSON Parsing Robustness:** - RestErrorMapper.java:36-37: Add null and type checks before calling getAsString() to handle JsonNull and non-primitive types gracefully - JSONRPCUtils.java:160: Validate JSON is an object before calling getAsJsonObject() to provide clear error message for arrays or primitives - A2AServerRoutes.java:97-101: Remove redundant inner try-catch block that made outer JsonSyntaxException handler unreachable, simplifying error handling logic **MEDIUM Priority - Performance Optimization:** - JSONRPCUtils.java:299: Eliminate inefficient double conversion (serialize → deserialize) - Deserialize AgentCard directly from JsonElement using Gson.fromJson(JsonElement, Class) instead of converting to string first All changes maintain backward compatibility and improve error handling for malformed input. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request migrates the SDK from Jackson to a custom Gson-based utility (JsonUtil) to handle polymorphic serialization and reduce dependency overhead. Key updates include the addition of specialized TypeAdapters for protocol types and the implementation of thread-safe lazy initialization in client transports. Feedback highlights several JSON-RPC 2.0 specification compliance issues in JSONRPCUtils, such as the requirement to include the id field in all responses (even if null), the need to handle optional params and null IDs in requests, and an issue where error data could be double-encoded during serialization.
Extract duplicated id-writing code into ensureId() helper method to reduce repetition in response serialization. Add @nullable annotations to handle null id values throughout the codebase per JSON-RPC 2.0 spec requirements. Fix error data serialization to use fromJson() instead of toString() to preserve proper JSON structure. Optimize params handling by using toJson() directly instead of inefficient serialize-parse roundtrip. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request migrates the SDK from Jackson to Gson, introducing a centralized JsonUtil with custom type adapters and JSONRPCUtils for Protocol Buffer integration. It also enhances thread safety in Client and transport implementations using double-checked locking. Feedback highlights critical compilation issues due to missing constructors in several error classes and identifies opportunities to optimize JSON processing by eliminating redundant serialization cycles. Additionally, a potential protocol violation in server-side error mapping was noted.
| throw new IllegalArgumentException("Unknown message type"); | ||
| private void handleMessage(JsonObject jsonObject, Future<Void> future) throws JsonProcessingException { | ||
| if (jsonObject.has("error")) { | ||
| JSONRPCError error = JsonUtil.fromJson(jsonObject.get("error").toString(), JSONRPCError.class); |
There was a problem hiding this comment.
Using toString() on a JsonElement followed by JsonUtil.fromJson is inefficient as it involves an unnecessary serialization and re-parsing cycle. Gson can deserialize directly from a JsonElement object using the fromJson(JsonElement, Class) method.
| JSONRPCError error = JsonUtil.fromJson(jsonObject.get("error").toString(), JSONRPCError.class); | |
| JSONRPCError error = JsonUtil.OBJECT_MAPPER.fromJson(jsonObject.get("error"), JSONRPCError.class); |
| String resultJson = jsonObject.get("result").toString(); | ||
| StreamingEventKind event = JsonUtil.fromJson(resultJson, StreamingEventKind.class); |
There was a problem hiding this comment.
Similar to the error handling above, converting the result element to a string before parsing is inefficient. You can use JsonUtil.OBJECT_MAPPER.fromJson with the JsonElement directly to avoid the overhead.
| String resultJson = jsonObject.get("result").toString(); | |
| StreamingEventKind event = JsonUtil.fromJson(resultJson, StreamingEventKind.class); | |
| StreamingEventKind event = JsonUtil.OBJECT_MAPPER.fromJson(jsonObject.get("result"), StreamingEventKind.class); |
| String methodName = (method != null && method.isTextual()) ? method.asText() : null; | ||
| if (methodName != null) { | ||
| context.getState().put(METHOD_NAME_KEY, methodName); | ||
| com.google.gson.JsonObject node = JsonParser.parseString(body).getAsJsonObject(); |
There was a problem hiding this comment.
Calling getAsJsonObject() directly on the result of JsonParser.parseString(body) will throw an IllegalStateException if the request body is valid JSON but not an object. This exception would be caught by the generic Throwable block and return an Internal Error (-32603). According to the A2A protocol specification (JSON-RPC 2.0), such cases should return an Invalid Request error (-32600).
com.google.gson.JsonElement element = JsonParser.parseString(body);
if (!element.isJsonObject()) {
throw new InvalidRequestError(null, "Invalid JSON-RPC request: expected a JSON object", null);
}
com.google.gson.JsonObject node = element.getAsJsonObject();References
- Adhere to the A2A protocol specification for error code mappings, even if other mappings seem more internally consistent.
| // Check if it has the structure of a serialized Throwable (type + message) | ||
| if (obj.has(TYPE_FIELD) && obj.has(MESSAGE_FIELD) && obj.has(THROWABLE_MARKER_FIELD)) { | ||
| // Deserialize as Throwable using ThrowableTypeAdapter | ||
| yield THROWABLE_ADAPTER.read(new JsonReader(new StringReader(element.toString()))); |
There was a problem hiding this comment.
Using element.toString() and StringReader to re-parse a JsonElement is inefficient. Since element is already a JsonObject, you can use THROWABLE_ADAPTER.fromJsonTree(element) to deserialize it directly.
| yield THROWABLE_ADAPTER.read(new JsonReader(new StringReader(element.toString()))); | |
| yield THROWABLE_ADAPTER.fromJsonTree(element); |
Eliminate inefficient toString() serialization followed by fromJson() parsing cycles by using Gson's direct JsonElement deserialization. Replace three instances of element.toString() + fromJson(String) with direct fromJson(JsonElement) calls in SSEEventListener and JsonUtil. Fix JSON-RPC 2.0 protocol compliance in A2AServerRoutes: validate that request body is a JSON object before calling getAsJsonObject(). Non-object JSON now correctly returns InvalidRequestError (-32600) instead of InternalError (-32603). Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
Closing in favor of #807 |
Uh oh!
There was an error while loading. Please reload this page.