-
-
Notifications
You must be signed in to change notification settings - Fork 158
Additional filter operations: isnull and isnotnull #387
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
Changes from 3 commits
c457f6e
a3d9e6c
b34bd3f
84a78a0
e0b3cef
edd0f22
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -75,3 +75,41 @@ public class Startup | |
} | ||
} | ||
``` | ||
|
||
### Development | ||
|
||
Restore all nuget packages with: | ||
|
||
```bash | ||
dotnet restore | ||
``` | ||
|
||
#### Testing | ||
|
||
Running tests locally requires access to a postgresql database. | ||
If you have docker installed, this can be propped up via: | ||
|
||
```bash | ||
docker run --rm --name jsonapi-dotnet-core-testing \ | ||
-e POSTGRES_DB=JsonApiDotNetCoreExample \ | ||
-e POSTGRES_USER=postgres \ | ||
-e POSTGRES_PASSWORD=postgres \ | ||
-p 5432:5432 \ | ||
postgres | ||
``` | ||
|
||
And then to run the tests: | ||
|
||
```bash | ||
dotnet test | ||
``` | ||
|
||
#### Cleaning | ||
|
||
Sometimes the compiled files can be dirty / corrupt from other branches / failed builds. | ||
If your bash prompt supports the globstar, you can recursively delete the `bin` and `obj` directories: | ||
|
||
```bash | ||
shopt -s globstar | ||
rm -rf **/bin && rm -rf **/obj | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
``` |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -113,31 +113,56 @@ public static IQueryable<TSource> Filter<TSource>(this IQueryable<TSource> sourc | |
|
||
var concreteType = typeof(TSource); | ||
var property = concreteType.GetProperty(filterQuery.FilteredAttribute.InternalAttributeName); | ||
var op = filterQuery.FilterOperation; | ||
|
||
if (property == null) | ||
throw new ArgumentException($"'{filterQuery.FilteredAttribute.InternalAttributeName}' is not a valid property of '{concreteType}'"); | ||
|
||
try | ||
{ | ||
if (filterQuery.FilterOperation == FilterOperations.@in || filterQuery.FilterOperation == FilterOperations.nin) | ||
if (op == FilterOperations.@in || op == FilterOperations.nin) | ||
{ | ||
string[] propertyValues = filterQuery.PropertyValue.Split(','); | ||
var lambdaIn = ArrayContainsPredicate<TSource>(propertyValues, property.Name, filterQuery.FilterOperation); | ||
var lambdaIn = ArrayContainsPredicate<TSource>(propertyValues, property.Name, op); | ||
|
||
return source.Where(lambdaIn); | ||
} | ||
else if (op == FilterOperations.@is || op == FilterOperations.isnot) { | ||
var parameter = Expression.Parameter(concreteType, "model"); | ||
// {model.Id} | ||
var left = Expression.PropertyOrField(parameter, property.Name); | ||
var right = Expression.Constant(filterQuery.PropertyValue, typeof(string)); | ||
|
||
var body = GetFilterExpressionLambda(left, right, op); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The problem you're running into is that we're trying to compare Now, backing up a bit to your comment:
I'm not sure it makes sense to just have a "special equals" that handles null/not null cases and everything else is the same as If we stick with a new operator, I think it should be a single value and not an operation/value pair. Maybe something like this. And any value after the operator is ignored.
Based on this it seems like others have chosen to just use options.AllowNullFilterValues = true; [Attr(allowNullFilters: false)]
public string NeedToFilterOnEmptyStrings { get; set; } or maybe define an enum: [Attr(unsetFilterValueBehavior: UnsetFilterValueBehavior.AsNull)]
[Attr(unsetFilterValueBehavior: UnsetFilterValueBehavior.AsEmptyString)]
[Attr(unsetFilterValueBehavior: UnsetFilterValueBehavior.Ignore)] @rtablada do you have any insight into how other frameworks handle this problem? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This is actually what I started with, should be easy to go back. :)
I guess the case I keep thinking of is what if someone has a nullable string field? In previously projects (rails), I used https://github.com/activerecord-hackery/ransack which has a bunch of matchers: https://github.com/activerecord-hackery/ransack#search-matchers including null and not_null |
||
var lambda = Expression.Lambda<Func<TSource, bool>>(body, parameter); | ||
|
||
return source.Where(lambda); | ||
} | ||
else | ||
{ // convert the incoming value to the target value type | ||
{ | ||
var isNullabe = IsNullable(property.PropertyType); | ||
var propertyValue = filterQuery.PropertyValue; | ||
var value = propertyValue; | ||
|
||
if (op == FilterOperations.@isnot || op == FilterOperations.isnot) | ||
{ | ||
if (isNullabe && propertyValue == "null") | ||
{ | ||
value = null; | ||
} | ||
} | ||
|
||
// convert the incoming value to the target value type | ||
// "1" -> 1 | ||
var convertedValue = TypeHelper.ConvertType(filterQuery.PropertyValue, property.PropertyType); | ||
var convertedValue = TypeHelper.ConvertType(value, property.PropertyType); | ||
// {model} | ||
var parameter = Expression.Parameter(concreteType, "model"); | ||
// {model.Id} | ||
var left = Expression.PropertyOrField(parameter, property.Name); | ||
// {1} | ||
var right = Expression.Constant(convertedValue, property.PropertyType); | ||
|
||
var body = GetFilterExpressionLambda(left, right, filterQuery.FilterOperation); | ||
var body = GetFilterExpressionLambda(left, right, op); | ||
|
||
var lambda = Expression.Lambda<Func<TSource, bool>>(body, parameter); | ||
|
||
|
@@ -204,6 +229,9 @@ public static IQueryable<TSource> Filter<TSource>(this IQueryable<TSource> sourc | |
} | ||
} | ||
|
||
private static bool IsNullable(Type type) => type.IsGenericType && type.GetGenericTypeDefinition() == typeof(Nullable<>); | ||
|
||
|
||
private static Expression GetFilterExpressionLambda(Expression left, Expression right, FilterOperations operation) | ||
{ | ||
Expression body; | ||
|
@@ -236,6 +264,14 @@ private static Expression GetFilterExpressionLambda(Expression left, Expression | |
case FilterOperations.ne: | ||
body = Expression.NotEqual(left, right); | ||
break; | ||
case FilterOperations.isnot: | ||
// {model.Id != null} | ||
body = Expression.NotEqual(left, right); | ||
break; | ||
case FilterOperations.@is: | ||
// {model.Id == null} | ||
body = Expression.Equal(left, right); | ||
break; | ||
default: | ||
throw new JsonApiException(500, $"Unknown filter operation {operation}"); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,4 @@ | ||
using System; | ||
using System.Collections.Generic; | ||
using System.Linq; | ||
using System.Net; | ||
|
@@ -90,6 +91,52 @@ public async Task Can_Filter_TodoItems() | |
Assert.Equal(todoItem.Ordinal, todoItemResult.Ordinal); | ||
} | ||
|
||
[Fact] | ||
public async Task Can_Filter_TodoItems_Using_IsNot_Operator() | ||
{ | ||
// Arrange | ||
var todoItem = _todoItemFaker.Generate(); | ||
todoItem.UpdatedDate = new DateTime(); | ||
_context.TodoItems.Add(todoItem); | ||
_context.SaveChanges(); | ||
|
||
var httpMethod = new HttpMethod("GET"); | ||
var route = $"/api/v1/todo-items?filter[updated-date]=isnot:null"; | ||
var request = new HttpRequestMessage(httpMethod, route); | ||
|
||
// Act | ||
var response = await _fixture.Client.SendAsync(request); | ||
var body = await response.Content.ReadAsStringAsync(); | ||
var deserializedBody = _fixture.GetService<IJsonApiDeSerializer>().DeserializeList<TodoItem>(body); | ||
|
||
// Assert | ||
Assert.Equal(HttpStatusCode.OK, response.StatusCode); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Generally, it is better to check the status code immediately after getting the response. De-serialization will fail if the response type is an // act
var response = await _fixture.Client.SendAsync(request);
// assert
Assert.Equal(HttpStatusCode.OK, response.StatusCode);
// or even something like:
Assert.True(response.StatusCode == HttpStatusCode.OK, $"Received response with status code: '{response.StatusCode}' and body: {response.Content.ReadAsString()}"); Otherwise, I just see this in the build log which isn't particularly useful: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I getchya. I was just copying from other tests ;) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jaredcnance how much refactoring would you want on acceptance tests? I typically extract all the repetitive stuff like request building and such into helper methods (I could do this in a subsequent PR) |
||
Assert.NotEmpty(deserializedBody); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See comment below. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. added assertion |
||
} | ||
|
||
[Fact] | ||
public async Task Can_Filter_TodoItems_Using_Is_Operator() | ||
{ | ||
// Arrange | ||
var todoItem = _todoItemFaker.Generate(); | ||
todoItem.UpdatedDate = null; | ||
_context.TodoItems.Add(todoItem); | ||
_context.SaveChanges(); | ||
|
||
var httpMethod = new HttpMethod("GET"); | ||
var route = $"/api/v1/todo-items?filter[updated-date]=is:null"; | ||
var request = new HttpRequestMessage(httpMethod, route); | ||
|
||
// Act | ||
var response = await _fixture.Client.SendAsync(request); | ||
var body = await response.Content.ReadAsStringAsync(); | ||
var deserializedBody = _fixture.GetService<IJsonApiDeSerializer>().DeserializeList<TodoItem>(body); | ||
|
||
// Assert | ||
Assert.Equal(HttpStatusCode.OK, response.StatusCode); | ||
Assert.NotEmpty(deserializedBody); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's verify the returned There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. added assertion |
||
} | ||
|
||
[Fact] | ||
public async Task Can_Filter_TodoItems_Using_Like_Operator() | ||
{ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👏 👏 👏