-
-
Notifications
You must be signed in to change notification settings - Fork 966
Fix duplicated eager loading joins #3525
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
Merged
alanpoulain
merged 1 commit into
api-platform:2.6
from
julienfalque:eager-loading-extension-priority
Mar 9, 2021
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,80 @@ | ||
| @!mongodb | ||
| Feature: Eager Loading | ||
| In order to have better performance | ||
| As a client software developer | ||
| The eager loading should be enabled | ||
|
|
||
| @createSchema | ||
| Scenario: Eager loading for a relation | ||
| Given there is a RelatedDummy with 2 friends | ||
| When I send a "GET" request to "/related_dummies/1" | ||
| And the response status code should be 200 | ||
| And the DQL should be equal to: | ||
| """ | ||
| SELECT o, thirdLevel_a1, relatedToDummyFriend_a2, dummyFriend_a3 | ||
| FROM ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\RelatedDummy o | ||
| LEFT JOIN o.thirdLevel thirdLevel_a1 | ||
| LEFT JOIN o.relatedToDummyFriend relatedToDummyFriend_a2 | ||
| LEFT JOIN relatedToDummyFriend_a2.dummyFriend dummyFriend_a3 | ||
| WHERE o.id = :id_id | ||
| """ | ||
|
|
||
| Scenario: Eager loading for the search filter | ||
| Given there is a dummy object with a fourth level relation | ||
| When I send a "GET" request to "/dummies?relatedDummy.thirdLevel.level=3" | ||
| Then the response status code should be 200 | ||
| And the DQL should be equal to: | ||
| """ | ||
| SELECT o | ||
| FROM ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\Dummy o | ||
| 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 | ||
| 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 | ||
| """ | ||
|
|
||
| Scenario: Eager loading for a relation and a search filter | ||
| Given there is a RelatedDummy with 2 friends | ||
| When I send a "GET" request to "/related_dummies?relatedToDummyFriend.dummyFriend=2" | ||
| And the response status code should be 200 | ||
| And the DQL should be equal to: | ||
| """ | ||
| SELECT o, thirdLevel_a4, relatedToDummyFriend_a1, dummyFriend_a5 | ||
| FROM ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\RelatedDummy o | ||
| INNER JOIN o.relatedToDummyFriend relatedToDummyFriend_a1 | ||
| LEFT JOIN o.thirdLevel thirdLevel_a4 | ||
| INNER JOIN relatedToDummyFriend_a1.dummyFriend dummyFriend_a5 | ||
| WHERE o IN( | ||
| SELECT o_a2 | ||
| FROM ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\RelatedDummy o_a2 | ||
| INNER JOIN o_a2.relatedToDummyFriend relatedToDummyFriend_a3 | ||
| WHERE relatedToDummyFriend_a3.dummyFriend = :dummyFriend_p1 | ||
| ) | ||
| ORDER BY o.id ASC | ||
| """ | ||
|
|
||
| Scenario: Eager loading for a relation with complex sub-query filter | ||
| Given there is a RelatedDummy with 2 friends | ||
| When I send a "GET" request to "/related_dummies?complex_sub_query_filter=1" | ||
| Then the response status code should be 200 | ||
| And the DQL should be equal to: | ||
| """ | ||
| SELECT o, thirdLevel_a3, relatedToDummyFriend_a4, dummyFriend_a5 | ||
| FROM ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\RelatedDummy o | ||
| LEFT JOIN o.thirdLevel thirdLevel_a3 | ||
| LEFT JOIN o.relatedToDummyFriend relatedToDummyFriend_a4 | ||
| LEFT JOIN relatedToDummyFriend_a4.dummyFriend dummyFriend_a5 | ||
| WHERE o.id IN ( | ||
| SELECT related_dummy_a1.id | ||
| FROM ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\RelatedDummy related_dummy_a1 | ||
| INNER JOIN related_dummy_a1.relatedToDummyFriend related_to_dummy_friend_a2 | ||
| WITH related_to_dummy_friend_a2.name = :name_p1 | ||
| ) | ||
| ORDER BY o.id ASC | ||
| """ |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
EagerLoadingExtensionapplied afterFilterEagerLoadingExtensionto fix #3487. I don't thinkFilterEagerLoadingExtension's priority can be changed (it has to be applied after all filters).If I remember correctly, the changes in this PR make
EagerLoadingExtensionable 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?There was a problem hiding this comment.
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.