Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

[Validator] [WIP] Validate controller arguments using constraints #47425

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

Closed
wants to merge 13 commits into from
Closed

[Validator] [WIP] Validate controller arguments using constraints #47425

wants to merge 13 commits into from

Conversation

artyuum
Copy link
Contributor

@artyuum artyuum commented Aug 29, 2022

Q A
Branch? 6.2
Bug fix? no
New feature? yes
Deprecations? no
Tickets Fix #43958
License MIT
Doc PR todo

As discussed in #43958, the purpose of this PR is to add a new feature to the Validator component to allow the validation of controller arguments through constraints.

Example:

#[Route(name: 'all', methods: ['GET'])]
public function all(#[ControllerArgument([new PositiveOrZero])] int $offset = 0, #[ControllerArgument([new Positive])] int $limit = 20): Response
{
    ...
}

Todo

  • Allow constraints to be used on method parameters
  • Implement a listener to validate controller arguments using constraints
  • Write tests
  • Submit changes to the documentation
  • Update CHANGELOG.md

@carsonbot
Copy link

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has a contribution guide which I suggest you to read.

In short:

  • Always add tests
  • Keep backward compatibility (see https://symfony.com/bc).
  • Bug fixes must be submitted against the lowest maintained branch where they apply (see https://symfony.com/releases)
  • Features and deprecations must be submitted against the 6.2 branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

@artyuum artyuum changed the title [Validator] [WIP] Validate controller arguments using constraint [Validator] [WIP] Validate controller arguments using constraints Aug 29, 2022
@carsonbot
Copy link

Hey!

I think @alexandre-daubois has recently worked with this code. Maybe they can help review this?

Cheers!

Carsonbot

@WebMamba
Copy link
Contributor

Hi @artyuum I just try it out! and it's working great! thanks for your work! 😁

src/Symfony/Component/Validator/Constraints/All.php Outdated Show resolved Hide resolved
$violations = $this->validator->validate($value, $constraint);

if ($violations->count() > 0) {
throw new ValidationFailedException($value, $violations);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm throwing ValidationFailedException if the validation failed, but I'm not sure that this is the right exception to throw. Should I create a new one instead?

return [KernelEvents::CONTROLLER_ARGUMENTS => ['onKernelControllerArguments', 10]];
}

private function getReflectionMethod(callable $controller): \ReflectionMethod
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's how I'm extracting the controller class + method from the event.
If you have a better way I'll take it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use the new ControllerEvent::getControllerReflector() method

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method seems to be available only in ControllerEvent and I'm listening on ControllerArgumentsEvent 😕

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's worth it, it could be easy to expose:

class ControllerArgumentsEvent extends KernelEvent
{
    // ...
    public function getControllerReflector(): \ReflectionFunctionAbstract
    {
        return $this->controllerEvent->getControllerReflector();
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be useful, yes. Should I create another PR to make that change and merge it into the current PR or can I change that method scope in this PR?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nicolas-grekas nicolas-grekas modified the milestones: 6.2, 6.3 Nov 5, 2022
@hantsy
Copy link

hantsy commented Jan 3, 2023

#[Route(name: 'all', methods: ['GET'])]
public function all(#[ControllerArgument([new PositiveOrZero])] int $offset = 0, >#[ControllerArgument([new Positive])] int $limit = 20): Response
{
   ...
}

Why it is like this, I would like use the PositiveOrZero on the arguments directly.

#[Route(name: 'all', methods: ['GET'])]
public function all(#[PositiveOrZero] int $offset = 0, #[Positive] int $limit = 20): Response
{
    ...
}

Controller argument binding is another topic, such as

#[Route(path: '{id}', methods: ['GET'])]
public function getById(
#[PathParam(name="id", defaultValue="xxx")] int blogId, // example: the method argument is different from the path variables.
#[RequestParam(name="offset", defaultValue="0")] #[PositiveOrZero] int $offset = 0,
#[RequestParam(name="limit", defaultValue="50")] #[Positive] int $limit = 20): Response
{
    ...
}

@artyuum
Copy link
Contributor Author

artyuum commented Jan 8, 2023

@hantsy the reason has been discussed here #47425 (comment)

@hantsy
Copy link

hantsy commented Feb 4, 2023

@hantsy the reason has been discussed here #47425 (comment)

Sadly see this decision.

nicolas-grekas added a commit that referenced this pull request 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
@OskarStark
Copy link
Contributor

@artyuum
Copy link
Contributor Author

artyuum commented Apr 14, 2023

Yes, I don't think it's necessary to finish this PR.

@artyuum artyuum closed this Apr 14, 2023
@artyuum artyuum deleted the ft/validate-controller-arguments branch April 16, 2023 17:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Validator] Attribute can not be applied on Controller method argument
8 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.