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

Cannot use criteria matching with entities where the primary key is a binary ULID #8995

Closed
jdelaune opened this issue Sep 8, 2021 · 7 comments · Fixed by #9010
Closed

Cannot use criteria matching with entities where the primary key is a binary ULID #8995

jdelaune opened this issue Sep 8, 2021 · 7 comments · Fixed by #9010

Comments

@jdelaune
Copy link

jdelaune commented Sep 8, 2021

I'm trying to do an efficient join using criteria in one of my entities like:

    /**
     * @ORM\Id()
     * @ORM\Column(type="ulid", unique=true)
     */
    private Ulid $id;

    /**
     * @var Collection<User>
     * @ORM\ManyToMany(targetEntity="User", mappedBy="teams", fetch="EXTRA_LAZY")
     */
    private Collection $users;

    public function getMemberSample(): Collection
    {
        $criteria = Criteria::create()
            ->orderBy(['lastName' => 'ASC'])
            ->setMaxResults(5)
        ;

        return $this->users->matching($criteria);
    }

However the query doctrine creates is:

SELECT te.id AS id, te.first_name AS first_name, te.last_name AS last_name FROM users te JOIN user_teams t ON t.user_id = te.id WHERE t.team_id = '01EP7GVKX9K05ZK8G9ZRZTTW3N' ORDER BY last_name ASC LIMIT 5;

As you can see it has just turned the ULID into a string which won't match the binary.

I feel like this maybe come from vendor/doctrine/orm/lib/Doctrine/ORM/Persisters/Collection/ManyToManyPersister.php

    /**
     * {@inheritDoc}
     */
    public function loadCriteria(PersistentCollection $collection, Criteria $criteria)
    {
        $mapping       = $collection->getMapping();
        $owner         = $collection->getOwner();
        $ownerMetadata = $this->em->getClassMetadata(get_class($owner));
        $id            = $this->uow->getEntityIdentifier($owner);
        $targetClass   = $this->em->getClassMetadata($mapping['targetEntity']);
        $onConditions  = $this->getOnConditionSQL($mapping);
        $whereClauses  = $params = [];
        if (! $mapping['isOwningSide']) {
            $associationSourceClass = $targetClass;
            $mapping                = $targetClass->associationMappings[$mapping['mappedBy']];
            $sourceRelationMode     = 'relationToTargetKeyColumns';
        } else {
            $associationSourceClass = $ownerMetadata;
            $sourceRelationMode     = 'relationToSourceKeyColumns';
        }
        foreach ($mapping[$sourceRelationMode] as $key => $value) {
            $whereClauses[] = sprintf('t.%s = ?', $key);
            $params[]       = $ownerMetadata->containsForeignIdentifier
                ? $id[$ownerMetadata->getFieldForColumn($value)]
                : $id[$ownerMetadata->fieldNames[$value]];
        }

Where maybe the $params[] being created here isn't using the binary value.

Anyone know where this bug might lie who knows a bit more about how it work, I'm also not sure who is responsible, Doctrine ORM, Doctrine Bundle, Symfony Uid...

@jdelaune
Copy link
Author

jdelaune commented Sep 9, 2021

If I make a modification to vendor/doctrine/orm/lib/Doctrine/ORM/Persisters/Collection/ManyToManyPersister.php

        foreach ($mapping[$sourceRelationMode] as $key => $value) {
            $whereClauses[] = sprintf('t.%s = ?', $key);
            $param          = $ownerMetadata->containsForeignIdentifier
                ? $id[$ownerMetadata->getFieldForColumn($value)]
                : $id[$ownerMetadata->fieldNames[$value]];
            if ($param instanceof Ulid) {
                $param = $param->toBinary();
            }
            $params[]       = $param;
        }

Then it works, but it isn't really a solution since this is Doctrine ORM code which knows nothing about the Symfony UID component.

@stof
Copy link
Member

stof commented Sep 9, 2021

Well, the right fix would be to use the metadata to determine the type of the field, and bind the parameter with an explicit type instead of relying on type guessing.

@ostrolucky
Copy link
Member

Should be moved to ORM?

@jdelaune
Copy link
Author

jdelaune commented Sep 9, 2021

Well, the right fix would be to use the metadata to determine the type of the field, and bind the parameter with an explicit type instead of relying on type guessing.

If you can do that great. I wasn't sure how the internals work and if the ORM would know about the Symfony ULID type which is non-standard. Will have a poke around some more and see if I can figure it out.

@ostrolucky ostrolucky transferred this issue from doctrine/DoctrineBundle Sep 9, 2021
@greg0ire
Copy link
Member

Is this a duplicate of #8406 ?

@jdelaune
Copy link
Author

Yes looks like it is

@greg0ire
Copy link
Member

Duplicate of #8406

@greg0ire greg0ire marked this as a duplicate of #8406 Sep 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants