Skip to content

fix: Merge $ref sibling nullable with OR so 3.0 nullable targets keep null (#64) #65

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
shadowhand wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: Merge $ref sibling nullable with OR so 3.0 nullable targets keep null (#64) #65

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion src/Schema/Model/Internal/ScalarSiblingMerger.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
126 changes: 126 additions & 0 deletions tests/Integration/NullableRefSiblingTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,126 @@
<?php

declare(strict_types=1);

namespace Duyler\OpenApi\Test\Integration;

use Duyler\OpenApi\Builder\OpenApiValidatorBuilder;
use Duyler\OpenApi\Builder\OpenApiValidatorInterface;
use Duyler\OpenApi\Validator\Exception\ValidationException;
use PHPUnit\Framework\Attributes\Test;
use PHPUnit\Framework\TestCase;

final class NullableRefSiblingTest extends TestCase
{
private const string SPEC = <<<'YAML'
openapi: 3.0.0
info:
title: Nullable Ref API
version: 1.0.0
paths: {}
components:
schemas:
NullableString:
type: string
nullable: true
UnionNullString:
type: [string, 'null']
NonNullableString:
type: string

InlineProp:
type: object
properties:
p:
type: string
nullable: true
RefProp:
type: object
properties:
p:
$ref: '#/components/schemas/NullableString'
RefPropRestatingNullable:
type: object
properties:
p:
$ref: '#/components/schemas/NullableString'
nullable: true
RefPropTypeUnion:
type: object
properties:
p:
$ref: '#/components/schemas/UnionNullString'
RefPropNullableSibling:
type: object
properties:
p:
$ref: '#/components/schemas/NonNullableString'
nullable: true
RefPropNonNullable:
type: object
properties:
p:
$ref: '#/components/schemas/NonNullableString'
YAML;

#[Test]
public function inline_nullable_property_accepts_null(): void
{
$this->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();
}
}
12 changes: 7 additions & 5 deletions tests/Integration/PostRefactorBehavioralSnapshotTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
28 changes: 25 additions & 3 deletions tests/Unit/Schema/Model/Internal/ScalarSiblingMergerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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
{
Expand Down
10 changes: 5 additions & 5 deletions tests/Unit/Schema/Model/SchemaSiblingMergerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down