Make it easier to pass a CancellationToken to FindAsync

[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. dotnet/roslyn-analyzers#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. :-)

[Youssef1313]

I opened dotnet/roslyn-analyzers#4842 to track the analyzer bug.

[carlossanlop]

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)

Hey @bcronje, I wrote that analyzer. I remember we narrowed its behavior to only fire when the FindAsync overload that takes a CancellationToken, has the parameter of that type in the last position. I'm not sure why it's firing when clearly the FindAsync overload has it in the first position.

If you don't want to propagate the CancellationToken (this is a rare, valid scenario, but needs to be addressed manually), then you can either pass a CancellationToken.None or a new CancellationToken().

You can propagate the CancellationToken to the FindAsync overload that takes it, but pass it with a named parameter. If you still see the analyzer being fired, then try putting the named CancellationToken parameter in the last position. If after all that, you still see the analyzer, then you can wrap that invocation with:

pragma warning disable CA2016
... your invocation ...
#pragma warning restore CA2016

Thanks @Youssef1313 for opening the issue in roslyn-analyzers, I'll take a look when I get a chance.

[carlossanlop]

Update: We do not support the scenario where the cancellation token is not in the last position:

https://github.com/dotnet/roslyn-analyzers/blob/a0522afd52b25ad211298e308fd5bbc7734865e0/src/NetAnalyzers/UnitTests/Microsoft.NetCore.Analyzers/Runtime/ForwardCancellationTokenToInvocationsTests.cs#L211-L227

So the problem seems to be that params object[] is not being detected, which essentially allows any object to be passed, even though there is another parameter with the explicit type.

[carlossanlop]

So this does not seem to be a problem with CA2016.

We are boxing the CancellationToken struct, and passing it to the params object[] argument. This would happen with any other value type and with any other method, unrelated to the cases that CA2016 can handle.

I can't repro the error case though. See my code snippet here: dotnet/roslyn-analyzers#4842 (comment)

[martincostello]

I've created this bug for myself about three times using EFCore now, and I found this issue while seeing if there was already one open related to it.

Would there be any value in adding an in-box extension method for DbSet<T> (or indeed adding another method to it directly) to make it easier for developers to fall into the pit of success when using this API? I've added this to a project of my own to try and prevent myself stepping on this particular rake again (assuming I remember to use it):

namespace Microsoft.EntityFrameworkCore
{
    public static class DbSetExtensions
    {
        public static ValueTask<TEntity?> FindItemAsync<TEntity, TKey>(
            this DbSet<TEntity> set,
            TKey keyValue,
            CancellationToken cancellationToken = default)
            where TEntity : class
        {
            if (keyValue is null)
            {
                throw new ArgumentNullException(nameof(keyValue));
            }

            return set.FindAsync(new object[] { keyValue }, cancellationToken);
        }
    }
}

[ChristopherHaws]

@martincostello Or you could do this (which is similar to my proposed solution)

public static class DbSetExtensions {
    public static async ValueTask<T?> FindItemAsync<T>(this DbSet<T> set, params object[] keyValues) where T : class =>
        keyValues[^1] is CancellationToken ct ? await set.FindAsync(keyValues[0..^1], ct) : await set.FindAsync(keyValues);
}

I also tend to make my own extension methods for every entity so that I have type safety:

public static class DbSetExtensions {
    public static async Task<Blog?> GetAsync(this DbSet<Blog> set, int blogId, CancellationToken ct = default) =>
        await set.FindAsync(new object[] { blogId }, ct);

    public static async Task<Blog> GetRequiredAsync(this DbSet<Blog> set, int blogId, CancellationToken ct = default) =>
        await set.GetAsync(blogId, ct) ?? throw new Exception($"Could not find blog {blogId}.");
}

[SoftCircuits]

So, two years later, and the issue still exists in .NET 8? And I didn't see a workaround?

[ajcvickers]

@SoftCircuits This issue was fixed in EF7. If you are still encountering problems, then please open a new issue and attach a small, runnable project or post a small, runnable code listing that reproduces what you are seeing so that we can investigate.