EasyCorp / EasyAdminBundle

EasyAdmin is a fast, beautiful and modern admin generator for Symfony applications.
MIT License
3.99k stars 1.01k forks source link

EntityFilter not working with UUIDs #4125

Closed j0r1s closed 2 years ago

j0r1s commented 3 years ago

Describe the bug I have entities identified by uuids, the EntityFilter is not filtering them properly and returns no result.

To Reproduce I've recreated a blank project and I'm able to reproduce the bug following those steps :

symfony new ea_uuids --full
cd ea_uuids
composer require easycorp/easyadmin-bundle #latest version then

Then I'm creating two simple entities identified by uuids with custom generators UuidV4, following this symfony blog post.

symfony console make:entity Book
symfony console make:entity Category

Configuring EA :

symfony console make:admin:dashboard
symfony console make:admin:crud # create Book crud (With EntityFilter on category field) 
symfony console make:admin:crud # create Category crud

Then I'm creating a category via EA crud, and a book with this category associated.

(OPTIONAL) Additional context

src/Entity/Book.php

<?php

namespace App\Entity;

use Doctrine\ORM\Mapping as ORM;
use Symfony\Bridge\Doctrine\IdGenerator\UuidV4Generator;

/**
 * @ORM\Entity(repositoryClass=BookRepository::class)
 */
class Book
{
    /**
     * @ORM\Id
     * @ORM\Column(type="uuid", unique=true)
     * @ORM\GeneratedValue(strategy="CUSTOM")
     * @ORM\CustomIdGenerator(class=UuidV4Generator::class)
     */
    private $id;

    /**
     * @ORM\Column(type="string", length=255)
     */
    private $title;

    /**
     * @ORM\ManyToOne(targetEntity=Category::class, inversedBy="books")
     */
    private $category;

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

    public function getTitle(): ?string
    {
        return $this->title;
    }

    public function setTitle(string $title): self
    {
        $this->title = $title;

        return $this;
    }

    public function getCategory(): ?Category
    {
        return $this->category;
    }

    public function setCategory(?Category $category): self
    {
        $this->category = $category;

        return $this;
    }
}

src/Entity/Category.php

<?php

namespace App\Entity;

use App\Repository\CategoryRepository;
use Doctrine\Common\Collections\ArrayCollection;
use Doctrine\Common\Collections\Collection;
use Doctrine\ORM\Mapping as ORM;
use Symfony\Bridge\Doctrine\IdGenerator\UuidV4Generator;

/**
 * @ORM\Entity(repositoryClass=CategoryRepository::class)
 */
class Category
{
    /**
     * @ORM\Id
     * @ORM\Column(type="uuid", unique=true)
     * @ORM\GeneratedValue(strategy="CUSTOM")
     * @ORM\CustomIdGenerator(class=UuidV4Generator::class)
     */
    private $id;

    /**
     * @ORM\Column(type="string", length=255)
     */
    private $name;

    /**
     * @ORM\OneToMany(targetEntity=Book::class, mappedBy="category")
     */
    private $books;

    public function __construct()
    {
        $this->books = new ArrayCollection();
    }

    public function __toString(): string
    {
        return $this->name;
    }

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

    public function getName(): ?string
    {
        return $this->name;
    }

    public function setName(string $name): self
    {
        $this->name = $name;

        return $this;
    }

    /**
     * @return Collection|Book[]
     */
    public function getBooks(): Collection
    {
        return $this->books;
    }

    public function addBook(Book $book): self
    {
        if (!$this->books->contains($book)) {
            $this->books[] = $book;
            $book->setCategory($this);
        }

        return $this;
    }

    public function removeBook(Book $book): self
    {
        if ($this->books->removeElement($book)) {
            // set the owning side to null (unless already changed)
            if ($book->getCategory() === $this) {
                $book->setCategory(null);
            }
        }

        return $this;
    }
}

src/Controller/Admin/BookCrudController.php

<?php

namespace App\Controller\Admin;

use App\Entity\Book;
use EasyCorp\Bundle\EasyAdminBundle\Config\Filters;
use EasyCorp\Bundle\EasyAdminBundle\Controller\AbstractCrudController;
use EasyCorp\Bundle\EasyAdminBundle\Field\AssociationField;
use EasyCorp\Bundle\EasyAdminBundle\Field\TextField;
use EasyCorp\Bundle\EasyAdminBundle\Filter\EntityFilter;

class BookCrudController extends AbstractCrudController
{
    public static function getEntityFqcn(): string
    {
        return Book::class;
    }

    public function configureFields(string $pageName): iterable
    {
        return [
            TextField::new('title'),
            AssociationField::new('category'),
        ];
    }

    public function configureFilters(Filters $filters): Filters
    {
        return $filters->add(EntityFilter::new('category'));
    }
}

I'm not adding other files (DasboardController / CategoryCrudController) since their configuration is trivial.


I've investigated EA code and found that if I add those modifications to vendor/easycorp/easyadmin-bundle/src/Filter/EntityFilter.php

- ->setParameter($parameterName, $value);
+ ->setParameter($parameterName, $value->getId(), 'uuid');

The filtering works fine, I have no idea how to fix this "properly" in a backward compatible way with classical auto-increment strategies.

OskarStark commented 3 years ago

Can you try to modify the file like this?

- ->setParameter($parameterName, $value);
+ ->setParameter($parameterName, $value->getId()->toBinary());

We could add a check if instanceof AbstractUuid or AbstractUlid, -> toBinary()

j0r1s commented 3 years ago

yes it's working with $value->getId()->toBinary()

We could check for $value->getId() instance of AbstractU[ul]id but how one could guess that the primary key is $id ? This is conventional and not mandatory, right ?

OskarStark commented 3 years ago

Lets wait for @javiereguiluz here

javiereguiluz commented 3 years ago

Sorry, I don't use UUIDs for entity IDs ... so I don't know how to properly fix this or even if we can fix this without breaking the existing behavior. Sorry!

vic-blt commented 3 years ago

@j0r1s I am using UUIDs and I can't reproduce this bug. Which version of EasyAdmin are you using ?

EDIT: My setup differs from yours, that's why I don't have the same behavior.

OskarStark commented 3 years ago

Are you using a UUID as string or an object?

vic-blt commented 3 years ago

As a string, here's my IdTrait

trait IdTrait
{
    /**
     * @ORM\Id
     * @ORM\Column(type="string")
     * @ORM\GeneratedValue(strategy="UUID")
     */
    protected $id;

    /**
     * @return string
     */
    public function getId()
    {
        return $this->id;
    }
}

I'm not using the new doctrine type. And I'm using UUID v1, not v4.

OskarStark commented 3 years ago

Ok yes this is working, but not using a Uid as object πŸ‘πŸ»

j0r1s commented 3 years ago

Ok yes this is working, but not using a Uid as object πŸ‘πŸ»

Yes exactly ! My UUIDs are stored as binary objects, and Doctrine is querying for the string representation.

In my project, I've solved the issue by creating a custom UuidEntityFilter that casts the parameter to its binary form.

Afterall I'm wondering if this could be an issue that belongs to Symfony since I've had to edit some of my classics (not EA) repository methods for the queries to work. The worst part is that it fails silently, the queries just return no result.

javiereguiluz commented 3 years ago

Anyone willing to investigate if we could "fix" (or "improve") this in EasyAdmin? Maybe we can add some "isBinary()" check somewhere to transform UUIDs into strings, if needed, when performing some query. Thanks!

OskarStark commented 3 years ago

Anyone willing to investigate if we could "fix" (or "improve") this in EasyAdmin? Maybe we can add some "isBinary()" check somewhere to transform UUIDs into strings, if needed, when performing some query. Thanks!

@javiereguiluz I will also take a look sone, as I am building a new project using symfony/uid πŸ‘

In my project, I've solved the issue by creating a custom UuidEntityFilter that casts the parameter to its binary form.

@j0r1s what about adding a check to the EntityFilter so we have a generic entry point for this?

Can you try if this works in your project by modifiying the vendor code?

svenkrefeld commented 3 years ago

Any updates? I'm currently working on a project with symfony/uid and could see the described behavior also on my side. I am trying to find a universal solution for the "isBinary()" check of the value

michaelKaefer commented 2 years ago

Hi @OskarStark , are still planning to work on this issue? If not then I would try to have a closer look on this.

OskarStark commented 2 years ago

Hi, I don't have some time to jump on this right now, sorry

michaelKaefer commented 2 years ago

I created a reproducer: https://github.com/michaelKaefer/easy-admin-reproducer/tree/issue/4125 (and a PR).

ToshY commented 2 years ago

I just wanted to add that I'm using v4.0.1 but the fix doesn't seem to work for me. Instead of OPs UuidV4Generator, I'm using UuidOrderedTimeGenerator from ramsey/uuid-doctrine for my identifiers.

michaelKaefer commented 2 years ago

@ToshY Yes, the fix was meant for symfony/uid and is not working for ramsey/uuid-doctrine. I am not sure but I guess if you want to use ramsey/uuid-doctrine you have to implement everything you need in your app. I guess that the maintainers do not want to support every UUID implementation but I am not sure, maybe a PR would be accepted.

Just because I am curious, do you think you could switch to symfony/uid for your use case?

ToshY commented 2 years ago

@michaelKaefer Well I could definitely switch as I'm just starting a new project. The thing is that I already used ramsey/uuid and ramsey/uuid-doctrine in other Symfony projects (without EasyAdmin), so I was under the impression that something like that would also be supported. Dangerous assumption in hindsight.

Guess it makes sense to refrain from supporting lots of UUID implementations, but it's also worth noting that symfony/uid "does not replace full-featured libraries such as ramsey/uuid". Correct me if I'm wrong, but with ramsey/uuid being the most popular library in this regard, maybe adding support wouldn't hurt?

michaelKaefer commented 2 years ago

@ToshY Thanks for the infos! - I don't know, maybe a PR is welcome but supporting a lot of packages means that the maintainers have to maintain a lot of code.

pluk77 commented 1 year ago

We are also using ramsey/uuid and ran into the same issue. We will investigate a fix and submit a PR.

pluk77 commented 1 year ago

If I am not mistaken, the following code eventually makes use of the doctrine types, which is why a single added line would make it work for Ramsey/uuid, seeing it is also using uuid as the doctrine type name:

if (('uuid' === $identifierType && $identifierValue instanceof Uuid)
    || ('ulid' === $identifierType && $identifierValue instanceof Ulid)
    || ('uuid' === $identifierType && $identifierValue instanceof Ramsey\Uuid)) {

Which is why I am wondering what the added value is of limiting the code to specific UUID or ULID classes.

Why not remove this restriction and go for the following?

if ('uuid' === $identifierType || 'ulid' === $identifierType) {
    try {
        return Type::getType($identifierType)->convertToDatabaseValue($identifierValue, $entityManager->getConnection()->getDatabasePlatform());
    } catch (\Throwable $exception) {
        // if the conversion fails we cannot process the uid parameter value
        return $parameterValue;
    }
}