Respect Constraint Validation Groups When Describing Models

[chrisguitarguy]

Should close #1857

I added a configuration option to enable this as it seems like a fairly large BC break to just turn it on. A user's entire OpenAPI doc could change underneath them.

One thing that I"m not sure I handled correclty is the default constraint group. There's a special constant for it in the constraint class as well as some handling on it with __{isset,get,set}, but the docs also mention that the class's short name is the default group as well.

There's also some things around group sequences that are not dealt with here, but the @Model annotation does not support them either.

This also fixes a deprecated call to getCollectionKeyType on Symfony 5.3+.

[GuilhemN]

Sorry it took me quite a while to leave a comment, I was not sure what to do as this is a fairly large change.

I wonder if it would not be safer to instead add a separate validationGroups property to the @Model annotation, and let users willing to create an annotation inheriting from @Model that would set validationGroups to the same value as groups, what do you think?

[chrisguitarguy]

I wonder if it would not be safer to instead add a separate validationGroups property to the @model annotation, and let users willing to create an annotation inheriting from @model that would set validationGroups to the same value as groups, what do you think?

I honestly don't know enough about how the combo of serializer + validator is used in the wider community to give a good answer to that. I can tell you that mine are always the same. Eg I have properties in a create group and validation constraints in the same create group, then I have an alias in the bundle configuraiton so the model name is Create{Thing}.

If validationGroups is the way you want to go, though, I'd say generated model names and model alias in bundle configuration need to account for that extra property. Because now the combo of serialization and validation groups will define what's in a model.

[chrisguitarguy]

I was not sure what to do as this is a fairly large change.

The added config option makes this disabled by default, at least, so it's not changing everything out from under users in one go.

[GuilhemN]

I fixed the conflicts :)

Actually I don't have a strong opinion here, I guess if this disabled by default this should be fine. But then I guess we should insist in the docs that this exists, shouldn't we?
So feel free to merge if you believe this should be in the bundle ;)

[chrisguitarguy]

But then I guess we should insist in the docs that this exists, shouldn't we?

Yep, I need to write something up on it. I don't disagree with you on the validation_groups thing though, need to think on it.

[FilipBenco]

We've just stumbled upon this problem as well. It sort of blocks us in development at this point. This MR looks pretty good to me and solves the issues, when Generated OpenAPI has model restrictions, that should not be present for given endpoint, based on those groups.

I also like, that you can enable / disable this feature, to preserve BC.

Regarding validation_groups, I would just mention in the docs, that validation group has to be the same as serialization group and then we can see, whether there are any issues with this constraint or not.

WDYT @GuilhemN @chrisguitarguy ?

[jkatruska]

Hi @chrisguitarguy I slightly edited your work in my fork, specifically isConstraintInGroup. Since groups in Constraint doesn't have type it can be null and then array_intersect was failing for me, so i added simple check.

Other issue is that if I don't have any groups specified in validation constraint they should run for every group regardless of deserializationContext so I added that as well.

Feel free to review my changes, but otherwise great work thanks 👍

[chrisguitarguy]

Other issue is that if I don't have any groups specified in validation constraint they should run for every group regardless of deserializationContext so I added that as well.

I'm not sure that's true.

When groups are not passed, it falls back to the default group in the validator.

• https://github.com/symfony/symfony/blob/8d028be3c14bb4d6bd88b39402991ba3bbd8004a/src/Symfony/Component/Validator/Validator/RecursiveContextualValidator.php#L62
• https://github.com/symfony/symfony/blob/8d028be3c14bb4d6bd88b39402991ba3bbd8004a/src/Symfony/Component/Validator/Validator/RecursiveContextualValidator.php#L83

Which is why empty groups default to the default group -- granted this is not great since default groups can depend on the validator context.

Since groups in Constraint doesn't have type it can be null and then array_intersect was failing for me, so i added simple check.

So someone set groups like this? groups={null} in an annotation/attribute or something? Or the groups attribute itself was null?

Symfony has some stuff in constraints that should prevent groups from being null:

• 4.4: https://github.com/symfony/symfony/blob/129a2710e6c884bdbed77eb18760904455633ce7/src/Symfony/Component/Validator/Constraint.php#L175-L210
• 5.4: https://github.com/symfony/symfony/blob/8d028be3c14bb4d6bd88b39402991ba3bbd8004a/src/Symfony/Component/Validator/Constraint.php#L190-L221
• 6.X: https://github.com/symfony/symfony/blob/acef571dc6014328373db0367ba5b0a363a21f7a/src/Symfony/Component/Validator/Constraint.php#L197-L226

[chrisguitarguy]

@GuilhemN I updated the docs here, but I'm not sure how I can verify that they look okay. Seems like the Symfony.com docs have some of their RST extension stuff.

[jkatruska]

I think there has been a little misunderstanding let me explain on example.
I have simple DTO like this

final class CreateCard
{
    /**
     * @Assert\NotNull
     * @Assert\NotBlank
     * @Groups({"api", "detail"})
     * @OA\Property(example="123456")
     */
    public int $userId;

    /**
     * @Assert\NotNull
     * @Assert\NotBlank
     * @Groups({"api", "detail"})
     * @OA\Property(example="nets")
     */
    public PaymentGateway $paymentGateway;

    /**
     * @Assert\NotNull
     * @Assert\NotBlank
     * @Groups({"api"})
     * @OA\Property(example="Visa")
     */
    public CardIssuer $issuer;

and my route is defined like this:

     /**
     * Card should be created for a user through the purchase process (PaymentController::purchase).
     * Saves an active credit card for the migrated user. Used only for user migration purposes.
     *
     * @Route("", methods={"POST"})
     * @OA\Response(response=201, description="Active card created")
     * @ParamConverter("createCard", options={"deserializationContext"={"groups"={"api"}}})
     */
    public function createCard(CreateCard $createCard, ConstraintViolationList $validationErrors): Card
    {
        if ($validationErrors->count() > 0) {
            throw new ValidationHttpException($validationErrors);
    }

as you can see my asserts don't have any groups specified which means validation on them is running for every deserialization group which is symfony doing correctly since I don't have explicit validationContext groups defined. But your current solution is not accounting for those asserts and therefore they are not generated in documentation as required properties.

Symfony has some stuff in constraints that should prevent groups from being null:

I thought so too, but as you can see property groups is public in Constraint class therefore that magic getter is never called, since you are accessing that public property directly. And I hit that error thats why I added that check, but thats mistake on symfony part.

[chrisguitarguy]

as you can see my asserts don't have any groups specified which means validation on them is running for every deserialization group which is symfony doing correctly since I don't have explicit validationContext groups defined. But your current solution is not accounting for those asserts and therefore they are not generated in documentation as required properties.

Are you sure your constraints are running because wahtever handles @ParamConverter doesn't pass the deserializationContent groups to the validator? So you've got one set of groups for the serializer and another for the validator (the default group for the validator in this case).

That's just a guess though, you'd have to look at whatever deals with @ParamConverter (got a link for this? I can't find anything in the sensio framework extra bundle wehre I thought this was). But I suspect the above is happening.

[chrisguitarguy]

Ah the handler for that is in FOSRestBundle and I was right:

Serializer: https://github.com/FriendsOfSymfony/FOSRestBundle/blob/54f5ffec4bff71b727a2aa4877915ad81358defc/Request/RequestBodyParamConverter.php#L72-L97

Validator:

• https://github.com/FriendsOfSymfony/FOSRestBundle/blob/54f5ffec4bff71b727a2aa4877915ad81358defc/Request/RequestBodyParamConverter.php#L102-L104
• https://github.com/FriendsOfSymfony/FOSRestBundle/blob/54f5ffec4bff71b727a2aa4877915ad81358defc/Request/RequestBodyParamConverter.php#L149-L159

[GuilhemN]

About the docs, I added #1997 which should now lint the docs so that they compile on symfony.com (it comes from part of the CI used by symfony/symfony-docs).
I must admit that before that, compilation of the docs was mosly luck 👀

To use this action, rebasing your pull request on master should do.

[chrisguitarguy]

Looks like it worked @GuilhemN !!

[jkatruska]

@chrisguitarguy ye you are right, I've got little confused there. So when can you see this merged? For now we are just going to use a fork, but we would like to use proper package versioning.

[GuilhemN]

It's all good for me. @chrisguitarguy is this finalized for you?

[chrisguitarguy]

Yep! I'll merge it.