Skip to content

Pluggable filter value conversions #1284

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
wants to merge 3 commits into from
Closed

Conversation

bkoelman
Copy link
Member

@bkoelman bkoelman commented Jun 15, 2023

This PR adds an extensibility point for API developers to add custom converters that turn literal text (a value surrounded by single quotes) in filter query string parameters into a .NET object. The resulting object may or may not be compatible with the property type. In the latter case, a custom expression rewriter can be used to manipulate the filter to match up, potentially adding additional conditions. Such a rewriter is typically invoked from IResourceDefinition<,>.OnApplyFilter(). An example is added in this PR to demonstrate that.

A converter must implement IFilterValueConverter. Its CanConvert method takes an AttrAttribute, which provides access to the attribute name and type, and its containing type in the resource graph. Multiple converters can be registered in the IoC container, which are tried sequentially. The Convert method performs the actual conversion and executes when CanConvert returns true. It takes the same AttrAttribute, the string value to convert from (without the surrounding quotes), and the outer filter function that consumes the value. The return type is object, which enables to return something incompatible to be rewritten later. It must never return null, because not all expression types support that. As a workaround, return a sentinel value (for example, Missing.Value) that is recognized by your rewriter. The consuming filter functions can be Comparison (equals, greaterThan, greaterOrEqual, lessThan, lessOrEqual), Any (pick from a set of candidates), or TextMatch (contains, startsWith, endsWith). The last one requires that your rewriter produces a string. If that's not possible, or the conversion fails for any other invalid input, throw a QueryParseException. By doing so, it gets translated into an appropriate JSON:API error response. Throwing anything else results in an HTTP 500 error.

Alternative designs considered:

  • Use TypeConverter. This requires developers to add [TypeConverter(typeof(YourConverter))] on resource properties, where YourConverter derives from System.ComponentModel.TypeConverter, overriding CanConvertTo/From and ConvertTo/From. This requires fine-grained control per property (which may be liked or not), but the downside is that everything is globally wired up, so IoC dependencies cannot be injected into the converter. It is also tricky to get right because ModelState validation triggers your type converter too. This API doesn't fit our needs well: we always convert from string, and to something the converter itself decides, which is unknown upfront.
  • Use ValueConverter<TModel, TProvider> from Entity Framework Core. They convert to/from an entity property type to a database type (for example, to store enums as strings). Aside from taking a dependency on EF Core for parsing query strings doesn't sound appealing, these are configured through EF Core's fluent API, which JsonApiDotNetCore doesn't have. In EF Core, they can be applied per entity property, or convention-based. It is also possible to configure a converter instance, whose construction can depend on injected dependencies in the DbContext. The static typing works against us because we'd like the converter to flexibly choose different return types, based on the incoming text. Using the non-generic ValueConverter base class doesn't solve that. What's also unfortunate is that the conversion must be a System.Expression, which doesn't permit to use modern C# syntax such as the ?. operator. Expressions are required because these converters are weaved into EF Core's projection shapers that turn SQL result sets into .NET objects.

Additionally, the first commit in this PR improves validation of filters and eliminates the multi-pass parsing of null comparisons against to-one relationships.

Fixes #1277.
Fixes #1275.
Fixes #1283.

QUALITY CHECKLIST

@bkoelman bkoelman force-pushed the filter-value-conversion branch from 1400248 to 937374a Compare June 15, 2023 11:32
@bkoelman bkoelman force-pushed the filter-value-conversion branch from 937374a to 5da2ad6 Compare June 15, 2023 11:44
@bkoelman bkoelman marked this pull request as ready for review June 15, 2023 12:01
@codecov
Copy link

codecov bot commented Jun 15, 2023

Codecov Report

Merging #1284 (8fab87c) into master (c0eb108) will increase coverage by 0.01%.
The diff coverage is 98.03%.

❗ Current head 8fab87c differs from pull request most recent head a8b3ecf. Consider uploading reports for the commit a8b3ecf to get more accurate results

@@            Coverage Diff             @@
##           master    #1284      +/-   ##
==========================================
+ Coverage   92.97%   92.98%   +0.01%     
==========================================
  Files         255      255              
  Lines        8254     8287      +33     
==========================================
+ Hits         7674     7706      +32     
- Misses        580      581       +1     
Impacted Files Coverage Δ
.../Queries/Internal/Parsing/QueryExpressionParser.cs 91.66% <50.00%> (-2.62%) ⬇️
...nternal/Parsing/QueryStringParameterScopeParser.cs 82.60% <66.66%> (-1.40%) ⬇️
...otNetCore/Queries/Internal/Parsing/FilterParser.cs 98.73% <100.00%> (+0.33%) ⬆️
...tNetCore/Queries/Internal/Parsing/IncludeParser.cs 95.91% <100.00%> (ø)
...tCore/Queries/Internal/Parsing/PaginationParser.cs 100.00% <100.00%> (ø)
...re/Queries/Internal/Parsing/QueryParseException.cs 100.00% <100.00%> (ø)
...iDotNetCore/Queries/Internal/Parsing/SortParser.cs 96.96% <100.00%> (ø)
...e/Queries/Internal/Parsing/SparseFieldSetParser.cs 100.00% <100.00%> (ø)
...rings/Internal/FilterQueryStringParameterReader.cs 100.00% <100.00%> (ø)
...Strings/Internal/SortQueryStringParameterReader.cs 100.00% <100.00%> (ø)
... and 1 more

@bkoelman bkoelman requested a review from maurei June 15, 2023 14:01
…shape of parsers, making it easier to use them directly from external code
@bkoelman bkoelman force-pushed the filter-value-conversion branch from cb5bf11 to a8b3ecf Compare June 22, 2023 22:15
@bkoelman
Copy link
Member Author

bkoelman commented Jul 5, 2023

Superseded by #1286.

@bkoelman bkoelman closed this Jul 5, 2023
@bkoelman bkoelman deleted the filter-value-conversion branch July 5, 2023 13:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
1 participant