-
Notifications
You must be signed in to change notification settings - Fork 66
fix: flatten TaskPushNotificationConfig to match v1 spec #418
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
372419e
6075ee1
2a5966c
702d8f9
7f582e4
b346819
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 |
|---|---|---|
|
|
@@ -196,16 +196,12 @@ internal static Task<IResult> GetExtendedAgentCardRestAsync( | |
|
|
||
| // REST handler: Create push notification config | ||
| internal static Task<IResult> CreatePushNotificationConfigRestAsync( | ||
| IA2ARequestHandler requestHandler, ILogger logger, string taskId, PushNotificationConfig config, CancellationToken cancellationToken) | ||
| IA2ARequestHandler requestHandler, ILogger logger, string taskId, TaskPushNotificationConfig config, CancellationToken cancellationToken) | ||
|
Contributor
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. One REST behavior to clarify: this endpoint now binds the body as TaskPushNotificationConfig, which includes Tenant, then forwards the config after only overriding TaskId from the route. That means a non-tenant REST call to /tasks/{id}/pushNotificationConfigs can still pass tenant in the body to the handler. But MapHttpA2A currently documents tenant REST variants as unsupported and says request Tenant fields are always null for REST calls. Could we make this consistent one way or the other? If tenant REST is out of scope, reject or clear config.Tenant on the non-tenant REST route and add a test for that behavior. |
||
| => WithExceptionHandlingAsync(logger, "REST.CreatePushNotificationConfig", async ct => | ||
| { | ||
| var request = new CreateTaskPushNotificationConfigRequest | ||
| { | ||
| TaskId = taskId, | ||
| Config = config, | ||
| ConfigId = config.Id ?? string.Empty, | ||
| }; | ||
| var result = await requestHandler.CreateTaskPushNotificationConfigAsync(request, ct).ConfigureAwait(false); | ||
| // Route provides the authoritative taskId; override whatever the body sent | ||
| config.TaskId = taskId; | ||
| var result = await requestHandler.CreateTaskPushNotificationConfigAsync(config, ct).ConfigureAwait(false); | ||
| return new A2AResponseResult(result); | ||
| }, taskId, cancellationToken); | ||
|
|
||
|
|
@@ -215,13 +211,13 @@ internal static Task<IResult> ListPushNotificationConfigRestAsync( | |
| CancellationToken cancellationToken) | ||
| => WithExceptionHandlingAsync(logger, "REST.ListPushNotificationConfig", async ct => | ||
| { | ||
| var request = new ListTaskPushNotificationConfigRequest | ||
| var request = new ListTaskPushNotificationConfigsRequest | ||
| { | ||
| TaskId = taskId, | ||
| PageSize = pageSize, | ||
| PageToken = pageToken, | ||
| }; | ||
| var result = await requestHandler.ListTaskPushNotificationConfigAsync(request, ct) | ||
| var result = await requestHandler.ListTaskPushNotificationConfigsAsync(request, ct) | ||
| .ConfigureAwait(false); | ||
| return new A2AResponseResult(result); | ||
| }, taskId, cancellationToken); | ||
|
|
@@ -258,7 +254,7 @@ internal sealed class A2AResponseResult : IResult | |
| internal A2AResponseResult(ListTasksResponse response) { _response = response; _responseType = typeof(ListTasksResponse); } | ||
| internal A2AResponseResult(AgentCard card) { _response = card; _responseType = typeof(AgentCard); } | ||
| internal A2AResponseResult(TaskPushNotificationConfig config) { _response = config; _responseType = typeof(TaskPushNotificationConfig); } | ||
| internal A2AResponseResult(ListTaskPushNotificationConfigResponse response) { _response = response; _responseType = typeof(ListTaskPushNotificationConfigResponse); } | ||
| internal A2AResponseResult(ListTaskPushNotificationConfigsResponse response) { _response = response; _responseType = typeof(ListTaskPushNotificationConfigsResponse); } | ||
|
|
||
| public async Task ExecuteAsync(HttpContext httpContext) | ||
| { | ||
|
|
||
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.
For the standalone create API, the config is being added to an already-known task, so taskId identifies the parent task for that operation. But in SendMessage, the push config is attached while the message is being sent, and the server should associate it with the task created or selected by that SendMessage request.
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.
TaskId = taskId says which task this config belongs to. In the embedded SendMessage path, that should come from the SendMessage task context, so I think this should be omitted.