Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
peterwurzinger opened this issue Jul 27, 2017 · 7 comments
Assignees
Labels
closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-bug
Milestone

Comments

@peterwurzinger
Copy link

peterwurzinger commented Jul 27, 2017

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 smitpatel self-assigned this Jul 27, 2017
@smitpatel
Copy link
Member

This happens in 2.0.0 rtm too.

@smitpatel
Copy link
Member

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
Copy link
Member

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

@divega divega added this to the 2.1.0 milestone Jul 28, 2017
@peterwurzinger
Copy link
Author

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
Copy link
Member

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
Copy link
Author

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.

@smitpatel smitpatel removed their assignment Aug 28, 2017
@Kirlac
Copy link

Kirlac commented Sep 12, 2017

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);

@ajcvickers ajcvickers modified the milestones: 2.1.0-preview1, 2.1.0 Jan 17, 2018
@maumar maumar added poachable and removed poachable labels Mar 1, 2018
@maumar maumar assigned smitpatel and unassigned maumar Mar 1, 2018
@smitpatel smitpatel changed the title Projecting 2 or more connected logical expressions in 1:n - relationships leading to ArgumentException Projecting bool property through optional nav access with other conditions throws ArgumentException Mar 5, 2018
@smitpatel smitpatel added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Mar 5, 2018
smitpatel added a commit that referenced this issue Mar 5, 2018
…in projection

ConditionalExpression requires test to be bool type. When an optional navigation is projected out with other conditions, it coerce everything to nullable bool. When trying to convert to conditional block it fails because test becomes of type nullable bool. The fix is to apply cast before creating ConditionalExpression

Resolves #9275
@ajcvickers ajcvickers modified the milestones: 2.1.0-preview2, 2.1.0 Nov 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-bug
Projects
None yet
Development

No branches or pull requests

6 participants