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

[soyuka]

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]

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]

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]

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

[soyuka]

@teohhanhui suggestions about that parameter name ? \o/

[teohhanhui]

fetch_partial?

[soyuka]

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

[teohhanhui]

Yes. So the default should be false.

Or perhaps partial_relations, partial_collections, partial_collection_fetch?

[teohhanhui]

Remind me to rewrite this for you. 😄

[soyuka]

!RemindMe

[teohhanhui]

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?

[teohhanhui]

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

[soyuka]

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

[teohhanhui]

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.

[teohhanhui]

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.")

[soyuka]

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

[teohhanhui]

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

[soyuka]

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]

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]

@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]

ping @api-platform/core-team

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

[teohhanhui]

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

[dunglas]

Thank you very much @soyuka

[vasilvestre]

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

[soyuka]

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

Nope.

[qDL7]

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