Skip to content

Commit e7d27fd

Browse files
Nemo64Nemo64
committed
refactor: unify exception handling and streamline tool execution logic
- Introduce `ExceptionHandlerTrait` in `AbstractTool` for consistent error handling across tools. - Replace individual `execute` methods with a templated `doExecute` pattern in all tools, ensuring uniform processing. - Standardize validation logic and replace inline error results with structured exceptions for enhanced clarity. - Remove redundant methods like `initializeWorkspaceContext`, integrating them into `initialize` for improved abstraction. - Enhance tools like `GetPageTool`, `GetPageTreeTool`, and `WriteTableTool` with refined validation, error messaging, and parameter handling. - Update functional tests to align with these refactored patterns and improve overall coverage.
1 parent 47a5c28 commit e7d27fd

21 files changed

+901
-260
lines changed

‎Classes/Exception/AccessDeniedException.php‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
namespace Hn\McpServer\Exception;
6+
7+
/**
8+
* Exception for access-related errors
9+
*
10+
* Thrown when a user attempts to access a resource without
11+
* proper permissions. Maps to HTTP 403 Forbidden status.
12+
*/
13+
class AccessDeniedException extends McpException
14+
{
15+
/**
16+
* @param string $resource The resource being accessed (e.g., table name, record identifier)
17+
* @param string $operation The operation being attempted (e.g., read, write, delete)
18+
* @param \Throwable|null $previous Previous exception for chaining
19+
*/
20+
public function __construct(string $resource, string $operation, ?\Throwable $previous = null)
21+
{
22+
parent::__construct(
23+
"Access denied to {$resource} for operation {$operation}",
24+
"You don't have permission to {$operation} this resource",
25+
403,
26+
$previous,
27+
['resource' => $resource, 'operation' => $operation]
28+
);
29+
}
30+
}

‎Classes/Exception/ConfigurationException.php‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
namespace Hn\McpServer\Exception;
6+
7+
/**
8+
* Exception for configuration errors
9+
*
10+
* Thrown when system configuration is missing, invalid,
11+
* or incompatible. Maps to HTTP 500 Internal Server Error
12+
* status as these are system-level issues.
13+
*/
14+
class ConfigurationException extends McpException
15+
{
16+
/**
17+
* @param string $config The configuration element that caused the error (e.g., "TCA", "site configuration")
18+
* @param string $reason Detailed reason for the configuration error
19+
* @param \Throwable|null $previous Previous exception for chaining
20+
*/
21+
public function __construct(string $config, string $reason, ?\Throwable $previous = null)
22+
{
23+
parent::__construct(
24+
"Configuration error for {$config}: {$reason}",
25+
"System configuration error",
26+
500,
27+
$previous,
28+
['config' => $config, 'reason' => $reason]
29+
);
30+
}
31+
}

‎Classes/Exception/DatabaseException.php‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
namespace Hn\McpServer\Exception;
6+
7+
/**
8+
* Exception for database-related errors
9+
*
10+
* Thrown when database operations fail. Maps to HTTP 500
11+
* Internal Server Error status as these are typically
12+
* system-level issues.
13+
*/
14+
class DatabaseException extends McpException
15+
{
16+
/**
17+
* @param string $operation The database operation that failed (e.g., select, insert, update, delete)
18+
* @param string $table The table name involved in the operation
19+
* @param \Throwable|null $previous Previous exception for chaining (often a Doctrine DBAL exception)
20+
*/
21+
public function __construct(string $operation, string $table, ?\Throwable $previous = null)
22+
{
23+
parent::__construct(
24+
"Database error during {$operation} on table {$table}: " . ($previous ? $previous->getMessage() : 'Unknown error'),
25+
"Failed to {$operation} record",
26+
500,
27+
$previous,
28+
['operation' => $operation, 'table' => $table]
29+
);
30+
}
31+
}

‎Classes/Exception/McpException.php‎

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,57 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
namespace Hn\McpServer\Exception;
6+
7+
/**
8+
* Base exception for all MCP Server exceptions
9+
*
10+
* This exception provides a consistent interface for error handling
11+
* across the MCP Server extension, including user-friendly messages
12+
* and context information for logging.
13+
*/
14+
class McpException extends \RuntimeException
15+
{
16+
protected string $userMessage;
17+
protected array $context = [];
18+
19+
/**
20+
* @param string $message Internal message for logging
21+
* @param string $userMessage User-friendly message for display
22+
* @param int $code Error code (HTTP status code conventions)
23+
* @param \Throwable|null $previous Previous exception for chaining
24+
* @param array $context Additional context for logging
25+
*/
26+
public function __construct(
27+
string $message,
28+
string $userMessage = '',
29+
int $code = 0,
30+
?\Throwable $previous = null,
31+
array $context = []
32+
) {
33+
parent::__construct($message, $code, $previous);
34+
$this->userMessage = $userMessage ?: $message;
35+
$this->context = $context;
36+
}
37+
38+
/**
39+
* Get the user-friendly error message
40+
*
41+
* @return string Message safe to display to end users
42+
*/
43+
public function getUserMessage(): string
44+
{
45+
return $this->userMessage;
46+
}
47+
48+
/**
49+
* Get additional context information
50+
*
51+
* @return array Context data for logging
52+
*/
53+
public function getContext(): array
54+
{
55+
return $this->context;
56+
}
57+
}

‎Classes/Exception/ValidationException.php‎

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
namespace Hn\McpServer\Exception;
6+
7+
/**
8+
* Exception for validation errors
9+
*
10+
* Thrown when input data fails validation rules.
11+
* Maps to HTTP 400 Bad Request status.
12+
*/
13+
class ValidationException extends McpException
14+
{
15+
private array $errors;
16+
17+
/**
18+
* @param array $errors Array of validation error messages
19+
* @param \Throwable|null $previous Previous exception for chaining
20+
*/
21+
public function __construct(array $errors, ?\Throwable $previous = null)
22+
{
23+
$this->errors = $errors;
24+
$errorList = implode(', ', $errors);
25+
26+
parent::__construct(
27+
"Validation failed: {$errorList}",
28+
"Invalid input: {$errorList}",
29+
400,
30+
$previous,
31+
['errors' => $errors]
32+
);
33+
}
34+
35+
/**
36+
* Get the validation errors
37+
*
38+
* @return array Array of validation error messages
39+
*/
40+
public function getErrors(): array
41+
{
42+
return $this->errors;
43+
}
44+
}

‎Classes/MCP/Tool/AbstractTool.php‎

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,11 +4,21 @@
44

55
namespace Hn\McpServer\MCP\Tool;
66

7+
use Hn\McpServer\Traits\ExceptionHandlerTrait;
8+
use Mcp\Types\CallToolResult;
9+
use Mcp\Types\TextContent;
10+
711
/**
812
* Abstract base class for MCP tools
13+
*
14+
* Implements the Template Method pattern for consistent error handling
15+
* across all tools. The execute() method is final and handles all
16+
* exceptions, while subclasses implement doExecute() for their logic.
917
*/
1018
abstract class AbstractTool implements ToolInterface
1119
{
20+
use ExceptionHandlerTrait;
21+
1222
/**
1323
* Get the tool name based on the class name
1424
*/
@@ -17,4 +27,60 @@ public function getName(): string
1727
$className = (new \ReflectionClass($this))->getShortName();
1828
return str_replace('Tool', '', $className);
1929
}
30+
31+
/**
32+
* Execute the tool with the given parameters
33+
*
34+
* This method is final to ensure consistent error handling across all tools.
35+
* Subclasses should implement doExecute() for their specific logic.
36+
*
37+
* @param array $params
38+
* @return CallToolResult
39+
*/
40+
final public function execute(array $params): CallToolResult
41+
{
42+
try {
43+
// Initialize any necessary context (overridden in subclasses)
44+
$this->initialize();
45+
46+
// Execute the actual tool logic
47+
return $this->doExecute($params);
48+
} catch (\Throwable $e) {
49+
// Use the trait's exception handler for consistent logging and messaging
50+
return $this->handleException($e, $this->getName());
51+
}
52+
}
53+
54+
/**
55+
* Initialize any necessary context before execution
56+
*
57+
* Override this method in subclasses to perform initialization
58+
* such as workspace context setup.
59+
*/
60+
protected function initialize(): void
61+
{
62+
// Default implementation does nothing
63+
// Subclasses can override to add initialization logic
64+
}
65+
66+
/**
67+
* Execute the tool logic
68+
*
69+
* This method must be implemented by subclasses to provide
70+
* the actual tool functionality. Any exceptions thrown will
71+
* be handled by the execute() method.
72+
*
73+
* @param array $params
74+
* @return CallToolResult
75+
* @throws \Exception Any exception thrown will be handled by execute()
76+
*/
77+
abstract protected function doExecute(array $params): CallToolResult;
78+
79+
/**
80+
* Create an error result (required by ExceptionHandlerTrait)
81+
*/
82+
protected function createErrorResult(string $message): CallToolResult
83+
{
84+
return new CallToolResult([new TextContent($message)], true);
85+
}
2086
}

‎Classes/MCP/Tool/GetPageTool.php‎

Lines changed: 22 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -96,23 +96,18 @@ public function getSchema(): array
9696
}
9797

9898
/**
99-
* Execute the tool
99+
* Execute the tool logic
100100
*/
101-
public function execute(array $params): CallToolResult
101+
protected function doExecute(array $params): CallToolResult
102102
{
103-
// Initialize workspace context
104-
$this->initializeWorkspaceContext();
105103

106104
// Handle language parameter
107105
$languageId = 0;
108106
if (isset($params['language'])) {
109107
// Convert ISO code to language UID
110108
$languageId = $this->languageService->getUidFromIsoCode($params['language']);
111109
if ($languageId === null) {
112-
return new CallToolResult(
113-
[new TextContent('Unknown language code: ' . $params['language'])],
114-
true // isError
115-
);
110+
throw new \InvalidArgumentException('Unknown language code: ' . $params['language']);
116111
}
117112
} elseif (isset($params['languageId'])) {
118113
// Backward compatibility with numeric languageId
@@ -127,43 +122,31 @@ public function execute(array $params): CallToolResult
127122
try {
128123
$uid = $this->resolveUrlToPageUid($params['url'], $languageId);
129124
} catch (\Throwable $e) {
130-
return new CallToolResult(
131-
[new TextContent('Could not resolve URL to page: ' . $e->getMessage())],
132-
true // isError
133-
);
125+
// Re-throw as InvalidArgumentException to preserve the message in error handling
126+
throw new \InvalidArgumentException($e->getMessage(), 0, $e);
134127
}
135128
}
136129

137130
if ($uid <= 0) {
138-
return new CallToolResult(
139-
[new TextContent('Invalid page UID or URL. Please provide a valid page ID or URL.')],
140-
true // isError
141-
);
131+
throw new \InvalidArgumentException('Invalid page UID or URL. Please provide a valid page ID or URL.');
142132
}
143133

144-
try {
145-
// Get page data (with language overlay if applicable)
146-
$pageData = $this->getPageData($uid, $languageId);
147-
148-
// Get page URL using SiteInformationService
149-
$pageUrl = $this->siteInformationService->generatePageUrl((int)$pageData['uid'], $languageId);
150-
151-
// Get records on this page (filtered by language if specified)
152-
$recordsInfo = $this->getPageRecords($uid, $languageId);
153-
154-
// Get available translations for this page
155-
$translations = $this->getPageTranslations($uid);
156-
157-
// Build a text representation of the page information
158-
$result = $this->formatPageInfo($pageData, $recordsInfo, $pageUrl, $languageId, $translations);
159-
160-
return new CallToolResult([new TextContent($result)]);
161-
} catch (\Throwable $e) {
162-
return new CallToolResult(
163-
[new TextContent('Error retrieving page information: ' . $e->getMessage())],
164-
true // isError
165-
);
166-
}
134+
// Get page data (with language overlay if applicable)
135+
$pageData = $this->getPageData($uid, $languageId);
136+
137+
// Get page URL using SiteInformationService
138+
$pageUrl = $this->siteInformationService->generatePageUrl((int)$pageData['uid'], $languageId);
139+
140+
// Get records on this page (filtered by language if specified)
141+
$recordsInfo = $this->getPageRecords($uid, $languageId);
142+
143+
// Get available translations for this page
144+
$translations = $this->getPageTranslations($uid);
145+
146+
// Build a text representation of the page information
147+
$result = $this->formatPageInfo($pageData, $recordsInfo, $pageUrl, $languageId, $translations);
148+
149+
return new CallToolResult([new TextContent($result)]);
167150
}
168151

169152
/**

0 commit comments

Comments
 (0)