fix: Look through composition when pre-checking null (#66) - #67
Open
shadowhand wants to merge 1 commit into
Open
fix: Look through composition when pre-checking null (#66)#67shadowhand wants to merge 1 commit into
shadowhand wants to merge 1 commit into
Conversation
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.
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 #66.
The bug
Before validating a property or an array item, the validators run a
SchemaValueNormalizer::normalize($value, $allowNull)pre-check and computed$allowNullfrom the immediate schema node alone. A composition node carries notypeand nonullable— both live in its branches — so$allowNullwasfalseand a permittednullwas rejected byInvalidDataTypeExceptionbeforea single branch ran.
The fix
Composition and
$refnodes now defer the decision to the branch or the resolvedtarget, which already evaluates
nullablecorrectly per branch. Nulls that nobranch 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:That fixed six further reachable sites the report did not list:
propertiesanditemsbehind a branch,prefixItems,dependentSchemas,if/then,oneOfbranches, and
not.The
notsite was an inversion bug worth calling out:{allOf: [nullable], not: {allOf: [nullable]}}acceptednull, because thepre-check's
InvalidDataTypeExceptionwas caught and read as "the inner schemadid not match". It now rejects correctly.
OneOfValidatorWithContext::hasNullableSchema()is deliberately left alone — itanswers a different question (may a discriminated
oneOfshort-circuit on null?),which is a definitive accept rather than a deferral. Routing it through the new
helper would make a discriminated
oneOfacceptnullwhen no branch permits it.Scope note: the report's
P_allOf_refcase is not covered hereallOf: [{$ref: <3.0 nullable target>}]still fails on this branch, and it is notthis bug. It trips the
$refsibling merge of #64:ScalarSiblingMergerANDs thestub's
nullablewith the target's, discardingnullable: truebefore thispre-check ever runs. I confirmed the attribution by temporarily applying #64's
one-line fix — all cases then pass. The
$refcase tested here reaches itsnullability 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.phpcovers 19 cases through the public
validateSchema()API — the composition and$refforms from the issue, the six deeper keyword paths, and negative casesproving 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
falseone at atime; all six clauses and the
nullableAsTypeguard are killed by the suite, sono clause is untested.
make tests(7159 passing),make psalm,make cs-fix, andmake rectorare allclean. 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 whycomposition 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.