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

Fix duplicated eager loading joins #3525

Merged

Conversation

julienfalque
Copy link
Contributor

@julienfalque julienfalque commented Apr 24, 2020

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tickets #3487
License MIT
Doc PR -

@soyuka
Copy link
Member

soyuka commented Apr 28, 2020

This looks good according to the tests, do you think that there's a way to add a non-regression test?

@julienfalque julienfalque force-pushed the eager-loading-extension-priority branch 2 times, most recently from 707d408 to 35c9ce6 Compare May 1, 2020 10:14
@julienfalque
Copy link
Contributor Author

I was not able to reproduce the issue I mentioned in #3487 yet, I need a more complex filter I think. For now, changing the priority results in:

 SELECT o, relatedDummy_a1
 FROM ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\Dummy o
-    LEFT JOIN o.relatedDummy relatedDummy_a1
-    LEFT JOIN relatedDummy_a1.thirdLevel thirdLevel_a2
+    INNER JOIN o.relatedDummy relatedDummy_a1
+    INNER JOIN relatedDummy_a1.thirdLevel thirdLevel_a2
 WHERE o IN(
         SELECT o_a3
         FROM ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\Dummy o_a3
-            LEFT JOIN o_a3.relatedDummy relatedDummy_a4
-            LEFT JOIN relatedDummy_a4.thirdLevel thirdLevel_a5
+            INNER JOIN o_a3.relatedDummy relatedDummy_a4
+            INNER JOIN relatedDummy_a4.thirdLevel thirdLevel_a5
         WHERE thirdLevel_a5.level = :level_p1
     )
 ORDER BY o.id ASC

which looks ok. What do you think about the current approach?

@julienfalque julienfalque force-pushed the eager-loading-extension-priority branch from 35c9ce6 to 91fbc79 Compare May 1, 2020 18:06
@julienfalque
Copy link
Contributor Author

Ok I reproduced the issue (see method testWithComplexFilterAndEagerLoading()). Without the changes the resulting DQL query would be:

SELECT o, relatedDummy_a1
FROM ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\Dummy o
    LEFT JOIN o.relatedDummy relatedDummy_a1
WHERE o IN(
        SELECT o_a7
        FROM ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\Dummy o_a7
            LEFT JOIN o_a7.relatedDummy relatedDummy_a8
        WHERE o_a7.id IN (
                SELECT dummy_a2.id
                FROM ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\Dummy dummy_a2
                    INNER JOIN dummy_a2.related_dummy related_dummy_a3
                        WITH related_dummy_a3.name = :name_p1
                        AND related_dummy_a3.dummyDate >= :dummy_date_p2
                    INNER JOIN dummy_a2.related_dummies related_dummies_a4
                        WITH related_dummies_a4.name = :name_p1
                        AND related_dummies_a4.dummyDate >= :dummy_date_p2
                    INNER JOIN dummy_a2.related_owned_dummy related_owned_dummy_a5
                        WITH related_owned_dummy_a5.name <= :name_p1
                    INNER JOIN dummy_a2.related_owning_dummy related_owning_dummy_a6
                        WITH related_owning_dummy_a6.name <= :name_p1
            )
    )
ORDER BY o.id ASC

The nested query in the first WHERE IN (...) is the same as the outer query. With the fix, this duplicate is removed:

SELECT o, relatedDummy_a6
FROM ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\Dummy o
    LEFT JOIN o.relatedDummy relatedDummy_a6
WHERE o.id IN (
        SELECT dummy_a1.id
        FROM ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\Dummy dummy_a1
            INNER JOIN dummy_a1.related_dummy related_dummy_a2
                WITH related_dummy_a2.name = :name_p1
                AND related_dummy_a2.dummyDate >= :dummy_date_p2
            INNER JOIN dummy_a1.related_dummies related_dummies_a3
                WITH related_dummies_a3.name = :name_p1
                AND related_dummies_a3.dummyDate >= :dummy_date_p2
            INNER JOIN dummy_a1.related_owned_dummy related_owned_dummy_a4
                WITH related_owned_dummy_a4.name <= :name_p1
            INNER JOIN dummy_a1.related_owning_dummy related_owning_dummy_a5
                WITH related_owning_dummy_a5.name <= :name_p1
    )
ORDER BY o.id ASC

@julienfalque julienfalque force-pushed the eager-loading-extension-priority branch 5 times, most recently from 3bb73a9 to 57c8e3c Compare May 8, 2020 17:52
@julienfalque
Copy link
Contributor Author

Behat failures seem unrelated.

@julienfalque julienfalque marked this pull request as ready for review May 8, 2020 18:18
@julienfalque julienfalque force-pushed the eager-loading-extension-priority branch from 57c8e3c to 54e5283 Compare June 10, 2020 18:06
@julienfalque julienfalque force-pushed the eager-loading-extension-priority branch 4 times, most recently from a933a88 to 18966be Compare November 2, 2020 07:52
@alanpoulain alanpoulain changed the base branch from 2.5 to 2.6 March 3, 2021 15:59
@alanpoulain alanpoulain force-pushed the eager-loading-extension-priority branch from 18966be to e5f3125 Compare March 3, 2021 16:00
@alanpoulain
Copy link
Member

alanpoulain commented Mar 3, 2021

Hello @julienfalque. Sorry for the time we took to review your PR.
I've replaced your integration tests with Behat ones, to make it easier to maintain.
However I haven't reproduced the testWithComplexFilterAndEagerLoading (can be seen in this commit: 18966be).
Could you explain to me what this test was doing and if we should keep it?

@alanpoulain alanpoulain force-pushed the eager-loading-extension-priority branch 4 times, most recently from f6d084b to 977ad06 Compare March 4, 2021 09:26
@julienfalque
Copy link
Contributor Author

@alanpoulain Thank you for tackling this. To be honest I don't remember exactly. I think this specific test case was the only one that actually reproduced #3487 (see #3525 (comment)) so it should be kept. I'll have a closer look as soon as I can.

@alanpoulain alanpoulain force-pushed the eager-loading-extension-priority branch 2 times, most recently from aaf901d to b0e827b Compare March 4, 2021 13:40
@alanpoulain
Copy link
Member

OK I've added it again then.

@alanpoulain
Copy link
Member

@soyuka could you take a look at it?

@alanpoulain alanpoulain force-pushed the eager-loading-extension-priority branch from b0e827b to a170059 Compare March 4, 2021 14:52
@julienfalque
Copy link
Contributor Author

I cannot approve my own PR but your changes look 👍, thanks!

@alanpoulain alanpoulain force-pushed the eager-loading-extension-priority branch from a170059 to ae973d3 Compare March 9, 2021 16:55
@alanpoulain alanpoulain merged commit 414351b into api-platform:2.6 Mar 9, 2021
@alanpoulain
Copy link
Member

Thank you 🙂

@julienfalque
Copy link
Contributor Author

Thank you @alanpoulain @soyuka.

@@ -131,7 +131,8 @@
<argument type="service" id="serializer.mapping.class_metadata_factory" />

<tag name="api_platform.doctrine.orm.query_extension.item" priority="-8" />
<tag name="api_platform.doctrine.orm.query_extension.collection" priority="-8" />
<!-- After filter_eager_loading -->
<tag name="api_platform.doctrine.orm.query_extension.collection" priority="-18" />
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this priority change breaks filters that rely on existing joins from the eager loading extension because now the eager loading runs after the filters.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

exmaple filter that now breaks.

protected function filterProperty(string $property, $value, QueryBuilder $queryBuilder, QueryNameGeneratorInterface $queryNameGenerator, string $resourceClass, string $operationName = null)
    {
        if ('searchTerm' !== $property || !in_array($resourceClass, [User::class, Employment::class]))
            return;

        $alias = $rootAlias = $queryBuilder->getRootAliases()[0];
        if (User::class !== $resourceClass) {
            $joins = $queryBuilder->getDQLPart("join");
            if (array_key_exists($rootAlias, $joins)) {
                /** @var \Doctrine\ORM\Query\Expr\Join $join */
                foreach ($joins[$rootAlias] as $join) {
                    if (sprintf('%s.user', $rootAlias) === $join->getJoin())
                        $alias = $join->getAlias();
                }
            }
        }

        $searchParam = $queryNameGenerator->generateParameterName('search');

        $conditions = [];
        $conditions[] = sprintf('%1$s.firstname LIKE :%2$s', $alias, $searchParam);
        $conditions[] = sprintf('%1$s.lastname LIKE :%2$s', $alias, $searchParam);
        $conditions[] = sprintf('CONCAT(%1$s.firstname, \' \', %1$s.lastname) LIKE :%2$s', $alias, $searchParam);
        $conditions[] = sprintf('%1$s.email LIKE :%2$s', $alias, $searchParam);

        $queryBuilder->andWhere(implode(' OR ', $conditions))
            ->setParameter($searchParam, $value . "%");
    }

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could workaround this issue by adding the join explicitly in the filter, but now we join the relation two times, one by the filter and another by the eager loading extension.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could workaround this issue by adding the join explicitly in the filter, but now we join the relation two times, one by the filter and another by the eager loading extension.

If the extension works correctly, the existing join is reused then.

@julienfalque do you think it's possible to solve the issue without changing the priority?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll have to check again, but as I remember this was required to prevent duplicating joins as described in #3487.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just had a look: changing this priority is required to get EagerLoadingExtension applied after FilterEagerLoadingExtension to fix #3487. I don't think FilterEagerLoadingExtension's priority can be changed (it has to be applied after all filters).

we could workaround this issue by adding the join explicitly in the filter, but now we join the relation two times, one by the filter and another by the eager loading extension.

If I remember correctly, the changes in this PR make EagerLoadingExtension able to rely on existing joins when possible instead of always adding its own. @zolex Are you sure you have duplicated joins if you add one in your filter?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just assumed it because in earlier Versions it happened. I will check the resulting queries tomorrow and let you know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants