From ced762614b8319634535452c18797fcbd2ca7fb5 Mon Sep 17 00:00:00 2001 From: Christopher Hertel Date: Tue, 18 Aug 2026 14:54:52 +0200 Subject: [PATCH] [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 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 --- src/Capability/Discovery/SchemaGenerator.php | 110 ++++++++++++--- .../Discovery/SchemaGeneratorUnionTest.php | 130 ++++++++++++++++++ .../Capability/Discovery/UnionFixture.php | 27 ++++ 3 files changed, 247 insertions(+), 20 deletions(-) create mode 100644 tests/Unit/Capability/Discovery/SchemaGeneratorUnionTest.php create mode 100644 tests/Unit/Capability/Discovery/UnionFixture.php diff --git a/src/Capability/Discovery/SchemaGenerator.php b/src/Capability/Discovery/SchemaGenerator.php index 80779c7..3d9f8aa 100644 --- a/src/Capability/Discovery/SchemaGenerator.php +++ b/src/Capability/Discovery/SchemaGenerator.php @@ -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` 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 + */ + 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`) 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)]+\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'], diff --git a/tests/Unit/Capability/Discovery/SchemaGeneratorUnionTest.php b/tests/Unit/Capability/Discovery/SchemaGeneratorUnionTest.php new file mode 100644 index 0000000..9a8a6cf --- /dev/null +++ b/tests/Unit/Capability/Discovery/SchemaGeneratorUnionTest.php @@ -0,0 +1,130 @@ +}> + */ + 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|int', ['array', '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', ['array']]; + yield 'union inside a shape' => ['array{a: int|string}', ['array{a: int|string}']]; + yield 'nested generics' => ['array>', ['array>']]; + 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 $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}> + */ + 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|bool', ['array', 'boolean']]; + + yield 'a generic union is still one array' => ['array', ['array']]; + yield 'a shape is still one object' => ['array{a: int|string}', ['object']]; + } + + /** + * @param list $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)); + } +} diff --git a/tests/Unit/Capability/Discovery/UnionFixture.php b/tests/Unit/Capability/Discovery/UnionFixture.php new file mode 100644 index 0000000..b6d400d --- /dev/null +++ b/tests/Unit/Capability/Discovery/UnionFixture.php @@ -0,0 +1,27 @@ +