[Server] Fix: ensure JSON-RPC error responses include message IDs (#77)
* fix: ensure JSON-RPC error responses include message IDs * fix: replace method_exists with instanceof for message ID extraction
This commit is contained in:
parent
325b4aaf9c
commit
32b8b2ea61
3 changed files with 38 additions and 35 deletions
|
|
@ -23,6 +23,7 @@ use Mcp\Exception\NotFoundExceptionInterface;
|
||||||
use Mcp\Schema\Implementation;
|
use Mcp\Schema\Implementation;
|
||||||
use Mcp\Schema\JsonRpc\Error;
|
use Mcp\Schema\JsonRpc\Error;
|
||||||
use Mcp\Schema\JsonRpc\HasMethodInterface;
|
use Mcp\Schema\JsonRpc\HasMethodInterface;
|
||||||
|
use Mcp\Schema\JsonRpc\Request;
|
||||||
use Mcp\Schema\JsonRpc\Response;
|
use Mcp\Schema\JsonRpc\Response;
|
||||||
use Mcp\Schema\Request\InitializeRequest;
|
use Mcp\Schema\Request\InitializeRequest;
|
||||||
use Mcp\Server\MethodHandlerInterface;
|
use Mcp\Server\MethodHandlerInterface;
|
||||||
|
|
@ -94,9 +95,6 @@ class Handler
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* @return iterable<array{string|null, array<string, mixed>}>
|
* @return iterable<array{string|null, array<string, mixed>}>
|
||||||
*
|
|
||||||
* @throws ExceptionInterface When a handler throws an exception during message processing
|
|
||||||
* @throws \JsonException When JSON encoding of the response fails
|
|
||||||
*/
|
*/
|
||||||
public function process(string $input, ?Uuid $sessionId): iterable
|
public function process(string $input, ?Uuid $sessionId): iterable
|
||||||
{
|
{
|
||||||
|
|
@ -163,7 +161,7 @@ class Handler
|
||||||
foreach ($messages as $message) {
|
foreach ($messages as $message) {
|
||||||
if ($message instanceof InvalidInputMessageException) {
|
if ($message instanceof InvalidInputMessageException) {
|
||||||
$this->logger->warning('Failed to create message.', ['exception' => $message]);
|
$this->logger->warning('Failed to create message.', ['exception' => $message]);
|
||||||
$error = Error::forInvalidRequest($message->getMessage(), 0);
|
$error = Error::forInvalidRequest($message->getMessage());
|
||||||
yield [$this->encodeResponse($error), []];
|
yield [$this->encodeResponse($error), []];
|
||||||
continue;
|
continue;
|
||||||
}
|
}
|
||||||
|
|
@ -172,6 +170,8 @@ class Handler
|
||||||
'method' => $message->getMethod(),
|
'method' => $message->getMethod(),
|
||||||
]);
|
]);
|
||||||
|
|
||||||
|
$messageId = $message instanceof Request ? $message->getId() : 0;
|
||||||
|
|
||||||
try {
|
try {
|
||||||
$response = $this->handle($message, $session);
|
$response = $this->handle($message, $session);
|
||||||
yield [$this->encodeResponse($response), ['session_id' => $session->getId()]];
|
yield [$this->encodeResponse($response), ['session_id' => $session->getId()]];
|
||||||
|
|
@ -183,17 +183,17 @@ class Handler
|
||||||
['exception' => $e],
|
['exception' => $e],
|
||||||
);
|
);
|
||||||
|
|
||||||
$error = Error::forMethodNotFound($e->getMessage());
|
$error = Error::forMethodNotFound($e->getMessage(), $messageId);
|
||||||
yield [$this->encodeResponse($error), []];
|
yield [$this->encodeResponse($error), []];
|
||||||
} catch (\InvalidArgumentException $e) {
|
} catch (\InvalidArgumentException $e) {
|
||||||
$this->logger->warning(\sprintf('Invalid argument: %s', $e->getMessage()), ['exception' => $e]);
|
$this->logger->warning(\sprintf('Invalid argument: %s', $e->getMessage()), ['exception' => $e]);
|
||||||
|
|
||||||
$error = Error::forInvalidParams($e->getMessage());
|
$error = Error::forInvalidParams($e->getMessage(), $messageId);
|
||||||
yield [$this->encodeResponse($error), []];
|
yield [$this->encodeResponse($error), []];
|
||||||
} catch (\Throwable $e) {
|
} catch (\Throwable $e) {
|
||||||
$this->logger->critical(\sprintf('Uncaught exception: %s', $e->getMessage()), ['exception' => $e]);
|
$this->logger->critical(\sprintf('Uncaught exception: %s', $e->getMessage()), ['exception' => $e]);
|
||||||
|
|
||||||
$error = Error::forInternalError($e->getMessage());
|
$error = Error::forInternalError($e->getMessage(), $messageId);
|
||||||
yield [$this->encodeResponse($error), []];
|
yield [$this->encodeResponse($error), []];
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
@ -202,7 +202,7 @@ class Handler
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* @throws \JsonException When JSON encoding fails
|
* Encodes a response to JSON, handling encoding errors gracefully.
|
||||||
*/
|
*/
|
||||||
private function encodeResponse(Response|Error|null $response): ?string
|
private function encodeResponse(Response|Error|null $response): ?string
|
||||||
{
|
{
|
||||||
|
|
@ -214,11 +214,26 @@ class Handler
|
||||||
|
|
||||||
$this->logger->info('Encoding response.', ['response' => $response]);
|
$this->logger->info('Encoding response.', ['response' => $response]);
|
||||||
|
|
||||||
if ($response instanceof Response && [] === $response->result) {
|
try {
|
||||||
return json_encode($response, \JSON_THROW_ON_ERROR | \JSON_FORCE_OBJECT);
|
if ($response instanceof Response && [] === $response->result) {
|
||||||
}
|
return json_encode($response, \JSON_THROW_ON_ERROR | \JSON_FORCE_OBJECT);
|
||||||
|
}
|
||||||
|
|
||||||
return json_encode($response, \JSON_THROW_ON_ERROR);
|
return json_encode($response, \JSON_THROW_ON_ERROR);
|
||||||
|
} catch (\JsonException $e) {
|
||||||
|
$this->logger->error('Failed to encode response to JSON.', [
|
||||||
|
'message_id' => $response->getId(),
|
||||||
|
'exception' => $e,
|
||||||
|
]);
|
||||||
|
|
||||||
|
$fallbackError = new Error(
|
||||||
|
id: $response->getId(),
|
||||||
|
code: Error::INTERNAL_ERROR,
|
||||||
|
message: 'Response could not be encoded to JSON'
|
||||||
|
);
|
||||||
|
|
||||||
|
return json_encode($fallbackError, \JSON_THROW_ON_ERROR);
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
|
|
|
||||||
|
|
@ -44,19 +44,12 @@ final class Server
|
||||||
]);
|
]);
|
||||||
|
|
||||||
$transport->onMessage(function (string $message, ?Uuid $sessionId) use ($transport) {
|
$transport->onMessage(function (string $message, ?Uuid $sessionId) use ($transport) {
|
||||||
try {
|
foreach ($this->jsonRpcHandler->process($message, $sessionId) as [$response, $context]) {
|
||||||
foreach ($this->jsonRpcHandler->process($message, $sessionId) as [$response, $context]) {
|
if (null === $response) {
|
||||||
if (null === $response) {
|
continue;
|
||||||
continue;
|
|
||||||
}
|
|
||||||
|
|
||||||
$transport->send($response, $context);
|
|
||||||
}
|
}
|
||||||
} catch (\JsonException $e) {
|
|
||||||
$this->logger->error('Failed to encode response to JSON.', [
|
$transport->send($response, $context);
|
||||||
'message' => $message,
|
|
||||||
'exception' => $e,
|
|
||||||
]);
|
|
||||||
}
|
}
|
||||||
});
|
});
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -14,27 +14,19 @@ namespace Mcp\Tests;
|
||||||
use Mcp\JsonRpc\Handler;
|
use Mcp\JsonRpc\Handler;
|
||||||
use Mcp\Server;
|
use Mcp\Server;
|
||||||
use Mcp\Server\Transport\InMemoryTransport;
|
use Mcp\Server\Transport\InMemoryTransport;
|
||||||
use PHPUnit\Framework\MockObject\Stub\Exception;
|
|
||||||
use PHPUnit\Framework\TestCase;
|
use PHPUnit\Framework\TestCase;
|
||||||
use Psr\Log\NullLogger;
|
|
||||||
|
|
||||||
class ServerTest extends TestCase
|
class ServerTest extends TestCase
|
||||||
{
|
{
|
||||||
public function testJsonExceptions()
|
public function testJsonExceptions()
|
||||||
{
|
{
|
||||||
$logger = $this->getMockBuilder(NullLogger::class)
|
|
||||||
->disableOriginalConstructor()
|
|
||||||
->onlyMethods(['error'])
|
|
||||||
->getMock();
|
|
||||||
$logger->expects($this->once())->method('error');
|
|
||||||
|
|
||||||
$handler = $this->getMockBuilder(Handler::class)
|
$handler = $this->getMockBuilder(Handler::class)
|
||||||
->disableOriginalConstructor()
|
->disableOriginalConstructor()
|
||||||
->onlyMethods(['process'])
|
->onlyMethods(['process'])
|
||||||
->getMock();
|
->getMock();
|
||||||
|
|
||||||
$handler->expects($this->exactly(2))->method('process')->willReturnOnConsecutiveCalls(
|
$handler->expects($this->exactly(2))->method('process')->willReturnOnConsecutiveCalls(
|
||||||
new Exception(new \JsonException('foobar')),
|
[['{"jsonrpc":"2.0","id":0,"error":{"code":-32700,"message":"Parse error"}}', []]],
|
||||||
[['success', []]]
|
[['success', []]]
|
||||||
);
|
);
|
||||||
|
|
||||||
|
|
@ -42,9 +34,12 @@ class ServerTest extends TestCase
|
||||||
->setConstructorArgs([['foo', 'bar']])
|
->setConstructorArgs([['foo', 'bar']])
|
||||||
->onlyMethods(['send'])
|
->onlyMethods(['send'])
|
||||||
->getMock();
|
->getMock();
|
||||||
$transport->expects($this->once())->method('send')->with('success', []);
|
$transport->expects($this->exactly(2))->method('send')->willReturnOnConsecutiveCalls(
|
||||||
|
null,
|
||||||
|
null
|
||||||
|
);
|
||||||
|
|
||||||
$server = new Server($handler, $logger);
|
$server = new Server($handler);
|
||||||
$server->connect($transport);
|
$server->connect($transport);
|
||||||
|
|
||||||
$transport->listen();
|
$transport->listen();
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue