refactor!: upgrade SDK to A2A 1.0 specs #572
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,137 @@ | ||
| # Response to AIP Discussion #1247 | ||
|
|
||
| > Re: [Respecting AIP response payloads in HTTP](https://github.com/a2aproject/A2A/discussions/1247) | ||
|
|
||
| Thanks for this detailed explanation of the AIP conventions, @darrelmiller. I've been working on the a2a-python SDK migration from Pydantic to protobuf types ([PR #572](https://github.com/a2aproject/a2a-python/pull/572)) and wanted to share how we've implemented this. | ||
|
|
||
| ## How we handle `SetTaskPushNotificationConfig` in the SDK | ||
|
|
||
| The key insight is that the request and response types serve different purposes: | ||
|
|
||
| **Request (`SetTaskPushNotificationConfigRequest`):** | ||
| ```protobuf | ||
| message SetTaskPushNotificationConfigRequest { | ||
| string parent = 1; // e.g., "tasks/{task_id}" | ||
| string config_id = 2; // e.g., "my-config-id" | ||
| TaskPushNotificationConfig config = 3; | ||
| } | ||
| ``` | ||
|
|
||
| **Response (`TaskPushNotificationConfig`):** | ||
| ```protobuf | ||
| message TaskPushNotificationConfig { | ||
| string name = 1; // Full resource name: "tasks/{task_id}/pushNotificationConfigs/{config_id}" | ||
| PushNotificationConfig push_notification_config = 2; | ||
| } | ||
| ``` | ||
|
|
||
| ## Implementation in Python | ||
|
|
||
| In our `DefaultRequestHandler`, we construct the proper `name` field from the request's `parent` and `config_id`: | ||
|
|
||
| ```python | ||
| async def on_set_task_push_notification_config( | ||
| self, | ||
| params: SetTaskPushNotificationConfigRequest, | ||
| context: ServerCallContext | None = None, | ||
| ) -> TaskPushNotificationConfig: | ||
| task_id = _extract_task_id(params.parent) # Extract from "tasks/{task_id}" | ||
|
|
||
| # Store the config | ||
| await self._push_config_store.set_info( | ||
| task_id, | ||
| params.config.push_notification_config, | ||
| ) | ||
|
|
||
| # Build response with proper AIP resource name | ||
| return TaskPushNotificationConfig( | ||
| name=f'{params.parent}/pushNotificationConfigs/{params.config_id}', | ||
| push_notification_config=params.config.push_notification_config, | ||
| ) | ||
| ``` | ||
|
|
||
| ## REST Handler Translation | ||
|
|
||
| For the HTTP binding, the REST handler extracts path parameters and constructs the request: | ||
|
|
||
| ```python | ||
| async def set_push_notification(self, request: Request, context: ServerCallContext): | ||
| task_id = request.path_params['id'] | ||
| body = await request.body() | ||
|
|
||
| params = SetTaskPushNotificationConfigRequest() | ||
| Parse(body, params) | ||
| params.parent = f'tasks/{task_id}' # Set from URL path | ||
|
|
||
| config = await self.request_handler.on_set_task_push_notification_config(params, context) | ||
| return MessageToDict(config) # Returns with proper `name` field | ||
| ``` | ||
|
|
||
| ## JSON-RPC Handler | ||
|
|
||
| The JSON-RPC handler passes the full request directly: | ||
|
|
||
| ```python | ||
| async def set_push_notification_config( | ||
| self, | ||
| request: SetTaskPushNotificationConfigRequest, | ||
| context: ServerCallContext | None = None, | ||
| ) -> SetTaskPushNotificationConfigResponse: | ||
| result = await self.request_handler.on_set_task_push_notification_config( | ||
| request, context | ||
| ) | ||
| return prepare_response_object(...) | ||
| ``` | ||
|
|
||
| ## Key Takeaways | ||
|
|
||
| 1. **The `name` field is constructed, not passed in** - The server builds the full resource name from `parent` + `config_id` | ||
|
|
||
| 2. **Consistent across bindings** - Both gRPC and HTTP handlers ultimately call the same `on_set_task_push_notification_config` method | ||
|
|
||
| 3. **AIP compliance** - The response always includes the full `name` field as required by [AIP-122](https://google.aip.dev/122) | ||
|
|
||
| 4. **Helper functions for resource name parsing**: | ||
| ```python | ||
| def _extract_task_id(resource_name: str) -> str: | ||
| """Extract task ID from a resource name like 'tasks/{task_id}' or 'tasks/{task_id}/...'.""" | ||
| match = re.match(r'^tasks/([^/]+)', resource_name) | ||
| if match: | ||
| return match.group(1) | ||
| return resource_name # Fall back for backwards compatibility | ||
|
|
||
| def _extract_config_id(resource_name: str) -> str | None: | ||
| """Extract config ID from 'tasks/{task_id}/pushNotificationConfigs/{config_id}'.""" | ||
| match = re.match(r'^tasks/[^/]+/pushNotificationConfigs/([^/]+)$', resource_name) | ||
| if match: | ||
| return match.group(1) | ||
| return None | ||
| ``` | ||
|
|
||
| ## E2E Test Example | ||
|
|
||
| Here's how a client uses this in practice: | ||
|
|
||
| ```python | ||
| # Client sets the push notification config | ||
| await a2a_client.set_task_callback( | ||
| SetTaskPushNotificationConfigRequest( | ||
| parent=f'tasks/{task.id}', | ||
| config_id='my-notification-config', | ||
| config=TaskPushNotificationConfig( | ||
| push_notification_config=PushNotificationConfig( | ||
| id='my-notification-config', | ||
| url=f'{notifications_server}/notifications', | ||
| token=token, | ||
| ), | ||
| ), | ||
| ) | ||
| ) | ||
| ``` | ||
|
|
||
| This approach keeps the abstract handler logic clean while ensuring AIP compliance at the protocol binding level. | ||
|
|
||
| --- | ||
|
|
||
| **Related PRs:** | ||
| - [a2a-python PR #572](https://github.com/a2aproject/a2a-python/pull/572) - Proto migration with these changes |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,17 +3,43 @@ | |
|
|
||
| from a2a.client.auth.credentials import CredentialService | ||
| from a2a.client.middleware import ClientCallContext, ClientCallInterceptor | ||
| from a2a.types.a2a_pb2 import ( | ||
| AgentCard, | ||
| APIKeySecurityScheme, | ||
| HTTPAuthSecurityScheme, | ||
| MutualTlsSecurityScheme, | ||
| OAuth2SecurityScheme, | ||
| OpenIdConnectSecurityScheme, | ||
| SecurityScheme, | ||
| ) | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
| _SecuritySchemeValue = ( | ||
| APIKeySecurityScheme | ||
| | HTTPAuthSecurityScheme | ||
| | OAuth2SecurityScheme | ||
| | OpenIdConnectSecurityScheme | ||
| | MutualTlsSecurityScheme | ||
| | None | ||
| ) | ||
|
|
||
|
|
||
| def _get_security_scheme_value(scheme: SecurityScheme) -> _SecuritySchemeValue: | ||
| """Extract the actual security scheme from the oneof union.""" | ||
| which = scheme.WhichOneof('scheme') | ||
| if which == 'api_key_security_scheme': | ||
| return scheme.api_key_security_scheme | ||
| if which == 'http_auth_security_scheme': | ||
| return scheme.http_auth_security_scheme | ||
| if which == 'oauth2_security_scheme': | ||
| return scheme.oauth2_security_scheme | ||
| if which == 'open_id_connect_security_scheme': | ||
| return scheme.open_id_connect_security_scheme | ||
| if which == 'mtls_security_scheme': | ||
| return scheme.mtls_security_scheme | ||
| return None | ||
|
|
||
|
|
||
| class AuthInterceptor(ClientCallInterceptor): | ||
| """An interceptor that automatically adds authentication details to requests. | ||
|
|
@@ -35,13 +61,13 @@ async def intercept( | |
| """Applies authentication headers to the request if credentials are available.""" | ||
| if ( | ||
| agent_card is None | ||
| or not agent_card.security | ||
| or not agent_card.security_schemes | ||
|
There was a problem hiding this comment. Double checking the protobuf docs https://protobuf.dev/reference/python/python-generated/#embedded_message it sounds like we may need to use |
||
| ): | ||
| return request_payload, http_kwargs | ||
|
|
||
| for requirement in agent_card.security: | ||
| for scheme_name in requirement.schemes: | ||
| credential = await self._credential_service.get_credentials( | ||
| scheme_name, context | ||
| ) | ||
|
|
@@ -51,7 +77,9 @@ async def intercept( | |
| ) | ||
| if not scheme_def_union: | ||
| continue | ||
| scheme_def = _get_security_scheme_value(scheme_def_union) | ||
| if not scheme_def: | ||
| continue | ||
|
There was a problem hiding this comment. Personal preference would be to use the functions available from proto to do the matching below instead of ending up with a non-statically typed function here. E.g. match scheme.WhichOneof('scheme'):
case 'http_auth_security_scheme' if scheme.http_auth_security_scheme.lower() == 'bearer':
...tbh I think it would read better as a set of scheme = agent_card.security_schemes.get(scheme_name)
if scheme.HasField('http_auth_security_scheme') and scheme.http_auth_security_scheme.lower() == 'bearer':
...
if scheme.HasField('oauth2_security_scheme') or scheme.HasField('open_id_connect_security_scheme'):
...
Member
Author
There was a problem hiding this comment. Not sure how's HasField improves things here. There was a problem hiding this comment. As explained our call, its mostly about readability, but also prevent us using reflection to check the python types, and instead we can just rely on the boolean check of "does the request have this field set" then it can use it. There was a problem hiding this comment. I don't seem to be able to resolve my own threads on the a2a repos, but I'll leave a LGTM if I think it should be resolved. |
||
|
|
||
| headers = http_kwargs.get('headers', {}) | ||
|
|
||
|
|
@@ -62,9 +90,8 @@ async def intercept( | |
| ): | ||
| headers['Authorization'] = f'Bearer {credential}' | ||
| logger.debug( | ||
| "Added Bearer token for scheme '%s'.", | ||
| scheme_name, | ||
| ) | ||
| http_kwargs['headers'] = headers | ||
| return request_payload, http_kwargs | ||
|
|
@@ -76,15 +103,16 @@ async def intercept( | |
| ): | ||
| headers['Authorization'] = f'Bearer {credential}' | ||
| logger.debug( | ||
| "Added Bearer token for scheme '%s'.", | ||
| scheme_name, | ||
| ) | ||
| http_kwargs['headers'] = headers | ||
| return request_payload, http_kwargs | ||
|
|
||
| # Case 2: API Key in Header | ||
| case APIKeySecurityScheme() if ( | ||
| scheme_def.location.lower() == 'header' | ||
| ): | ||
| headers[scheme_def.name] = credential | ||
| logger.debug( | ||
| "Added API Key Header for scheme '%s'.", | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
I think this change has just proven my point about moving this inline in the function below and changing how the "match" is done.