fix: Honor nullable when it sits beside a composition keyword - #51
Open
shadowhand wants to merge 1 commit into
Open
fix: Honor nullable when it sits beside a composition keyword#51shadowhand wants to merge 1 commit into
nullable when it sits beside a composition keyword#51shadowhand wants to merge 1 commit into
Conversation
…er#50) In OAS 3.0 a `$ref` cannot have siblings, so the documented way to make a referenced schema nullable is to wrap it: data: allOf: - $ref: '#/components/schemas/FormIdentifier' nullable: true `TypeValidator` honors `nullable` on the schema being validated, but the composition validators consulted `nullable` on each *branch* instead — `AbstractCompositionalValidator::normalizeForBranch()` and `OneOfValidatorWithContext` never saw the parent that carries the keyword. So `null` was dispatched into every branch and failed there. Adding `type: object` next to the composition keyword did not help: `TypeValidator`'s early return only skips its own keyword. `AllOfValidator`, `AnyOfValidator`, `OneOfValidator` and `OneOfValidatorWithContext` now short-circuit when the schema carrying the keyword is nullable. `anyOf`/`oneOf` treat this as the keyword being satisfied rather than as a matching branch, so `oneOf` still enforces exactly-one for non-null data. Branch-level `nullable` keeps working unchanged, and the short-circuit is gated on `nullableAsType`. Audit of the other `SchemaValueNormalizer::normalize()` callers: `properties`, `items` and `prefixItems` (and their `*WithContext` variants) normalize a *child* value against that child's own schema, so consulting the sub-schema is correct there — no defect. `DependentSchemasValidator` returns early for non-array data, so `null` never reaches its normalize call. `NotValidator` already accepts `null` under a nullable parent, because a `not` branch that fails to normalize counts as not-matched. `IfThenElseValidator` did share the defect and is fixed: `{if: …, else: …, nullable: true}` leaked an `InvalidDataTypeException` out of the `else` branch for `null`. The nullable rule now lives in one place, `SchemaValueNormalizer::isNullableSchema()`, which `TypeValidator` also routes through.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #50.
The bug
In OAS 3.0 a
$refcannot have siblings, so the documented way to make a referenced schema nullable is to wrap it:nullwas rejected.TypeValidator::validate()honorsnullableon the schema being validated, butAbstractCompositionalValidator::normalizeForBranch()consulted$subSchema->nullable— the nullability of each branch — and never saw the parent that carries the keyword.AllOfValidator,AnyOfValidatorandOneOfValidatortherefore dispatchednullinto every branch, where it failed.OneOfValidatorWithContext::hasNullableSchema()made the same branch-only assumption on the context-carrying path, so both validator families were affected. Addingtype: objectbeside the composition keyword did not help —TypeValidator's early return only skips its own keyword.Before / after
The issue's repro script, unchanged:
Full matrix via
validateSchema(), same branch schemaB={type: object, required: [type, id], properties: {type: {enum: [forms]}, id: {type: string}}}in every row:{allOf: [B], nullable: true}{type: object, allOf: [B], nullable: true}{anyOf: [B], nullable: true}{oneOf: [B], nullable: true}{allOf: [B]}(not nullable){allOf: [B + nullable: true]}(branch nullable){type: object, nullable: true, …}(no composition)The fix
AllOfValidator,AnyOfValidator,OneOfValidatorandOneOfValidatorWithContextshort-circuit when the schema carrying the keyword is nullable, mirroringTypeValidator. ForanyOf/oneOfthis means the keyword is satisfied — not that a branch matched — sooneOfstill enforces exactly-one for non-null data. The rule lives in one place,SchemaValueNormalizer::isNullableSchema(), whichTypeValidatornow routes through too.Deliberate boundaries:
nullableAsType. WithdisableNullableAsType(),nullagainst a nullable composition still fails, matching today's behaviour for plain nullable schemas.nullableand OAS 3.1type: ['object', 'null']are unchanged, inside and outside compositions. Anullmember of atypeunion is ordinary JSON Schema and keeps composing —{type: ['object','null'], allOf: [B]}still rejectsnull, becauseallOfis an in-place applicator. Onlynullable: true, which OAS 3.0.3 defines as "allows sending anullvalue for the defined schema", waives branches. Both directions are pinned by tests.oneOfwith a nullable parent acceptsnulland still discriminates non-null data.Audit of the other
SchemaValueNormalizer::normalize()callersThe defect is specific to in-place applicators — a sub-schema applied to the same data instance the parent's
nullablecovers.PropertiesValidator,ItemsValidator,PrefixItemsValidatorand the*WithContextvariants normalize a child value against that child's own schema. The sub-schema consulted is the schema being applied to that value, so branch-level nullability is the correct question there; the parent'snullabledescribes the parent value, not its children.DependentSchemasValidatorreturns early for non-array data, sonullnever reaches itsnormalize()call.NotValidatoralready acceptsnullunder a nullable parent: anotbranch that fails to normalize counts as not-matched, which satisfiesnot.IfThenElseValidatordid share the defect.{if: …, else: …, nullable: true}leaked anInvalidDataTypeException(not even aValidationException) out of theelsebranch fornull.The compiler path needs no change:
UnsupportedKeywordDetectorrejectsallOf/anyOf/oneOf, so compiled validators never reach this code.Tests
Written before the fix and confirmed failing for the documented reason:
tests/Unit/Validator/SchemaValidator/NullableCompositionTest.php— the full acceptance matrix viavalidateSchema(), thenullableAsType: falserows, OAS 3.1 type unions in both directions,oneOfoverlap rejection, discriminatedoneOf, and the other in-place applicators.tests/Functional/Response/NullableCompositionTest.php— the issue's exact JSON:API-shaped document through the PSR-7validateResponse()path and its nestedpropertieschain, which is where real documents hit this.AllOfValidatorTest,AnyOfValidatorTest,OneOfValidatorTestandIfThenElseValidatorTestto cover the no-context path.make tests(7177 tests),make psalm(no errors),make cs-fixandmake rectorare clean. Scopedmake infectionover the touched files: covered MSI 90% (threshold 78) — the one escaped mutant in new code is an equivalent?->→->rewrite inside a??.Out of scope
ValidationException::getErrors()returning an empty list for composition failures (the issue's secondary observation) is untouched and still reproduces for genuinely-failingallOfbranches —AbstractCompositionalValidator::validateBranch()drops branch errors that are notAbstractValidationErrorinstances. Worth its own issue.