diff --git a/src/Tool/Tool.php b/src/Tool/Tool.php index 3c5ef27..29e7bb7 100644 --- a/src/Tool/Tool.php +++ b/src/Tool/Tool.php @@ -233,12 +233,14 @@ protected function validateArguments(array $arguments): void } foreach ($this->parameters as $name => $param) { - if ($param->required && !isset($arguments[$name])) { + if ($param->required && !array_key_exists($name, $arguments)) { throw new \InvalidArgumentException("Missing required argument: {$name}"); } // If parameter is present, validate its type. // If not required and not present, skip type validation. - if (isset($arguments[$name])) { + // For required parameters that are null (like a pAny = null), they should still be validated by type. + // So, we check if the key exists for type validation. + if (array_key_exists($name, $arguments)) { if (!$this->validateType($arguments[$name], $param->type)) { throw new \InvalidArgumentException( "Invalid type for argument {$name}: expected {$param->type}, got " . gettype($arguments[$name]) @@ -255,8 +257,34 @@ protected function validateArguments(array $arguments): void * @param string $type The expected type. * @return bool True if the value is of the expected type, false otherwise. */ - private function validateType($value, string $type): bool + private function validateType(mixed $value, string $type): bool { + // If the type is 'any', any value (including null) is acceptable. + if ($type === 'any') { + return true; + } + + // For other types, if the value is null, it's only valid if the type explicitly allows null + // (e.g. future support for nullable types like "string|null" or an attribute property). + // For now, basic types like 'string', 'integer' do not implicitly accept null + // unless we define them as such (e.g. by convention or a new 'nullable' property in ParameterAttribute). + // However, the current problem is about 'any' type and 'required' check. + // The validateType method for non-'any' types should typically return false for null + // if the type itself isn't nullable. + // This part of logic might need refinement if nullable types (e.g. ?string) are formally introduced. + + // If value is null and type is not 'any', it's an invalid type for basic scalar types by default. + // This strictness might be too much if a required parameter of type 'string' can be null. + // But for now, let's assume required non-'any' types cannot be null. + if ($value === null) { + // This depends on how schema/types define nullability. + // For now, if it's not 'any', and it's null, let's consider it not matching basic types like 'string', 'integer'. + // This might need adjustment based on broader type system design. + // For the specific test case (pAny: null), type 'any' handles it. + // If we had pString: null (required:true, type:string), this would make it fail here. + return false; // Example: 'string' does not accept null unless explicitly stated. + } + return match ($type) { 'string' => is_string($value), 'number' => is_numeric($value), @@ -264,9 +292,6 @@ private function validateType($value, string $type): bool 'boolean' => is_bool($value), 'array' => is_array($value), 'object' => is_object($value), - // For 'any' or other custom types, we might need more sophisticated validation or assume true. - // 'any' type essentially means no type validation beyond presence if required. - 'any' => true, // Explicitly allow 'any' type. default => true // Allow other unknown types by default, or could be stricter. }; } diff --git a/tests/Tool/Fixture/AllNullAnnotationsTool.php b/tests/Tool/Fixture/AllNullAnnotationsTool.php new file mode 100644 index 0000000..6fade55 --- /dev/null +++ b/tests/Tool/Fixture/AllNullAnnotationsTool.php @@ -0,0 +1,18 @@ +text("test"); + } +} diff --git a/tests/Tool/Fixture/DestructiveTool.php b/tests/Tool/Fixture/DestructiveTool.php new file mode 100644 index 0000000..395b33e --- /dev/null +++ b/tests/Tool/Fixture/DestructiveTool.php @@ -0,0 +1,18 @@ +text("test"); + } +} diff --git a/tests/Tool/Fixture/EmptyAnnotationsTool.php b/tests/Tool/Fixture/EmptyAnnotationsTool.php new file mode 100644 index 0000000..2aa3ebb --- /dev/null +++ b/tests/Tool/Fixture/EmptyAnnotationsTool.php @@ -0,0 +1,18 @@ +text("test"); + } +} diff --git a/tests/Tool/Fixture/IdempotentTool.php b/tests/Tool/Fixture/IdempotentTool.php new file mode 100644 index 0000000..2a08bd7 --- /dev/null +++ b/tests/Tool/Fixture/IdempotentTool.php @@ -0,0 +1,18 @@ +text("test"); + } +} diff --git a/tests/Tool/Fixture/LifecycleTestTool.php b/tests/Tool/Fixture/LifecycleTestTool.php new file mode 100644 index 0000000..e8974e4 --- /dev/null +++ b/tests/Tool/Fixture/LifecycleTestTool.php @@ -0,0 +1,54 @@ +log[] = "Before parent constructor"; + parent::__construct($config); + $this->log[] = "After parent constructor"; + } + + public function initialize(): void + { + parent::initialize(); // Good practice to call parent + $this->initialized = true; + $this->log[] = "initialize called"; + } + + public function shutdown(): void + { + parent::shutdown(); // Good practice to call parent + $this->shutdown = true; + $this->log[] = "shutdown called"; + } + + protected function doExecute(array $arguments): array|ContentItemInterface + { + return $this->text("executed"); + } + + // Helper to get the log for assertions + /** @return string[] */ + public function getLog(): array + { + return $this->log; + } + + public function clearLog(): void + { + $this->log = []; + } +} diff --git a/tests/Tool/Fixture/OpenWorldTool.php b/tests/Tool/Fixture/OpenWorldTool.php new file mode 100644 index 0000000..6474685 --- /dev/null +++ b/tests/Tool/Fixture/OpenWorldTool.php @@ -0,0 +1,18 @@ +text("test"); + } +} diff --git a/tests/Tool/Fixture/ReadOnlyTool.php b/tests/Tool/Fixture/ReadOnlyTool.php new file mode 100644 index 0000000..1b4b2df --- /dev/null +++ b/tests/Tool/Fixture/ReadOnlyTool.php @@ -0,0 +1,18 @@ +text("test"); + } +} diff --git a/tests/Tool/Fixture/ToolWithAnnotations.php b/tests/Tool/Fixture/ToolWithAnnotations.php new file mode 100644 index 0000000..344954f --- /dev/null +++ b/tests/Tool/Fixture/ToolWithAnnotations.php @@ -0,0 +1,18 @@ +text("test"); + } +} diff --git a/tests/Tool/Fixture/ValidationTestTool.php b/tests/Tool/Fixture/ValidationTestTool.php new file mode 100644 index 0000000..e4dd2c1 --- /dev/null +++ b/tests/Tool/Fixture/ValidationTestTool.php @@ -0,0 +1,25 @@ +text(implode(', ', $results)); + } +} diff --git a/tests/Tool/ToolAnnotationsTest.php b/tests/Tool/ToolAnnotationsTest.php new file mode 100644 index 0000000..0f39af7 --- /dev/null +++ b/tests/Tool/ToolAnnotationsTest.php @@ -0,0 +1,95 @@ +getAnnotations(); + $this->assertNotNull($annotations); + $this->assertArrayHasKey('title', $annotations); + $this->assertEquals('Test Tool Annotations', $annotations['title']); + } + + public function testInitializeMetadataWithDestructiveHint(): void + { + $tool = new DestructiveTool(); + $annotations = $tool->getAnnotations(); + $this->assertNotNull($annotations); + $this->assertArrayHasKey('destructiveHint', $annotations); + $this->assertTrue($annotations['destructiveHint']); + } + + public function testInitializeMetadataWithIdempotentHint(): void + { + $tool = new IdempotentTool(); + $annotations = $tool->getAnnotations(); + $this->assertNotNull($annotations); + $this->assertArrayHasKey('idempotentHint', $annotations); + $this->assertTrue($annotations['idempotentHint']); + } + + public function testInitializeMetadataWithOpenWorldHint(): void + { + $tool = new OpenWorldTool(); + $annotations = $tool->getAnnotations(); + $this->assertNotNull($annotations); + $this->assertArrayHasKey('openWorldHint', $annotations); + $this->assertTrue($annotations['openWorldHint']); + } + + public function testInitializeMetadataWithReadOnlyHint(): void + { + $tool = new ReadOnlyTool(); + $annotations = $tool->getAnnotations(); + $this->assertNotNull($annotations); + $this->assertArrayHasKey('readOnlyHint', $annotations); + $this->assertTrue($annotations['readOnlyHint']); + } + + public function testInitializeMetadataWithNoAnnotations(): void + { + // Use an anonymous class that extends Tool without any ToolAnnotations attribute + $tool = new class extends Tool { + protected function doExecute(array $arguments): array|ContentItemInterface + { + return $this->text("test"); + } + }; + $annotations = $tool->getAnnotations(); + $this->assertNull($annotations, "Annotations should be null when no ToolAnnotations attribute is present."); + } + + public function testInitializeMetadataWithEmptyToolAnnotations(): void + { + $tool = new EmptyAnnotationsTool(); + $annotations = $tool->getAnnotations(); + $this->assertNull($annotations, "Annotations should be null when ToolAnnotations attribute is empty."); + } + + public function testInitializeMetadataWithAllHintsNullInToolAnnotations(): void + { + $tool = new AllNullAnnotationsTool(); + $annotations = $tool->getAnnotations(); + $this->assertNull($annotations, "Annotations should be null when all hints in ToolAnnotations are null."); + } +} diff --git a/tests/Tool/ToolTest.php b/tests/Tool/ToolTest.php index a06bbf7..b5b1f3e 100644 --- a/tests/Tool/ToolTest.php +++ b/tests/Tool/ToolTest.php @@ -7,6 +7,8 @@ use MCP\Server\Tool\Attribute\Parameter as ParameterAttribute; use PHPUnit\Framework\TestCase; use MCP\Server\Tool\Content\ContentItemInterface; +use MCP\Server\Tests\Tool\Fixture\LifecycleTestTool; +use MCP\Server\Tests\Tool\Fixture\ValidationTestTool; // TestTool and CalculatorTool are now in separate files. @@ -384,4 +386,269 @@ protected function doExecute(array $arguments): array $this->expectExceptionMessage('All items returned by doExecute must be instances of ContentItemInterface.'); $tool->execute([]); } + + public function testInitializeMethodSetsFlagAndLogs(): void + { + $tool = new LifecycleTestTool(); + // Assert initial state (before explicit initialize call) + $this->assertFalse($tool->initialized, "Tool should not be initialized before initialize() is called."); + $initialLog = $tool->getLog(); + $this->assertNotContains("initialize called", $initialLog, "Log should not contain 'initialize called' before execution."); + $this->assertContains("Before parent constructor", $initialLog); + $this->assertContains("After parent constructor", $initialLog); + + // Call initialize + $tool->initialize(); + + $this->assertTrue($tool->initialized, "Tool initialize flag should be true after calling initialize()."); + $finalLog = $tool->getLog(); + $this->assertContains("initialize called", $finalLog, "Log should contain 'initialize called' after execution."); + + // Check full log order now + $expectedLog = [ + "Before parent constructor", + "After parent constructor", + "initialize called" + ]; + $this->assertEquals($expectedLog, $finalLog); + } + + public function testShutdownMethodSetsFlag(): void + { + $tool = new LifecycleTestTool(); + // Reset log to make assertions cleaner for shutdown only + $tool->clearLog(); + + $tool->shutdown(); + $this->assertTrue($tool->shutdown, "Tool shutdown flag should be true after calling shutdown()."); + $this->assertContains("shutdown called", $tool->getLog(), "Log should contain 'shutdown called'."); + $this->assertCount(1, $tool->getLog()); // Ensure only shutdown was logged now + $this->assertEquals("shutdown called", $tool->getLog()[0]); + } + + /** + * Helper method to set the private 'parameters' property on a Tool instance using reflection. + * + * @param Tool $tool The tool instance. + * @param array $paramsConfig + * An array where keys are parameter names and values are arrays defining the parameter. + * Example: ['paramName' => ['type' => 'string', 'required' => true]] + */ + private function setToolParameters(Tool $tool, array $paramsConfig): void + { + $parameters = []; + foreach ($paramsConfig as $name => $config) { + $parameters[$name] = new \MCP\Server\Tool\Attribute\Parameter( + name: $name, + type: $config['type'], + description: $config['description'] ?? null, + required: $config['required'] ?? true + ); + } + + // Get reflection of the parent class (Tool) to access its private properties + $reflection = new \ReflectionClass(Tool::class); + $parametersProperty = $reflection->getProperty('parameters'); + $parametersProperty->setAccessible(true); // Allow modification of private property + $parametersProperty->setValue($tool, $parameters); // Set value on the $tool instance + } + + // Tests for 'integer' type validation + public function testValidateArgumentsIntegerTypeCorrect(): void + { + $tool = new ValidationTestTool(); + $this->setToolParameters($tool, [ + 'pInt' => ['type' => 'integer'], + 'pObj' => ['type' => 'object'], + 'pAny' => ['type' => 'any'], + ]); + $result = $tool->execute(['pInt' => 123, 'pObj' => new \stdClass(), 'pAny' => 'hello']); + $this->assertStringContainsString("pInt type: integer", $result[0]['text']); + } + + public function testValidateArgumentsIntegerTypeIncorrect(): void + { + $tool = new ValidationTestTool(); + $this->setToolParameters($tool, [ + 'pInt' => ['type' => 'integer'], + 'pObj' => ['type' => 'object'], + 'pAny' => ['type' => 'any'], + ]); + $this->expectException(\InvalidArgumentException::class); + $this->expectExceptionMessage("Invalid type for argument pInt: expected integer, got string"); + $tool->execute(['pInt' => "not-an-integer", 'pObj' => new \stdClass(), 'pAny' => 'hello']); + } + + public function testValidateArgumentsIntegerTypeMissingRequired(): void + { + $tool = new ValidationTestTool(); + $this->setToolParameters($tool, [ + 'pInt' => ['type' => 'integer', 'required' => true], + 'pObj' => ['type' => 'object'], + 'pAny' => ['type' => 'any'], + ]); + $this->expectException(\InvalidArgumentException::class); + $this->expectExceptionMessage("Missing required argument: pInt"); + $tool->execute(['pObj' => new \stdClass(), 'pAny' => 'hello']); + } + + public function testValidateArgumentsOptionalIntegerTypeCorrect(): void + { + $tool = new ValidationTestTool(); + $this->setToolParameters($tool, [ + 'pInt' => ['type' => 'integer'], + 'pObj' => ['type' => 'object'], + 'pAny' => ['type' => 'any'], + 'pOptInt' => ['type' => 'integer', 'required' => false], + ]); + // pOptInt is provided + $result = $tool->execute(['pInt' => 1, 'pObj' => new \stdClass(), 'pAny' => 'a', 'pOptInt' => 456]); + $this->assertStringContainsString("pOptInt type: integer", $result[0]['text']); + + // pOptInt is omitted (which is valid) + $result = $tool->execute(['pInt' => 1, 'pObj' => new \stdClass(), 'pAny' => 'a']); + $this->assertStringNotContainsString("pOptInt type", $result[0]['text']); + } + + // Tests for 'object' type validation + public function testValidateArgumentsObjectTypeCorrect(): void + { + $tool = new ValidationTestTool(); + $this->setToolParameters($tool, [ + 'pInt' => ['type' => 'integer'], + 'pObj' => ['type' => 'object'], + 'pAny' => ['type' => 'any'], + ]); + $result = $tool->execute(['pInt' => 123, 'pObj' => new \stdClass(), 'pAny' => 'hello']); + $this->assertStringContainsString("pObj type: object", $result[0]['text']); + } + + public function testValidateArgumentsObjectTypeIncorrect(): void + { + $tool = new ValidationTestTool(); + $this->setToolParameters($tool, [ + 'pInt' => ['type' => 'integer'], + 'pObj' => ['type' => 'object'], + 'pAny' => ['type' => 'any'], + ]); + $this->expectException(\InvalidArgumentException::class); + $this->expectExceptionMessage("Invalid type for argument pObj: expected object, got string"); + $tool->execute(['pInt' => 123, 'pObj' => "not-an-object", 'pAny' => 'hello']); + } + + public function testValidateArgumentsObjectTypeMissingRequired(): void + { + $tool = new ValidationTestTool(); + $this->setToolParameters($tool, [ + 'pInt' => ['type' => 'integer'], + 'pObj' => ['type' => 'object', 'required' => true], + 'pAny' => ['type' => 'any'], + ]); + $this->expectException(\InvalidArgumentException::class); + $this->expectExceptionMessage("Missing required argument: pObj"); + $tool->execute(['pInt' => 123, 'pAny' => 'hello']); + } + + public function testValidateArgumentsOptionalObjectTypeCorrect(): void + { + $tool = new ValidationTestTool(); + $this->setToolParameters($tool, [ + 'pInt' => ['type' => 'integer'], + 'pObj' => ['type' => 'object'], + 'pAny' => ['type' => 'any'], + 'pOptObj' => ['type' => 'object', 'required' => false], + ]); + // pOptObj is provided + $result = $tool->execute(['pInt' => 1, 'pObj' => new \stdClass(), 'pAny' => 'a', 'pOptObj' => new \stdClass()]); + $this->assertStringContainsString("pOptObj type: object", $result[0]['text']); + + // pOptObj is omitted + $result = $tool->execute(['pInt' => 1, 'pObj' => new \stdClass(), 'pAny' => 'a']); + $this->assertStringNotContainsString("pOptObj type", $result[0]['text']); + } + + // Tests for 'any' type validation + public function testValidateArgumentsAnyTypeCorrect(): void + { + $tool = new ValidationTestTool(); + $this->setToolParameters($tool, [ + 'pInt' => ['type' => 'integer'], + 'pObj' => ['type' => 'object'], + 'pAny' => ['type' => 'any'], // 'required' defaults to true + ]); + + // Test null value explicitly for 'any' type + $resultNull = $tool->execute(['pInt' => 1, 'pObj' => new \stdClass(), 'pAny' => null]); + $this->assertStringContainsString("pAny type: NULL", $resultNull[0]['text'], "Failed for 'any' type with null value."); + + $testValues = [ + 'string_val' => "hello", + 'int_val' => 123, + 'bool_val' => true, + 'array_val' => ['a', 'b'], + 'object_val' => new \stdClass(), + 'double_val' => 1.23, + ]; + + foreach ($testValues as $key => $value) { + $result = $tool->execute(['pInt' => 1, 'pObj' => new \stdClass(), 'pAny' => $value]); + $this->assertStringContainsString("pAny type: " . gettype($value), $result[0]['text'], "Failed for 'any' type with key: " . $key . " value type: " . gettype($value)); + } + } + + public function testValidateArgumentsAnyTypeMissingRequired(): void + { + $tool = new ValidationTestTool(); + $this->setToolParameters($tool, [ + 'pInt' => ['type' => 'integer'], + 'pObj' => ['type' => 'object'], + 'pAny' => ['type' => 'any', 'required' => true], + ]); + $this->expectException(\InvalidArgumentException::class); + $this->expectExceptionMessage("Missing required argument: pAny"); + $tool->execute(['pInt' => 123, 'pObj' => new \stdClass()]); + } + + public function testValidateArgumentsOptionalAnyTypeCorrect(): void + { + $tool = new ValidationTestTool(); + $this->setToolParameters($tool, [ + 'pInt' => ['type' => 'integer'], + 'pObj' => ['type' => 'object'], + 'pAny' => ['type' => 'any'], + 'pOptAny' => ['type' => 'any', 'required' => false], + ]); + // pOptAny is provided with a string + $result = $tool->execute(['pInt' => 1, 'pObj' => new \stdClass(), 'pAny' => 'a', 'pOptAny' => 'optional string']); + $this->assertStringContainsString("pOptAny type: string", $result[0]['text']); + + // pOptAny is omitted + $result = $tool->execute(['pInt' => 1, 'pObj' => new \stdClass(), 'pAny' => 'a']); + $this->assertStringNotContainsString("pOptAny type", $result[0]['text']); + } + + public function testValidateTypeWithNullForNonAnyTypes(): void + { + $tool = new TestTool(); // Using TestTool, or any concrete Tool implementation + $reflectionMethod = new \ReflectionMethod(Tool::class, 'validateType'); + $reflectionMethod->setAccessible(true); + + $typesToTest = [ + 'string', + 'number', + 'integer', + 'boolean', + 'array', + 'object', + ]; + + foreach ($typesToTest as $type) { + $isValid = $reflectionMethod->invoke($tool, null, $type); + $this->assertFalse($isValid, "validateType(null, '{$type}') should return false."); + } + + // Also confirm 'any' type still accepts null + $isValidForAny = $reflectionMethod->invoke($tool, null, 'any'); + $this->assertTrue($isValidForAny, "validateType(null, 'any') should return true."); + } }