Skip to content

[DoctrineBridge] UniqueEntityValidator strictly compares ID fields that are objects (like UUID) #58883

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

Closed
zavarock opened this issue Nov 14, 2024 · 5 comments

Comments

@zavarock
Copy link

zavarock commented Nov 14, 2024

Symfony version(s) affected

>=7.1

Description

I'm unsure if the approach taken by the UniqueEntityValidator is appropriate for all situations. Typically, we use the entity object to make changes with a FormType, which usually works well. However, in the case of DTOs, this can be a problem.

if ($fieldValues[$identifierFieldName] !== $propertyValue) {

How to reproduce

  • Make an entity DTO with a UniqueEntity constraint and an ID property which must be a UUID (or any other type of object)
  • Validate this entity DTO with parameters of an existent entity and you should see the problem

DTO sample:

use App\Entity\User;
use Symfony\Bridge\Doctrine\Validator\Constraints\UniqueEntity;
use Symfony\Component\Uid\Uuid;
use Symfony\Component\Validator\Constraints as Assert;

#[UniqueEntity(
    fields: ['name'],
    entityClass: User::class,
    identifierFieldNames: ['id'],
)]
class UserDto
{
    public ?Uuid $id = null;

    #[Assert\NotBlank]
    public ?string $name = null;
}

Possible Solution

Maybe this is an edge case, but I wonder if it would be better to check if the field is an object and, if yes, make a loose one (!=) instead of (!==).

Additional Context

No response

@zavarock zavarock added the Bug label Nov 14, 2024
@zavarock zavarock changed the title [DoctrineBridge] [Validator] UniqueEntityValidator is strictly comparing ID fields that are objects (like UUID) [DoctrineBridge] UniqueEntityValidator strictly compares ID fields that are objects (like UUID) Nov 14, 2024
@GiuseppeArcuti
Copy link
Contributor

Hello zavarock,

I've tried to reproduce your issue but was not able to (see #59126)
I had to slightly change your entity to make doctrine happy. Could you advise on how to reproduce?

@zavarock
Copy link
Author

zavarock commented Dec 7, 2024

Hi @GiuseppeArcuti.
I created a demonstration using the Symfony Demo: https://github.com/zavarock/symfony-unique-entity-bug

Steps

It will throw a ValidationFailedException: This value is already used.
As I see, if I'm using the identifierFieldNames in the DTO, it shouldn't throw this exception.

@GiuseppeArcuti
Copy link
Contributor

Hi @zavarock

If I understand correctly what you actually want to do is add a ignoreProperties: ['id'] on the ChangeBugLabelDto, because you are trying to edit an existing entity

@zavarock
Copy link
Author

@GiuseppeArcuti

Let me try to explain better...
Considering the following scenario below...

  • We have a Person entity object using UUID as an identifier and its fullName must be unique:
namespace App\Entity;

use App\Repository\PersonRepository;
use Doctrine\ORM\Mapping as ORM;
use Symfony\Bridge\Doctrine\Types\UuidType;
use Symfony\Component\Uid\Uuid;

#[ORM\Entity(repositoryClass: PersonRepository::class)]
class Person
{
    #[ORM\Id]
    #[ORM\Column(type: UuidType::NAME, unique: true)]
    #[ORM\GeneratedValue(strategy: 'CUSTOM')]
    #[ORM\CustomIdGenerator(class: 'doctrine.uuid_generator')]
    private ?Uuid $id = null;

    #[ORM\Column(length: 255, unique: true)]
    private ?string $fullName = null;

    #[ORM\Column(length: 255, nullable: true)]
    private ?string $address = null;

    public function getId(): ?Uuid
    {
        return $this->id;
    }

    public function getFullName(): ?string
    {
        return $this->fullName;
    }

    public function setFullName(string $fullName): static
    {
        $this->fullName = $fullName;

        return $this;
    }

    public function getAddress(): ?string
    {
        return $this->address;
    }

    public function setAddress(?string $address): static
    {
        $this->address = $address;

        return $this;
    }
}
  • We also have a PersonDto object as a layer for editing this entity:
namespace App\Dto;

use App\Entity\Person;
use Symfony\Bridge\Doctrine\Validator\Constraints\UniqueEntity;
use Symfony\Component\Validator\Constraints as Assert;
use Symfony\Component\Validator\Constraints\Uuid;

#[UniqueEntity(
    fields: ['fullName'],
    entityClass: Person::class,
    identifierFieldNames: ['id'],
)]
class PersonDto
{
    public ?Uuid $id;

    #[Assert\NotBlank]
    public ?string $fullName;

    public ?string $address;
}

The Person's table has already a few rows:

id fullName address
ec562e21-1fc8-4e55-8de7-a42389ac75c5 Finnley Benton 1000 FM 2004 Rd
b3842e7e-ad0b-46f2-b677-8d86bd3c546f Megan Herman NULL
5e417c18-769e-467e-b484-00cb12641b7c Leona Middleton 114 S 4th Ave
namespace App\Controller;

use App\Dto\PersonDto;
use App\Entity\Person;
use Doctrine\ORM\EntityManagerInterface;
use Symfony\Bundle\FrameworkBundle\Controller\AbstractController;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpFoundation\Response;
use Symfony\Component\Routing\Attribute\Route;
use Symfony\Component\Serializer\Normalizer\AbstractNormalizer;
use Symfony\Component\Serializer\Normalizer\DenormalizerInterface;
use Symfony\Component\Serializer\Normalizer\NormalizerInterface;

class PersonController extends AbstractController
{
    #[Route('/person/{id:person}', name: 'edit')]
    public function edit(
        Request $request,
        Person $person,
        NormalizerInterface $normalizer,
        DenormalizerInterface $denormalizer,
        EntityManagerInterface $entityManager,
    ): Response {
        $personArray = $normalizer->normalize($person);
        $personDto = $denormalizer->denormalize($personArray, PersonDto::class);

        $form = $this
            ->createForm(PersonType::class, $personDto)
            ->handleRequest($request)
        ;

        if ($form->isSubmitted() && $form->isValid()) {
            $editedPersonArray = $normalizer->normalize($form->getData());

            $denormalizer->denormalize($editedPersonArray, Person::class, context: [
                AbstractNormalizer::OBJECT_TO_POPULATE => $person,
            ]);

            $entityManager->flush();

            $this->addFlash('success', 'Person edited successfully.');
        }

        return $this->render('person/edit.html.twig', [
            'form' => $form,
        ]);
    }
}

You see.. when submitting this form, validation will fail on fullName because Serializer will create a completely new UUID object when denormalizing $personArray, but the real problem is the strict comparison here:

if ($fieldValues[$identifierFieldName] !== $propertyValue) {

Because of course, if we have this:

$uuid1 = new UUID('ec562e21-1fc8-4e55-8de7-a42389ac75c5');
$uuid2 = new UUID('ec562e21-1fc8-4e55-8de7-a42389ac75c5');
$uuid1 === $uuid2; // false

Then it won't work.

I don't know if it is intended behavior and if yes, I'm sorry, but otherwise, maybe a solution could be checking if both $fieldValues[$identifierFieldName] and $propertyValue are Stringable and then converting it to a string before the comparison.

@GiuseppeArcuti
Copy link
Contributor

Thanks for the detailed explanation. I think I've managed to reproduce with a unit test case. Please take a look

fabpot added a commit that referenced this issue May 10, 2025
…tifiers (GiuseppeArcuti, wkania)

This PR was merged into the 7.2 branch.

Discussion
----------

[DoctrineBridge] Fix UniqueEntityValidator Stringable identifiers

| Q             | A
| ------------- | ---
| Branch?       | 7.2
| Bug fix?     | yes
| New feature?  | no
| Deprecations? | no
| Issues        | Fix #58883
| License       | MIT

`@GiuseppeArcuti` , thanks for creating the test in [PR](#59126).

More info in the [comment](#59126 (comment)).

Commits
-------

b0f012f [DoctrineBridge] Fix UniqueEntityValidator Stringable identifiers
572ebe8 [DoctrineBridge] Fix UniqueEntity for non-integer identifiers
@fabpot fabpot closed this as completed May 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants