SearchFilter and different identifier

[dannyvw]

API Platform version(s) affected: 3.1.13

Description

This PR (#5623) breaks our test suite because no where query is added to doctrine.

How to reproduce

The classes below should describe the issue. We are using uuids in API request and internal it uses ids.

use ApiPlatform\Metadata\ApiFilter;
use ApiPlatform\Metadata\ApiProperty;
use ApiPlatform\Metadata\ApiResource;
use ApiPlatform\Metadata\Get;
use ApiPlatform\Metadata\GetCollection;
use ApiPlatform\Doctrine\Orm\Filter\SearchFilter;
use Doctrine\Common\Collections\ArrayCollection;
use Doctrine\ORM\Mapping as ORM;
use Symfony\Component\Uid\Uuid;

#[ApiResource(
    operations: [
        new GetCollection(),
        new Get(),
    ],
)]
#[ApiFilter(SearchFilter::class, properties: ['groups' => 'exact'])]
#[ORM\Entity]
class User
{
    #[ApiProperty(readable: false, writable: false, identifier: false)]
    #[ORM\Id]
    #[ORM\Column(type: 'integer')]
    #[ORM\GeneratedValue(strategy: 'AUTO')]
    private int $id;

    #[ApiProperty(writable: false, identifier: true)]
    #[ORM\Column(type: 'uuid', unique: true)]
    private Uuid $uuid;

    #[ORM\ManyToMany(targetEntity: Group::class, inversedBy: 'users')]
    #[ORM\JoinColumn(name: 'user_id', referencedColumnName: 'id')]
    #[ORM\InverseJoinColumn(name: 'group_id', referencedColumnName: 'id')]
    private Collection $groups;
  
    public function __construct()
    {
        $this->uuid = Uuid::v4();
        $this->groups = new ArrayCollection();
    }
}

---

use ApiPlatform\Metadata\ApiProperty;
use ApiPlatform\Metadata\ApiResource;
use ApiPlatform\Metadata\Get;
use ApiPlatform\Metadata\GetCollection;
use Symfony\Component\Uid\Uuid;

#[ApiResource(
    operations: [
        new GetCollection(),
        new Get(),
    ],
)]
#[ORM\Entity]
class Group
{
    #[ApiProperty(readable: false, writable: false, identifier: false)]
    #[ORM\Id]
    #[ORM\Column(type: 'integer')]
    #[ORM\GeneratedValue(strategy: 'AUTO')]
    private int $id;

    #[ApiProperty(writable: false, identifier: true)]
    #[ORM\Column(type: 'uuid', unique: true)]
    private Uuid $uuid;

    public function __construct()
    {
        $this->uuid = Uuid::v4();
    }
}

The request we execute is "/api/v2/users?groups=/api/v2/groups/61817181-0ecc-42fb-a6e7-d97f2ddcb344"

Line https://github.com/api-platform/core/blob/main/src/Doctrine/Common/Filter/SearchFilterTrait.php#L133 now returns the uuid "61817181-0ecc-42fb-a6e7-d97f2ddcb344" and before we get the internal id value from https://github.com/api-platform/core/blob/main/src/Doctrine/Common/Filter/SearchFilterTrait.php#L128

This line https://github.com/api-platform/core/blob/main/src/Doctrine/Orm/Filter/SearchFilter.php#L225 returns the id field and will now return in this line https://github.com/api-platform/core/blob/main/src/Doctrine/Orm/Filter/SearchFilter.php#L233. So the where query (https://github.com/api-platform/core/blob/main/src/Doctrine/Orm/Filter/SearchFilter.php#L243) is not added anymore.

Possible Solution

Additional Context

[soyuka]

I was sure of that -_- every time we try to fix an issue on that class it breaks someone's code sorry about that... @mrossard do you know if there's an easy fix to that situation?

[mrossard]

I'll see if I can find a simple fix for this
@soyuka there was another PR regarding UUIDs in SearchFilter, started around the same time mine was merged, isn't it related too? #5618

[mrossard]

@dannyvw Would it be possible for you to add a more complete reproducer I could use?

[dannyvw]

@mrossard I've updated the code with some attributes, does this help you?

[mrossard]

Thanks, that should do the trick, i'll try to write a new test using this and work from there.

[mrossard]

@soyuka I think i've isolated the problem : in the default ItemProvider, a call with $context['fetch_data'] set to false calls manager->getReference() without checking if the uriVariables actually contain the ORM id.

@dannyvw Can you confirm that changing this line

core/src/Doctrine/Orm/State/ItemProvider.php

Line 55 in ebf0310

if (!$fetchData) {

to

if (!$fetchData && array_key_exists('id', $uriVariables)) {

fixes it for you?

If it does i'll try to work on a quick fix (as in, see if I can craft a better condition).

[dannyvw]

@mrossard No this does not work. $uriVariables contains the uuid value and not the id

[mrossard]

@mrossard No this does not work. $uriVariables contains the uuid value and not the id

Well that was the point - since you don't have the id in the uriVariables you can't just call getReference(). That's weird, my test seems to work just fine like that.
I'll start a PR as a draft so you can take a look and tell me what I'm not doing the same way you do!

[soyuka]

there is no 1-1 relation between uriVariables and doctrine identifiers, if you inspect @dannyvw there's no id within uriVariables.

A possible fix would be to check if there's an $item->getValue('id') https://github.com/api-platform/core/blob/main/src/Doctrine/Common/Filter/SearchFilterTrait.php#L128 we use that instead of the ones from the identifiers extractor. This is a hard problem to fix for everyone, I think that at some point we should only rely on uriVariables as it makes things hard not to...

[mrossard]

@dannyvw If you don't mind checking the draft i just pushed so i can understand what's different in you case? #5740

[soyuka]

you'll still not get that $item->getId value no?

[mrossard]

you'll still not get that $item->getId value no?

Instead of trying to get the reference to the resource with the id, this version just gets the full resource with the uuid, just like a simple GET on it.
But I forgot to try handling the UUID in hasValidValues(), the filter is actually ignored here. 😓

[dannyvw]

The ItemProvider return the correct group with or without this change. Also fetch_data is true on our side to fix some other issues with relations (discriminator mapping if i got it right). But setting this to false the ItemProvider still returns the correct group.

In the testcase from the draft pr all users have the same group right? Add a user with a different group and then it returns all users instead of the users with the specified group.

[mrossard]

The ItemProvider return the correct group with or without this change. Also fetch_data is true on our side to fix some other issues with relations (discriminator mapping if i got it right). But setting this to false the ItemProvider still returns the correct group.

In the testcase from the draft pr all users have the same group right? Add a user with a different group and then it returns all users instead of the users with the specified group.

Yes my change fixed (?) something else entirely, i'll rework asap.

[mrossard]

@dannyvw Can you please try the latest version of my draft on your case? It would still need some work, but it should be ok for the use case you described.

[dannyvw]

@mrossard Yes that fixes the issue, but now other tests fail.

We also have cases like this /api/v2/users?groups[]=/api/v2/groups/61817181-0ecc-42fb-a6e7-d97f2ddcb344&groups[]=/api/v2/groups/32510d53-f737-4e70-8d9d-58e292c871f8 and will trigger the following error Argument #1 ($iri) must be of type string, array given, called in vendor/api-platform/core/src/Doctrine/Orm/Filter/SearchFilter.php on line 227

[mrossard]

Great, that's pretty much expected at this point. I'll try to finish the PR based on this - there'll be a bunch of others tests to check/update but i think that's a better way top do this.

[soyuka]

#5744 patch reverted but let's continue working on that, we need to first reproduce the cases by @dannyvw on your patch @mrossard then we'll look for a proper solution!

[mrossard]

#5744 patch reverted but let's continue working on that, we need to first reproduce the cases by @dannyvw on your patch @mrossard then we'll look for a proper solution!

No problem! I don't have much time to work on this just now (i'm on vacation), but I'll try to submit a better patch asap - I feel like this version is actually working on this use case "by mistake", and this definitely needs to be adressed!

[mrossard]

I ended up creating a new PR to "reboot" #5623 but with this issue correctly (?) handled : #5760

Hope this does the trick, I have other things in mind to try after that :D

[dannyvw]

@mrossard Test is green :)

[mrossard]

If this is ok i have a working POC that would let you use IDs instead of full IRIs too - you'd be able to do things like /api/v2/users?groups[]=61817181-0ecc-42fb-a6e7-d97f2ddcb344&groups[]=32510d53-f737-4e70-8d9d-58e292c871f8.

@soyuka > is it a functionnality that's been requested? It is already supported for numeric ids, but i'm not sure whether you'd want to add (a,nd maintain) support for this or not?

[dannyvw]

I think it would be useful to support that as well because for numeric ids it already works.

[mrossard]

I think it would be useful to support that as well because for numeric ids it already works.

I'll create a separate PR once this one goes through then!

[mrossard]

I think it would be useful to support that as well because for numeric ids it already works.

here's the PR if you want to take a look! #5771