Dapper throws an error when operating on IDbCommand that is not DbCommand

[ViktorHavrysh]

I have a wrapper implementation of IDbCommand for logging purposes. When I use it, Dapper throws an error during mapping. I believe this piece of code is at fault:

https://github.com/StackExchange/Dapper/blob/61e965eed900355e0dbd27771d6469248d798293/Dapper/SqlMapper.Async.cs#L480

Here my wrapper (which implements IDbCommand rather than inheriting from DbCommand) gets cast directly to DbCommand, producing an InvalidCastException:

System.InvalidCastException occurred
  HResult=0x80004002
  Message=Unable to cast object of type 'Core.Logging.Database.ProfiledDbCommand' to type 'System.Data.Common.DbCommand'.
  Source=Dapper
  StackTrace:
   at Dapper.SqlMapper.<ExecuteImplAsync>d__29.MoveNext()
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at System.Runtime.CompilerServices.ConfiguredTaskAwaitable`1.ConfiguredTaskAwaiter.GetResult()
   at Balloting.DataAccess.PostgreSQL.Repository.ApplicationRepository.<DeleteApplication>d__5.MoveNext() in C:\Projects\Api\src\DataAccess.PostgreSQL\Repository\ApplicationRepository.cs:line 123

[NickCraver]

Yep - but we have to do this, as IDbCommand pre-dates any async functionality in ADO.NET, no async methods exist on the interface.

Can you elaborate on why you're calling async methods on something that doesn't implement async?

[ViktorHavrysh]

I just want to log all the SQL that gets sent to the database. A wrapper around IDbConnection seemed like an obvious solution...

May I ask a question in return: Why is Dapper defined against the IDbConnection interface if it actually requires DbConnection to operate? That seems to go against all kinds of best practices.

[NickCraver]

Why is Dapper defined against the IDbConnection interface if it actually requires DbConnection to operate?

This is only true in async, not in general. Could we make the extensions against DbConnection for the async set? Maybe, but that has other issues across providers.

If you want to log all SQL that gets sent, I'd wrap your actual connection to log things as they pass. If you want an example of this, look at MiniProfiler where we do exactly that: here's where to start.

[ViktorHavrysh]

I hoped for a more elegant solution... Oh well, I'll work around it. But I do think that the way the async methods work is a big no-no. It breaks all kinds of encapsulation. I only caught this breakage when moving to Dapper with async during integration tests, whereas the wrapper used to work just fine before.

[NickCraver]

@VictorGavrish let me approach it from this way: how else would you do it? Is the argument that we should move the extension methods to DbCommand in v2 for async things?

If so, @mgravell - thoughts?

[ViktorHavrysh]

@NickCraver You mentioned that that had some problems, but I can't think of any really big ones. Maybe I'm missing something?

[ViktorHavrysh]

Actually, there is another way to solve the breakage, at least. You can still define the methods against IDbCommand, but stop using a direct cast. Isntead, check if it's DbCommand first, and if it is, then proceed as you do now; otherwise, emulate the async API with sync calls. That's still not at all elegant, and should probably throw a warning flag somehow (not sure how), but it's at least better than unexpected breakage. I'm not sure if this is a better solution than just defining the async methods against DbCommand, mind you.

[ViktorHavrysh]

Actually, the very least thing you can do is detect whether the IDbCommand you are working with is an instance of DbCommand, and then throw a custom error explaining what exactly went wrong. It took me a while to figure out the problem, I would have really appreciated a better error message than "An invalid cast somewhere deep within the async pipeline".

[felipecruz91]

Any updates on how to solve this?

[mgravell]

"don't do it"? I mean, we can improve the error message, but I'm loathe to implement async-over-sync - it is basically a lie to the caller. If we can't support it properly, I'd rather the caller knows that and simply: uses the sync API if their chosen provider or wrapper type only exposes that.

[benmccallum]

Just ran into the IDbConnection >> DbConnection part of this issues as well in a PR for Dapper.GraphQL. In Dapper.GraphQL.Tests we have a wrapper around a DbConnection implementing IDbConnection for test fixture purposes. All worked well in sync calls but when we implemented async, tests failed on "Could not be parsed to DbConnection" because of above.

If there's a legitimate reason to accept IDbConnection over DbConnection in async method sigs, e.g. to resolve issues across providers like @NickCraver mentioned, I think at the very least a better error message would be helpful. Instructing caller to open themselves or use a real DbConnection.

[NickCraver]

I agree the error message should be better here until we can even possibly move the methods in a breaking change.

Can people here please take a look at #986 and see if the error there is clear or suggest what would be clearer?

[benmccallum]

I think that message is fine if the intent is to discourage use of anything but DbConnection. But it does work with a simple IDbConnection if the user opens the connection themselves beforehand, so you could lean towards something like "Async operations require use of a DbConnection or open IDbConnection" and only throw if the connection isn't DbConnection AND is closed.

Also, for TrySetupAsyncCommand, does the type check need to be on the connection? Shouldn't it be if the command is a DbCommand before it's cast to one. That allows for the scenario/message above.

Edit: I think I see why you were checking for DbConnection in the command setup method too, because the connection is what creates the command. I guess there could be a scenario where someone implemented an IDbConnection that produces DbCommands... would be weird but how about something like: #987

[benmccallum]

They had a similar issue here: madelson/DistributedLock#3.

It's a good discussion because it points out that MS can't add new methods to the interface for backwards compat, so the library author was using DbConnection and DbCommand as Dapper currently does. But the issue was raised to achieve testability with the interfaces. So we're stuck between a rock and a hard place it would seem. I tend to think that the error message may be enough but you guys are the experts 😄

They ended up going with the interfaces and wrapping the synchronous calls in async wrappers, which is what @mgravell was loathing haha.

[NickCraver]

@benmccallum I didn't see your PR before sitting down and adjusting based on email - please see #986 for updated error messages and a bit more allowance on the DbCommand side.

[benmccallum]

Haha, no worries. Would've been sweet to be on the contributors list for such a great library but all good! You've nailed it!

[NickCraver]

@benmccallum You're the one calling out these things! Just yanked what I could from your PR into the fold - welcome, contributor :)

[benmccallum]

Haha legend!

PS. You're both legends. Read both of your blogs. The stuff on StackOverflow's architecture is always super interesting. Thanks for sharing with the community. cc: @mgravell

[jstafford5380]

"don't do it"? I mean, we can improve the error message, but I'm loathe to implement async-over-sync - it is basically a lie to the caller. If we can't support it properly, I'd rather the caller knows that and simply: uses the sync API if their chosen provider or wrapper type only exposes that.

But isn't already a lie to the caller? I mean, we don't call OpenAsync; we call QueryAsyc. I'm not super sure on how it all works under the cover, but, is Querying asynchronously somehow coupled to Opening the connection asynchronously? My thought is that I should be able to open the connection synchronously and still be able to run an actual query asynchronously.

[mgravell]

@jstafford5380 the reality is that nothing is async if you're using the interface; frankly, it was a mistake to make those methods take the interface, so yes: that part is a lie. My vote is: next time we do a break (probably when adding IAsyncEnumerable<T> support), we make the async methods take DbConnection.

[Khaos66]

Most of Dapper uses IDbConnection by now, but I just created PR #1797 to fix this for Dapper.Rainbow, too.

Looks like this issue can be closed?

[mgravell]

The reality is that as more and more emphasis goes on async, anything using non-DbConnection etc: is going to fail; similarly, there are requested features that demand those APIs, including some breaking changes

I think ultimately that any change here is going to further solidify that DbConnection et al are needed, not the opposite. In reality, we should probably have based Dapper on DbConnection (not IDbConnection) in the first place; that ship, however, has long sailed