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

Query: NRE when trying to project count of navigation typed as ICollection<T> #22701

Closed
maumar opened this issue Sep 24, 2020 · 5 comments · Fixed by #25736
Closed

Query: NRE when trying to project count of navigation typed as ICollection<T> #22701

maumar opened this issue Sep 24, 2020 · 5 comments · Fixed by #25736
Assignees
Labels
area-query closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-bug
Milestone

Comments

@maumar
Copy link
Contributor

maumar commented Sep 24, 2020

query:

ss.Set<Customer>()
    .OrderBy(c => c.CustomerID)
    .Select(c => ((ICollection<Order>)c.Orders).Count)

exception:

 Object reference not set to an instance of an object.
  Stack Trace: 
    SelectExpression.ApplyCollectionJoin(Int32 collectionIndex, Int32 collectionId, Expression innerShaper, INavigationBase navigation, Type elementType, Boolean splitQuery) line 1373
    CollectionJoinApplyingExpressionVisitor.VisitExtension(Expression extensionExpression) line 74
    CollectionJoinApplyingExpressionVisitor.VisitExtension(Expression extensionExpression) line 95
    RelationalQueryTranslationPostprocessor.Process(Expression query) line 43
    QueryCompilationContext.CreateQueryExecutor[TResult](Expression query) line 175
    Database.CompileQuery[TResult](Expression query, Boolean async) line 72
    QueryCompiler.CompileQueryCore[TResult](IDatabase database, Expression query, IModel model, Boolean async) line 112
    <>c__DisplayClass12_0`1.<ExecuteAsync>b__0() line 149
    CompiledQueryCache.GetOrAddQuery[TResult](Object cacheKey, Func`1 compiler) line 76
    QueryCompiler.ExecuteAsync[TResult](Expression query, CancellationToken cancellationToken) line 145
    EntityQueryProvider.ExecuteAsync[TResult](Expression expression, CancellationToken cancellationToken) line 98
    EntityQueryable`1.GetAsyncEnumerator(CancellationToken cancellationToken) line 109
    ConfiguredCancelableAsyncEnumerable`1.GetAsyncEnumerator()
@smitpatel
Copy link
Contributor

Note: This should be handled in preprocessing visitor (probably in nav expansion) to remove redundant cast and convert Count to Queryable.Count().

@ajcvickers ajcvickers added this to the Backlog milestone Sep 25, 2020
@ajcvickers ajcvickers modified the milestones: Backlog, 6.0.0 Nov 5, 2020
@JonPSmith
Copy link

One of my previous clients has just updated to EF Core 5 and this bug has hit them. They can get around it by using Count() rather than Count, but it is a (small) breaking change. I have confirmed this on the code I am using for my book.

The client's code uses DDD-styled entity classes, with collections held in a backing fields and a IReadOnlyCollection<TEntity> property to read.

I haven't built a complete example as you already have some simple code that fails, but here is a bare-bones example

Entity class (bare-bones)

This is my code. The client uses List<T> for backing field and IReadOnlyCollection<T> for the read-only property.

public class Book : EventsAndCreatedUpdated, ISoftDelete
{
    //-----------------------------------------------
    //relationships backing fields

    private HashSet<Review> _reviews;
    private HashSet<BookAuthor> _authorsLink;
    private HashSet<Tag> _tags;

    //scalar properties

    public int BookId { get; private set; }
   //... rest of code left out

    //---------------------------------------
    //relationships

    public IReadOnlyCollection<Review> Reviews => _reviews?.ToList();
    public IReadOnlyCollection<BookAuthor> AuthorsLink => _authorsLink?.ToList();
    public IReadOnlyCollection<Tag> Tags => _tags?.ToList();

   //... rest of code left out
}

My test

I wrote this to check that Count() fixed the problem in my setup (I don't currently have access the client's code)

[Theory]
[InlineData(false)]
[InlineData(true)]
public void TestCountOfCollectionWithTypeChanged(bool countNoBrackets)
{
    //SETUP
    var options = SqliteInMemory.CreateOptions<BookDbContext>();
    using (var context = new BookDbContext(options))
    {
        context.Database.EnsureCreated();
        var bookId = context.SeedDatabaseFourBooks().Last().BookId;

        context.ChangeTracker.Clear();

        var query1 = context.Books
            .Where(b => b.BookId == bookId);
        var query2 = countNoBrackets
            ? query1.Select(b => b.Reviews.Count)
            : query1.Select(b => b.Reviews.Count());

        //ATTEMPT
        int reviewCount;
        try
        {
            reviewCount = query2.Single();
        }
        catch
        {
            countNoBrackets.ShouldBeTrue();
            return;
        }

        //VERIFY
        countNoBrackets.ShouldBeFalse();
        _output.WriteLine(query2.ToQueryString());
        reviewCount.ShouldEqual(2);
    }
}

The stacktrace is similar to the one shown (that's how my client found this issue)

System.NullReferenceException
Object reference not set to an instance of an object.
   at Microsoft.EntityFrameworkCore.Query.SqlExpressions.SelectExpression.ApplyCollectionJoin(Int32 collectionIndex, Int32 collectionId, Expression innerShaper, INavigationBase navigation, Type elementType, Boolean splitQuery)
   at Microsoft.EntityFrameworkCore.Query.Internal.CollectionJoinApplyingExpressionVisitor.VisitExtension(Expression extensionExpression)
   at System.Linq.Expressions.Expression.Accept(ExpressionVisitor visitor)
   at System.Linq.Expressions.ExpressionVisitor.Visit(Expression node)
   at System.Linq.Expressions.ExpressionVisitor.VisitMember(MemberExpression node)
   at System.Linq.Expressions.MemberExpression.Accept(ExpressionVisitor visitor)
   at System.Linq.Expressions.ExpressionVisitor.Visit(Expression node)
   at System.Linq.Expressions.ExpressionVisitor.VisitUnary(UnaryExpression node)
   at System.Linq.Expressions.UnaryExpression.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.VisitUnary(UnaryExpression node)
   at System.Linq.Expressions.UnaryExpression.Accept(ExpressionVisitor visitor)
   at System.Linq.Expressions.ExpressionVisitor.Visit(Expression node)
   at Microsoft.EntityFrameworkCore.Query.Internal.CollectionJoinApplyingExpressionVisitor.VisitExtension(Expression extensionExpression)
   at System.Linq.Expressions.Expression.Accept(ExpressionVisitor visitor)
   at System.Linq.Expressions.ExpressionVisitor.Visit(Expression node)
   at Microsoft.EntityFrameworkCore.Query.RelationalQueryTranslationPostprocessor.Process(Expression query)
   at Microsoft.EntityFrameworkCore.Sqlite.Query.Internal.SqliteQueryTranslationPostprocessor.Process(Expression query)
   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.<Execute>b__0()
   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 System.Linq.Queryable.Single[TSource](IQueryable`1 source)
   at Test.UnitTests.TestPersistenceSqlBooks.TestBookDbContext.TestCountOfCollectionWithTypeChanged(Boolean countNoBrackets) in C:\Users\JonPSmith\source\repos\EfCoreinAction-SecondEdition\Test\UnitTests\TestPersistenceSqlBooks\TestBookDbContext.cs:line 111

My test, and my client tests, are using EF Core 5.0.1

@V0ldek
Copy link

V0ldek commented Mar 28, 2021

Just hit the exact same issue.

The client's code uses DDD-styled entity classes, with collections held in a backing fields and a IReadOnlyCollection property to read.

My codebase is the same.

Can confirm that the Count() workaround works, however the number of analyser warnings I had to suppress to write that made me sick. This is also a very hard bug to locate since you don't exactly expect a framework to throw an NRE.

@JonPSmith
Copy link

Could this be fixed in EF Core 6? This is a regression from EF Core 3.1 and keeps causing problems for people updating from the last LTS version.

@smitpatel
Copy link
Contributor

Could be fixed in #25577

@smitpatel smitpatel added the verify-fixed This issue is likely fixed in new query pipeline. label Aug 19, 2021
maumar added a commit that referenced this issue Aug 26, 2021
@maumar maumar added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Aug 26, 2021
maumar added a commit that referenced this issue Aug 26, 2021
@maumar maumar removed the verify-fixed This issue is likely fixed in new query pipeline. label Aug 26, 2021
@maumar maumar closed this as completed in a2c32f1 Aug 27, 2021
@ajcvickers ajcvickers modified the milestones: 6.0.0, 6.0.0-rc2 Aug 27, 2021
@ajcvickers ajcvickers modified the milestones: 6.0.0-rc2, 6.0.0 Nov 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-query 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

Successfully merging a pull request may close this issue.

5 participants