refactor: refactor base client#784
Conversation
…guglielmo-san/a2a-python into guglielmoc/refactor_base_client
Summary of ChangesHello, 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 client-side API by centralizing the management of service-level parameters, such as extensions, within a dedicated Highlights
Changelog
Activity
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.
Code Review
This pull request refactors how client-side extensions are handled by introducing ServiceParameters within the ClientCallContext. This is a good architectural improvement that centralizes service-level parameters and cleans up method signatures across the client and transport layers.
However, there are a few significant points to consider:
- Breaking API Change: The
send_messagemethod signature has been changed to accept aSendMessageRequestobject instead of aMessageand other parameters. This is a major breaking change for users of the SDK and should be clearly communicated. - Interceptor Removal: The refactoring appears to have removed support for
ClientCallInterceptorfrom theJsonRpcTransport. This is another significant breaking change that is not mentioned in the PR description. The skipping of interceptor-related tests confirms this. This needs to be addressed, either by restoring the functionality or by explicitly documenting its removal. - Potential Bug: I've identified a potential logic reversal in
base_client.pyfor handling theblockingparameter in polling mode, which could lead to incorrect behavior.
I've left specific comments on the code with more details. Overall, the direction of the refactoring is positive, but these critical issues should be resolved before merging.
Note: Security Review did not run due to the size of the PR.
I am having trouble creating individual review comments. Click here to see my feedback.
src/a2a/client/transports/jsonrpc.py (436-457)
The _apply_interceptors method has been removed from this transport. This is a significant breaking change as it appears to remove support for client-side call interceptors for the JsonRpcTransport. This was not mentioned in the pull request description. If this removal was intentional, it should be documented. If it was accidental, it should be restored.
src/a2a/client/base_client.py (98-99)
There appears to be a logic reversal in how the blocking configuration is applied. The previous implementation set blocking=not self._config.polling, meaning if polling is enabled, the call is non-blocking. The new logic here appears to set blocking=True when polling is enabled, which is the opposite. This could be a bug.
src/a2a/client/transports/rest.py (366-392)
There is an inconsistency in how headers are handled between _execute_request and _send_stream_request. The _send_stream_request method correctly merges headers from self.httpx_client with request-specific headers. However, _execute_request only uses headers from the context (via _get_http_args) and does not include the base client headers from self.httpx_client.headers. This could lead to missing headers (e.g., A2A-Version) in non-streaming REST requests.
To ensure consistency, _execute_request should also merge the base client headers.
async def _execute_request(
self,
method: str,
target: str,
tenant: str,
payload: dict[str, Any] | None = None,
context: ClientCallContext | None = None,
) -> dict[str, Any]:
path = self._get_path(target, tenant)
http_kwargs = self._get_http_args(context)
payload = payload or {}
headers = dict(self.httpx_client.headers.items())
headers.update(http_kwargs.get('headers', {}))
timeout = http_kwargs.get('timeout', httpx.USE_CLIENT_DEFAULT)
json_payload = payload if method == 'POST' else None
params = payload if method != 'POST' else None
request = self.httpx_client.build_request(
method,
f'{self.url}{path}',
json=json_payload,
params=params,
headers=headers, # type: ignore[arg-type]
timeout=timeout, # type: ignore[arg-type]
)
return await self._send_request(request)tests/client/transports/test_jsonrpc_client.py (651-688)
This test contains a large block of commented-out code which seems to be from a previous implementation or debugging session. It would be best to remove this commented-out code to improve test readability.
Description
This PR refactors the base_client API definitions, introducing a new object to store the extensions, avoiding passing it as external parameter.
Interceptors have been removed as their logic will be changed in next PR