-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
QueryFilter (cont): KeyNotFoundException: The given key '__ef_filter__p_0' was not present in the dictionary. #18759
Comments
Confirmed that this happens with latest 3.1, minimal repro: 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<Person> 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; }
} |
@roji - Things to look at, there should not be any parameter generated. It should be inlined as null constant. |
@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 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 :) |
Oh I understand what's going on... our Northwind entity classes define typed equality operators (==), so in our test, 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). |
Excluding object sounds like a correct fix. |
Thanks @smitpatel! Submitted #18761 to fix. Seems to me we should consider bringing this into 3.1, /cc @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? |
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. |
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. |
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.
Comparing this to null in a query filter seems much less common. |
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... |
The null constant for those cases is null of actual type rather than object. |
Reopening to consider for 3.1 |
Opened PR #18770 against release/3.1. |
@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 ( 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). |
Comparing a navigation property to null in a QueryFilter works fine in EFCore-3.1. Thx |
Follow up of: #18158
Repository: https://github.com/MintPlayer/QueryFilterIssue (Only mind DotNetCore31)
Now I'm getting the following exception when yielding the DbSet from the database:
The QueryFilter on my DbContext:
Once again uncommenting the QueryFilter "fixes" the problem.
Steps to reproduce
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
Further technical details
Information
Commit reference: https://github.com/MintPlayer/QueryFilterIssue/commit/2be9e5e3565a03827bebaacc3453223e2b388aa3
The text was updated successfully, but these errors were encountered: