fix: allow omitted task push config id - #1092
Conversation
kabir
left a comment
kabir
left a comment
There was a problem hiding this comment.
Hi @hutiefang76 - thank you for the PR 👍
I found a few small issues in the code, but we also need to take JpaDatabasePushNotificationConfigStore into account and make some changes, and test this inline with what you have done here. JpaPushNotificationConfigStoreTest might be the right place.
Also GetTaskPushNotificationConfigParams asserts id non-null despite its Javadoc calling it optional. Might as well fix this since it is related.
| public void testSetInfoWithoutConfigId() { | ||
| String taskId = "task1"; | ||
| TaskPushNotificationConfig initialConfig = TaskPushNotificationConfig.builder() | ||
| .id("") // No ID set |
There was a problem hiding this comment.
Please create a new test rather than changing this existing one.
Removing .id("") repurposes this test from the empty-string path to the null path rather than covering both.
The empty-string case is the one production actually hits — the mapper doesn't apply emptyToNull to id, so at runtime a missing id arrives as "", never null.
Recommend keeping the "" case here and adding a separate null-id test (or parameterizing over both), so the load-bearing branch stays asserted.
| assertEquals(taskId, configResult.configs().get(0).id()); | ||
|
|
||
| TaskPushNotificationConfig updatedConfig = TaskPushNotificationConfig.builder() | ||
| .id("") // No ID set |
There was a problem hiding this comment.
Please create a new test rather than changing this existing one.
| public TaskPushNotificationConfig setInfo(TaskPushNotificationConfig notificationConfig) { | ||
| String taskId = Assert.checkNotNullParam("taskId", notificationConfig.taskId()); | ||
| TaskPushNotificationConfig.Builder builder = TaskPushNotificationConfig.builder(notificationConfig); | ||
| if (notificationConfig.id().isEmpty()) { |
There was a problem hiding this comment.
Not a comment about this code, but JpaDatabasePushNotificationConfigStore has the same latent NPE, and needs fixing too.
2 participants
Description
Allow
TaskPushNotificationConfigto omit itsidwhen a client creates a push notification configuration, matching the A2A specification. The in-memory store now assigns the task ID when the supplied ID is null or empty, preserving its existing defaulting behavior.Tests
mvn -pl spec,server-common -am -Dtest=TaskPushNotificationConfigTest,InMemoryPushNotificationConfigStoreTest -Dsurefire.failIfNoSpecifiedTests=false testTaskPushNotificationConfigTest: 1 passedInMemoryPushNotificationConfigStoreTest: 33 passedFollow the CONTRIBUTING guide
Use a conventional commit title
Tests pass
README changes are not needed for this API-contract correction
Fixes #1081 🦕