#6217 #6284 when hydrating an entity with a composite primary key that is both an `EAGER` and a `LAZY` association and also cached, the `DefaultQueryCache` tries to pass L2 cache implementation detail objects to the `UnitOfWork` #6640

Ocramius · 2017-08-22T22:31:47Z

This is a proper version of #6284, fixing the deeper problem.

Basically, what is happening is that you have a L2 cached entity class like following:

class A
{
    /** @Id @ManyToOne(targetEntity=b) */
    private $b;
    /** @Id @ManyToOne(targetEntity=c)*/
    private $c;
}

And a DQL query like following:

SELECT a, b
FROM A a
JOIN a.b b

The L2 cache for A.c won't have any information to cache, but it will still contain the identifier for A.c. This means that for that association, a AssociationCacheEntry exists anyway, but no definition in the QueryCacheEntry on how to process it.

I also tried fixing the problem on the other side (where the QueryCacheEntry is produced, rather than consumed), but overfitting it when no association data is actually available causes empty AssociationCacheEntry objects to be hydrated into empty entities (really bad!).

This is just a shotgun patch to fix the problem by iterating over all possible association fields, so if you have any better ideas, please do let me know.

…ry should never ever reach metadata

…id field

…e issue

…ching issue. We are not hydrating some of the cached association data into entities due to keys missing in the cache association definition. Since this is an extreme edge case that is just a mismatch between db and cache, a detailed explanation was provided in the fix snippet as well

…id fix that was actually fixing the symptom

lcobucci · 2017-08-23T07:56:36Z

tests/Doctrine/Tests/ORM/Functional/Ticket/GH6217Test.php

+        /* @var $found GH6217FetchedEntity[] */
+        $found = $repository->findBy($filters);
+
+        $this->assertCount(1, $found);


lcobucci · 2017-08-23T08:10:02Z

lib/Doctrine/ORM/Cache/DefaultQueryCache.php

+                //
+                // We need to unwrap those associations into proxy references,
+                // since we don't have actual data for them except for identifiers.
+                if ($unCachedAssociationData instanceof AssociationCacheEntry) {


It makes sense IMO, but I feel like we could do that conversion while iterating $entry['associations'] in https://github.com/doctrine/doctrine2/pull/6640/files#diff-c5343cb0b202102a59de56dfdcb8ee47R147 to avoid looping over the entire object data.

$entry['associations'] does not contain the keys we're iterating upon here.

I considered using an accumulator of already processed keys, but since these are usually about associations, and the number of fields and associations is (typically) < 10 on any entity, looping is actually faster than aggregating the keys and excluding them here.

Ocramius · 2017-08-25T07:22:34Z

🚢

gadelkareem and others added 16 commits August 22, 2017 19:45

Add tests for #6217

309b286

#6284 removing the "WTF" part of the logic - an association cache ent…

a8453dd

…ry should never ever reach metadata

#6284 simplifying tests to a minimum, removing unused auto-generated …

c7281f6

…id field

#6284 simplifying tests to a minimum, removing unused auto-generated …

3842ad8

…id field

#6284 reducing annotation mapping clutter

805ba04

#6284 eager fetching is strictly required to verify the issue

4e0b76c

#6284 reducing annotation mapping clutter

ca39abc

#6284 removing profile entity, since it is not needed to reproduce th…

dac1ce4

…e issue

#6284 adding assertions about equality of the loaded classes

6f6e88c

#6284 renaming entities to match the scope of this test

220dc79

#6284 correcting alignment, removing stuff that isn't strictly needed

3f09e20

#6284 renaming variables, classes and properties to fit tested behavior

a2f4053

#6284 removing unused separate class

c29a1e9

#6284 removing unused lazy entity

c9d1f85

#6284 #6217 removing hacks around the ClassMetadata details - inval…

caa008b

…id fix that was actually fixing the symptom

Ocramius added the Bug label Aug 22, 2017

Ocramius added this to the 2.6.0 milestone Aug 22, 2017

Ocramius assigned lcobucci Aug 22, 2017

Ocramius requested a review from lcobucci August 22, 2017 22:31

Ocramius mentioned this pull request Aug 22, 2017

#6217 when hydrating an entity with a composite primary key that is both an EAGER and a LAZY association and also cached, the DefaultQueryCache tries to pass L2 cache implementation detail objects to the UnitOfWork #6284

Closed

lcobucci suggested changes Aug 23, 2017

View reviewed changes

#6284 #6217 s/$this->assert/self::assert as per @lcobucci's review

660f164

Ocramius assigned Ocramius and unassigned lcobucci Aug 25, 2017

Ocramius merged commit 61404e2 into master Aug 25, 2017

Ocramius deleted the fix/#6284-#6217-avoid-passing-l2-cache-information-internals-to-the-uow branch August 25, 2017 07:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#6217 #6284 when hydrating an entity with a composite primary key that is both an `EAGER` and a `LAZY` association and also cached, the `DefaultQueryCache` tries to pass L2 cache implementation detail objects to the `UnitOfWork` #6640

#6217 #6284 when hydrating an entity with a composite primary key that is both an `EAGER` and a `LAZY` association and also cached, the `DefaultQueryCache` tries to pass L2 cache implementation detail objects to the `UnitOfWork` #6640

Ocramius commented Aug 22, 2017

lcobucci Aug 23, 2017

lcobucci Aug 23, 2017 •

edited

Loading

Ocramius Aug 25, 2017

Ocramius commented Aug 25, 2017

#6217 #6284 when hydrating an entity with a composite primary key that is both an EAGER and a LAZY association and also cached, the DefaultQueryCache tries to pass L2 cache implementation detail objects to the UnitOfWork #6640

#6217 #6284 when hydrating an entity with a composite primary key that is both an EAGER and a LAZY association and also cached, the DefaultQueryCache tries to pass L2 cache implementation detail objects to the UnitOfWork #6640

Conversation

Ocramius commented Aug 22, 2017

lcobucci Aug 23, 2017

Choose a reason for hiding this comment

lcobucci Aug 23, 2017 • edited Loading

Choose a reason for hiding this comment

Ocramius Aug 25, 2017

Choose a reason for hiding this comment

Ocramius commented Aug 25, 2017

#6217 #6284 when hydrating an entity with a composite primary key that is both an `EAGER` and a `LAZY` association and also cached, the `DefaultQueryCache` tries to pass L2 cache implementation detail objects to the `UnitOfWork` #6640

#6217 #6284 when hydrating an entity with a composite primary key that is both an `EAGER` and a `LAZY` association and also cached, the `DefaultQueryCache` tries to pass L2 cache implementation detail objects to the `UnitOfWork` #6640

lcobucci Aug 23, 2017 •

edited

Loading