Efcore: FindAsync rough edges when passing CancellationToken

[ChristopherHaws]

FindAsync currently takes a params object array as its first parameter and cancellation token as its second parameter. In entities that use struct's as keys (which is probably the norm for most entities) it is easy to mess up the parameters to this method.

For example, given the following entity context:

public class ApplicationContext : DbContext {
    public ApplicationContext(DbContextOptions<ApplicationContext> options)
        : base(options) {
    }

    public DbSet<Blog> Blogs { get; set; } = default!;
}

public sealed class Blog {
    public Guid BlogId { get; set; }
    public string Name { get; set; } = default!;
}

Attempt One

The first attempt to use FindAsync would probably look like this, which will compile but is actually passing both parameters to the object array and not passing the cancellation token to the correct parameter:

var blog = await db.Blogs.FindAsync(blogId, cancellationToken);

This results in the following error:

ArgumentException:
Entity type 'Blog' is defined with a single key property, but 2 values were passed to the 'DbSet.Find' method.

Attempt Two

The second attempt to use FindAsync would probably be to change the first parameter to be an array so the type system passes the correct parameters:

var blog = await db.Blogs.FindAsync(new[] { blogId }, cancellationToken);

This however does not work because the array is not specifically an object[], so you still get the following error:

ArgumentException:
Entity type 'Blog' is defined with a single key property, but 2 values were passed to the 'DbSet.Find' method.

NOTE: If BlogId were a reference type (i.e. string) instead of a value type, this usage actually works.

Correct Usage

var blog = await db.Blogs.FindAsync(new object[] { blogId }, cancellationToken);

Possible Solutions

Solution 1

The first, an my prefered, solution would be to add some checks in FindAsync to detect if the last item in the keyValues parameter is a CancellationToken if the count of keys in the entity being queried does not match the count of keys passed in. In this scenario, if the last item is a CancellationToken, use it as a CancellationToken instead of part of the key.

Solution 2

Add analyzers to detect this incorrect usage of the API to provide build time errors instead of runtime errors.

[ajcvickers]

Duplicate of #12012.

@ChristopherHaws Solution 1.1 was discussed in #12012. Solution 1.2 would not work because keys can be arrays with a value converter (or without if your database supports them).

An analyzer is interesting, although it's worth keeping in mind that analyzers have turned out to be expensive to create and tend to have a long bug tail, so I think we would only add one if the value was very high.

[ChristopherHaws]

Thanks for the response. I will remove solution 1.2.

For working code, this has an overhead of checking the last element which just should not be needed.

Could this overhead be avoided by checking the last element of the array only if the key count doesn't match? This check is already happening somewhere since there is an exception for it.

You find out your error in using the wrong overload immediately you run.

Unfortunately I have had this happen more times than I wish to admit 😎, and it's not a fun one to track down in a dll with optimizations enabled. If we don't want to change the functionality of the FindAsync method I really think an analyzer is the right solution. It's too easy to misuse this API (specifically the scenario where you pass new[] { id } instead of new object[] { id } throws me off everytime 🤦‍♂️. I almost wonder if that is an analyzer that could exist at the CSharp level).

[ajcvickers]

@ChristopherHaws This comment:

Could this overhead be avoided by checking the last element of the array only if the key count doesn't match?

Made me reconsider and I talked about it with the team today. I think we can use this to allow the cancellation token to be passed as the last value in the array. Thanks for bringing this up!

[bcronje]

The problem is the API signature also causes analyzers to suggest the wrong fix as can be seen in this image which is unfortunate:

[smitpatel]

How is the code fix incorrect? Your method takes cancellation token, you should pass it forward to any async methods invoked inside if they support it.

[bcronje]

How is the code fix incorrect? Your method takes cancellation token, you should pass it forward to any async methods invoked inside if they support it.

I understand the logic behind the fix suggestion, the problem is if you apply the code fix as is then you end up with the following error because EF Core decided to put the cancellation token argument after the key values array:

Error CS1503 Argument 1: cannot convert from 'string' to 'object[]'

EF does not have this design flaw because it puts the token argument before the params key values argument, compare the two signatures:

EF DBSet DbSet.FindAsync(CancellationToken, params Object[])

EF Core DBSet DbSet.FindAsync(Object[], CancellationToken)

So the correct analyzer code fix for EF Core specifically should unfortunately be:

var user = await db.Users.FindAsync(new object[] { id }, token)

[smitpatel]

Seems like the code fix is erroneous here though. Argument 1 type actually changes between both methods. It is not just overload which takes extra cancellation token parameter. So it should not suggest that code fix.

[ajcvickers]

@bcronje Regardless of the EF Core behavior, this bug with the analyzer should be reported against the analyzer. (I'm not sure where analyzer issues are tracked.)

[bcronje]

@ajcvickers I agree and apologize if it came across that I wanted to hijack this issue, I just mentioned it because the OP's solution 2 mentioned analyzers and I ran into this issue yesterday.

[smitpatel]

https://github.com/dotnet/roslyn-analyzers is the repo. https://github.com/dotnet/roslyn-analyzers/pull/3641 implemented that particular code fix.

[ajcvickers]

@bcronje Sorry, I didn't mean it to come across like that. I just wanted to make sure the analyzer bug didn't get missed. :-)