fix: Merge Path Item level parameters into the operation before request validation - #62
Open
shadowhand wants to merge 1 commit into
Open
fix: Merge Path Item level parameters into the operation before request validation#62shadowhand wants to merge 1 commit into
shadowhand wants to merge 1 commit into
Conversation
…st validation `PathItem.parameters` were parsed into the schema model but dropped before validation: `RequestValidator` builds its parameter set from the `Operation` alone, so `required` and `schema` constraints on parameters hoisted to the path item were silently unenforced for every location. `PathItemParameterMerger` merges the path item's parameters into the operation's set where the operation is resolved from the path item, so `$operation->parameters` is complete by the time it reaches request validation. Per the specification, an operation level parameter overrides a path level one on matching `name` + `in`. Applied at all three resolution sites: `PathFinder`, `WebhookValidator`, and `CallbackValidator`. Fixes duyler#61
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 #61.
The bug
PathItem.parametersare parsed into the schema model and then dropped before validation.RequestValidator::validate()receives only theOperationand builds$parameterSchemasfrom$operation->parametersalone, so parameters declared on the Path Item — which the specification defines as applying to every operation under that path — are never enforced.required: truepasses when omitted, andschemaconstraints (enum,format, …) are never checked, for every location (path,query,header,cookie). Nothing errors, so a spec author gets no signal that a whole class of parameters is unvalidated.The fix
Validator\Internal\PathItemParameterMergermerges the path item's parameters into the operation's set at the point where the operation is resolved from the path item, so$operation->parametersis already complete by the time it reaches request validation —RequestValidatoris unchanged. Per the specification, an operation level parameter overrides the path level one on matchingname+in; the merger returns the operation unchanged when the path item declares no parameters, so specs that do not use the feature pay nothing.Applied at all three sites that resolve an
Operationfrom aPathItem:PathFinder::getOperation()— request validation, includingadditionalOperationsWebhookValidator::extractOperation()— webhooks are Path Item objects and had the identical defectCallbackValidator::extractOperation()— likewise, after$refresolutionResponseValidationHandleralso resolves an operation from a path item, but response validation does not read parameters, so it is left alone.Tests
Written test-first; each was watched failing against the unfixed source before the implementation landed.
tests/Functional/Request/PathItemParametersTest.php(8 tests) — the three violations from the issue (required query parameter omitted, queryenum, pathformat: uuid), a valid request still accepted, path item parameters applying to a second operation on the same path, and the override rule in both directions.tests/Integration/Validator/PathItemParametersMergeTest.php(4 tests) — the same enforcement for webhooks (in: header) and callbacks (in: query).Two checks on the tests themselves:
PathFinder.phpand re-running turns 4 of the 8 functional tests red, so they do target the reported defect rather than existing behaviour.name+inoverride skip makesoperation_level_parameter_overrides_path_item_level_parameter_with_same_name_and_infail — the path levelenum: [alpha, beta]rejects a request the operation levelenum: [gamma]allows — so that test genuinely pins the override rule and not just the merge.The reproduction script from the issue now prints
REJECTEDfor all three cases.Verification
vendor/bin/phpunit— 7143 tests, 14734 assertions, OK. The 2 deprecation notices are pre-existing and unrelated to this change.vendor/bin/psalm— no errors.vendor/bin/php-cs-fixer fix --dry-run— 0 of 880 files to fix.vendor/bin/rector process --dry-run— clean.CHANGELOG entry added under a new
## [Unreleased]/### Fixedheading.