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

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

Open
xammmue opened this issue Apr 28, 2023 · 1 comment
Labels

Comments

@xammmue
Copy link

xammmue commented Apr 28, 2023

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.

@xammmue xammmue changed the title Query build incorrectly when having embedMany with discriminatorMap and non BSON id field Query built incorrectly when having embedMany with discriminatorMap and non BSON id field Apr 29, 2023
@malarzm
Copy link
Member

malarzm commented May 2, 2023

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?

@malarzm malarzm added the Bug label May 2, 2023
@malarzm malarzm added this to the 2.5.3 milestone May 2, 2023
@malarzm malarzm modified the milestones: 2.5.3, 2.5.4 Oct 23, 2023
@malarzm malarzm modified the milestones: 2.5.4, 2.5.5 Nov 3, 2023
@alcaeus alcaeus removed this from the 2.5.5 milestone Nov 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants