dotnet / efcore

EF Core is a modern object-database mapper for .NET. It supports LINQ queries, change tracking, updates, and schema migrations.
https://docs.microsoft.com/ef/
MIT License
13.78k stars 3.19k forks source link

Projecting bool property through optional nav access with other conditions throws ArgumentException #9275

Closed peterwurzinger closed 6 years ago

peterwurzinger commented 7 years ago

Hi!

We encountered an error in EF Core 2.0 Preview 1 and EF Core 2.0 Preview 2. Since I couldn't find an appropriate short title I'll just describe the problem - feel free to edit the title. The following model was used to reproduce the error:

public class ChildEntity
{
    [Key]
    public Guid Id { get; set; }

    [ForeignKey(nameof(ParentEntity))]
    public Guid? ParentEntityId { get; set; }
    public ParentEntity ParentEntity { get; set; }
}

public class ParentEntity
{
    [Key]
    public Guid Id { get; set; }

    public bool MyValue { get; set; }
}

DbContext was implemented straightforward with

public DbSet<ParentEntity> Entities { get; set; }
public DbSet<ChildEntity> ChildEntities { get; set; }

and

 optionsBuilder.UseSqlServer(@"<imagine a valid connection string here>");

Reproduced with an instance of LocalDB (Version 13.0.2151.0), untested with a "real" SQL-Server instance - shouldn't make a difference, imho.

Wanting to project the Property ParentEntity.MyValue to a DTO/ViewModel or even an anonymous type leads to a System.ArgumentException: 'Argument must be boolean':

//DOESN'T WORK
ctx.ChildEntities.Select(child => new 
                {
                    child.Id,
                    IsFieldSet = child.ParentEntity != null && child.ParentEntity.MyValue
                }).ToList();

The funny thing is, selecting the Parent-Property without connecting it to another logical term works fine:

//WORKS
ctx.ChildEntities.Select(child => new 
                {
                    child.Id,
                    IsFieldSet = child.ParentEntity.MyValue
                }).ToList();

It seems, that the logical operator between the individual expressions causes the problem, since connecting any (senseless) non-constant Expression leads to this ArgumentException:

//DOESN'T WORK
ctx.ChildEntities.Select(child => new 
                {
                    child.Id,
                    IsFieldSet = child.ParentEntity.MyValue && child.Id.ToString().StartsWith("f")
                }).ToList();

Tried to debug/fix that on my own, but that is too much expressiontree-madness for me - sorry!

Regards Peter

cc @bigbabyjesus

smitpatel commented 7 years ago

This happens in 2.0.0 rtm too.

smitpatel commented 7 years ago

Looking at query models

'from ChildEntity child in DbSet<ChildEntity>
      join ParentEntity child.ParentEntity in DbSet<ParentEntity>
      on Property([child], "ParentEntityId") equals (Nullable<Guid>)Property([child.ParentEntity], "Id") into child.ParentEntity_group
      from ParentEntity child.ParentEntity in
          (from ParentEntity child.ParentEntity_groupItem in [child.ParentEntity_group]
          select [child.ParentEntity_groupItem]).DefaultIfEmpty()
      select new <>f__AnonymousType0<Guid, bool>(
          [child].Id,
          (bool)(Nullable<bool>)Property([child], "ParentEntityId") != null && ?[child.ParentEntity]?.MyValue == True?
      )'

Here child.ParantEntity.MyValue is nullable type (allegedly) because of group join but its a value. If you use it alone then all works fine because its in projection so you need value only in SQL. But when you combine it with other condition, it coerce the other condition to nullable bool and cast to bool on whole expression. Since inner expression is logical operation it becomes search condition rather than value so we create case block for it but since this is inside of Convert, we hit type mismatch issue. We need to avoid causing conversion when inside of Convert block.

smitpatel commented 7 years ago

@maumar - On a different issue, Child.ParentEntity.MyValue shouldn't be nullable because there is null check already.

peterwurzinger commented 7 years ago

Since there is a high chance of needing to release our application with EFCore 2.0, is there a known workaround where it is not necessary to touch the queries? We found out, that an explicit comparison with true fixes the problem, but since it is an redundant expression we do not know if a compiler or refactoring tool would reduce this expression in Release builds:

//WORKS
ctx.ChildEntities.Select(child => new 
                {
                    child.Id,
                    IsFieldSet = child.ParentEntity != null && child.ParentEntity.MyValue == true
                }).ToList();

Regards Peter

smitpatel commented 7 years ago

Try this

ctx.ChildEntities.Select(child => new
    {
        child.Id,
        IsFieldSet = child.ParentEntity.MyValue
    }).ToList();

You don't need null check when accessing through navigation. EF Core takes care of while expanding navigation.

peterwurzinger commented 7 years ago

I'm well aware of the built-in null check. But as already mentioned, projecting a single bool-value to a view/anonymous object works fine (it would be really critical if not). The error only occours when connecting two or more logical terms in 1:N relations and projecting them. So IsFieldSet = child.ParentEntity.MyValue will ofc work fine while IsFieldSet = child.ParentEntity.MyValue && child.AnotherBoolValue won't.

Kirlac commented 7 years ago

Seeing the same thing on an azure hosted SQL server instance. Full framework 4.7 application w/ netstandard2.0 class libraries using EF Core 2.0.0-rtm-26452.

Seeing this same behaviour with calling .Any() eg.

// Does not work - System.ArgumentException: Argument must be boolean
//    at System.Linq.Expressions.Expression.Condition(Expression test, Expression ifTrue, Expression ifFalse)
bool exists = ctx.ChildEntities.Any(child => child.Id == childId && child.ParentEntity.MyValue);

The following examples were able to work around the issue

// Only using a single expression
bool exists = ctx.ChildEntities.Where(child => child.Id == childId).Any(child.ParentEntity.MyValue);
// Explicitly comparing against true
bool exists = ctx.ChildEntities.Any(child => child.Id == childId && child.ParentEntity.MyValue == true);
// Casting as bool appeared to work as well
bool exists = ctx.ChildEntities.Any(child => child.Id == childId && (bool)child.ParentEntity.MyValue);