refactor: Complete migration from Jackson to Gson across codebase#514
refactor: Complete migration from Jackson to Gson across codebase#514jmesnil merged 1 commit intoa2aproject:mainfrom
Conversation
Summary of ChangesHello @ehsavoie, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request completes a significant refactoring effort to transition the entire codebase from using Jackson for JSON processing to Gson. The primary goal is to remove the dependency on Jackson and gain more granular control over serialization, especially for complex and polymorphic types within the A2A Protocol. This change impacts all client, server, and utility modules that handle JSON data, ensuring a consistent and robust serialization layer across the SDK. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This is an impressive and extensive refactoring to migrate the entire codebase from Jackson to Gson. The introduction of JsonUtil with custom TypeAdapters is a solid approach to abstracting the JSON library and handling the domain model's serialization complexities. The new and updated tests, especially TaskSerializationTest, provide good confidence in the new implementation.
My review has identified a few minor but important issues related to the migration. These include a potential data loss issue with numeric ID serialization, a missing annotation for a field name that is a Java keyword, and an overly broad exception catch. Addressing these points will help ensure the new implementation is fully robust and correct.
server-common/src/main/java/io/a2a/server/tasks/BasePushNotificationSender.java
Show resolved
Hide resolved
ehsavoie
force-pushed
the
removing_jackson
branch
2 times, most recently
from
5b7cee7 to
f367911
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request migrates the project's JSON processing library from Jackson to Gson. This involved replacing all direct usages of Jackson's ObjectMapper and related classes with io.a2a.json.JsonUtil methods for serialization and deserialization. Custom JsonProcessingException and JsonMappingException classes were introduced in io.a2a.json to provide a library-agnostic exception hierarchy. Consequently, many import statements were updated, catch blocks were modified to handle the new exception types, and pom.xml files were adjusted to remove Jackson dependencies and add Gson. Additionally, custom Gson TypeAdapter implementations were added in JsonUtil to handle polymorphic types (like Part, StreamingEventKind, FileContent, JSONRPCError), enum serialization (TaskState, Message.Role, Part.Kind), and OffsetDateTime formatting. A review comment highlighted that the JSONRPCUtils class in spec-grpc was incorrectly converting numeric requestId values to strings during JSON-RPC error and result response serialization, which should be corrected to preserve the original numeric type for spec compliance.
| output.name("id").value(string); | ||
| } else if (requestId instanceof Number number) { | ||
| output.name("id").value(number); | ||
| output.name("id").value(Integer.toString(number.intValue())); |
There was a problem hiding this comment.
Similar to the previous comment, this change converts a numeric requestId to a string, altering its type in the JSON output. To maintain type consistency with the JSON-RPC specification, it's better to serialize numbers as numbers.
| output.name("id").value(Integer.toString(number.intValue())); | |
| output.name("id").value(number); |
There was a problem hiding this comment.
With the suggested change the integer is serialized as 1.0 which is deserialized as a double and not an int
kabir
left a comment
kabir
left a comment
There was a problem hiding this comment.
Looks good, apart from the FQN nitpicks.
I'm not totally sure about the integer handling though. It would be good to fix that, but if impossible feel free to merge.
| assertNotNull(retrieved.getMetadata()); | ||
| assertEquals("value1", retrieved.getMetadata().get("key1")); | ||
| assertEquals(42, retrieved.getMetadata().get("key2")); | ||
| assertEquals(42, ((Number)retrieved.getMetadata().get("key2")).intValue()); |
There was a problem hiding this comment.
This makes it look like it serialises {"key2": "42"} instead of {"key2"; 42}. Which might be fine, but also a bit strange
reference/jsonrpc/src/main/java/io/a2a/server/apps/quarkus/A2AServerRoutes.java
Outdated
Show resolved
Hide resolved
server-common/src/main/java/io/a2a/server/tasks/BasePushNotificationSender.java
Show resolved
Hide resolved
ehsavoie
force-pushed
the
removing_jackson
branch
from
f367911 to
7ca6b8b
Compare
jmesnil
left a comment
jmesnil
left a comment
There was a problem hiding this comment.
The removal of Jackson sounds good (it just needs to be removed from the parent pom too) but I'm not sure that the serialisation is correct (eg the kind field)
In the tests, we verify that we can unserialize/deserialize the object but the JSON strings does not comply with the 1.0 spec
More generally, there are too many unnecessary fields that complicates the API (code in Error, kind in payload, type in other classes
I also wonder if the domain class that have String values (Role, TaskState) should be updated to use the same values that the spec.
| public A2AHttpResponse post() throws IOException, InterruptedException { | ||
| tasks.add(Utils.OBJECT_MAPPER.readValue(body, Task.TYPE_REFERENCE)); | ||
| try { | ||
| System.out.println("TestPostBuilder.post() body: " + body); |
server-common/src/test/java/io/a2a/server/requesthandlers/AbstractA2ARequestHandlerTest.java
Outdated
Show resolved
Hide resolved
| System.out.println("TestPostBuilder.post() body: " + body); | ||
| Task task = JsonUtil.fromJson(body, Task.class); | ||
| tasks.add(task); | ||
| System.out.println("Successfully parsed task: " + task.getId()); |
| output.name("id").value(string); | ||
| } else if (requestId instanceof Number number) { | ||
| output.name("id").value(number); | ||
| output.name("id").value(number.intValue()); |
There was a problem hiding this comment.
I'm not sure that an int is correct here? What if the id is bigger than Integer.MAX?
Should we use instead a long?
There was a problem hiding this comment.
yes, should be better but the ID is supposed to be a String or an Integer
There was a problem hiding this comment.
is it?
According to https://www.jsonrpc.org/specification#request_object:
An identifier established by the Client that MUST contain a String, Number, or NULL value if included. If it is not included it is assumed to be a notification. The value SHOULD normally not be Null [1] and Numbers SHOULD NOT contain fractional parts [2]
and a JSON Number is not tied to a Java int (https://www.json.org/json-en.html)
|
|
||
| // Verify JSON contains history data | ||
| assertTrue(json.contains("\"role\":\"user\"")); | ||
| assertTrue(json.contains("Test message")); |
| String json = JsonUtil.toJson(task); | ||
|
|
||
| // Verify state is serialized correctly | ||
| assertTrue(json.contains("\"state\":\"" + state.asString() + "\"")); |
There was a problem hiding this comment.
that's not correct, I would expect to get a value that is compliant with the spec
"state": "TASK_STATE_SUBMITTED"
and not
"state": "submitted"
There was a problem hiding this comment.
This is a domain object serialization, not a protocol payload
There was a problem hiding this comment.
and I'm wrong anyway, the json serialisation must use submitted according to https://a2a-protocol.org/latest/specification/#55-json-field-naming-convention
| String json = JsonUtil.toJson(task); | ||
|
|
||
| // Verify JSON contains file part data | ||
| assertTrue(json.contains("\"kind\":\"file\"")); |
There was a problem hiding this comment.
This is a domain object serialization, not a protocol payload
| assertTrue(deserializedFilePart.getFile() instanceof FileWithBytes); | ||
| FileWithBytes fileWithBytes = (FileWithBytes) deserializedFilePart.getFile(); | ||
| assertEquals("document.pdf", fileWithBytes.name()); | ||
| assertEquals("application/pdf", fileWithBytes.mimeType()); |
There was a problem hiding this comment.
btw, mimeType should be renamed to mediaType (https://a2a-protocol.org/latest/specification/#417-filepart)
There was a problem hiding this comment.
This is a domain object serialization, not a protocol payload
| String json = JsonUtil.toJson(task); | ||
|
|
||
| // Verify JSON contains data part | ||
| assertTrue(json.contains("\"kind\":\"data\"")); |
|
another note on my review, I've been working on updating the TCK after #484 and a lot of tests passes so we are able to receive/return valid JSON from our transports. My comments about producing invalid JSON payload that is not compliant to the spec is not correct. |
|
one thing that trips me up is that all my comments are for the "internal" JSON payload that is not used by clients or server but is exposed by the API through the io.a2a.util.Utils class. |
ehsavoie
force-pushed
the
removing_jackson
branch
3 times, most recently
from
9a784fe to
eb95bdb
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This is an impressive and comprehensive pull request that successfully migrates the entire codebase from Jackson to Gson. The creation of JsonUtil with custom TypeAdapters to handle polymorphic types and other custom serialization logic is well-executed. The abstraction of JSON processing exceptions is also a great improvement for library independence. The new TaskSerializationTest is thorough and provides excellent coverage for the new serialization logic.
I've found a couple of potential issues in the new JsonUtil related to the implementation of the custom TypeAdapters. One is a high-severity issue regarding incomplete configuration of delegate Gson instances which could lead to incorrect deserialization of nested complex types. The other is a medium-severity issue about a fragile heuristic for detecting Throwable types. Addressing these will make the new serialization utility more robust.
Overall, this is a high-quality refactoring effort. Great work!
| private final Gson delegateGson = new GsonBuilder() | ||
| .registerTypeAdapter(OffsetDateTime.class, new OffsetDateTimeTypeAdapter()) | ||
| .registerTypeAdapter(TaskState.class, new TaskStateTypeAdapter()) | ||
| .registerTypeAdapter(Message.Role.class, new RoleTypeAdapter()) | ||
| .registerTypeAdapter(Part.Kind.class, new PartKindTypeAdapter()) | ||
| .registerTypeHierarchyAdapter(FileContent.class, new FileContentTypeAdapter()) | ||
| .create(); |
There was a problem hiding this comment.
The delegateGson instance created here to avoid recursion is missing some of the custom type adapters registered on the main OBJECT_MAPPER, specifically JSONRPCErrorTypeAdapter. If a Part contains a JSONRPCError in its structure (e.g., in metadata), it will be deserialized using Gson's default mechanism instead of your custom adapter, leading to incorrect deserialization of error subclasses.
The same issue exists in StreamingEventKindTypeAdapter on line 606.
To fix this, you should ensure the delegate Gson instances are configured with all necessary adapters, except for the one that would cause recursion. A good approach would be to centralize the GsonBuilder configuration and reuse it.
For example, you could have a helper method:
private static GsonBuilder createBaseGsonBuilder() {
return new GsonBuilder()
.setObjectToNumberStrategy(ToNumberPolicy.LONG_OR_DOUBLE)
.registerTypeAdapter(OffsetDateTime.class, new OffsetDateTimeTypeAdapter())
.registerTypeHierarchyAdapter(JSONRPCError.class, new JSONRPCErrorTypeAdapter())
.registerTypeAdapter(TaskState.class, new TaskStateTypeAdapter())
.registerTypeAdapter(Message.Role.class, new RoleTypeAdapter())
.registerTypeAdapter(Part.Kind.class, new PartKindTypeAdapter())
.registerTypeHierarchyAdapter(FileContent.class, new FileContentTypeAdapter());
}Then, the main OBJECT_MAPPER can be built like this:
public static final Gson OBJECT_MAPPER = createBaseGsonBuilder()
.registerTypeHierarchyAdapter(Part.class, new PartTypeAdapter())
.registerTypeHierarchyAdapter(StreamingEventKind.class, new StreamingEventKindTypeAdapter())
.create();And inside PartTypeAdapter, the delegate would be:
private final Gson delegateGson = createBaseGsonBuilder()
// Add adapters that PartTypeAdapter might need, e.g., StreamingEventKindTypeAdapter if nesting is possible
.registerTypeHierarchyAdapter(StreamingEventKind.class, new StreamingEventKindTypeAdapter())
.create();This ensures consistent adapter registration. You'll need to manage the recursive dependencies carefully.
This is a high severity issue because it can lead to data corruption or ClassCastExceptions at runtime if nested polymorphic types are used.
| if (obj.has("type") && obj.has("message") && obj.size() == 2) { | ||
| // Deserialize as Throwable using ThrowableTypeAdapter | ||
| yield THROWABLE_ADAPTER.read(new com.google.gson.stream.JsonReader( | ||
| new java.io.StringReader(element.toString()))); | ||
| } |
There was a problem hiding this comment.
The current heuristic for detecting a serialized Throwable in the data field (obj.has("type") && obj.has("message") && obj.size() == 2) is a bit fragile. Any other JSON object that happens to have exactly these two fields will be incorrectly deserialized as a Throwable.
A more robust approach could be to check if the value of the type field corresponds to a class that extends Throwable. While this involves reflection, it would prevent incorrect deserialization of valid data objects. For example:
try {
Class<?> dataClass = Class.forName(obj.get("type").getAsString());
if (Throwable.class.isAssignableFrom(dataClass)) {
// It's a throwable, deserialize it as such
yield THROWABLE_ADAPTER.read(new com.google.gson.stream.JsonReader(
new java.io.StringReader(element.toString())));
}
} catch (ClassNotFoundException e) {
// Not a known class, or not a throwable, proceed to default deserialization
}This is a conceptual suggestion. The implementation would need to be careful about performance and security if loading arbitrary classes. An alternative could be to add a specific marker field to serialized Throwables.
Replace Jackson ObjectMapper with Gson throughout the codebase to eliminate
Jackson dependency and provide custom serialization control.
Key Changes:
- Created JsonUtil.java with Gson-based OBJECT_MAPPER singleton
- Implemented 9 custom TypeAdapters for complex type handling:
* OffsetDateTimeTypeAdapter - ISO-8601 datetime serialization
* JSONRPCErrorTypeAdapter - Polymorphic error deserialization (12 error types)
* ThrowableTypeAdapter - Avoids Java 17+ reflection restrictions
* TaskStateTypeAdapter - Wire format enum serialization
* RoleTypeAdapter - Message.Role enum handling
* PartKindTypeAdapter - Part.Kind enum handling
* PartTypeAdapter - Polymorphic Part deserialization (Text/File/Data)
* StreamingEventKindTypeAdapter - Event type deserialization
* FileContentTypeAdapter - FileWithBytes/FileWithUri discrimination
- Added TaskSerializationTest with 21 comprehensive tests covering:
* Round-trip serialization for all Task components
* Direct JSON deserialization tests
* All Part types and FileContent variants
- Removed Jackson-specific deserializers and annotations
- Updated all usages across 142 files (client, server, extras, tests)
- Removed Jackson dependencies from spec/pom.xml
Technical Details:
- Thread-safe singleton OBJECT_MAPPER with all adapters registered
- Handles polymorphic types via two-pass parsing strategy
- Maintains wire format compatibility with existing JSON-RPC protocol
- Preserves null-safety with NullAway/JSpecify conventions
Breaking Changes: None - wire format remains compatible
Signed-off-by: Emmanuel Hugonnet <ehugonne@redhat.com>
ehsavoie
force-pushed
the
removing_jackson
branch
from
eb95bdb to
5a8e52b
Compare
jmesnil
left a comment
jmesnil
left a comment
There was a problem hiding this comment.
PR looks good to me in its current state.
However, I think we should clarify the exposure of the internal JSON serialisation in the API (through the Utils class).
If we need to use an internal wire format inside a2a-java, there is no reason to expose it in our user API.
| @JsonCreator | ||
| public HTTPAuthSecurityScheme(@JsonProperty("bearerFormat") String bearerFormat, @JsonProperty("scheme") String scheme, | ||
| @JsonProperty("description") String description, @JsonProperty("type") String type) { | ||
| public HTTPAuthSecurityScheme(String bearerFormat, String scheme, String description, String type) { |
There was a problem hiding this comment.
The ctor parameter is required or the type field?
| @JsonCreator | ||
| public SendMessageRequest(@JsonProperty("jsonrpc") String jsonrpc, @JsonProperty("id") Object id, | ||
| @JsonProperty("method") String method, @JsonProperty("params") MessageSendParams params) { | ||
| public SendMessageRequest(String jsonrpc, Object id, String method, MessageSendParams params) { |
|
@ehsavoie thanks! |
…aproject#514) Replace Jackson ObjectMapper with Gson throughout the codebase to eliminate Jackson dependency and provide custom serialization control. Key Changes: - Created JsonUtil.java with Gson-based OBJECT_MAPPER singleton - Implemented 9 custom TypeAdapters for complex type handling: * OffsetDateTimeTypeAdapter - ISO-8601 datetime serialization * JSONRPCErrorTypeAdapter - Polymorphic error deserialization (12 error types) * ThrowableTypeAdapter - Avoids Java 17+ reflection restrictions * TaskStateTypeAdapter - Wire format enum serialization * RoleTypeAdapter - Message.Role enum handling * PartKindTypeAdapter - Part.Kind enum handling * PartTypeAdapter - Polymorphic Part deserialization (Text/File/Data) * StreamingEventKindTypeAdapter - Event type deserialization * FileContentTypeAdapter - FileWithBytes/FileWithUri discrimination - Added TaskSerializationTest with 21 comprehensive tests covering: * Round-trip serialization for all Task components * Direct JSON deserialization tests * All Part types and FileContent variants - Removed Jackson-specific deserializers and annotations - Updated all usages across 142 files (client, server, extras, tests) - Removed Jackson dependencies from spec/pom.xml Technical Details: - Thread-safe singleton OBJECT_MAPPER with all adapters registered - Handles polymorphic types via two-pass parsing strategy - Maintains wire format compatibility with existing JSON-RPC protocol - Preserves null-safety with NullAway/JSpecify conventions Fixes a2aproject#487 🦕 Signed-off-by: Emmanuel Hugonnet <ehugonne@redhat.com>
3 participants
Replace Jackson ObjectMapper with Gson throughout the codebase to eliminate Jackson dependency and provide custom serialization control.
Key Changes:
- Created JsonUtil.java with Gson-based OBJECT_MAPPER singleton
- Implemented 9 custom TypeAdapters for complex type handling:
* OffsetDateTimeTypeAdapter - ISO-8601 datetime serialization
* JSONRPCErrorTypeAdapter - Polymorphic error deserialization (12 error types)
* ThrowableTypeAdapter - Avoids Java 17+ reflection restrictions
* TaskStateTypeAdapter - Wire format enum serialization
* RoleTypeAdapter - Message.Role enum handling
* PartKindTypeAdapter - Part.Kind enum handling
* PartTypeAdapter - Polymorphic Part deserialization (Text/File/Data)
* StreamingEventKindTypeAdapter - Event type deserialization
* FileContentTypeAdapter - FileWithBytes/FileWithUri discrimination
- Added TaskSerializationTest with 21 comprehensive tests covering:
* Round-trip serialization for all Task components
* Direct JSON deserialization tests
* All Part types and FileContent variants
- Removed Jackson-specific deserializers and annotations
- Updated all usages across 142 files (client, server, extras, tests)
- Removed Jackson dependencies from spec/pom.xml
Technical Details:
- Thread-safe singleton OBJECT_MAPPER with all adapters registered
- Handles polymorphic types via two-pass parsing strategy
- Maintains wire format compatibility with existing JSON-RPC protocol
- Preserves null-safety with NullAway/JSpecify conventions
Fixes #487 🦕