fix: allow omitted task push config id by hutiefang76 · Pull Request #1092 · a2aproject/a2a-java

hutiefang76 · 2026-08-25T16:16:40Z

Description

Allow TaskPushNotificationConfig to omit its id when 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 test
- TaskPushNotificationConfigTest: 1 passed
- InMemoryPushNotificationConfigStoreTest: 33 passed
Follow the CONTRIBUTING guide
Use a conventional commit title
Tests pass
README changes are not needed for this API-contract correction

Fixes #1081 🦕

kabir

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.

kabir · 2026-08-26T11:09:39Z

    public void testSetInfoWithoutConfigId() {
        String taskId = "task1";
        TaskPushNotificationConfig initialConfig = TaskPushNotificationConfig.builder()
-                .id("") // No ID set


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.

The previous comment should still be addressed.

kabir · 2026-08-26T11:09:43Z

        assertEquals(taskId, configResult.configs().get(0).id());

        TaskPushNotificationConfig updatedConfig = TaskPushNotificationConfig.builder()
-                .id("") // No ID set


Please create a new test rather than changing this existing one.

The previous comment should still be addressed.

kabir · 2026-08-26T11:10:57Z

    public TaskPushNotificationConfig setInfo(TaskPushNotificationConfig notificationConfig) {
        String taskId = Assert.checkNotNullParam("taskId", notificationConfig.taskId());
        TaskPushNotificationConfig.Builder builder = TaskPushNotificationConfig.builder(notificationConfig);
-        if (notificationConfig.id().isEmpty()) {


Not a comment about this code, but JpaDatabasePushNotificationConfigStore has the same latent NPE, and needs fixing too.

hutiefang76 · 2026-08-26T12:46:35Z

Updated in 5bd4916.

JpaDatabasePushNotificationConfigStore now normalizes a null or empty config ID to the task ID, with a Quarkus/JPA regression test.
GetTaskPushNotificationConfigParams now permits an omitted ID through both its constructor and builder, consistent with its Javadoc. The request handler resolves an omitted ID to the task ID, matching creation and v0.3 compatibility behavior.
Added focused model and request-handler coverage.

Verified on JDK 17 with:
mvn -pl spec,server-common,extras/push-notification-config-store-database-jpa -am -Dtest=GetTaskPushNotificationConfigParamsTest,DefaultRequestHandlerTest,JpaPushNotificationConfigStoreTest -Dsurefire.failIfNoSpecifiedTests=false test

Result: 2 spec tests, 23 request-handler tests, and 16 JPA-store tests passed (one pre-existing disabled test).`

kabir · 2026-08-26T14:11:59Z

    public void testSetInfoWithoutConfigId() {
        String taskId = "task1";
        TaskPushNotificationConfig initialConfig = TaskPushNotificationConfig.builder()
-                .id("") // No ID set


The previous comment should still be addressed.

kabir · 2026-08-26T14:12:03Z

        assertEquals(taskId, configResult.configs().get(0).id());

        TaskPushNotificationConfig updatedConfig = TaskPushNotificationConfig.builder()
-                .id("") // No ID set


The previous comment should still be addressed.

hutiefang76 · 2026-08-26T15:09:20Z

Addressed in dab6633.

Restored the existing empty-id coverage with .id(""), so it continues to test the exact value emitted by the mapper.
Added a separate null-id regression test; it verifies the store derives the config id from the task id when the id is absent.
The equivalent JPA null/empty normalization is already included in 5bd4916.

Validated with GetTaskPushNotificationConfigParamsTest, DefaultRequestHandlerTest, InMemoryPushNotificationConfigStoreTest, and JpaPushNotificationConfigStoreTest (75 tests, 0 failures/errors).`

kabir · 2026-08-26T15:13:00Z

Sorry @hutiefang76 we're doing a lot of merging today, and there is now a conflict :-)

Fixes a2aproject#1081

hutiefang76 · 2026-08-26T15:35:00Z

Rebased onto current main to resolve the merge conflict (latest head: e4a3d37). The resolution preserves upstream tenant validation while retaining the optional config-id contract and its empty-id/null-id coverage.\n\nValidated after the rebase with the focused spec, request-handler, in-memory-store, and JPA-store suites: 84 tests, 0 failures/errors; git diff --check also passes.

ehsavoie

This is mostly good and correct but I have a behaviour question:
When creating multiple push notification configs for the same task with omitted IDs. Both would default to taskId, so the second setInfo call would silently update the first config. This behaviour should be documented and covered a test. Also I think that at least we should 'trace' that somehow or maybe we should fail. Using empty id only to create.

Signed-off-by: hutiefang76 <137664623+hutiefang76@users.noreply.github.com>

hutiefang76 · 2026-08-29T00:59:29Z

Addressed the remaining review concern in 1c21dcd.\n\nA missing or empty ID still creates the default config with the task ID, preserving the existing default-get behavior. A second create with an omitted ID now fails with InvalidParamsError instead of silently replacing that default config; clients must send the task ID explicitly to update it. This is implemented consistently in the in-memory and JPA stores, and the public store contract now documents the rule.\n\nAdded coverage for both empty-string and null IDs in both stores. Verified with:\n- mvn -pl server-common -am -Dtest=InMemoryPushNotificationConfigStoreTest -Dsurefire.failIfNoSpecifiedTests=false test (34 passing)\n- mvn -pl extras/push-notification-config-store-database-jpa -am -Dtest=JpaPushNotificationConfigStoreTest -Dsurefire.failIfNoSpecifiedTests=false test (16 passing, 1 skipped)

Signed-off-by: hutiefang76 <137664623+hutiefang76@users.noreply.github.com>

hutiefang76 · 2026-08-30T12:57:56Z

Addressed the v0.3 compatibility regression in bd875377.

The duplicate omitted-ID guard added in the previous commit is still enforced for the current multi-config protocol, but v0.3 now preserves its historical single-config set/update behavior. The protocol-aware path is applied consistently to the in-memory and JPA stores, and the v0.3 test adapter now records the correct protocol version.

Added regression coverage for both stores and verified on JDK 21 with:

InMemoryPushNotificationConfigStoreTest, DefaultRequestHandlerTest, JpaPushNotificationConfigStoreTest, and GetTaskPushNotificationConfigParamsTest: 86 tests, 0 failures/errors (1 skipped)
v0.3 JSON-RPC, gRPC, and REST reference suites: 315 tests, 0 failures/errors (38 skipped)
git diff --check

This fixes the Java 21 CI failures where repeated v0.3 setPushNotification calls were rejected instead of updating the single stored config.

Signed-off-by: hutiefang76 <137664623+hutiefang76@users.noreply.github.com>

hutiefang76 · 2026-08-30T13:41:27Z

I traced the four build failures to the multiversion test setup path rather than the production v0.3 route.

The v0.3 client/server path already carries requestedProtocolVersion = "0.3", but AbstractA2AServerServerTest_v0_3 seeds push configs through the shared /test/task/{taskId} endpoint. That test endpoint discarded the protocol version, so the setup call used v1.0 create-only semantics and the second omitted-ID config failed (or leaked an extra config into later assertions).

Commit 05422c57 now:

sends the standard A2A-Version: 0.3 header from the v0.3 test helper;
forwards that header through the JSON-RPC, REST, and gRPC multiversion test endpoints;
keeps the existing no-header helper behavior for v1.0 tests.

Local JDK 21 verification:

MultiVersionJSONRPC_v0_3_Test: 38 tests, 0 failures/errors
MultiVersion_v0_3_RestTest: 38 tests, 0 failures/errors, 9 skipped
MultiVersion_v0_3_GrpcTest: 38 tests, 0 failures/errors, 9 skipped

Total: 114 tests, 0 failures/errors.

hutiefang76 · 2026-08-30T14:47:36Z

@kabir @ehsavoie The latest head 05422c57 addresses the requested store coverage, omitted-ID duplicate behavior, and v0.3 compatibility setup. All 13 GitHub checks are now green, including multiversion JSON-RPC, REST, gRPC, TCK, Windows, and JDK 17/21/25 builds. Could you please take a fresh look when convenient?

kabir requested changes Aug 26, 2026

View reviewed changes

hutiefang76 added 3 commits August 26, 2026 23:26

fix: allow omitted task push config id

78494f1

Fixes a2aproject#1081

fix: complete optional push config id handling

483868d

test: cover empty and null push config ids

e4a3d37

hutiefang76 force-pushed the codex/a2a-java-1081-optional-push-config-id branch from dab6633 to e4a3d37 Compare August 26, 2026 15:34

Merge branch 'main' into codex/a2a-java-1081-optional-push-config-id

a9b96c8

ehsavoie requested changes Aug 27, 2026

View reviewed changes

Reject duplicate push configs with omitted IDs

1c21dcd

Signed-off-by: hutiefang76 <137664623+hutiefang76@users.noreply.github.com>

Preserve v0.3 push config update semantics

bd87537

Signed-off-by: hutiefang76 <137664623+hutiefang76@users.noreply.github.com>

Test v0.3 push config setup in multiversion servers

05422c5

Signed-off-by: hutiefang76 <137664623+hutiefang76@users.noreply.github.com>

Uh oh!

Conversation

hutiefang76 commented Aug 25, 2026

Description

Tests

Uh oh!

kabir left a comment

Choose a reason for hiding this comment

Uh oh!

kabir Aug 26, 2026

Choose a reason for hiding this comment

Uh oh!

kabir Aug 26, 2026

Choose a reason for hiding this comment

Uh oh!

kabir Aug 26, 2026

Choose a reason for hiding this comment

Uh oh!

kabir Aug 26, 2026

Choose a reason for hiding this comment

Uh oh!

kabir Aug 26, 2026

Choose a reason for hiding this comment

Uh oh!

hutiefang76 commented Aug 26, 2026

Uh oh!

kabir Aug 26, 2026

Choose a reason for hiding this comment

Uh oh!

kabir Aug 26, 2026

Choose a reason for hiding this comment

Uh oh!

hutiefang76 commented Aug 26, 2026

Uh oh!

kabir commented Aug 26, 2026

Uh oh!

hutiefang76 commented Aug 26, 2026

Uh oh!

ehsavoie left a comment

Choose a reason for hiding this comment

Uh oh!

hutiefang76 commented Aug 29, 2026

Uh oh!

hutiefang76 commented Aug 30, 2026

Uh oh!

hutiefang76 commented Aug 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hutiefang76 commented Aug 30, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hutiefang76 commented Aug 30, 2026 •

edited

Loading