Expression.Constant reference to DbSet causes translation failure

[plpolak]

The following query works perfectly fine on EFCore 2.2, but fails on EFCore 3.0: (running against MSSQL 2017, or Azure SQL)

            var query = from p1 in context.MyEntities
                        where (p1.MasterCIRId == userContext.CIRId)
                           && (p1.IsActive == true)
                           && !context.MyEntities.Any(p2 => p2.EntityKey == p1.EntityKey)
                        select p1;

Running the query on 2.2 translates nicely into a 'where exists ..' clause in the SQL, whilst 3.0 creates a where clause, passing in a list of DbMyEntity classes which logically cannot be translated into valid SQL.

The entity class is pretty straightforward:

public class DbMyEntity()
{
    public virtual ICollection<DbConditionEntityList> ConditionEntityLists { get; set; }

    [ForeignKey("MasterCIRId")]
    public virtual DbCIR MasterCIR { get; set; }

    [Column("MasterCIRId", Order = 1, TypeName = "int")]
    [Required]
    public int MasterCIRId { get; set; }

    [Column("IsActive", Order = 2, TypeName = "bit")]
    [Required]
    public bool IsActive { get; set; }

    [Column("EntityKey", Order = 3, TypeName = "int")]
    [Required]
    public int EntityKey { get; set; }
}

[smitpatel]

@roji to de-dupe.

[roji]

The issue did not repro for me. However, your model above is incomplete (no key, no definitions DbCIR, DbConditionEntityList...) so there could be a discrepancy that explains this. Please submit a full runnable code sample similar to the below which demonstrates the issue.

My attempted repro:

class Program
{
    static void Main()
    {
        using var ctx = new BlogContext();
        ctx.Database.EnsureDeleted();
        ctx.Database.EnsureCreated();

        var query = from p1 in ctx.MyEntities
                    where (p1.MasterCIRId == 8)
                          && (p1.IsActive == true)
                          && !ctx.MyEntities.Any(p2 => p2.EntityKey == p1.EntityKey)
                    select p1;

        query.ToList();
    }
}

public class BlogContext : DbContext
{
    public DbSet<DbMyEntity> MyEntities { get; set; }

    protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
        => optionsBuilder.UseSqlServer(...);
}

public class DbMyEntity
{
    //public virtual ICollection<DbConditionEntityList> ConditionEntityLists { get; set; }

    [ForeignKey("MasterCIRId")]
    public virtual int MasterCIR { get; set; }
    // public virtual DbCIR MasterCIR { get; set; }

    [Column("MasterCIRId", Order = 1, TypeName = "int")]
    [Required]
    public int MasterCIRId { get; set; }

    [Column("IsActive", Order = 2, TypeName = "bit")]
    [Required]
    public bool IsActive { get; set; }

    [Column("EntityKey", Order = 3, TypeName = "int")]
    [Required, Key]
    public int EntityKey { get; set; }
}

The following SQL is produced:

SELECT [m].[EntityKey], [m].[IsActive], [m].[MasterCIR], [m].[MasterCIRId]
FROM [MyEntities] AS [m]
WHERE (([m].[MasterCIRId] = 8) AND ([m].[IsActive] = CAST(1 AS bit))) AND NOT (EXISTS (
    SELECT 1
    FROM [MyEntities] AS [m0]
    WHERE [m0].[EntityKey] = [m].[EntityKey]))

[plpolak]

The query is part of an ASP.NET core application, which makes to context a bit more complex (the query is build in an expression tree). I have isolated the code in a small program here: https://github.com/plpolak/EFCore3BugAny

This works perfectly on EFCore 2.2.

[roji]

Your example application fails to create a database with the error Introducing FOREIGN KEY constraint 'FK_Entity_CIR_MasterCIRId' on table 'Entity' may cause cycles or multiple cascade paths. Specify ON DELETE NO ACTION or ON UPDATE NO ACTION, or modify other FOREIGN KEY constraints. - can you please correct it so that it runs as-is? I'm not doing this to be annoying - apart from the time it would take for me to fix your sample, we very frequently get non-running code, that when fixed no longer reproduces the issue...

[plpolak]

No worries. This issue is blocking for us to migrate to EF3, so I am glad you are willing to help me. I have updated the example code to fix your issues. I have alse added some test data after creating the database. It now runs in as is on MSSQL 2017.

[roji]

Your expression tree generation seems to have a bug. Without digging too much into it, the entire filter expression seems like it could be replaced with:

e.MasterCIRId == masterCirId ||
e.MasterCIRId == masterCirId &&
ctx.Entities.Any(o => o.EntityKey == e.EntityKey && o.MasterCIRId == masterCirId) == false)

Note the redundant double comparison of masterCirId, so this could be simplified to yield the following in your Program.cs, which works:

var list = ctx.Entities
    .Where(e => e.MasterCIRId == masterCirId || !ctx.Entities.Any(o => o.EntityKey == e.EntityKey && o.MasterCIRId == masterCirId))
    .OrderBy(e => e.Code)
    .Skip(0)
    .Take(10).ToList();

If you need this filter in a separate method for reuse, you can simply redefine

public static Expression<Func<T, bool>> GetViewModeFilterCIRMaintenance2<T>(int cirId, DbSet<T> others)
    where T : class, ICIREntity
    => e => e.MasterCIRId == cirId || !others.Any(o => o.EntityKey == e.EntityKey && o.MasterCIRId == cirId);

Bottom line, the compiler is much better at writing expression trees than us humans :) Manual expression general, as you've done, does make sense when the query structure itself varies as a result of some value, but that doesn't seem to be the case here.

One more important note: in your manual expression tree logic, masterCirId was being included as a constant. This means that for each value of that variable, a different query is internally compiled and cached, yielding in very bad perf. When masterCirId is parameterized (as it is in the examples I've given), only one query is compiled and reused, and the different values of the variable are sent as parameters (so SQL Server also reuses the same plan).

[plpolak]

Thanks for your suggestions to optimize the expressions. These are very helpful.

However the actual code is bigger then this small subset, but by rewriting the logic to build the queries (and have the compiler to build parts of the expression tree), I could work-around the issue here.

Question still rises for me: this worked fine on EF 2.2. Does it mean we hit a bug in EF 2.2?

[roji]

this worked fine on EF 2.2. Does it mean we hit a bug in EF 2.2?

Could definitely be, I'll try to take a deeper look at the expression tree you generated.

[roji]

Took another look at this, and indeed there seems to be a regression from 2.2. tl;dr, if you replace Expression.Constant(others) with Expression.Property(Expression.Constant(ctx), "Entities"), the code will start working.

@smitpatel, here's a minimal that passes on 2.2 but fails on 3.0:

[Fact]
public void Foo()
{
    using var ctx = CreateContext();

    var anyWithPredicateMethod = typeof(Queryable).GetMethods().Single(m => m.Name == "Any" && m.GetParameters().Length == 2);
    var method = anyWithPredicateMethod.MakeGenericMethod(typeof(Customer));

    var anyFilter = Expression.Lambda<Func<Customer, bool>>(
        Expression.Call(
            method,
            // The following triggers the failure:
            Expression.Constant(ctx.Customers),
            // The following works:
            // Expression.Property(
            //     Expression.Constant(ctx),
            //     nameof(ctx.Customers)),
            Expression.Lambda<Func<Customer, bool>>(
                Expression.Constant(true),
                Expression.Parameter(typeof(Customer), "p2"))),
        Expression.Parameter(typeof(Customer), "p1"));

    var results = ctx.Customers.Where(anyFilter).ToList();
}

Basically ctx.Customers remains a InternalDbSet constant in the tree and is never converted into an EntityQueryable, and so makes NavigationExpandingEV fail. If, instead of taking the DbSet as a constant, we pass access it as a property on a context constant, everything works.

[smitpatel]

@roji - Can you try running on 3.0-preview5?

[roji]

Ugh, I just cleaned my machine of all preview bits :/

Confirmed, test passes in preview5.

[ajcvickers]

Note from triage: putting this on the backlog to consider for the future--especially if we get more feedback on it. However, we prioritize expression trees that can be created by the C# compiler, and since this expression tree will not be created by the compiler it is low-priority to fix. Instead, manually created expression trees should strive to be the same as those created by the compiler such that the already large problem space is not made much bigger.

[GerardSmit]

@roji Thanks a lot. I've been struggling 4 hours with this issue.

In my model I register generic classes for my localizations. So for example, I have the class Localization<EntityA> that maps to the table EntityALocalizations and class Localization<EntityB> that maps to the table EntityBLocalizations. I made a method in EF 2.2 that I can query the localizations, which creates expressions but in EF 3.0 this stopped working.

Basically ctx.Customers remains a InternalDbSet constant in the tree and is never converted into an EntityQueryable

Because of this I found my solution. A workaround of this limitation is as follow:

var createQueryable = typeof(InternalDbSet<Localization<T>>)
    .GetProperty("EntityQueryable", BindingFlags.NonPublic | BindingFlags.Instance);

var set = context.Set<Localization<T>>();
var queryable = Expression.Constant(createQueryable.GetValue(set));

// Now you can use "queryable" in your expressions.

It gets the EntityQueryable from the DbSet and everything works as expected.

[smitpatel]

@GerardSmit - Just call context.Set<>.Expression, and use that instead of creating constant of queryable. So code you posted can be replaced with

var queryable = ((IQueryable) context.Set<Localization<T>>()).Expression;

Edit: Updated after @gerardsmit pointed out a mistake.

[GerardSmit]

@smitpatel Thanks, that's even better!

I had to make a little change. The property Expression is an direct interface-implementation, so I had to cast it to an IQueryable:

var queryable = ((IQueryable) context.Set<Localization<T>>()).Expression;

But after that, everything works as expected.