Skip to content
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

Added a new filter expression for multiple search on strings #205

Merged
merged 12 commits into from
Oct 25, 2018
20 changes: 20 additions & 0 deletions Saule/Queries/Filtering/FilterContext.cs
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,26 @@ where query.Key.StartsWith(Constants.QueryNames.Filtering)
/// </summary>
public QueryFilterExpressionCollection QueryFilters { get; internal set; } = new QueryFilterExpressionCollection();

/// <summary>
/// checking that specified property exists and returns converted value for it
/// </summary>
/// <typeparam name="T">Type of property</typeparam>
/// <param name="name">property name</param>
/// <param name="value">property value</param>
/// <returns>true if property is specified. Otherwise false</returns>
public bool TryGetValue<T>(string name, out List<T> value)
{
var property = Properties.FirstOrDefault(p => p.Name == name);
value = new List<T>();
if (property == null)
{
return false;
}

value = property.Values.Select(v => (T)Lambda.TryConvert(v, typeof(T))).ToList();
return true;
}

/// <summary>
/// checking that specified property exists and returns converted value for it
/// </summary>
Expand Down
8 changes: 4 additions & 4 deletions Saule/Queries/Filtering/FilterInterpreter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ public IQueryable Apply(IQueryable queryable)
if (_context.Properties.Any())
{
return _context.Properties
.Select(p => p.Name == "Id" ? new FilterProperty(_resource.IdProperty, p.Value) : p)
.Select(p => p.Name == "Id" ? new FilterProperty(_resource.IdProperty, p.Values) : p)
.Aggregate(queryable, ApplyProperty);
}

Expand All @@ -33,7 +33,7 @@ public IEnumerable Apply(IEnumerable enumerable)
if (_context.Properties.Any())
{
return _context.Properties
.Select(p => p.Name == "Id" ? new FilterProperty(_resource.IdProperty, p.Value) : p)
.Select(p => p.Name == "Id" ? new FilterProperty(_resource.IdProperty, p.Values) : p)
.Aggregate(enumerable, ApplyProperty);
}

Expand All @@ -57,7 +57,7 @@ private IEnumerable ApplyProperty(IEnumerable enumerable, FilterProperty propert

enumerable = enumerable.ApplyQuery(
QueryMethod.Where,
Lambda.SelectPropertyValue(elementType, property.Name, property.Value, _context.QueryFilters))
Lambda.SelectPropertyValue(elementType, property.Name, property.Values, _context.QueryFilters))
as IEnumerable;

return enumerable;
Expand All @@ -74,7 +74,7 @@ private IQueryable ApplyProperty(IQueryable queryable, FilterProperty property)
{
queryable = queryable.ApplyQuery(
QueryMethod.Where,
Lambda.SelectPropertyValue(queryable.ElementType, property.Name, property.Value, _context.QueryFilters))
Lambda.SelectPropertyValue(queryable.ElementType, property.Name, property.Values, _context.QueryFilters))
as IQueryable;
return queryable;
}
Expand Down
85 changes: 81 additions & 4 deletions Saule/Queries/Filtering/FilterProperty.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,9 @@
namespace Saule.Queries.Filtering
using System;
using System.Collections.Generic;
using System.Linq;
using System.Text.RegularExpressions;

namespace Saule.Queries.Filtering
{
/// <summary>
/// Property for filtering
Expand All @@ -9,11 +14,78 @@ public class FilterProperty
/// Initializes a new instance of the <see cref="FilterProperty"/> class.
/// </summary>
/// <param name="name">property name</param>
/// <param name="value">property value</param>
public FilterProperty(string name, string value)
/// <param name="values">property values in one string in csv notation</param>
public FilterProperty(string name, string values)
{
Value = value;
Name = name.ToPascalCase();
Value = values;

// Spliting the string into multiple values with csv notation
Values = new List<string>();

// If there is no comma we add the whole string as one value
if (!values.Contains(','))
{
Values.Add(values);
return;
}

// Check for a multiple values in case of quotes
// If the string does start with a quote it has to have an end quote followed by a comma.
bool multipleValues = !(values.StartsWith("\"") && !values.Contains("\","));

if (!multipleValues)
{
Values.Add(values);
return;
}

/*
* This Regex contains two matching groups
*
* Regex in plain: (?:,"|^")(""|[\w\W]*?)(?=",|"$)|(?:,(?!")|^(?!"))([^,]*?)(?=$|,)
*
* 1. (?:,"|^")(""|[\w\W]*?)(?=",|"$)
* It starts of with a non capturing group on commas followed by quotes or just
* quotes at the beginning. Then we will look for as few words as possible until the next
* quotes followed by a comma or the end of the string. This will handle all the cases of
* values enclosed by quotes.
*
* 2. (?:,(?!")|^(?!"))([^,]*?)(?=$|,)
* This starts of with a big non capturing group. Either a comma NOT followed by quotes or
* no quotes at the start of the string. Then it will match until the next comma is is reached.
* The last part again is to make sure it ends with either a comma or the end of the string.
* This group handles all cases with simple comma separation.
*/
var match = new Regex("(?:,\"|^\")(\"\"|[\\w\\W]*?)(?=\",|\"$)|(?:,(?!\")|^(?!\"))([^,]*?)(?=$|,)").Matches(values);
Copy link
Owner

Choose a reason for hiding this comment

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

Can you maybe put a comment that explains this regex? Some time in the future we may want to remember how it works.

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 tried my best to explain this one. I hope that it's enough.

foreach (Match matchCapture in match)
{
var captureValue = string.IsNullOrEmpty(matchCapture.Groups[1].Value) ? matchCapture.Groups[2].Value : matchCapture.Groups[1].Value;

// We need to check if it is empty because capture group 2 will have some empty results e.g.for a trailing comma.
if (!string.IsNullOrEmpty(captureValue))
{
Values.Add(captureValue);
}
}

// If there were no matches we have a malformed csv and we just add it as one filter
if (Values.Count == 0)
{
Values.Add(values);
}
}

/// <summary>
/// Initializes a new instance of the <see cref="FilterProperty"/> class.
/// </summary>
/// <param name="name">property name</param>
/// <param name="values">property values</param>
public FilterProperty(string name, List<string> values)
{
Name = name.ToPascalCase();
Values = values;
Value = string.Join(",", values);
}

/// <summary>
Expand All @@ -24,6 +96,11 @@ public FilterProperty(string name, string value)
/// <summary>
/// Gets property value
/// </summary>
public List<string> Values { get; }

/// <summary>
/// Gets property string value
/// </summary>
public string Value { get; }

/// <inheritdoc/>
Expand Down
27 changes: 22 additions & 5 deletions Saule/Queries/Lambda.cs
Original file line number Diff line number Diff line change
@@ -1,18 +1,28 @@
using System;
using System.Collections;
using System.Collections.Generic;
using System.ComponentModel;
using System.Linq;
using System.Linq.Expressions;
using System.Reflection;
using Saule.Http;
using Expression = System.Linq.Expressions.Expression;

namespace Saule.Queries
{
internal static class Lambda
{
public static Expression SelectPropertyValue(Type type, string property, string value, QueryFilterExpressionCollection queryFilter)
public static Expression SelectPropertyValue(Type type, string property, List<string> values, QueryFilterExpressionCollection queryFilter)
{
var valueType = GetPropertyType(type, property);
var parsedValue = TryConvert(value, valueType);
var parsedValuesAsObjects = values.Select(v => TryConvert(v, valueType)).ToList();

var parsedValues = (IList)Activator.CreateInstance(typeof(List<>).MakeGenericType(valueType));
foreach (var parsedValueAsObject in parsedValuesAsObjects)
{
parsedValues.Add(parsedValueAsObject);
}

var param = Expression.Parameter(type, "i");
var propertyExpression = Expression.Property(param, property);

Expand All @@ -21,7 +31,7 @@ public static Expression SelectPropertyValue(Type type, string property, string
return typeof(Lambda)
.GetMethod(nameof(Convert), BindingFlags.Static | BindingFlags.NonPublic)
.MakeGenericMethod(valueType, type)
.Invoke(null, new[] { expression, parsedValue, propertyExpression, param })
.Invoke(null, new object[] { expression, parsedValues, propertyExpression, param })
as Expression;
}

Expand All @@ -47,11 +57,18 @@ internal static object TryConvert(string value, Type type)
// ReSharper disable once UnusedMethodReturnValue.Local
private static Expression<Func<TClass, bool>> Convert<TProperty, TClass>(
Expression<Func<TProperty, TProperty, bool>> expression,
TProperty constant,
List<TProperty> constant,
MemberExpression propertyExpression,
ParameterExpression parameter)
{
var curriedBody = new FilterLambdaVisitor<TProperty>(propertyExpression, constant).Visit(expression.Body);
// initialize the expression with a always false one to make chaining possible
Expression curriedBody = null;
foreach (TProperty c in constant)
{
// Initialize expression if this is the first loop, else chain the expression with an "orElse" Expression
curriedBody = curriedBody == null ? new FilterLambdaVisitor<TProperty>(propertyExpression, c).Visit(expression.Body) : Expression.OrElse(curriedBody, new FilterLambdaVisitor<TProperty>(propertyExpression, c).Visit(expression.Body));
}

return Expression.Lambda<Func<TClass, bool>>(curriedBody, parameter);
}

Expand Down
1 change: 1 addition & 0 deletions Tests/Controllers/PeopleController.cs
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ public IEnumerable<Person> ManualQueryAndPaginatePeople(QueryContext context)
bool includeCar = context.Include.Includes.Any(p => p.Name == nameof(Person.Car));
bool includeJob = context.Include.Includes.Any(p => p.Name == nameof(Person.Job));


context.Filter.TryGetValue("HideLastName", out hideLastName);

if (hideLastName.GetValueOrDefault() || !includeCar)
Expand Down
5 changes: 3 additions & 2 deletions Tests/Helpers/Get.cs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@ internal static class Get
{
"Summers", "Bockman", "Duque", "Cline", "Neufeld", "Mcray", "Hix",
"Daniel", "Baumbach", "Forry", "Bozek", "Chichester", "Petri", "Folk",
"Yadon", "Holliday", "Paniagua", "Hofstetter", "Vasques", "Russel"
"Yadon", "Holliday", "Paniagua", "Hofstetter", "Vasques", "Russel",
"Comma,Test", "\"Quote,Test",
};

private static readonly string[] StreetNames =
Expand All @@ -28,7 +29,7 @@ internal static class Get
"Atlantic Avenue", "Lincoln Avenue", "Route 10", "Water Street",
"Brookside Drive", "Hillcrest Drive", "Madison Avenue", "Union Street",
"Lake Avenue", "6th Street", "Broad Street West", "Market Street",
"North Street", "Heritage Drive", "Cooper Street", "Route 44"
"North Street", "Heritage Drive", "Cooper Street", "Route 44",
};

public static IEnumerable<Person> People()
Expand Down
72 changes: 70 additions & 2 deletions Tests/Queries/FilterInterpreterTests.cs
Original file line number Diff line number Diff line change
@@ -1,10 +1,8 @@
using System.Collections.Generic;
using System.Linq;
using System.Linq.Expressions;
using Saule;
using Saule.Queries;
using Saule.Queries.Filtering;
using Saule.Queries.Sorting;
using Tests.Helpers;
using Tests.Models;
using Xunit;
Expand Down Expand Up @@ -97,6 +95,20 @@ public void WorksOnInts()
Assert.Equal(expected, result);
}

[Fact(DisplayName = "Applies filtering on multiple ints")]
public void WorksOnMultipleInts()
{
var people = Get.People(100).ToList().AsQueryable();
var expected = people.Where(c => c.Age == 20 || c.Age == 30).ToList();

var query = GetQuery(new[] { "Age" }, new[] { "20,30" });

var result = Query.ApplyFiltering(people, new FilterContext(query), new PersonResource())
as IQueryable<Person>;

Assert.Equal(expected, result);
}

[Fact(DisplayName = "Applies filtering on enums (string)")]
public void WorksOnEnumsAsStrings()
{
Expand All @@ -111,6 +123,20 @@ public void WorksOnEnumsAsStrings()
Assert.Equal(expected, result);
}

[Fact(DisplayName = "Applies filtering on multiple enums (string)")]
public void WorksOnMultipleEnumsAsStrings()
{
var companies = Get.Companies(100).ToList().AsQueryable();
var expected = companies.Where(c => c.Location == LocationType.National || c.Location == LocationType.Local).ToList();

var query = GetQuery(new[] { "Location" }, new[] { "national,local" });

var result = Query.ApplyFiltering(companies, new FilterContext(query), new CompanyResource())
as IQueryable<Company>;

Assert.Equal(expected, result);
}

[Fact(DisplayName = "Applies filtering on strings")]
public void WorksOnStrings()
{
Expand All @@ -125,6 +151,48 @@ public void WorksOnStrings()
Assert.Equal(expected, result);
}

[Fact(DisplayName = "Applies filtering on strings with spaces")]
public void WorksOnStringsWithSpaces()
{
var companies = Get.Companies(100).ToList().AsQueryable();
var expected = companies.Where(c => c.Name == "Awesome Inc.").ToList();

var query = GetQuery(new[] { "Name" }, new[] { "Aweseom Inc." });

var result = Query.ApplyFiltering(companies, new FilterContext(query), new CompanyResource())
as IQueryable<Company>;

Assert.Equal(expected, result);
}

[Fact(DisplayName = "Applies filtering on strings with multiple values")]
public void WorksOnStringsMultiple()
{
var people = Get.People(100).ToList().AsQueryable();
var expected = people.Where(c => c.LastName == "Russel" || c.LastName == "Comma,Test").ToList();

var query = GetQuery(new[] { "LastName" }, new[] { "Russel,\"Comma,Test\"" });
Copy link
Owner

Choose a reason for hiding this comment

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

Maybe we can also see what happens on some edge cases/malformed strings such as "a"b (no comma), "ab,c (no end quote), etc.

FWIW i'm not sure what should happen in this case.. I would be fine with a 400 although technically that would be a breaking change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right now this is failing silently as anything not captured will be ignored. It's certainly possible to add a 400. I am not sure if this would be the best way tho.

Copy link
Owner

Choose a reason for hiding this comment

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

I would vote that either

  1. We throw an error for this (400 bad request)
  2. We identify the filter as "malformed" and interpret it as a single filter value

I don't think we should let parts drop, throw 500, etc.

I think you're right that option 2 is probably better.

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 added a few checks and one more unit test for this.


var result = Query.ApplyFiltering(people, new FilterContext(query), new PersonResource())
as IQueryable<Person>;

Assert.Equal(expected, result);
}

[Fact(DisplayName = "Applies filtering on strings with multiple values including quotes and commas")]
public void WorksOnStringsMultipleWithQuotes()
{
var people = Get.People(100).ToList().AsQueryable();
var expected = people.Where(c => c.LastName == "\"Quote,Test").ToList();

var query = GetQuery(new[] { "LastName" }, new[] { "\"Quote,Test" });

var result = Query.ApplyFiltering(people, new FilterContext(query), new PersonResource())
as IQueryable<Person>;

Assert.Equal(expected, result);
}

[Fact(DisplayName = "Works on enumerables")]
public void WorksOnEnumerables()
{
Expand Down