Repository navigation
Port fix of nullable allof ref type check to the 2.x branch - #1280
Conversation
A property declared nullable: true and composed via allOf containing only a $ref (e.g. OpenAPI 3.0 style composition) incorrectly rejected null values. The schema owning the failing type keyword is resolved through $ref and is a cached object whose lexical parent reflects where it is declared in the document, not where it was referenced from, so the existing one-hop parent/grandparent nullable check never found the nullable declaration on the referencing schema. Walk the dynamic evaluation stack instead, following through any chain of $ref schemas, to find the referencing schema and check it (and its lexical parent) for nullable: true. Ported from the equivalent fix on master. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Per PR review feedback (networknt#1279), the dynamic evaluation stack walk added to support nullable allOf + $ref compositions incorrectly honored nullable: true declared as a direct sibling of $ref. That's a Reference Object, and the OpenAPI 3.0 specification requires siblings of $ref to be ignored, so it must never grant nullability on its own. Track whether the current frame in the walk was reached via $ref and, if so, skip checking that frame's own nullable — only its lexical parent (the schema actually composing it, e.g. the allOf owner) is consulted. The allOf + $ref case that motivated the original fix still passes. Ported from the equivalent fix on master. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Confirms, per maintainer feedback on the upstream issue, that the dynamic evaluation stack walk added for nullable allOf + $ref compositions has no effect outside dialects that enable the nullable keyword (currently only OpenAPI 3.0.x) — null is still rejected for the same schema shape under draft-07. Ported from the equivalent fix on master. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
No ReportFindings tool is available in this environment (I searched the deferred tool list — only GitNexus/Monitor/WebFetch etc. are exposed), so the findings are stated in full here. What I did
Findings
After findReferencingSchema hops to the $ref holder, line 107 unconditionally checks that holder's lexical parent for nullable. The javadoc assumes that parent is "the schema actually composing it (e.g. the allOf owner)", but the holder's lexical parent is identical in shape for allOf: [{$ref}] and for properties: {x: {$ref}} / items: {$ref} / additionalProperties: {$ref} — the code cannot tell them apart, so the parent's nullable is applied to the referenced value instead of to the container. Verified differential behaviour (OpenAPI 3.0 dialect, base 2.x vs PR head):
Net effect: null values silently validate against non-nullable referenced components. (For fairness: the inline equivalent — items: {"type":"object"} under a nullable array — already leaked on 2.x via the old grandparent check, so this is a widening of a pre-existing quirk rather than a brand-new class; but for $ref children it is a real regression against the base branch, and $ref is the dominant OpenAPI style.) Fix direction: ExecutionContext.getEvaluationSchemaPath() is maintained in lockstep with evaluationSchema (see AbstractKeywordValidator:133-153), so the walk can require that the keyword linking parent→holder is a composition keyword (allOf/anyOf/oneOf/not) before honouring the parent's nullable.
findReferencingSchema returns null unless the immediately preceding frame's node has $ref, so the walk cannot cross an intermediate composition frame. Both of these still fail on the PR head (same as base):
Not a regression, but the heuristic is arbitrary in the "too narrow" direction while being over-broad in the direction of finding 1; a keyword-aware walk would address both.
It states equalsToSchemaType "only consults the schema that owns the failing type keyword and that schema's own parent" and that "the null-friendly check never fires and validation incorrectly fails" — that is exactly the code the same PR deletes. A reader hitting this test later will conclude the implementation still has the one-hop limitation. It should describe the expected behaviour, not the old defect. Line-summary
|
…/negatives Per PR review feedback (networknt#1280), the nullable walk had two bugs: it trusted a schema's lexical parent unconditionally, so nullable: true on a container leaked into any $ref-ed property/item/additionalProperties it held (a false accept); and it only crossed a single $ref hop, so nullable + allOf + $ref still failed to validate null when the referenced component itself used allOf internally (a false reject). Both stem from the same cause: getParentSchema() is set the same way for a composing keyword (allOf/oneOf/anyOf/not, which describes the same value) and a containing keyword (properties/items/ additionalProperties, which describes a distinct nested value), so the walk couldn't tell them apart. Determine this structurally from each schema's own schemaLocation instead, and only trust/continue through a parent when the connecting keyword is a composing one, falling back to the existing $ref hop otherwise. The walk is no longer capped at one hop in either direction. Add regression tests for the leaked-nullable case and for two levels of both composing depth and reference depth, confirming the fix generalizes rather than just patching the reported repro. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
isComposingKeyword read the wrong path segment for keywords whose
sub-schema is not array-indexed (additionalProperties, not, and
OpenAPI 3.0's single-schema items): their location ends directly in
the keyword name, unlike allOf/oneOf/anyOf array items which append
an index after it. This meant the check was actually reading the name
of whatever contained the keyword, not the keyword itself.
In the ordinary case this went unnoticed since that misread segment
rarely collides with a composing-keyword name, but a property or
schema literally named "allOf"/"oneOf"/"anyOf" containing an
additionalProperties/items $ref caused nullable to incorrectly leak
into it. Distinguish the two shapes by checking whether the schema's
own last path element is an array index (composing) or already the
keyword name itself (single-schema).
Also drop "not" from the composing-keyword set: not: {$ref: X} means
the value must NOT match X, not that X also describes it, so there's
no basis for propagating nullable through it.
Add a dedicated test per composing keyword (allOf/oneOf/anyOf/not)
and per containing keyword (properties/items/additionalProperties),
plus regression tests for the naming-collision leak.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…tail Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
This now properly covers both the cases your review has highlighted plus a few extra. I'll await the next review before opening another PR to port the additional fixes back to main. |
|
Reviewed networknt/json-schema-validator PR #1280 (fetched refs/pull/1280/head, base 2.x-line at ae64ed6; real change scope is cb258af~1...0a6feab, touching JsonNodeTypes.java plus two test classes). I built and ran the branch, plus the same probes against the pre-PR baseline, to confirm each finding empirically. Full existing suite: Tests run: 8434, Failures: 0 (74 errors are pre-existing GraalJS NoClassDefFound env issues, identical on baseline). Note: no ReportFindings tool is exposed in this environment (ToolSearch returns no match), so the findings are stated below. Findings (3)
Not flagged (checked and found sound): the $ref hop via findReferencingSchema picks the most recent stack occurrence, which is correct under recursion (Node→child→Node probes returned the right answers and did not loop); the deliberate exclusion of not is genuinely exercised by nullableNotDoesNotPropagateThroughNot; getElement(-1) on a root fragment returns null rather than throwing, so the null guard at line 131 is safe. |
|
Hi @stevehu please could you share the configuration of the review agent/skill you're using? I feel like this would be a lot easier if I could run it myself after making changes. |
…tion The static schema-location fragment could be rewritten by a dialect's id keyword, silently dropping a legitimately nullable allOf/oneOf/anyOf branch, and could also be mistaken for a composing keyword when a property happens to be literally named allOf/oneOf/anyOf. Checking whether the schema node is actually one of the parent's composing branches avoids both problems. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Each branch's type check independently treats null as a match when the oneOf is nullable, which only yields a valid oneOf result when there's exactly one branch — with two or more, every branch matches and oneOf reports "more than one match". Handling nullable directly in the validator avoids relying on that per-branch mechanism. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@tomakehurst The above review is just built in from Claude CLI with /review command. As this library is just a standalone component. For my other projects, I am using light-agent from light-fabric to drive codex and claude to mutually review each others work. The light-workflow is orchestrate all the activities. |
…diate parent The short-circuit only checked the oneOf schema's own node for "nullable", so a oneOf wrapped in allOf/anyOf/$ref with nullable declared further up was invisible to it, causing null to incorrectly fail with "multiple schemas matched" even though each branch's own type check already treats null as valid via the ancestor walk. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Returning before the branch loop for a null instance under a nullable oneOf meant branch schema.validate()/walk() never ran, silently dropping annotations and WalkListener invocations for that instance location even though the pass/fail verdict was unchanged. Branches now always run; only the "exactly one branch matched" check is skipped for the nullable-null case. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Code review(medium · 3 findings) Ran 1 shell command I reviewed PR #1280 (fix/nullable-allof-ref-type-check-2.x → 2.x) by building both the PR head and the 2.x base (ae64ed6) and diffing actual validation behavior. The full existing suite passes on the PR head (the only 74 errors are GraalJS/regex tests failing on Java 25, unrelated). Findings
Verified as non-issues along the way: no NPE path into OneOfValidator.validate with a Java-null node (the shouldValidateSchema && node != null guard matches AllOfValidator), no runaway loop or measurable cost in isNullableAncestor under deep/mutual $ref recursion (200 levels, ~2 ms), nullable does not leak into a $ref'd branch reused from a non-nullable site, and the id-rewrite test is meaningful since the OAS 3.0 dialect really does set idKeyword("id"). |
…cture The walk climbed parent pointers, identified composing branches by JsonNode reference identity, and inferred $ref hops by testing whether the previous frame's node had a $ref member. It could revisit a frame it had already left, so a recursive schema made it spin forever, and the $ref test misread a properties hop out of a schema that also carried a $ref, leaking nullable into a nested value. The engine already records both the schemas entered and the keyword each was entered by. Reading that instead makes the walk a single descent of a finite stack, so it cannot loop, and gives the hop keyword exactly rather than by inference. This also picks up if/then/else, $dynamicRef and $recursiveRef, and re-checks the dialect at each hop so a nullable member in a resource whose dialect lacks the keyword is not honoured. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The enum keyword decided whether null was permitted when the validator was constructed, by reading nullable off its immediate parent. That missed nullable declared further up, so a nullable allOf referencing an enum component still rejected null even though type accepted it. Enum components behind allOf and $ref are the common OpenAPI shape, so the keyword disagreed with type on the schemas most likely to use it. Whether null is permitted depends on the ancestors the value is reached through, which is only known once validation is under way, so the check moves there. The constructor keeps reporting null in the message when the owning schema declares it, leaving existing messages unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Suppressing the whole error block meant that if no branch matched, the branch errors explaining why were discarded and the value passed silently. Only the "exactly one branch matched" requirement conflicts with nullable, so only that is relaxed now, and a null that matches nothing reports its errors as before. Stopping the loop once two branches match also cannot apply here. That short-circuit assumes a second match settles the outcome, which is no longer true once the count is allowed to exceed one, and it left the remaining branches without their annotations and walk listeners. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Replacing the two-level ancestor check with a walk that stops at keywords describing a nested value also stopped nullable on a container from reaching its inline properties, items and additionalProperties children. That is the intended reading, but nothing pinned it: the existing tests all reach their children through $ref and pass equally well against the previous behaviour. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The nullable tests repeated the same components block nine times and the same registry setup twenty-four times, and the allOf, oneOf and anyOf cases differed only in that one word. Sharing the block and the setup, and driving the three composing keywords from one parameterised test, leaves each test showing only what makes it distinct. Also corrects a doubled negative in a method name. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Separately from the review feedback, I did a deeper pass over this branch and found a cluster of problems in the ancestor-walk implementation that hadn't been raised. Rather than patch them one at a time, I rewrote the walk on top of information the engine already tracks. The change
None of that is necessary. Net −101 lines in Bugs this fixesInfinite loop on recursive schemas. {"$ref": "#/$defs/X",
"$defs": {"T": {"title": "t"},
"X": {"$ref": "#/$defs/T", "type": "object",
"properties": {"c": {"$ref": "#/$defs/X"}}}}}against Worth noting this needs a specific shape — a
{"properties": {"value": {"allOf": [{"$ref": "#/components/schemas/E"}], "nullable": true}},
"components": {"schemas": {"E": {"type": "string", "enum": ["a", "b"]}}}}
This also turned out to fix the composing-keyword inconsistency raised in review at a deeper level than the suggested narrowing: because branches now accept Keyword coverage gaps.
Dialect boundaries. Correctness of the Also in this branch
VerificationFull suite at each commit: 8453 tests, 0 failures. The 74 errors are the pre-existing GraalJS/regex failures caused by the JDK blocking GraalVM's native access (JDK 26 locally), unrelated to this change and equally present on the base commit. The behavioural claims above were checked by running the same probes against base Two items I addressed by construction but did not independently reproduce first, so flagging them as such: the |
|
Re-reviewed the 5 new commits (384b23b…b540a79) against a fresh build. Full suite green: 8396 tests, 0 failures, 0 errors (Graal/regex classes excluded — they fail on Java 25 for unrelated reasons). All three earlier findings are resolved, and the fixes go further than patching symptoms:
Two remaining items:
Neither blocks the OpenAPI 3.0 behavior the PR targets; the first is the one I'd fix before merge, since it's the regression guard for the change made in 392b127. |
The schema's single branch matched the null, so oneOf reported nothing and the assertion was satisfied entirely by the sibling not keyword. Reverting the cardinality narrowing left the test green, so it guarded nothing. The branch now rejects null on its own terms, and the assertion looks for the oneOf error specifically rather than any error at all. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Members declared alongside a reference are only dropped before 2019-09, but the walk skipped the referencing schema's nullable in every dialect. On a 2020-12 dialect with nullable enabled that made one declaration mean two things: honoured for the holder's own type check, ignored for the referenced schema's. The skip is now gated on the same specification version the engine uses when it assembles a schema's validators. OpenAPI 3.0 is draft-04 based and unaffected; it is the only shipped dialect registering nullable. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Re-reviewed at Full suite: 8454 tests, 0 failures. The 74 errors are the pre-existing GraalJS/ Both prior items are fixed — and the tests genuinely guard themI mutation-tested rather than trusting the green suite, since "the test guards nothing" was the substance of the first item:
Two new findings, both low1. 2. Head is the more defensible answer — the literal string Checked and soundMember order around Neither finding blocks the OpenAPI 3.0 behaviour the PR targets. I'd merge after a changelog line for the |
…se one Three user-visible changes were missing from the changelog: the oneOf cardinality relaxation, which is the largest of them, nullable no longer reaching into a not subschema, and the typeLoose enum case. That last one was an unintended consequence of moving the null decision out of the enum constructor. The accepted set no longer holds a NullNode, and the typeLoose comparison matched on its text, so the string "null" used to satisfy an enum on a nullable schema. Rejecting it is the better answer, but it was unguarded, so it gets a test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
I found no bugs in PR #1280, either in the latest commit (09deacf, which adds changelog entries and a typeLoose test) or in the nullable changes it builds on. How I checked:
|
Resolves the OpenAPI nullable keyword by descending the engine's own evaluation stacks rather than reconstructing provenance from schema structure. The previous walk climbed lexical parents and sniffed for $ref members, which misattributed nullable across reference sites, was sensitive to schema-node identity, and missed $dynamicRef and $recursiveRef entirely. The walk pairs evaluationSchema with evaluationSchemaPath to recover the keyword each schema was entered by, propagating nullable through allOf/oneOf/anyOf, if/then/else and the three referencing keywords, and stopping at any keyword that moves to a different value. As a result nullable no longer leaks into a container's properties, items or additionalProperties children, and no longer reaches inside not. Applies the same walk to enum, which previously decided at construction time whether null was accepted, and relaxes only the oneOf cardinality check for a nullable null, leaving branch errors reported when nothing matches. Ported from #1280. Two adaptations were needed for this branch rather than a straight copy: - isNodeNullable now calls asBoolean(false). Jackson 3 throws on a node it cannot coerce where Jackson 2 returns false, and the walk reads nullable on every composing ancestor, so a malformed member anywhere in the chain would abort the validation. - The typeLoose enum change concerns the empty string here, not the string "null". NullNode.asString() is "" in Jackson 3, so that is the value the accepted set silently permitted. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
2 participants
No description provided.