Efcore: Ternary operation being evaluated when it should not

[fschlaef]

If you write a ternary in a Linq operation, all the operands will be evaluated by the expression visitor, even though they should not. If any operand happens to be an object property, EF will try to access it even if the ternary specifies not to. If that object is null, the query will throw an exception.

Exception message:
An exception was thrown while attempting to evaluate the LINQ query parameter expression 'value(Repro.Program+<>c__DisplayClass0_0).foo.Bar'.

Stack trace:

   at Microsoft.EntityFrameworkCore.Query.ExpressionVisitors.Internal.ParameterExtractingExpressionVisitor.Evaluate(Expression expression, String& parameterName)
   at Microsoft.EntityFrameworkCore.Query.ExpressionVisitors.Internal.ParameterExtractingExpressionVisitor.TryExtractParameter(Expression expression)
   at Microsoft.EntityFrameworkCore.Query.ExpressionVisitors.Internal.ParameterExtractingExpressionVisitor.VisitMember(MemberExpression memberExpression)
   at System.Linq.Expressions.MemberExpression.Accept(ExpressionVisitor visitor)
   at System.Linq.Expressions.ExpressionVisitor.Visit(Expression node)
   at System.Linq.Expressions.ExpressionVisitor.VisitConditional(ConditionalExpression node)
   at System.Linq.Expressions.ConditionalExpression.Accept(ExpressionVisitor visitor)
   at System.Linq.Expressions.ExpressionVisitor.Visit(Expression node)
   at System.Linq.Expressions.ExpressionVisitor.Visit(ReadOnlyCollection`1 nodes)
   at Microsoft.EntityFrameworkCore.Query.ExpressionVisitors.Internal.ParameterExtractingExpressionVisitor.VisitNew(NewExpression node)
   at System.Linq.Expressions.NewExpression.Accept(ExpressionVisitor visitor)
   at System.Linq.Expressions.ExpressionVisitor.Visit(Expression node)
   at System.Linq.Expressions.ExpressionVisitor.VisitLambda[T](Expression`1 node)
   at Microsoft.EntityFrameworkCore.Query.ExpressionVisitors.Internal.ParameterExtractingExpressionVisitor.VisitLambda[T](Expression`1 node)
   at System.Linq.Expressions.Expression`1.Accept(ExpressionVisitor visitor)
   at System.Linq.Expressions.ExpressionVisitor.Visit(Expression node)
   at System.Linq.Expressions.ExpressionVisitor.VisitUnary(UnaryExpression node)
   at Microsoft.EntityFrameworkCore.Query.ExpressionVisitors.Internal.ParameterExtractingExpressionVisitor.VisitUnary(UnaryExpression unaryExpression)
   at System.Linq.Expressions.UnaryExpression.Accept(ExpressionVisitor visitor)
   at System.Linq.Expressions.ExpressionVisitor.Visit(Expression node)
   at System.Dynamic.Utils.ExpressionVisitorUtils.VisitArguments(ExpressionVisitor visitor, IArgumentProvider nodes)
   at System.Linq.Expressions.ExpressionVisitor.VisitMethodCall(MethodCallExpression node)
   at Microsoft.EntityFrameworkCore.Query.ExpressionVisitors.Internal.ParameterExtractingExpressionVisitor.VisitMethodCall(MethodCallExpression methodCallExpression)
   at System.Linq.Expressions.MethodCallExpression.Accept(ExpressionVisitor visitor)
   at System.Linq.Expressions.ExpressionVisitor.Visit(Expression node)
   at System.Dynamic.Utils.ExpressionVisitorUtils.VisitArguments(ExpressionVisitor visitor, IArgumentProvider nodes)
   at System.Linq.Expressions.ExpressionVisitor.VisitMethodCall(MethodCallExpression node)
   at Microsoft.EntityFrameworkCore.Query.ExpressionVisitors.Internal.ParameterExtractingExpressionVisitor.VisitMethodCall(MethodCallExpression methodCallExpression)
   at System.Linq.Expressions.MethodCallExpression.Accept(ExpressionVisitor visitor)
   at System.Linq.Expressions.ExpressionVisitor.Visit(Expression node)
   at Microsoft.EntityFrameworkCore.Query.ExpressionVisitors.Internal.ParameterExtractingExpressionVisitor.ExtractParameters(Expression expression)
   at Microsoft.EntityFrameworkCore.Query.Internal.QueryModelGenerator.ExtractParameters(IDiagnosticsLogger`1 logger, Expression query, IParameterValues parameterValues, Boolean parameterize, Boolean generateContextAccessors)
   at Microsoft.EntityFrameworkCore.Query.Internal.QueryCompiler.Execute[TResult](Expression query)
   at Microsoft.EntityFrameworkCore.Query.Internal.EntityQueryProvider.Execute[TResult](Expression expression)
   at System.Linq.Queryable.FirstOrDefault[TSource](IQueryable`1 source, Expression`1 predicate)
   at _12522Repro.Program.Main(String[] args) in C:\Users\fschlaef\Documents\Visual Studio 2017\Projects\Repro\Repro\Program.cs:line 33

Steps to reproduce

Quick example : someBoolean ? foo.Bar : 1

If someBoolean is false and foo is null, the query will crash.

Full repro :

```c#
using Microsoft.EntityFrameworkCore;
using Microsoft.Extensions.Logging;
using System;
using System.Diagnostics;
using System.Linq;

namespace _12521Repro
{
class Foo
{
public int Bar { get; set; }
}

class Program
{
    static void Main(string[] args)
    {
        using (var context = new MyContext())
        {
            context.Database.EnsureDeleted();
            context.Database.EnsureCreated();
            context.Customer.Add(new Customer());
            context.SaveChanges();
        }

        try
        {
            using (var context = new MyContext())
            {
                Foo foo = null;
                var hasData = !(foo is null);
                int control = (hasData ? foo.Bar : 1); // Equals 1, our ternary is correct

                var query = context.Customer
                    .Select(c => new
                    {
                        c.Id,
                        Data1 = hasData ? foo.Bar : 1, // Will crash
                        Data2 = foo != null ? foo.Bar : 1, // Will also crash
                        Data3 = !hasData ? 1 : foo.Bar // Order doesn't matter, crashes
                    })
                    .FirstOrDefault(c => c.Id == 1);
            }
        }
        catch(Exception e)
        {
            Debug.WriteLine(e.Message);
        }
    }
}

public class MyContext : DbContext
{
    public virtual DbSet<Customer> Customer { get; set; }

    protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
    {
        string connectionString = "Server=.;Database=Repro12521;Trusted_Connection=True;MultipleActiveResultSets=true";

        optionsBuilder.UseSqlServer(connectionString)
            .EnableSensitiveDataLogging()
            .UseLoggerFactory(new LoggerFactory().AddDebug(LogLevel.Debug));
    }
}


public partial class Customer
{
    public int Id { get; set; }
}

}

### Workaround

The easiest workaround I could find was to reinstanciate the object manually before the query :

```csharp
if(!hasData)
{
    foo = new Foo();
}

This works but this is a very simple case, the solution might not be applicable for more complex cases.

Further technical details

EF Core version: 2.1.1
Database Provider: Microsoft.EntityFrameworkCore.SqlServer
Operating system: Windows 7 x64
IDE: Visual Studio 2017 15.7.4

[ajcvickers]

@smitpatel to explain.

[smitpatel]

This is happening because we are not evaluating ternary operator as single piece and rather try to evaluate it piecewise.
Ideally we should create parameter for expression sub tree of hasData ? foo.Bar : 1 but we go into components and create parameter for foo.Bar only which causes above expression.

Simple fix for this involves adding overriding method for VisitConditional in ParameterExctractingExpressionVisitor which basically calls TryExtractParameter whenever the conditionalExpression is evaluatable.

Marking this for up-for-grabs.

[Matthiee]

@smitpatel Can I take this one?

[smitpatel]

@Matthiee - Sure.

[smitpatel]

@Matthiee - Thanks for contribution.

[fschlaef]

@smitpatel any chance to see this in 2.2.0 or still punted for 3.0.0 ?

[ajcvickers]

@fschlaef Since we have no more previews of 2.2 we're being somewhat conservitive in taking changes, so by default the answer is no. But we'll take another look and re-assess the risk.

@smitpatel Any chance of regression here if we take this for 2.2?

[smitpatel]

I created PR to port the fix to 2.2

[fschlaef]

@smitpatel @ajcvickers bad news, got this tested today on 2.2.0, turns out that this fix work ... except for DateTime

An exception was thrown while attempting to evaluate the LINQ query parameter expression 'value(_12521Repro.Program+<>c__DisplayClass0_0).foo.DateTime'.

{System.NullReferenceException: Object reference not set to an instance of an object.    
   at lambda_method(Closure )   
   at Microsoft.EntityFrameworkCore.Query.ExpressionVisitors.Internal.ParameterExtractingExpressionVisitor.Evaluate(Expression expression, String& parameterName)
   at Microsoft.EntityFrameworkCore.Query.ExpressionVisitors.Internal.ParameterExtractingExpressionVisitor.TryExtractParameter(Expression expression)
   at Microsoft.EntityFrameworkCore.Query.ExpressionVisitors.Internal.ParameterExtractingExpressionVisitor.VisitMember(MemberExpression memberExpression)
   at System.Linq.Expressions.MemberExpression.Accept(ExpressionVisitor visitor)
   at System.Linq.Expressions.ExpressionVisitor.Visit(Expression node)
   at System.Linq.Expressions.ExpressionVisitor.VisitConditional(ConditionalExpression node)
   at Microsoft.EntityFrameworkCore.Query.ExpressionVisitors.Internal.ParameterExtractingExpressionVisitor.VisitConditional(ConditionalExpression conditionalExpression)
   at System.Linq.Expressions.ConditionalExpression.Accept(ExpressionVisitor visitor)
   at System.Linq.Expressions.ExpressionVisitor.Visit(Expression node)
   at System.Linq.Expressions.ExpressionVisitor.Visit(ReadOnlyCollection`1 nodes)
   at Microsoft.EntityFrameworkCore.Query.ExpressionVisitors.Internal.ParameterExtractingExpressionVisitor.VisitNew(NewExpression node)
   at System.Linq.Expressions.NewExpression.Accept(ExpressionVisitor visitor)
   at System.Linq.Expressions.ExpressionVisitor.Visit(Expression node)
   at System.Linq.Expressions.ExpressionVisitor.VisitLambda[T](Expression`1 node)
   at Microsoft.EntityFrameworkCore.Query.ExpressionVisitors.Internal.ParameterExtractingExpressionVisitor.VisitLambda[T](Expression`1 node)
   at System.Linq.Expressions.Expression`1.Accept(ExpressionVisitor visitor)
   at System.Linq.Expressions.ExpressionVisitor.Visit(Expression node)
   at System.Linq.Expressions.ExpressionVisitor.VisitUnary(UnaryExpression node)
   at Microsoft.EntityFrameworkCore.Query.ExpressionVisitors.Internal.ParameterExtractingExpressionVisitor.VisitUnary(UnaryExpression unaryExpression)
   at System.Linq.Expressions.UnaryExpression.Accept(ExpressionVisitor visitor)
   at System.Linq.Expressions.ExpressionVisitor.Visit(Expression node)
   at System.Dynamic.Utils.ExpressionVisitorUtils.VisitArguments(ExpressionVisitor visitor, IArgumentProvider nodes)
   at System.Linq.Expressions.ExpressionVisitor.VisitMethodCall(MethodCallExpression node)
   at Microsoft.EntityFrameworkCore.Query.ExpressionVisitors.Internal.ParameterExtractingExpressionVisitor.VisitMethodCall(MethodCallExpression methodCallExpression)
   at System.Linq.Expressions.MethodCallExpression.Accept(ExpressionVisitor visitor)
   at System.Linq.Expressions.ExpressionVisitor.Visit(Expression node)
   at System.Dynamic.Utils.ExpressionVisitorUtils.VisitArguments(ExpressionVisitor visitor, IArgumentProvider nodes)
   at System.Linq.Expressions.ExpressionVisitor.VisitMethodCall(MethodCallExpression node)
   at Microsoft.EntityFrameworkCore.Query.ExpressionVisitors.Internal.ParameterExtractingExpressionVisitor.VisitMethodCall(MethodCallExpression methodCallExpression)
   at System.Linq.Expressions.MethodCallExpression.Accept(ExpressionVisitor visitor)
   at System.Linq.Expressions.ExpressionVisitor.Visit(Expression node)
   at Microsoft.EntityFrameworkCore.Query.ExpressionVisitors.Internal.ParameterExtractingExpressionVisitor.ExtractParameters(Expression expression)
   at Microsoft.EntityFrameworkCore.Query.Internal.QueryModelGenerator.ExtractParameters(IDiagnosticsLogger`1 logger, Expression query, IParameterValues parameterValues, Boolean parameterize, Boolean generateContextAccessors)
   at Microsoft.EntityFrameworkCore.Query.Internal.QueryCompiler.Execute[TResult](Expression query)
   at Microsoft.EntityFrameworkCore.Query.Internal.EntityQueryProvider.Execute[TResult](Expression expression)
   at System.Linq.Queryable.FirstOrDefault[TSource](IQueryable`1 source)
   at _12521Repro.Program.Main(String[] args) in C:\Users\fschlaef\Documents\Visual Studio 2017\Projects\12521Repro\12521Repro\Program.cs:line 42

Updated repro

EDIT : snipped, updated repro below

[smitpatel]

@fschlaef - Since DateTime.Now is forced to server eval, the whole conditional expression is not evaluatable (as oppose to being evaluatable in all other cases). To allow short-circuiting in such scenario, we need to work with the value of test to chop off the tree but this should only happen with conditional itself is not evaluatable since it causes multiple query cache entries. Tracking issue #13859

[fschlaef]

@smitpatel there is a simple workaround for this case (something along the lines of DateTime now = DateTime.UtcNow; then using now in the Linq expression), however I found other cases when this workaround is impossible.

If Customer has a DateTime property and you try to write your ternary like this :

foo != null ? foo.DateTime : customer.Date

It still throws.

Updated repro :

using Microsoft.EntityFrameworkCore;
using Microsoft.Extensions.Logging;
using System;
using System.Linq;

namespace _12521Repro
{
    class Foo
    {
        public DateTime DateTime { get; set; }
    }

    class Program
    {
        static void Main(string[] args)
        {
            using (var context = new MyContext())
            {
                context.Database.EnsureDeleted();
                context.Database.EnsureCreated();
                context.Customer.Add(new Customer());
                context.SaveChanges();

                Foo foo = null;
                DateTime now = DateTime.UtcNow;

                // All of those are OK
                context.Customer.Select(c => new { Data = foo != null ? foo.DateTime : new DateTime(2018, 02, 02) }).FirstOrDefault();
                context.Customer.Select(c => new { Data = foo != null ? c.Date : DateTime.UtcNow }).FirstOrDefault();
                context.Customer.Select(c => new { Data = foo != null ? DateTime.UtcNow : c.Date }).FirstOrDefault();
                context.Customer.Select(c => new { Data = foo != null ? foo.DateTime : now }).FirstOrDefault();

                // But those are KO 
                context.Customer.Select(c => new { Data = foo != null ? foo.DateTime : DateTime.UtcNow }).FirstOrDefault();
                context.Customer.Select(c => new { Data = foo != null ? foo.DateTime : c.Date }).FirstOrDefault();
            }
        }
    }

    public class MyContext : DbContext
    {
        public virtual DbSet<Customer> Customer { get; set; }

        protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
        {
            string connectionString = "Server=.;Database=Repro12521;Trusted_Connection=True;MultipleActiveResultSets=true";

            optionsBuilder.UseSqlServer(connectionString)
                .EnableSensitiveDataLogging()
                .UseLoggerFactory(new LoggerFactory().AddDebug(LogLevel.Debug));
        }
    }


    public partial class Customer
    {
        public int Id { get; set; }
        public DateTime Date { get; set; }
    }
}