Fix duplicate data in Eager loads relations

[ilicmsreten]

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets
License	MIT
Doc PR

This PR is related to reported bug in api-platform/api-platform project.

So problem is caused because the same tables is joined multiple times ON same condition but with different aliases.

I find useful article by Marco Pivetta about Doctrine hydration where they said about coding multi-step hydration, that is used in PR.

[soyuka]

Mhhh but this will execute the query multiple time right? I don't get the point.

May we have a test illutrating the bug in api-platform/api-platform#799 ?

[ilicmsreten]

@soyuka that will not affect performance. The whole process is described here

[soyuka]

I assume you're talking about the comment saying: // result is discarded (this is just re-hydrating the collections). May you add a comment like this because it's really confusing if you're not familiar with doctrine internals...

[ilicmsreten]

@soyuka sure, also I will also add tests to cover this

[soyuka]

thanks @ilicmsreten !

[raziel057]

Hi, this PR generate an issue when getting data from a table containing lots of data because a query is generated and hydration is done without any LIMIT. This is linked to the fact the PaginationExtension is not yet applied to the query builder.

As you can see in https://github.com/api-platform/core/blob/master/src/Bridge/Doctrine/Orm/CollectionDataProvider.php, when calling applyToCollection when iterating first on EagerLoadingExtension, a query is done without the LIMIT.

For me getResult should never be called directly in applyToCollection of any extension.

[raziel057]

@soyuka @dunglas Could you please revert this PR? It produce a lot of errors on existing APIs.

[raziel057]

Or if you wants to keep the multi step hydration it should be done after all other extensions...

[soyuka]

Or if you wants to keep the multi step hydration it should be done after all other extensions...

We will definitely look into this asap because performances could be improved. Sorry about the mistake, we're in the process of tagging 2.3.3 which will revert this patch.

[raziel057]

Ok thanks, in fact it's more than just a performance issue. For example I always get memory limit issues (error 500) when getting a limited list of items from a table containing a lot of data (more than 50000).

[soyuka]

@raziel057 we tagged 2.3.3 without this patch and we'll try to improve it taking your inputs into consideration before it gets merged! Thanks!

[raziel057]

Thank you very much for your fast action!

[errogaht]

@soyuka fix is reverted, but I'm here because of original issue ))) I have doctrine collections with the same elements inside. Can you recommend a fix for this?

[soyuka]

@soyuka fix is reverted, but I'm here because of original issue ))) I have doctrine collections with the same elements inside. Can you recommend a fix for this?

I'm really not sure how this is possible, would you mind giving me a way to reproduce?

/edit: gonna look into api-platform/api-platform#799

[soyuka]

@errogaht the solution is to simply disable the eager loading feature or build your own query. I'm really not sure how we could fix this otherwise...

For the api-platform/api-platform#799 example:

/**
 * @ApiResource(
 *     attributes={"normalization_context"={"groups"={"order_read", "customer_read", "address_read"}}, "force_eager"=false}
 * )
 * @ORM\Entity
 * @ORM\Table(name="`order`")
 */
class Order
{
}

[errogaht]

$queryBuilder
        ->select('o, room_a1, floor_a2, cookRooms_a3, floor_a7, cookRooms_a8')
        ->where($queryBuilder->expr()->eq('o.id', ':id_id'))

        ->leftJoin('o.room', 'room_a1')
        ->leftJoin('room_a1.floor', 'floor_a2')
        ->leftJoin('floor_a2.cookRooms', 'cookRooms_a3')

        ->leftJoin('o.floor', 'floor_a7')
        ->leftJoin('floor_a7.cookRooms', 'cookRooms_a8')

        ->setParameter('id_id', '123')
    ;

this is the query what EagerLoadingExtension generating.
after that Floor entity have 2 elements in cookRooms collection and they are the same object