-
Notifications
You must be signed in to change notification settings - Fork 503
fix(server): return the stored id when creating a push notification config #1236
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
982f7ad
3435cda
639d34b
d7da247
f99fe21
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,8 +18,13 @@ async def set_info( | |
| task_id: str, | ||
| notification_config: TaskPushNotificationConfig, | ||
| context: ServerCallContext, | ||
| ) -> None: | ||
| """Sets or updates the push notification configuration for a task.""" | ||
| ) -> TaskPushNotificationConfig: | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. to avoid breaking changes for existing custom implementations: lets make return type TaskPushNotificationConfig | None. inmemory and database stores can stay just TaskPushNotificationConfig |
||
| """Sets or updates the push notification configuration for a task. | ||
|
|
||
| Implementations MUST NOT mutate notification_config. They store a | ||
| copy with task_id set to the given task and an empty id defaulted to | ||
| the task id, and return that stored configuration. | ||
| """ | ||
|
|
||
| @abstractmethod | ||
| async def get_info( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -513,6 +513,55 @@ async def test_set_task_push_notification_config_task_not_found(): | |
| mock_push_store.set_info.assert_not_awaited() | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| @pytest.mark.parametrize('store_kind', ['inmemory', 'database']) | ||
| async def test_create_task_push_notification_config_returns_stored_id( | ||
| store_kind, | ||
| ): | ||
| """Test on_create_task_push_notification_config returns the id that was stored.""" | ||
| if store_kind == 'database': | ||
| pytest.importorskip('sqlalchemy') | ||
| pytest.importorskip('aiosqlite') | ||
| from a2a.server.tasks.database_push_notification_config_store import ( | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Guarded the database branch in both handler test modules with |
||
| DatabasePushNotificationConfigStore, | ||
| ) | ||
| from sqlalchemy.ext.asyncio import create_async_engine | ||
|
|
||
| engine = create_async_engine( | ||
| 'sqlite+aiosqlite:///file:pushidv2?mode=memory&cache=shared&uri=true' | ||
| ) | ||
| push_config_store = DatabasePushNotificationConfigStore(engine=engine) | ||
| else: | ||
| push_config_store = InMemoryPushNotificationConfigStore() | ||
|
|
||
| task = create_sample_task() | ||
| task_store = InMemoryTaskStore() | ||
| context = create_server_call_context() | ||
| await task_store.save(task, context) | ||
|
|
||
| request_handler = DefaultRequestHandlerV2( | ||
| agent_executor=MockAgentExecutor(), | ||
| task_store=task_store, | ||
| push_config_store=push_config_store, | ||
| agent_card=create_default_agent_card(), | ||
| ) | ||
| params = TaskPushNotificationConfig( | ||
| task_id=task.id, url='http://example.com' | ||
| ) | ||
|
|
||
| response = await request_handler.on_create_task_push_notification_config( | ||
| params, context | ||
| ) | ||
|
|
||
| stored = await push_config_store.get_info(task.id, context) | ||
| if store_kind == 'database': | ||
| await engine.dispose() | ||
|
|
||
| assert response.id == task.id | ||
| assert list(stored) == [response] | ||
| assert params.id == '', 'the request object must not be mutated' | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_get_task_push_notification_config_no_store(): | ||
| """Test on_get_task_push_notification_config when _push_config_store is None.""" | ||
|
|
@@ -1710,6 +1759,40 @@ async def test_on_message_send_with_push_notification(): | |
| ) | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_on_message_send_stores_inline_push_config_under_its_task(): | ||
| task_store = InMemoryTaskStore() | ||
| push_store = InMemoryPushNotificationConfigStore() | ||
|
|
||
| request_handler = DefaultRequestHandlerV2( | ||
| agent_executor=HelloAgentExecutor(), | ||
| task_store=task_store, | ||
| push_config_store=push_store, | ||
| agent_card=create_default_agent_card(), | ||
| ) | ||
| # SendMessageConfiguration carries neither task_id nor id for the config. | ||
| params = SendMessageRequest( | ||
| message=Message( | ||
| role=Role.ROLE_USER, | ||
| message_id='msg_push_inline', | ||
| parts=[Part(text='Hi')], | ||
| ), | ||
| configuration=SendMessageConfiguration( | ||
| task_push_notification_config=TaskPushNotificationConfig( | ||
| url='http://example.com/webhook' | ||
| ) | ||
| ), | ||
| ) | ||
|
|
||
| context = create_server_call_context() | ||
| result = await request_handler.on_message_send(params, context) | ||
|
|
||
| stored = await push_store.get_info(result.id, context) | ||
| assert [(config.task_id, config.id) for config in stored] == [ | ||
| (result.id, result.id) | ||
| ] | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_on_message_send_with_empty_push_notification_config_does_not_call_set_info(): | ||
| task_store = InMemoryTaskStore() | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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: