Skip to content
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

[RFC] Injecting DTO into controller actions (DTOArgumentValueResolver) #36093

Closed
faizanakram99 opened this issue Mar 16, 2020 · 15 comments
Closed
Labels
Feature HttpKernel RFC RFC = Request For Comments (proposals about features that you want to be discussed) Stalled

Comments

@faizanakram99
Copy link
Contributor

faizanakram99 commented Mar 16, 2020

TLDR;
See gist linked in example .. implementation can change, it is just POC

Description
The idea is to have an ArgumentValueResolver which would convert request body (form data, json string and/or query string) to a DTO.

(Initially shared this idea in symfony slack channel earlier today but thought it might get lost in a few hours so figured it is best to open an issue here on github)

So for example a request with body "{ id: 12, name:' foo' }" is made to a route, we can simply type hint our controller action with a DTO say Person (containing $id and $name property). This argument resolver will automatically inject the Person DTO with $id and $name properties populated from request data.
In order to check whether DTO should be injected by argument resolver or not we can have an interface say RequestDTOInterface (see the example linked below)

Among many benefits, it would save us from writing a lot of boilerplate code especially with large payloads.

The nice thing about this RFC is, it doesn't add any new annotations and just re-uses what we already have in symfony framework.

Need a neat way to add it to "default argument value resolvers" , cannot add this in HttpKernel package as it depends upon Serializer and Validator components

Example
I made a gist with a sample implementation (and tests), it uses Symfony serializer (DenormalizerInterface) to convert request content to DTO and also validates it using Symfony validator

https://gist.github.com/faizanakram99/33c97a5f5b834dceeb90003574c7b98d

@faizanakram99 faizanakram99 changed the title [RFC] Request body to Arbitrary objects (DTO) argument value resolver [RFC] Request body to arbitrary objects (DTO) argument value resolver Mar 16, 2020
@faizanakram99 faizanakram99 changed the title [RFC] Request body to arbitrary objects (DTO) argument value resolver [RFC] Injecting DTO into controller actions (DTO argument value resolver) Mar 16, 2020
@tsantos84
Copy link
Contributor

Hi @faizanakram99, we're discussing exactly the same thing on #36037. Take a look at this comment.

@faizanakram99
Copy link
Contributor Author

Hi @tsantos84

Yep, I saw that RFC too.

While your RFC has a very large scope, this RFC merely adds an ArugmentValueResolver to convert request body into DTO without adding any extra annotations.

I'd suggest to checkout the gist in OP, it is a fairly simple implementation.

@faizanakram99 faizanakram99 changed the title [RFC] Injecting DTO into controller actions (DTO argument value resolver) [RFC] Injecting DTO into controller actions (DTOArgumentValueResolver) Mar 17, 2020
@tsantos84
Copy link
Contributor

Although I agree with some kind of automatic injection and validation, marking an application class with DTOInterface is a form of coupling I'd prefer to avoid. Another think to consider is that you're arbitrating the source of data (request, post, get). This could cause confusion for requests containing mix of sources (e.g POST /save?query1=1&query2=2).

I like the idea, but I think devs should have more control on what is happening.

@faizanakram99
Copy link
Contributor Author

Not sure I agree with the idea of implementing an Interface as "coupling", I am open to different implementations though (including annotations).

I do agree that requests with mix of sources can cause confusion, that is why this is an RFC and not an actual PR 😄

@zajca
Copy link

zajca commented Mar 22, 2020

This could be done with ParamConverter, I don't see any benefit.

@faizanakram99
Copy link
Contributor Author

@zajca ParamConverter is not in symfony core, it is part of framework-extra-bundle, ArgumentResolvers are however part of Symfony core.

And yes it can be done in both with very small amount of code, so wouldn't it make sense to have this argument value resolver (DTO) in default arg value resolvers ?

@versh23
Copy link

versh23 commented Mar 27, 2020

i agree with @zajca
similar https://github.com/FriendsOfSymfony/FOSRestBundle/blob/master/Request/RequestBodyParamConverter.php
RequestBodyParamConverter can inject ConstraintViolationListInterface - its good.
but in you solution ist always throw BadRequest

@javiereguiluz javiereguiluz added HttpKernel RFC RFC = Request For Comments (proposals about features that you want to be discussed) Feature labels Apr 9, 2020
@carsonbot
Copy link

Thank you for this suggestion.
There has not been a lot of activity here for a while. Would you still like to see this feature?

@carsonbot carsonbot added Stalled and removed Stalled labels Feb 19, 2021
@carsonbot
Copy link

Hello? This issue is about to be closed if nobody replies.

@faizanakram99
Copy link
Contributor Author

Yep, I'd like to have this but I think it's probably covered by some other issue

@carsonbot
Copy link

Thank you for this suggestion.
There has not been a lot of activity here for a while. Would you still like to see this feature?

@tsantos84
Copy link
Contributor

As commented by @fabpot here, this discussion was resumed through this PR, but I don't saw the feature implemented in the Symfony's core although.

@carsonbot carsonbot removed the Stalled label Nov 29, 2021
@carsonbot
Copy link

Thank you for this suggestion.
There has not been a lot of activity here for a while. Would you still like to see this feature?

@carsonbot
Copy link

Friendly reminder that this issue exists. If I don't hear anything I'll close this.

@carsonbot
Copy link

Hey,

I didn't hear anything so I'm going to close it. Feel free to comment if this is still relevant, I can always reopen!

nicolas-grekas added a commit that referenced this issue Apr 14, 2023
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature HttpKernel RFC RFC = Request For Comments (proposals about features that you want to be discussed) Stalled
Projects
None yet
Development

No branches or pull requests

6 participants