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.66k stars 3.15k forks source link

ArgumentNullException with InMemory database when initializing new DTO in projection #19726

Closed BaerMitUmlaut closed 3 years ago

BaerMitUmlaut commented 4 years ago

When using a ternary if in combination with an inner .Select(...).ToList(), an ArgumentNullException is thrown. This seems to only happen with an InMemory database and did not appear with SqlServer.

This issue was originally found by testing some code using AutoMapper with nested entities, the original issue can be found here: AutoMapper/AutoMapper#3309

Possibly related to #9223.

Steps to reproduce

Within an InMemory database, execute a query that contains a ternary if which contains another expression using .Select(...).ToList(), like the following:

var alphaDtosQuery = dbContext.Alphas
    .Select(alpha => (alpha.Bravos.Count > 0) ? null : new AlphaDto
    {
        Bravos = alpha.Bravos
            .Select(bravo => new BravoDto())
            .ToList()
    });

var alphaDtos = alphaDtosQuery.ToList();
The following exception is thrown (click to open): ``` System.ArgumentNullException HResult=0x80004003 Message=Value cannot be null. (Parameter 'bindings') Source=System.Linq.Expressions StackTrace: at System.Linq.Expressions.Expression.ValidateMemberInitArgs(Type type, ReadOnlyCollection`1 bindings) at System.Linq.Expressions.Expression.MemberInit(NewExpression newExpression, IEnumerable`1 bindings) at System.Linq.Expressions.MemberInitExpression.Update(NewExpression newExpression, IEnumerable`1 bindings) at System.Linq.Expressions.ExpressionVisitor.VisitMemberInit(MemberInitExpression node) at System.Linq.Expressions.MemberInitExpression.Accept(ExpressionVisitor visitor) at System.Linq.Expressions.ExpressionVisitor.Visit(Expression node) at Microsoft.EntityFrameworkCore.InMemory.Query.Internal.InMemoryExpressionTranslatingExpressionVisitor.VisitConditional(ConditionalExpression conditionalExpression) at System.Linq.Expressions.ConditionalExpression.Accept(ExpressionVisitor visitor) at System.Linq.Expressions.ExpressionVisitor.Visit(Expression node) at Microsoft.EntityFrameworkCore.InMemory.Query.Internal.InMemoryExpressionTranslatingExpressionVisitor.Translate(Expression expression) at Microsoft.EntityFrameworkCore.InMemory.Query.Internal.InMemoryProjectionBindingExpressionVisitor.Visit(Expression expression) at Microsoft.EntityFrameworkCore.InMemory.Query.Internal.InMemoryProjectionBindingExpressionVisitor.Translate(InMemoryQueryExpression queryExpression, Expression expression) at Microsoft.EntityFrameworkCore.InMemory.Query.Internal.InMemoryQueryableMethodTranslatingExpressionVisitor.TranslateSelect(ShapedQueryExpression source, LambdaExpression selector) at Microsoft.EntityFrameworkCore.Query.QueryableMethodTranslatingExpressionVisitor.VisitMethodCall(MethodCallExpression methodCallExpression) at System.Linq.Expressions.MethodCallExpression.Accept(ExpressionVisitor visitor) at System.Linq.Expressions.ExpressionVisitor.Visit(Expression node) at Microsoft.EntityFrameworkCore.Query.QueryCompilationContext.CreateQueryExecutor[TResult](Expression query) at Microsoft.EntityFrameworkCore.Storage.Database.CompileQuery[TResult](Expression query, Boolean async) at Microsoft.EntityFrameworkCore.Query.Internal.QueryCompiler.CompileQueryCore[TResult](IDatabase database, Expression query, IModel model, Boolean async) at Microsoft.EntityFrameworkCore.Query.Internal.QueryCompiler.<>c__DisplayClass9_0`1.b__0() at Microsoft.EntityFrameworkCore.Query.Internal.CompiledQueryCache.GetOrAddQueryCore[TFunc](Object cacheKey, Func`1 compiler) at Microsoft.EntityFrameworkCore.Query.Internal.CompiledQueryCache.GetOrAddQuery[TResult](Object cacheKey, Func`1 compiler) at Microsoft.EntityFrameworkCore.Query.Internal.QueryCompiler.Execute[TResult](Expression query) at Microsoft.EntityFrameworkCore.Query.Internal.EntityQueryProvider.Execute[TResult](Expression expression) at Microsoft.EntityFrameworkCore.Query.Internal.EntityQueryable`1.GetEnumerator() at System.Collections.Generic.List`1..ctor(IEnumerable`1 collection) at System.Linq.Enumerable.ToList[TSource](IEnumerable`1 source) at MinimalTest.MinimalTest.TestWithTernary() in D:\Some\File\Path\MinimalTest.cs:line 107 ```

Remove the .Select(...) or the ternary if, and the query will be executed just fine.

A sub 100loc repro can be found here: https://gist.github.com/BaerMitUmlaut/a5feed7af2132b5e5f7d31cd4e7cc08b

Further technical details

EF Core version: 3.0.0 and 3.1.1 Database provider: Microsoft.EntityFrameworkCore.InMemory Target framework: .NET Core 3.0 Operating system: Windows 10 IDE: Visual Studio 2019 Professional 16.4.3

ajcvickers commented 4 years ago

@maumar to take a look.

maumar commented 4 years ago

problem is that InMemoryExpressionTranslatingExpressionVisitor is not correctly handling MemberInit expression node. Subquery:

alpha.Bravos
            .Select(bravo => new BravoDto())
            .ToList()

can't be translated, so the binding expression returns null. However default implementation of VisitMemberInit doesn't account for null binding, so the exception is thrown. Instead, we should handle it, similar to what InMemoryProjectionBindingExpressionVisitor is doing.

smitpatel commented 4 years ago

Duplicate of https://github.com/dotnet/efcore/issues/19316

joakimriedel commented 3 years ago

Still in 5.0 rc1.

I get hit by this in unit tests every time I forget to add DoNotAllowNull() in the AutoMapper configuration option for a one-to-one relationship marked with [Required]. It works in SQL Server provider, but not in InMemory.

Any progress on this?

ajcvickers commented 3 years ago

@smitpatel Is this really a duplicate of #19316? If so, can we close this one?

@joakimriedel This issue is in the Backlog milestone. This means that it is not planned for the next release (EF Core 5.0). We will re-assess the backlog following the this release and consider this item at that time. However, keep in mind that there are many other high priority features with which it will be competing for resources.

joakimriedel commented 3 years ago

@ajcvickers no stress, the workaround helps and it's only affecting unit tests for us.

But it would be nice to have it fixed, since I have a bad memory and always forget to add the workaround in new queries - and then spend 30 minutes to find this very same issue to remind me how to fix it. 😉

edit: that said - I just looked at #19316 and I did not understand how this could be a duplicate of that - the exception and stacktrace is totally different. I get the "Value cannot be null. (Parameter 'bindings')" like @BaerMitUmlaut posted in this issue.

BaerMitUmlaut commented 3 years ago

If what @maumar wrote is the cause (and I agree with @joakimriedel, the exception would match better than the one from #19316), I could take a shot at this. Would a PR help with getting this into 5.1?

ajcvickers commented 3 years ago

@BaerMitUmlaut There is no 5.1 planned. The next release after EF Core 5.0 is planned to be EF Core 6.0 in November 2021.

yousiftouma commented 3 years ago

What's a workaround when not using AutoMapper? Loading the result to memory first using .ToList() and then selecting on the enumerable instead of the queryable?

ajcvickers commented 3 years ago

@yousiftouma From the original post, where AutoMapper has aleardy been removed:

Remove the .Select(...) or the ternary if, and the query will be executed just fine.

fcicc commented 3 years ago

@yousiftouma From the original post, where AutoMapper has aleardy been removed:

Remove the .Select(...) or the ternary if, and the query will be executed just fine.

What if I cannot remove the ternary (i.e., I have a column in my DB that can be null), but I still have to use Select because I need to map my entities to DTOs?

fcicc commented 3 years ago

@yousiftouma From the original post, where AutoMapper has aleardy been removed:

Remove the .Select(...) or the ternary if, and the query will be executed just fine.

What if I cannot remove the ternary (i.e., I have a column in my DB that can be null), but I still have to use Select because I need to map my entities to DTOs?

For now, I'm querying as entity then mapping the entity to DTO (see example below).

Looking forward for a definitive solution for this issue :)

var entity = await context.Entities
    .Where(...) // some condition
    .Include(...) // necessary nested entities
    .FirstOrDefaultAsync();

if (entity != null)
{
    return new MyDTO
    {
        // filling necessary DTO fields, including nested DTOs
    };
}
smitpatel commented 3 years ago

This is likely fixed in main

worthy7 commented 3 years ago

@smitpatel I don't think so, I just had this problem a few days ago after adding a many-many relationship on he latest packages.

smitpatel commented 3 years ago

@worthy7 - Without looking at repro code which may or may not be duplicate of this and knowing which exact package you tried, it would hard to make a claim that this is not fixed. Query can throw same exception for very different kind of bugs too.

worthy7 commented 3 years ago

Using original post code from here: https://gist.github.com/BaerMitUmlaut/a5feed7af2132b5e5f7d31cd4e7cc08b

image

If it's a duplicate fair enough to close.

smitpatel commented 3 years ago

@worthy7 main branch has 6.0 alpha packages. I am not referring to 5.0 release.

worthy7 commented 3 years ago

Ah, ok thanks.

On Fri, 4 Dec 2020, 15:40 Smit Patel, notifications@github.com wrote:

@worthy7 https://github.com/worthy7 main branch has 6.0 alpha packages. I am not referring to 5.0 release.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dotnet/efcore/issues/19726#issuecomment-738598622, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABEKWH2WYMWMKYOUV37KDDTSTB74HANCNFSM4KMRC22Q .

Akinzekeel commented 3 years ago

I've had this issue with AutoMapper and Mapster in EF Core 3 & 5 - mainly in the unit tests that use InMemory.

With the latest daily build of EF Core 6 the tests are finally passing though, so I am fairly certain that this has been fixed.

JMadle commented 3 years ago

Please can the fix applied in 6.0 be applied to Core 5.0 urgently; this issue is preventing us from upgrading our major product to .NET5/EFcore5 (hundreds of Integration Tests that were successful in EFcore3.1 now fail and we can find no workaround for this issue).

This is a regression from the previous version of EF Core and therefore either a bug or a breaking change (without workaround).

BaerMitUmlaut commented 3 years ago

@JMadle as you can see from the OP, this bug happens in 3.1, too. You might be experiencing a different bug?

JMadle commented 3 years ago

Error is same "Value cannot be null. (Parameter 'bindings')" and repro is as per top of page.

It worked for us previously in 3.1 as we did not need the ternary if, but in 5.0 we get “Nullable object must have a value” exception without adding the ternary, for example:

In 3.1 we had:

var parent = await dbContext.Parents.Select(p => new Parent
{
    Id = p.Id,
    Child = new Child
    {
        Id = p.Child.Id,
        GrandChildren = p.Child.GrandChildren.Select(gc => new GrandChild())
    },
})
.ToListAsync();

In 5.0 we need:

var parent = await dbContext.Parents.Select(p => new Parent
{
    Id = p.Id,
    Child = p.Child == null ? null : new Child
    {
        Id = p.Child.Id,
        GrandChildren = p.Child.GrandChildren.Select(gc => new GrandChild())
    },
})
.ToListAsync();

Child and GrandChildren are both permitted to be null in our model.

So it seems to me that the nullable handling has changed in 5.0 such that we have to add the ternary, perhaps I should raise the requirement to null check as a different issue rather than asking for the ternary fix to be applied to 5.0?

smitpatel commented 3 years ago

@JMadle - It is not a bug, it is incorrect LINQ query causing it. If Child is null for the parent then p.Child will be null and Id on it will be undefined (or null in database). You want us to assign that to Id property in newly created Child which takes only int values. That value is not defined hence the error. You would either need ternary check before creating new child or define a default value when p.Child.Id is null to assign to Id property.

JMadle commented 3 years ago

Thanks @smitpatel but that code was successfully parsed in EF2.1, 3.0 and 3.1 without the ternary in our code for the last 2 years. Adding the ternary to check for the null (as seems to be required in EF5 and shown in my example above) results in the bug which this whole page relates to. To me this is a breaking change without a workaround, it is blocking us from upgrading to .NET5/EF5

smitpatel commented 3 years ago

@JMadle - This issue tracks throwing better exception than ArgumentNullException. The query is not translatable without client eval of projection. Your query is failing not because of Select part, even if you remove it (which is covered in this issue), it will still throw error because p.Child.Id is null in database and you are asking us to assign it to int property, which is not possible. It worked in 3.x because it was a bug, now it is fixed in 5.0. You need to carefully think what do you want to assign as a non-nullable int property if null value was encountered. Since your issue is not relevant to this issue, please file a new issue if you want to continue further discussion. If you think this is bug in EF Core (apart from the part it was working before), then please post a runnable repro code with sample data and explain what is the expected results of the query.

hisuwh commented 3 years ago

A workaround is alluded to in this thread but not explicitly stated. I'm using automapper projectto and this was working in 2.1 - upon upgrading to 3.1 (yes I know I'm late to the party here) this is no longer working.

achobanov commented 3 years ago

Is this supposed to be fixed in v5? I'm running a WPF app on .Net 5, and cannot currently migrate to .Net 6. I think I've hit the same issue on Microsoft.EntityFrameworkCore.InMemory v5.0.10.

ajcvickers commented 3 years ago

@achobanov The milestone indicates where this is or is planned to be fixed. In the this case, we know it is fixed in 6.0.0-rc2, although it may also be fixed in 6.0.0-rc1, but this has not been tested. It is not fixed in any 5.0 release. Note also that this issue only changes the type of exception thrown. Read the full thread for full details.

achobanov commented 2 years ago

@ajcvickers I'm aware. By now (a few hours after posting here) I've pinned down the cause of the issue by excluding properties in the projection one by one, but I still don't understand the cause of it. I've double and triple checked and even found a workaround that doesn't make any sense to me. I thought better exception could help me figure it out. Thanks for your reply though. How would a milestone look if the fix were to be applied to 5 as well?

ajcvickers commented 2 years ago

@achobanov The issue would need meet the bar for patching, at which point we would either move this to the 5.0.x milestone, or possibly create a new issue for the patch and link it from here, depending on circumstances. Note that this issue doesn't meet the bar for patching.