fix(server): return the stored id when creating a push notification config - #1236
ConnorMoss02 wants to merge 5 commits into
Conversation
…onfig Both create handlers returned the caller's request object rather than what the store persisted. The in-memory store defaults an empty id to the task id on the caller's object, so the id survived; the database store copies first and defaults on its private copy, so the response carried no id and reading the config back with it failed validation. Normalize the id in the handler before set_info so the response matches what was stored on every backend.
🧪 Code Coverage (vs
|
| Base | PR | Delta | |
|---|---|---|---|
| src/a2a/server/request_handlers/default_request_handler.py | 97.90% | 97.92% | 🟢 +0.02% |
| src/a2a/server/request_handlers/default_request_handler_v2.py | 92.53% | 92.65% | 🟢 +0.12% |
| Total | 93.06% | 93.07% | 🟢 +0.01% |
Generated by coverage-comment.yml
|
@mykytanetipa ready for review when you have a moment. It fixes #1237. |
mykytanetipa
left a comment
mykytanetipa
left a comment
There was a problem hiding this comment.
general design comment
Instead of duplicating the store's ID-defaulting rule (if not params.id: params.id = task_id) across both request handlers and mutating the caller's request object, we could have PushNotificationConfigStore.set_info copy/normalize the config (including id and task_id) and return the persisted TaskPushNotificationConfig for the handler to return.
| ): | ||
| """Test on_create_task_push_notification_config returns the id that was stored.""" | ||
| if store_kind == 'database': | ||
| from a2a.server.tasks.database_push_notification_config_store import ( |
There was a problem hiding this comment.
sqlalchemy and aiosqlite are optional extras (a2a-sdk[sqlite]), not core dependencies in pyproject.toml.
Unlike test_database_push_notification_config_store.py, these core handler test modules run in environments without optional SQL extras.
Guard the database branch with pytest.importorskip('sqlalchemy') and pytest.importorskip('aiosqlite')
There was a problem hiding this comment.
Guarded the database branch in both handler test modules with pytest.importorskip('sqlalchemy') and pytest.importorskip('aiosqlite').
| task_id = cast('str', request_context.task_id) | ||
| context_id = cast('str', request_context.context_id) | ||
|
|
||
| if self._push_config_store and params.configuration.HasField( |
There was a problem hiding this comment.
Not a blocker for this PR, but there is a related gap when registering a push notification config inline via SendMessage / SendStreamingMessage.
Per a2a.proto task_id is empty on SendMessageConfiguration.task_push_notification_config and id may be empty too.
Because neither this path before calling set_info nor store sets task_id or normalizes id, subsequent GetTaskPushNotificationConfig / ListTaskPushNotificationConfigs calls return a TaskPushNotificationConfig with task_id="".
We should normalize both id and task_id on this path (or in set_info) as well.
There was a problem hiding this comment.
Covered by the set_info change, since it now sets task_id and the default id for every caller, including SendMessage / SendStreamingMessage. Added test_on_message_send_stores_inline_push_config_under_its_task in the v2 handler tests, which reads the config back with task_id and id set to the task.
|
Moved the normalization into |
|
The failing check is |
| context: ServerCallContext, | ||
| ) -> None: | ||
| """Sets or updates the push notification configuration for a task.""" | ||
| ) -> TaskPushNotificationConfig: |
There was a problem hiding this comment.
to avoid breaking changes for existing custom implementations: lets make return type TaskPushNotificationConfig | None.
inmemory and database stores can stay just TaskPushNotificationConfig
| await self._reject_unsafe_push_url(params.url) | ||
|
|
||
| await self._push_config_store.set_info( | ||
| return await self._push_config_store.set_info( |
There was a problem hiding this comment.
to avoid breaking changes for existing custom implementations: here and in legacy handler lets implement a fallback for None case (see typing comment)
smth like this:
stored = await self._push_config_store.set_info(
task_id,
params,
context,
)
if stored is not None:
return stored
fallback = TaskPushNotificationConfig()
fallback.CopyFrom(params)
fallback.task_id = task_id
if not fallback.id:
fallback.id = task_id
return fallback
|
Regarding the test, lets retry, should pass. These are new tests related to multi-server deployment logic, some flakiness is expected (we'll implement a durable fix later). |
This branch has not been deployed
2 participants
Problem
The create handler returned the caller's request instead of what the store saved, so on the database store the response had no
id. Neither store settask_id, so configs registered inline viaSendMessagecame back withtask_id="".Fix
set_infonow stores a copy withtask_idset and an emptyiddefaulted to the task id, and returns it without mutating the input. Both handlers return that.set_infonow returnsTaskPushNotificationConfiginstead ofNone.Tests
Create-handler tests over both real stores (database skipped without the sqlite extras), an inline
SendMessagetest, and aset_infotest per store. All fail with the source change reverted.Fixes #1237