Efcore: QueryFilter (cont): KeyNotFoundException: The given key '__ef_filter__p_0' was not present in the dictionary.

[MintPlayer]

Follow up of: https://github.com/aspnet/EntityFrameworkCore/issues/18158
Repository: https://github.com/MintPlayer/QueryFilterIssue (Only mind DotNetCore31)

• I updated the .NET Core SDK
• I updated the NuGet packages in my project
• I'm able to generate the database migrations
• I'm able to perform the database migrations

Now I'm getting the following exception when yielding the DbSet from the database:

KeyNotFoundException: The given key '__ef_filter__p_0' was not present in the dictionary.

The QueryFilter on my DbContext:

// The following line breaks the app
modelBuilder.Entity<Person>().HasQueryFilter(p => p.UserDelete == null);

Once again uncommenting the QueryFilter "fixes" the problem.

Steps to reproduce

git clone https://github.com/MintPlayer/QueryFilterIssue
cd QueryFilterIssue\DotNetCore31\QueryFilterIssue31.Data
dotnet ef database update

Run the project. This will try to fetch all people from the database.

Exception

KeyNotFoundException: The given key '__ef_filter__p_0' was not present in the dictionary.

Stack trace

System.Collections.Generic.Dictionary<TKey, TValue>.get_Item(TKey key)
Microsoft.EntityFrameworkCore.Query.Internal.EntityEqualityRewritingExpressionVisitor.ParameterValueExtractor<T>(QueryContext context, string baseParameterName, IProperty property)
lambda_method(Closure , QueryContext )
Microsoft.EntityFrameworkCore.Query.Internal.QueryCompiler.ExecuteAsync<TResult>(Expression query, CancellationToken cancellationToken)
Microsoft.EntityFrameworkCore.Query.Internal.EntityQueryProvider.ExecuteAsync<TResult>(Expression expression, CancellationToken cancellationToken)
Microsoft.EntityFrameworkCore.Query.Internal.EntityQueryable<TResult>.GetAsyncEnumerator(CancellationToken cancellationToken)
System.Runtime.CompilerServices.ConfiguredCancelableAsyncEnumerable<T>.GetAsyncEnumerator()
Microsoft.EntityFrameworkCore.EntityFrameworkQueryableExtensions.ToListAsync<TSource>(IQueryable<TSource> source, CancellationToken cancellationToken)
QueryFilterIssue31.Data.Repositories.PersonRepository.GetPeople() in PersonRepository.cs
QueryFilterIssue31.Web.Controllers.PersonController.Get() in PersonController.cs
...
MVC method pointers

Further technical details

| Item | Version |
| ------------- | ------------- |
| .NET Core | 3.1.100-preview2-014569 |
| EF Core version | 3.1.0-preview2.19525.5 |
| Database provider | Microsoft.EntityFrameworkCore.SqlServer |
| Target framework | .NET Core 3.1 |
| Operating system | Windows 10-1903 (build 18362.418) |
| IDE | Visual Studio 2019 16.4.0 Preview 3.0 |

Information

C:\Users\user>dotnet --info
.NET Core SDK (reflecting any global.json):
 Version:   3.1.100-preview2-014569
 Commit:    4bd5d24d87

Runtime Environment:
 OS Name:     Windows
 OS Version:  10.0.18362
 OS Platform: Windows
 RID:         win10-x64
 Base Path:   C:\Program Files\dotnet\sdk\3.1.100-preview2-014569\

Host (useful for support):
  Version: 3.1.0-preview2.19525.6
  Commit:  5672978d91

.NET Core SDKs installed:
  1.0.0-preview2-003131 [C:\Program Files\dotnet\sdk]
  2.1.202 [C:\Program Files\dotnet\sdk]
  2.1.505 [C:\Program Files\dotnet\sdk]
  2.1.508 [C:\Program Files\dotnet\sdk]
  2.1.509 [C:\Program Files\dotnet\sdk]
  2.1.701 [C:\Program Files\dotnet\sdk]
  2.1.801 [C:\Program Files\dotnet\sdk]
  2.1.802 [C:\Program Files\dotnet\sdk]
  2.2.108 [C:\Program Files\dotnet\sdk]
  2.2.301 [C:\Program Files\dotnet\sdk]
  3.0.100 [C:\Program Files\dotnet\sdk]
  3.1.100-preview1-014459 [C:\Program Files\dotnet\sdk]
  3.1.100-preview2-014569 [C:\Program Files\dotnet\sdk]

.NET Core runtimes installed:
  Microsoft.AspNetCore.All 2.1.9 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All]
  Microsoft.AspNetCore.All 2.1.12 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All]
  Microsoft.AspNetCore.All 2.1.13 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All]
  Microsoft.AspNetCore.All 2.2.6 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All]
  Microsoft.AspNetCore.App 2.1.9 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 2.1.12 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 2.1.13 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 2.2.6 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 3.0.0 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 3.1.0-preview1.19508.20 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 3.1.0-preview2.19528.8 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.NETCore.App 1.0.1 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 2.0.9 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 2.1.9 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 2.1.12 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 2.1.13 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 2.2.6 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 3.0.0 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 3.1.0-preview1.19506.1 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 3.1.0-preview2.19525.6 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.WindowsDesktop.App 3.0.0 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
  Microsoft.WindowsDesktop.App 3.1.0-preview1.19506.1 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
  Microsoft.WindowsDesktop.App 3.1.0-preview2.19525.6 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]

Commit reference: https://github.com/MintPlayer/QueryFilterIssue/commit/2be9e5e3565a03827bebaacc3453223e2b388aa3

[roji]

Confirmed that this happens with latest 3.1, minimal repro:

```c#
class Program
{
static void Main(string[] args)
{
using var ctx = new BlogContext();
ctx.Database.EnsureDeleted();
ctx.Database.EnsureCreated();

    var people = ctx.People.ToList();
}

}

public class BlogContext : DbContext
{
public DbSet People { get; set; }

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

protected override void OnModelCreating(ModelBuilder modelBuilder)
{
    modelBuilder.Entity<Person>().HasQueryFilter(p => p.UserDelete == null);
}

}

public class Person
{
public int Id { get; set; }
public User UserDelete { get; set; }
}

public class User
{
public int Id { get; set; }
}
```

[smitpatel]

@roji - Things to look at, there should not be any parameter generated. It should be inlined as null constant.

[roji]

@smitpatel here's some interesting intermediate findings... In this failing test case, the null ConstantExpression in the query filter has a type of object, and so it seems to be mistakenly identified by ContextParameterReplacingExpressionVisitor as IsAssignableFrom the context class. I confirmed that a check there excluding typeof(object) makes the error go away (is that a good fix though?).

Interestingly, in our existing test FiltersTestBase.Entity_Equality which I added, the null is a TypedConstantExpression of Customer, so this doesn't happen. I don't yet know why the filter lambdas get represented differently though - am looking into it (any ideas?).

A larger question for me is why ParameterExtractingEV considers constants evaluatable at all (in the sense that they're in _evaluatableExpressions). Although I'm sure there's a good reason and I'm missing some context. In any case learning a lot :)

[roji]

Oh I understand what's going on... our Northwind entity classes define typed equality operators (==), so in our test, o.Customer == null uses that and the null is typed to Customer. In the regular case, on the other hand, there's no equality operator so the null is an Object.

Note that this hasn't bitten us since the codepath using _contextParameterReplacingExpressionVisitor is only taken when _generateContextAccessors is true, which isn't the normal case (i.e. from query filters).

In light of all this I think simply excluding object in ContextParameterReplacingExpressionVisitor may be a correct fix, will submit a PR (but a test for this will probably require a BugTest).

[smitpatel]

Excluding object sounds like a correct fix.

[roji]

Thanks @smitpatel! Submitted #18761 to fix.

Seems to me we should consider bringing this into 3.1, /cc @ajcvickers.

[ajcvickers]

@roji What is the scope of what is not working? Is it any query filter that compares a navigation property to null? Is it more than that? Are there other conditions?

[roji]

Is it any query filter that compares a navigation property to null

That's right. There could be some other edge cases - any appearance of something typed as Object in the expression tree would trigger this bug; maybe @smitpatel can think up other cases.

[ajcvickers]

@roji Okay, sounds common enough to patch. Please create a PR on release/3.1 but don't merge yet. We'll take it through Tactics.
/cc @Pilchie

[smitpatel]

From my understanding only non-primitive compared to null. Primitive types have their own Equals. For entity/navigation it is easy to write in terms of key equality by user itself. For a collection type which is mapped to scalar by usage of value converter can cause the issue without work-around.

[ajcvickers]

@smitpatel

For entity/navigation it is easy to write in terms of key equality by user itself.

This is a good workaround. So I guess it depends just how many people will hit this and need to work around it. It seems to me that comparing a navigation to null in a filter would be quite common, but we could wait for more feedback before patching. Let's prepare the PR anyway.

For a collection type which is mapped to scalar by usage of value converter can cause the issue without work-around.

Comparing this to null in a query filter seems much less common.

[roji]

Am not sure (am outside), but won't this also affect null checks on non-navigation reference properties (e.g. e.SomeStringProperty == null)? Also harder to work around...

[smitpatel]

The null constant for those cases is null of actual type rather than object.

[roji]

Reopening to consider for 3.1

[roji]

Opened PR #18770 against release/3.1.

[roji]

The null constant for those cases is null of actual type rather than object.

@smitpatel you're right that this isn't a problem for string (which defines an equality operator like our Customer).

However, other reference types are affected when used as a simple non-navigation property. For example, checking if a byte array property is null in a query filter also fails; the error is different (Failed to convert parameter value from a BugContext18759 to a Byte[].; Object must implement IConvertible) but it disappears once the fix in #18761 is applied. Other examples of mappable reference types are all the NetTopologySuite spatial types, and Npgsql has some others.

What's worse, with non-navigation properties I'm not sure what the workaround for null comparison is (we can't simply rewrite to a comparison on the key).

[MintPlayer]

Comparing a navigation property to null in a QueryFilter works fine in EFCore-3.1. Thx