[Server] Serve an extension's methods under the modern lifecycle (#454)
* [Server] Serve an extension's methods under the modern lifecycle The modern dispatcher takes the method-to-extension map the builder collects, so a method belonging to an extension this server does not serve is answered -32601 naming the extension instead of a bare "no handler found". It is still an unknown method - the server genuinely does not implement it - but the caller can now act on the answer. * [Server] Reject two extensions claiming the same RPC method The message factory resolves a contested method to whichever class registered first, while extensionMethods kept the last one — so error messages could name the wrong extension. Also fixes a test that claimed to prove a method gets named by its extension while asserting the opposite; the case it meant to cover (an enabled extension with no handler for one of its methods) had no coverage at all.
This commit is contained in:
parent
7e38c94954
commit
b97389325f
5 changed files with 165 additions and 4 deletions
|
|
@ -236,6 +236,9 @@ final class Builder
|
|||
/** @var list<class-string<\Mcp\Schema\JsonRpc\Request>|class-string<\Mcp\Schema\JsonRpc\Notification>> */
|
||||
private array $extensionMessages = [];
|
||||
|
||||
/** @var array<string, string> RPC method to the extension identifier defining it */
|
||||
private array $extensionMethods = [];
|
||||
|
||||
/**
|
||||
* @var LoaderInterface[]
|
||||
*/
|
||||
|
|
@ -389,7 +392,8 @@ final class Builder
|
|||
* for extensions that only announce a capability.
|
||||
*
|
||||
* @throws InvalidArgumentException if the identifier is not a valid `_meta` prefix
|
||||
* @throws LogicException if the same extension is enabled more than once
|
||||
* @throws LogicException if the same extension is enabled more than once, or
|
||||
* two enabled extensions define the same RPC method
|
||||
*/
|
||||
public function enableExtension(ExtensionInterface ...$extensions): self
|
||||
{
|
||||
|
|
@ -405,7 +409,19 @@ final class Builder
|
|||
// Without this the method cannot be decoded at all, so nothing
|
||||
// downstream ever sees it.
|
||||
foreach ($extension->getMessages() as $message) {
|
||||
$method = $message::getMethod();
|
||||
|
||||
// The message factory resolves a method to whichever class was
|
||||
// registered first, so a second owner here would silently lose
|
||||
// the dispatch race while still being named in error messages.
|
||||
if (isset($this->extensionMethods[$method]) && $this->extensionMethods[$method] !== $id) {
|
||||
throw new LogicException(\sprintf('Method "%s" is already claimed by extension "%s", so extension "%s" cannot also define it.', $method, $this->extensionMethods[$method], $id));
|
||||
}
|
||||
|
||||
$this->extensionMessages[] = $message;
|
||||
// Recorded even though the handler answers it, so a server with
|
||||
// the extension *off* can say so instead of "no such method".
|
||||
$this->extensionMethods[$method] = $id;
|
||||
}
|
||||
|
||||
foreach ($extension->getRequestHandlers() as $handler) {
|
||||
|
|
@ -836,6 +852,7 @@ final class Builder
|
|||
: null,
|
||||
cachePolicy: $this->cachePolicy,
|
||||
notificationBus: $this->notificationBus,
|
||||
extensionMethods: $this->extensionMethods,
|
||||
);
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -89,6 +89,7 @@ final class StatelessProtocol
|
|||
/**
|
||||
* @param iterable<RequestHandlerInterface<ResultInterface>> $requestHandlers
|
||||
* @param list<ProtocolVersion> $supportedVersions
|
||||
* @param array<string, string> $extensionMethods RPC method to the extension identifier defining it
|
||||
*/
|
||||
public function __construct(
|
||||
private readonly iterable $requestHandlers,
|
||||
|
|
@ -102,6 +103,7 @@ final class StatelessProtocol
|
|||
private readonly ?RequestStateCodec $requestStateCodec = null,
|
||||
?CachePolicy $cachePolicy = null,
|
||||
private readonly ?NotificationBusInterface $notificationBus = null,
|
||||
private readonly array $extensionMethods = [],
|
||||
) {
|
||||
$this->codec = $codec ?? new Rev2026Codec($configuration->serverInfo, $cachePolicy);
|
||||
|
||||
|
|
@ -396,7 +398,7 @@ final class StatelessProtocol
|
|||
} catch (\Throwable $e) {
|
||||
$this->logger->warning('Rejected an unparseable modern-era request.', ['method' => $method, 'exception' => $e]);
|
||||
|
||||
return StatelessResult::error(Error::forMethodNotFound(\sprintf('Method "%s" is not supported.', $method), $id), 404);
|
||||
return StatelessResult::error($this->unknownMethod($method, $id), 404);
|
||||
}
|
||||
|
||||
$request = $messages[0] ?? null;
|
||||
|
|
@ -410,7 +412,7 @@ final class StatelessProtocol
|
|||
|
||||
return StatelessResult::error(
|
||||
$unknownMethod
|
||||
? Error::forMethodNotFound($request->getMessage(), $id)
|
||||
? $this->unknownMethod($method, $id)
|
||||
: Error::forInvalidRequest($request->getMessage(), $id),
|
||||
$unknownMethod ? 404 : 400,
|
||||
);
|
||||
|
|
@ -510,7 +512,28 @@ final class StatelessProtocol
|
|||
return $this->encode($method, $id, $result->result, null === $input);
|
||||
}
|
||||
|
||||
return StatelessResult::error(Error::forMethodNotFound(\sprintf('No handler found for method "%s".', $method), $id), 404);
|
||||
return StatelessResult::error($this->unknownMethod($method, $id), 404);
|
||||
}
|
||||
|
||||
/**
|
||||
* A method with no handler, said as precisely as the server can.
|
||||
*
|
||||
* An extension's method is still `-32601` when the extension is off — the
|
||||
* server genuinely does not implement it — but naming the extension turns
|
||||
* an opaque refusal into something the caller can act on.
|
||||
*/
|
||||
private function unknownMethod(string $method, string|int $id): Error
|
||||
{
|
||||
$extension = $this->extensionMethods[$method] ?? null;
|
||||
|
||||
if (null !== $extension) {
|
||||
return Error::forMethodNotFound(
|
||||
\sprintf('Method "%s" belongs to the "%s" extension, which this server does not serve.', $method, $extension),
|
||||
$id,
|
||||
);
|
||||
}
|
||||
|
||||
return Error::forMethodNotFound(\sprintf('No handler found for method "%s".', $method), $id);
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
|
|||
|
|
@ -203,6 +203,15 @@ final class BuilderTest extends TestCase
|
|||
$this->assertInstanceOf(ThingListRequest::class, $decoded[0]);
|
||||
}
|
||||
|
||||
#[TestDox('enableExtension() throws when two enabled extensions define the same RPC method')]
|
||||
public function testEnableExtensionRejectsClaimedMethod(): void
|
||||
{
|
||||
$this->expectException(LogicException::class);
|
||||
$this->expectExceptionMessage('com.example/things.list');
|
||||
|
||||
Server::builder()->enableExtension(new ThingExtension('com.example/things-a'), new ThingExtension('com.example/things-b'));
|
||||
}
|
||||
|
||||
#[TestDox('A method-providing extension contributes the handlers serving its methods')]
|
||||
public function testEnableExtensionRegistersItsHandlers(): void
|
||||
{
|
||||
|
|
|
|||
42
tests/Unit/Server/Extension/UnservedThingExtension.php
Normal file
42
tests/Unit/Server/Extension/UnservedThingExtension.php
Normal file
|
|
@ -0,0 +1,42 @@
|
|||
<?php
|
||||
|
||||
/*
|
||||
* This file is part of the official PHP MCP SDK.
|
||||
*
|
||||
* A collaboration between Symfony and the PHP Foundation.
|
||||
*
|
||||
* For the full copyright and license information, please view the LICENSE
|
||||
* file that was distributed with this source code.
|
||||
*/
|
||||
|
||||
namespace Mcp\Tests\Unit\Server\Extension;
|
||||
|
||||
use Mcp\Schema\Extension\ExtensionIdentifier;
|
||||
use Mcp\Schema\Extension\ExtensionInterface;
|
||||
|
||||
/**
|
||||
* An extension that declares a method it does not serve: {@see ThingListRequest}
|
||||
* is registered so the method decodes, but no handler answers it.
|
||||
*/
|
||||
final class UnservedThingExtension implements ExtensionInterface
|
||||
{
|
||||
public function getId(): ExtensionIdentifier
|
||||
{
|
||||
return new ExtensionIdentifier('com.example/unserved-things');
|
||||
}
|
||||
|
||||
public function getCapabilities(): array
|
||||
{
|
||||
return [];
|
||||
}
|
||||
|
||||
public function getMessages(): array
|
||||
{
|
||||
return [ThingListRequest::class];
|
||||
}
|
||||
|
||||
public function getRequestHandlers(): iterable
|
||||
{
|
||||
return [];
|
||||
}
|
||||
}
|
||||
|
|
@ -34,6 +34,8 @@ use Mcp\Server\Stateless\StatelessProtocol;
|
|||
use Mcp\Server\Stateless\StatelessResult;
|
||||
use Mcp\Server\Subscription\InMemoryNotificationBus;
|
||||
use Mcp\Server\Wire\CachePolicy;
|
||||
use Mcp\Tests\Unit\Server\Extension\ThingExtension;
|
||||
use Mcp\Tests\Unit\Server\Extension\UnservedThingExtension;
|
||||
use PHPUnit\Framework\Attributes\DataProvider;
|
||||
use PHPUnit\Framework\Attributes\TestDox;
|
||||
use PHPUnit\Framework\TestCase;
|
||||
|
|
@ -791,6 +793,74 @@ class StatelessProtocolTest extends TestCase
|
|||
$this->assertSame(['toolsListChanged' => true], (array) $first['params']['notifications']);
|
||||
}
|
||||
|
||||
#[TestDox('an extension method is served by the extension that claims it')]
|
||||
public function testExtensionMethodIsServed(): void
|
||||
{
|
||||
$protocol = Server::builder()
|
||||
->setServerInfo('test-server', '1.0.0')
|
||||
->enableExtension(new ThingExtension())
|
||||
->buildStateless([ProtocolVersion::V2026_07_28]);
|
||||
|
||||
$answer = self::callWithHeaders($protocol, 'com.example/things.list', [], [
|
||||
'MCP-Protocol-Version' => ProtocolVersion::V2026_07_28->value,
|
||||
'Mcp-Method' => 'com.example/things.list',
|
||||
]);
|
||||
|
||||
$this->assertSame(200, $answer['status']);
|
||||
$this->assertSame(['a', 'b'], $answer['body']['result']['things']);
|
||||
}
|
||||
|
||||
#[TestDox('the extension is advertised under capabilities.extensions')]
|
||||
public function testExtensionIsAdvertised(): void
|
||||
{
|
||||
$protocol = Server::builder()
|
||||
->setServerInfo('test-server', '1.0.0')
|
||||
->enableExtension(new ThingExtension())
|
||||
->buildStateless([ProtocolVersion::V2026_07_28]);
|
||||
|
||||
$answer = self::call($protocol, 'server/discover');
|
||||
|
||||
$this->assertSame(['flavour' => 'vanilla'], (array) $answer['body']['result']['capabilities']['extensions']['com.example/things']);
|
||||
}
|
||||
|
||||
#[TestDox('a method of an extension this server has never heard of stays generic')]
|
||||
public function testUnknownExtensionMethodStaysGeneric(): void
|
||||
{
|
||||
$protocol = Server::builder()
|
||||
->setServerInfo('test-server', '1.0.0')
|
||||
->buildStateless([ProtocolVersion::V2026_07_28]);
|
||||
|
||||
$answer = self::callWithHeaders($protocol, 'com.example/things.list', [], [
|
||||
'MCP-Protocol-Version' => ProtocolVersion::V2026_07_28->value,
|
||||
'Mcp-Method' => 'com.example/things.list',
|
||||
]);
|
||||
|
||||
$this->assertSame(404, $answer['status']);
|
||||
$this->assertSame(Error::METHOD_NOT_FOUND, $answer['body']['error']['code']);
|
||||
// The extension was never enabled, so it never entered the method map
|
||||
// — there is nothing to name it by.
|
||||
$this->assertStringContainsString('com.example/things.list', $answer['body']['error']['message']);
|
||||
$this->assertStringNotContainsString('extension', $answer['body']['error']['message']);
|
||||
}
|
||||
|
||||
#[TestDox('a method of an extension this server does not serve says so by name')]
|
||||
public function testUnservedExtensionMethodNamesItsExtension(): void
|
||||
{
|
||||
$protocol = Server::builder()
|
||||
->setServerInfo('test-server', '1.0.0')
|
||||
->enableExtension(new UnservedThingExtension())
|
||||
->buildStateless([ProtocolVersion::V2026_07_28]);
|
||||
|
||||
$answer = self::callWithHeaders($protocol, 'com.example/things.list', [], [
|
||||
'MCP-Protocol-Version' => ProtocolVersion::V2026_07_28->value,
|
||||
'Mcp-Method' => 'com.example/things.list',
|
||||
]);
|
||||
|
||||
$this->assertSame(404, $answer['status']);
|
||||
$this->assertSame(Error::METHOD_NOT_FOUND, $answer['body']['error']['code']);
|
||||
$this->assertStringContainsString('com.example/unserved-things', $answer['body']['error']['message']);
|
||||
}
|
||||
|
||||
#[TestDox('a notification is acknowledged with no body, never answered')]
|
||||
public function testNotificationIsAcknowledged(): void
|
||||
{
|
||||
|
|
|
|||
Loading…
Reference in a new issue