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.81k stars 3.2k forks source link

Support ContainsKey (as an analyzer is telling us to use it) #26856

Open benmccallum opened 2 years ago

benmccallum commented 2 years ago

What problem are you trying to solve? After recently upgrading to .NET 6, there's a new analyzer prompting you to convert dict.Keys.Contains("foo") to dict.ContainsKey("foo"), but that's not supported by EF Core, so a quick refactor can have nasty consequences.

Describe the solution you'd like Can dict.ContainsKey be supported?

I could probably help out with some direction.

(didn't see any previous issues actually ask for this, sorry if it's been previously rejected)

benmccallum commented 2 years ago

It might also be worth raising an issue on the analyzer side if they're able to selectively ignore this scenario (inside a Where on an IQueryable I guess?). I can also do that, would just need to figure out which repo to raise it on.

roji commented 2 years ago

@benmccallum where in the EF Core API is ContainsKey not supported? Can you give a quick code sample that shows it?

ajcvickers commented 2 years ago

@roji Do we translate ContainsKey?

roji commented 2 years ago

@ajcvickers are we talking about property bags? Or some database type that's mapped to a .NET Dictionary (e.g. jsonb or hstore on PostgreSQL)? Just want to make sure we're talking about the same thing with @benmccallum

stevendarby commented 2 years ago
var vehicleTypes = new Dictionary<string, bool> { {"Car", true }, { "Lorry", true } };
var vehicles1 = context.Set<Vehicle>().Where(x => vehicleTypes.Keys.Contains(x.Type)).ToList();
var vehicles2 = context.Set<Vehicle>().Where(x => vehicleTypes.ContainsKey(x.Type)).ToList();

Pretty sure it's about this sort of thing. First query translates to Type IN ('Car', 'Lorry') and second one can't be translated.

roji commented 2 years ago

Triage decision: translating ContainsKey as above is something that is hard for EF Core to support in an efficient way. We will consider adding a diagnostics suppressor to suppress this warning when it appears inside an EF Core LINQ query (best effort). This would depend on having infrastructure for identifying EF Core LINQ queries at compile-time, which we don't currently have.

benmccallum commented 2 years ago

Sorry I didn't get back in time and for my lack of an example. Just wanted to confirm the example that Steven shared is exactly it. It's the ContainsKey on a dictionary in a Where expression.

JustArchi commented 2 years ago

The issue seems to be pretty old but I've stumbled upon this just today as well. I'd have expected dictionary.ContainsKey(x) to be internally handled the same as dictionary.Keys.Contains(x). If this is not possible to do in an efficient way, CA1841 should take it into account when inside EF LINQ query.

Barsonax commented 1 year ago

The analyzer seem to be fixed in .net7 as it doesnt warn you anymore if you use Keys.Contains(x) in a ef query. On .net6 I still see the warning.

Time to upgrade as I don't think the analyzer will be fixed in .net 6.

kescherCode commented 1 year ago

@Barsonax I encountered this in .NET 8-RC2 (along with EF Core 8), though.