-
Notifications
You must be signed in to change notification settings - Fork 110
feat!: Updating to newest proto to align with 1.0 Draft specifications #484
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
fb75b4d to
729bc88
Compare
client/transport/rest/src/main/java/io/a2a/client/transport/rest/RestTransport.java
Outdated
Show resolved
Hide resolved
|
It might be good to add some info in the PR description or commit message to indicate the commit hash that was used when copying the .proto file to this repo. |
431152c to
e879e8c
Compare
- a2a.proto from commit b0784199543eebf2e95dcb02e9336cb213923506 - use "Dskip.protobuf.generate=false" to generate source code from a2a.proto Signed-off-by: Emmanuel Hugonnet <ehugonne@redhat.com>
The spec-grpc classes are ugly, and we can't let users use them. When using MapStruct an effort has been made to not use 'manual' conversion (i.e. a 'default' implementation of the Mapper interface methods). When automatic conversion happens, when the spec-grpc and spec classes differ a compile error will be raised. Which in turn helps us keep our manually generated spec classes in sync with the spec-grpc ones.
cd1eaf6 to
6a2441f
Compare
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a significant and valuable refactoring to align the codebase with the latest protobuf specification. Key changes include the simplification of the AgentCard data model by replacing url, preferredTransport, and additionalInterfaces with a unified supportedInterfaces list. This greatly improves clarity. Another major improvement is the refactoring of the JSON-RPC transport to use protobuf data models internally, which brings consistency across all transport implementations (gRPC, REST, and JSON-RPC) and reduces code duplication. The build process for protobuf generation has also been modernized. Overall, these changes enhance maintainability and consistency. I have a couple of suggestions to further improve readability by using modern Java language features.
reference/jsonrpc/src/main/java/io/a2a/server/apps/quarkus/A2AServerRoutes.java
Show resolved
Hide resolved
reference/jsonrpc/src/main/java/io/a2a/server/apps/quarkus/A2AServerRoutes.java
Show resolved
Hide resolved
|
|
||
| public JSONRPCTransport(AgentCard agentCard) { | ||
| this(null, agentCard, agentCard.url(), null); | ||
| this(null, agentCard, agentCard.supportedInterfaces().get(0).url(), null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can supportedInterfaces() be empty?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kabir ^^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to https://a2a-protocol.org/latest/definitions/, it can be empty:
message AgentCard {
[...]
// Ordered list of supported interfaces. First entry is preferred.
repeated AgentInterface supported_interfaces = 19;
}
repeated means 0 or more
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ehsavoie I guess if null/empty we should throw some kind of exception
client/transport/jsonrpc/src/main/java/io/a2a/client/transport/jsonrpc/JSONRPCTransport.java
Outdated
Show resolved
Hide resolved
client/transport/jsonrpc/src/test/java/io/a2a/client/transport/jsonrpc/JsonMessages.java
Show resolved
Hide resolved
client/transport/jsonrpc/src/test/java/io/a2a/client/transport/jsonrpc/JsonMessages.java
Show resolved
Hide resolved
client/transport/jsonrpc/src/test/java/io/a2a/client/transport/jsonrpc/JsonMessages.java
Outdated
Show resolved
Hide resolved
| /** | ||
| * Converts domain DataPart to proto DataPart. | ||
| * Uses CommonFieldMapper for Map → Struct conversion. | ||
| * Metadata is ignored (not part of proto definition). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does that mean that there can't be metadata attached to a data part?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kabir maybe you can answer there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it seems that the DataPartmessage is protobuf does not have metadata according to https://github.com/a2aproject/A2A/blob/b6ef9eec558c877fb69024df090a8bb63c542a1c/specification/grpc/a2a.proto#L228C9-L228C17.
The spec does not mention them either: https://a2a-protocol.org/latest/specification/#418-datapart
So is this something that was present in a previous spec but is no longer relevant?
Now it seems that metadata can only be attached to artifacts and not to their parts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That'd make sense to me to be able to attach metadata to parts (eg for a file, it would allow to put its creation/update date which would not make sense if it was at the artifact level)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I created #497 to track this, and made it a P0 issue in https://github.com/orgs/a2aproject/projects/4. I don't think this needs to block the merge
dee90e7 to
e81d66f
Compare
* Update AgentCard and transport discovery. Removing the deprecated fields for now * Added ProtoUtils.FromProto.agentCard() method to convert io.a2a.grpc.AgentCard to io.a2a.spec.AgentCard. * Fixing the AGENT_CARD test constant to use the correct protobuf JSON format * Updated ProtoUtils.ToProto and ProtoUtils.FromProto * Update streaming test JSON to protobuf format * Update SSE listener to use proto JSON instead of Jackson * Updating and renaming all JSON-RPC method names to the new standard. * Fixing NullSpecify * Fixing Making unmarshalResponse type-safe * Fixing cloud example * Fixing streaming tests with http client * Fixing issues with configId * Fixing issues with missing id in jsonrpc requests. * Add comprehensive tests for JSONRPCUtils * Update protocol version to 1.0.0 * Unifying spec/grpc name clashes Signed-off-by: Emmanuel Hugonnet <ehugonne@redhat.com>
e81d66f to
48aa033
Compare
a2aproject#484) [Feat]: Use protobuf as the serialization mechanism Fixes #<issue_number_goes_here> 🦕 --------- Signed-off-by: Emmanuel Hugonnet <ehugonne@redhat.com> Co-authored-by: Kabir Khan <kkhan@redhat.com>
[Feat]: Use protobuf as the serialization mechanism
Fixes #<issue_number_goes_here> 🦕