Analyzer to spot comparison with primitives in EFCore Linq selectors

[SteveDunn]

In a Linq query across a collection I can do this without a problem

var x = list.Where(e => e.Id == 1).Single();

However, the same query in EFCore will fail with an InvalidCastException and a message of 'Invalid cast from 'System.Int32' to 'Domain.Id'

To fix this, an explicit cast is required

var x = context.Entities.Where(e => e.Id == (Id)id).Single();  

or

var x = context.Entities.Where(e => e.Id == Id.From(id)).Single();  

These are difficult to spot at compile time as the Id and int types are equatable.

One way to prevent the runtime errors is to generate the implicit cast, but this allows bypassing the From method and introduces the potential to create invalid instances. Originally posted by @rbanks54 in https://github.com/SteveDunn/Vogen/issues/502#issuecomment-2421061334

[rbanks54]

Just some thoughts...

If the analyzer is applied to all linq queries regardless of their usage, it could break other people's code where they have non-EF queries. Probably not the best outcome 🙂

If the analyzer can detect an EF query by checking if the class being queried inherits from DBContext that would be wonderful. This is this is likely harder that the simple scenario I added as the comparison could be defined in a helper method or specification object that generates a where clause. I don't know how easy it is chasing that back up the call tree to see where it might be used.

The analyzer would also need to check the other form of EF querying as the syntax tree is different; i.e.

var x = (from e in context.Entities
  where e.Id == id
  select e).Single();

I've not done much with analyzers, so I don't know how tricky any of this might be. It sounds like a fun problem to try and solve, though!

[SteveDunn]

Just some thoughts...

If the analyzer is applied to all linq queries regardless of their usage, it could break other people's code where they have non-EF queries. Probably not the best outcome 🙂

If the analyzer can detect an EF query by checking if the class being queried inherits from DBContext that would be wonderful. This is this is likely harder that the simple scenario I added as the comparison could be defined in a helper method or specification object that generates a where clause. I don't know how easy it is chasing that back up the call tree to see where it might be used.

The analyzer would also need to check the other form of EF querying as the syntax tree is different; i.e.

var x = (from e in context.Entities
  where e.Id == id
  select e).Single();

I've not done much with analyzers, so I don't know how tricky any of this might be. It sounds like a fun problem to try and solve, though!

Hi @rbanks54 - I've had a first attempt at this. I'll release an alpha version later today if you wouldn't mind playing about with it? It looks for invocation expressions named Where, Select TakeWhile, SkipWhile etc. It then checks to see if they're being called on anything that derives from DbSet. It then looks at each sub-expression to see if the left value is a value object and the right value is a primitive (int).

It seems to work quite well. Good point re. the other syntax for querying. I just tried that and it doesn't work! I'll see if I can get it working.

Many thanks.

[rbanks54]

It's looking like a good start :)

Both line 127 and 130 in the screenshot are detected as problems.

The last approach (lines 132 to 135), where I split up a query, doesn't get detected as a problem though :(

Because I split the line up, the type for step1 is DbSet<EmployeeEntity>? rather than the non-nullable type. You might need to check for nullable DbSets in the analyzer if you haven't already done so.

When we get to the where clause, the type is just an IQueryable<EmployeeEntity>?. The DbSet typing has been lost due to the fluent chaining.

[Edited: I had a corrected screenshot in the original message]

[viceroypenguin]

When we get to the where clause, the type is just an IQueryable?. The DbSet typing has been lost due to the fluent chaining.

I would be very cautious about extending the analyzer to IQueryable<> since that would necessarily include other db engines and even linq to objects .AsQueryable() into the detection, which could lead to false positives.

[SteveDunn]

When we get to the where clause, the type is just an IQueryable?. The DbSet typing has been lost due to the fluent chaining.

I would be very cautious about extending the analyzer to IQueryable<> since that would necessarily include other db engines and even linq to objects .AsQueryable() into the detection, which could lead to false positives.

Thanks for the steer @viceroypenguin . It first checks for IQueryable and then checks to see if it's specifically a DBSet in the efcore namespace, so I think we should be good.

[SteveDunn]

It's looking like a good start :)

Both line 127 and 130 in the screenshot are detected as problems.

The last approach (lines 132 to 135), where I split up a query, doesn't get detected as a problem though :(

Because I split the line up, the type for step1 is DbSet<EmployeeEntity>? rather than the non-nullable type. You might need to check for nullable DbSets in the analyzer if you haven't already done so.

When we get to the where clause, the type is just an IQueryable<EmployeeEntity>?. The DbSet typing has been lost due to the fluent chaining.

Thanks for the quick feedback! I'll take a look at those (very cunning) scenarios!

[SteveDunn]

I can get it to work for:

var step1 = context.Entities;
var step2 = step1.Where(e => e.Id == id);

But, I can't get it to work for the extra level of indirection in:

var step1 = context.Entities;
var step2 = step1.Take(4);
var step3 = step2.Where(e => e.Id == id);

step2 changes from DbSet<> to IQueryable<>.

[rbanks54]

Would a recursive check up the call tree be viable?

i.e. if the analyzer finds the source is an IQueryable, then use SymbolFinder.FindDeclarationsAsync get the IQueryable's definition and and see if it, in turn, is using a DbSet or IQueryable as it's source?