Skip to content
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

[DoctrineBridge] UniqueEntity validator problem with entityClass #22592

Open
tabbi89 opened this issue Apr 30, 2017 · 17 comments

Comments

Projects
None yet
@tabbi89
Copy link

commented Apr 30, 2017

Q A
Bug report? yes/no
Feature request? no
BC Break report? yes/no
RFC? yes
Symfony version 3.2.0

I wonder how this should work for very common example using DTOs for forms. I have applied validation to my DTO

AppBundle\Command\Register:
    constraints:
            - Symfony\Bridge\Doctrine\Validator\Constraints\UniqueEntity:
                fields: email
                entityClass: AppBundle\Entity\User
                em: default

With this configuration I get error: "The class 'AppBundle\Command\Register' was not found in the chain configured namespaces AppBundle\Entity"

The flow currently works like this I set em for my entity: AppBundle\Entity\User but in validator we have getting current class metadata for main entity (in my example AppBundle\Command\Register this is not an entity) $em->getClassMetadata(get_class($entity)); so this is why it fails.

# Symfony\Bridge\Doctrine\Validator\Constraints\UniqueEntityValidator

        if ($constraint->em) {
            $em = $this->registry->getManager($constraint->em);

            if (!$em) {
                throw new ConstraintDefinitionException(sprintf('Object manager "%s" does not exist.', $constraint->em));
            }
        } else {
            $em = $this->registry->getManagerForClass(get_class($entity));

            if (!$em) {
                throw new ConstraintDefinitionException(sprintf('Unable to find the object manager associated with an entity of class "%s".', get_class($entity)));
            }
        }

        $class = $em->getClassMetadata(get_class($entity));
        /* @var $class \Doctrine\Common\Persistence\Mapping\ClassMetadata */

IMHO if we specify entityClass we should expect that everything will concern this entityClass. Another example of failure is when two entities: entity1 and entity2 belong to different entity managers. If this is an expected behaviour (IMHO it should work like this) we should at least add this information to documentation ?

@linaori

This comment has been minimized.

Copy link
Contributor

commented May 1, 2017

The UniqueEntity does not work on non-entity objects afaik. However, I think a proper solution would would be to just try and insert, then catch the unique constraint exception and add a form error:

try {
    $this->em->flush();
} catch (UniqueConstraintViolationException $exception) {
    $form->get('username')->addError(new FormError(
        $this->translator->trans('authentication.username.taken', [], 'validators')
    ));
}
@yceruto

This comment has been minimized.

Copy link
Contributor

commented May 1, 2017

Even when the purpose of the entityClass was to execute the query in a different repository (in some cases, such as when using Doctrine inheritance mapping), I wonder if we could expand its scope to apply UniqueEntity() for non-entity objects (being entityClass mandatory in these case).

The target of this constraint is validate that a particular field (or fields) in a Doctrine entity is (are) unique, so what if:

  • we use entityClass option (if set) instead of get_class($entity),
  • 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),
  • we remove the check about the repository being able to support the current object,

We would have DTO's with multiple UniqueEntity constraints so:

/**
 * @UniqueEntity(fields={"email"}, entityClass="AppBundle\Entity\User")
 * @UniqueEntity(fields={"phone"}, entityClass="AppBundle\Entity\Phone", em="custom")
 */
class RegisterDTO
{
    /** @var string */
    private $email;

    /** @var string */
    private $phone;
    
    //...
}

Is it something we want/can to support?

#14917 related

ping @ogizanagi, @HeahDude

@ogizanagi

This comment has been minimized.

Copy link
Member

commented May 1, 2017

@yceruto I considered something similar in the past; I wasn't sure about reusing the same constraint but now I'd say it makes sense. And the feature would be particularly useful for validating commands (command bus) for instance.

But we will also need a mapping between the DTOs fields and the targeted entity fields as their naming might slightly differ.

Foolish example:

/**
 * @UniqueEntity(fields={"userEmail": "email"}, entityClass="AppBundle\Entity\User")
 * @UniqueEntity(fields={"userPhone": "phoneNumber"}, entityClass="AppBundle\Entity\Phone", em="custom")
 */
class RegisterDTO
{
    /** @var string */
    private $userEmail;

    /** @var string */
    private $userPhone;
    
    //...
}
@xabbuh

This comment has been minimized.

Copy link
Member

commented May 1, 2017

That's probably not enough. You also need to consider a way to determine whether or not a possible result is the object currently being modified (the current implementation checks for the object being returned to be the same as the one being validated).

@linaori

This comment has been minimized.

Copy link
Contributor

commented May 2, 2017

Please also note that validating unique fields might still trigger the case I've listed before, so you'll need to add that either way. If so, you might just as well omit the validation imo, saves you a lot of work.

@HeahDude

This comment has been minimized.

Copy link
Member

commented Jul 30, 2017

While this is not properly supported in core, one may consider using custom constraints to do so (example https://github.com/EnMarche/en-marche.fr/blob/master/src/Validator/UniqueMembershipValidator.php).

@ostrolucky

This comment has been minimized.

Copy link
Contributor

commented Dec 17, 2017

Here's more generic solution we are using https://gist.github.com/ostrolucky/7b4a7133e51f6b5ffcb598a287acb5f7

Also, can we give this issue more appropriate title please?

@Xymanek

This comment has been minimized.

Copy link

commented Jan 21, 2018

Another possible solution: https://gist.github.com/Xymanek/369f468d02069090770a1e4626d9f1e9

I can make a PR if it looks good

@syther101

This comment has been minimized.

Copy link

commented Feb 27, 2018

@iltar I'm unsure how your proposed solution would work. Or maybe I'm confused. But at the point of calling $this->em->flush(); the form should be in a submitted and valid state.

How would you then add errors to a field of an already submitted form?

@linaori

This comment has been minimized.

Copy link
Contributor

commented Feb 27, 2018

While it may not display the errors in the WDT, the error will still be shown when you render the form.

But at the point of calling $this->em->flush(); the form should be in a submitted and valid state.

This is true. However, 1ms after the form was valid (it was unique at the moment of validation) and after that moment, someone else just flushed their entity with the exact same value, your flush will now fail with a UniqueConstraintException.

@syther101

This comment has been minimized.

Copy link

commented Feb 27, 2018

Interesting. You are correct. I was only calling {{ form_errors(form.field) }} for the form field itself and not also outputting form related errors.

In this case, I rather like the solution. Checking the entities uniqueness is pointless in most regards seeing as you'd have to catch the exception regardless. Many thanks, @iltar.

@stoccc

This comment has been minimized.

Copy link
Contributor

commented May 9, 2018

We should also consider that in some cases matches should be case-sensitive, while some others should be case-insensitive (i.e. uniqueness of e-mail addresses in a db).

@Xymanek

This comment has been minimized.

Copy link

commented May 10, 2018

the comparison (and case-sensitivity) is usually handled by the db engine

@webbertakken

This comment has been minimized.

Copy link

commented May 30, 2018

@ogizanagi Perhaps this could help?

https://gist.github.com/webbertakken/569409670bfc7c079e276f79260105ed

A working mapping like you suggested.

ogizanagi commented on May 1, 2017 •
@yceruto I considered something similar in the past; I wasn't sure about reusing the same constraint but now I'd say it makes sense. And the feature would be particularly useful for validating commands (command bus) for instance.

But we will also need a mapping between the DTOs fields and the targeted entity fields as their naming might slightly differ.
Foolish example:

/**
 * @UniqueEntity(fields={"userEmail": "email"}, entityClass="AppBundle\Entity\User")
 * @UniqueEntity(fields={"userPhone": "phoneNumber"}, entityClass="AppBundle\Entity\Phone", em="custom")
 */
class RegisterDTO
{
    /** @var string */
    private $userEmail;

    /** @var string */
    private $userPhone;
    
    //...
}
@NicoBoos

This comment has been minimized.

Copy link

commented Jul 6, 2018

I'm facing the same issue using DTO for forms' validation and it would be really nice to have this feature.

I just gave a try at @webbertakken and it's working for my use case.

Is someone already working on this feature taking care of @xabbuh note to make it implemented and released?

@danaki

This comment has been minimized.

Copy link

commented Sep 18, 2018

This breaks my DDD

@spackmat

This comment has been minimized.

Copy link

commented Mar 21, 2019

My workaround for now is a manual check inside a validate() callback inside my DTO (which has the existing User or null in its existingUser property and the corresponding UserRepository in its userRepository property:

/**
 * @Assert\Callback
 */
public function validate(ExecutionContextInterface $context)
{
    // not the best solution, see https://github.com/symfony/symfony/issues/22592 for details
    $usernameNotUniqueQb = $this->userRepository->getEntityRepository()->createQueryBuilder('u')
        ->select('COUNT(u.id)')
        ->where('u.username = :username')
        ->setParameter('username', $this->getUsername())
    ;
    if (null !== $this->existingUser) {
        $usernameNotUniqueQb->andWhere('u.id != :id')->setParameter('id', $this->existingUser->getId());
    }

    if ($usernameNotUniqueQb->getQuery()->getSingleScalarResult() > 0) {
        $context->buildViolation('entities.User.username.nonUniqueMessage')
            ->setTranslationDomain('messages')
            ->atPath('username')
            ->addViolation()
        ;
    }
}

And yes, this could be affected by race conditions, where the database will throw a non unique error when this check passes and in the meantime another request creates another entity with that username.

But @webbertakken s solution looks good though.

P.S. Btw. would it be cool, if the count()-method of a DoctrineRepository would accept a Criteria object (as the matching() method does) instead of just a simple array named $criteria, so the != comparison wouldn't enforce the whole querybuilder stuff here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.