From 804ea3c32584da0d16284c3e1f3db29bc31bdb78 Mon Sep 17 00:00:00 2001 From: Woody Gilk Date: Tue, 11 Aug 2026 13:47:13 -0500 Subject: [PATCH] fix: Merge $ref sibling `nullable` with OR so 3.0 nullable targets keep null (#64) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ScalarSiblingMerger::merge()` combined the `$ref` stub's `nullable` flag with the resolved target's using a logical AND. `Schema::$nullable` defaults to `false` and a bare `{$ref: ...}` node never sets it, so every plain `$ref` evaluated `false && true` and discarded the target's `nullable: true`. The resolved schema kept its `type: string` but lost its nullability, and `TypeValidator` then rejected a legitimate `null`. The flag now merges with OR. Per OpenAPI 3.0 a `nullable` sibling next to `$ref` can only widen the target — there is no spelling for "narrow this to non-nullable" — which also makes `nullable` consistent with how the same class merges every other scalar ("sibling wins if set, otherwise the resolved value"). Three existing tests asserted the AND behaviour and were updated to the widening semantics; the sibling-false cases now document that `nullable: false` is indistinguishable from an omitted flag and so cannot remove the target's nullability. --- CHANGELOG.md | 13 ++ .../Model/Internal/ScalarSiblingMerger.php | 2 +- tests/Integration/NullableRefSiblingTest.php | 126 ++++++++++++++++++ .../PostRefactorBehavioralSnapshotTest.php | 12 +- .../Internal/ScalarSiblingMergerTest.php | 28 +++- .../Schema/Model/SchemaSiblingMergerTest.php | 10 +- 6 files changed, 177 insertions(+), 14 deletions(-) create mode 100644 tests/Integration/NullableRefSiblingTest.php diff --git a/CHANGELOG.md b/CHANGELOG.md index b546e73..c084dc4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,19 @@ 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 + +- OpenAPI 3.0 `nullable: true` is no longer discarded when a schema is + reached through `$ref`. `ScalarSiblingMerger::merge()` combined the `$ref` + stub's `nullable` flag with the resolved target's using a logical AND, and + because `Schema::$nullable` defaults to `false` a bare `{$ref: ...}` stub + always evaluated `false && true` — erasing the target's nullability, so a + legitimate `null` was rejected by `TypeValidator`. The flag now merges with + OR: per OpenAPI 3.0 a `nullable` sibling next to `$ref` can only widen the + target, and there is no spelling for "narrow this to non-nullable" (#64). + ## [0.7.0] Preparation for the 1.0.0 stable release. This section tracks work that diff --git a/src/Schema/Model/Internal/ScalarSiblingMerger.php b/src/Schema/Model/Internal/ScalarSiblingMerger.php index d8f8b79..c662de1 100644 --- a/src/Schema/Model/Internal/ScalarSiblingMerger.php +++ b/src/Schema/Model/Internal/ScalarSiblingMerger.php @@ -38,7 +38,7 @@ public function merge(SiblingMergeContext $context): array return [ 'format' => $this->mergeFormat($resolved->format, $sibling->format), 'type' => $this->mergeType($resolved->type, $sibling->type), - 'nullable' => $sibling->nullable && $resolved->nullable, + 'nullable' => $sibling->nullable || $resolved->nullable, 'const' => $sibling->hasConst ? $sibling->const : $resolved->const, 'hasConst' => $sibling->hasConst || $resolved->hasConst, 'multipleOf' => $this->mergeNullableIdentical($resolved->multipleOf, $sibling->multipleOf), diff --git a/tests/Integration/NullableRefSiblingTest.php b/tests/Integration/NullableRefSiblingTest.php new file mode 100644 index 0000000..cceba6b --- /dev/null +++ b/tests/Integration/NullableRefSiblingTest.php @@ -0,0 +1,126 @@ +validator()->validateSchema(['p' => null], '#/components/schemas/InlineProp'); + + $this->expectNotToPerformAssertions(); + } + + #[Test] + public function bare_ref_to_nullable_schema_accepts_null(): void + { + $this->validator()->validateSchema(['p' => null], '#/components/schemas/RefProp'); + + $this->expectNotToPerformAssertions(); + } + + #[Test] + public function ref_restating_nullable_accepts_null(): void + { + $this->validator()->validateSchema(['p' => null], '#/components/schemas/RefPropRestatingNullable'); + + $this->expectNotToPerformAssertions(); + } + + #[Test] + public function ref_to_type_union_with_null_accepts_null(): void + { + $this->validator()->validateSchema(['p' => null], '#/components/schemas/RefPropTypeUnion'); + + $this->expectNotToPerformAssertions(); + } + + #[Test] + public function nullable_sibling_widens_non_nullable_target(): void + { + $this->validator()->validateSchema(['p' => null], '#/components/schemas/RefPropNullableSibling'); + + $this->expectNotToPerformAssertions(); + } + + #[Test] + public function bare_ref_to_non_nullable_schema_still_rejects_null(): void + { + $this->expectException(ValidationException::class); + + $this->validator()->validateSchema(['p' => null], '#/components/schemas/RefPropNonNullable'); + } + + #[Test] + public function bare_ref_to_nullable_schema_still_rejects_wrong_type(): void + { + $this->expectException(ValidationException::class); + + $this->validator()->validateSchema(['p' => 42], '#/components/schemas/RefProp'); + } + + private function validator(): OpenApiValidatorInterface + { + return OpenApiValidatorBuilder::create()->fromYamlString(self::SPEC)->build(); + } +} diff --git a/tests/Integration/PostRefactorBehavioralSnapshotTest.php b/tests/Integration/PostRefactorBehavioralSnapshotTest.php index 35c0b1b..f61fd06 100644 --- a/tests/Integration/PostRefactorBehavioralSnapshotTest.php +++ b/tests/Integration/PostRefactorBehavioralSnapshotTest.php @@ -355,23 +355,25 @@ public function validator_compiler_combined_schema_enforces_all_supported_keywor } // --------------------------------------------------------------------- - // Zone 2: SchemaSiblingMerger — AND-merge semantics + // Zone 2: SchemaSiblingMerger — merge semantics // --------------------------------------------------------------------- #[Test] - public function sibling_merger_nullable_AND_semantics_sibling_false_blocks_null(): void + public function sibling_merger_nullable_widening_semantics_sibling_false_keeps_null(): void { $resolved = new Schema(type: 'string', nullable: true); $sibling = new Schema(nullable: false); $merged = new SchemaSiblingMerger()->merge($resolved, $sibling); - // AND semantics: both must allow null for the merge to allow null. - self::assertFalse($merged->nullable); + // Widening semantics: a `nullable` sibling next to `$ref` can only add + // null to the target. `nullable: false` is indistinguishable from an + // omitted flag, so it never removes the target's nullability. + self::assertTrue($merged->nullable); } #[Test] - public function sibling_merger_nullable_AND_semantics_both_true_allows_null(): void + public function sibling_merger_nullable_widening_semantics_both_true_allows_null(): void { $resolved = new Schema(type: 'string', nullable: true); $sibling = new Schema(nullable: true); diff --git a/tests/Unit/Schema/Model/Internal/ScalarSiblingMergerTest.php b/tests/Unit/Schema/Model/Internal/ScalarSiblingMergerTest.php index e6f7d5b..ffde949 100644 --- a/tests/Unit/Schema/Model/Internal/ScalarSiblingMergerTest.php +++ b/tests/Unit/Schema/Model/Internal/ScalarSiblingMergerTest.php @@ -34,18 +34,29 @@ public function merge_returns_overrides_for_eight_scalar_fields(): void } #[Test] - public function merge_nullable_uses_and_semantics_sibling_false_blocks_null(): void + public function merge_nullable_keeps_resolved_nullable_when_sibling_omits_it(): void { $resolved = new Schema(type: 'string', nullable: true); $sibling = new Schema(nullable: false); $overrides = new ScalarSiblingMerger()->merge(new SiblingMergeContext($resolved, $sibling)); - self::assertFalse($overrides['nullable']); + self::assertTrue($overrides['nullable']); + } + + #[Test] + public function merge_nullable_sibling_widens_non_nullable_resolved(): void + { + $resolved = new Schema(type: 'string', nullable: false); + $sibling = new Schema(nullable: true); + + $overrides = new ScalarSiblingMerger()->merge(new SiblingMergeContext($resolved, $sibling)); + + self::assertTrue($overrides['nullable']); } #[Test] - public function merge_nullable_and_semantics_both_true_allows_null(): void + public function merge_nullable_both_true_allows_null(): void { $resolved = new Schema(type: 'string', nullable: true); $sibling = new Schema(nullable: true); @@ -55,6 +66,17 @@ public function merge_nullable_and_semantics_both_true_allows_null(): void self::assertTrue($overrides['nullable']); } + #[Test] + public function merge_nullable_stays_false_when_neither_side_is_nullable(): void + { + $resolved = new Schema(type: 'string', nullable: false); + $sibling = new Schema(nullable: false); + + $overrides = new ScalarSiblingMerger()->merge(new SiblingMergeContext($resolved, $sibling)); + + self::assertFalse($overrides['nullable']); + } + #[Test] public function merge_format_returns_null_when_formats_differ(): void { diff --git a/tests/Unit/Schema/Model/SchemaSiblingMergerTest.php b/tests/Unit/Schema/Model/SchemaSiblingMergerTest.php index 98c8790..493ba3c 100644 --- a/tests/Unit/Schema/Model/SchemaSiblingMergerTest.php +++ b/tests/Unit/Schema/Model/SchemaSiblingMergerTest.php @@ -108,29 +108,29 @@ public function merge_upper_bound_inherits_resolved_when_sibling_null(): void } #[Test] - public function merge_nullable_and_when_resolved_rejects_null(): void + public function merge_nullable_widens_when_only_sibling_allows_null(): void { $resolved = new Schema(type: 'string', nullable: false); $sibling = new Schema(nullable: true); $merged = new SchemaSiblingMerger()->merge($resolved, $sibling); - self::assertFalse($merged->nullable); + self::assertTrue($merged->nullable); } #[Test] - public function merge_nullable_and_when_sibling_rejects_null(): void + public function merge_nullable_keeps_resolved_when_sibling_omits_nullable(): void { $resolved = new Schema(type: 'string', nullable: true); $sibling = new Schema(nullable: false); $merged = new SchemaSiblingMerger()->merge($resolved, $sibling); - self::assertFalse($merged->nullable); + self::assertTrue($merged->nullable); } #[Test] - public function merge_nullable_and_when_both_allow_null(): void + public function merge_nullable_when_both_allow_null(): void { $resolved = new Schema(type: 'string', nullable: true); $sibling = new Schema(nullable: true);