-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Validator] Define which collection keys should be checked for uniqueness #42403
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
Conversation
It looks like you unchecked the "Allow edits from maintainer" box. That is fine, but please note that if you have multiple commits, you'll need to squash your commits into one before this can be merged. Or, you can check the "Allow edits from maintainers" box and the maintainer can squash for you. Cheers! Carsonbot |
Hey! I think @lmasforne has recently worked with this code. Maybe they can help review this? Cheers! Carsonbot |
@wkania I just wanted to thank you for your great pull request description: you explained things well and showed some code examples, making the review much easier. Thanks! |
Would be very useful if this could work also with collection of DTOs, so I could provide property path where to look. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here are some superficial comments, thanks for the PR. Please rebase also.
src/Symfony/Component/Validator/Constraints/UniqueValidator.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Validator/Tests/Constraints/UniqueValidatorTest.php
Show resolved
Hide resolved
src/Symfony/Component/Validator/Tests/Constraints/UniqueValidatorTest.php
Show resolved
Hide resolved
You should check the normalizer option. |
Thank you @wkania. |
…cked for uniqueness (wkania) This PR was squashed before being merged into the 6.1 branch. Discussion ---------- [Validator] Define which collection keys should be checked for uniqueness | Q | A | ------------- | --- | Branch? | 5.4 | Bug fix? | no | New feature? | yes | Deprecations? | no | Tickets | Fix #9888 | License | MIT | Doc PR | symfony/symfony-docs#16713 Currently, the validator checks each element of the collection as a whole. We already have a custom normalizer (which is great), but it would be nice to be able to check for uniqueness certain [collection](https://symfony.com/doc/current/reference/constraints/Collection.html) keys. For example, some fields in the collection element can be identifiers. They should be unique within the collection, even when the rest of the element data are different. Current state: - validates that all the elements of the given collection are unique New state: - preserve the current state, - all old tests pass (no changes in them), - no breaking changes, - define which collection fields should be checked for uniqueness (optional), - fields are optional in each element of the collection. Use [collection constraints](https://symfony.com/doc/current/reference/constraints/Collection.html) if they are required Examples: 1. Basic example. Each translation of the same resource must be in a different language. ```php use Symfony\Component\Validator\Constraints as Assert; /** * @Assert\Count(min=1), * @Assert\Unique(fields={"language"}), * @Assert\Collection( * fields = { * "language" = { * @Assert\NotBlank, * @Assert\Length(min = 2, max = 2), * @Assert\Language * }, * "title" = { * @Assert\NotBlank, * @Assert\Length(max = 255) * }, * "description" = { * @Assert\NotBlank, * @Assert\Length(max = 255) * } * } * ) */ public array $translations = []; ``` 2. An example where Optional is recognizable. Items with the id are changed and without are new. ```php use Symfony\Component\Validator\Constraints as Assert; use Symfony\Component\Validator\Constraints\Optional; /** * @Assert\Unique(fields={"id"}), * @Assert\Collection( * fields = { * "id" = @Assert\Optional({ * @Assert\Uuid * }), * "name" = { * @Assert\NotBlank, * @Assert\Length(max = 255) * } * } * ) */ public array $items = []; ``` 3. An example with composite uniqueness ```php use Symfony\Component\Validator\Constraints as Assert; /** * @Assert\Unique(fields={"latitude", "longitude"}), * @Assert\Collection( * fields = { * "latitude" = { * @Assert\NotBlank * }, * "longitude" = { * @Assert\NotBlank * }, * "poi" = { * @Assert\Length(max = 255) * } * } * ) */ public array $coordinates = []; ``` Commits ------- 0e8f4ce [Validator] Define which collection keys should be checked for uniqueness
…ys (Brajk19) This PR was squashed before being merged into the 6.4 branch. Discussion ---------- [Validator] UniqueValidator - normalize before reducing keys | Q | A | ------------- | --- | Branch? | 6.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Issues | | License | MIT In #42403 checking for uniqueness of certain collection keys was enabled. Method `UniqueValidator::reduceElementKeys` removes all keys which are not specified. Problem is that this happens before normalization, which in my opinion is not great because that method accepts array argument and if i have some object (DTO), TypeError will be thrown. Example: ```php class ParentDTO { /** * `@var` ChildDTO[] */ #[Assert\Unique( normalizer: [ChildDTO::class, 'normalize'] fields: 'id' )] public array $children; } ``` ```php class ChildDTO { public string $id; public string $name; public static function normalize(self $obj): array { return [ 'id' => $obj->id, 'name' => $obj->name ]; } } ``` Because normalization will happen after `reduceElementKeys` this will be thrown: `TypeError: Symfony\Component\Validator\Constraints\UniqueValidator::reduceElementKeys(): Argument #2 ($element) must be of type array, ...\ChildDTO given, called in .../UniqueValidator.php on line 48` If `$element = $normalizer($element);` is executed before `reduceElementKeys` it would enable using Assert\Unique with array of objects when correctly normalized Commits ------- 77df90b [Validator] UniqueValidator - normalize before reducing keys
…ys (Brajk19) This PR was squashed before being merged into the 6.4 branch. Discussion ---------- [Validator] UniqueValidator - normalize before reducing keys | Q | A | ------------- | --- | Branch? | 6.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Issues | | License | MIT In symfony/symfony#42403 checking for uniqueness of certain collection keys was enabled. Method `UniqueValidator::reduceElementKeys` removes all keys which are not specified. Problem is that this happens before normalization, which in my opinion is not great because that method accepts array argument and if i have some object (DTO), TypeError will be thrown. Example: ```php class ParentDTO { /** * `@var` ChildDTO[] */ #[Assert\Unique( normalizer: [ChildDTO::class, 'normalize'] fields: 'id' )] public array $children; } ``` ```php class ChildDTO { public string $id; public string $name; public static function normalize(self $obj): array { return [ 'id' => $obj->id, 'name' => $obj->name ]; } } ``` Because normalization will happen after `reduceElementKeys` this will be thrown: `TypeError: Symfony\Component\Validator\Constraints\UniqueValidator::reduceElementKeys(): Argument #2 ($element) must be of type array, ...\ChildDTO given, called in .../UniqueValidator.php on line 48` If `$element = $normalizer($element);` is executed before `reduceElementKeys` it would enable using Assert\Unique with array of objects when correctly normalized Commits ------- 77df90b959 [Validator] UniqueValidator - normalize before reducing keys
Currently, the validator checks each element of the collection as a whole. We already have a custom normalizer (which is great), but it would be nice to be able to check for uniqueness certain collection keys.
For example, some fields in the collection element can be identifiers. They should be unique within the collection, even when the rest of the element data are different.
Current state:
New state:
Examples: