Skip to content

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

Merged
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 38 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -75,3 +75,41 @@ public class Startup
}
}
```

### Development
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏 👏 👏


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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The dotnet CLI comes with a command for this: dotnet clean

```
6 changes: 6 additions & 0 deletions src/Examples/JsonApiDotNetCoreExample/Models/TodoItem.cs
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,12 @@ public TodoItem()

[Attr("achieved-date", isFilterable: false, isSortable: false)]
public DateTime? AchievedDate { get; set; }


[Attr("updated-date")]
public DateTime? UpdatedDate { get; set; }



public int? OwnerId { get; set; }
public int? AssigneeId { get; set; }
Expand Down
46 changes: 41 additions & 5 deletions src/JsonApiDotNetCore/Extensions/IQueryableExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Contributor

@jaredcnance jaredcnance Aug 29, 2018

Choose a reason for hiding this comment

The 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 DateTime? (left) and string (right). I think you meant to include the logic in the else block (ln143-153) here (the conditional on ln 147 is always false).
In this block, right should be null.

Now, backing up a bit to your comment:

what's to stop the usage of is:somethingelse? :-\ would we hard-code checks on null value usage?

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 eq: and ne:....:thinking:

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.

  • ?filter[attr]=isnull:
  • ?filter[attr]=isnotnull:

Based on this it seems like others have chosen to just use null as a special filter value. Maybe we could hide it behind a global feature flag? And maybe provide fine-grained control at the Attr level:

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe something like this. And any value after the operator is ignored.

This is actually what I started with, should be easy to go back. :)

[Attr(unsetFilterValueBehavior: UnsetFilterValueBehavior.AsNull)]

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);

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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}");
}
Expand Down
4 changes: 3 additions & 1 deletion src/JsonApiDotNetCore/Internal/Query/FilterOperations.cs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ public enum FilterOperations
like = 5,
ne = 6,
@in = 7, // prefix with @ to use keyword
nin = 8
nin = 8,
@is = 9,
isnot = 10
}
}
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;
Expand Down Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The 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 ErrorCollection which makes it difficult to understand the actual error. Something like this would be fine:

// 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:

image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I getchya. I was just copying from other tests ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's verify the returned TodoItems have actually been filtered (e.g. all results have UpdatedDate = null.
Maybe even consider explicitly adding a TodoItem to the db with UpdatedDate = notNullDateValue;
It shouldn't be an issue after #294, but for now I think we should assume we're not working with an empty db.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added assertion

}

[Fact]
public async Task Can_Filter_TodoItems_Using_Like_Operator()
{
Expand Down