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

Filter OR operator and complex predicates #183

Closed
jaredcnance opened this issue Nov 9, 2017 · 13 comments · Fixed by #792
Closed

Filter OR operator and complex predicates #183

jaredcnance opened this issue Nov 9, 2017 · 13 comments · Fixed by #792

Comments

@jaredcnance
Copy link
Contributor

jaredcnance commented Nov 9, 2017

Currently, filter values are split by comma into separate FiterQuerys. This makes things like this possible:

?filter[attr]=abc,like:def

But, this doesn't make much sense and the proper query should be:

?filter[attr]=abc&filter[attr]=like:def

It also prevents custom queries from being picked up using IQueryAccessor.GetRequired<>()

This issue has been re-purposed for the discussion of complex filter predicates (anything other than simple serial AND)

@dnperfors
Copy link
Contributor

This would also make 'or' filtering possible. A feature I would like to have is to filter on multiple values, so that for example this:

?filter[attr]abc,def

returns resources where attr is either abc or def. This will not be possible until this splitting is fixed (and then could be possible in combination with #178)

@jaredcnance
Copy link
Contributor Author

jaredcnance commented Nov 21, 2017

@dnperfors I agree that your format makes sense (to me at least) for OR on a single attribute. But, I suppose the bigger question is whether or not we should allow a mechanism for expressing a logical OR operator spanning multiple attributes. For example, your proposal could handle the following:

(attr == "abc" || attr == "def")

but doesn't solve for the more general:

(attrA == "abc" || attrB == "def")

I could see something like this to solve that case:

?filter[attrA|attrB]=abc|def
?filter[attrA,attrB]=abc,def

This would allow you to construct more complex conditional clauses across multiple attributes. I'm not particularly fond of that syntax, so I'm open to suggestions. If we go with this, I would still like to preserve the original value (unsplit abc,def) somewhere in the FilterQuery object. Another thing worth considering is the added power this provides clients. If this is introduced, I would feel much better providing a verification hook (like JR) so server applications can easily reject filter queries. This might also be an opt-in feature (?)

@jaredcnance jaredcnance added this to the v3.0.0 milestone Dec 7, 2017
@rtablada
Copy link
Contributor

@jaredcnance I need to check but I think that multiple filters should be filter[prop][] to signify an array of values.

This also interops with common patterns and request readers in frameworks like Laravel/Express/Rails.

@rtablada
Copy link
Contributor

Is there anything in the spec around arrays of values?

@rtablada
Copy link
Contributor

From the jsonapi.org spec:

Multiple filter values can be combined in a comma-separated list. For example:

GET /comments?filter[post]=1,2 HTTP/1.1

http://jsonapi.org/recommendations/#filtering

@dnperfors
Copy link
Contributor

dnperfors commented Dec 15, 2017

@jaredcnance, For a previous project where I had to implement the jsonapi spec, I didn’t really find a good solution for OR-ing multiple attributes as you describe, but how about something like:
?filter[attr1]=abc|filter[attr2]=def
This way it looks similar to the AND operation. The only problem left would be grouping operations (a&by or c&d)

@rtablada, there is nothing specific mentioned about arrays, the best you could do is indeed using the comma separator and for testing for specific values in an array you could use a contains operator (not sure if it is already supported)

@jaredcnance jaredcnance changed the title Filter values should not be split by comma Filter OR operator Feb 24, 2018
@jaredcnance jaredcnance changed the title Filter OR operator Filter OR operator and complex predicates Feb 24, 2018
@ghost
Copy link

ghost commented Mar 28, 2018

@jaredcnance I need the OR operator asap, so I've quickly implemented the following prototype:

  • api/v1/users?filter[firstName|lastName|login]=like:ander
    or
  • api/v1/users?filter[firstName|lastName|login]=like:ander|mustermann|like:company.com

Startup:

services.AddScoped<IQueryParser, CustomQuerySetParser>();
services.AddScoped<IResourceService<Entity, int>, CustomResourceService<Entity, int>>();
  1. CustomQuerySetParser: I had to override the ParseFilterQuery Method from the QuerySetParser. The change is that upon creation of the ApiContext the OR-filter-parameter is recognized, but not yet split, rather stored as a single FilterQuery.
protected override List<FilterQuery> ParseFilterQuery(string key, string value)
{
    ...
    if (propertyName.Contains('|'))
    {
        queries.Add(new FilterQuery(propertyName, value, "disjunction"));
        return queries;
    }
    ...
}
  1. CustomEntityResourceService: the idea is to override the point when the filters are actually executed, split the OR-filter-query into basic queries and take the Union of the reults.

    Overriding the ApplySortAndFilterQuery Method would be enough, unfortunately, as with many methods in this project, ApplySortAndFilterQuery and other helper Methods are private, so that there is the need to just copy them from the original EntityResourceService.

    Only one line in the ApplySortAndFilterQuery Method has to change, I simply call my custom Filter-trigger-method:

private IQueryable<T> ApplySortAndFilterQuery(IQueryable<T> entities)
{
    ...
            foreach (var propertyName in propertyNames)
            {
              entities = ExecuteFilter(entities, filter);
            }
    ...
}

My methods implemeting the OR logic:

  1. Split the OR-filter-query into basic filter operations:
internal virtual List<FilterQuery> ParseDisjunctionFilterQuery(string key, string value)
{
    var filterQueries = new List<FilterQuery>();

    var propertyNames = key.Split('|');
    var filterValues = value.Split('|');

    if (filterValues.Length == 1)
    {
        (var operation, var filterValue) = ParseFilterOperation(filterValues[0]);

        foreach (var propertyName in propertyNames)
        {
             filterQueries.Add(new FilterQuery(propertyName, filterValue, operation));
        }
    }
    else if (propertyNames.Length == filterValues.Length)
    {
        for (int i = 0; i < filterValues.Length; i++)
        {
            (var operation, var filterValue) = ParseFilterOperation(filterValues[i]);
            filterQueries.Add(new FilterQuery(propertyNames[i], filterValue, operation));
        }
    }
    else
    {
       // throw Exception
    }

    return filterQueries;
}
  1. Execute the basic filtering operations separatly and return the union of the result
internal virtual IQueryable<T> ExecuteFilter(IQueryable<T> entities, FilterQuery filter)
{
    if (filter.Operation.Equals("disjunction"))
    {
        var normalFilterQueries = ParseDisjunctionFilterQuery(filter.Key, filter.Value);

        var filteringResults = new List<IQueryable<T>>();

        foreach (var filterQuery in normalFilterQueries)
        {
           filteringResults.Add(_entityRepository.Filter(entities, filterQuery));
        }

        var resultEntities = new List<T>().AsQueryable();
        foreach (var filteringResult in filteringResults)
        {
            resultEntities = resultEntities.Union(filteringResult);
        }

        return resultEntities;
    }

    return _entityRepository.Filter(entities, filter);
}

What do you think of this approach?

@jaredcnance
Copy link
Contributor Author

@nikalfa thanks for sharing! This seems like a valid approach. I'm not sure if the semantics of the OR-ing are a good fit for a general use case, but maybe it's something we can iterate on. I think I agree with the use of | (%7C) as an OR symbol, but from what I can tell, this can't meet the criteria @dnperfors mentioned:

(A&B or C&D)

But, it does look like it's perfectly fine for:

(AorB & CorD)

Regarding the use of private methods, I do intend to expose more methods for overrides in the future. Increasing the surface area of the public API is a challenge because it decreases our ability to make changes to implementation details w/o making breaking changes. One of the major goals I have for v3 is to improve the API has a whole and make it more clear where things should go when you're extending it. (e.g. when to put X in the service layer vs. the repository layer). So, with v3 I'd like to expose more of these details so you don't have to C&P your way to a simple modification.

@ghost
Copy link

ghost commented Mar 28, 2018

@jaredcnance Yes, this construct can't handle the the ORing of ANDs, but this would make the parsing of the FilterQuery much more complex.

As for combinations with ANDs - my implementation can handle any number of them. In Fact it can also handle any amount of ORs as long they are "not interrupted". This filter sequence works:

A & B & (C|D) & E & (F|G|H|J) & G

The only Issue I have with my current solution is that the Union() operation requires a properly working Equals() Method.

This would mean custom implementation for every Entity, or maybe a more general implementation which would Serialize the Entity to JSON and then compare the strings.

@mihairadulescu
Copy link

filter[name][]=name1,name2

@wisepotato
Copy link
Contributor

Wouldn't the basic implementation of @nikalfa serve as a good stepping stone? Or do you wish for a fully implemented version for v4?

@jaredcnance
Copy link
Contributor Author

If @nikalfa or someone else has time to implement a solution, I'll happily back this approach moving forward. I think more complex filtering operations may be supported through json:api v1.1 operations.

@ghost
Copy link

ghost commented Oct 26, 2018

@wisepotato @jaredcnance it's pretty ironic that the trigger for the OR filter reappeared right now. Although I did implement this feature in March, it was never included in our product.

For the upcoming sprint (two weeks) I got the task to dust off/finalize the implementation and get it into production.

I'll happily share my solution.

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

Successfully merging a pull request may close this issue.

5 participants