doctrine / mongodb-odm

The Official PHP MongoDB ORM/ODM
https://www.doctrine-project.org/projects/doctrine-mongodb-odm/en/latest/
MIT License
1.09k stars 504 forks source link

Query built incorrectly when having embedMany with discriminatorMap and non BSON id field #2531

Open xammmue opened 1 year ago

xammmue commented 1 year ago

Bug Report

Q A
BC Break no
Version 2.5.2

Summary

I created a combination of 5 classes Tree, Branch, Fruit, Apple and Mango. Apple and Mango extend Frui. A Tree embeds many branches and a branch embeds many fruits using a discriminator field and discriminator map. Fruit also has an id property which is not a BSON id but some other string (in this example UUID V4) which was generated from outside.

Current behavior

Writing a query to find a branch by a fruit id fails, as the query builder converts the value for the id field into a BSON Object id.

$this->createQueryBuilder()
            ->field('branches.fruits.id')->equals($fruitId)
            ->getQuery();

image

This is what it looks like when also specifying the targetDocument for Branch.fruits to be Fruit::class which has been deprecated. image

How to reproduce

I provided an example here: https://github.com/xammmue/embedManyWithNonBSONId But these are the essentials:

The bug only occurs when there is a "chain" of at least two embedded documents.

#[Document]
class Tree
{
    #[Id]
    private $id;
    #[EmbedMany(targetDocument: Branch::class)]
    private Collection $branches;

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

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

    public function getBranches(): array
    {
        return $this->branches->toArray();
    }
}
#[EmbeddedDocument]
class Branch
{
    #[
        EmbedMany(
            //targetDocument: Fruit::class,
            discriminatorField: Fruit::DISCRIMINATOR_FIELD,
            discriminatorMap: Fruit::DISCRIMINATOR_MAP
        )
    ]
    private Collection $fruits;

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

    public function getFruits(): Collection
    {
        return $this->fruits;
    }
}
#[EmbeddedDocument]
class Fruit
{
    public const DISCRIMINATOR_FIELD = 'type';
    public const DISCRIMINATOR_MAP = [
        'Apple' => Apple::class,
        'Mango' => Mango::class
    ];

    #[Field]
    private string $id;

    #[Field]
    private bool $ripe;

    public function __construct(string $id, bool $ripe)
    {
        $this->id = $id;
        $this->ripe = $ripe;
    }

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

    public function isRipe(): bool
    {
        return $this->ripe;
    }
}
class TreeRepository extends ServiceDocumentRepository
{
    public function __construct(ManagerRegistry $registry)
    {
        parent::__construct($registry, Tree::class);
    }

    public function findByFruitId(string $fruitId): ?Tree
    {
        return $this->createQueryBuilder()
            ->field('branches.fruits.id')->equals($fruitId)
            ->getQuery()
            ->getSingleResult();
    }
}

Expected behavior

I expected the correct Tree to be returned / the correct query to be built.

malarzm commented 1 year ago

The bug occurs because both top level document and deeply nested one both have field named id that are mapped differently. ODM tries to be helpful but is getting lost without targetDocument and falls back to Tree's class metadata, where id shall be converted to ObjectId. targetDoucment fixes the issue, but is indeed deprecated when discriminatorMap is used at the same time.

So, an easy fix is to name Fruid::$id differently on your end. Not the best one, but will work.

The proper one is to dive into \Doctrine\ODM\MongoDB\Persisters\DocumentPersister::prepareQueryElement and let there be no ClassMetadata instead of falling back to the top one.

On a closing thought on combining discriminatorMap with targetDocument: maybe it would make sense to allow combining them IF class pointed to by targetDocument is abstract and all classes in discriminatorMap have that class as an ancestor?