Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] Filter composition #2400

Open
teohhanhui opened this issue Dec 19, 2018 · 23 comments
Open

[RFC] Filter composition #2400

teohhanhui opened this issue Dec 19, 2018 · 23 comments
Labels

Comments

@teohhanhui
Copy link
Contributor

teohhanhui commented Dec 19, 2018

Instead of giving the client the ability to construct arbitrary queries / filtering (something better served by existing standards / specifications, e.g. OData and GraphQL), we could allow predefined composite filters in the API, each composed of one or more filter primitives.

For example:

/users
q: match('username', 'ipartial') or match('email', 'ipartial') or match('firstName', 'iword_start') or match('lastName', 'iword_start')

/products
q: match('sku', 'iexact') or match('name', 'iword_start') or match('description', 'iword_start')

This also supports the aliasing use case where you could easily define multiple filter parameters for the same property, e.g.:

/users
email: match('email', 'iexact')
email_partial: match('email', 'ipartial')

/products
sku: match('sku', 'iexact')
sku_partial: match('sku', 'ipartial')

Of course, this also forces us to have decoupled code between the declaration of filter parameters (currently the $properties argument) and the actual filtering (currently the filterProperty function). It'd be a good opportunity to get rid of inheritance of filter classes (mark as deprecated; to be removed in API Platform 3.0).

Related:

@teohhanhui teohhanhui added the RFC label Dec 19, 2018
@dunglas
Copy link
Member

dunglas commented Dec 19, 2018

There are 2 other popular syntaxes:

Both already have a lot of compatible client tools (including React Admin providers IIRC) and would be fine to me.

@dunglas
Copy link
Member

dunglas commented Dec 19, 2018

I actually like the loopback syntax because it looks very PHPish.

@teohhanhui
Copy link
Contributor Author

teohhanhui commented Dec 19, 2018

My suggestion here has nothing to do with complex querying from the client. These composite filter parameters are defined by the API. In fact, these 2 approaches could coexist.

Also, this would make it easier to write custom filters by being able to use composition of filter primitives. Whether we want to expose these primitives through configuration is another story.

@ragboyjr
Copy link
Contributor

regarding query syntax, https://github.com/krakphp/aql is a solution I built that basically allows sql type expressions in the API, but it's parsed and validated and can contain certain constraints, it might be something of use here for allowing more complex querying/flexibility

@teohhanhui
Copy link
Contributor Author

teohhanhui commented Jan 11, 2019

I'm strongly -1 for giving too much control to the client (there's GraphQL for that lol, at least it's better than inventing yet another format / syntax) (or OData if we go that route).

Anyway, it's irrelevant to this RFC, which is about filter composition on the server side only.

@ragboyjr
Copy link
Contributor

ah, i guess I misunderstood your original post, the idea is that if we do:

/users?q=abc123

We could define a proper query portion in the resource of what q maps to. So instead of q mapping to a simple field, we could build up a complex expression for q is searched on.

Is that understanding correct?

@teohhanhui
Copy link
Contributor Author

@ragboyjr Yes 😄

@ragboyjr
Copy link
Contributor

ah, ok, cool beans, y, this would be awesome, part of me wonders if maybe just making the custom filter API easier would fix this. Like, if making filters was just more composable already, then we could easily have filter factories that can be used to create filters instead of building our own expression parser or something on the backend.

@teohhanhui
Copy link
Contributor Author

@ragboyjr Could you elaborate? I'm not sure I get what you mean, about the composable filter API and filter factories.

@ragboyjr
Copy link
Contributor

Sure,

The basic idea would be similar to this: https://github.com/krakphp/php-inc/blob/master/src/php-inc.php#L30

Where we have filter classes that decorate certain things
We'd provide decorating filters like AllFilter and AnyFilter, which take a list of filter interfaces and will delegate to their children when apply or filterProperty is called.

Then in the filter config for a resource, we'd define structured config like:

@ApiFilter(
    ASTFilter::class,
    config={
        "type": "or",
        "filters": [
            {
                "type": "property", 
                "property": "username", 
                "filter": { "type": "match", "partial": true, "ignoreCase": true }
            },
            {
                "type": "property", 
                "property": "email", 
                "filter": { "type": "match", "partial": true, "ignoreCase": true }
            }
        ]
    }
)

The config is a bit verbose, but the idea is that we'd introduce a few new filters which handled one thing: OrFilter that takes a set of filters in its constructor. PropertyFilter, that takes on property key and the filter to apply on the property value. And a MatchFilter which is basically our search filter which takes some configuration parameters on if it's caseless and a partial match or not.

@teohhanhui
Copy link
Contributor Author

Please see my examples in the first post. We could use expression language. No need for verbose config. But the real problem is Doctrine's QueryBuilder. It's not composable at all.

@ymarillet
Copy link
Contributor

any update about this feature ? :-)

@teohhanhui
Copy link
Contributor Author

teohhanhui commented Nov 7, 2019

any update about this feature ? :-)

Not unless we are able to switch to a composable query builder (on top of / as an alternative to Doctrine's QueryBuilder). Perhaps it need not (and should not) support the same expressiveness, which might be an impediment to composability indeed.

@metaclass-nl
Copy link

metaclass-nl commented May 29, 2021

IMHO the problem is not in the QueryBuilder but the filters not separating two concerns:

  1. The generation of query expressions
  2. The (logical) combination of those expressions into the actual query

They can be made them composable by implementing an extra interface. The behavior of ::apply does not change so all tests keep running. But custom subclasses may break because ::filterProperty no longer adds the expressions to the QueryBuilder but instead returns them. You can see the refactoring here. (still needs workarounds if combining filtering by nested properties through OR)

Here is an example of composition in php code using the interface and here one of a WhereFilter (FilterLogic). Also see my comment on Filters RFC #1724.

@BrandonlinU
Copy link

I was working in Proof-of-Concept of a bundle for provide a filter that supports expressions for the filter logic (you can see for yourself), based on the commentaries of @teohhanhui and the separation of the generating query logic and the documentation inspired by the work of @metaclass-nl. In summary you can do the following with Api Platform with this bundle:

use ApiPlatform\Core\Annotation\ApiFilter;
use ApiPlatform\Core\Annotation\ApiResource;
use Doctrine\ORM\Mapping as ORM;
use Instacar\ExtraFiltersBundle\Doctrine\Orm\Filter\ExpressionFilter;

#[ApiResource]
#[ApiFilter(ExpressionFilter::class, properties: ['search' => 'orWhere(match("name", "ipartial"), match("author.name", "ipartial"), match("year", "exact"))'])]
#[ORM\Entity]
class Book {
    // real implementation
}

You can extend the filter composability implementing a interface and creating your own operators. I'd love to work in a similar functionality for Api Platform, but I'm not sure if this is what you want. For my part, I'm going to continue developing the Bundle for Symfony.

@stale
Copy link

stale bot commented Nov 4, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Nov 4, 2022
@soyuka
Copy link
Member

soyuka commented Nov 5, 2022

still relevant

@mrossard
Copy link
Contributor

Is this subject still ongoing / in reflexion? I was trying to find a good way to do this and actually started toying with symfony's ExpressionLanguage. It felt like a pretty good solution since it was already "in the ecosystem" and could provide the AST from the textual expression, so getting a rough POC for basic stuff (and/or and ==) was fairly easy.

Is there any point to keep digging in that direction?

@metaclass-nl
Copy link

@mrossard

Is there any point to keep digging in that direction?
I made a suggestion in this direction over 2 years ago but still no reaction. So i spent no more time on it.

@mrossard
Copy link
Contributor

mrossard commented Sep 5, 2023

@mrossard

Is there any point to keep digging in that direction?
I made a suggestion in this direction over 2 years ago but still no reaction. So i spent no more time on it.

I'll take a look at that, thanks. And then maybe start an "exploratory" PR based on what I did; I feel like we have the means to build something pretty neat, but it would likely come with a big rework of what is currently available.

@soyuka
Copy link
Member

soyuka commented Sep 5, 2023

Yeah and for now we had more urgent work. I think that #5732 is a good extension point to have for "selecting the data you want". Now we either need to choose what query parameters specification to pick (or create one), or we need to provide an extension point to handle that? Anyways probably that creating a filter using expression language would work. Also, I'd like to make some order in these parts of the code:

  • how is a query parameter documented (hydra, OpenApi)
  • how is a query parameter validated (current QueryParameterValidator actually is an openapi schema validator based on query parameters, it needs improvements)
  • how can we provide a good extension point so that the above is almost transparent (today it's the poorly documented getDescription which doesn't support using OpenApi classes)
  • how do we transform the query parameter values to their persisted values (think uuid/id searchfilter problem that's going on for ever)
  • is it possible to have a same query parameter that'd be used by multiple filters (for example searching relations or by id is different then a search term, and imo that's one of the biggest issue with the search filter) this is the same for composeable filters
  • and last but not least I'm not a huge fan of the Doctrine Extensions although I can't think of a better system for now...

@metaclass-nl thanks for your input it's a valuable comment and I often look at it!

@mrossard
Copy link
Contributor

mrossard commented Sep 5, 2023

I feel like chosing the exact query parameter specification is not actually that important an issue : we could actually support multiple specs by simply having an internal AST representation of the query and use that to build orm queries. Then for any query parameter spec there's only a need to implement the transformation into that representation, which shouldn't be too difficult.

As for the rest...well yes, lots of stuff need looking at!

@soyuka
Copy link
Member

soyuka commented Sep 21, 2023

@helyakin @aegypius

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

8 participants