feat: Support multiple payload types in push notifications #493
Conversation
Signed-off-by: Jeff Mesnil <[email protected]>
Summary of ChangesHello @jmesnil, 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 generalizes the push notification mechanism to support a wider array of event types. By transitioning the PushNotificationSender from handling only Task objects to accommodating any StreamingEventKind as defined in the A2A specification, the system gains significant flexibility. This enhancement allows for more diverse event-driven notifications, such as messages, status updates, and artifact updates, ensuring better alignment with the broader A2A event model. 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
|
|
This PR is built on top of #492 and should be merged after. |
![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 successfully expands the PushNotificationSender to support various StreamingEventKind payloads, which is a great feature enhancement. The core logic changes are well-implemented, although there's a minor opportunity to use more modern Java features for improved readability. However, the accompanying tests have several critical issues. Some existing tests haven't been correctly updated to handle the new generic event types, which will lead to compilation errors. More importantly, the mock HTTP client and the new test case for sending Message payloads are flawed, causing the test to fail at runtime. There is also some leftover debugging code that should be removed.
| Task task = Utils.OBJECT_MAPPER.readValue(body, Task.TYPE_REFERENCE); | ||
| tasks.add(task); |
There was a problem hiding this comment.
The mock HTTP client is hardcoded to deserialize the request body into a Task object. With the changes in this PR, the payload can be any StreamingEventKind, such as a Message. This will cause a JsonProcessingException when testSendNotificationWithMessage is run. The mock client needs to be updated to handle different event types. I suggest deserializing to StreamingEventKind and then conditionally adding to the tasks list if it's a Task. You will also need to update the new test testSendNotificationWithMessage to assert against the correct event type (a Message), which may require adding a new list to TestHttpClient to store all captured events.
| Task task = Utils.OBJECT_MAPPER.readValue(body, Task.TYPE_REFERENCE); | |
| tasks.add(task); | |
| StreamingEventKind event = Utils.OBJECT_MAPPER.readValue(body, StreamingEventKind.class); | |
| if (event instanceof Task task) { | |
| tasks.add(task); | |
| } |
| assertEquals(1, testHttpClient.tasks.size()); | ||
| Task sentTask = testHttpClient.tasks.get(0); | ||
| assertEquals(taskData.getId(), sentTask.getId()); |
There was a problem hiding this comment.
These assertions are incorrect. You are sending a Message object, but asserting that a Task was captured. The PushNotificationSender sends the Message object itself. This test will fail because TestHttpClient is not equipped to handle Message payloads and will throw an exception during deserialization, as noted in another comment. The TestHttpClient needs to be updated to capture StreamingEventKind objects, and this test should assert that a Message was received.
| Queue<Task> captured = mockPushNotificationSender.getCapturedEvents(); | ||
| assertEquals(1, captured.size()); | ||
| assertEquals("direct-test-task", captured.peek().getId()); |
There was a problem hiding this comment.
The type of the captured variable is incorrect. mockPushNotificationSender.getCapturedEvents() returns a Queue<StreamingEventKind>, not a Queue<Task>. This will cause a compilation error. You need to update the variable type and add a cast to access Task-specific methods. It's also safer to check the type before casting.
Queue<StreamingEventKind> captured = mockPushNotificationSender.getCapturedEvents();
assertEquals(1, captured.size());
assertTrue(captured.peek() instanceof Task, "Captured event should be a Task");
assertEquals("direct-test-task", ((Task) captured.peek()).getId());| String taskId = switch (kind.getKind()) { | ||
| case TASK -> ((Task) kind).getId(); | ||
| case MESSAGE -> ((Message)kind).getTaskId(); | ||
| case STATUS_UPDATE -> ((TaskStatusUpdateEvent)kind).getTaskId(); | ||
| case ARTIFACT_UPDATE -> ((TaskArtifactUpdateEvent)kind).getTaskId(); | ||
| default -> null; | ||
| }; |
There was a problem hiding this comment.
Since this project uses a modern Java version (as indicated by the use of sealed interfaces), you can leverage pattern matching for switch. This will make the code more concise, readable, and type-safe by avoiding explicit string comparisons and casts.
String taskId = switch (kind) {
case Task t -> t.getId();
case Message m -> m.getTaskId();
case TaskStatusUpdateEvent e -> e.getTaskId();
case TaskArtifactUpdateEvent e -> e.getTaskId();
};| @Override | ||
| public PostBuilder body(String body) { | ||
| this.body = body; | ||
| System.out.println("body = " + body); |
There was a problem hiding this comment.
This System.out.println appears to be leftover debugging code and should be removed before merging.
| Task task = Utils.OBJECT_MAPPER.readValue(body, Task.TYPE_REFERENCE); | ||
| tasks.add(task); | ||
| urls.add(url); | ||
| System.out.println(requestHeaders); |
There was a problem hiding this comment.
This System.out.println appears to be leftover debugging code and should be removed before merging.
1 participant
Expand PushNotificationSender to support all StreamingEventKind payload types
as defined in the A2A specification, not just Task objects.
Fixes: #490
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.Fixes #490 🦕