Movatterモバイル変換

[0]ホーム
URL:

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

 Sign up
Appearance settings

[Messenger] Added a middleware that validates messages#26648

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
sroze merged 1 commit intosymfony:masterfromNyholm:middleware-validator
Apr 3, 2018

Conversation

@Nyholm
Copy link
Member

@NyholmNyholm commentedMar 23, 2018•
edited
Loading

QA
Branch?master
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets
LicenseMIT
Doc PRcoming

This is inspired byFervo. It runs the validator over messages implementing theValidatedMessageInterface.

linaori and andreybolonin reacted with thumbs up emoji
<serviceid="messenger.middleware.validator"class="Symfony\Component\Messenger\Middleware\ValidatingMiddleware">
<argumenttype="service"id="validator" />

<tagname="message_bus_middleware"priority="8" />
Copy link
Contributor

Choose a reason for hiding this comment

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

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 ValidatingMiddlewareimplements MiddlewareInterface
Copy link
Contributor

Choose a reason for hiding this comment

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

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

}
}

$next($message);
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't forget thereturn here.

*/
class ValidatingMiddlewareimplements MiddlewareInterface
{
protected$validator;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why would this one beprotected? I'd close first if we can soprivate, nah?

@nicolas-grekasnicolas-grekas added this to the4.1 milestoneMar 23, 2018
@sroze
Copy link
Contributor

sroze commentedMar 24, 2018•
edited
Loading

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

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 reacted with thumbs up emoji

@Nyholm
Copy link
MemberAuthor

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

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

publicfunction__toString()
Copy link
Contributor

Choose a reason for hiding this comment

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

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

return$this->violatingMessage;
}

publicfunctiongetViolations()
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing return type?

protected$violations;
protected$violatingMessage;

publicfunction__construct($violatingMessage,ConstraintViolationListInterface$violations)
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing a@param object $violatingMessage docblock?

class ValidationMiddlewareimplements MiddlewareInterface
{
/**
* @var ValidatorInterface
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@Nyholm
Copy link
MemberAuthor

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());
Copy link
Contributor

Choose a reason for hiding this comment

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

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

as the__toString method is now removed?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Oh, Thanks

Copy link
Contributor

@srozesroze left a comment

Choose a reason for hiding this comment

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

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 atframework.messenger.validation would do it. Could you add that?


/**
* @param object $violatingMessage
* @param ConstraintViolationListInterface $violations
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need this one.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Okey. Thanks

@Nyholm
Copy link
MemberAuthor

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

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

sroze reacted with thumbs up emoji

@NyholmNyholmforce-pushed themiddleware-validator branch 2 times, most recently frombac60a9 toab6c055CompareMarch 27, 2018 11:02
@Nyholm
Copy link
MemberAuthor

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

->end()
->end()
->arrayNode('validation')
->canBeDisabled()
Copy link
Contributor

Choose a reason for hiding this comment

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

- ->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?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

class ValidationFailedExceptionextends \RuntimeExceptionimplements ExceptionInterface
{
protected$violations;
protected$violatingMessage;
Copy link
Member

Choose a reason for hiding this comment

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

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

ogizanagi and sroze reacted with thumbs up emoji
Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Thank you

*/
publicfunction__construct($violatingMessage,ConstraintViolationListInterface$violations)
{
$this->violatingMessage =$violatingMessage;
Copy link
Member

Choose a reason for hiding this comment

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

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 existingNoHandlerForMessageException doesn't expose it

Copy link
Contributor

@ogizanagiogizanagiMar 29, 2018•
edited
Loading

Choose a reason for hiding this comment

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

The difference is that theNoHandlerForMessageException 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. 👍

chalasr, sroze, and Nyholm reacted with thumbs up emoji
$this->violatingMessage =$violatingMessage;
$this->violations =$violations;

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

Choose a reason for hiding this comment

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

Missing dot at the end 😉

@Nyholm
Copy link
MemberAuthor

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

symfony-splitter pushed a commit to symfony/messenger that referenced this pull requestMar 30, 2018
…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     <!-- seehttps://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 requestMar 30, 2018
…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     <!-- seehttps://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
Copy link
Contributor

@srozesroze left a comment•
edited
Loading

Choose a reason for hiding this comment

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

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


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

Choose a reason for hiding this comment

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

- validator component+ Validator component

@NyholmNyholmforce-pushed themiddleware-validator branch from3111acc toa1f086fCompareApril 3, 2018 09:37
@Nyholm
Copy link
MemberAuthor

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

@sroze
Copy link
Contributor

sroze commentedApr 3, 2018•
edited
Loading

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

@fabpot
Copy link
Member

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

Copy link
Contributor

@srozesroze left a comment

Choose a reason for hiding this comment

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

@Nyholm can you squash and rebase?

@fabpot
Copy link
Member

@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
Copy link
Contributor

sroze commentedApr 3, 2018•
edited
Loading

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

@srozesrozeforce-pushed themiddleware-validator branch froma1f086f to43a5171CompareApril 3, 2018 20:18
@sroze
Copy link
Contributor

Thank you@Nyholm.

@srozesroze merged commit43a5171 intosymfony:masterApr 3, 2018
sroze added a commit that referenced this pull requestApr 3, 2018
… (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        | comingThis 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
Copy link
Contributor

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

@NyholmNyholm deleted the middleware-validator branchApril 4, 2018 09:29
@Nyholm
Copy link
MemberAuthor

Thank you for merging

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

Reviewers

@chalasrchalasrchalasr left review comments

+2 more reviewers

@ogizanagiogizanagiogizanagi approved these changes

@srozesrozesroze approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

4.1

Development

Successfully merging this pull request may close these issues.

7 participants

@Nyholm@sroze@fabpot@ogizanagi@chalasr@nicolas-grekas@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp