fix: Resolve $ref in requestBody instead of dropping it - #57
Open
shadowhand wants to merge 1 commit into
Open
Conversation
`ComponentTreeBuilder::buildRequestBody()` read only `description`, `content` and `required`, so a `requestBody` that was a Reference Object parsed to an empty `RequestBody` with the pointer discarded. `RequestBodyValidatorWithContext::validate()` then returned at its `null === $requestBody->content` early exit, because the `required: true` that would have forced a `MissingRequestBodyException` had been dropped along with the `$ref`. The result was fail-open: every request body behind a `$ref` went completely unvalidated. Malformed payloads passed, `required` was not enforced, and unsupported media types were accepted — with no exception and no warning. Inlining the same body validated correctly, so the breakage was invisible from the spec alone. Reference support is now wired through the five layers that `Parameter` and `Response` already used: - `RequestBody` gains `ref`/`refSummary`/`refDescription` and serializes back to a Reference Object, matching `Response`. - `ComponentTreeBuilder::buildRequestBody()` branches on `$ref`. - `DocumentNavigator` accepts `#/components/requestBodies/*` targets; `RefCache` widens to match. - `RefResolverInterface` gains `resolveRequestBody()` and `resolveRequestBodyWithOverride()`. - `RequestBodyValidatorWithContext::validate()` dereferences before use. Chained and dangling references behave as they do for responses: chains follow through, and an unresolvable or wrongly-typed pointer throws `UnresolvableRefException` rather than failing open. Per OAS 3.1+, a `description` sibling of `$ref` overrides the referenced description. Webhooks and callbacks are fixed by the same change, since both validate through `RequestValidator`. Inline request bodies take an unchanged path — `resolveRequestBodyWithOverride()` returns the same instance when `ref` is null. Closes duyler#56
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.
Fixes #56.
The bug
ComponentTreeBuilder::buildRequestBody()read onlydescription,contentandrequired, so arequestBodythat was a Reference Object parsed to an emptyRequestBodywith the pointer discarded.RequestBodyValidatorWithContext::validate()then returned at itsnull === $requestBody->contentearly exit — and therequired: truethat would otherwise have thrownMissingRequestBodyExceptionhad been dropped along with the$ref.The result was fail-open: every request body behind a
$refwent completely unvalidated. Malformed payloads passed,requiredwas not enforced, and unsupported media types were accepted, with no exception and no warning. Inlining the same body validated correctly, so nothing about the spec hinted at the breakage.The fix
Reference support is wired through the five layers that
ParameterandResponsealready used. The shape mirrorsResponseat every step rather than inventing anything new:Schema\Model\RequestBodyref/refSummary/refDescription;jsonSerialize()emits a Reference Object whenrefis setSchema\Parser\Internal\ComponentTreeBuilderbuildRequestBody()branches on$refValidator\Schema\Internal\DocumentNavigator#/components/requestBodies/*targets;RefCachewidens to matchValidator\Schema\RefResolver{,Interface}resolveRequestBody()andresolveRequestBodyWithOverride()Validator\Request\RequestBodyValidatorWithContextBehaviour matches the response side: chained references follow through, and an unresolvable or wrongly-typed pointer throws
UnresolvableRefExceptioninstead of failing open. Per OAS 3.1+, adescriptionsibling of$refoverrides the referenced description. Webhooks and callbacks are fixed by the same change, since both validate throughRequestValidator— there's a test for the webhook path.Inline request bodies take an unchanged path:
resolveRequestBodyWithOverride()returns the same instance whenrefis null.Tests
21 new tests. I confirmed they fail without the
src/changes (31 failures/errors withsrc/reverted, all green with it) so none of them pass vacuously — worth checking, since the pre-fix behaviour was "everything is accepted", which quietly satisfies any happy-path assertion.tests/Functional/Request/RequestBodyRefTest.php— end-to-end: pointer survives parsing, valid body passes, schema violation / missing required property / missing body / unsupported media type are all rejected, chained refs resolve, dangling and mistyped refs throw, siblingdescriptionoverrides, webhook path, serialization round-trip.tests/Unit/Validator/Schema/RefResolverRequestBodyTest.php— resolver level: caching, chaining, circular refs, external refs, cross-type rejection in both directions (resolveRequestBodyon a schema/response, andresolve/resolveParameter/resolveResponseon a request body), and the override semantics.tests/Unit/Schema/Parser/ReferenceOverrideTest.php— addsRequestBodyto the existing Reference Object parity suite.Verified on the full suite: 7163 tests pass (7142 before),
psalmreports no errors,php-cs-fixerandrectorare clean. The 2 reported deprecations are pre-existing and unrelated (tests that deliberately exercise deprecated factories).Two things worth a maintainer's call
Both are consequences of mirroring
Response, and I flag them rather than assume:RefResolverInterfacegains two methods. Breaking for any external implementor.RefResolveris the only implementor in the repo. Given the interface already carriesresolveParameter/resolveResponsepairs, theRequestBodypair is the consistent shape, and the bug can't be fixed without a way to resolve the reference.RequestBody's constructor gains three leading parameters, sorefis now first — exactly as inResponseandParameter. Breaking for positional construction; every call site insrc/andtests/uses named arguments, so nothing in-repo is affected. Appending the three instead would avoid the break at the cost of diverging from the other two models. Happy to switch if you'd rather not take the break before 1.0.