[Server] Fix: standardize error handling across handlers with MCP specificatio… (#124)

* fix: standardize error handling across handlers with MCP specification compliance * Update mcp-elements.md Co-authored-by: Christopher Hertel <mail@christopher-hertel.de> * docs: clarify error handling behavior for all element types * docs: remove unnecessary whitespace in mcp-elements.md * fix: simplify exception signatures to accept simple messages * refactor: use specific exceptions in examples for better error handling * feat: throw exceptions from reference provider instead of returning null --------- Co-authored-by: Christopher Hertel <mail@christopher-hertel.de>
2025-10-30 17:30:58 +01:00 · 2025-10-30 17:30:58 +01:00 · ca18caf29d
commit ca18caf29d
parent 8885d29a4e
5 changed files with 278 additions and 185 deletions
--- a/tests/Unit/Capability/Registry/RegistryProviderTest.php
+++ b/tests/Unit/Capability/Registry/RegistryProviderTest.php
@ -16,6 +16,9 @@ use Mcp\Capability\Registry\PromptReference;
 use Mcp\Capability\Registry\ResourceReference;
 use Mcp\Capability\Registry\ResourceTemplateReference;
 use Mcp\Capability\Registry\ToolReference;
+use Mcp\Exception\PromptNotFoundException;
+use Mcp\Exception\ResourceNotFoundException;
+use Mcp\Exception\ToolNotFoundException;
 use Mcp\Schema\Prompt;
 use Mcp\Schema\Resource;
 use Mcp\Schema\ResourceTemplate;
@ -45,10 +48,12 @@ class RegistryProviderTest extends TestCase
        $this->assertFalse($toolRef->isManual);
    }

-    public function testGetToolReturnsNullForUnregisteredTool(): void
+    public function testGetToolThrowsExceptionForUnregisteredTool(): void
    {
-        $toolRef = $this->registry->getTool('non_existent_tool');
-        $this->assertNull($toolRef);
+        $this->expectException(ToolNotFoundException::class);
+        $this->expectExceptionMessage('Tool not found: "non_existent_tool".');
+
+        $this->registry->getTool('non_existent_tool');
    }

    public function testGetResourceReturnsRegisteredResource(): void
@ -65,10 +70,12 @@ class RegistryProviderTest extends TestCase
        $this->assertFalse($resourceRef->isManual);
    }

-    public function testGetResourceReturnsNullForUnregisteredResource(): void
+    public function testGetResourceThrowsExceptionForUnregisteredResource(): void
    {
-        $resourceRef = $this->registry->getResource('test://non_existent');
-        $this->assertNull($resourceRef);
+        $this->expectException(ResourceNotFoundException::class);
+        $this->expectExceptionMessage('Resource not found for uri: "test://non_existent".');
+
+        $this->registry->getResource('test://non_existent');
    }

    public function testGetResourceMatchesResourceTemplate(): void
@ -84,15 +91,17 @@ class RegistryProviderTest extends TestCase
        $this->assertEquals($handler, $resourceRef->handler);
    }

-    public function testGetResourceWithIncludeTemplatesFalse(): void
+    public function testGetResourceWithIncludeTemplatesFalseThrowsException(): void
    {
        $template = $this->createValidResourceTemplate('test://{id}');
        $handler = fn (string $id) => "content for {$id}";

        $this->registry->registerResourceTemplate($template, $handler);

-        $resourceRef = $this->registry->getResource('test://123', false);
-        $this->assertNull($resourceRef);
+        $this->expectException(ResourceNotFoundException::class);
+        $this->expectExceptionMessage('Resource not found for uri: "test://123".');
+
+        $this->registry->getResource('test://123', false);
    }

    public function testGetResourcePrefersDirectResourceOverTemplate(): void
@ -125,10 +134,12 @@ class RegistryProviderTest extends TestCase
        $this->assertFalse($templateRef->isManual);
    }

-    public function testGetResourceTemplateReturnsNullForUnregisteredTemplate(): void
+    public function testGetResourceTemplateThrowsExceptionForUnregisteredTemplate(): void
    {
-        $templateRef = $this->registry->getResourceTemplate('test://{non_existent}');
-        $this->assertNull($templateRef);
+        $this->expectException(ResourceNotFoundException::class);
+        $this->expectExceptionMessage('Resource not found for uri: "test://{non_existent}".');
+
+        $this->registry->getResourceTemplate('test://{non_existent}');
    }

    public function testGetPromptReturnsRegisteredPrompt(): void
@ -145,10 +156,12 @@ class RegistryProviderTest extends TestCase
        $this->assertFalse($promptRef->isManual);
    }

-    public function testGetPromptReturnsNullForUnregisteredPrompt(): void
+    public function testGetPromptThrowsExceptionForUnregisteredPrompt(): void
    {
-        $promptRef = $this->registry->getPrompt('non_existent_prompt');
-        $this->assertNull($promptRef);
+        $this->expectException(PromptNotFoundException::class);
+        $this->expectExceptionMessage('Prompt not found: "non_existent_prompt".');
+
+        $this->registry->getPrompt('non_existent_prompt');
    }

    public function testGetToolsReturnsAllRegisteredTools(): void
--- a/tests/Unit/Capability/Registry/RegistryTest.php
+++ b/tests/Unit/Capability/Registry/RegistryTest.php
@ -13,6 +13,9 @@ namespace Mcp\Tests\Unit\Capability\Registry;

 use Mcp\Capability\Completion\EnumCompletionProvider;
 use Mcp\Capability\Registry;
+use Mcp\Exception\PromptNotFoundException;
+use Mcp\Exception\ResourceNotFoundException;
+use Mcp\Exception\ToolNotFoundException;
 use Mcp\Schema\Prompt;
 use Mcp\Schema\Resource;
 use Mcp\Schema\ResourceTemplate;
@ -264,23 +267,36 @@ class RegistryTest extends TestCase
        $this->registry->registerResourceTemplate($manualTemplate, fn () => 'manual', [], true);
        $this->registry->registerResourceTemplate($discoveredTemplate, fn () => 'discovered', [], false);

-        $this->logger
-            ->expects($this->once())
-            ->method('debug')
-            ->with('Removed 4 discovered elements from internal registry.');
+        // Test that all elements exist
+        $this->registry->getTool('manual_tool');
+        $this->registry->getResource('test://manual');
+        $this->registry->getPrompt('manual_prompt');
+        $this->registry->getResourceTemplate('manual://{id}');
+        $this->registry->getTool('discovered_tool');
+        $this->registry->getResource('test://discovered');
+        $this->registry->getPrompt('discovered_prompt');
+        $this->registry->getResourceTemplate('discovered://{id}');

        $this->registry->clear();

-        $this->assertNotNull($this->registry->getTool('manual_tool'));
-        $this->assertNull($this->registry->getTool('discovered_tool'));
-        $this->assertNotNull($this->registry->getResource('test://manual'));
-        $this->assertNull(
-            $this->registry->getResource('test://discovered', false),
-        ); // Don't include templates to avoid debug log
-        $this->assertNotNull($this->registry->getPrompt('manual_prompt'));
-        $this->assertNull($this->registry->getPrompt('discovered_prompt'));
-        $this->assertNotNull($this->registry->getResourceTemplate('manual://{id}'));
-        $this->assertNull($this->registry->getResourceTemplate('discovered://{id}'));
+        // Manual elements should still exist
+        $this->registry->getTool('manual_tool');
+        $this->registry->getResource('test://manual');
+        $this->registry->getPrompt('manual_prompt');
+        $this->registry->getResourceTemplate('manual://{id}');
+
+        // Test that all discovered elements throw exceptions
+        $this->expectException(ToolNotFoundException::class);
+        $this->registry->getTool('discovered_tool');
+
+        $this->expectException(ResourceNotFoundException::class);
+        $this->registry->getResource('test://discovered');
+
+        $this->expectException(PromptNotFoundException::class);
+        $this->registry->getPrompt('discovered_prompt');
+
+        $this->expectException(ResourceNotFoundException::class);
+        $this->registry->getResourceTemplate('discovered://{id}');
    }

    public function testClearLogsNothingWhenNoDiscoveredElements(): void
@ -294,7 +310,7 @@ class RegistryTest extends TestCase

        $this->registry->clear();

-        $this->assertNotNull($this->registry->getTool('manual_tool'));
+        $this->registry->getTool('manual_tool');
    }

    public function testRegisterToolHandlesStringHandler(): void
--- a/tests/Unit/Server/Handler/Request/CallToolHandlerTest.php
+++ b/tests/Unit/Server/Handler/Request/CallToolHandlerTest.php
@ -164,7 +164,7 @@ class CallToolHandlerTest extends TestCase
            ->expects($this->once())
            ->method('getTool')
            ->with('nonexistent_tool')
-            ->willThrowException(new ToolNotFoundException($request));
+            ->willThrowException(new ToolNotFoundException('nonexistent_tool'));

        $this->logger
            ->expects($this->once())
@ -177,10 +177,10 @@ class CallToolHandlerTest extends TestCase
        $this->assertEquals(Error::METHOD_NOT_FOUND, $response->code);
    }

-    public function testHandleToolExecutionExceptionReturnsError(): void
+    public function testHandleToolCallExceptionReturnsResponseWithErrorResult(): void
    {
        $request = $this->createCallToolRequest('failing_tool', ['param' => 'value']);
-        $exception = new ToolCallException($request, new \RuntimeException('Tool execution failed'));
+        $exception = new ToolCallException('Tool execution failed');

        $toolReference = $this->createMock(ToolReference::class);
        $this->referenceProvider
@ -201,9 +201,15 @@ class CallToolHandlerTest extends TestCase

        $response = $this->handler->handle($request, $this->session);

-        $this->assertInstanceOf(Error::class, $response);
+        $this->assertInstanceOf(Response::class, $response);
        $this->assertEquals($request->getId(), $response->id);
-        $this->assertEquals(Error::INTERNAL_ERROR, $response->code);
+
+        $result = $response->result;
+        $this->assertInstanceOf(CallToolResult::class, $result);
+        $this->assertTrue($result->isError);
+        $this->assertCount(1, $result->content);
+        $this->assertInstanceOf(TextContent::class, $result->content[0]);
+        $this->assertEquals('Tool execution failed', $result->content[0]->text);
    }

    public function testHandleWithNullResult(): void
@ -246,7 +252,7 @@ class CallToolHandlerTest extends TestCase
    public function testHandleLogsErrorWithCorrectParameters(): void
    {
        $request = $this->createCallToolRequest('test_tool', ['key1' => 'value1', 'key2' => 42]);
-        $exception = new ToolCallException($request, new \RuntimeException('Custom error message'));
+        $exception = new ToolCallException('Custom error message');

        $toolReference = $this->createMock(ToolReference::class);
        $this->referenceProvider
@ -265,7 +271,7 @@ class CallToolHandlerTest extends TestCase
            ->expects($this->once())
            ->method('error')
            ->with(
-                'Error while executing tool "test_tool": "Tool call "test_tool" failed with error: "Custom error message".".',
+                'Error while executing tool "test_tool": "Custom error message".',
                [
                    'tool' => 'test_tool',
                    'arguments' => ['key1' => 'value1', 'key2' => 42, '_session' => $this->session],
@ -274,8 +280,43 @@ class CallToolHandlerTest extends TestCase

        $response = $this->handler->handle($request, $this->session);

+        // ToolCallException should now return Response with CallToolResult having isError=true
+        $this->assertInstanceOf(Response::class, $response);
+        $this->assertEquals($request->getId(), $response->id);
+
+        $result = $response->result;
+        $this->assertInstanceOf(CallToolResult::class, $result);
+        $this->assertTrue($result->isError);
+        $this->assertCount(1, $result->content);
+        $this->assertInstanceOf(TextContent::class, $result->content[0]);
+        $this->assertEquals('Custom error message', $result->content[0]->text);
+    }
+
+    public function testHandleGenericExceptionReturnsError(): void
+    {
+        $request = $this->createCallToolRequest('failing_tool', ['param' => 'value']);
+        $exception = new \RuntimeException('Internal database connection failed');
+
+        $toolReference = $this->createMock(ToolReference::class);
+        $this->referenceProvider
+            ->expects($this->once())
+            ->method('getTool')
+            ->with('failing_tool')
+            ->willReturn($toolReference);
+
+        $this->referenceHandler
+            ->expects($this->once())
+            ->method('handle')
+            ->with($toolReference, ['param' => 'value', '_session' => $this->session])
+            ->willThrowException($exception);
+
+        $response = $this->handler->handle($request, $this->session);
+
+        // Generic exceptions should return Error, not Response
        $this->assertInstanceOf(Error::class, $response);
+        $this->assertEquals($request->getId(), $response->id);
        $this->assertEquals(Error::INTERNAL_ERROR, $response->code);
+        $this->assertEquals('Error while executing tool', $response->message);
    }

    public function testHandleWithSpecialCharactersInToolName(): void
--- a/tests/Unit/Server/Handler/Request/GetPromptHandlerTest.php
+++ b/tests/Unit/Server/Handler/Request/GetPromptHandlerTest.php
@ -231,7 +231,7 @@ class GetPromptHandlerTest extends TestCase
    public function testHandlePromptNotFoundExceptionReturnsError(): void
    {
        $request = $this->createGetPromptRequest('nonexistent_prompt');
-        $exception = new PromptNotFoundException($request);
+        $exception = new PromptNotFoundException('nonexistent_prompt');

        $this->referenceProvider
            ->expects($this->once())
@ -243,14 +243,14 @@ class GetPromptHandlerTest extends TestCase

        $this->assertInstanceOf(Error::class, $response);
        $this->assertEquals($request->getId(), $response->id);
-        $this->assertEquals(Error::METHOD_NOT_FOUND, $response->code);
-        $this->assertEquals('Prompt not found for name: "nonexistent_prompt".', $response->message);
+        $this->assertEquals(Error::RESOURCE_NOT_FOUND, $response->code);
+        $this->assertEquals('Prompt not found: "nonexistent_prompt".', $response->message);
    }

    public function testHandlePromptGetExceptionReturnsError(): void
    {
        $request = $this->createGetPromptRequest('failing_prompt');
-        $exception = new PromptGetException($request, new \RuntimeException('Failed to get prompt'));
+        $exception = new PromptGetException('Failed to get prompt');

        $this->referenceProvider
            ->expects($this->once())
@ -263,7 +263,7 @@ class GetPromptHandlerTest extends TestCase
        $this->assertInstanceOf(Error::class, $response);
        $this->assertEquals($request->getId(), $response->id);
        $this->assertEquals(Error::INTERNAL_ERROR, $response->code);
-        $this->assertEquals('Error while handling prompt: Handling prompt "failing_prompt" failed with error: "Failed to get prompt".', $response->message);
+        $this->assertEquals('Failed to get prompt', $response->message);
    }

    public function testHandlePromptGetWithComplexArguments(): void
--- a/tests/Unit/Server/Handler/Request/ReadResourceHandlerTest.php
+++ b/tests/Unit/Server/Handler/Request/ReadResourceHandlerTest.php
@ -178,7 +178,7 @@ class ReadResourceHandlerTest extends TestCase
    {
        $uri = 'file://nonexistent/file.txt';
        $request = $this->createReadResourceRequest($uri);
-        $exception = new ResourceNotFoundException($request);
+        $exception = new ResourceNotFoundException($uri);

        $this->referenceProvider
            ->expects($this->once())
@ -194,14 +194,31 @@ class ReadResourceHandlerTest extends TestCase
        $this->assertEquals('Resource not found for uri: "'.$uri.'".', $response->message);
    }

-    public function testHandleResourceReadExceptionReturnsGenericError(): void
+    public function testHandleResourceReadExceptionReturnsActualErrorMessage(): void
    {
        $uri = 'file://corrupted/file.txt';
        $request = $this->createReadResourceRequest($uri);
-        $exception = new ResourceReadException(
-            $request,
-            new \RuntimeException('Failed to read resource: corrupted data'),
-        );
+        $exception = new ResourceReadException('Failed to read resource: corrupted data');
+
+        $this->referenceProvider
+            ->expects($this->once())
+            ->method('getResource')
+            ->with($uri)
+            ->willThrowException($exception);
+
+        $response = $this->handler->handle($request, $this->session);
+
+        $this->assertInstanceOf(Error::class, $response);
+        $this->assertEquals($request->getId(), $response->id);
+        $this->assertEquals(Error::INTERNAL_ERROR, $response->code);
+        $this->assertEquals('Failed to read resource: corrupted data', $response->message);
+    }
+
+    public function testHandleGenericExceptionReturnsGenericError(): void
+    {
+        $uri = 'file://problematic/file.txt';
+        $request = $this->createReadResourceRequest($uri);
+        $exception = new \RuntimeException('Internal database connection failed');

        $this->referenceProvider
            ->expects($this->once())
@ -382,7 +399,7 @@ class ReadResourceHandlerTest extends TestCase
    {
        $uri = 'file://custom/missing.txt';
        $request = $this->createReadResourceRequest($uri);
-        $exception = new ResourceNotFoundException($request);
+        $exception = new ResourceNotFoundException($uri);

        $this->referenceProvider
            ->expects($this->once())