[Capability] Stop a union type losing a branch (#434)
* [Capability] Stop a union type losing a branch
mapPhpTypeToJsonSchemaType() tested for array syntax before splitting the
union, so string[]|int mapped to array alone; applyArrayConstraints() then
overwrote the type with array whenever the union contained one, dropping the
scalar branch a second time. A tool declaring such a parameter rejected a
valid integer argument.
The split now happens first and only at the top level, so array<int|string>
and array{a: int|string} stay one type rather than two that do not exist, and
the array branch keeps its items alongside the others - which is what 2020-12
means by items: a constraint that applies only when the instance is an array.
* Apply suggestion from @chr-hertel
* Isolate the array branch before inferring its element type
* Don't apply items to arrays whose union branches disagree on element type
This commit is contained in:
Christopher Hertel
GitHub
parent
0751a83658
commit
ced762614b
3 changed files with 247 additions and 20 deletions
|
|
@ -443,7 +443,23 @@ final class SchemaGenerator implements SchemaGeneratorInterface
|
|||
}
|
||||
// Handle regular arrays
|
||||
elseif (\in_array('array', $this->mapPhpTypeToJsonSchemaType($typeString))) {
|
||||
$itemsType = $this->inferArrayItemsType($typeString);
|
||||
// `inferArrayItemsType()` expects a single array type, not a union — for
|
||||
// `string[]|int` it would see the whole string, match nothing, and lose
|
||||
// the element-type constraint. Isolate the branch(es) that are arrays.
|
||||
$arrayBranches = array_values(array_filter(
|
||||
self::splitUnion($typeString),
|
||||
fn (string $branch): bool => \in_array('array', $this->mapPhpTypeToJsonSchemaType($branch), true),
|
||||
));
|
||||
if ([] === $arrayBranches) {
|
||||
$arrayBranches = [$typeString];
|
||||
}
|
||||
|
||||
// Several array branches (`string[]|int[]`) can disagree on their
|
||||
// element type. Applying just one would reject values valid under
|
||||
// the other, so `items` only applies when every array branch agrees.
|
||||
$itemsTypes = array_map($this->inferArrayItemsType(...), $arrayBranches);
|
||||
$itemsType = 1 === \count(array_unique(array_map(json_encode(...), $itemsTypes))) ? $itemsTypes[0] : 'any';
|
||||
|
||||
if ('any' !== $itemsType) {
|
||||
if (\is_string($itemsType)) {
|
||||
$paramSchema['items'] = ['type' => $itemsType];
|
||||
|
|
@ -455,11 +471,25 @@ final class SchemaGenerator implements SchemaGeneratorInterface
|
|||
}
|
||||
}
|
||||
|
||||
if ($allowsNull) {
|
||||
$paramSchema['type'] = ['array', 'null'];
|
||||
// Only when the parameter is an array and nothing else. A union
|
||||
// like `string[]|int` keeps both branches: `items` constrains the
|
||||
// instance only when it *is* an array, so the two coexist and
|
||||
// overwriting the type here would drop the scalar branch —
|
||||
// rejecting a value the handler accepts.
|
||||
$otherTypes = array_values(array_diff(
|
||||
(array) $paramSchema['type'],
|
||||
['array', 'null'],
|
||||
));
|
||||
|
||||
if ([] === $otherTypes) {
|
||||
$paramSchema['type'] = $allowsNull ? ['array', 'null'] : 'array';
|
||||
|
||||
if (\is_array($paramSchema['type'])) {
|
||||
sort($paramSchema['type']);
|
||||
}
|
||||
} elseif ($allowsNull && !\in_array('null', (array) $paramSchema['type'], true)) {
|
||||
$paramSchema['type'] = [...(array) $paramSchema['type'], 'null'];
|
||||
sort($paramSchema['type']);
|
||||
} else {
|
||||
$paramSchema['type'] = 'array';
|
||||
}
|
||||
}
|
||||
|
||||
|
|
@ -655,6 +685,42 @@ final class SchemaGenerator implements SchemaGeneratorInterface
|
|||
return $typeString ?: 'mixed';
|
||||
}
|
||||
|
||||
/**
|
||||
* Splits a type string on its top-level `|`, leaving generics and shapes
|
||||
* intact.
|
||||
*
|
||||
* `int|string` splits; `array<int|string>` and `array{a: int|string}` do
|
||||
* not, because the `|` there is a parameter of the outer type rather than
|
||||
* an alternative to it.
|
||||
*
|
||||
* @return list<string>
|
||||
*/
|
||||
public static function splitUnion(string $type): array
|
||||
{
|
||||
$branches = [];
|
||||
$depth = 0;
|
||||
$current = '';
|
||||
|
||||
foreach (str_split($type) as $character) {
|
||||
if (\in_array($character, ['<', '{', '('], true)) {
|
||||
++$depth;
|
||||
} elseif (\in_array($character, ['>', '}', ')'], true)) {
|
||||
$depth = max(0, $depth - 1);
|
||||
} elseif ('|' === $character && 0 === $depth) {
|
||||
$branches[] = trim($current);
|
||||
$current = '';
|
||||
|
||||
continue;
|
||||
}
|
||||
|
||||
$current .= $character;
|
||||
}
|
||||
|
||||
$branches[] = trim($current);
|
||||
|
||||
return array_values(array_filter($branches, static fn (string $branch): bool => '' !== $branch));
|
||||
}
|
||||
|
||||
/**
|
||||
* Maps a PHP type string (potentially a union) to an array of JSON Schema type names.
|
||||
*
|
||||
|
|
@ -664,12 +730,28 @@ final class SchemaGenerator implements SchemaGeneratorInterface
|
|||
{
|
||||
$normalizedType = strtolower(trim($phpTypeString));
|
||||
|
||||
// PRIORITY 1: Check for array{} syntax which should be treated as object
|
||||
// PRIORITY 1: Handle unions before anything else. A `|` at the top
|
||||
// level joins alternatives; one inside `<>` or `{}` belongs to a
|
||||
// generic (`array<int|string>`) and is not a union of the whole type.
|
||||
// Checked first because a branch may itself be an array shape — and
|
||||
// `string[]|int` read as "an array" silently loses the `int`.
|
||||
$branches = self::splitUnion($normalizedType);
|
||||
|
||||
if (\count($branches) > 1) {
|
||||
$jsonTypes = [];
|
||||
foreach ($branches as $branch) {
|
||||
$jsonTypes = array_merge($jsonTypes, $this->mapPhpTypeToJsonSchemaType($branch));
|
||||
}
|
||||
|
||||
return array_values(array_unique($jsonTypes));
|
||||
}
|
||||
|
||||
// PRIORITY 2: Check for array{} syntax which should be treated as object
|
||||
if (preg_match('/^array\s*{/i', $normalizedType)) {
|
||||
return ['object'];
|
||||
}
|
||||
|
||||
// PRIORITY 2: Check for array syntax first (T[] or generics)
|
||||
// PRIORITY 3: Check for array syntax (T[] or generics)
|
||||
if (
|
||||
str_contains($normalizedType, '[]')
|
||||
|| preg_match('/^(array|list|iterable|collection)</i', $normalizedType)
|
||||
|
|
@ -677,23 +759,11 @@ final class SchemaGenerator implements SchemaGeneratorInterface
|
|||
return ['array'];
|
||||
}
|
||||
|
||||
// PRIORITY 3: Treat PHPStan/Psalm integer ranges as their base type
|
||||
// PRIORITY 4: Treat PHPStan/Psalm integer ranges as their base type
|
||||
if (preg_match('/^int\s*<\s*[^<>]+\s*>$/i', $normalizedType)) {
|
||||
return ['integer'];
|
||||
}
|
||||
|
||||
// PRIORITY 4: Handle unions (recursive)
|
||||
if (str_contains($normalizedType, '|')) {
|
||||
$types = explode('|', $normalizedType);
|
||||
$jsonTypes = [];
|
||||
foreach ($types as $type) {
|
||||
$mapped = $this->mapPhpTypeToJsonSchemaType(trim($type));
|
||||
$jsonTypes = array_merge($jsonTypes, $mapped);
|
||||
}
|
||||
|
||||
return array_values(array_unique($jsonTypes));
|
||||
}
|
||||
|
||||
// PRIORITY 5: Handle simple built-in types
|
||||
return match ($normalizedType) {
|
||||
'string', 'scalar' => ['string'],
|
||||
|
|
|
|||
130
tests/Unit/Capability/Discovery/SchemaGeneratorUnionTest.php
Normal file
130
tests/Unit/Capability/Discovery/SchemaGeneratorUnionTest.php
Normal file
|
|
@ -0,0 +1,130 @@
|
|||
<?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\Capability\Discovery;
|
||||
|
||||
use Mcp\Capability\Discovery\DocBlockParser;
|
||||
use Mcp\Capability\Discovery\SchemaGenerator;
|
||||
use Mcp\Capability\Discovery\SchemaValidator;
|
||||
use PHPUnit\Framework\Attributes\DataProvider;
|
||||
use PHPUnit\Framework\Attributes\TestDox;
|
||||
use PHPUnit\Framework\TestCase;
|
||||
|
||||
class SchemaGeneratorUnionTest extends TestCase
|
||||
{
|
||||
/**
|
||||
* @return iterable<string, array{string, list<string>}>
|
||||
*/
|
||||
public static function unions(): iterable
|
||||
{
|
||||
yield 'not a union' => ['string', ['string']];
|
||||
yield 'simple union' => ['int|string', ['int', 'string']];
|
||||
yield 'three branches' => ['int|string|bool', ['int', 'string', 'bool']];
|
||||
yield 'array branch alongside a scalar' => ['string[]|int', ['string[]', 'int']];
|
||||
yield 'generic branch alongside a scalar' => ['array<string>|int', ['array<string>', 'int']];
|
||||
|
||||
// The `|` here parameterises the outer type; splitting it would turn
|
||||
// one type into two that do not exist.
|
||||
yield 'union inside a generic' => ['array<int|string>', ['array<int|string>']];
|
||||
yield 'union inside a shape' => ['array{a: int|string}', ['array{a: int|string}']];
|
||||
yield 'nested generics' => ['array<string, array<int|bool>>', ['array<string, array<int|bool>>']];
|
||||
yield 'shape beside a scalar' => ['array{a: int|string}|null', ['array{a: int|string}', 'null']];
|
||||
|
||||
yield 'whitespace is trimmed' => ['int | string', ['int', 'string']];
|
||||
yield 'empty branches are dropped' => ['int||string', ['int', 'string']];
|
||||
}
|
||||
|
||||
/**
|
||||
* @param list<string> $expected
|
||||
*/
|
||||
#[DataProvider('unions')]
|
||||
#[TestDox('a type string splits on its top-level union only')]
|
||||
public function testSplitUnion(string $type, array $expected): void
|
||||
{
|
||||
$this->assertSame($expected, SchemaGenerator::splitUnion($type));
|
||||
}
|
||||
|
||||
/**
|
||||
* @return iterable<string, array{string, list<string>}>
|
||||
*/
|
||||
public static function mappedTypes(): iterable
|
||||
{
|
||||
yield 'scalar union' => ['int|string', ['integer', 'string']];
|
||||
|
||||
// The regression: the `[]` check ran before the union split, so this
|
||||
// came back as ['array'] and the `int` branch vanished.
|
||||
yield 'array beside a scalar keeps both' => ['string[]|int', ['array', 'integer']];
|
||||
yield 'scalar before an array keeps both' => ['int|string[]', ['integer', 'array']];
|
||||
yield 'generic beside a scalar keeps both' => ['array<string>|bool', ['array', 'boolean']];
|
||||
|
||||
yield 'a generic union is still one array' => ['array<int|string>', ['array']];
|
||||
yield 'a shape is still one object' => ['array{a: int|string}', ['object']];
|
||||
}
|
||||
|
||||
/**
|
||||
* @param list<string> $expected
|
||||
*/
|
||||
#[DataProvider('mappedTypes')]
|
||||
#[TestDox('a union maps to every JSON Schema type its branches need')]
|
||||
public function testMappedTypes(string $type, array $expected): void
|
||||
{
|
||||
$generator = new SchemaGenerator(new DocBlockParser());
|
||||
|
||||
$map = new \ReflectionMethod($generator, 'mapPhpTypeToJsonSchemaType');
|
||||
|
||||
$this->assertSame($expected, $map->invoke($generator, $type));
|
||||
}
|
||||
|
||||
#[TestDox('a generated schema keeps both branches of an array-or-scalar union')]
|
||||
public function testGeneratedUnionSchemaKeepsBothBranches(): void
|
||||
{
|
||||
$schema = (new SchemaGenerator(new DocBlockParser()))
|
||||
->generate(new \ReflectionMethod(UnionFixture::class, 'handle'));
|
||||
|
||||
// `items` constrains the instance only when it *is* an array, so it
|
||||
// coexists with the scalar branch instead of excluding it.
|
||||
$this->assertSame(['array', 'integer'], $schema['properties']['mixedish']['type']);
|
||||
// The array branch keeps its own element-type constraint rather than
|
||||
// losing it to the union as a whole (`items` would otherwise be `{}`).
|
||||
$this->assertSame(['type' => 'string'], $schema['properties']['mixedish']['items']);
|
||||
|
||||
$this->assertSame(['integer', 'string'], $schema['properties']['scalars']['type']);
|
||||
$this->assertSame('array', $schema['properties']['arrayOnly']['type']);
|
||||
$this->assertSame(['array', 'null'], $schema['properties']['nullableArray']['type']);
|
||||
|
||||
// Two array branches with different element types can't both be
|
||||
// enforced without rejecting a value that is valid under one of
|
||||
// them, so neither constraint is applied (falls back to the "matches
|
||||
// anything" empty schema `ensureArrayItems()` defaults every array to).
|
||||
$this->assertSame('array', $schema['properties']['disagreeingArrays']['type']);
|
||||
$this->assertEquals(new \stdClass(), $schema['properties']['disagreeingArrays']['items']);
|
||||
}
|
||||
|
||||
#[TestDox('a value from either branch validates against the generated schema')]
|
||||
public function testBothBranchesValidate(): void
|
||||
{
|
||||
$schema = (new SchemaGenerator(new DocBlockParser()))
|
||||
->generate(new \ReflectionMethod(UnionFixture::class, 'handle'));
|
||||
|
||||
$validator = new SchemaValidator();
|
||||
$base = ['scalars' => 1, 'arrayOnly' => ['a'], 'nullableArray' => null, 'disagreeingArrays' => []];
|
||||
|
||||
$this->assertSame([], $validator->validateAgainstJsonSchema([...$base, 'mixedish' => ['a', 'b']], $schema));
|
||||
$this->assertSame([], $validator->validateAgainstJsonSchema([...$base, 'mixedish' => 42], $schema));
|
||||
|
||||
// A float is in neither branch, so it is still refused.
|
||||
$this->assertNotSame([], $validator->validateAgainstJsonSchema([...$base, 'mixedish' => 1.5], $schema));
|
||||
|
||||
// Neither element type is enforced, so both branches' values pass.
|
||||
$this->assertSame([], $validator->validateAgainstJsonSchema([...$base, 'mixedish' => 42, 'disagreeingArrays' => ['a', 'b']], $schema));
|
||||
$this->assertSame([], $validator->validateAgainstJsonSchema([...$base, 'mixedish' => 42, 'disagreeingArrays' => [1, 2]], $schema));
|
||||
}
|
||||
}
|
||||
27
tests/Unit/Capability/Discovery/UnionFixture.php
Normal file
27
tests/Unit/Capability/Discovery/UnionFixture.php
Normal file
|
|
@ -0,0 +1,27 @@
|
|||
<?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\Capability\Discovery;
|
||||
|
||||
class UnionFixture
|
||||
{
|
||||
/**
|
||||
* @param string[]|int $mixedish an array branch beside a scalar one
|
||||
* @param int|string $scalars two scalar branches
|
||||
* @param string[] $arrayOnly one branch, and it is an array
|
||||
* @param string[]|null $nullableArray an array branch that may be null
|
||||
* @param string[]|int[] $disagreeingArrays two array branches with different element types
|
||||
*/
|
||||
public function handle(array|int $mixedish, int|string $scalars, array $arrayOnly, ?array $nullableArray, array $disagreeingArrays): string
|
||||
{
|
||||
return '';
|
||||
}
|
||||
}
|
||||
Loading…
Reference in a new issue