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

[Messenger] Added a middleware that validates messages #26648

Merged
merged 1 commit into from Apr 3, 2018

Conversation

Projects
None yet
7 participants
@Nyholm
Member

Nyholm commented Mar 23, 2018

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR coming

This is inspired by Fervo. It runs the validator over messages implementing the ValidatedMessageInterface.

<service id="messenger.middleware.validator" class="Symfony\Component\Messenger\Middleware\ValidatingMiddleware">
<argument type="service" id="validator" />
<tag name="message_bus_middleware" priority="8" />

This comment has been minimized.

@sroze

sroze Mar 23, 2018

Member

Any reasoning for such priority? I'd argue it should be higher by default as it would be one of the most important "first steps".

* @author Magnus Nordlander <magnus@fervo.se>
* @author Tobias Nyholm <tobias.nyholm@gmail.com>
*/
class ValidatingMiddleware implements MiddlewareInterface

This comment has been minimized.

@sroze

sroze Mar 23, 2018

Member

ValidationMiddleware feels better to me and is more coherent with the other middleware names 🤔 (except LoggingMiddleware 🙊)

}
}
$next($message);

This comment has been minimized.

@sroze

sroze Mar 23, 2018

Member

Don't forget the return here.

*/
class ValidatingMiddleware implements MiddlewareInterface
{
protected $validator;

This comment has been minimized.

@sroze

sroze Mar 23, 2018

Member

Why would this one be protected? I'd close first if we can so private, nah?

@nicolas-grekas nicolas-grekas added this to the 4.1 milestone Mar 23, 2018

@chalasr chalasr added the Messenger label Mar 23, 2018

@sroze

This comment has been minimized.

Member

sroze commented Mar 24, 2018

The only remaining question is "Should this validation be enabled by default" (i.e. shouldn't we drop the ValidatedMessageInterface).

To me, it sounds like we should enable it by default because:

  1. It's a common practice to validate messages before sending them to a bus
  2. There is no real "risk" as it would require the developers to add the @Valid annotation (or similar configuration) on the message's fields to have entities validated
  3. We can add an opt-in/opt-out configuration later if it's too much of a problem.

What do you think? /cc @symfony/deciders

@Nyholm

This comment has been minimized.

Member

Nyholm commented Mar 26, 2018

I've squashed my commits and added validation by default. Validating a message without validation metadata will always pass.

@sroze

sroze approved these changes Mar 26, 2018

parent::__construct($this->__toString());
}
public function __toString()

This comment has been minimized.

@ogizanagi

ogizanagi Mar 26, 2018

Member

AFAIK, we never defined __toString method on exceptions in the core. Not sure this is desired now.

return $this->violatingMessage;
}
public function getViolations()

This comment has been minimized.

@ogizanagi

ogizanagi Mar 26, 2018

Member

Missing return type?

protected $violations;
protected $violatingMessage;
public function __construct($violatingMessage, ConstraintViolationListInterface $violations)

This comment has been minimized.

@ogizanagi

ogizanagi Mar 26, 2018

Member

Missing a @param object $violatingMessage docblock?

class ValidationMiddleware implements MiddlewareInterface
{
/**
* @var ValidatorInterface

This comment has been minimized.

@ogizanagi

ogizanagi Mar 26, 2018

Member

Can be removed as there is already a typehint on the constructor

@Nyholm

This comment has been minimized.

Member

Nyholm commented Mar 27, 2018

Thank you for the review. I've updated accordingly. I've also rebased the PR on master.

$this->violatingMessage = $violatingMessage;
$this->violations = $violations;
parent::__construct($this->__toString());

This comment has been minimized.

@ogizanagi

ogizanagi Mar 27, 2018

Member
- parent::__construct($this->__toString());
+ parent::__construct(sprintf("Message of type %s failed validation", get_class($this->violatingMessage));

as the __toString method is now removed?

This comment has been minimized.

@Nyholm

Nyholm Mar 27, 2018

Member

Oh, Thanks

@sroze

Almost there, thanks for the work.

I slept on it and I believe we need an opt-out. It doesn't need to be complicated: a boolean at framework.messenger.validation would do it. Could you add that?

/**
* @param object $violatingMessage
* @param ConstraintViolationListInterface $violations

This comment has been minimized.

@sroze

sroze Mar 27, 2018

Member

We don't need this one.

This comment has been minimized.

@Nyholm

Nyholm Mar 27, 2018

Member

Okey. Thanks

@Nyholm

This comment has been minimized.

Member

Nyholm commented Mar 27, 2018

I agree. But there will be plenty of merge conflicts.

I will add "opt-out" config once #26647 is merged.

@Nyholm

This comment has been minimized.

Member

Nyholm commented Mar 27, 2018

I've rebased my PR and added a way to opt-out from message validation

->end()
->end()
->arrayNode('validation')
->canBeDisabled()

This comment has been minimized.

@ogizanagi

ogizanagi Mar 27, 2018

Member
- ->canBeDisabled()
+ ->{!class_exists(FullStack::class) && class_exists(Validation::class) ? 'canBeDisabled' : 'canBeEnabled'}()

so it does not appear as enabled when the validator component isn't installed?

This comment has been minimized.

@Nyholm

Nyholm Mar 27, 2018

Member

Excellent idea. I was looking for something like this. And if someone tries to use validation without the validation component I'll throw an exception.

This comment has been minimized.

@ogizanagi

ogizanagi Mar 27, 2018

Member

Indeed, we use to throw such exceptions from the FrameworkExtension :)

class ValidationFailedException extends \RuntimeException implements ExceptionInterface
{
protected $violations;
protected $violatingMessage;

This comment has been minimized.

@chalasr

chalasr Mar 29, 2018

Member

if no inheritance use case for these properties in core then they should be private

This comment has been minimized.

@Nyholm

Nyholm Mar 29, 2018

Member

Thank you

*/
public function __construct($violatingMessage, ConstraintViolationListInterface $violations)
{
$this->violatingMessage = $violatingMessage;

This comment has been minimized.

@chalasr

chalasr Mar 29, 2018

Member

Is it really needed? I'm used to handle one command at a time so I always have the message at hand, am I missing a use case? The existing NoHandlerForMessageException doesn't expose it

This comment has been minimized.

@ogizanagi

ogizanagi Mar 29, 2018

Member

The difference is that the NoHandlerForMessageException is a logic exception (or at least, should be to me), it is meant to fail badly. No need to access the exception programmatically and retrieve the issuing message.

ValidationFailedException is a runtime exception, meant to be caught and handled/transformed to a better representation (e.g: an API problem response). It can also be used in a profiler panel or whatever :)

Anyway, even if we often handle one message at the time, use-cases with multiple messages do exist. We should cover them. 👍

$this->violatingMessage = $violatingMessage;
$this->violations = $violations;
parent::__construct(sprintf('Message of type "%s" failed validation', get_class($this->violatingMessage)));

This comment has been minimized.

@sroze

sroze Mar 29, 2018

Member

Missing dot at the end 😉

@Nyholm

This comment has been minimized.

Member

Nyholm commented Mar 29, 2018

Thank you for the reviews. I've updated the PR accordingly

symfony-splitter pushed a commit to symfony/messenger that referenced this pull request Mar 30, 2018

minor #26706 [Messenger] Make NoHandlerForMessageException a logic ex…
…ception (ogizanagi)

This PR was merged into the 4.1-dev branch.

Discussion
----------

[Messenger] Make NoHandlerForMessageException a logic exception

| Q             | A
| ------------- | ---
| Branch?       | master <!-- see below -->
| Bug fix?      | no
| New feature?  | no <!-- don't forget to update src/**/CHANGELOG.md files -->
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | symfony/symfony#26648 (comment)   <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | N/A <!-- required for new features -->

To me, a missing handler for a message is a program logic exception (or misconfiguration). Even if it's only detected at runtime here.
It's the same as `ServiceNotFoundException` which even is an `\InvalidArgumentException`. Could eventually also be the case here.

Commits
-------

0489bbd948 [Messenger] Make NoHandlerForMessageException a logic exception

fabpot added a commit that referenced this pull request Mar 30, 2018

minor #26706 [Messenger] Make NoHandlerForMessageException a logic ex…
…ception (ogizanagi)

This PR was merged into the 4.1-dev branch.

Discussion
----------

[Messenger] Make NoHandlerForMessageException a logic exception

| Q             | A
| ------------- | ---
| Branch?       | master <!-- see below -->
| Bug fix?      | no
| New feature?  | no <!-- don't forget to update src/**/CHANGELOG.md files -->
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | #26648 (comment)   <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | N/A <!-- required for new features -->

To me, a missing handler for a message is a program logic exception (or misconfiguration). Even if it's only detected at runtime here.
It's the same as `ServiceNotFoundException` which even is an `\InvalidArgumentException`. Could eventually also be the case here.

Commits
-------

0489bbd [Messenger] Make NoHandlerForMessageException a logic exception
@sroze

sroze requested changes Mar 30, 2018 edited

Can you add some tests? That it adds (and not if disabled) the middleware.

if ($config['middlewares']['validation']['enabled']) {
if (!$container->has('validator')) {
throw new LogicException('The Validation middleware is only available when the validator component is installed and enabled. Try running "composer require symfony/validator".');

This comment has been minimized.

@sroze

sroze Mar 30, 2018

Member
- validator component
+ Validator component
@Nyholm

This comment has been minimized.

Member

Nyholm commented Apr 3, 2018

I've fixed the typo and added tests. I've also rebased on master. The Travis tests failure seams unrelated.

@sroze

This comment has been minimized.

Member

sroze commented Apr 3, 2018

@fabpot as you disagreed with the Doctrine one... WDYT about this one? 🤔

@fabpot

This comment has been minimized.

Member

fabpot commented Apr 3, 2018

This one is fine as this is about integrating various Symfony components.

@sroze

sroze approved these changes Apr 3, 2018

@Nyholm can you squash and rebase?

@fabpot

This comment has been minimized.

Member

fabpot commented Apr 3, 2018

@sroze no need to rebase (as there are no merge commits) and no need to rebase (as there are no conflicts). Our gh tools does that automatically :)

@sroze

This comment has been minimized.

Member

sroze commented Apr 3, 2018

@fabpot that rebase request was meant so that we would have green ticks when I gh merge it :)
But yeah, they aren't related to I guess I can... 💣.

@sroze

This comment has been minimized.

Member

sroze commented Apr 3, 2018

Thank you @Nyholm.

@sroze sroze merged commit 43a5171 into symfony:master Apr 3, 2018

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details

sroze added a commit that referenced this pull request Apr 3, 2018

feature #26648 [Messenger] Added a middleware that validates messages…
… (Nyholm)

This PR was squashed before being merged into the 4.1-dev branch (closes #26648).

Discussion
----------

[Messenger] Added a middleware that validates messages

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets |
| License       | MIT
| Doc PR        | coming

This is inspired by [Fervo](https://github.com/fervo/ValidatedMessage).  It runs the validator over messages implementing the `ValidatedMessageInterface`.

Commits
-------

43a5171 [Messenger] Added a middleware that validates messages
@sroze

This comment has been minimized.

Member

sroze commented Apr 3, 2018

One of the failing tests was actually related. Fixed in #26782.

@Nyholm Nyholm deleted the Nyholm:middleware-validator branch Apr 4, 2018

@Nyholm

This comment has been minimized.

Member

Nyholm commented Apr 4, 2018

Thank you for merging

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment