Skip to content

Commit 57696a8

Browse files
ishymkoishymko
authored
fix: properly parse ProtoJSON body for POST pushNotificationConfigs (#352)
Apparently the code removed in #346 wasn't "dead", this method wasn't properly migrated in #292. Tests are updated to cover this. Fixes #336
1 parent 1323897 commit 57696a8

File tree

3 files changed

+202
-39
lines changed

3 files changed

+202
-39
lines changed

‎.betterer.results‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ exports[`TypeScript Strict Mode`] = {
2727
[248, 15, 4, "tsc: Expected 2 arguments, but got 1.", "2087764327"],
2828
[271, 15, 4, "tsc: Expected 2 arguments, but got 1.", "2087764327"]
2929
],
30-
"src/server/express/rest_handler.ts:2741309436": [
30+
"src/server/express/rest_handler.ts:2436698925": [
3131
[204, 50, 17, "tsc: Argument of type \'unknown\' is not assignable to parameter of type \'Message | Task | TaskArtifactUpdateEvent | TaskStatusUpdateEvent\'.", "3749434707"]
3232
],
3333
"src/server/request_handler/default_request_handler.ts:1208203039": [

‎src/server/express/rest_handler.ts‎

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -425,12 +425,9 @@ export function restHandler(options: RestHandlerOptions): RequestHandler {
425425
'/v1/tasks/:taskId/pushNotificationConfigs',
426426
asyncHandler(async (req, res) => {
427427
const context = await buildContext(req);
428-
const config = {
429-
...req.body,
430-
taskId: req.params.taskId,
431-
task_id: req.params.taskId,
432-
};
433-
const result = await restTransportHandler.setTaskPushNotificationConfig(config, context);
428+
const protoReq = a2a.CreateTaskPushNotificationConfigRequest.fromJSON(req.body);
429+
const params = FromProto.createTaskPushNotificationConfig(protoReq);
430+
const result = await restTransportHandler.setTaskPushNotificationConfig(params, context);
434431
const protoResult = ToProto.taskPushNotificationConfig(result);
435432
sendResponse<a2a.TaskPushNotificationConfig>(
436433
res,

‎test/server/express/rest_handler.spec.ts‎

Lines changed: 198 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -119,6 +119,24 @@ describe('restHandler', () => {
119119

120120
const response = await request(app).post('/v1/message:send').send({ message }).expect(201);
121121

122+
expect(mockRequestHandler.sendMessage).toHaveBeenCalledWith(
123+
{
124+
message: {
125+
messageId: 'msg-1',
126+
role: 'user',
127+
parts: [{ kind: 'text', text: 'Hello' }],
128+
kind: 'message',
129+
contextId: undefined,
130+
extensions: [],
131+
metadata: undefined,
132+
taskId: undefined,
133+
},
134+
configuration: undefined,
135+
metadata: undefined,
136+
},
137+
expect.anything()
138+
);
139+
122140
const converted_result = FromProto.sendMessageResult(
123141
SendMessageResponse.fromJSON(response.body)
124142
);
@@ -148,6 +166,24 @@ describe('restHandler', () => {
148166
const response = await request(app).post('/v1/message:stream').send({ message }).expect(200);
149167

150168
assert.equal(response.headers['content-type'], 'text/event-stream');
169+
170+
expect(mockRequestHandler.sendMessageStream).toHaveBeenCalledWith(
171+
{
172+
message: {
173+
messageId: 'msg-1',
174+
role: 'user',
175+
parts: [{ kind: 'text', text: 'Hello' }],
176+
kind: 'message',
177+
contextId: undefined,
178+
extensions: [],
179+
metadata: undefined,
180+
taskId: undefined,
181+
},
182+
configuration: undefined,
183+
metadata: undefined,
184+
},
185+
expect.anything()
186+
);
151187
});
152188

153189
it('should return 400 if streaming is not supported', async () => {
@@ -227,7 +263,8 @@ describe('restHandler', () => {
227263

228264
assert.deepEqual(response.body.id, cancelledTask.id);
229265
assert.deepEqual(response.body.status.state, 'TASK_STATE_CANCELLED');
230-
expect(mockRequestHandler.cancelTask as Mock).toHaveBeenCalledWith(
266+
267+
expect(mockRequestHandler.cancelTask).toHaveBeenCalledWith(
231268
{ id: 'task-1' },
232269
expect.anything()
233270
);
@@ -265,7 +302,8 @@ describe('restHandler', () => {
265302
const response = await request(app).post('/v1/tasks/task-1:subscribe').expect(200);
266303

267304
assert.equal(response.headers['content-type'], 'text/event-stream');
268-
expect(mockRequestHandler.resubscribe as Mock).toHaveBeenCalledWith(
305+
306+
expect(mockRequestHandler.resubscribe).toHaveBeenCalledWith(
269307
{ id: 'task-1' },
270308
expect.anything()
271309
);
@@ -309,7 +347,29 @@ describe('restHandler', () => {
309347
{
310348
name: 'camelCase',
311349
payload: {
312-
pushNotificationConfig: { id: 'config-1', url: 'https://example.com/webhook' },
350+
parent: 'tasks/task-1',
351+
configId: 'push-954f670f-598d-49bf-9981-642d523f7746',
352+
config: {
353+
name: 'tasks/task-1/pushNotificationConfigs/push-954f670f-598d-49bf-9981-642d523f7746',
354+
pushNotificationConfig: {
355+
id: 'push-954f670f-598d-49bf-9981-642d523f7746',
356+
url: 'http://127.0.0.1:9999/webhook',
357+
},
358+
},
359+
},
360+
},
361+
{
362+
name: 'snake_case',
363+
payload: {
364+
parent: 'tasks/task-1',
365+
config_id: 'push-954f670f-598d-49bf-9981-642d523f7746',
366+
config: {
367+
name: 'tasks/task-1/pushNotificationConfigs/push-954f670f-598d-49bf-9981-642d523f7746',
368+
push_notification_config: {
369+
id: 'push-954f670f-598d-49bf-9981-642d523f7746',
370+
url: 'http://127.0.0.1:9999/webhook',
371+
},
372+
},
313373
},
314374
},
315375
])('should accept $name config and return 201', async ({ payload }) => {
@@ -324,6 +384,19 @@ describe('restHandler', () => {
324384
TaskPushNotificationConfig.fromJSON(response.body)
325385
);
326386
assert.deepEqual(protoResponse.taskId, mockConfig.taskId);
387+
388+
expect(mockRequestHandler.setTaskPushNotificationConfig).toHaveBeenCalledWith(
389+
{
390+
taskId: 'task-1',
391+
pushNotificationConfig: {
392+
id: 'push-954f670f-598d-49bf-9981-642d523f7746',
393+
url: 'http://127.0.0.1:9999/webhook',
394+
token: undefined,
395+
authentication: undefined,
396+
},
397+
},
398+
expect.anything()
399+
);
327400
});
328401

329402
it('should return 400 if push notifications not supported', async () => {
@@ -363,6 +436,11 @@ describe('restHandler', () => {
363436
);
364437
assert.isArray(convertedResult);
365438
assert.lengthOf(convertedResult, configs.length);
439+
440+
expect(mockRequestHandler.listTaskPushNotificationConfigs).toHaveBeenCalledWith(
441+
{ id: 'task-1' },
442+
expect.anything()
443+
);
366444
});
367445
});
368446

@@ -379,11 +457,9 @@ describe('restHandler', () => {
379457
TaskPushNotificationConfig.fromJSON(response.body)
380458
);
381459
assert.deepEqual(convertedResult.taskId, mockConfig.taskId);
382-
expect(mockRequestHandler.getTaskPushNotificationConfig as Mock).toHaveBeenCalledWith(
383-
{
384-
id: 'task-1',
385-
pushNotificationConfigId: 'config-1',
386-
},
460+
461+
expect(mockRequestHandler.getTaskPushNotificationConfig).toHaveBeenCalledWith(
462+
{ id: 'task-1', pushNotificationConfigId: 'config-1' },
387463
expect.anything()
388464
);
389465
});
@@ -408,11 +484,8 @@ describe('restHandler', () => {
408484

409485
await request(app).delete('/v1/tasks/task-1/pushNotificationConfigs/config-1').expect(204);
410486

411-
expect(mockRequestHandler.deleteTaskPushNotificationConfig as Mock).toHaveBeenCalledWith(
412-
{
413-
id: 'task-1',
414-
pushNotificationConfigId: 'config-1',
415-
},
487+
expect(mockRequestHandler.deleteTaskPushNotificationConfig).toHaveBeenCalledWith(
488+
{ id: 'task-1', pushNotificationConfigId: 'config-1' },
416489
expect.anything()
417490
);
418491
});
@@ -439,27 +512,93 @@ describe('restHandler', () => {
439512
it.each([
440513
{
441514
name: 'camelCase',
442-
message: {
443-
messageId: 'msg-file',
444-
role: 'user',
445-
kind: 'message',
446-
parts: [
447-
{
448-
kind: 'file',
449-
file: {
450-
uri: 'https://example.com/file.pdf',
451-
mimeType: 'application/pdf',
452-
name: 'document.pdf',
515+
payload: {
516+
message: {
517+
messageId: 'msg-parts',
518+
role: 'ROLE_USER',
519+
kind: 'message',
520+
content: [
521+
{
522+
file: {
523+
fileWithUri: 'https://example.com/file.pdf',
524+
mimeType: 'application/pdf',
525+
},
453526
},
454-
},
455-
],
527+
{
528+
text: 'Hello world',
529+
},
530+
{
531+
data: {
532+
data: { foo: 'bar' },
533+
},
534+
},
535+
],
536+
},
456537
},
457538
},
458-
])('should accept $name file parts', async ({ message }) => {
539+
{
540+
name: 'snake_case',
541+
payload: {
542+
message: {
543+
message_id: 'msg-parts',
544+
role: 'ROLE_USER',
545+
kind: 'message',
546+
content: [
547+
{
548+
file: {
549+
file_with_uri: 'https://example.com/file.pdf',
550+
mime_type: 'application/pdf',
551+
},
552+
},
553+
{
554+
text: 'Hello world',
555+
},
556+
{
557+
data: {
558+
data: { foo: 'bar' },
559+
},
560+
},
561+
],
562+
},
563+
},
564+
},
565+
])('should accept $name message parts', async ({ payload }) => {
459566
(mockRequestHandler.sendMessage as Mock).mockResolvedValue(testTask);
567+
await request(app).post('/v1/message:send').send(payload).expect(201);
460568

461-
const protoMessage = ProtoMessage.toJSON(ToProto.message(message as Message));
462-
await request(app).post('/v1/message:send').send({ message: protoMessage }).expect(201);
569+
expect(mockRequestHandler.sendMessage).toHaveBeenCalledWith(
570+
{
571+
message: {
572+
kind: 'message',
573+
messageId: 'msg-parts',
574+
role: 'user', // ROLE_USER is converted to 'user'
575+
parts: [
576+
{
577+
kind: 'file',
578+
file: {
579+
uri: 'https://example.com/file.pdf',
580+
mimeType: 'application/pdf',
581+
},
582+
},
583+
{
584+
kind: 'text',
585+
text: 'Hello world',
586+
},
587+
{
588+
kind: 'data',
589+
data: { foo: 'bar' },
590+
},
591+
],
592+
contextId: undefined,
593+
extensions: [],
594+
metadata: undefined,
595+
taskId: undefined,
596+
},
597+
configuration: undefined,
598+
metadata: undefined,
599+
},
600+
expect.anything()
601+
);
463602
});
464603
});
465604

@@ -471,15 +610,42 @@ describe('restHandler', () => {
471610
{
472611
name: 'camelCase',
473612
payload: {
474-
message: testMessage,
613+
message: { messageId: 'msg-1', role: 'ROLE_USER', kind: 'message' },
475614
configuration: { acceptedOutputModes: ['text/plain'], historyLength: 5 },
476615
},
477616
},
617+
{
618+
name: 'snake_case',
619+
payload: {
620+
message: { message_id: 'msg-1', role: 'ROLE_USER', kind: 'message' },
621+
configuration: { accepted_output_modes: ['text/plain'], history_length: 5 },
622+
},
623+
},
478624
])('should accept $name configuration fields', async ({ payload }) => {
479625
(mockRequestHandler.sendMessage as Mock).mockResolvedValue(testTask);
626+
await request(app).post('/v1/message:send').send(payload).expect(201);
480627

481-
const protoMessage = ProtoMessage.toJSON(ToProto.message(payload.message as Message));
482-
await request(app).post('/v1/message:send').send({ message: protoMessage }).expect(201);
628+
expect(mockRequestHandler.sendMessage).toHaveBeenCalledWith(
629+
{
630+
message: {
631+
kind: 'message',
632+
messageId: 'msg-1',
633+
role: 'user', // ROLE_USER is converted to 'user'
634+
parts: [], // empty content converts to empty parts
635+
contextId: undefined,
636+
extensions: [],
637+
metadata: undefined,
638+
taskId: undefined,
639+
},
640+
configuration: {
641+
acceptedOutputModes: ['text/plain'],
642+
blocking: false,
643+
pushNotificationConfig: undefined,
644+
},
645+
metadata: undefined,
646+
},
647+
expect.anything()
648+
);
483649
});
484650
});
485651

0 commit comments

Comments
 (0)