FindAsync + CancellationToken issue

[ilmax]

Hi,

working on a side project I found out that calling Set.FindAsync(key, cancellationToken) throws this error:

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

on version 2.1.0-rc1-final

Just removing the cancellationToken parameter from the FindAsync call fixes the problem, so I decided to check if it's reproing on the current dev branch thus I have extended the FindTestBase class to add a new FindAsync overload because I wasn't able to find an existing test using FindAsync and passing in a cancellationToken.

The added overload looks like the following:

FindAsync<TEntity>(DbContext context, object[] keyValues, CancellationToken cancellationToken)

this is the added test:

[Fact]
public virtual async Task Find_int_key_from_store_async_with_cancellation_token()
{
	using (var context = CreateContext())
	{
		Assert.Equal("Smokey", (await FindAsync<IntKey>(context, 77, CancellationToken.None)).Foo);
	}
}

and the code fails on current dev with the following exception:

   System.ArgumentException : Entity type 'IntKey' is defined with a single key property, but 2 values were passed to the 'DbSet.Find' method.                                                                     
  Stack Trace:                                                                                                                                                                                                     
     at Microsoft.EntityFrameworkCore.Internal.EntityFinder`1.FindTracked(Object[] keyValues, IReadOnlyList`1& keyProperties) in C:\Code\EntityFrameworkCore\src\EFCore\Internal\EntityFinder.cs:line 259          
     at Microsoft.EntityFrameworkCore.Internal.EntityFinder`1.Microsoft.EntityFrameworkCore.Internal.IEntityFinder.FindAsync(Object[] keyValues, CancellationToken cancellationToken) in C:\Code\EntityFrameworkCor
e\src\EFCore\Internal\EntityFinder.cs:line 102                                                                                                                                                                     
     at Microsoft.EntityFrameworkCore.DbContext.FindAsync(Type entityType, Object[] keyValues) in C:\Code\EntityFrameworkCore\src\EFCore\DbContext.cs:line 1367                                                    
     at Microsoft.EntityFrameworkCore.FindSqliteTest.FindSqliteTestNonGeneric.FindAsync[TEntity](DbContext context, Object[] keyValues) in C:\Code\EntityFrameworkCore\test\EFCore.Sqlite.FunctionalTests\FindSqlit
eTest.cs:line 62                                                                                                                                                                                                   
     at Microsoft.EntityFrameworkCore.FindTestBase`1.Find_int_key_from_store_async_with_cancellation_token() in C:\Code\EntityFrameworkCore\src\EFCore.Specification.Tests\FindTestBase.cs:line 395    

This smells like an issue to me and I'm looking into it, just created this issue for letting you know guys.
I wasn't able to find any similar issue, if it's a dupe, feel free to close it

[ilmax]

The reason behind this is quite simple, the C# compiler binds a call to the FindAsync(whatever, cancellationToken) to this FindAsync(params object[] keyValues); overload, instead of using the FindAsync(object[] keyValues, CancellationToken cancellationToken) version.

This is a bit unfortunate IMHO

[ajcvickers]

@ilmax This is a common binding issue that we don't have any great solutions for. It's the reason that the overload that takes a cancellation token is not params, but we haven't been able to come up with a great solution for the one that just takes params and no cancellation token. We're unlikely to change the method signatures at this point, but would be interested in hearing if you have any ideas on how it could have been done differently?

[ilmax]

@ajcvickers thanks for the response, I was thinking about how common is to pass multiple values to the FindAsync(params object[] keyValues), maybe we can just remove the params modifier, but this would result in a breaking change.

We should suggest to the C# team to implement wunderbar betterness :) and ask them to lower the priority of overloads with params modifier... just kidding :)

Apart from the aforementioned breaking change solution, no other obvious workaround comes to my mind yet

[divega]

@ajcvickers do you remember if we decided explicitly against special-casing when params size is 1 more than expected and the last element is a CancellationToken?

[ajcvickers]

@divega Yes, we decided against it. I think it was because:

• For working code, this has an overhead of checking the last element which just should not be needed.
• You can't get it wrong just from not knowing about binding rules. If you look at the signature that takes a cancellation token it doesn't take params. So the signature actually tells you that you can't call the method with a cancellation token using params syntax.
• You find out your error in using the wrong overload immediately you run.

[ajcvickers]

Triage: we decided not to change this.

[space-alien]

Am I correct in understanding that there is no way to call FindAsync with a cancellationToken?

[ajcvickers]

@space-alien You should be able to use something like:

FindAsync<IntKey>(new object[] { context, 77 }, CancellationToken.None);

[henrikdahl8240]

Can't you just do it like in EF 6, where an overload takes the CancellationToken as the first argument and the params as the second argument?

[JohannSig]

Can't you just do it like in EF 6, where an overload takes the CancellationToken as the first argument and the params as the second argument?

Just write an extension method for it:

	public static class XDbSet
	{
		public static ValueTask<T> FindByIdAsync<T>(this DbSet<T> dbSet, object key, CancellationToken cancellationToken)
			where T : class
			=> dbSet.FindAsync(new object[] { key }, cancellationToken);
	}