-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[HttpKernel] Add #[MapQueryParameter]
to map and validate individual query parameters to controller arguments
#49134
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[HttpKernel] Add #[MapQueryParameter]
to map and validate individual query parameters to controller arguments
#49134
Conversation
#[QueryParameter]
or #[RequestParameter]
attribute#[QueryParameter]
or #[RequestParameter]
attribute
427874e
to
878ddd5
Compare
A like the idea, deporting some basic checks and use type hint |
@carsonbot what's needed to get some reviews ? |
I guess this one is competing with a similar proposal #49138, right? |
I think this is a very different approach and not similar. This provides scalar primitives and the use case is to allow some query parameters in the controller action. While the other PR tries to deserialize request content into typed objects. |
@ruudk the other PR have two attributes. One for the Query and one for the Content. |
I see but it serializes to an object, while this PR serializes a query string parameter to a typed property, for example And for the RequestParameter is used the form data (equivalent to request->request->get). If the other PR can also support these cases then this is a obsolete indeed. But AFAIK the other works on request content (json/xml)? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like MapQueryParameter
but I don't see the use case for MapRequestParameter
.
...ny/Component/HttpKernel/Controller/ArgumentResolver/QueryOrRequestParameterValueResolver.php
Outdated
Show resolved
Hide resolved
...ny/Component/HttpKernel/Controller/ArgumentResolver/QueryOrRequestParameterValueResolver.php
Outdated
Show resolved
Hide resolved
...nt/HttpKernel/Tests/Controller/ArgumentResolver/QueryOrRequestParameterValueResolverTest.php
Outdated
Show resolved
Hide resolved
878ddd5
to
a57bf66
Compare
#[QueryParameter]
or #[RequestParameter]
attribute#[MapQueryParameter]
attribute
a57bf66
to
161d283
Compare
@nicolas-grekas Updated the PR. Renamed |
src/Symfony/Component/HttpKernel/Controller/ArgumentResolver/MapQueryParameterValueResolver.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/HttpKernel/Controller/ArgumentResolver/MapQueryParameterValueResolver.php
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like that yes, but with a try/catch to generate 404
This made me realize: what about adding support for simple filtering rules?
https://www.php.net/manual/en/filter.filters.validate.php
src/Symfony/Component/HttpKernel/Controller/ArgumentResolver/MapQueryParameterValueResolver.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/HttpKernel/Controller/ArgumentResolver/MapQueryParameterValueResolver.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/HttpKernel/Controller/ArgumentResolver/MapQueryParameterValueResolver.php
Outdated
Show resolved
Hide resolved
@nicolas-grekas @stof Thanks for your feedback. Applied the changes. I think it's way better now. |
I don't know if you've read my comment fully because you've picked exactly the case where I agree with you and ignored the rest of it. |
I read it, I just don't make any difference between both cases, because there aren't any, from an HTTP spec perspective. |
@ro0NL Looking at https://httpwg.org/specs/rfc9110.html#status.422 and the example given there, 422 indeed does not look like a good fix for a malformed URL. The spec clearly talks about the request content, not the resource URL. |
i stand corrected :) i wouldnt dare betting on 422 v 400 v 404 anymore from a spec POV, i can only imagine it helps API consumers it looks like we found an RFC that calls it "request content" explicitly 👍 |
im not really convinced adding the attribute without constraint validation adds much value then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note $request->query->get()
throws 400, here it's 404
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will it be possible to introduce a name converter afterwards? This seems very opinionated regarding query parameters naming and conventions (they're nonexistent anyways). Also, query[key]=value
doesn't seem to be tested yet.
src/Symfony/Component/HttpKernel/Controller/ArgumentResolver/QueryParameterValueResolver.php
Show resolved
Hide resolved
The names can already be remapped if needed, see argument $name of the attribute. Or did you mean something else?
This is not about validation but about the last step of the routing logic. Thus the 404 when the input doesn't match. |
…and `#[MapQueryString]` to map Request input to typed objects (Koc) This PR was merged into the 6.3 branch. Discussion ---------- [HttpKernel] Create Attributes `#[MapRequestPayload]` and `#[MapQueryString]` to map Request input to typed objects | Q | A | ------------- | --- | Branch? | 6.3 | Bug fix? | no | New feature? | yes | Deprecations? | no | Tickets | #36037, #36093, #45628, #47425, #49002, #49134 | License | MIT | Doc PR | TBD Yet another variation of how we can map raw Request data to typed objects with validation. We can even build OpenApi Specification based on this DTO classes using [NelmioApiDocBundle](https://github.com/nelmio/NelmioApiDocBundle). ## Usage Example 🔧 ### `#[MapRequestPayload]` ```php class PostProductReviewPayload { public function __construct( #[Assert\NotBlank] #[Assert\Length(min: 10, max: 500)] public readonly string $comment, #[Assert\GreaterThanOrEqual(1)] #[Assert\LessThanOrEqual(5)] public readonly int $rating, ) { } } class PostJsonApiController { public function __invoke( #[MapRequestPayload] PostProductReviewPayload $payload, ): Response { // $payload is validated and fully typed representation of the request body $request->getContent() // or $request->request->all() } } ``` ### `#[MapQueryString]` ```php class GetOrdersQuery { public function __construct( #[Assert\Valid] public readonly ?GetOrdersFilter $filter, #[Assert\LessThanOrEqual(500)] public readonly int $limit = 25, #[Assert\LessThanOrEqual(10_000)] public readonly int $offset = 0, ) { } } class GetOrdersFilter { public function __construct( #[Assert\Choice(['placed', 'shipped', 'delivered'])] public readonly ?string $status, public readonly ?float $total, ) { } } class GetApiController { public function __invoke( #[MapQueryString] GetOrdersQuery $query, ): Response { // $query is validated and fully typed representation of the query string $request->query->all() } } ``` ### Exception handling 💥 - Validation errors will result in an HTTP 422 response, accompanied by a serialized `ConstraintViolationList`. - Malformed data will be responded to with an HTTP 400 error. - Unsupported deserialization formats will be responded to with an HTTP 415 error. ## Comparison to another implementations 📑 Differences to #49002: - separate Attributes for explicit definition of the used source - no need to define which class use to map because Attributes already associated with typed argument - used ArgumentValueResolver mechanism instead of Subscribers - functional tests Differences to #49134: - it is possible to map whole query string to object, not per parameter - there is validation of the mapped object - supports both `$request->getContent()` and `$request->request->all()` mapping - functional tests Differences to #45628: - separate Attributes for explicit definition of the used source - supports `$request->request->all()` and `$request->query->all()` mapping - Exception handling opt-in - functional tests ## Bonus part 🎁 - Extracted `UnsupportedFormatException` which thrown when there is no decoder for a given format Commits ------- d987093 [HttpKernel] Create Attributes `#[MapRequestPayload]` and `#[MapQueryString]` to map Request input to typed objects
Can we close this PR? was merged right now. |
Why close? This PR is complementary, not "instead of". |
…g them with `#[MapQueryParameter]` attribute
#[MapQueryParameter]
to map query parameters to controller arguments#[MapQueryParameter]
to map and validate individual query parameters to controller arguments
fa71093
to
bd7c669
Compare
@nicolas-grekas Can we merge this PR? |
Thank you @ruudk. |
Just imho but #[QueryParameter] would be much readable because what else can this attribute can do except a map attribute to method property, "Map" prefix is just superfluous. I mean no else framework as i know uses Map prefix for such attributes and the attribute itself is also responsible for validation whatever... Like in sesnioextrabundle #[ParamConverter] is not #[MapParamConverter] just beacuse we map this attribute to property it's so clear. Also it's just looks so terrible in code with "Map" prefix. I understand you may say this will help with autocomplete so all such attributes will be grouped with "Map" but other frameworks are fine without map prefix as well as sensio extra bundle we are fine without prefixes cause there is just few attributes which devs are need to learn we are not so lazy))) |
…or custom http status code (zim32) This PR was merged into the 6.4 branch. Discussion ---------- [HttpKernel] RequestPayloadValueResolver Add support for custom http status code | Q | A | ------------- | --- | Branch? | 6.4 | Bug fix? | no | New feature? | yes | Deprecations? | no | Tickets | no | License | MIT | Doc PR | Will do if accepted Reading this discussion #49134 (comment) it looks like a good idea to have ability to customize http status code in case of a validation error. This MR does not create any BC's. **MapQueryString** and **MapRequestPayload** attributes now have additional parameter `validationFailedStatusCode`, which allows to specify which http status code will be used if validation fails. Commits ------- 4d118c0 [HttpKernel] RequestPayloadValueResolver Add support for custom http status code
If some length validation fails, why the response status code should be 404? |
We increased the PHPStan level from 8 to 9. This lead to some problems when working with query or request parameters.
For example:
Because Symfony types
Request::get()
asmixed
there is no type safety and you have to assert everything manually.We then learned that we shouldn't use
Request::get()
but use the explicit parameter bag like:This
ParameterBag
is used forrequest
andquery
. It contains interesting methods like:This has the benefit that now the returned value is of a correct type.
Why aren't we being explicit by requiring the parameters and their types in the controller action instead?
Luckily Symfony has a concept called ValueResolver.
It allows you to do dynamically alter what is injected into a controller action.
So in this PR, we introduces a new attribute:
#[MapQueryParameter]
that can be used in controller arguments.It allows you to define which parameters your controller is using and which type they should be. For example:
When requesting
/?ids[]=1&ids[]=2&firstName=Ruud&required=3&age=123
you'll get:It even supports variadic arguments like this:
When requesting
/?ids[]=111&ids[]=222
the$ids
argument will have an array with values ['111','222'].Unit testing the controller now also becomes a bit easier, as you only have to pass the required parameters instead of constructing the
Request
object.It's possible to use
FILTER_VALIDATE_*
consts for more precise type descriptions.For example, this ensures that
$ids
is an array of integers:And this declares a string that must match a regexp:
When a filter fails, a 404 is returned.