netstandard2.0: DataTable isn't usable as a parameter (SqlClient/DataTable issue) #21671
Comments
This adds a netstandard2.0 build to Dapper (and StrongName until v2), adds a netstandard2.0 test lineup, restores most functionality (except UDTs for SQL, e.g. .UdtTypeName isn't in netstandard2), and also breaks things not actually implemented or implemented correctly in netstandard2.0. Pushing this up so we can work through these with the .NET teams. Several things are still broken here (and fail tests): - Parameter decimal values (not filed yet) - DataTables as parameters (https://github.com/dotnet/corefx/issues/19708) - .GetSchemaTable() (not yet filed, implementation is completely missing from SqlDataReader in CoreFX) While the compile is clean, the above are runtime issues. `SqlParameter.UdtTypeName` is a separate issue, that one just wasn't in `netstandard2.0` at all...so we can't support UDTs there until it is. See https://github.com/dotnet/corefx/issues/17126 for details.
|
@saurabh500 @corivera can you please take a look? This looks like 2.0 blocker. Thanks! |
|
I've pushed up the After pulling, you can do a |
|
@NickCraver While running the Dapper tests I get the following error: Do you have anything special setup before running build.cmd to setup dotnet-xunit? |
|
@saurabh500 Nothing special, and I just tried it on a fresh VM with only .NET Core 2 installed. Perhaps you have a newer/internal |
|
@NickCraver After running |
|
@geleems Dapper assumes a local SQL Server running (since we have to have a database to hit). If you fire one up, it uses your login and runs in tempdb. |
|
@NickCraver Thanks. I ran localdb, and was able to kick off the tests. |
|
@geleems I only see the |
|
@geleems I could get the tests running I cloned Dapper and then checked out Nick's commit I uninstalled all the versions of .Net Core. I have .Net Core 2.0 preview 1 installed from https://www.microsoft.com/net/core/preview#windowscmd I ran the tests and I got an error After this I installed .Net Core Runtime 1.0.5 from https://www.microsoft.com/net/download/core#/runtime. After this I can execute the tests successfully and I can repro the failures that @NickCraver has reported. The beginning of my Console Output looks like this. |
|
I have a repro with SqlClient code, I extracted this from Dapper What this repro highlights is that SqlClient Parameters don't support TVPs. |
|
I was able to port the code for TVPs using DataTables in SqlParameters and after running the Dapper tests using my changes I could see 100% pass rate in Dapper.Tests @NickCraver I do see a couple failures in One of them is |
|
@saurabh500 how can customers detect the missing feature? Is it runtime failure? Or compile time / missing API failure? |
|
@karelz I think you are referring to my comment This will lead to runtime anomaly in behavior. If one expects SqlClient command executions to be wrapped in a Single With TransactionScope support in SqlClient, we will not be able to get to the complete Desktop behavior, because only Local transactions are supported in Sys.Transactions and Distributed transactions across different Sql Servers will not be supported due to the current limitations of Sys.Transactions. |
|
What will the customer see? Exception? Or just "wrong non-transactional behavior"? |
|
No exceptions. They will see non-transactional behavior |
|
How bad and surprising is it? Can we flag specific method, field, enum value or something in the ApiCompat tool? |
|
IMHO, if the user uses TransactionScope in the code, they'll assume it will work if there are no compile errors. If the user gets a runtime exception, it will be frustrating perhaps for the user, but at least they won't assume the work is performed using a single transaction. If things are silently done without transactional behavior it can be seriously bad if they have to rollback due to an error and the code won't roll back, resulting in an inconsistent database. |
|
I have opened an issue https://github.com/dotnet/corefx/issues/19894 to discuss this behavior. I agree with @FransBouma . In case all the operations in a TransactionScope go well, then there would be no customer complains, but if one of the operations fail, the the application would expect all the operations in TransactionScope to be rolled back. This is bad for data integrity. |
|
Reopening to track port to rel/2.0.0 branch |
|
If I can chime in, I think the ongoing lack of support for TransactionScope in SqlClient is going to make it difficult to embrace .NET Core. The usual suggestion for creating connections is that you create and open them as late as possible, then dispose of them as soon as possible, meaning that for a given business process, the work of writing to the database may very well be split across multiple connections. As I understand it, if TransactionScope is not available, the only option is to create a transaction from an existing connection, which leaves no simple way to guarantee transactional behavior when a process requires multiple writes to a database -- which in my case, and I would imagine for a lot of others, happens quite frequently. For my application, the only clear workaround -- using the same connection for multiple commands -- would force connection and transaction management up into the domain layer, increasing complexity and adding extra responsibilities. In other words, the lack of support for TransactionScope seems to make less-than-desirable design choices inevitable. I'm certainly not on expert on this subject, so I would definitely welcome any insight on this problem. There's more about my particular situation here: https://stackoverflow.com/questions/44163915/net-core-transaction-decorator-without-transactionscope |
|
@moleculomjd thanks for you information. Definitely valuable view point. |
|
cc @divega |
|
@moleculomjd ambient transactions aren't always a plus, as they create some overhead, and not every database supports them on the server side (afaik, only sqlserver does, and Oracle over MTS/COM+ with a separate service). An alternative can be to use a unit of work which collects the activity you have to perform for the business process, and then performs the actions quickly in a single ado.net transaction. Some ORMs supply a Unit of work class which is separated from persistence logic (EF does not btw). It isn't always a good alternative (e.g. when the business process is long with a lot of back/forth activity), but for multiple save actions in a pipeline of calls it might be. |
|
@karelz @moleculomjd @FransBouma Any discussion about inherent limitations of |
|
@NickCraver I am also facing the same issue |
ghost
locked as resolved and limited conversation to collaborators
I'm not sure if there are other issues beyond this, but hitting the first boom with SQL TVPs (table valued parameters) when attempting to restore support in Dapper with
netstandard2.0.In the reference source for
SqlParameter.CoerceValue(), there's an explicit check to not attempt conversion on aDataTable:Compared to the code in CoreFX:
Without this match, it falls down below resulting in:
This may be a single backporting oversight, or there may be deeper issues with
DataTableas a parameter beyond this. Filing because this is the wall I'm hitting at the moment.The text was updated successfully, but these errors were encountered: