Conversation
Summary of ChangesHello @ishymko, 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 expands the existing push notification configuration management by adding capabilities to list all configurations for a given task and to delete specific configurations. This provides users with more granular control over their task-related push notifications, moving beyond just creation and retrieval of individual configurations. The changes span client interfaces, transport implementations, and server-side request handlers, ensuring consistent functionality across different communication protocols. 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
|
![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 pull request introduces functionality for listing and deleting task push notification configurations, with changes spanning the client, server, and all transport layers (gRPC, JSON-RPC, REST), complemented by new tests. The implementation is solid, but I've identified a few areas for improvement. There's significant code duplication in the new JSON-RPC transport methods that could be refactored. Additionally, I've found some minor inconsistencies in test mocks that should be corrected to better align with the production code's behavior.
| async def list_task_callback( | ||
| self, | ||
| request: ListTaskPushNotificationConfigRequest, | ||
| *, | ||
| context: ClientCallContext | None = None, | ||
| extensions: list[str] | None = None, | ||
| ) -> ListTaskPushNotificationConfigResponse: | ||
| """Lists push notification configurations for a specific task.""" | ||
| rpc_request = JSONRPC20Request( | ||
| method='ListTaskPushNotificationConfig', | ||
| params=json_format.MessageToDict(request), | ||
| _id=str(uuid4()), | ||
| ) | ||
| modified_kwargs = update_extension_header( | ||
| self._get_http_args(context), | ||
| extensions if extensions is not None else self.extensions, | ||
| ) | ||
| payload, modified_kwargs = await self._apply_interceptors( | ||
| 'ListTaskPushNotificationConfig', | ||
| cast('dict[str, Any]', rpc_request.data), | ||
| modified_kwargs, | ||
| context, | ||
| ) | ||
| response_data = await self._send_request(payload, modified_kwargs) | ||
| json_rpc_response = JSONRPC20Response(**response_data) | ||
| if json_rpc_response.error: | ||
| raise A2AClientJSONRPCError(json_rpc_response.error) | ||
| response: ListTaskPushNotificationConfigResponse = ( | ||
| json_format.ParseDict( | ||
| json_rpc_response.result, | ||
| ListTaskPushNotificationConfigResponse(), | ||
| ) | ||
| ) | ||
| return response | ||
|
|
||
| async def delete_task_callback( | ||
| self, | ||
| request: DeleteTaskPushNotificationConfigRequest, | ||
| *, | ||
| context: ClientCallContext | None = None, | ||
| extensions: list[str] | None = None, | ||
| ) -> None: | ||
| """Deletes the push notification configuration for a specific task.""" | ||
| rpc_request = JSONRPC20Request( | ||
| method='DeleteTaskPushNotificationConfig', | ||
| params=json_format.MessageToDict(request), | ||
| _id=str(uuid4()), | ||
| ) | ||
| modified_kwargs = update_extension_header( | ||
| self._get_http_args(context), | ||
| extensions if extensions is not None else self.extensions, | ||
| ) | ||
| payload, modified_kwargs = await self._apply_interceptors( | ||
| 'DeleteTaskPushNotificationConfig', | ||
| cast('dict[str, Any]', rpc_request.data), | ||
| modified_kwargs, | ||
| context, | ||
| ) | ||
| response_data = await self._send_request(payload, modified_kwargs) | ||
| json_rpc_response = JSONRPC20Response(**response_data) | ||
| if json_rpc_response.error: | ||
| raise A2AClientJSONRPCError(json_rpc_response.error) |
There was a problem hiding this comment.
These two new methods, list_task_callback and delete_task_callback, share a lot of boilerplate code for making a JSON-RPC request. This is a pattern repeated across other methods in this class as well. To improve maintainability and reduce code duplication, consider extracting this logic into a private helper method.
A helper method could handle creating the JSONRPC20Request, applying interceptors, sending the request, and handling the response, including error checking. The specific methods could then call this helper with the RPC method name and expected response type.
For example, you could have a helper like _make_rpc_call(self, method, request, response_cls, ...).
| @@ -285,3 +304,3 @@ | |||
| result = await self.request_handler.on_list_tasks(params, context) | |||
| return MessageToDict(result) | |||
|
|
|||
| @@ -292,16 +311,24 @@ | |||
| ) -> dict[str, Any]: | |||
| """Handles the 'tasks/pushNotificationConfig/list' REST method. | |||
|
|
|||
| This method is currently not implemented. | |||
|
|
|||
| Args: | |||
| request: The incoming `Request` object. | |||
| context: Context provided by the server. | |||
|
|
|||
| Returns: | |||
| A list of `dict` representing the `TaskPushNotificationConfig` objects. | |||
|
|
|||
| Raises: | |||
| NotImplementedError: This method is not yet implemented. | |||
There was a problem hiding this comment.
The docstring for list_tasks still mentions NotImplementedError, but the method is now implemented. The docstring should be updated to reflect the current implementation.
| @@ -292,16 +311,24 @@ | |||
| ) -> dict[str, Any]: | |||
| """Handles the 'tasks/pushNotificationConfig/list' REST method. | |||
|
|
|||
| This method is currently not implemented. | |||
|
|
|||
| Args: | |||
| request: The incoming `Request` object. | |||
| context: Context provided by the server. | |||
|
|
|||
| Returns: | |||
| A list of `dict` representing the `TaskPushNotificationConfig` objects. | |||
|
|
|||
| Raises: | |||
| NotImplementedError: This method is not yet implemented. | |||
| """ | |||
There was a problem hiding this comment.
The Raises section in the docstring for list_push_notifications is no longer accurate as the method is now implemented. It should be removed.
| mock_grpc_stub.DeleteTaskPushNotificationConfig.return_value = ( | ||
| sample_task_push_notification_config | ||
| ) |
There was a problem hiding this comment.
The DeleteTaskPushNotificationConfig gRPC method is defined to return google.protobuf.Empty. To make the mock more accurate, it should return None since the transport method does not use the return value and empty_pb2 is not imported in this file.
mock_grpc_stub.DeleteTaskPushNotificationConfig.return_value = None| ) | ||
| response = await transport.delete_task_callback(request) | ||
|
|
||
| mock_httpx_client.post.assert_called_once() # Assuming mock_send_request was a typo for mock_httpx_client.post |
There was a problem hiding this comment.
This comment appears to be a leftover development note and can be removed for cleaner code.
| mock_httpx_client.post.assert_called_once() # Assuming mock_send_request was a typo for mock_httpx_client.post | |
| mock_httpx_client.post.assert_called_once() |
| mock_response.json.return_value = { | ||
| 'taskId': task_id, | ||
| 'id': 'config-1', | ||
| 'pushNotificationConfig': { | ||
| 'id': 'config-1', | ||
| 'url': 'https://example.com', | ||
| }, | ||
| } |
There was a problem hiding this comment.
The mocked JSON response for a successful deletion returns a full object. However, the corresponding server-side handler (delete_push_notification in rest_handler.py) returns an empty dictionary {}. To make the test more aligned with the server implementation, the mocked json.return_value should be {}.
mock_response.json.return_value = {}| handler.on_delete_task_push_notification_config.return_value = ( | ||
| CALLBACK_CONFIG | ||
| ) |
There was a problem hiding this comment.
The on_delete_task_push_notification_config method in the RequestHandler interface is defined to return None. To align the mock with the interface definition, it should be configured to return None.
handler.on_delete_task_push_notification_config.return_value = None1 participant
Fixes #702