Repository navigation
Conversation
83e1142 to
b02a6f6
Compare
b02a6f6 to
ed87c22
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Critical unresolved-reference errors can still be silently discarded by schema applicators.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 3
Open (3)
What changed in this PR
Reports unresolved $ref values as validation errors in strict draft-specific validation.
Changes:
- Adds
UNRESOLVABLE_REF. - Updates Draft 06, 07, and 2019 reference constraints.
- Adds resolved and unresolved reference tests.
| File | Review |
|---|---|
tests/Constraints/Draft2019/RefConstraintTest.php |
Adds Draft 2019 reference tests. |
tests/Constraints/Draft07/RefConstraintTest.php |
Adds Draft 07 reference tests. |
tests/Constraints/Draft06/RefConstraintTest.php |
Adds Draft 06 reference tests. |
src/JsonSchema/Constraints/Drafts/Draft2019/RefConstraint.php |
Unresolved-reference errors can be erased by applicators; chained references may report the wrong pointer. |
src/JsonSchema/Constraints/Drafts/Draft07/RefConstraint.php |
Unresolved-reference errors can be erased by applicators; chained references may report the wrong pointer. |
src/JsonSchema/Constraints/Drafts/Draft06/RefConstraint.php |
Unresolved-reference errors can be erased by applicators; chained references may report the wrong pointer. |
src/JsonSchema/ConstraintError.php |
Defines the new constraint error and message. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| } catch (\Exception $e) { | ||
| // A $ref that cannot be resolved is a broken schema, not an absent keyword: | ||
| // returning here would let the value pass as if the $ref had not been written. | ||
| $this->addError(ConstraintError::UNRESOLVABLE_REF(), $path, ['ref' => $schema->{'$ref'}]); |
There was a problem hiding this comment.
Confirmed, measured on all three drafts in strict mode: anyOf [$ref, {}], oneOf [$ref, {}] and not $ref all come out valid with an unresolvable $ref. I laid out the options in the main thread, since the fix changes the approach the PR was approved with.
ed87c22 to
ea53d6d
Compare
|
@DannyvdSluijs thanks for the approval! Before you merge: CI went red after I rebased onto
Two ways I see: keep the validation error, skip the 44 and handle the applicators in a follow-up; or switch to option 3 from #916 (throw in strict mode, honouring |
|
@Amoifr Thanks for your efforts and your persistence. In all honestly the report #916 was created by Claude while I was working on something else and it asked confirmation to do so. I agreed and forgot about it. Now reading the original report I believe that option 2 throwing the error is the best option (I've should have researched this earlier, my apologies.). The Strict mode was designed to validate the schema per its draft. Currently the default behaviour is a historical mix and mash of Draft 3 and Draft 4. Which I no longer wanted to support / Could not hold longer due to the incompatibilities between drafts. Would you mind updating the PR to throw an exception? Or actually no longer capture the exception. Curious to see what the fallout would be. |
ea53d6d to
04380ae
Compare
|
No need to apologise, it was a fun one to dig into, and thanks for making the call! 🙂 Done: the three draft The fallout you were curious about:
That last group is the real user-facing impact: a schema using one of those patterns in strict mode used to pass (partly by accident) and now throws. Fine by me given your reasoning, just flagging it so it does not come as a surprise in the release notes. I also retitled the PR to match. |
|
@Amoifr Code looks good but the pipeline still reports some issues. Could you take a look? |
|
Thanks for the review! The two red tests are |

Closes #916.
This takes option 1 from the issue, a validation error, and there is an argument for it beyond it being the least disruptive:
CHECK_MODE_STRICTis currently more permissive than the default mode. Same schema, same data, onmain:The legacy path already refuses to let a broken
$refthrough; it is only the draft-specific constraints that swallow it. So this is not just a missing feature, it is the strict mode being laxer than the mode it is supposed to tighten. After the change, strict mode reports one error and the default mode is untouched.On the error constant
The issue suggests
MISSING_ERRORor a new one.MISSING_ERRORis not available: its message is commented out inConstraintErrorwithUsed for tests; this error is deliberately commented out, so it is reserved. This addsUNRESOLVABLE_REFwith the messageThe $ref "%s" could not be resolved, which also names the pointer that failed rather than leaving the reader to find it.On the offline case
The issue rightly raises unreachable remote schemas. Nothing changes there for anyone on the default mode, which already throws. For strict mode, a remote
$refthat cannot be fetched now produces a validation error rather than a silent pass, which I think is the honest answer: the validator cannot claim a document conforms to a schema it never read. Option 3, gating onCHECK_MODE_EXCEPTIONS, remains available on top of this if you would rather make it configurable.Verification
Two tests per draft: an unresolvable
$refis reported with the new constraint name, and a resolvable one still validates. Each fails without the change.Full suite green at 3166 tests, including the official JSON-Schema-Test-Suite, which is what I would have expected to break had the previous behaviour been load-bearing. PHPStan clean.