Core: Filtering by a relation's property

[ymarillet]

Hi,

This is affecting version 2.0.1 to current (2.0.3)

Having the following entities:

<?php

namespace AppBundle\Entity;

use ApiPlatform\Core\Annotation\ApiResource;
use Doctrine\Common\Collections\ArrayCollection;
use Doctrine\ORM\Mapping as ORM;
use Symfony\Component\Validator\Constraints as Assert;
use Symfony\Component\Serializer\Annotation as Serializer;

/**
 * @ApiResource(
 *     attributes={
 *          "normalization_context"={"groups"={"foo"}},
 *          "filters"={"foo.search_filter"}
 *     }
 * )
 * @ORM\Entity
 */
class Foo
{
    /**
     * @var int The entity Id
     *
     * @ORM\Id
     * @ORM\GeneratedValue
     * @ORM\Column(type="integer")
     */
    private $id;

    /**
     * @var string Something else
     *
     * @ORM\OneToMany(targetEntity="Bar", mappedBy="foo")
     *
     * @Serializer\Groups({"foo"})
     */
    private $bars;

    public function __construct()
    {
        $this->bars = new ArrayCollection();
    }

    public function getId()
    {
        return $this->id;
    }

    /**
     * @return string
     */
    public function getBars()
    {
        return $this->bars;
    }

    /**
     * @param string $bars
     * @return static
     */
    public function setBars($bars)
    {
        $this->bars = $bars;
        return $this;
    }
}

<?php

namespace AppBundle\Entity;

use ApiPlatform\Core\Annotation\ApiResource;
use Doctrine\ORM\Mapping as ORM;
use Symfony\Component\Serializer\Annotation as Serializer;
use Symfony\Component\Validator\Constraints as Assert;

/**
 * @ApiResource
 * @ORM\Entity
 */
class Bar
{
    /**
     * @var int The entity Id
     *
     * @ORM\Id
     * @ORM\GeneratedValue
     * @ORM\Column(type="integer")
     */
    private $id;

    /**
     * @var Foo
     *
     * @ORM\ManyToOne(targetEntity="Foo", inversedBy="bars")
     * @ORM\JoinColumn(nullable=false, onDelete="CASCADE")
     * @Assert\NotBlank
     */
    private $foo;

    /**
     * @var string
     *
     * @ORM\Column(nullable=false)
     * @Assert\NotBlank
     *
     * @Serializer\Groups({"foo"})
     */
    private $prop = '';

    public function getId()
    {
        return $this->id;
    }

    /**
     * @return Foo|null
     */
    public function getFoo()
    {
        return $this->foo;
    }

    /**
     * @param Foo $foo
     * @return static
     */
    public function setFoo(Foo $foo)
    {
        $this->foo = $foo;
        return $this;
    }

    /**
     * @return string
     */
    public function getProp()
    {
        return $this->prop;
    }

    /**
     * @param string $prop
     * @return static
     */
    public function setProp($prop)
    {
        $this->prop = $prop;
        return $this;
    }
}

Services defined as:

services:
  app.entity.filter.foo:
      parent:    'api_platform.doctrine.orm.search_filter'
      arguments:
          - { bars.prop: 'ipartial' }
      tags:
          - { name: 'api_platform.filter', id: 'foo.search_filter' }

Having the following data:

AppBundle\Entity\Foo:
  foo1:
    id: 1
    bars: ["@bar1", "@bar2"]

AppBundle\Entity\Bar:
  bar1:
    id: 1
    prop: "toto"
    foo: "@foo1"
  bar2:
    id: 2
    prop: "tata"
    foo: "@foo1"

Having set the proper filters in order to run that simple query:
http://localhost/foos?bar.prop=toto

Will result:

{
  "@context": "/contexts/Foo",
  "@id": "/foos",
  "@type": "hydra:Collection",
  "hydra:member": [
    {
      "@id": "/foos/1",
      "@type": "Foo",
      "bars": [
        {
          "@id": "/bars/1",
          "@type": "Bar",
          "prop": "toto"
        }
      ]
    }
  ],
  "hydra:totalItems": 1,
  "...otherProps...": "...otherValues...."
}

I understand that the second Bar attached to foo1 gets filtered through the doctrine query, but since we're on the Foo endpoint:

• is it still a valid behaviour ?
• if so, how to tell api-platform/doctrine to fetch all relations upon filtering on them ?

Regards,
Yohann

[teohhanhui]

Filtering does not affect normalization. See https://api-platform.com/docs/core/serialization-groups-and-relations#embedding-relations

[ymarillet]

@teohhanhui I appreciate your quickness on this issue, however, it does not answer me at all. Are you saying that it's normal that the 2nd bar entity does not appear in the response ?

[dunglas]

IMO there is a problem: the list of foo.bars should not be filtered. The expected behavior is:

• Return only foos containing a bar with prop=toto
• But return the full list of bars for this foo

[soyuka]

@ymarillet is it possible for you to get the DQL from the profiler? Thanks!

[teohhanhui]

Uhh, sorry about that. I overlooked.

[ymarillet]

@teohhanhui no problem ;-)

@soyuka hmm I've only got sql in the profiler, any tip to show dql there ?

SELECT 
  f0_.id AS id_0, 
  b1_.id AS id_1, 
  b1_.prop AS prop_2, 
  b1_.foo_id AS foo_id_3 
FROM 
  foo f0_ 
  LEFT JOIN bar b1_ ON f0_.id = b1_.foo_id 
WHERE 
  LOWER(b1_.prop) LIKE LOWER('%' || 'toto' || '%') 
  AND f0_.id IN ('1')

[ymarillet]

I've also updated my original post in order to include normalization groups (so I posted more config/entities), without that it works like a charm

[soyuka]

Fine like this. This query will get only bars having the prop LIKE toto. To me, this is the intended behavior for the current filter implementation.

• Return only foos containing a bar with prop=toto
• But return the full list of bars for this foo

For this to work we need more work as we first need to find ids that have a such relation, and then do a SELECT IN (subquery) or something like that. Anyway, not an easy task, nor currently supported.

[ymarillet]

I think the problem got introduced in 2.0.1 with the use of the DQL "partial" keyword (EagerLoadingExtension:183). Would it be a pain to make it revertable as per the behaviour of 2.0.0 with a feature flag ?

[teohhanhui]

I'd consider it a bug. Partial embedded relation is unacceptable.

Let's revert the commit which is the culprit if possible.

On Thu, Feb 16, 2017 at 12:28 AM, ymarillet [e-mail]
wrote:

I think the problem got introduced in 2.0.1 with the use of the DQL
"partial" keyword (EagerLoadingExtension:183). Would it be a pain to make
it revertable as per the behaviour of 2.0.0 with a feature flag ?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/api-platform/core/issues/944#issuecomment-280060262,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAhf6zr8C8QqFbEdpd13RaxwZdfgIzmdks5rcyeogaJpZM4MBxr_
.

[soyuka]

I'm 👎 to revert. I'd prefer if we either make it customizable or find a clean fix. I'll think about this.

Also, if I'm filtering on bars.prop I don't see the point in getting the bars that doesn't meet the filter requirements.

[dunglas]

I agree with @teohhanhui, this behavior is weird. Filtering the root collection should not impact embedded documents.

[teohhanhui]

Not returning other members of the relation is wrong. The test should be
whether you can PUT it back the same way you received it without any
apparent change of state (PUT is idempotent). The current behavior clearly
fails that test. It's a bug that should be fixed and a regression test
added.

On 16 Feb 2017 07:25, "Kévin Dunglas" [e-mail] wrote:

I agree with @teohhanhui https://github.com/teohhanhui, this behavior
is weird. Filtering the root collection should not impact embedded
documents.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/api-platform/core/issues/944#issuecomment-280175173,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAhf63TPLJwqFAnYCL0OXACscc3ORT2Bks5rc4l7gaJpZM4MBxr_
.

[soyuka]

I'm not sure that the partial thing has something to do with this as the query JOIN instruction will stay the same and will return only data that matches the WHERE clause.

If I understand this correctly, you want

http://localhost/foos?bar.prop=toto

to return

{
  "@context": "/contexts/Foo",
  "@id": "/foos",
  "@type": "hydra:Collection",
  "hydra:member": [
    {
      "@id": "/foos/1",
      "@type": "Foo",
      "bars": [
        {
          "@id": "/bars/1",
          "@type": "Bar",
          "prop": "toto"
        },
        {
          "@id": "/bars/2",
          "@type": "Bar",
          "prop": "tata"
        }
      ]
    }
  ],
  "hydra:totalItems": 1,
  "...otherProps...": "...otherValues...."
}

?

I'll investigate. Still, to me it's a really valid behavior.

[ymarillet]

Yep that's the expected behaviour

[soyuka]

@ymarillet I've reproduce the bug successfully :) (only with OneToMany, ManyToMany works).

Thanks for the test cases.

For now you can disable force_eager on the resource to keep the correct filtering behavior:

/**
 * @ApiResource(
 *     attributes={
 *          "normalization_context"={"groups"={"foo"}},
 *          "filters"={"foo.search_filter"},
 *          "force_eager"=false
 *     }
 * )
 * @ORM\Entity
 */
class Foo{}

---

To keep this behavior consistent with Eager loading, we need to change how the filter behaves. Without fetch eager doctrine will do the following:

1. Fetch the Foos where Bar matches the requested parameter

SELECT b0_.id AS ID_0 FROM bdr_Foo b0_ INNER JOIN bdr_Bar b1_ ON b0_.id = b1_.foo_id WHERE LOWER(b1_.prop) LIKE LOWER('%' || ? || '%') ORDER BY b0_.id ASC
Parameters: [0 => toto]
1.36 ms 

Then fetch every previously given id one by one:

SELECT t0.id AS ID_1, t0.prop AS PROP_2, t0.foo_id AS FOO_ID_3 FROM bdr_Bar t0 WHERE t0.foo_id = ?
Parameters: [0 => 1]
0.67 ms 

SELECT t0.id AS ID_1, t0.prop AS PROP_2, t0.foo_id AS FOO_ID_3 FROM bdr_Bar t0 WHERE t0.foo_id = ?
Parameters: [0 => 3]
0.56 ms 

Whereas with the eager loading, it will fetch that first request and deal with it:

SELECT b0_.id AS ID_0, b1_.id AS ID_1, b1_.prop AS PROP_2, b1_.foo_id AS FOO_ID_3 FROM bdr_Foo b0_ LEFT JOIN bdr_Bar b1_ ON b0_.id = b1_.foo_id WHERE LOWER(b1_.prop) LIKE LOWER('%' || ? || '%') ORDER BY b0_.id ASC
Parameters: [0 => toto]

0.98 ms 

The behavior is NOT consistent and I agree it's a bug. Still, performances without eager loading are really bad. Here there are only 2 fetched items, and we already observe a 2ms decrease.

Note that to me this is also a feature in term of performances and in term of results. Why would one want the relation filtered results to show up? Anyway I think I'm alone in this so nevermind :smile:.

A proper resolution (in term of performances and result) would end up querying something like this:

SELECT b3.*, f.* FROM (
 SELECT b1_.FOO_ID FROM BDR_BAR b1_ WHERE LOWER(b1_.prop) LIKE LOWER('%' || :foo || '%')
) b2 
INNER JOIN BDR_FOO f ON f.id = b2.foo_id
INNER JOIN BDR_BAR b3 ON b3.foo_id = b2.foo_id;

I'm not sure yet how nor where I could implement this.

[teohhanhui]

The filters should not have to deal with / know about eager loading.

[soyuka]

Indeed they should not, but in this particular case I have no clue on how it could be implemented easily.

You don't agree that both behavior can be useful? To me they both make sense:

• Return only foos containing a bar with prop=toto and their bars having prop=toto
• Return only foos containing a bar with prop=toto and their full bars

[dunglas]

It may be useful, but it shouldn't be the default behavior (it should be enabled by the user explicitly, and probably client-side).

[ymarillet]

@soyuka Thanks for the lightning-fast investigation - as you said performance without eager loading is not exceptional, so for now we will keep up with 2.0.0 until the proper fix is released ;-)

[teohhanhui]

Do you think you could pinpoint which commit between 2.0.0 and 2.0.1 caused
this regression?

On 16 Feb 2017 22:28, "ymarillet" [e-mail] wrote:

@soyuka https://github.com/soyuka Thanks for the lightning-fast
investigation - as you said performance without eager loading is not
exceptional, so for now we will keep up with 2.0.0 until the proper fix is
released ;-)

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
https://github.com/api-platform/core/issues/944#issuecomment-280344583,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAhf61oPWe9B2bXYgaUaNHE1FR-Ua9kRks5rdF0igaJpZM4MBxr_
.

[ymarillet]

57afc29 through 25c6a2d

[soyuka]

Nope, it's not the partial selection, I just tried and the bug is still the same.

My guess is that this behavior was already the same when EagerLoading was not enabled by default, and therefore we didn't really see it. Also, we've no tests covering that specific use case.

I'd say that it's since we enabled it by default, before that everything was lazy loaded anyway.

[teohhanhui]

Was the default for eager loading changed between 2.0.0 and 2.0.1? That
seems like a change unfit for a patch release.

On Thu, 16 Feb 2017, 22:42 Antoine Bluchet, [e-mail]
wrote:

Nope, it's not the partial selection, I just tried and the bug is still
the same.

My guess is that this behavior was already the same when EagerLoading was
not enabled by default, and therefore we didn't really see it. Also, we've
no tests covering that specific use case.

I'd say that it's since we enabled it by default, before that everything
was lazy loaded anyway.

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
https://github.com/api-platform/core/issues/944#issuecomment-280348466,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAhf6-NEXwO1SLlpmVPKJ3yOQLiT__Piks5rdGBfgaJpZM4MBxr_
.

[ymarillet]

that's even stranger then, because if you remove the normalization groups on the Foo entity, the result will be:

{
  "@context": "/contexts/Foo",
  "@id": "/foos",
  "@type": "hydra:Collection",
  "hydra:member": [
    {
      "@id": "/foos/1",
      "@type": "Foo",
      "bars": [
        "/bars/1",
        "/bars/2"
      ]
    }
  ],
  "hydra:totalItems": 1,
  "...otherProps...": "...otherValues...."
}

[soyuka]

Was the default for eager loading changed between 2.0.0 and 2.0.1?

4d6c532733f8c1f0a14f5f685a520052f015e4d6 ?

Here without groups it doesn't eager load at all, and falls back to lazy loading.

[ymarillet]

~nope, actually it's 25c6a2d~

[soyuka]

I mean were the eager loading has been set to be the default behavior. 25c6a2d just adds back the partial select.

[ymarillet]

oh ok, nevermind

But to answer @teohhanhui on the particular commit that caused the regression, I've tested the request on all the commits between v2.0.0 and v2.0.1 using git checkout HEAD^ on the api-platform/core vendor and the difference appear exactly on this commit 25c6a2d (updated my first comment above)

Re-update: 57afc29 was still the culprit since 25c6a2d is a merge commit

[soyuka]

Yes this is the one.

For example with current code I need to comment this part.
Then, you've the full relation, but then again it just lazy loads and it'd be the same as using "force_eager"=false.

If we want to revert that partial select commit, we'll end up with something like:

                $queryBuilder->addSelect($associationAlias);

Which you can leave uncommented here but comment 148-158. It'd give us the exact same results as with the partial data (ie one query but only filtered relations).

[ymarillet]

so it means eager loading is not working properly on 2.0.0 ?

[soyuka]

If in 2.0.0 8efed0bf3dfe7afd6bbed2d1ca83b60c436c2a60 was effective, indeed it'd still be lazy loading, and so the eager loading was not working properly.

I think this is why I asked to revert it.

Anyway I'm close to a fix for this but I just don't like the way I've fixed it.

[soyuka]

@teohhanhui 2.0.0 was released with EagerLoading by default it just wasn't working properly
@ymarillet you should be fine using 2.0.1/2.0.2 as EagerLoading is working sweet except for this bug ofc. For this entity just disable EagerLoading, it'll be the same as with 2.0.0

[teohhanhui]

If in 2.0.0 8efed0b was effective, indeed it'd still be lazy loading, and so the eager loading was not working properly.

I don't understand why partial select is necessary for eager loading? Aren't the joins enough? Without partial select this bug would not manifest itself, because Doctrine should be smart enough to load the missing relations, right?

[teohhanhui]

We'd need to check if my hypothesis is correct, but if I understand it correctly:

• when you fetch join related entities, Doctrine would keep those loaded entities in its identity map
• but the collection would not be initialized (because otherwise the result may be wrong)

[soyuka]

The alternative to partial select is to use the full association alias in
the select, which will end up the same for this particular bug. Without any
select, doctrine will do lazy loading even though there are relations.
On Fri, 17 Feb 2017 at 16:46, Teoh Han Hui [e-mail] wrote:

If in 2.0.0 8efed0b
https://github.com/api-platform/core/commit/8efed0bf3dfe7afd6bbed2d1ca83b60c436c2a60
was effective, indeed it'd still be lazy loading, and so the eager loading
was not working properly.

I don't understand why partial select is necessary for eager loading?
Aren't the joins enough? Without partial select this bug would not manifest
itself, because Doctrine should be smart enough to load the missing
relations, right?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/api-platform/core/issues/944#issuecomment-280685157,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABQr8x0727B7TSEd9EJselsa0rB4AG_cks5rdcDDgaJpZM4MBxr_
.

[teohhanhui]

Okay, I get what you mean now. There will be additional queries just as before, so the eager loading is ineffective.

[teohhanhui]

What we need is something very similar to the Doctrine Paginator. (Actually, is it possible for us to use it directly? If not, I think this is something that should - at least eventually - belong in the Doctrine ORM project.)

At a glance I think you use the same concept in #950, yes?

[teohhanhui]

Oh wait. You use a subquery, while the Doctrine Paginator uses an additional query.

[soyuka]

Exact! Additional Query may work too !
On Fri, 17 Feb 2017 at 16:58, Teoh Han Hui [e-mail] wrote:

Oh wait. You use a subquery, while the Doctrine Paginator uses an
additional query.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/api-platform/core/issues/944#issuecomment-280688605,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABQr81hy951m9J5Li2j_wH2z--hhTK59ks5rdcOXgaJpZM4MBxr_
.

[DariuszLuber]

Hi.

I'm trying to get what @ymarillet had at the begining, I want to filter data by nested values, and don't want to get nested values, which not meet my filter requirements.

I tried with "force_eager"=false and "force_eager"=true and not working as I want.

I have 3 Entities: DrugsUnified which is connected OneToMany to Drugs and Drugs are connecter ManyToOne to DrugStore.

I wan't to filter by drug.drugstore.id and get only DrugsUnified with only drugs from selected DrugStore.

I tried with build in filters, which I enabled:

drug_unified.search_filter: parent: 'api_platform.doctrine.orm.search_filter' arguments: [ {drugs.drugstore.id: 'exact'} ] tags: [ { name: 'api_platform.filter', id: 'drug_unified.search' } ]

Any sugestion how can I get what I want? I always get all drugs from all drugstores.

[soyuka]

Not sure I understood correctly, please open a new issue. Also, check out Subresources (declares a route to a collection belonging to an item).