Skip to content

Add IN filter operation for array searching #288

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.

Sign up for GitHub

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 2 commits into from
Jun 6, 2018
Merged

Add IN filter operation for array searching #288

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
88f5cbe
Add IN filter for array searching
May 29, 2018
09b6e5b
Code improvements of "in" filtering
Jun 6, 2018
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
107 changes: 78 additions & 29 deletions src/JsonApiDotNetCore/Extensions/IQueryableExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
using System.Collections.Generic;
using System.Linq;
using System.Linq.Expressions;
using System.Reflection;
using JsonApiDotNetCore.Internal;
using JsonApiDotNetCore.Internal.Query;
using JsonApiDotNetCore.Services;
Expand Down Expand Up @@ -101,21 +102,30 @@ public static IQueryable<TSource> Filter<TSource>(this IQueryable<TSource> sourc

try
{
// convert the incoming value to the target value type
// "1" -> 1
var convertedValue = TypeHelper.ConvertType(filterQuery.PropertyValue, 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 lambda = Expression.Lambda<Func<TSource, bool>>(body, parameter);

return source.Where(lambda);
if (filterQuery.FilterOperation == FilterOperations.@in )
{
string[] propertyValues = filterQuery.PropertyValue.Split(',');
var lambdaIn = ArrayContainsPredicate<TSource>(propertyValues, property.Name);

return source.Where(lambdaIn);
}
else
{ // convert the incoming value to the target value type
// "1" -> 1
var convertedValue = TypeHelper.ConvertType(filterQuery.PropertyValue, 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 lambda = Expression.Lambda<Func<TSource, bool>>(body, parameter);

return source.Where(lambda);
}
}
catch (FormatException)
{
Expand All @@ -140,26 +150,36 @@ public static IQueryable<TSource> Filter<TSource>(this IQueryable<TSource> sourc

try
{
// convert the incoming value to the target value type
// "1" -> 1
var convertedValue = TypeHelper.ConvertType(filterQuery.PropertyValue, relatedAttr.PropertyType);
// {model}
var parameter = Expression.Parameter(concreteType, "model");
if (filterQuery.FilterOperation == FilterOperations.@in)
{
string[] propertyValues = filterQuery.PropertyValue.Split(',');
var lambdaIn = ArrayContainsPredicate<TSource>(propertyValues, relatedAttr.Name, relation.Name);

return source.Where(lambdaIn);
}
else
{
// convert the incoming value to the target value type
// "1" -> 1
var convertedValue = TypeHelper.ConvertType(filterQuery.PropertyValue, relatedAttr.PropertyType);
// {model}
var parameter = Expression.Parameter(concreteType, "model");

// {model.Relationship}
var leftRelationship = Expression.PropertyOrField(parameter, relation.Name);
// {model.Relationship}
var leftRelationship = Expression.PropertyOrField(parameter, relation.Name);

// {model.Relationship.Attr}
var left = Expression.PropertyOrField(leftRelationship, relatedAttr.Name);
// {model.Relationship.Attr}
var left = Expression.PropertyOrField(leftRelationship, relatedAttr.Name);

// {1}
var right = Expression.Constant(convertedValue, relatedAttr.PropertyType);
// {1}
var right = Expression.Constant(convertedValue, relatedAttr.PropertyType);

var body = GetFilterExpressionLambda(left, right, filterQuery.FilterOperation);
var body = GetFilterExpressionLambda(left, right, filterQuery.FilterOperation);

var lambda = Expression.Lambda<Func<TSource, bool>>(body, parameter);
var lambda = Expression.Lambda<Func<TSource, bool>>(body, parameter);

return source.Where(lambda);
return source.Where(lambda);
}
}
catch (FormatException)
{
Expand Down Expand Up @@ -206,6 +226,35 @@ private static Expression GetFilterExpressionLambda(Expression left, Expression
return body;
}

private static Expression<Func<TSource, bool>> ArrayContainsPredicate<TSource>(string[] propertyValues, string fieldname, string relationName = null)
{
ParameterExpression entity = Expression.Parameter(typeof(TSource), "entity");
MemberExpression member;
if (!string.IsNullOrEmpty(relationName))
{
var relation = Expression.PropertyOrField(entity, relationName);
member = Expression.Property(relation, fieldname);
}
else
member = Expression.Property(entity, fieldname);

var containsMethods = typeof(Enumerable).GetMethods(BindingFlags.Static | BindingFlags.Public).Where(m => m.Name == "Contains");
MethodInfo method = null;
foreach (var m in containsMethods)
{
if (m.GetParameters().Count() == 2)
Copy link
Contributor

@jaredcnance jaredcnance Jun 5, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I recommend consolidating this into:

var method = typeof(Enumerable)
  .GetMethods(BindingFlags.Static | BindingFlags.Public)
  .Where(m => 
    m.Name == nameof(Enumerable.Contains) // not sure if this actually works, but would be nice
    && m.GetParameters().Count() == 2)
  .First(m => m);

Also, the result of this query never changes. It would probably be best to run it in a static constructor and store the MethodInfo in a private field so we don't do this on every invocation at runtime. I suspect there are several places in these extensions that could benefit from such a refactor

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Uf, this is much better solution. And method will be singleton, good point.

{
method = m;
break;
}
}
method = method.MakeGenericMethod(member.Type);
var obj = TypeHelper.ConvertListType(propertyValues, member.Type);

var exprContains = Expression.Call(method, new Expression[] { Expression.Constant(obj), member });
return Expression.Lambda<Func<TSource, bool>>(exprContains, entity);
}

public static IQueryable<TSource> Select<TSource>(this IQueryable<TSource> source, List<string> columns)
{
if (columns == null || columns.Count == 0)
Expand Down
3 changes: 2 additions & 1 deletion src/JsonApiDotNetCore/Internal/Query/FilterOperations.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ public enum FilterOperations
le = 3,
ge = 4,
like = 5,
ne = 6
ne = 6,
@in = 7, // prefix with @ to use keyword
}
}
24 changes: 24 additions & 0 deletions src/JsonApiDotNetCore/Internal/TypeHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
using System;
using System.Collections;
using System.Collections.Generic;
using System.Reflection;

namespace JsonApiDotNetCore.Internal
Expand Down Expand Up @@ -54,5 +56,27 @@ public static T ConvertType<T>(object value)
{
return (T)ConvertType(value, typeof(T));
}

/// <summary>
/// Convert collection of query string params to Collection of concrete Type
/// </summary>
/// <param name="values">Collection like ["10","20","30"]</param>
/// <param name="type">Non array type. For e.g. int</param>
/// <returns>Collection of concrete type</returns>
public static object ConvertListType(IEnumerable<string> values, Type type)
{
var convertedArray = new List<object>();
foreach (var value in values)
{
convertedArray.Add(ConvertType(value, type));
}
var listType = typeof(List<>).MakeGenericType(type);
IList list = (IList)Activator.CreateInstance(listType);
foreach (var item in convertedArray)
{
list.Add(item);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

2 things:

  1. the signature return should be IList instead of object
  2. any reason for multiple enumeration? it seems like you would enumerate values once and perform ConvertType at the same time you add it to list. L68-L72 don't seem necessary to me.

Copy link
Contributor Author

@milosloub milosloub Jun 5, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

This statement was hell of pain:
Expression.Call(method, new Expression[] { Expression.Constant(obj), member });

Expression.Call does some unboxing during runtime and if there are uncompatible object types, it throws exception of Type mismatch. So idea of this is to convert boxed types in object to compatible IList of target property.

I'll make some investigation later to simplify enumerations.

return list;
}
}
}
47 changes: 33 additions & 14 deletions src/JsonApiDotNetCore/Services/QueryParser.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,14 +82,23 @@ protected virtual List<FilterQuery> ParseFilterQuery(string key, string value)
// expected input = filter[id]=1
// expected input = filter[id]=eq:1
var queries = new List<FilterQuery>();

var propertyName = key.Split(QueryConstants.OPEN_BRACKET, QueryConstants.CLOSE_BRACKET)[1];

var values = value.Split(QueryConstants.COMMA);
foreach (var val in values)
// InArray case
var op = GetFilterOperation(value);
if (op == FilterOperations.@in.ToString())
Copy link
Contributor

Choose a reason for hiding this comment

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

should ignore case

op.Equals(FilterOperations.@in.ToString(), StringComparer.OrdinalIgnoreCase)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, good point

{
(var operation, var filterValue) = ParseFilterOperation(value);
queries.Add(new FilterQuery(propertyName, filterValue, op));
}
else
{
(var operation, var filterValue) = ParseFilterOperation(val);
queries.Add(new FilterQuery(propertyName, filterValue, operation));
var values = value.Split(QueryConstants.COMMA);
foreach (var val in values)
{
(var operation, var filterValue) = ParseFilterOperation(val);
queries.Add(new FilterQuery(propertyName, filterValue, operation));
}
}

return queries;
Expand All @@ -100,19 +109,15 @@ protected virtual (string operation, string value) ParseFilterOperation(string v
if (value.Length < 3)
return (string.Empty, value);

var operation = value.Split(QueryConstants.COLON);
var operation = GetFilterOperation(value);
var values = value.Split(QueryConstants.COLON);

if (operation.Length == 1)
return (string.Empty, value);

// remove prefix from value
if (Enum.TryParse(operation[0], out FilterOperations op) == false)
if (string.IsNullOrEmpty(operation))
return (string.Empty, value);

var prefix = operation[0];
value = string.Join(QueryConstants.COLON_STR, operation.Skip(1));
value = string.Join(QueryConstants.COLON_STR, values.Skip(1));

return (prefix, value);
return (operation, value);
}

protected virtual PageQuery ParsePageQuery(PageQuery pageQuery, string key, string value)
Expand Down Expand Up @@ -225,6 +230,20 @@ protected virtual AttrAttribute GetAttribute(string propertyName)
}
}

private string GetFilterOperation(string value)
{
var operation = value.Split(QueryConstants.COLON);

if (operation.Length == 1)
return string.Empty;

// remove prefix from value
if (Enum.TryParse(operation[0], out FilterOperations op) == false)
return string.Empty;

return operation[0];
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 assign operation[0] to a local var so we don't have to perform the index lookup twice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, no problem

}

private FilterQuery BuildFilterQuery(ReadOnlySpan<char> query, string propertyName)
{
var (operation, filterValue) = ParseFilterOperation(query.ToString());
Expand Down
71 changes: 71 additions & 0 deletions test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/AttributeFilterTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using System;
using System.Collections.Generic;
using System.Linq;
using System.Net;
using System.Net.Http;
Expand All @@ -8,6 +9,7 @@
using JsonApiDotNetCore.Serialization;
using JsonApiDotNetCoreExample.Data;
using JsonApiDotNetCoreExample.Models;
using Microsoft.EntityFrameworkCore;
using Newtonsoft.Json;
using Xunit;
using Person = JsonApiDotNetCoreExample.Models.Person;
Expand Down Expand Up @@ -131,5 +133,74 @@ public async Task Can_Filter_On_Not_Equal_Values()
Assert.Equal(HttpStatusCode.OK, response.StatusCode);
Assert.False(deserializedTodoItems.Any(i => i.Ordinal == todoItem.Ordinal));
}

[Fact]
public async Task Can_Filter_On_In_Array_Values()
Copy link
Contributor

Choose a reason for hiding this comment

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

need a negative test, i.e. instances with property values that are not included in the array get excluded from the results

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I'll add this test case.

{
// arrange
var context = _fixture.GetService<AppDbContext>();
var todoItems = _todoItemFaker.Generate(3);
var guids = new List<Guid>();
foreach (var item in todoItems)
{
context.TodoItems.Add(item);
guids.Add(item.GuidProperty);
}
context.SaveChanges();

var totalCount = context.TodoItems.Count();
var httpMethod = new HttpMethod("GET");
var route = $"/api/v1/todo-items?filter[guid-property]=in:{string.Join(",", guids)}";
var request = new HttpRequestMessage(httpMethod, route);

// act
var response = await _fixture.Client.SendAsync(request);
var body = await response.Content.ReadAsStringAsync();
var deserializedTodoItems = _fixture
.GetService<IJsonApiDeSerializer>()
.DeserializeList<TodoItem>(body);

// assert
Assert.Equal(HttpStatusCode.OK, response.StatusCode);
Assert.Equal(guids.Count(), deserializedTodoItems.Count());
foreach (var item in deserializedTodoItems)
Assert.True(guids.Contains(item.GuidProperty));
}

[Fact]
public async Task Can_Filter_On_Related_In_Array_Values()
Copy link
Contributor

Choose a reason for hiding this comment

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

👏 👏 👏

{
// arrange
var context = _fixture.GetService<AppDbContext>();
var todoItems = _todoItemFaker.Generate(3);
var ownerFirstNames = new List<string>();
foreach (var item in todoItems)
{
var person = _personFaker.Generate();
ownerFirstNames.Add(person.FirstName);
item.Owner = person;
context.TodoItems.Add(item);
}
context.SaveChanges();

var httpMethod = new HttpMethod("GET");
var route = $"/api/v1/todo-items?include=owner&filter[owner.first-name]=in:{string.Join(",", ownerFirstNames)}";
var request = new HttpRequestMessage(httpMethod, route);

// act
var response = await _fixture.Client.SendAsync(request);
var body = await response.Content.ReadAsStringAsync();
var documents = JsonConvert.DeserializeObject<Documents>(await response.Content.ReadAsStringAsync());
var included = documents.Included;

// assert
Assert.Equal(HttpStatusCode.OK, response.StatusCode);
Assert.Equal(ownerFirstNames.Count(), documents.Data.Count());
Assert.NotNull(included);
Assert.NotEmpty(included);
foreach (var item in included)
Assert.True(ownerFirstNames.Contains(item.Attributes["first-name"]));

}
}
}