-
Notifications
You must be signed in to change notification settings - Fork 92
feat: Provide a new http-client layer to create new HTTP client imple… #347
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
Summary of ChangesHello @brasseld, 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 significantly refactors the HTTP client infrastructure within the SDK by introducing a new, extensible abstraction layer. This change decouples the client logic from specific HTTP client implementations, providing a default JDK-based client and a new optional Vertx-based client. It also centralizes the management of HTTP client instances on the server-side and enhances the handling of Server-Sent Events, making the SDK more adaptable and robust for various application environments. 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
|
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 new abstraction layer for HTTP clients, allowing for different implementations like the newly added Vert.x-based client alongside the existing JDK one. The refactoring is extensive and touches many parts of the client and transport layers. My review focuses on ensuring the new abstraction is used correctly and the new implementations are robust. I've found a critical issue in the JSON-RPC transport related to agent card resolution, and a few medium-severity issues in the new Vert.x and JDK HTTP client implementations concerning SSE parsing and exception handling.
client/transport/jsonrpc/src/main/java/io/a2a/client/transport/jsonrpc/JSONRPCTransport.java
Outdated
Show resolved
Hide resolved
extras/http-client-vertx/src/main/java/io/a2a/client/http/vertx/VertxHttpClient.java
Show resolved
Hide resolved
extras/http-client-vertx/src/main/java/io/a2a/client/http/vertx/sse/SSEHandler.java
Outdated
Show resolved
Hide resolved
e1232f2 to
f6d7f34
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 by creating a new http-client layer, making the HTTP client implementation pluggable. This is a great architectural improvement that will enhance modularity and testability. The introduction of a Vert.x-based implementation as an alternative to the default JDK one is a good example of this new flexibility. My review focuses on ensuring the new abstraction is used correctly and identifying potential issues in the new implementations. I've found a critical issue in A2ACardResolver's usage of the new API, a bug in the new Vert.x SSE handler, and some minor areas for code cleanup. Overall, this is a solid contribution.
http-client/src/main/java/io/a2a/client/http/A2ACardResolver.java
Outdated
Show resolved
Hide resolved
extras/http-client-vertx/src/main/java/io/a2a/client/http/vertx/sse/SSEHandler.java
Show resolved
Hide resolved
client/transport/jsonrpc/src/main/java/io/a2a/client/transport/jsonrpc/JSONRPCTransport.java
Outdated
Show resolved
Hide resolved
| when(mockHttpClient.createPost()).thenReturn(mockPostBuilder); | ||
| when(mockPostBuilder.url(any(String.class))).thenReturn(mockPostBuilder); | ||
| when(mockHttpClient.post(any())).thenReturn(mockPostBuilder); | ||
| // when(mockPostBuilder.url(any(String.class))).thenReturn(mockPostBuilder); |
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.
f6d7f34 to
26001ef
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 well-structured refactoring to create an abstraction layer for HTTP clients, allowing for multiple implementations like the default JDK-based one and a new Vert.x-based one. The changes are extensive but follow a clear pattern, improving modularity and extensibility. I've identified a critical issue in the new Vert.x SSE handler that needs to be addressed, along with a couple of medium-severity suggestions for code improvement and consistency. Overall, this is a great enhancement to the SDK.
extras/http-client-vertx/src/main/java/io/a2a/client/http/vertx/sse/SSEHandler.java
Show resolved
Hide resolved
client/transport/jsonrpc/src/main/java/io/a2a/client/transport/jsonrpc/JSONRPCTransport.java
Outdated
Show resolved
Hide resolved
client/transport/jsonrpc/src/main/java/io/a2a/client/transport/jsonrpc/JSONRPCTransport.java
Show resolved
Hide resolved
26001ef to
f7fa2bf
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 well-designed refactoring of the HTTP client layer. The new HttpClient abstraction, along with the HttpClientBuilder, provides a clean and extensible way to support different HTTP client implementations, as demonstrated by the new Vert.x client module. The introduction of AbstractClientTransport to centralize interceptor logic and HttpClientManager for efficient client reuse are also excellent improvements.
However, I've found a critical issue in both JSONRPCTransport and RestTransport where the agentPath is incorrectly used instead of the full agentUrl when resolving the agent card. This will cause getAgentCard() to fail. I've left specific comments with suggestions on how to fix this.
Overall, this is a great contribution that significantly improves the architecture of the client. Once the identified issue is resolved, this will be ready to merge.
| try { | ||
| if (agentCard == null) { | ||
| resolver = new A2ACardResolver(httpClient, agentUrl, null, getHttpHeaders(context)); | ||
| A2ACardResolver resolver = new A2ACardResolver(httpClient, agentPath, null, getHttpHeaders(context)); |
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.
A2ACardResolver is being initialized with agentPath, but it requires the full base agentUrl to correctly resolve the agent card's location. This will cause requests for the agent card to fail.
To fix this, you should store the original agentUrl as a field in the JSONRPCTransport constructor and use that field here.
| A2ACardResolver resolver = new A2ACardResolver(httpClient, agentPath, null, getHttpHeaders(context)); | |
| A2ACardResolver resolver = new A2ACardResolver(httpClient, agentUrl, null, getHttpHeaders(context)); |
| try { | ||
| if (agentCard == null) { | ||
| resolver = new A2ACardResolver(httpClient, agentUrl, null, getHttpHeaders(context)); | ||
| resolver = new A2ACardResolver(httpClient, agentPath, null, getHttpHeaders(context)); |
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.
A2ACardResolver is being initialized with agentPath, but it requires the full base agentUrl to correctly resolve the agent card's location. This will cause requests for the agent card to fail.
To fix this, you should store the original agentUrl as a field in the RestTransport constructor and use that field here.
| resolver = new A2ACardResolver(httpClient, agentPath, null, getHttpHeaders(context)); | |
| resolver = new A2ACardResolver(httpClient, agentUrl, null, getHttpHeaders(context)); |
f7fa2bf to
b877ee2
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 by abstracting the HTTP client layer. This improves modularity and allows for different client implementations like the new Vert.x-based one. The introduction of AbstractClientTransport to share interceptor logic is also a great improvement for code reuse.
My review focuses on a few areas to further enhance robustness and performance. I've pointed out a fragile URL parsing implementation in the new JDK client and a potential runtime issue with handling response bodies in streaming scenarios. I've also suggested making a server-side component fully non-blocking to improve scalability. Finally, there's a small suggestion to reduce code duplication.
Overall, this is a solid architectural improvement.
extras/http-client-vertx/src/main/java/io/a2a/client/http/vertx/VertxHttpClient.java
Show resolved
Hide resolved
client/transport/jsonrpc/src/main/java/io/a2a/client/transport/jsonrpc/JSONRPCTransport.java
Show resolved
Hide resolved
b877ee2 to
d0b7c29
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 by creating a new http-client abstraction layer. This change greatly improves modularity, allowing for different HTTP client implementations like the newly added Vert.x-based client. The introduction of AbstractClientTransport to centralize interceptor logic and HttpClientManager for efficient client reuse are excellent design choices. While the overall direction is very positive, I've identified a few areas for improvement related to asynchronous design consistency and potential resource management issues that would further strengthen this new architecture.
client/transport/rest/src/main/java/io/a2a/client/transport/rest/sse/RestSSEEventListener.java
Outdated
Show resolved
Hide resolved
client/transport/jsonrpc/src/main/java/io/a2a/client/transport/jsonrpc/JSONRPCTransport.java
Show resolved
Hide resolved
client/transport/rest/src/main/java/io/a2a/client/transport/rest/RestTransport.java
Show resolved
Hide resolved
d0b7c29 to
d5e4ff5
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 well-executed refactoring to provide a new HTTP client abstraction layer. This is a great step towards making the transport layer more extensible. The introduction of HttpClient and HttpClientBuilder interfaces, along with the default JDK implementation and a new Vert.x implementation, is well-designed. The refactoring of existing code to adopt this new layer is thorough. My review has identified a critical compilation issue due to missing imports, a missing nullability annotation that could affect code safety, and a minor naming inconsistency. Addressing these points will further improve the quality of this contribution.
client/transport/jsonrpc/src/main/java/io/a2a/client/transport/jsonrpc/JSONRPCTransport.java
Outdated
Show resolved
Hide resolved
client/transport/rest/src/main/java/io/a2a/client/transport/rest/RestTransportProvider.java
Outdated
Show resolved
Hide resolved
…mentation / extensions (vertx)
d5e4ff5 to
20392a0
Compare
|
I'm trying to add support for SSE in vertx-web vert-x3/vertx-web#2794 that I will backport to vertx 4 once I can get it merged. It should be more appropriate to handle this. WDYT ? |
Good one @ehsavoie , definitely something which was missing in vertx-web ! My only (small) concern is that it will force us to use vertx-web, this PR was only relying on vertx-core. |
|
I've anticipated a bit what could come after this PR regarding the Async Client. |
…entation to the transport layer (jsonrpc / rest+json)
The JDK-based implementation is kept as the reference implementation and a new extra module has been created to also provide a Vertx-based implementation which may be more suitable in the future.
Also, because of the non-blocking nature of Vertx, the current vertx implementation is not optimal (yet), and so, not suitable for production. Some additional work will be needed in the future to make it fully functional. What's needed:
Description
Thank you for opening a Pull Request!
Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
CONTRIBUTINGGuide.fix:which represents bug fixes, and correlates to a SemVer patch.feat:represents a new feature, and correlates to a SemVer minor.feat!:, orfix!:,refactor!:, etc., which represent a breaking change (indicated by the!) and will result in a SemVer major.