Make the experience better around FindAsync and cancellation tokens

[voroninp]

What problem are you trying to solve?

FindAsync method has two overloads.

If someone makes a mistake and calls dbSet.FindAsync(id, cancellationToken) instead of dbSet.FindAsync([id], cancellationToken)
wrong overload version will be selected. Compiler won't complain.

Describe the solution you'd like

I believe, it's worth either having an additional prioritized overload:

Task<TEntity?> FindAsync(object id, CancellationToken cancellationToken);

Or just move cancellationToken out of the last component of the array.

Alternatively, it can be an analyzer.

[roji]

Previously discussed in #34966, #22667.

FWIW I agree that the current situation is a bad pit of failure, and that we should make it better.

Just adding the proposed additional overload (for single-property keys only) would address the problem for that scenario, but scenarios with composite keys will continue to suffer from the same problem (even though they're more rare).

The correct IMHO - but extreme - solution here would be to remove the params overload altogether; with collection initializers, params becomes that much less important, since users can just do FindAsync([...], token). We could still introduce the single-property overload proposed above to remove the need for the collection initializer in the single-property key case. Of course, that would be a (non-trivial) breaking change, and we also can't obsolete here (since the change would only remove the params, keeping the overload otherwise the same).

Otherwise, an analyzer to identify cases where the cancellation token argument would be considered part of the key should be pretty easy and safe, including a code-fix to correct the code (i.e. use a collection initializer).

[cincuranet]

I would support removing the params (in every place around Find/FindAsync for consistency sake) and "forcing" people to use collection initializers (#34966 (comment)).

[roji]

Design decision: for 10, we'll obsolete the current params-based overload. This will guide users to move away from it, and over to the other overload (FindAsync(object?[]?, CancellationToken). In EF 11, we'll remove the obsolete overload, replacing it with a FindAsync(object?[]?) that has no params. At that point we have the ideal API shape. Note that C# collection initializers makes calling code nice without needing params:

_ = await FindAsync([id1, id2], token);

As additional optional sugar, we can also introduce an overload that accepts a single key property, as suggested above by @voroninp. This would mean single-property users (the vast majority) to not need collection initializers, only composite key users:

_ = await FindAsync(id, token);

We can do this right away in 10: this would remove the obsoletion and the need to change code for single-property keys.

An alternative proposal which was discussed was to add an overload where the cancellation token is the first parameter, followed by the params array. While that overload would be safe, it would not solve the pit of failure problem: users would understandably still try to call FindAsync with an Id property followed by a cancellation token, invoking the wrong overload.

[roji]

... and one additional thought about the proposed single-property FindAsync: this overload carries the same issues as with any object-typed parameter, namely, anything will match it. For example, I can imagine someone with a composite string key trying to do the following:

List<string> key = ...;
var result = await context.Blogs.FindAsync(key, token);

Since the key is a List<string> rather than a string[] (which would maybe object[] because arrays are co-variant), this would resolve to the single-key overload and (usually) fail. Note that in some contrived cases, an array may actually be valid as a (single-property) entity key (e.g. in document databases, in PostgreSQL).

On the other hand, the majority of uses are single-property keys, so the above pit of failure only applies to quite a small minority, while simplifying the majority use.

@dotnet/efteam let me know what you think - at this point I'm slightly leaning towards not having the single-property overload and just forcing everyone to use collection initializers always, to be super-clear and unambiguous.

[voroninp]

If not performance concerns, I'd suggest adding more dynamism and checking object shape.

If it's a collection, then use it as components of the key.
If it's an object and there's no value converter for its type, use its properties as components of the key (matching by name).
Otherwise use the value as a key.

But this increases the complexity a lot and is hardly worth it.

With a single object overload there's a potential for saving one allocation:

FindAsync is part of DbSet<T>, hence it should know the number of key's components of the entity T, so it can use ArrayPool to borrow an array, but then this number of components should cascade down the call stack. Again, it is not worth it, I assume.

[cincuranet]

just forcing everyone to use collection initializers always, to be super-clear and unambiguous

Yes.