Filtering replacement

[bkoelman]

My end goal was to provide my front-end team with the following:

• An easier / more-readable sql-ish filter expression language
• Access to advanced features like full text search or regex search
• Access to more functions available through EFCore such as date, numeric and string functions
• Access to arithmetic functions through standard symbols like +, -, *, /
• Filter parameterization eg: filter=name like $nameMatch&filter[$nameMatch]=%example%

I've got an Antlr4 grammar & parser generated for this language. In a perfect world I'd like to submit a PR for this either in the core or as a separate package. I'm struggling to identify exactly what I'd need to that as I find the current model confusing / cumbersome. Specifically I'm running into trouble:

• Having to reflect every possible type of expression as a function on QueryExpressionVisitor.
• Keeping the various layers separate in my head as I find the names of the various trees indistinct

It would be nice if:

• The JsonApiDotNetCore.Queries.Expressions.QueryExpression.Visit could be purely representational / not have the Visit function.
• Logic for converting QueryExpressions to System.Linq.Expressions could be encapsulated separately

Originally posted by @jeffreyabecker in #1277 (comment)

[bkoelman]

I have an example of what this /might/ look like here: https://github.com/jeffreyabecker/JsonApiDotNetCore/tree/query-expression-parsing-v3

That's an interesting experiment. How do you intend to address the mismatch in expression shapes?

We chose function-style syntax to avoid the complexities of operator precedence. Even in the latest version of EF Core, operator precedence bugs are still being fixed. I'm glad we never had to be concerned with that.

Personally, I would want to refrain from executing regular expressions against a potentially large database table. I don't think using indexes is possible, but I'm not sure.

What do you find confusing or cumbersome about the expression shape? It's pretty straightforward for filters:

The visitor pattern is pretty standard for traversing or rewriting trees. It provides type safety and extensibility. EF Core and Roslyn are both full of them. Having data-only nodes would make that a lot harder. The transforms you're using are conceptually similar to our visitors. Note you can override DefaultVisit() and delegate to any extra node types you'd like to add.

The conversion from QueryExpression to System.Linq.Expression trees is already separate. This enables developers to consume our tree to target data sources that do not use EF Core.

We can't introduce interfaces for the QueryExpression nodes because JADNC depends on the exact structure for correct workings. For example, adding bool IgnoreCase to MyNewComparsionExpression : IComparsionExpression would break everything because it's never taken into account. Another limitation is that JADNC itself creates these expressions, and creating an instance of an interface is not possible. Injecting them neither. Also, many existing users have code that depends on the current expression shape and we don't want to break them.

However, you can add extra nodes that derive from QueryExpression and make your parser produce those. Though you'll also need to handle the existing nodes for correct workings.

I've created a PR that opens up the parsers and LINQ builders at #1286.

[jeffreyabecker]

I have an example of what this /might/ look like here: https://github.com/jeffreyabecker/JsonApiDotNetCore/tree/query-expression-parsing-v3

That's an interesting experiment. How do you intend to address the mismatch in expression shapes?

We chose function-style syntax to avoid the complexities of operator precedence. Even in the latest version of EF Core, operator precedence bugs are still being fixed. I'm glad we never had to be concerned with that.
So at the end of the day Antlr4 handles the operator precedence problem through the ordering of the sub-expressions in the primary expression.

Personally, I would want to refrain from executing regular expressions against a potentially large database table. I don't think using indexes is possible, but I'm not sure.

I completely agree and in our case we'd definitely be setting up a full-text index for this feature. That said, my preference for framework design is to treat people like adults. Give them the flexibility they need and a solemn warning that they should avoid using this freedom to shoot themselves in the foot.

What do you find confusing or cumbersome about the expression shape? It's pretty straightforward for filters:

Personally /I/ don't find anything about them cumbersome or confusing -- but my front-end team dislikes the prefix notation and has requested a more flexible strategy.

The visitor pattern is pretty standard for traversing or rewriting trees. It provides type safety and extensibility. EF Core and Roslyn are both full of them. Having data-only nodes would make that a lot harder. The transforms you're using are conceptually similar to our visitors. Note you can override DefaultVisit() and delegate to any extra node types you'd like to add.

The conversion from QueryExpression to System.Linq.Expression trees is already separate. This enables developers to consume our tree to target data sources that do not use EF Core.

We can't introduce interfaces for the QueryExpression nodes because JADNC depends on the exact structure for correct workings. For example, adding bool IgnoreCase to MyNewComparsionExpression : IComparsionExpression would break everything because it's never taken into account. Another limitation is that JADNC itself creates these expressions, and creating an instance of an interface is not possible. Injecting them neither. Also, many existing users have code that depends on the current expression shape and we don't want to break them.

However, you can add extra nodes that derive from QueryExpression and make your parser produce those. Though you'll also need to handle the existing nodes for correct workings.

I've created a PR that opens up the parsers and LINQ builders at #1286.

So my complaint about visitors as they're traditionally implemented is that they need to declare public members for every possible type of node. My preference is to approach them as a map-of-strategies see GrandUnifiedVisitorBase. That said I do realize that the existing query-building system is /very/ tied into how this is all currently structured and changes would involve breaking stuff marked as PublicAPI.

I think the more reasonable ask here is work to make it possible for me to inject my strategy through the DI container. I think this fits better with my original comment seeing this work as an optional extra package too. I'm not sure exactly what that will be yet but I'm pretty sure it will be fairly simple. At first glance I'll need to make some classes public / not sealed and add a factory or two. I'll rebase my branch on this PR and work through that.

[bkoelman]

@jeffreyabecker to answer your question:

I'm noticing a lot of places in JsonApiDotNetCore.Queries.QueryableBuilding.WhereClauseBuilder explicitly passing typeof(System.Linq.Enumerable) when I would have expected context.ExtensionType. Is intentional?

Great question. Yes, it is intentional. Only the top-level data source is IQueryable<>. Operations on any nested to-many relationships are always IEnumerable<>.

For example, the picture below demonstrates that the .Books property accessor is of type ICollection<Book>. Therefore we need to call Enumerable.Any(person.Books) instead of Queryable.Any(person.Books).

[bkoelman]

Closing due to inactivity. Please let me know if it needs to stay open.