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

Identity map error when running SELECT on the same table alias more than once #10889

Closed
emhovis opened this issue Aug 8, 2023 · 17 comments
Closed

Comments

@emhovis
Copy link

emhovis commented Aug 8, 2023

BC Break Report

Q A
BC Break yes
Version 2.16.0

Summary

Upon upgrading to 2.16, the fetch queries for the entity "Foo" started throwing the following:

(Bar@13041) has no identity/no id values set. It cannot be added to the identity map. 

I went absolutely crazy searching for the issue. Note, Bar is a 3rd degree association to Foo-there are 2 entities between Foo and Bar!

After much digging, I found that the problem comes from the many joins we have on Foo and its associated entities. There are many joins on Foo and when all of the aliases were first written, one of the aliases was entered twice when passed into the array for select. Removing this duplicate join-table alias from the array passed into select resolved the issue.

I'm not 100% certain that the new behavior is a "bug" per-se, but it seems like it breaking change that should be noted.

Previous behavior

Querying the DB with multiple of the same table alias selected would return results without error.

Current behavior

Querying the DB returns with multiple of the same selected table alias causes an identity mapping error.

How to reproduce

Add joins to an entity, call a select statement with an argument of an array that includes the same join alias twice, e.g.

$qb->leftJoin("{$a}.foo", "joinFoo")
      ->leftJoin("{$a}.bar", 'joinBar');
$qb->select([
            self::TABLE_ALIAS,
            'joinFoo',
            'joinBar',
            'joinFoo'
]);
@mpdude
Copy link
Contributor

mpdude commented Aug 9, 2023

Do you think you could provide a (failing) test that reproduces the issue, ideally as a PR?

Have a look at the GHxxxTest.php functional test cases in this repo, that should help you get going. You can define entities in the test files as well, and there are also some entity “models” that you might want to use.

@mpdude
Copy link
Contributor

mpdude commented Aug 9, 2023

That duplicate alias, has it been reused for different tables? Or for the same table but in multiple stages of a join hierarchy?

Or was it just given multiple times in the DQL SELECT clause?

@emhovis
Copy link
Author

emhovis commented Aug 9, 2023

That duplicate alias, has it been reused for different tables? Or for the same table but in multiple stages of a join hierarchy?

Or was it just given multiple times in the DQL SELECT clause?

In my case, It was passed in multiple times in a singular DQL SELECT clause.
I just tested adding one again in a separate addSelect clause and adding a select to a table that was previously defined also throws the issue.

The following throws the error:

$qb->select([
            self::TABLE_ALIAS,
            'joinA',
            'joinB',
            'joinC',
            'joinB'
]);

This will also throw the error:

$qb->select([
            self::TABLE_ALIAS,
            'joinA',
            'joinB',
            'joinC',
]);
$qb->addSelect('joinC');

Prior to 2.16.0, the above 2 don't cause any issue. I don't necessarily think the old behavior is necessarily ideal though. I don't personally see any valid reason using select on the same alias twice- even if you try to modularize joins+selects, an error gets thrown if you try to create an already-existing table alias. The only way I can think of someone running into this issue is via typo or an entry mistakenly being added twice. In my case, someone wrote a fed a select a ~20 value array with similarly-named aliases and a single one of those was mistakenly added twice.

My biggest gripe is that if upgrading to 2.16 breaks existing code then it should at least be noted in the changelog, changed to a deprecation warning instead of error and or pushed to 3.0. Also note, the error given doesn't exactly help pinpoint the issue... fixing that may alleviate some woes.

@emhovis
Copy link
Author

emhovis commented Aug 9, 2023

Do you think you could provide a (failing) test that reproduces the issue, ideally as a PR?

Have a look at the GHxxxTest.php functional test cases in this repo, that should help you get going. You can define entities in the test files as well, and there are also some entity “models” that you might want to use.

I'll see what I can do!

@mpdude
Copy link
Contributor

mpdude commented Aug 9, 2023

I’d assume this was not an intentional change, but a side effect of something else and not covered/caught by tests.

mpdude added a commit to mpdude/doctrine2 that referenced this issue Aug 9, 2023
@mpdude
Copy link
Contributor

mpdude commented Aug 9, 2023

I've created #10901, but that's not yet sufficient to show the issue.

Feel free to open a PR against https://github.com/mpdude/doctrine2/tree/repro-10889 if you have any idea what needs to be included.

@emhovis
Copy link
Author

emhovis commented Aug 9, 2023

Now I'm intrigued... I can't seem to replicate the issue in the doctrine test suite. I'll have to port over some more associations, filters etc and dig a bit deeper to see what the real problem is.

@emhovis
Copy link
Author

emhovis commented Aug 9, 2023

Still haven't managed to replicate it in a functional test but found something intriguing.

This doesn't throw the error:

        $a = self::TABLE_ALIAS;
        $qb = $this->createQueryBuilder($a)->select();
        $qb->leftJoin("{$a}.entityB", "joinB")
            ->leftJoin("joinB.entityC", "joinC");
        $qb->select([
            $a,
            'joinB',
            'joinB',
            'joinC',
            'joinC',
        ]);

But this does:

        $a = self::TABLE_ALIAS;
        $qb = $this->createQueryBuilder($a)->select();
        $qb->leftJoin("{$a}.entityB", "joinB")
            ->leftJoin("joinB.entityC", "joinC");
        $qb->select([
            $a,
            'joinB',
            'joinC',
            'joinC',
        ]);

@mpdude
Copy link
Contributor

mpdude commented Aug 15, 2023

X-Ref sulu/sulu#7138

@mpdude
Copy link
Contributor

mpdude commented Aug 23, 2023

Anyone been able to reproduce?

@noemi-salaun
Copy link
Contributor

I managed to replicate the problem with a minimum of code with doctrine/orm 2.16.1

Create this entities

use Doctrine\ORM\Mapping as ORM;

#[ORM\Entity, ORM\Table(name: 'user')]
class User
{
    #[ORM\Id, ORM\Column]
    private int $id;
}

#[ORM\Entity, ORM\Table(name: 'finger')]
class Finger
{
    #[ORM\Id, ORM\Column]
    private int $id;
}

#[ORM\Entity, ORM\Table(name: 'hand')]
class Hand
{
    #[ORM\OneToOne, ORM\Id]
    private User $user;

    #[ORM\ManyToOne]
    private ?Finger $thumb;
}

Create the MySQL database

create table finger
(
    id int not null primary key
);

create table user
(
    id int not null primary key
);

create table hand
(
    userId  int not null primary key,
    thumbId int null
);

INSERT INTO user (id) VALUES (1);
INSERT INTO hand (userId, thumbId) VALUES (1, null);

And then execute this query

        $entityManager
            ->getRepository(Hand::class)
            ->createQueryBuilder('hand')
            ->leftJoin('hand.thumb', 'thumb')->addSelect('thumb')
            ->getQuery()
            ->getResult();

It should produce this error

The given entity of type 'Finger' (Finger@471) has no identity/no id values set. It cannot be added to the identity map.

If you create a finger and set a finger NOT NULL in the hand, no error
If you join but not addSelect in the query, no error
If the Hand use a simple int as ID instead of a relation, no error
If you rollback doctrine/orm to v2.15, no error

I hope it will help you identify the issue

@mpdude
Copy link
Contributor

mpdude commented Aug 24, 2023

So this is related to having an object and/or association value serve as the ID?

I have seen this pattern quite a few times in recent bug reports, maybe that deserves a more thoughtful examination.

@mpdude
Copy link
Contributor

mpdude commented Aug 24, 2023

#10788, #10852

@noemi-salaun
Copy link
Contributor

Any news on this issue ? The v2.17 is released but I'm still locked on v2.15 because of this :/
The issue is still present on v2.17

@noemi-salaun
Copy link
Contributor

By going threw each commits between 2.15.5 and 2.16.0 I found that the issue appeared after this commit 01a1432

I'll continue digging

noemi-salaun added a commit to noemi-salaun/doctrine-orm that referenced this issue Jan 28, 2024
noemi-salaun added a commit to noemi-salaun/doctrine-orm that referenced this issue Jan 28, 2024
@noemi-salaun
Copy link
Contributor

Here #11194 is my PR which seems to fix the issue, at least for my case

noemi-salaun added a commit to noemi-salaun/doctrine-orm that referenced this issue Jan 28, 2024
noemi-salaun added a commit to noemi-salaun/doctrine-orm that referenced this issue Feb 4, 2024
noemi-salaun added a commit to noemi-salaun/doctrine-orm that referenced this issue May 2, 2024
noemi-salaun added a commit to noemi-salaun/doctrine-orm that referenced this issue Jun 10, 2024
noemi-salaun added a commit to noemi-salaun/doctrine-orm that referenced this issue Jun 14, 2024
noemi-salaun added a commit to noemi-salaun/doctrine-orm that referenced this issue Jun 14, 2024
greg0ire added a commit that referenced this issue Jun 14, 2024
Skip joined entity creation for empty relation (#10889)
derrabus added a commit to derrabus/orm that referenced this issue Jun 18, 2024
* 2.19.x:
  Fix OneToManyPersister::deleteEntityCollection missing discriminator column/value. (doctrineGH-11500)
  Skip joined entity creation for empty relation (doctrine#10889)
  ci: maintained and stable mariadb version (11.4 current lts) (doctrine#11490)
  fix(docs): use string value in `addAttribute`
  Replace assertion with exception (doctrine#11489)
  Use ramsey/composer-install in PHPBench workflow
  update EntityManager#transactional to EntityManager#wrapInTransaction
  Fix cloning entities
  Consider usage of setFetchMode when checking for simultaneous usage of fetch-mode EAGER and WITH condition.
derrabus added a commit to derrabus/orm that referenced this issue Jun 19, 2024
* 2.19.x:
  Fix OneToManyPersister::deleteEntityCollection missing discriminator column/value. (doctrineGH-11500)
  Skip joined entity creation for empty relation (doctrine#10889)
  ci: maintained and stable mariadb version (11.4 current lts) (doctrine#11490)
  fix(docs): use string value in `addAttribute`
  Replace assertion with exception (doctrine#11489)
  Use ramsey/composer-install in PHPBench workflow
  update EntityManager#transactional to EntityManager#wrapInTransaction
  Fix cloning entities
  Consider usage of setFetchMode when checking for simultaneous usage of fetch-mode EAGER and WITH condition.
derrabus added a commit to derrabus/orm that referenced this issue Jun 19, 2024
* 3.3.x:
  Fix OneToManyPersister::deleteEntityCollection missing discriminator column/value. (doctrineGH-11500)
  Skip joined entity creation for empty relation (doctrine#10889)
  ci: maintained and stable mariadb version (11.4 current lts) (doctrine#11490)
  fix(docs): use string value in `addAttribute`
  Replace assertion with exception (doctrine#11489)
  Use ramsey/composer-install in PHPBench workflow
  update EntityManager#transactional to EntityManager#wrapInTransaction
  Fix cloning entities
  Consider usage of setFetchMode when checking for simultaneous usage of fetch-mode EAGER and WITH condition.
  Update branch metadata (doctrine#11474)
derrabus added a commit to derrabus/orm that referenced this issue Jun 21, 2024
* 3.3.x:
  Fix OneToManyPersister::deleteEntityCollection missing discriminator column/value. (doctrineGH-11500)
  Skip joined entity creation for empty relation (doctrine#10889)
  ci: maintained and stable mariadb version (11.4 current lts) (doctrine#11490)
  fix(docs): use string value in `addAttribute`
  Replace assertion with exception (doctrine#11489)
  Use ramsey/composer-install in PHPBench workflow
  update EntityManager#transactional to EntityManager#wrapInTransaction
  Fix cloning entities
  Consider usage of setFetchMode when checking for simultaneous usage of fetch-mode EAGER and WITH condition.
  Update branch metadata (doctrine#11474)
derrabus added a commit to derrabus/orm that referenced this issue Jun 21, 2024
* 3.3.x:
  Fix deprecated array access usage (doctrine#11517)
  Address doctrine/persistence 3.3.3 release
  Add the propoer void return type on the __load method of proxies
  Deprecate DatabaseDriver
  Remove unneeded CS rule
  Fix OneToManyPersister::deleteEntityCollection missing discriminator column/value. (doctrineGH-11500)
  Skip joined entity creation for empty relation (doctrine#10889)
  ci: maintained and stable mariadb version (11.4 current lts) (doctrine#11490)
  fix(docs): use string value in `addAttribute`
  Replace assertion with exception (doctrine#11489)
  Use ramsey/composer-install in PHPBench workflow
  update EntityManager#transactional to EntityManager#wrapInTransaction
  Fix cloning entities
  Consider usage of setFetchMode when checking for simultaneous usage of fetch-mode EAGER and WITH condition.
  Update branch metadata (doctrine#11474)
derrabus added a commit to derrabus/orm that referenced this issue Jun 21, 2024
* 3.3.x:
  Fix deprecated array access usage (doctrine#11517)
  Address doctrine/persistence 3.3.3 release
  Add the propoer void return type on the __load method of proxies
  Deprecate DatabaseDriver
  Remove unneeded CS rule
  Fix OneToManyPersister::deleteEntityCollection missing discriminator column/value. (doctrineGH-11500)
  Skip joined entity creation for empty relation (doctrine#10889)
  ci: maintained and stable mariadb version (11.4 current lts) (doctrine#11490)
  fix(docs): use string value in `addAttribute`
  Replace assertion with exception (doctrine#11489)
  Use ramsey/composer-install in PHPBench workflow
  update EntityManager#transactional to EntityManager#wrapInTransaction
  Fix cloning entities
  Consider usage of setFetchMode when checking for simultaneous usage of fetch-mode EAGER and WITH condition.
  Update branch metadata (doctrine#11474)
@alexander-schranz
Copy link
Contributor

Think this can be closed as fixed in #11194 ?

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

No branches or pull requests

5 participants