Skip to content

How to implement a custom filter #1277

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

Closed
max-maurice opened this issue May 24, 2023 · 12 comments · Fixed by #1286
Closed

How to implement a custom filter #1277

max-maurice opened this issue May 24, 2023 · 12 comments · Fixed by #1286
Labels

Comments

@max-maurice
Copy link

SUMMARY

I'm trying to allow custom filter strings like this:

http://localhost/ressource?filter=lessThan(createdAt,'-00:10:00')

which would mean to return all ressources where the createdAt datetime value is less than 10 minutes in the past.

In JADNC before 5.2.0 this worked when using a QueryRewriter like my TimespanExpressionRewriter below. Since version 5.2.0, JADNC throws an exception: "Failed to convert '-00:10:00' of type 'String' to type 'DateTime'."

I suppose the TimespanExpressionRewriter is not the right way to go. Hence my question: How do we implement a custom filter like that?

I've already dug around in the JADNC sources without success.

DETAILS

My TimespanExpressionRewriter class overrides VisitComparison() and replaces an expression when the right value looks like -00:00:00. It returns the rewritten expression like this:

  • Input: lessThan(updatedAt,'-1.06:00')
  • Output: and(greaterThan(updatedAt,'2000-06-30T10:00:00+02:00'),lessOrEqual(updatedAt,'2000-07-01T16:00:00+02:00'))

In JADNC <5.2.0 this worked, when adding the expression rewriting in a ResourceDefinition like this:

public override FilterExpression? OnApplyFilter(FilterExpression? existingFilter)
{
    if (existingFilter != null)
    {
        // Timespan expression
        // Example:
        // http://localhost/ressources?filter=lessThan(createdAt%2C'-0.00%3A50%3A00')

        existingFilter = new TimespanExpressionRewriter().Visit(existingFilter, null) as FilterExpression;
    }

    return existingFilter;
}

TimespanExpressionRewriter:

public class TimespanExpressionRewriter : QueryExpressionRewriter<object?>
{
    public override QueryExpression? VisitComparison(ComparisonExpression expression, object? argument)
    {
        // TryCreate() replaces "expression" if it contains a relative timespan
        if (TimespanExpressionFactory.TryCreate(expression, () => DateTime.UtcNow, out var expr))
        {
            return expr;
        }

        return base.VisitComparison(expression, argument);
    }

    public override QueryExpression? Visit(QueryExpression expression, object argument)
    {
        return base.Visit(expression, argument);
    }
}

STEPS TO REPRODUCE

VERSIONS USED

  • JsonApiDotNetCore version: 5.2.0
  • ASP.NET Core version: 7
  • Entity Framework Core version: 7.0.5
  • Database provider: MSSQL
@bkoelman
Copy link
Member

Hi @max-maurice, thanks for your detailed report.

What changed in v5.2 is that the conversion from string to .NET type now happens while parsing the filter. In earlier versions, the conversion happened while building the LINQ query. It makes sense you're affected by this. The reason this changed is to make it easier to implement your own non-EF Core data access.

I think JADNC needs to provide some form of extensibility for this during filter parsing. I'm still figuring out how to best model that. Coincidentally, a related conversation is happening at #1275.

@bkoelman
Copy link
Member

@max-maurice I'm working on tests to cover your scenario. Can you please verify that the following cases match your expectations of how relative times are interpreted?

// Rows in database:
// t1 = Now - 15 min
// t2 = Now - 10 min
// t3 = Now - 5 min
// t4 = Now
// t5 = Now + 5 min
// t6 = Now + 10 min
// t7 = Now + 15 min

// Filter expression                            Meaning                                 Query                                       Matching rows
// ComparisonOperator.GreaterThan    -0:10:00   more than 10 minutes ago                value <  (Now - 10 min)                     t1
// ComparisonOperator.GreaterOrEqual -0:10:00   at least 10 minutes ago                 value <= (Now - 10 min)                     t1, t2
// ComparisonOperator.Equals         -0:10:00   exactly 10 minutes ago                  value == (Now - 10 min)                     t2
// ComparisonOperator.LessThan       -0:10:00   less than 10 minutes ago                value >  (Now - 10 min) && value <= Now     t3, t4
// ComparisonOperator.LessOrEqual    -0:10:00   at most 10 minutes ago                  value >= (Now - 10 min) && value <= Now     t2, t3, t4

// ComparisonOperator.GreaterThan    +0:10:00   more than 10 minutes in the future      value >  (Now + 10 min)                     t7
// ComparisonOperator.GreaterOrEqual +0:10:00   at least 10 minutes in the future       value >= (Now + 10 min)                     t6, t7
// ComparisonOperator.Equals         +0:10:00   in exactly 10 minutes                   value == (Now + 10 min)                     t6
// ComparisonOperator.LessThan       +0:10:00   less than 10 minutes in the future      value <  (Now + 10 min) && value >= Now     t4, t5
// ComparisonOperator.LessOrEqual    +0:10:00   at most 10 minutes in the future        value <= (Now + 10 min) && value >= Now     t4, t5, t6

// ComparisonOperator.GreaterThan    t5         later than t5                           value >  t5                                 t6, t7
// ComparisonOperator.GreaterOrEqual t5         later or at t5                          value >= t5                                 t5, t6, t7
// ComparisonOperator.Equals         t5         exactly at t5                           value == t5                                 t5
// ComparisonOperator.LessThan       t5         before t5                               value <  t5                                 t1, t2, t3, t4
// ComparisonOperator.LessOrEqual    t5         before or at t5                         value <= t5                                 t1, t2, t3, t4, t5

@bkoelman
Copy link
Member

bkoelman commented Jun 8, 2023

@max-maurice Are you still interested in getting this implemented? I'd like to get your feedback before opening up a PR.

@max-maurice
Copy link
Author

@max-maurice Are you still interested in getting this implemented? I'd like to get your feedback before opening up a PR.

Yes, I'm still interested in this very much. Sorry for not replying earlier. Your test cases look just perfect, thank you!

@bkoelman
Copy link
Member

@max-maurice can you take a look at #1284 and give it a try within the next few days? I'd like to know if this addresses your needs before merging it. I'm open to alternatives, any feedback you'd be able to give me is welcome.

@max-maurice
Copy link
Author

Thank you @bkoelman for your PR! This works like a charm, dank je wel :)

@jeffreyabecker
Copy link

@bkoelman Would you accept a PR allowing for the injection of a FilterParser instance into the FilterQueryStringParameterReader on this branch? I've got a request from my team to implement sql-ish queries and It would be nice to avoid copying the entire parameter reader class just so I can inject a custom FilerParser.

@bkoelman
Copy link
Member

Hi @jeffreyabecker Thanks for asking. Coincidentally, I'm working on that right now. It's part of a bigger effort to make it possible to define custom filter functions. Can you share a bit more on your needs, ie what you aim to accomplish?

@jeffreyabecker

This comment was marked as off-topic.

@jeffreyabecker

This comment was marked as off-topic.

@bkoelman
Copy link
Member

bkoelman commented Jul 5, 2023

@max-maurice I'm sorry to let you know that #1284 won't be merged. The reason is that this approach is too generic and it paints us in a corner. Returning object from parsing string values gets in the way of opening up the parsers for extension, where we need to know the (return) type of functions and attributes. It's also unfortunate for anyone implementing the translation to LINQ, having to deal with constants whose type is potentially incompatible with the attribute type.

I've tried to address your concerns in #1286, by using a pluggable 'timeOffset' function instead. It is a breaking change for your REST API, however. See the file TimeOffsetTests.cs at https://github.com/json-api-dotnet/JsonApiDotNetCore/pull/1286/files#diff-9032b4754cd2d543369673cd79ef0350abc731af872c787a846631d3502c349b.

Would this work for you?

@max-maurice
Copy link
Author

@bkoelman Yes, I guess this would work for me. Thank You!

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