Fix #944 - allows to filter by relation by keeping EagerLoading enabled #950

soyuka · 2017-02-16T17:29:14Z

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

/!\ This is probably the worse code I've ever produced here and I hope we can find how to improve it together.

To fix #944 I needed to alter the query generated by the filters and the eager loading extension. To do so, I've added a FilterEagerLoadingExtension that patches the Query generated by the FilterExtension.

It runs directly after the FilterExtension and transforms the query so that it ends up doing something like:

SELECT o, partial bars_a1_p.{id,prop} 
FROM ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\Foo o 
LEFT JOIN o.bars bars_a1_p 
WHERE o IN(
  SELECT p 
  FROM ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\Foo p 
  LEFT JOIN p.bars bars_a1 
  WHERE LOWER(bars_a1.prop) LIKE LOWER(CONCAT('%', :prop_p1, '%'))
)

TODO:

Rename test class Foo/Bar too generic
Unit tests
Allow the user to skip this extension so that we can get the relation items filtered (previous behavior that is considered as a bug)
Prevent the extension from running when forceEager is not set
Performances might be improved slightly by selecting the wanted id with the were clause (removes the JOIN inside the IN and select from that entity directly, but it's complex)
Test OneToMany
Test ManyToMany

~~Apologizes to Scrutinizer, that complexity index will be very high -_-.~~

soyuka · 2017-02-21T16:32:23Z

Query is now better:

SELECT o, partial bars_a1.{id,prop} FROM ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\Foo o
LEFT JOIN o.bars bars_a1 
WHERE o IN(
    SELECT IDENTITY(bars_a1_a2.foo) 
    FROM ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\Bar bars_a1_a2 
    WHERE LOWER(bars_a1_a2.prop) LIKE LOWER(CONCAT('%', :prop_p1, '%'))
)

This will work with the PaginationExtension as it doesn't alter the results, and it is optimized.

soyuka · 2017-02-23T11:28:23Z

WDYT? I'll add unit tests but I want to know first if this solution is fine to you or if you want to try another approach :).

@api-platform/core-team

dunglas · 2017-02-24T16:51:10Z

Looks better, indeed. @ymarillet can you try if this patch fix the problem?

soyuka · 2017-02-24T17:01:52Z

src/Bridge/Symfony/Bundle/DependencyInjection/ApiPlatformExtension.php

@@ -94,6 +94,7 @@ private function handleConfig(ContainerBuilder $container, array $config, array
        $container->setParameter('api_platform.eager_loading.enabled', $config['eager_loading']['enabled']);
        $container->setParameter('api_platform.eager_loading.max_joins', $config['eager_loading']['max_joins']);
        $container->setParameter('api_platform.eager_loading.force_eager', $config['eager_loading']['force_eager']);
+        $container->setParameter('api_platform.eager_loading.filter_relations', $config['eager_loading']['filter_relations']);


@teohhanhui suggestions about that parameter name ? \o/

fetch_partial?

fetch_partial to true would mean we want to fetch a partial relation?

Yes. So the default should be false.

Or perhaps partial_relations, partial_collections, partial_collection_fetch?

teohhanhui · 2017-02-24T17:08:28Z

src/Bridge/Symfony/Bundle/DependencyInjection/Configuration.php

@@ -49,6 +49,7 @@ public function getConfigTreeBuilder()
                        ->booleanNode('enabled')->defaultTrue()->info('To enable or disable eager loading')->end()
                        ->integerNode('max_joins')->defaultValue(30)->info('Max number of joined relations before EagerLoading throws a RuntimeException')->end()
                        ->booleanNode('force_eager')->defaultTrue()->info('Force join on every relation. If disabled, it will only join relations having the EAGER fetch mode.')->end()
+                        ->booleanNode('filter_relations')->defaultFalse()->info('Allow to filter the relation collection when the filter is on a nested property. By default, it will retrieve the full relation of the filtered resource. If enabled, only relations matching the filter will be returned.')->end()


Remind me to rewrite this for you. 😄

When filtering on a nested property through a to-many relation, do not fetch the other items in the collection. Warning: this breaks round trip, as you cannot PUT the returned collection back to the parent property.

p/s: @soyuka Does this only happen when filtering on a nested property? Or whenever the other side of the relation is fetch-joined?

Also, we need to warn that this would apply to every level in a multi-level nested property.

Only when filtering on a nested property because it only joins the result matching that filter.

When filtering by e.g. products?brands=/brands/1, doesn't it result in SQL something like:

SELECT ... FROM products INNER JOIN products_brands ON products.id = products_brands.product_id INNER JOIN brands ON products_brands.brand_id = brands.id WHERE brands.id = 1

and thus:

{ "@id": "/products/1", "brands": [ "/brands/1" ] }

even if product 1 has other brands?

This is not a nested property. So I think the problem manifests itself when filtering on any to-many relation.

I really think we should not have this option, because it results in returning invalid data. ("Product 1 has 1 brand which is Brand 1.")

Not sure I understood, what is the relation type on products_brands and brands?

Many-to-many. products_brands is the join table.

Yeah your right I think that this option is too much confusing, ManyToMany relations are working fine, I've added a test case anyway.

I'm removing the availability to filter on the relation. I think that the best to filter a relation would be to use subresource + filter.

soyuka · 2017-02-24T17:10:21Z

Haha I will <3

…

On Fri, 24 Feb 2017 at 18:08, Teoh Han Hui ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/Bridge/Symfony/Bundle/DependencyInjection/Configuration.php <#950 (comment)>: > @@ -49,6 +49,7 @@ public function getConfigTreeBuilder() ->booleanNode('enabled')->defaultTrue()->info('To enable or disable eager loading')->end() ->integerNode('max_joins')->defaultValue(30)->info('Max number of joined relations before EagerLoading throws a RuntimeException')->end() ->booleanNode('force_eager')->defaultTrue()->info('Force join on every relation. If disabled, it will only join relations having the EAGER fetch mode.')->end() + ->booleanNode('filter_relations')->defaultFalse()->info('Allow to filter the relation collection when the filter is on a nested property. By default, it will retrieve the full relation of the filtered resource. If enabled, only relations matching the filter will be returned.')->end() Remind me to rewrite this for you. 😄 — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#950 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABQr82anStSF9DznfHu8OFAzADViAO3cks5rfw6mgaJpZM4MDT7B> .

soyuka · 2017-03-02T09:35:55Z

@teohhanhui I refactored the fix, it's much cleaner and simple now. It clones the query and uses a subquery with the where clause to find the root entity identifiers.

Origin query:

SELECT o, partial relatedToDummyFriend_a1.{name,dummyFriend,relatedDummy}, partial dummyFriend_a2.{id,name} 
FROM ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\RelatedDummy o 
LEFT JOIN o.relatedToDummyFriend relatedToDummyFriend_a1 
LEFT JOIN relatedToDummyFriend_a1.dummyFriend dummyFriend_a2 
WHERE dummyFriend_a2.id = :dummyFriend_p1

Fixed query:

SELECT o, partial relatedToDummyFriend_a1.{name,dummyFriend,relatedDummy}, partial dummyFriend_a2.{id,name} 
FROM ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\RelatedDummy o 
LEFT JOIN o.relatedToDummyFriend relatedToDummyFriend_a1 
LEFT JOIN relatedToDummyFriend_a1.dummyFriend dummyFriend_a2 
WHERE o IN(
  SELECT o_2 FROM ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\RelatedDummy o_2 
  LEFT JOIN o_2.relatedToDummyFriend relatedToDummyFriend_a1_a3 
  LEFT JOIN relatedToDummyFriend_a1_a3.dummyFriend dummyFriend_a2_a4 
  WHERE dummyFriend_a2_a4.id = :dummyFriend_p1
)

This will work with any relation (manytomany, manytoone etc.). I've removed the parameter to skip this extension.

soyuka · 2017-03-11T11:07:00Z

ping @api-platform/core-team

I think that this (as it's a bug fix) should target 2.0. Lmk.

teohhanhui · 2017-03-13T09:56:56Z

Yes, this is a bug fix. 2.0 it should be then.

dunglas · 2017-03-20T11:08:56Z

Thank you very much @soyuka

Fix api-platform#944 - allows to filter by relation by keeping EagerLoading enabled

vasilvestre · 2019-05-15T10:23:31Z

filter_relations parameter seem not documented, it's still exist ?

soyuka · 2019-05-15T11:52:37Z

filter_relations parameter seem not documented, it's still exist ?

Nope.

qDL7 · 2020-02-05T12:55:51Z

is it possible then to filter relations without a custom filters? @soyuka

teohhanhui mentioned this pull request Feb 17, 2017

Filtering by a relation's property #944

Closed

soyuka force-pushed the reproduce-attempt-944 branch 2 times, most recently from 65686a3 to f220063 Compare February 21, 2017 16:30

soyuka force-pushed the reproduce-attempt-944 branch from f220063 to 105b6b2 Compare February 23, 2017 11:26

soyuka requested a review from teohhanhui February 24, 2017 14:22

soyuka commented Feb 24, 2017

View reviewed changes

teohhanhui reviewed Feb 24, 2017

View reviewed changes

soyuka force-pushed the reproduce-attempt-944 branch from 105b6b2 to f06a3a7 Compare March 2, 2017 09:18

soyuka force-pushed the reproduce-attempt-944 branch 3 times, most recently from 8cdbec9 to 36c1ad5 Compare March 2, 2017 15:45

soyuka added bug [RFR] Ready for Review labels Mar 11, 2017

soyuka force-pushed the reproduce-attempt-944 branch 2 times, most recently from fb1d84d to d03d4fe Compare March 11, 2017 10:32

soyuka force-pushed the reproduce-attempt-944 branch from d03d4fe to 167e3be Compare March 13, 2017 10:06

soyuka changed the base branch from master to 2.0 March 13, 2017 10:07

Fix filter Eager Loading api-platform#944

495ea9e

soyuka force-pushed the reproduce-attempt-944 branch from 167e3be to 495ea9e Compare March 13, 2017 10:08

dunglas merged commit 603ad01 into api-platform:2.0 Mar 20, 2017

jsamouh mentioned this pull request Mar 20, 2017

Do not expose sensitive information #978

Merged

antograssiot mentioned this pull request Mar 24, 2017

Avoid to truncate WHERE clauses when clonning the querybuilder #1009

Closed

teohhanhui mentioned this pull request Mar 30, 2017

Remove FilterEagerLoadingExtension when eager loading is disabled #1020

Merged

soyuka deleted the reproduce-attempt-944 branch March 30, 2017 06:32

soyuka mentioned this pull request Jul 18, 2017

Filtering by nested entity's id leads to an unnecessary join #1250

Closed

hoangnd25 pushed a commit to hoangnd25/core that referenced this pull request Feb 23, 2018

Merge pull request api-platform#950 from soyuka/reproduce-attempt-944

d4a0e4e

Fix api-platform#944 - allows to filter by relation by keeping EagerLoading enabled

Uh oh!

Fix #944 - allows to filter by relation by keeping EagerLoading enabled #950

Fix #944 - allows to filter by relation by keeping EagerLoading enabled #950

Uh oh!

Conversation

soyuka commented Feb 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

soyuka commented Feb 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

soyuka commented Feb 23, 2017

Uh oh!

dunglas commented Feb 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

teohhanhui Feb 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

teohhanhui Feb 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soyuka commented Feb 24, 2017 via email

Uh oh!

soyuka commented Mar 2, 2017

Uh oh!

soyuka commented Mar 11, 2017

Uh oh!

teohhanhui commented Mar 13, 2017

Uh oh!

dunglas commented Mar 20, 2017

Uh oh!

vasilvestre commented May 15, 2019

Uh oh!

soyuka commented May 15, 2019

Uh oh!

qDL7 commented Feb 5, 2020

Uh oh!

Uh oh!

soyuka commented Feb 16, 2017 •

edited

Loading

soyuka commented Feb 21, 2017 •

edited

Loading

teohhanhui Feb 27, 2017 •

edited

Loading

teohhanhui Feb 27, 2017 •

edited

Loading