fix(doctrine): Handle invalid uuid in ORM SearchFilter

[alexndlm]

Q	A
Branch?	3.4
Tickets
License	MIT
Doc PR

[soyuka]

Thanks for your patch but an invalid uuid parameter should never reach the filter, in 3.x there's the query_parameter_validator and since 3.4 and 4.0 you can use a QueryParameter with a symfony Constraint: https://symfony.com/doc/current/reference/constraints/Uuid.html

[alexndlm]

Thanks, I got it. But why does the SearchFilter have similar behaviour for integer fields?

Also, all other filters have similar behaviour - ignore the filter if the value is invalid.

[ERuban]

I'm a bit disagree that the filtering by wrong id should envolve the filter ignoring, I think that the returning of the empty response in that case will be more correct.

[soyuka]

We're trying to improve filtering on API Platform and it has been really difficult not to break the SearchFilter as it is used for way too many data types. If you follow what's been done at #6775 we should just implement an UuidFilter. Validation can now be done optionally using a Symfony constraint on a Parameter.

[alexndlm]

I looked into the proposed solution with parameter validation and found that it works with strings but not with arrays, although SearchFilter supports both strings and arrays.

[ERuban]

@soyuka for me it looks strange a bit to have a coupling to Symfony's Asserts on resources that cannot be created or updated by api operation.

[soyuka]

Assertions are used for validating any kind of input, it's perfect for query parameters and already available since API platform 3.4!

[BOB41K1987]

@soyuka let me clarify the issue with validating a parameter that could be a string or an array (which is exaclty the case with SearchFilter).

Let's say we have a filter #[ApiFilter(filterClass: SearchFilter::class, properties: ['user' => SearchFilter::STRATEGY_EXACT])], where user is an entity with a uuid identifier.

The SearchFilter allows us to filter by either a string (1st case)
?user=1efbdb3c-21a8-6bd0-a4b3-4b201f3791eb
or an array (2nd case)
?user[]=1efbdb3c-21a8-6bd0-a4b3-4b201f3791eb&user[]=1efbdb3d-2cd9-69e0-9b3d-1b85ebd72ac6

Now let's try to use QueryParameter with constraints. If we add

parameters: [
    'user' => new QueryParameter(
        constraints: [
            new Assert\Uuid()
        ],
    )
]

then the first case will work, but the second case will give an error This value should be of type string., because Symfonu Uuid constraint doesn't support arrays.

And if we change it to support arrays

parameters: [
    'user' => new QueryParameter(
        constraints: [
            new Assert\All([
                new Assert\Uuid()
            ]),
        ],
    )
]

Then the 2nd case will work, but the 1st one will give an error This value should be of type iterable.. So either way, one of the cases will not work.
Yeah, it's more like a Symfony constraints limitation, but it is directly affecting QueryParameter feature compatibility with SearchFilter.

[soyuka]

What about https://symfony.com/doc/current/reference/constraints/AtLeastOneOf.html ? Also you can transform strings into Real uuids if needed with a Parameter provider.

[soyuka]

An example of a transformation in a parameter provider: https://github.com/api-platform/core/pull/6876/files#diff-9f7fc3d04b4b395b3d6962bac2cdf74b4349979e377cc26dedd092c3b15319d6

[ERuban]

Anyway, don't you think that it brokes consistensy a bit, otherwise we need to implement such parameter validation for any property in SearchFilter?
In example, we have the entity with user association and email attribute. We want to filter exactly by both attributes. If we will do parameter validation only user UUID, then we will get validation error with the wrong value in it, but for email we can provide any value and we will get just an empty result if value is not correct.

Moreover, the most projects are didn't use validation on query parameters, the most common cases when validation is used is cases when request has a body and we validating its content.

[BOB41K1987]

@soyuka thank you for the idea, AtLeastOneOf did the trick!

The Parameter provider option works too, but I'm not sure how to specify SearchFilter::STRATEGY_EXACT in this case. Overall, the QueryParameter feature looks very powerful!

new GetCollection(
    parameters: [
        'user' => new QueryParameter(
            provider: UuidSearchFilterParameterProvider::class,
            filter: SearchFilter::class,
        )
    ]
),

[soyuka]

There's a filter context however we're reworking filters to make this easier

[soyuka]

@ERuban this is a long going work, history is at #2400 and @vinceAmstoutz did a tremendous work around filters to make them easier to use with Parameters (#6775). The idea is to have more separation of concerns and having a SearchFilter on multiple types is, in my opinion, a very bad idea. It brought so many bugs in the past we need something more future-proof. Therefore, the future of the SearchFilter will be splitted into several filters such as: ExactStringFilter, PartialStringFilter, IriFilter (this one is for searching relations and takes an IRI or an id), work started at #6865.
Once we're done with adding new filters we will provide ways to do filtering operations (or vs and).

In example, we have the entity with user association and email attribute. We want to filter exactly by both attributes. If we will do parameter validation only user UUID, then we will get validation error with the wrong value in it, but for email we can provide any value and we will get just an empty result if value is not correct.

The idea is that you won't have more work to do:

#[QueryParameter(key: 'user', filter: IriFilter::class)]
#[QueryParameter(key: 'user.email', filter: ExactSearchFilter::class, constraints: new Email())] 

And this also solves the issue we had where you could not easily change how to create your queryParameters.