Skip to content

fix: Look through composition when pre-checking null (#66) - #67

Open
shadowhand wants to merge 1 commit into
duyler:mainfrom
shadowhand:fix/66-nullable-in-composition
Open

fix: Look through composition when pre-checking null (#66)#67
shadowhand wants to merge 1 commit into
duyler:mainfrom
shadowhand:fix/66-nullable-in-composition

Conversation

@shadowhand

Copy link
Copy Markdown

Closes #66.

The bug

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 fix

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 arriving as a pathless error — which also addresses the "error carries
no location" note in the issue for this class of failure.

Consolidation

The issue counted four copies of the rule. There were nine, and they disagreed
on more than $ref. All nine now delegate to one helper:

SchemaValueNormalizer::allowsNull(Schema $schema, bool $nullableAsType = true): bool

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 worth calling out:
{allOf: [nullable], not: {allOf: [nullable]}} accepted null, because the
pre-check's InvalidDataTypeException was caught and read as "the inner schema
did not match". It now rejects correctly.

OneOfValidatorWithContext::hasNullableSchema() is deliberately left alone — it
answers a different question (may a discriminated oneOf short-circuit on null?),
which is a definitive accept rather than a deferral. Routing it through the new
helper would make a discriminated oneOf accept null when no branch permits it.

Scope note: the report's P_allOf_ref case is not covered here

allOf: [{$ref: <3.0 nullable target>}] still fails on this branch, and it is not
this bug. It trips the $ref sibling merge of #64: ScalarSiblingMerger ANDs the
stub's nullable with the target's, discarding nullable: true before this
pre-check ever runs. I confirmed the attribution by temporarily applying #64's
one-line fix — all cases then pass. The $ref case tested here reaches its
nullability through a composition instead, so this suite proves the deferral
without depending on #64. The exclusion is documented in the test docblock.

Tests

Written test-first. tests/Unit/Regression/NullableInCompositionRegressionTest.php
covers 19 cases through the public validateSchema() API — the composition and
$ref forms from the issue, the six deeper keyword paths, and negative cases
proving nulls that no branch permits are still rejected. Nine unit tests cover the
helper directly.

I also mutation-tested the helper by replacing each clause with false one at a
time; all six clauses and the nullableAsType guard are killed by the suite, so
no clause is untested.

make tests (7159 passing), make psalm, make cs-fix, and make rector are all
clean. The two deprecation notices in the test output are pre-existing — I verified
them on a stashed tree.

Deviation to flag

SchemaValueNormalizer::allowsNull() carries a short prose docblock explaining why
composition defers. Per §12 this is a docblock rather than an inline // comment,
and grep -rn "^\s*//" src/ still returns only the // §6 exemption: ADR marker.
Happy to strip the prose if you would rather the helper stand on its name alone.

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 duyler#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 duyler#64.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

nullable inside allOf/anyOf/oneOf is invisible to the null pre-check, so valid nulls are rejected before any branch runs

1 participant