Generate a LINQ Expression Tree from a filter string

Question

I have a C# Service that takes a filter string and a FilterMap class and generates an Expression<Func<T, bool>> for use within Linq to * Scenarios (currently it is in use to create dynamic filtering for documentdb queries).

Filters have the form *parameter*_*operation*_*value*, for example: firstName_eq_Joe. Multiple filters can be contained within the same string if they are space delimited, for example: firstName_eq_Joe lastName_eq_Bloggs.

The FilterMap class is a simple POCO that states what filters are available and where they exist on the target object. Each property on the FilterMap is a string, where the property name is the property part of a filter and its value is where that property lives on T.

Example FilterMap class:

public class FilterMap
{
    public string Reputation => "Reputation";
    public string FirstName => "User.FirstName";
    public string LastName => "User.LastName";
}

In the example above Reputation is mapped to the property Reputation. FirstName is mapped to an object property on T called User and then a property on that User object called FirstName.

The code for the service can be found below:

public class FilterService<TModel> : IFilterService<TModel> where TModel : class
{
    private readonly FilterTypeFactory _filterTypeFactory;
    private readonly Lazy<IEnumerable<PropertyInfo>> _modelPropertiesLazy;

    public FilterService()
    {
        _filterTypeFactory = new FilterTypeFactory();
        _modelPropertiesLazy = new Lazy<IEnumerable<PropertyInfo>>(() => typeof(TModel).GetTypeInfo().GetProperties(BindingFlags.Instance | BindingFlags.Public));
    }

    public Expression<Func<TModel, bool>> GenerateFilterFromFilterMapSync<TFilterMap>(string filterString) where TFilterMap : class, new()
    {
        IEnumerable<RawFilterParts> filters = ParseFilterString(filterString);

        if (!filters.Any())
        {
            return null;
        }

        IEnumerable<PropertyInfo> filterMapProperties = typeof(TFilterMap).GetTypeInfo().GetProperties(BindingFlags.Instance | BindingFlags.Public);

        TFilterMap filterMap = new TFilterMap();

        ParameterExpression modelParameterExpression = Expression.Parameter(typeof(TModel));

        Expression filterExpression = null;

        foreach (RawFilterParts filterParts in filters)
        {
            string propertyPath = GetPropertyPath(filterParts, filterMap, filterMapProperties);
            FilterInfo filterInfo = GetFilterInfoFromFilterMap(propertyPath, _modelPropertiesLazy.Value, modelParameterExpression);

            filterExpression = GetFilterExpression(filterExpression, filterParts, filterInfo);
        }

        return Expression.Lambda<Func<TModel, bool>>(filterExpression, modelParameterExpression);
    }

    private Expression GetFilterExpression(Expression filterExpression, RawFilterParts filterParts, FilterInfo filterInfo)
    {
        filterInfo.Operation = filterParts.Operation;
        filterInfo.Value = filterParts.Value;

        IFilterType filterType = _filterTypeFactory.GetFilterType(filterInfo.Property);

        Expression propertyExpression = filterType.GetPropertyExpression(filterInfo);

        if (filterExpression == null)
        {
            filterExpression = propertyExpression;
        }
        else
        {
            filterExpression = Expression.AndAlso(filterExpression, propertyExpression);
        }

        return filterExpression;
    }

    private string GetPropertyPath<TFilterMap>(RawFilterParts filterParts, TFilterMap filterMap, IEnumerable<PropertyInfo> filterMapProperties)
    {
        PropertyInfo property = filterMapProperties.FirstOrDefault(p => p.Name.ToLowerInvariant() == filterParts.Property.ToLowerInvariant());

        return (string)property.GetValue(filterMap);
    }

    private FilterInfo GetFilterInfoFromFilterMap(string propertyPath, IEnumerable<PropertyInfo> modelProperties, ParameterExpression parameterExpression)
    {
        Expression currentPropertyExpression = parameterExpression;
        PropertyInfo currentProperty;

        int dotIndex;

        do
        {
            dotIndex = propertyPath.IndexOf(".", StringComparison.Ordinal);

            string propertyName = dotIndex < 0 ? propertyPath : propertyPath.Substring(0, dotIndex);

            currentProperty = modelProperties.FirstOrDefault(pi => pi.Name.ToLowerInvariant() == propertyName.ToLowerInvariant());
            currentPropertyExpression = Expression.Property(currentPropertyExpression, currentProperty);

            if (dotIndex < 0)
            {
                break;
            }

            IEnumerable<PropertyInfo> subModelProperties = currentProperty.PropertyType.GetTypeInfo().GetProperties(BindingFlags.Instance | BindingFlags.Public);

            modelProperties = subModelProperties;
            propertyPath = propertyPath.Substring(dotIndex + 1);

        } while (dotIndex >= 0);

        return new FilterInfo()
        {
            AccessExpression = currentPropertyExpression,
            Property = currentProperty
        };
    }

    private IEnumerable<RawFilterParts> ParseFilterString(string filterString)
    {
        if (filterString == null)
        {
            throw new ArgumentNullException(nameof(filterString));
        }

        IEnumerable<string> filterStringSegments = filterString.Split(' ');

        List<RawFilterParts> filters = new List<RawFilterParts>(filterStringSegments.Count());

        foreach (string filterStringSegment in filterStringSegments)
        {
            string[] filterParts = filterStringSegment.Split('_');

            if (filterParts.Length != 3)
            {
                throw new InvalidOperationException($"Invalid Filter String Segment: {filterStringSegment}");
            }

            filters.Add(new RawFilterParts()
            {
                Property = filterParts[0],
                Operation = filterParts[1],
                Value = filterParts[2]
            });
        }

        return filters;
    }
}

The service uses the following internal DTOs to pass information between methods:

internal class RawFilterParts
{
    public string Property { get; set; }
    public string Operation { get; set; }
    public string Value { get; set; }
}

---

internal class FilterInfo
{
    public Expression AccessExpression { get; set; }
    public PropertyInfo Property { get; set; }
    public string Operation { get; set; }
    public string Value { get; set; }
}

The service also makes use of a IFilterType object which is created using a FilterTypeFactory. The FilterTypeFactory creates a different IFilterType depending on the type of the property that the filter corresponds to. The IFilterType defines what filter operations are available for that type (it throws if the operation is not valid) and generates the expression that performs the filter comparison.

FilterTypeFactory:

internal class FilterTypeFactory
{
    public IFilterType GetFilterType(PropertyInfo property)
    {
        TypeCode typeCode = Type.GetTypeCode(property.PropertyType);

        switch (typeCode)
        {
            case TypeCode.String:
                return new StringFilterType();
            case TypeCode.Boolean:
                return new BoolFilterType();
            case TypeCode.Int32:
                return new IntFilterType();
            case TypeCode.Double:
                return new DoubleFilterType();
            case TypeCode.DateTime:
                return new DateTimeFilterType();
            case TypeCode.Object:
                if (property.PropertyType == typeof(Guid))
                {
                    return new GuidFilterType();
                }

                if (property.PropertyType == typeof(Guid?))
                {
                    return new NullableFilterType<Guid>(new GuidFilterType());
                }

                if (property.PropertyType == typeof(DateTime?))
                {
                    return new NullableFilterType<DateTime>(new DateTimeFilterType());
                }

                if (property.PropertyType == typeof(int?))
                {
                    return new NullableFilterType<int>(new IntFilterType());
                }

                if (property.PropertyType == typeof(double?))
                {
                    return new NullableFilterType<double>(new DoubleFilterType());
                }

                if (property.PropertyType == typeof(bool?))
                {
                    return new NullableFilterType<bool>(new BoolFilterType());
                }

                break;
        }

        throw new InvalidOperationException($"The type {typeCode} is not supported for filtering for the property {property.Name}");
    }
}

IFilterType:

internal interface IFilterType
{
    Expression GetPropertyExpression(FilterInfo filterInfo);
}

The implementations of IFilterType share an abstract BaseFilterType which handles creating the Expression to return. The implementations of BaseFilterType simply defines the operations available for that Filter Type and handles converting the string value provided in the filter string into the required type for the filter.

internal abstract class BaseFilterType<T> : IFilterType
{
    internal readonly IEnumerable<string> _allowedOperations;

    protected BaseFilterType(IEnumerable<string> allowedOperations)
    {
        _allowedOperations = allowedOperations;
    }

    public Expression GetPropertyExpression(FilterInfo filterInfo)
    {
        if (!IsOperationAllowed(filterInfo.Operation))
        {
            throw new InvalidOperationException($"Filter string is invalid, operation {filterInfo.Operation} is not valid for {filterInfo.Property.Name}");
        }

        Expression valueExpression = Expression.Convert(Expression.Constant(GetFilterValue(filterInfo.Value)), filterInfo.Property.PropertyType);

        switch (filterInfo.Operation.ToLowerInvariant())
        {
            case "eq":
                return Expression.Equal(filterInfo.AccessExpression, valueExpression);
            case "ne":
                return Expression.NotEqual(filterInfo.AccessExpression, valueExpression);
            case "lt":
                return Expression.LessThan(filterInfo.AccessExpression, valueExpression);
            case "le":
                return Expression.LessThanOrEqual(filterInfo.AccessExpression, valueExpression);
            case "gt":
                return Expression.GreaterThan(filterInfo.AccessExpression, valueExpression);
            case "ge":
                return Expression.GreaterThanOrEqual(filterInfo.AccessExpression, valueExpression);
            case "ct":
                return Expression.Call(filterInfo.AccessExpression, typeof(string).GetRuntimeMethods().FirstOrDefault(mi => mi.Name == "Contains"), valueExpression);
            default:
                throw new InvalidOperationException($"Error getting expression for filter type, the operation {filterInfo.Operation} is not supported.");
        }
    }

    protected abstract T GetFilterValue(string value);

    private bool IsOperationAllowed(string operation)
    {
        return _allowedOperations.Contains(operation);
    }
}

Example FilterType Implementation:

internal class IntFilterType : BaseFilterType<int>
{
    public IntFilterType() : base(FilterOperations.NumericOperations)
    {
    }

    protected override int GetFilterValue(string value)
    {
        int integer;

        bool success = Int32.TryParse(value, out integer);

        if (success)
        {
            return integer;
        }

        throw new ArgumentException("Value is not a valid integer");
    }
}

FilterOperations is a static class that provides preconfigured arrays of available operations for the types.

I'm looking for design and clean code advice, but also any tips on obvious performance issues would be helpful.

Answer 1

Good things

• Everything is nicely encapsulated and each class has only one responsibility. Not only the big ones but also the small ones like RawFilterParts or FilterInfo.
• You use abstractions and generics that allow you to add new filters without modifying the core implementation.

---

Improvable

private readonly Lazy<IEnumerable<PropertyInfo>> _modelPropertiesLazy;

Lazy fields or properties make sense when there is a high probability that they won't actually be used so you save processing time by not calculating their values. Here however you'll always use it because the FilterService class has only a single public API and it uses this field. You could make it non-lazy and nothing would change.

---

IEnumerable<RawFilterParts> filters = ParseFilterString(filterString);

if (!filters.Any())
{
  return null;
}

Beware of the Any when working with an IEnumerable<> because it could be enumerated multiple times. The ParseFilterString API creates a List internally but returns an IEnumerable so it is not clear to the user (even to another programmer) that he doesn't need to call ParseFilterString(..).ToList() to cache the results. ParseFilterString should either return a List<> an IList<> or a true deferred IEnumerable<> by using yield return.

---

if (filterExpression == null)
{
  filterExpression = propertyExpression;
}
else
{
  filterExpression = Expression.AndAlso(filterExpression, propertyExpression);
}

return filterExpression;

or simpler

return 
    filterExpression == null
        ? propertyExpression
        : Expression.AndAlso(filterExpression, propertyExpression);

---

p.Name.ToLowerInvariant() == filterParts.Property.ToLowerInvariant()

or safer and less error prone

p.Name.Equals(filterParts.Property, StringComparison.OrdinalIgnoreCase)

---

IEnumerable<string> filterStringSegments = filterString.Split(' ');

List<RawFilterParts> filters = new List<RawFilterParts>(filterStringSegments.Count());

don't you like var?

var filterStringSegments = filterString.Split(' ');

var filters = new List<RawFilterParts>(filterStringSegments.Count());

and you are not consistant because later it's not an IEnumerable anymore but string[]

string[] filterParts = filterStringSegment.Split('_');

---

throw new InvalidOperationException($"Invalid Filter String Segment: {filterStringSegment}");

This is the exception that is thrown by the ParseFilterString API but it's the wrong one. InvalidOperationException means that a method has been called or a property used while the state of the class is invalid for this call. Here the ArgumentException should be thrown or even better a custom exception like InvalidFilterSegmentException.

---

With everything suggested so far the ParseFilterString method could be simplified to:

private IEnumerable<RawFilterParts> ParseFilterString2(string filterString)
{
    if (filterString == null)
    {
        throw new ArgumentNullException(nameof(filterString));
    }

    foreach (string filterStringSegment in filterString.Split(' '))
    {
        var filterParts = filterStringSegment.Split('_');

        if (filterParts.Length != 3)
        {
            throw new ArgumentException($"Invalid Filter String Segment: {filterStringSegment}");
        }

        yield return new RawFilterParts
        {
            Property = filterParts[0],
            Operation = filterParts[1],
            Value = filterParts[2]
        };
    }
}

---

The GetPropertyExpression method in the BaseFilterType class is throwing the InvalidOperationException again but this one is wrong too. It should throw either the ArgumentOutOfRangeException or better a custom exception like InvalidOperatorException - notice it's called Operator not Operation. The property also should be changed to Operator because it is an operator.

See: https://en.wikipedia.org/wiki/Operator_(computer_programming)

As the other review suggested, the operators should be enum values.

This might give you some inspiration about operators: http://www.tomsitpro.com/articles/powershell-comparison-operators-conditional-logic,2-850.html

---

I think there are still more things that could be improved but the code is not complete (some interfaces and classes are missing) so it's hard to judge the overall design.

Answer 2

internal abstract class BaseFilterType<T>

The GetPropertyExpression() just got into my eyes asking myself what all of the case evaluations means. The same will likely happen to Sam the maintainer while hunting down a bug or adding functionality to that class.

Instead of having values in the case statements like "eq", "ne", "ct" etc you should either extract this to meaningful constants like e.g

private const string equal = eq";
private const string notEqual = "ne";  

or use an enum like so

public enum Operation
{
    Equal, NotEqual, GreaterThan, and so on
}  

Using an enum approach would need to change your FilterInfo class to use this Operation enum instead of string but would come with the benefit that you wouldn't need to call ToLowerInvariant().

You could replace the internal readonly IEnumerable<string> _allowedOperations; with Operation as well by using the [FlagsAttribute] on the enum so you can combine the enums by AND, OR, EXCLUSIVE OR. Using the FalgsAttribute should go hand in hand with naming the enum using a plural form, hence Operations should then be used.

---

internal class FilterTypeFactory

I would extract the case TypeCode.Object: in the GetFilterType() method to a separate method because that method/case is too big.

In addition instead of accessing the PropertyType of the PropertyInfo over and over again, I would access it once, store it in a variable and check against this variable.