-
Notifications
You must be signed in to change notification settings - Fork 63
feat: add support for JSON+HTTP/REST #229
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
base: main
Are you sure you want to change the base?
Conversation
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.
Summary of Changes
Hello @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 introduces a significant refactoring of the client-side A2A SDK, shifting from monolithic client implementations to a modular architecture that supports multiple transport protocols. It establishes a flexible client factory, abstract client interfaces, and dedicated transport implementations for JSON-RPC, gRPC, and a new HTTP+JSON REST client. This change enhances the SDK's extensibility and maintainability by decoupling transport logic from core client functionalities.
Highlights
- Modular Client Architecture: The old monolithic client implementations ('A2AClient' and 'A2AGrpcClient') have been replaced by a flexible 'AbstractClient' and 'Client' structure, supported by a 'ClientFactory' and 'ClientTransport' interfaces. This significantly improves modularity and extensibility.
- Multi-Transport Protocol Support: New dedicated transport implementations for JSON-RPC, gRPC, and a new HTTP+JSON REST client have been introduced. This allows the SDK to dynamically select the appropriate communication protocol based on agent capabilities.
- Enhanced Client-Side Event Handling: A new event processing mechanism, including 'ClientEvent' and 'ClientTaskManager', ensures consistent handling of messages, tasks, and updates across all supported transport protocols.
- Standardized Agent Card Path: The standard path for agent card discovery has been updated from '.well-known/agent.json' to '.well-known/agent-card.json' for consistency.
- Integration of New Protocol Features: The SDK now includes support for new protocol features such as Agent Card Signatures and Mutual TLS security schemes, enhancing security and verification capabilities.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command>
or @gemini-code-assist <command>
. Below is a summary of the supported commands.
Feature | Command | Description |
---|---|---|
Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
Help | /gemini help |
Displays a list of available commands. |
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 .gemini/
folder in the base of the repository. Detailed instructions can be found here.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
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 major refactoring of the client-side implementation, abstracting the transport layer to support JSON-RPC, gRPC, and a new REST+JSON transport. This is a significant architectural improvement that enhances modularity and extensibility. The changes include removing the old monolithic clients and introducing a transport-agnostic Client
with a ClientFactory
and SPI for transport providers. The protobuf definitions and spec classes have been updated to align with version 0.3.0 of the A2A protocol.
My review focuses on the new REST+JSON transport implementation. I've found that it is currently incomplete, with several methods throwing UnsupportedOperationException
. There are also some areas for improvement regarding logging consistency and error handling. Overall, this is a great step forward for the SDK's architecture.
client/src/main/java/io/a2a/client/transport/JSONRestTransport.java
Outdated
Show resolved
Hide resolved
client/src/main/java/io/a2a/client/transport/JSONRestTransport.java
Outdated
Show resolved
Hide resolved
client/src/main/java/io/a2a/client/sse/JSONRestSSEEventListener.java
Outdated
Show resolved
Hide resolved
client/src/test/java/io/a2a/client/transport/JSONRestTransportTest.java
Outdated
Show resolved
Hide resolved
7e4ec93
to
a72f662
Compare
client/src/main/java/io/a2a/client/transport/JSONRestTransport.java
Outdated
Show resolved
Hide resolved
/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 major refactoring of the A2A client, abstracting the transport layer to support multiple protocols like JSON-RPC, gRPC, and a new REST+JSON implementation. The changes are extensive, including the removal of old client classes and the addition of a new modular structure with a client factory, abstract client, and transport-specific implementations. The code is also updated to align with a newer version of the A2A specification. My feedback focuses on a couple of minor issues in the new implementation, such as a parameter typo and leftover debugging code. Overall, this is a significant improvement to the SDK's architecture and capabilities.
client/src/main/java/io/a2a/client/transport/JSONRestTransport.java
Outdated
Show resolved
Hide resolved
@ehsavoie Just FYI, I've merged the v0.3.0 PR so this one can be rebased now. |
67c8163
to
13367c4
Compare
/gemini-review |
It looks like you might have intended to use the |
eb68867
to
9700c25
Compare
9ffdeb5
to
83add84
Compare
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.
Thanks @ehsavoie! I'm starting to review this so adding an initial set of comments.
...ansport/jsonrest/src/main/java/io/a2a/client/transport/jsonrest/JSONRestTransportConfig.java
Outdated
Show resolved
Hide resolved
client/transport/jsonrest/src/main/java/io/a2a/client/transport/jsonrest/JSONRestTransport.java
Outdated
Show resolved
Hide resolved
client/transport/jsonrest/src/main/java/io/a2a/client/transport/jsonrest/JSONRestTransport.java
Outdated
Show resolved
Hide resolved
client/transport/jsonrest/src/main/java/io/a2a/client/transport/jsonrest/JSONRestTransport.java
Outdated
Show resolved
Hide resolved
client/transport/jsonrest/src/main/java/io/a2a/client/transport/jsonrest/JSONRestTransport.java
Outdated
Show resolved
Hide resolved
client/transport/jsonrest/src/main/java/io/a2a/client/transport/jsonrest/JSONRestTransport.java
Outdated
Show resolved
Hide resolved
...transport/jsonrest/src/test/java/io/a2a/client/transport/jsonrest/JSONRestTransportTest.java
Outdated
Show resolved
Hide resolved
server-common/src/main/java/io/a2a/server/requesthandlers/RequestHandler.java
Outdated
Show resolved
Hide resolved
transport/rest/src/main/java/io/a2a/transport/rest/handler/RestHandler.java
Show resolved
Hide resolved
transport/rest/src/main/java/io/a2a/transport/rest/handler/RestHandler.java
Outdated
Show resolved
Hide resolved
transport/rest/src/main/java/io/a2a/transport/rest/handler/RestHandler.java
Show resolved
Hide resolved
Question: |
@priyanshus1 it covers all the rest part (client and server) |
1ac5baa
to
4a70fbf
Compare
@ehsavoie Do we know an ETA on this PR, we are waiting for either adopt this or self implement :) |
@priyanshus1 We're aiming to get it in for the next release (hopefully later this week) |
a45d770
to
ed6f73a
Compare
d3c1bb0
to
00a73d3
Compare
393a6a1
to
87f2869
Compare
* Create JSON+HTTP/REST Client. * Integrate PR from @ronantakizawa using regexp for server routing. * Update server code to use proper JSON (de)serialization with Proto. * Create Reference Implementation * Removing the listTasks since it has been removed from the specs * Removing io.grpc:* as direct dependencies for a2a-java-sdk-spec-grpc. * Support client preferences. * Running the TCK Signed-off-by: Emmanuel Hugonnet <[email protected]>
Uh oh!
There was an error while loading. Please reload this page.