Why runtime Expressions cause collisions on the Cache of Entity Framework Core 5?

Question

Before I forget it, my execution context, I'm using .Net 5 with the packages:

• Microsoft.EntityFrameworkCore.Design 5.0.6
• Microsoft.EntityFrameworkCore.Relational 5.0.6
• MySql.EntityFrameworkCore 5.0.3.1

My main goal was to remove the repetitive task of doing expressions when I need to retrieve entities, something like:

public class GetListEntity
{
   property int QueryProperty { get; set }
}

public class Entity
{
   property int Property { get; set }
}

public async Task<ActionResult> List(GetListEntity getListEntity)
{
   var restrictions = new List<Expression<Func<Entity>
   if (model.QueryProperty != null)
   { 
      restrictions.Add(e => e.Property == model.QueryProperty);
   }
   nonTrackedQueryableEntities = this.dbContext.Set<Entity>()
                                               .AsNoTracking();

   var expectedEntity = restrictions.Aggregate((sr, nr) => sr.And(nr)); //The And method is below as an extension
   var expectedNonTrackedQueryableEntities = nonTrackedQueryableEntities.Where(expectedEntity);

   // I will get the total first because the API was meant to paginate the responses.
   var total = await expectedNonTrackedQueryableEntities.CountAsync();
}


public static class ExpressionExtensions
{
    public static Expression<Func<T, bool>> Or<T>(this Expression<Func<T, bool>> selfExpression, Expression<Func<T, bool>> otherExpression)
    {
        return selfExpression.Compose(otherExpression, Expression.OrElse);
    }

    public static Expression<Func<T, bool>> And<T>(this Expression<Func<T, bool>> selfExpression, Expression<Func<T, bool>> otherExpression)
    {
        return selfExpression.Compose(otherExpression, Expression.AndAlso);
    }

    private static InvocationExpression Casting<T>(this Expression<Func<T, bool>> selfExpression, Expression<Func<T, bool>> otherExpression)
    {
        return Expression.Invoke(otherExpression, selfExpression.Parameters.Cast<Expression>());
    }

    private static Expression<Func<T, bool>> Compose<T>(this Expression<Func<T, bool>> selfExpression, Expression<Func<T, bool>> otherExpression, Func<Expression, Expression, Expression> merge)
    {
        var invocationExpression = selfExpression.Casting(otherExpression);
        return Expression.Lambda<Func<T, bool>>(merge(selfExpression.Body, invocationExpression), selfExpression.Parameters);
    }
}

I've managed to achieve what I wanted but let's say... partially, because if I try to Query the Database at least two times in a row I get this exception:

System.ArgumentException: An item with the same key has already been added. Key: e
   at System.Collections.Generic.Dictionary`2.TryInsert(TKey key, TValue value, InsertionBehavior behavior)
   at Microsoft.EntityFrameworkCore.Query.ExpressionEqualityComparer.ExpressionComparer.CompareLambda(LambdaExpression a, LambdaExpression b)
   at Microsoft.EntityFrameworkCore.Query.ExpressionEqualityComparer.ExpressionComparer.Compare(Expression left, Expression right)
   at Microsoft.EntityFrameworkCore.Query.ExpressionEqualityComparer.ExpressionComparer.Compare(Expression left, Expression right)
   at Microsoft.EntityFrameworkCore.Query.ExpressionEqualityComparer.ExpressionComparer.CompareBinary(BinaryExpression a, BinaryExpression b)
   at Microsoft.EntityFrameworkCore.Query.ExpressionEqualityComparer.ExpressionComparer.Compare(Expression left, Expression right)
   at Microsoft.EntityFrameworkCore.Query.ExpressionEqualityComparer.ExpressionComparer.CompareLambda(LambdaExpression a, LambdaExpression b)
   at Microsoft.EntityFrameworkCore.Query.ExpressionEqualityComparer.ExpressionComparer.Compare(Expression left, Expression right)
   at Microsoft.EntityFrameworkCore.Query.ExpressionEqualityComparer.ExpressionComparer.CompareUnary(UnaryExpression a, UnaryExpression b)
   at Microsoft.EntityFrameworkCore.Query.ExpressionEqualityComparer.ExpressionComparer.Compare(Expression left, Expression right)
   at Microsoft.EntityFrameworkCore.Query.ExpressionEqualityComparer.ExpressionComparer.CompareExpressionList(IReadOnlyList`1 a, IReadOnlyList`1 b)
   at Microsoft.EntityFrameworkCore.Query.ExpressionEqualityComparer.ExpressionComparer.CompareMethodCall(MethodCallExpression a, MethodCallExpression b)
   at Microsoft.EntityFrameworkCore.Query.ExpressionEqualityComparer.ExpressionComparer.Compare(Expression left, Expression right)
   at Microsoft.EntityFrameworkCore.Query.ExpressionEqualityComparer.ExpressionComparer.CompareExpressionList(IReadOnlyList`1 a, IReadOnlyList`1 b)
   at Microsoft.EntityFrameworkCore.Query.ExpressionEqualityComparer.ExpressionComparer.CompareMethodCall(MethodCallExpression a, MethodCallExpression b)
   at Microsoft.EntityFrameworkCore.Query.ExpressionEqualityComparer.ExpressionComparer.Compare(Expression left, Expression right)
   at Microsoft.EntityFrameworkCore.Query.ExpressionEqualityComparer.ExpressionComparer.CompareExpressionList(IReadOnlyList`1 a, IReadOnlyList`1 b)
   at Microsoft.EntityFrameworkCore.Query.ExpressionEqualityComparer.ExpressionComparer.CompareMethodCall(MethodCallExpression a, MethodCallExpression b)
   at Microsoft.EntityFrameworkCore.Query.ExpressionEqualityComparer.ExpressionComparer.Compare(Expression left, Expression right)
   at Microsoft.EntityFrameworkCore.Query.ExpressionEqualityComparer.Equals(Expression x, Expression y)
   at Microsoft.EntityFrameworkCore.Query.CompiledQueryCacheKeyGenerator.CompiledQueryCacheKey.Equals(CompiledQueryCacheKey other)
   at Microsoft.EntityFrameworkCore.Query.RelationalCompiledQueryCacheKeyGenerator.RelationalCompiledQueryCacheKey.Equals(RelationalCompiledQueryCacheKey other)
   at MySql.EntityFrameworkCore.Query.Internal.MySQLCompiledQueryCacheKeyGenerator.MySQLCompiledQueryCacheKey.Equals(MySQLCompiledQueryCacheKey other)
   at MySql.EntityFrameworkCore.Query.Internal.MySQLCompiledQueryCacheKeyGenerator.MySQLCompiledQueryCacheKey.Equals(Object obj)
   at System.Collections.Concurrent.ConcurrentDictionary`2.TryGetValue(TKey key, TValue& value)
   at Microsoft.Extensions.Caching.Memory.MemoryCache.TryGetValue(Object key, Object& result)
   at Microsoft.Extensions.Caching.Memory.CacheExtensions.TryGetValue[TItem](IMemoryCache cache, Object key, TItem& value)
   at Microsoft.EntityFrameworkCore.Query.Internal.CompiledQueryCache.GetOrAddQuery[TResult](Object cacheKey, Func`1 compiler)
   at Microsoft.EntityFrameworkCore.Query.Internal.QueryCompiler.ExecuteAsync[TResult](Expression query, CancellationToken cancellationToken)
   at Microsoft.EntityFrameworkCore.Query.Internal.EntityQueryProvider.ExecuteAsync[TResult](Expression expression, CancellationToken cancellationToken)
   at Microsoft.EntityFrameworkCore.EntityFrameworkQueryableExtensions.ExecuteAsync[TSource,TResult](MethodInfo operatorMethodInfo, IQueryable`1 source, Expression expression, CancellationToken cancellationToken)
   at Microsoft.EntityFrameworkCore.EntityFrameworkQueryableExtensions.ExecuteAsync[TSource,TResult](MethodInfo operatorMethodInfo, IQueryable`1 source, CancellationToken cancellationToken)
   at Microsoft.EntityFrameworkCore.EntityFrameworkQueryableExtensions.CountAsync[TSource](IQueryable`1 source, CancellationToken cancellationToken)'
   

Following the trace I managed to discover that the ORM is caching for some reason my expressions (and putting the parameter name, in this case 'e') and failing to detect a key collision the second time it has a similar expression to query the database. I said for some reason because, it's not the main deal but at least is odd that cache is involved in a non tracked query, maybe I'm missing something in the middle.

To undenrstand how i got here i will put the code below.

First an interface to implement in every model related with querying a list of entities and expose the extension method ListRestrictions (almost at the bottom).

public interface IEntityFilter<TEntity>
{ 
}

The next step was to define Attributes to summarize the action to do with the property and generate a partial expression to use in the extension method:

    [AttributeUsage(AttributeTargets.Property, AllowMultiple = false)]
    public abstract class FilterByPropertyAttribute : Attribute
    {
        protected string FirstPropertyPath { get; }

        protected IEnumerable<string> NPropertyPath { get; }

        public FilterByPropertyAttribute(string firstPropertyPath, params string[] nPropertyPath)
        {
            this.FirstPropertyPath = firstPropertyPath;
            this.NPropertyPath = nPropertyPath;
        }

        protected MemberExpression GetPropertyExpression(ParameterExpression parameterExpression)
        {
            var propertyExpression = Expression.Property(parameterExpression, this.FirstPropertyPath);
            foreach (var propertyPath in this.NPropertyPath)
            {
                propertyExpression = Expression.Property(propertyExpression, propertyPath);
            }
            return propertyExpression;
        }

       public abstract Expression GetExpression(ParameterExpression parameterExpression, object propertyValue);
    }

And to avoid comparisons with nullable structs

    public abstract class NonNullableValuePropertyFilterAttribute : FilterByPropertyAttribute
    {
        public NonNullableValuePropertyFilterAttribute(string firstPropertyPath, params string[] nPropertyPath)
            : base(firstPropertyPath, nPropertyPath)
        {
        }

        public override Expression GetExpression(ParameterExpression parameterExpression, object propertyValue)
        {
            var propertyExpression = this.GetPropertyExpression(parameterExpression);
            return this.GetExpression(propertyExpression, this.GetConvertedConstantExpression(propertyExpression, Expression.Constant(propertyValue)));
        }

        protected abstract Expression GetExpression(MemberExpression memberExpression, UnaryExpression unaryExpression);

        private UnaryExpression GetConvertedConstantExpression(MemberExpression memberExpression, ConstantExpression constantExpression)
        {
            var convertedConstantExpression = Expression.Convert(constantExpression, memberExpression.Type);
            return convertedConstantExpression;
        }
    }

An Attribute with a defined role would be:

    public class EqualPropertyFilterAttribute : NonNullableValuePropertyFilterAttribute
    {

        public EqualPropertyFilterAttribute(string firstPropertyPath, params string[] nPropertyPath)
            : base(firstPropertyPath, nPropertyPath)
        {
        }

        protected override Expression GetExpression(MemberExpression memberExpression, UnaryExpression unaryExpression)
        {
            return Expression.Equal(memberExpression, unaryExpression);
        }
    }

And last, the extension itself:

    public static class EntityFilterExtensions
    {
        public static List<Expression<Func<TEntity, bool>>> ListRestrictions<TEntity>(this IEntityFilter<TEntity> entityFilter)
        {
            var entityFilterType = entityFilter.GetType();            
            var propertiesInfo = entityFilterType.GetProperties()
                                                 .Where(pi => pi.GetValue(entityFilter) != null 
                                                              && pi.CustomAttributes.Any(ca => ca.AttributeType
                                                                                                 .IsSubclassOf(typeof(FilterByPropertyAttribute))));

            var expressions = Enumerable.Empty<Expression<Func<TEntity, bool>>>();
            if (propertiesInfo.Any())
            {
                var entityType = typeof(TEntity);
                var parameterExpression = Expression.Parameter(entityType, "e");
                expressions =  propertiesInfo.Select(pi =>
                {
                    var filterByPropertyAttribute = Attribute.GetCustomAttribute(pi, typeof(FilterByPropertyAttribute)) as FilterByPropertyAttribute;
                    var propertyValue = pi.GetValue(entityFilter);
                    var expression = filterByPropertyAttribute.GetExpression(parameterExpression, propertyValue);
                    return Expression.Lambda<Func<TEntity, bool>>(expression, parameterExpression);
                });
            }

            return expressions.ToList();
        }
    }

A usage would be:

public class GetListEntity : IEntityFilter<Entity>
{
   [EqualPropertyFilter(nameof(Entity.Property))]
   property int QueryProperty { get; set }
}

public class Entity
{
   property int Property { get; set }
}

public async Task<ActionResult> List(GetListEntity getListEntity)
{
   var restrictions = getListEntity.ListRestrictions();
   nonTrackedQueryableEntities = this.dbContext.Set<Entity>()
                                               .AsNoTracking();

   var expectedEntity = restrictions.Aggregate((sr, nr) => sr.And(nr));
   var expectedNonTrackedQueryableEntities = nonTrackedQueryableEntities .Where(expectedEntity);

   // I will get the total first because the API was meant to paginate the responses.
   var total = await expectedNonTrackedQueryableEntities.CountAsync();
}

And to be discarded, if I Aggregate a non dynamic expression of a list of expressions, the ORM works fine, when I do it with the dynamic ones I get the exception at the beginning.

I found a workaround, changing in the extension method this line:

var parameterExpression = Expression.Parameter(entityType, "e");

For this one:

var parameterExpression = Expression.Parameter(entityType, $"{entityType.Name}{entityFilter.GetHashCode()}");

I wanna know why this happens and maybe if there is another way to fix it. I posted here before opening a thread in any Github repository because I'm still curious if is a fault of mine for missing something in the way or a bug.

Answer 1

From the explanations it was pretty clear that there is some issue with ParameterExpressions of the dynamically built predicates. And at the end it was in the one of the custom expression extension methods used.

While technically it could be considered ORM bug/issue, they have to solve very complex things during the expression tree transformation, so we must be tolerant and fix our code when possible.

There are some important things you need to be aware of when building dynamically query expression trees.

First, the name of the used ParameterExpressions doesn't matter - they are identified by reference. It's perfectly fine to have all parameters with one and the same name (something that C# compiler won't allow you to create at compile time) as soon as they are separate instances properly referenced by the other expressions.

Second, some things which make sense when creating expression trees for compiling and executing as code (like in LINQ to Objects) are not good for expression trees which are supposed to be transformed to something else (they are valid, but make the transformation harder and lead to bugs/issues). Specifically (what was causing the issue in question) is "calling" lambda expressions. Yes, there is a dedicated Expression.Invoke, but it is causing issues with almost all IQueryable implementations, so it is better to emulate it by "inlining" it, which means replacing parameter instances inside the body with actual expressions.

Here is the modified version of your ExpressionExtensions class applying the aforementioned principle:

public static partial class ExpressionExtensions
{
    public static Expression<Func<T, bool>> And<T>(this Expression<Func<T, bool>> left, Expression<Func<T, bool>> right)
        => Combine(left, right, ExpressionType.AndAlso);

    public static Expression<Func<T, bool>> Or<T>(this Expression<Func<T, bool>> left, Expression<Func<T, bool>> right)
        => Combine(left, right, ExpressionType.OrElse);

    private static Expression<Func<T, bool>> Combine<T>(Expression<Func<T, bool>> left, Expression<Func<T, bool>> right, ExpressionType type)
    {
        if (left is null) return right;
        if (right is null) return left;
        bool constValue = type == ExpressionType.AndAlso ? false : true;
        if ((left.Body as ConstantExpression)?.Value is bool leftValue)
            return leftValue == constValue ? left : right;
        if ((right.Body as ConstantExpression)?.Value is bool rightValue)
            return rightValue == constValue ? right : left;
        return Expression.Lambda<Func<T, bool>>(Expression.MakeBinary(type,
            left.Body, right.Invoke(left.Parameters[0])),
            left.Parameters);
    }

    public static Expression Invoke<T, TResult>(this Expression<Func<T, TResult>> source, Expression arg)
        => source.Body.ReplaceParameter(source.Parameters[0], arg);
}

which uses the following little helpers for parameter replacing:

public static partial class ExpressionExtensions
{
    public static Expression ReplaceParameter(this Expression source, ParameterExpression parameter, Expression value)
        => new ParameterReplacer { Parameter = parameter, Value = value }.Visit(source);

    class ParameterReplacer : ExpressionVisitor
    {
        public ParameterExpression Parameter;
        public Expression Value;
        protected override Expression VisitParameter(ParameterExpression node)
            => node == Parameter ? Value : node;
    }
}

As confirmed in the comments, this solves the issue in question.

---

Now, unrelated, but as a bonus. Another thing which makes sense for expressions supposed to be compiled is the usage of ConstantExpressions - they are evaluated once and then used in potentially many places.

However for expression trees which are supposed to be transformed to SQL or similar, using ConstantExpressions makes each query different, thus non cacheable. For performance reasons, it is better to use expression type which is treated as variable, thus allowing the cache the transformation and parameterizing the generated SQL query, so both client and database query processors can reuse the "compiled" query/execution plan.

Doing so is quite easy. It does not require changing the type of the predicate or the way you generate. All you need is to replace the ConstantExpression with member (property/field) of a ConstantExpression. In your case it's a matter of replacing

var propertyValue = pi.GetValue(entityFilter);

with

var propertyValue = Expression.Property(Expression.Constant(entityFilter), pi);

and of course adjusting the signatures/implementation (in general try to not use specific expression types if they are not essential for the method), e.g.

FilterByPropertyAttribute class:

public abstract Expression GetExpression(ParameterExpression parameter, Expression value);

NonNullableValuePropertyFilterAttribute class:

public override Expression GetExpression(ParameterExpression parameter, Expression value)
{
    var property = this.GetPropertyExpression(parameter);
    if (value.Type != property.Type)
        value = Expression.Convert(value, property.Type);
    return this.GetExpression(property, value);
}

protected abstract Expression GetExpression(MemberExpression member, Expression value);

EqualPropertyFilterAttribute class:

protected override Expression GetExpression(MemberExpression member, Expression value)
    => Expression.Equal(member, value);

All other things, including the usage remain the same. But the result would be nicely parameterized query as if it was created at compile time.