Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[DoctrineBridge][WIP] Allow validating every class against unique entity constraint#24974
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| foreach ($fields as $fieldName) { | ||
| if (!$class->hasField($fieldName) && !$class->hasAssociation($fieldName)) { | ||
| throw new ConstraintDefinitionException(sprintf('The field "%s" is not mapped by Doctrine, so it cannot be validated for uniqueness.', $fieldName)); | ||
| foreach ($fields as $formField => $entityField) { |
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.
$sourceField?
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.
Why not
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.
$formField =>$objectField? because it is about the mapping field configuration between a DTO and the Entity.
| } | ||
| $fieldValue = $class->reflFields[$fieldName]->getValue($entity); | ||
| $fieldValue = $class->reflFields[is_int($formField) ? $entityField : $formField]->getValue($entity); |
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.
we need to consider#22592 (comment) here?
we use PropertyAccess component instead of ReflectionProperty to read the field value from current object (maintain the metadata verification and the DTO fields must be readable too),
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.
Yes indeed
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.
But that would add a dependency, as we just read fields maybe reflection is simpler.
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.
why not :P
| return; | ||
| } | ||
| if (1 === count($result)) { | ||
| if ($constraint->forUpdate) { |
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.
what about checking the corresponding source fields against the entity fields instead?
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.
I don't see how this could work?
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.
for each source field get the entity field value, compare entity field values against source field values. But im not sure thats waterproof; nor isforUpdate.
MatTheCatNov 15, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
If I want to update an entity from a DTO without changing a unique constrained field then this wouldn't work, that's why I let the decision to the developer.
dmaicher commentedNov 15, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
but there is a fundamental logic issue with the I have a user with email I mean we have to make sure the existing entity is actually my user. Only then its fine. Just counting does not really work. Or am I missing something? |
MatTheCat commentedNov 15, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@dmaicher nope you're right. Any idea welcomed! Maybe an |
Another idea:0b87dea |
| return; | ||
| } | ||
| if ($entity instanceof EntityDto && $entity->isForEntity($match)) { |
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.
Probably as constraint option should be "better", wdyt? something likeisEqualToMethod?
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.
Hmm maybe, this way would be more callback-constraint-like
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.
This approach will solve#22592 (comment)
| } | ||
| $fieldValue = $class->reflFields[$fieldName]->getValue($entity); | ||
| $field = new \ReflectionProperty($entityClass, is_int($formField) ? $entityField : $formField); |
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.
This seems wrong,$entityClass is not the class of the subject$entity always, should beget_class($entity) here, no?
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.
Yes indeed. Followingyour suggestion I'll rename$entity$object this will be clearer
| $reflMethod = new \ReflectionMethod($object, $method); | ||
| if ($reflMethod->isStatic() ? $reflMethod->invoke(null, $object, $match) : $reflMethod->invoke($object, $match)) { |
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.
Also we could pass here the current$em as argument too?
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.
Do you have an example showing how it could be used?
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.
Both objects can't be compared directly, right? So I guess inside this method we need to find somehow ($em) the current object to compare it.
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.
A DTO would include the entity identifier so it would not need the entity manager.
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.
not sure about dto's always including an entity id or so; in my experience dto + entity live decoupled.
Both objects can't be compared directly, right?
Dunno, depends on context i guess. So yes having$em might be needed 👍
Also calling this "ToUpdate" is opinionated. I would expect something likeentityComparator or so.
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.
How can an handler update an entity if its identifier is not part of the DTO?
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.
because i generate a new id / use the current id from url (or so) directly ;-)
handle(new Id(), $dto->email) for example.
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.
not sure what's the better practice though, now that i think of it. It's just a decision i made beforehand, i also dont dostatic Dto::fromEntity() and such.
| return; | ||
| } | ||
| $method = $constraint->isEntityToUpdateMethod; |
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.
We must add support to\Closure andarray callable too.
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.
This method is just for saying an entity is the one we're updating or not I don't think we need more complicated?
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.
This constraint can be used in PHP flat and maybe for advanced checking where I need some dependencies as service for instance.
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.
It should be acallable yes. I would not like to add this method to my dto. Defeats decoupling IMHO.
We can do the standard approach no? Check for callable, then for method on current context which we do elsewhere also believe.
| @@ -30,6 +30,7 @@ class UniqueEntity extends Constraint | |||
| public $em = null; | |||
| public $entityClass = null; | |||
| public $repositoryMethod = 'findBy'; | |||
| public $isEntityToUpdateMethod = null; | |||
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.
This method is not just about update event, on create it is called too. MaybeisEqualToEntityMethod? Basically it is what the validator asks to throw the violation or not.
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.
DTO can't be equal to an entity
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.
It would make no sense to call it if an entity is created: if an entity exists in database with fields of same value it cannot be the one which comes from the DTO as it isn't in the database yet.
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.
When we are creating a new entity through a DTO, we also need to call to the comparison method to check the equality.
Take the example below, on create$this->authorId isnull, on update$this->authorId is the author ID that is being modified:
/** * @UniqueEntity( * fields={"email"}, * entityClass="App\Entity\Author", * isEqualToMethod="isEqualTo" * ) */class AuthorDTO{private$authorId;private$email;// ...publicfunctionisEqualTo(Author$author) {return$author->getId() ===$this->authorId; }}
Results:
- On create it always return
false, so throws the violation. - On update it depends of the equality, if
truepass, else throws the violation.
The validator doesn't know nothing about the current event, so the method must be called always.
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.
I also think something likeisEqualToEntityMethod would make more sense.
| if (!$em) { | ||
| throw new ConstraintDefinitionException(sprintf('Unable to find the object manager associated with an entity of class "%s".',get_class($entity))); | ||
| throw new ConstraintDefinitionException(sprintf('Unable to find the object manager associated with an entity of class "%s".',$objectClass)); |
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.
this should use$entityClass instead of$objectClass
| @@ -121,22 +127,7 @@ public function validate($entity, Constraint $constraint) | |||
| return; | |||
| } | |||
| if (null !== $constraint->entityClass) { | |||
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.
I guess there was a reason for this?
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.
well.. given $entity now can be any $object, it doesnt make sense to validate the inheritance anymore. I dont think it hurts.
| * @expectedException \Symfony\Component\Validator\Exception\ConstraintDefinitionException | ||
| * @expectedExceptionMessage The "Symfony\Bridge\Doctrine\Tests\Fixtures\SingleStringIdEntity" entity repository does not support the "Symfony\Bridge\Doctrine\Tests\Fixtures\Person" entity. The entity should be an instance of or extend "Symfony\Bridge\Doctrine\Tests\Fixtures\SingleStringIdEntity". | ||
| */ | ||
| public function testInvalidateRepositoryForInheritance() |
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.
see my other comment: I think there was a reason for this:#15002
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.
we should add new tests for sure.
| @@ -30,6 +30,7 @@ class UniqueEntity extends Constraint | |||
| public $em = null; | |||
| public $entityClass = null; | |||
| public $repositoryMethod = 'findBy'; | |||
| public $isEntityToUpdateMethod = null; | |||
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.
I also think something likeisEqualToEntityMethod would make more sense.
| } | ||
| $method = $constraint->isEntityToUpdateMethod; | ||
| if (null !== $method) { |
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.
if (null !==$method =$constraint->isEntityToUpdateMethod) {
Why would I use this for a DTO? Why would I even use this for an Entity? This check is very sensitive to race conditions and makes it therefore redundant IMO. You still have to catch the duplicate key exception either and give the user a unique field error, why would to first do the check and then check via a catch statement? |
@iltar what would be the easiest way to map a duplicate exception to a form field? Anyways I find my proposal becoming too heavy to be a better alternative to just create specific constraints per DTO. |
ro0NL commentedNov 19, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
agree. I would settle with validating beforehand in case of e.g. a user form. In general the unique field exception would be caught anyway - resulting in a system error page or so. |
@MatTheCat Whenever you try to flush an entity that would trigger the database to violate the unique constraint, it will throw a unique constraint exception. You can catch this exception, add a form error and handle it gracefully. Even if you use a check for unique values, if 2 users submit the same value, it might return non-unique for both and then trigger the exception either way for 1 of the users as the database is the only point where this can be enforced. So you're either going to have to implement it twice, or accept the fact that you spew 500 errors to the user, just because you want the validation constraint on object. I understand that it's more logical to add it to the object, but it's also useless in this scenario if you have to rely on the database insertion. |
If I understand correctly this constraint seems quite useless whatever a form populates..! Optimally the controller would catch a UniqueConstraintViolationException and add an error to the form? Is there a way to get the concerned field? |
…ss against unique entity constraint (wkania)This PR was merged into the 7.1 branch.Discussion----------[DoctrineBridge][Validator] Allow validating every class against unique entity constraint| Q | A| ------------- | ---| Branch? | 7.x <!-- see below -->| Bug fix? | no| New feature? | yes <!-- pleasedate src/**/CHANGELOG.md files -->| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->| Tickets |Fix#22592| License | MIT| Doc PR |symfony/symfony-docs#14458 <!-- required for new features -->Yet another try to allow validating every class against unique entity constraint.Based on the knowledge from issue#22592 and pull request#24974.This constraint doesn’t provide any protection against race conditions, which is enough in most cases. You can always try-catch flush method. Let's not talk about this problem.Current state:- can be applied only to an entity,- support entity inheritance,- can validate unique combinations of multiple fields,- meaningful errors related to entities,- is valid during an update, when the only matched entity is the same as the entity being validatedNew state:- preserve the current state,- all old tests pass (no changes in them),- no breaking changes,- can be applied to any class (like DTO),- can map object fields to entity fields (optional),- when the object update some entity, there is an option to provide the identifier field names to match that entity (optional)Examples:1. DTO adds a new entity.```namespace App\Message;use Symfony\Bridge\Doctrine\Validator\Constraints\UniqueEntity;/** * `@UniqueEntity`(fields={"name"}, entityClass="App\Entity\User") */class HireAnEmployee{ public $name; public function __construct($name) { $this->name = $name; }}```2. DTO adds a new entity, but the name of the field in the entity is different.```namespace App\Message;use Symfony\Bridge\Doctrine\Validator\Constraints\UniqueEntity;/** * `@UniqueEntity`(fields={"name": "username"}, entityClass="App\Entity\User") */class HireAnEmployee{ public $name; public function __construct($name) { $this->name = $name; }}```3. DTO updates an entity.```namespace App\Message;use Symfony\Bridge\Doctrine\Validator\Constraints\UniqueEntity;/** * `@UniqueEntity`(fields={"name"}, entityClass="App\Entity\User", identifierFieldNames={"uid": "id"}) */class UpdateEmployeeProfile{ public $uid; public $name; public function __construct($uid, $name) { $this->uid = $uid; $this->name = $name; }}```Commits-------adb9afa [DoctrineBridge][Validator] Allow validating every class against unique entity constraint
…ss against unique entity constraint (wkania)This PR was merged into the 7.1 branch.Discussion----------[DoctrineBridge][Validator] Allow validating every class against unique entity constraint| Q | A| ------------- | ---| Branch? | 7.x <!-- see below -->| Bug fix? | no| New feature? | yes <!-- pleasedate src/**/CHANGELOG.md files -->| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->| Tickets | Fix #22592| License | MIT| Doc PR |symfony/symfony-docs#14458 <!-- required for new features -->Yet another try to allow validating every class against unique entity constraint.Based on the knowledge from issuesymfony/symfony#22592 and pull requestsymfony/symfony#24974.This constraint doesn’t provide any protection against race conditions, which is enough in most cases. You can always try-catch flush method. Let's not talk about this problem.Current state:- can be applied only to an entity,- support entity inheritance,- can validate unique combinations of multiple fields,- meaningful errors related to entities,- is valid during an update, when the only matched entity is the same as the entity being validatedNew state:- preserve the current state,- all old tests pass (no changes in them),- no breaking changes,- can be applied to any class (like DTO),- can map object fields to entity fields (optional),- when the object update some entity, there is an option to provide the identifier field names to match that entity (optional)Examples:1. DTO adds a new entity.```namespace App\Message;use Symfony\Bridge\Doctrine\Validator\Constraints\UniqueEntity;/** * `@UniqueEntity`(fields={"name"}, entityClass="App\Entity\User") */class HireAnEmployee{ public $name; public function __construct($name) { $this->name = $name; }}```2. DTO adds a new entity, but the name of the field in the entity is different.```namespace App\Message;use Symfony\Bridge\Doctrine\Validator\Constraints\UniqueEntity;/** * `@UniqueEntity`(fields={"name": "username"}, entityClass="App\Entity\User") */class HireAnEmployee{ public $name; public function __construct($name) { $this->name = $name; }}```3. DTO updates an entity.```namespace App\Message;use Symfony\Bridge\Doctrine\Validator\Constraints\UniqueEntity;/** * `@UniqueEntity`(fields={"name"}, entityClass="App\Entity\User", identifierFieldNames={"uid": "id"}) */class UpdateEmployeeProfile{ public $uid; public $name; public function __construct($uid, $name) { $this->uid = $uid; $this->name = $name; }}```Commits-------adb9afa4a5 [DoctrineBridge][Validator] Allow validating every class against unique entity constraint
Uh oh!
There was an error while loading.Please reload this page.
For now
UniqueEntityconstraint only works if the validated value is an entity, which prevent its usage on DTOs. This PR tries to make this constraint work on every class.entityClassis used to get metadata if not nullfieldscan be used to map class fields to entity fieldsforUpdateoption allows to tell a single matching entity does not constitues a violationWDYT?