From b64410664b54469daf481c239123fd4a7cf090e7 Mon Sep 17 00:00:00 2001 From: Woody Gilk Date: Tue, 11 Aug 2026 16:18:48 -0500 Subject: [PATCH] fix: Look through composition when pre-checking null (#66) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Before validating a property or an array item the validators run a `SchemaValueNormalizer::normalize($value, $allowNull)` pre-check, and computed `$allowNull` from the immediate schema node alone. A composition node carries no `type` and no `nullable` — both live in its branches — so `$allowNull` was `false` and a permitted `null` was rejected by `InvalidDataTypeException` before a single branch ran. The impact is largest for JSON:API-style documents, where resources are modelled as `allOf` compositions and every nullable attribute became unrepresentable. Composition and `$ref` nodes now defer the decision to the branch or the resolved target, which already evaluates `nullable` correctly per branch. Nulls that no branch permits are still rejected, and now carry the failing branch's data path instead of a pathless error. The issue counted four copies of the rule; there were nine, and they disagreed on more than `$ref`. All nine now delegate to a single `SchemaValueNormalizer::allowsNull()` helper. That fixed six further reachable sites the report did not list: `properties` and `items` behind a branch, `prefixItems`, `dependentSchemas`, `if`/`then`, `oneOf` branches, and `not`. The `not` site was an inversion bug — `{allOf: [nullable], not: {allOf: [nullable]}}` accepted `null` because the pre-check's rejection was caught and read as "inner did not match". `OneOfValidatorWithContext::hasNullableSchema()` is deliberately left alone: it answers a different question (may a discriminated `oneOf` short-circuit on null?), which is a definitive accept, not a deferral. The report's `P_allOf_ref` case is not covered here. It trips the `$ref` sibling merge of #64, which discards the target's `nullable` before this pre-check runs; that fix is on its own branch. The `$ref` case tested here reaches nullability through a composition instead, so the suite proves this fix without depending on #64. --- CHANGELOG.md | 22 +++ .../Schema/ItemsValidatorWithContext.php | 4 +- .../Schema/OneOfValidatorWithContext.php | 3 +- .../Schema/PropertiesValidatorWithContext.php | 4 +- .../Schema/SchemaValueNormalizer.php | 22 +++ .../AbstractCompositionalValidator.php | 4 +- .../DependentSchemasValidator.php | 3 +- .../SchemaValidator/IfThenElseValidator.php | 3 +- .../SchemaValidator/ItemsValidator.php | 3 +- .../SchemaValidator/NotValidator.php | 3 +- .../SchemaValidator/PrefixItemsValidator.php | 3 +- .../SchemaValidator/PropertiesValidator.php | 3 +- .../NullableInCompositionRegressionTest.php | 136 ++++++++++++++++++ .../Validator/Schema/SchemaHelperTest.php | 36 +++++ 14 files changed, 226 insertions(+), 23 deletions(-) create mode 100644 tests/Unit/Regression/NullableInCompositionRegressionTest.php diff --git a/CHANGELOG.md b/CHANGELOG.md index b546e73..a5b91d0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,28 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [Unreleased] + +### Fixed + +- A `null` permitted by a branch of an `allOf`/`anyOf`/`oneOf` is no longer + rejected before any branch runs. The null pre-check that guards property + and item validation computed `$allowNull` from the immediate schema node, + but a composition node carries no `type` and no `nullable` — both live in + its branches — so `InvalidDataTypeException` fired before a single branch + was evaluated. This made every nullable attribute unrepresentable in + JSON:API-style documents, where resources are modelled as `allOf` + compositions. Composition and `$ref` nodes now defer the decision to the + branch or resolved target, which already evaluates `nullable` correctly. + Nulls no branch permits are still rejected, and now carry the failing + branch's data path instead of a pathless error (#66). +- The same rule was implemented nine times and the copies disagreed — only + two consulted `$ref`, so a `$ref` property that accepted `null` in one code + path was rejected in another. All nine now delegate to a single + `SchemaValueNormalizer::allowsNull()` helper, closing the divergence across + `properties`, `items`, `prefixItems`, `dependentSchemas`, `if`/`then`/ + `else`, `not`, and composition branches (#66). + ## [0.7.0] Preparation for the 1.0.0 stable release. This section tracks work that diff --git a/src/Validator/Schema/ItemsValidatorWithContext.php b/src/Validator/Schema/ItemsValidatorWithContext.php index a6349dc..83413a9 100644 --- a/src/Validator/Schema/ItemsValidatorWithContext.php +++ b/src/Validator/Schema/ItemsValidatorWithContext.php @@ -56,9 +56,7 @@ private function validate(array $data, Schema $schema, ValidationContext $contex $errors = []; $itemSchema = $schema->items; $prefixCount = null !== $schema->prefixItems ? count($schema->prefixItems) : 0; - $allowNull = $context->nullableAsType && ($itemSchema->nullable - || SchemaValueNormalizer::doesTypeIncludeNull($itemSchema->type) - || null !== $itemSchema->ref); + $allowNull = SchemaValueNormalizer::allowsNull($itemSchema, $context->nullableAsType); $rootValidator = $this->dependencies->rootSchemaValidator($this->document, $this->configuration); foreach ($data as $index => $arrayItem) { diff --git a/src/Validator/Schema/OneOfValidatorWithContext.php b/src/Validator/Schema/OneOfValidatorWithContext.php index 78263ff..b644230 100644 --- a/src/Validator/Schema/OneOfValidatorWithContext.php +++ b/src/Validator/Schema/OneOfValidatorWithContext.php @@ -105,8 +105,7 @@ private function validateWithoutDiscriminator(mixed $data, array $oneOf, Validat $childContext = $context->forkForBranch(); try { - $allowNull = $context->nullableAsType && ($subSchema->nullable - || SchemaValueNormalizer::doesTypeIncludeNull($subSchema->type)); + $allowNull = SchemaValueNormalizer::allowsNull($subSchema, $context->nullableAsType); $normalizedData = SchemaValueNormalizer::normalize($data, $allowNull); $rootValidator->validateWithContext($normalizedData, $subSchema, $childContext); ++$validCount; diff --git a/src/Validator/Schema/PropertiesValidatorWithContext.php b/src/Validator/Schema/PropertiesValidatorWithContext.php index 91e05cf..66d810f 100644 --- a/src/Validator/Schema/PropertiesValidatorWithContext.php +++ b/src/Validator/Schema/PropertiesValidatorWithContext.php @@ -53,9 +53,7 @@ private function validate(array $data, Schema $schema, ValidationContext $contex } try { - $allowNull = $context->nullableAsType && ($propertySchema->nullable - || SchemaValueNormalizer::doesTypeIncludeNull($propertySchema->type) - || null !== $propertySchema->ref); + $allowNull = SchemaValueNormalizer::allowsNull($propertySchema, $context->nullableAsType); $value = SchemaValueNormalizer::normalize($data[$name], $allowNull); $context->enterBreadcrumb($name); diff --git a/src/Validator/Schema/SchemaValueNormalizer.php b/src/Validator/Schema/SchemaValueNormalizer.php index 29149f2..dd8826f 100644 --- a/src/Validator/Schema/SchemaValueNormalizer.php +++ b/src/Validator/Schema/SchemaValueNormalizer.php @@ -4,6 +4,7 @@ namespace Duyler\OpenApi\Validator\Schema; +use Duyler\OpenApi\Schema\Model\Schema; use Duyler\OpenApi\Validator\Exception\InvalidDataTypeException; use stdClass; @@ -50,6 +51,27 @@ public static function normalize(mixed $value, bool $allowNull = false): array|i )); } + /** + * Decides whether the pre-check may hand a null to $schema. + * + * A composition or $ref node carries neither type nor nullable of its + * own — both live in the branches or in the resolved target — so null + * is deferred to them instead of being rejected here. + */ + public static function allowsNull(Schema $schema, bool $nullableAsType = true): bool + { + if (false === $nullableAsType) { + return false; + } + + return $schema->nullable + || self::doesTypeIncludeNull($schema->type) + || null !== $schema->ref + || null !== $schema->allOf + || null !== $schema->anyOf + || null !== $schema->oneOf; + } + /** * @param string|array|null $type */ diff --git a/src/Validator/SchemaValidator/AbstractCompositionalValidator.php b/src/Validator/SchemaValidator/AbstractCompositionalValidator.php index 23fce04..a1ff7fb 100644 --- a/src/Validator/SchemaValidator/AbstractCompositionalValidator.php +++ b/src/Validator/SchemaValidator/AbstractCompositionalValidator.php @@ -111,9 +111,7 @@ private function validateBranch(mixed $data, Schema $subSchema, ?ValidationConte */ private function normalizeForBranch(mixed $data, Schema $subSchema, ?ValidationContext $context): array|int|string|float|bool|null { - $nullableAsType = $context?->nullableAsType ?? true; - $allowNull = $nullableAsType && ($subSchema->nullable - || SchemaValueNormalizer::doesTypeIncludeNull($subSchema->type)); + $allowNull = SchemaValueNormalizer::allowsNull($subSchema, $context?->nullableAsType ?? true); return SchemaValueNormalizer::normalize($data, $allowNull); } diff --git a/src/Validator/SchemaValidator/DependentSchemasValidator.php b/src/Validator/SchemaValidator/DependentSchemasValidator.php index ee19dcd..e40fce6 100644 --- a/src/Validator/SchemaValidator/DependentSchemasValidator.php +++ b/src/Validator/SchemaValidator/DependentSchemasValidator.php @@ -57,8 +57,7 @@ private function validateDependent(array $data, string $propertyName, Schema $de $validator = $this->createSchemaValidator(); try { - $allowNull = $nullableAsType && ($dependentSchema->nullable - || SchemaValueNormalizer::doesTypeIncludeNull($dependentSchema->type)); + $allowNull = SchemaValueNormalizer::allowsNull($dependentSchema, $nullableAsType); $normalizedData = SchemaValueNormalizer::normalize($data, $allowNull); $validator->validate($normalizedData, $dependentSchema, $context); } catch (InvalidDataTypeException $e) { diff --git a/src/Validator/SchemaValidator/IfThenElseValidator.php b/src/Validator/SchemaValidator/IfThenElseValidator.php index 61b447c..cb744bb 100644 --- a/src/Validator/SchemaValidator/IfThenElseValidator.php +++ b/src/Validator/SchemaValidator/IfThenElseValidator.php @@ -157,8 +157,7 @@ private function validateThenOrElse( */ private function normalizeFor(mixed $data, Schema $subSchema, bool $nullableAsType): array|int|string|float|bool|null { - $allowNull = $nullableAsType && ($subSchema->nullable - || SchemaValueNormalizer::doesTypeIncludeNull($subSchema->type)); + $allowNull = SchemaValueNormalizer::allowsNull($subSchema, $nullableAsType); return SchemaValueNormalizer::normalize($data, $allowNull); } diff --git a/src/Validator/SchemaValidator/ItemsValidator.php b/src/Validator/SchemaValidator/ItemsValidator.php index f5137c1..a7736a8 100644 --- a/src/Validator/SchemaValidator/ItemsValidator.php +++ b/src/Validator/SchemaValidator/ItemsValidator.php @@ -63,8 +63,7 @@ private function validateSchemaItems(array $data, Schema $itemsSchema, ?array $p { $prefixCount = null !== $prefixItems ? count($prefixItems) : 0; $nullableAsType = $context?->nullableAsType ?? true; - $allowNull = $nullableAsType && ($itemsSchema->nullable - || SchemaValueNormalizer::doesTypeIncludeNull($itemsSchema->type)); + $allowNull = SchemaValueNormalizer::allowsNull($itemsSchema, $nullableAsType); $state = new ItemValidationState( itemsSchema: $itemsSchema, diff --git a/src/Validator/SchemaValidator/NotValidator.php b/src/Validator/SchemaValidator/NotValidator.php index 212ba48..357dd04 100644 --- a/src/Validator/SchemaValidator/NotValidator.php +++ b/src/Validator/SchemaValidator/NotValidator.php @@ -50,8 +50,7 @@ private function matchesNotSchema(mixed $data, Schema $notSchema, ?ValidationCon $childContext = null !== $context ? $context->forkForBranch() : null; try { - $allowNull = $nullableAsType && ($notSchema->nullable - || SchemaValueNormalizer::doesTypeIncludeNull($notSchema->type)); + $allowNull = SchemaValueNormalizer::allowsNull($notSchema, $nullableAsType); $normalizedData = SchemaValueNormalizer::normalize($data, $allowNull); $validator->validate($normalizedData, $notSchema, $childContext); } catch (InvalidDataTypeException|ValidationException|AbstractValidationError) { diff --git a/src/Validator/SchemaValidator/PrefixItemsValidator.php b/src/Validator/SchemaValidator/PrefixItemsValidator.php index 0aec204..c2034bd 100644 --- a/src/Validator/SchemaValidator/PrefixItemsValidator.php +++ b/src/Validator/SchemaValidator/PrefixItemsValidator.php @@ -63,8 +63,7 @@ private function validatePrefixItemAt(mixed $item, int $index, ItemValidationSta } try { - $allowNull = $state->nullableAsType && ($subSchema->nullable - || SchemaValueNormalizer::doesTypeIncludeNull($subSchema->type)); + $allowNull = SchemaValueNormalizer::allowsNull($subSchema, $state->nullableAsType); $value = SchemaValueNormalizer::normalize($item, $allowNull); if (null === $state->context) { diff --git a/src/Validator/SchemaValidator/PropertiesValidator.php b/src/Validator/SchemaValidator/PropertiesValidator.php index a1f7432..b9f84ef 100644 --- a/src/Validator/SchemaValidator/PropertiesValidator.php +++ b/src/Validator/SchemaValidator/PropertiesValidator.php @@ -51,8 +51,7 @@ public function validate(mixed $data, Schema $schema, ?ValidationContext $contex private function validateProperty(mixed $value, string $name, Schema $propertySchema, SchemaValidatorInterface $validator, bool $nullableAsType, ?ValidationContext &$context): void { try { - $allowNull = $nullableAsType && ($propertySchema->nullable - || SchemaValueNormalizer::doesTypeIncludeNull($propertySchema->type)); + $allowNull = SchemaValueNormalizer::allowsNull($propertySchema, $nullableAsType); $normalized = SchemaValueNormalizer::normalize($value, $allowNull); if (null === $context) { diff --git a/tests/Unit/Regression/NullableInCompositionRegressionTest.php b/tests/Unit/Regression/NullableInCompositionRegressionTest.php new file mode 100644 index 0000000..f1640b5 --- /dev/null +++ b/tests/Unit/Regression/NullableInCompositionRegressionTest.php @@ -0,0 +1,136 @@ +}]` is deliberately absent: it also + * trips the `$ref` sibling merge of issue #64, which discards the target's + * `nullable` before this pre-check ever runs. The `$ref` branch covered here + * reaches its nullability through a composition instead, so it exercises the + * deferral without depending on that separate fix. + * + * @internal + */ +final class NullableInCompositionRegressionTest extends TestCase +{ + private const string SPEC_YAML = <<<'YAML' +openapi: 3.0.0 +info: { title: Nullable Composition API, version: 1.0.0 } +paths: {} +components: + schemas: + NullableStringComposition: + allOf: [{type: string, nullable: true}] + + P_inline: {type: object, properties: {p: {type: string, nullable: true}}} + P_allOf_inline: {type: object, properties: {p: {allOf: [{type: string, nullable: true}]}}} + P_anyOf_inline: {type: object, properties: {p: {anyOf: [{type: string, nullable: true}]}}} + P_oneOf_inline: {type: object, properties: {p: {oneOf: [{type: string, nullable: true}]}}} + P_allOf_ref: {type: object, properties: {p: {allOf: [{$ref: '#/components/schemas/NullableStringComposition'}]}}} + P_allOf_nullable_sibling: + {type: object, properties: {p: {nullable: true, allOf: [{type: string, nullable: true}]}}} + + P_allOf_non_nullable: {type: object, properties: {p: {allOf: [{type: string}]}}} + + A_inline: {type: array, items: {type: string, nullable: true}} + A_allOf: {type: array, items: {allOf: [{type: string, nullable: true}]}} + A_anyOf: {type: array, items: {anyOf: [{type: string, nullable: true}]}} + A_oneOf: {type: array, items: {oneOf: [{type: string, nullable: true}]}} + + A_allOf_non_nullable: {type: array, items: {allOf: [{type: string}]}} + + # The same rule lives behind six further keywords; each reaches the + # pre-check through a different validator. + Nested_properties: {allOf: [{type: object, properties: {p: {allOf: [{type: string, nullable: true}]}}}]} + Nested_items: {allOf: [{type: array, items: {allOf: [{type: string, nullable: true}]}}]} + Dependent_schemas: + type: object + properties: {a: {type: string}} + dependentSchemas: + a: {type: object, properties: {p: {allOf: [{type: string, nullable: true}]}}} + If_then: + if: {type: object} + then: {type: object, properties: {p: {allOf: [{type: string, nullable: true}]}}} + Prefix_items: {type: array, prefixItems: [{allOf: [{type: string, nullable: true}]}]} + Discriminated_oneOf: {type: object, properties: {p: {oneOf: [{allOf: [{type: string, nullable: true}]}]}}} + + # `not` is reached only once the node beside it defers, so the outer + # allOf is what carries null this far. The inner composition matches + # null, which means `not` must reject it. + Not_matching_composition: + allOf: [{type: string, nullable: true}] + not: {allOf: [{type: string, nullable: true}]} +YAML; + + /** + * @return iterable}> + */ + public static function nullAcceptingSchemaProvider(): iterable + { + yield 'inline nullable property' => ['P_inline', ['p' => null]]; + yield 'allOf-wrapped nullable property' => ['P_allOf_inline', ['p' => null]]; + yield 'anyOf-wrapped nullable property' => ['P_anyOf_inline', ['p' => null]]; + yield 'oneOf-wrapped nullable property' => ['P_oneOf_inline', ['p' => null]]; + yield 'allOf-wrapped $ref to a nullable composition' => ['P_allOf_ref', ['p' => null]]; + yield 'allOf with nullable sibling' => ['P_allOf_nullable_sibling', ['p' => null]]; + yield 'inline nullable item' => ['A_inline', [null]]; + yield 'allOf-wrapped nullable item' => ['A_allOf', [null]]; + yield 'anyOf-wrapped nullable item' => ['A_anyOf', [null]]; + yield 'oneOf-wrapped nullable item' => ['A_oneOf', [null]]; + yield 'properties behind an allOf branch' => ['Nested_properties', ['p' => null]]; + yield 'items behind an allOf branch' => ['Nested_items', [null]]; + yield 'property of a dependent schema' => ['Dependent_schemas', ['a' => 'x', 'p' => null]]; + yield 'property of a then branch' => ['If_then', ['p' => null]]; + yield 'prefixItems entry' => ['Prefix_items', [null]]; + yield 'oneOf branch that is itself a composition' => ['Discriminated_oneOf', ['p' => null]]; + } + + /** + * @return iterable + */ + public static function nullRejectingSchemaProvider(): iterable + { + yield 'allOf-wrapped non-nullable property' => ['P_allOf_non_nullable', ['p' => null]]; + yield 'allOf-wrapped non-nullable item' => ['A_allOf_non_nullable', [null]]; + yield 'null matched by a composition under not' => ['Not_matching_composition', null]; + } + + #[Test] + #[DataProvider('nullAcceptingSchemaProvider')] + public function null_is_accepted_when_a_composition_branch_permits_it(string $schemaName, array $data): void + { + $validator = OpenApiValidatorBuilder::create()->fromYamlString(self::SPEC_YAML)->build(); + + $validator->validateSchema($data, '#/components/schemas/' . $schemaName); + + $this->expectNotToPerformAssertions(); + } + + #[Test] + #[DataProvider('nullRejectingSchemaProvider')] + public function null_is_still_rejected_when_no_composition_branch_permits_it(string $schemaName, mixed $data): void + { + $validator = OpenApiValidatorBuilder::create()->fromYamlString(self::SPEC_YAML)->build(); + + $this->expectException(ValidationException::class); + + $validator->validateSchema($data, '#/components/schemas/' . $schemaName); + } +} diff --git a/tests/Unit/Validator/Schema/SchemaHelperTest.php b/tests/Unit/Validator/Schema/SchemaHelperTest.php index f5d6777..36e1dd6 100644 --- a/tests/Unit/Validator/Schema/SchemaHelperTest.php +++ b/tests/Unit/Validator/Schema/SchemaHelperTest.php @@ -5,8 +5,10 @@ namespace Duyler\OpenApi\Test\Unit\Validator\Schema; use DateTime; +use Duyler\OpenApi\Schema\Model\Schema; use Duyler\OpenApi\Validator\Exception\InvalidDataTypeException; use Duyler\OpenApi\Validator\Schema\SchemaValueNormalizer; +use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\Attributes\Test; use PHPUnit\Framework\TestCase; use stdClass; @@ -251,4 +253,38 @@ public function type_includes_null_returns_true_for_array_with_only_null(): void self::assertTrue($result); } + + #[Test] + #[DataProvider('allowsNullProvider')] + public function allows_null_reads_the_whole_node(Schema $schema, bool $expected): void + { + $result = SchemaValueNormalizer::allowsNull($schema); + + self::assertSame($expected, $result); + } + + /** + * @return iterable + */ + public static function allowsNullProvider(): iterable + { + yield 'bare string' => [new Schema(type: 'string'), false]; + yield 'nullable string' => [new Schema(type: 'string', nullable: true), true]; + yield 'type array including null' => [new Schema(type: ['string', 'null']), true]; + yield 'ref' => [new Schema(ref: '#/components/schemas/Anything'), true]; + yield 'allOf' => [new Schema(allOf: [new Schema(type: 'string')]), true]; + yield 'anyOf' => [new Schema(anyOf: [new Schema(type: 'string')]), true]; + yield 'oneOf' => [new Schema(oneOf: [new Schema(type: 'string')]), true]; + yield 'not is not a deferral' => [new Schema(not: new Schema(type: 'string')), false]; + } + + #[Test] + public function allows_null_defers_nothing_when_nullable_is_not_a_type(): void + { + $schema = new Schema(allOf: [new Schema(type: 'string', nullable: true)]); + + $result = SchemaValueNormalizer::allowsNull($schema, nullableAsType: false); + + self::assertFalse($result); + } }