fix: Report each composition branch error once, with a structured error - #53
Open
shadowhand wants to merge 1 commit into
Open
fix: Report each composition branch error once, with a structured error#53shadowhand wants to merge 1 commit into
shadowhand wants to merge 1 commit into
Conversation
allOf/anyOf/oneOf could throw a ValidationException with an empty getErrors(): a value rejected while normalizing a branch (typically null against a non-nullable branch) was wrapped in a bare exception carrying nothing structured, so getFormattedErrors() returned "". allOf also reported every branch error twice, and counted failures from a bucket that bare AbstractValidationError branches never reached — hence "but 0 failed" beside a non-empty error list. BranchOutcome and ValidationResult now carry a single canonical error list plus a failedCount; a rejected value synthesises a TypeMismatchError with the branch dataPath/schemaPath; and the allOf message counts the branches that did not match. The MAX_COMPOSITION_ERRORS cap counts the merged list. Closes duyler#52
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 #52.
Composition branch errors were lost, duplicated, or inconsistent between
keywords, and
getErrors()could come back empty. Three defects, all in thebranch-error plumbing shared by
allOf/anyOf/oneOf:AbstractCompositionalValidator::validateBranch()caught the
InvalidDataTypeExceptionthrown bySchemaValueNormalizerandwrapped it in a bare
ValidationException— noerrors:, noabstractErrors. Nothing structured was ever synthesised for the rejectedvalue, so
getErrors()was[]andgetFormattedErrors()was"".OneOfValidatorWithContext::validateWithoutDiscriminator()reached the sameend state via
catch (InvalidDataTypeException) { continue; }.allOfduplication.validateBranch()returned each failure twice — aserrors: [$e]and again asabstractErrorsextracted from$e->getErrors()— andAllOfValidatormerged both buckets.AnyOf/OneOfValidatorread onlyabstractErrors, hence the disagreementbetween keywords.
allOfmessage formattedcount($result->errors), which a branch throwing a bareAbstractValidationErrornever reached —"but 0 failed"beside anon-empty error list.
Changes
BranchOutcomeandValidationResultnow carry a single canonical errorlist (
list<ValidationErrorInterface>) instead of two overlapping ones, sono caller can double-count.
ValidationResult::$abstractErrorsis mergedinto
$errors, and a new$failedCountrecords how many branches did notmatch.
InvalidDataTypeExceptionwas swallowed, both validator families nowsynthesise a
TypeMismatchErrorcarrying the branch'sdataPathandschemaPath(/allOf/0,/oneOf/1, …) withactual: 'null'for the commoncase — following the
DependentSchemasValidatorprecedent of never throwingwith an empty list. A
ValidationExceptionfrom a branch that carries nostructured errors falls back to
NestedValidationErrorfor the same reason.allOfmessage derives its count fromfailedCount, not from an errorbucket.
MAX_COMPOSITION_ERRORS/TooManyErrorsErrornow caps the merged list.Behaviour is equivalent for the previously-counted case (20 branch errors →
20 +
TooManyErrorsError); previouslyallOfre-appended the wrappererrors after the cap, so a 30-failing-branch
allOfreturned 41 errorsdespite the cap. It now returns 21. Covered by
CompositionBranchErrorsTest::composition_errors_are_capped.Error identity is preserved: nested branch failures keep their original
keyword /
dataPath/schemaPathrather than being flattened into a genericwrapper.
DiscriminatorDataErrorandOneOfErrorreporting are untouched.Before / after
validateSchema(), running the repro from #52 verbatim(
B = FormIdentifier,errors=iscount($e->getErrors())):{allOf: [B]}+nulltype@/{anyOf: [B]}+nulltype@/{oneOf: [B]}+nulltype@/{allOf: [B]}+"not-an-object"type@/type@/Balone +{type: widgets}required@/required@/(unchanged){allOf: [B]}+{type: widgets}enum@/type, enum@/typeenum@/type{anyOf: [B]}+{type: widgets}enum@/typeenum@/type{oneOf: [B]}+{type: widgets}required@/required@/Message wording for
anyOf/oneOfis unchanged.PSR-7
validateResponse(), root-levelcontent.application/json.schemaof{allOf: [$ref B]}:nullgetFormattedErrors()=""Expected type "object", but got "null" at /{"type":"widgets"}And
{allOf: [B, C]}with both branches failing now reports one entry perdistinct violation with
"but 2 failed"(was 4 entries).Tests
Written before the fix, each verified failing against
mainfirst:tests/Unit/Validator/SchemaValidator/CompositionBranchErrorsTest.php—AllOf/AnyOf/OneOfValidator(the non-context family): per-branchschemaPathindices, single-branch and multi-branch failure counts,multiple errors from one branch, keyword agreement, and the error cap.
tests/Integration/Validator/Schema/OneOfValidatorWithContextTest.php—OneOfValidatorWithContext: structured errors for rejectednull(including untyped, scalar, and
typearray branches), no duplication,accumulation across branches, and preservation of every error from a single
branch.
tests/Functional/Schema/CompositionBranchErrorsTest.php— the Composition validators lose, duplicate, and disagree about branch errors; getErrors() can be empty #52 reprothrough
validateSchema(), plusgetFormattedErrors()is never empty andnever repeats.
tests/Functional/Response/CompositionBranchErrorsTest.php— the PSR-7validateResponse()path with anullbody.ValidationResultTestis updated for the merged bucket.make tests(7161 tests, green — the 2 reported deprecations are pre-existingand unrelated),
make psalm(no errors),make cs-fix,make rectorallclean.
make infectionscoped to the seven touched files: covered MSI 85% →91% (gate is 78%). The remaining escaped mutants are on lines this PR does
not modify, plus one harmless
array_values()unwrap.Notes
violations surfaces first still depends on the wrapper — branches go through
the non-context
SchemaValidator, which orders keyword groups differentlyfrom
SchemaValidatorWithContext. This PR does not change that (see theoneOf + {type: widgets}row:required@/before and after), and neitherpath is exhaustive. The one functional assertion that would have straddled
the two families is scoped to
allOf/anyOfwith a comment pointing at Composition validators lose, duplicate, and disagree about branch errors; getErrors() can be empty #52.ValidationResultis a shared internal DTO of the compositional validators(its only callers are the four classes touched here); its property set
changes as described above.
ValidationException::getErrors()and theAbstractValidationErrorshape are unchanged — this PR only adds errorswhere there were none.
nullablewhen it sits beside a composition keyword #51 (issue Combining "nullable" and "allOf" does not work correctly #50), which touches the same threevalidate()methods but only adds an early
returnat the top of each. This branch iscut from
main; it will need no more than a trivial rebase whichever landsfirst.