dotnet / SqlClient

Microsoft.Data.SqlClient provides database connectivity to SQL Server for .NET applications.
MIT License
853 stars 286 forks source link

SqlBulkCopy throws inconsistent exceptions depending on the target column datatype. #1274

Open mburbea opened 3 years ago

mburbea commented 3 years ago

Describe the bug

SqlBulkCopy throws inconsistent exceptions depending on the target datatype. Not only are the messages different, but the exception types are different as well. When the target datatype is nvarchar for truncation issues you get a InvalidOperationException, but if the type is varchar you get a completely unhelpful SqlException.

varchar

``` Microsoft.Data.SqlClient.SqlException (0x80131904): Received an invalid column length from the bcp client for colid 1. at Microsoft.Data.SqlClient.SqlConnection.OnError(SqlException exception, Boolean breakConnection, Action`1 wrapCloseInAction) at Microsoft.Data.SqlClient.SqlInternalConnection.OnError(SqlException exception, Boolean breakConnection, Action`1 wrapCloseInAction) at Microsoft.Data.SqlClient.TdsParser.ThrowExceptionAndWarning(TdsParserStateObject stateObj, Boolean callerHasConnectionLock, Boolean asyncClose) at Microsoft.Data.SqlClient.TdsParser.TryRun(RunBehavior runBehavior, SqlCommand cmdHandler, SqlDataReader dataStream, BulkCopySimpleResultSet bulkCopyHandler, TdsParserStateObject stateObj, Boolean& dataReady) at Microsoft.Data.SqlClient.TdsParser.Run(RunBehavior runBehavior, SqlCommand cmdHandler, SqlDataReader dataStream, BulkCopySimpleResultSet bulkCopyHandler, TdsParserStateObject stateObj) at Microsoft.Data.SqlClient.SqlBulkCopy.RunParser(BulkCopySimpleResultSet bulkCopyHandler) at Microsoft.Data.SqlClient.SqlBulkCopy.CopyBatchesAsyncContinuedOnSuccess(BulkCopySimpleResultSet internalResults, String updateBulkCommandText, CancellationToken cts, TaskCompletionSource`1 source) at Microsoft.Data.SqlClient.SqlBulkCopy.CopyBatchesAsyncContinued(BulkCopySimpleResultSet internalResults, String updateBulkCommandText, CancellationToken cts, TaskCompletionSource`1 source) at Microsoft.Data.SqlClient.SqlBulkCopy.CopyBatchesAsync(BulkCopySimpleResultSet internalResults, String updateBulkCommandText, CancellationToken cts, TaskCompletionSource`1 source) at Microsoft.Data.SqlClient.SqlBulkCopy.WriteToServerInternalRestContinuedAsync(BulkCopySimpleResultSet internalResults, CancellationToken cts, TaskCompletionSource`1 source) at Microsoft.Data.SqlClient.SqlBulkCopy.WriteToServerInternalRestAsync(CancellationToken cts, TaskCompletionSource`1 source) at Microsoft.Data.SqlClient.SqlBulkCopy.WriteToServerInternalAsync(CancellationToken ctoken) at Microsoft.Data.SqlClient.SqlBulkCopy.WriteRowSourceToServerAsync(Int32 columnCount, CancellationToken ctoken) at Microsoft.Data.SqlClient.SqlBulkCopy.WriteToServer(DataTable table, DataRowState rowState) at Microsoft.Data.SqlClient.SqlBulkCopy.WriteToServer(DataTable table) at Program.Main() in C:\Users\Michael\source\repos\ConsoleApp1\ConsoleApp1\Program.cs:line 27 ```

nvarchar

``` System.InvalidOperationException: The given value '12' of type String from the data source cannot be converted to type nvarchar for Column 1 [b] Row 1. ---> System.InvalidOperationException: String or binary data would be truncated in table '#t', column 'b'. Truncated value: '1'. at Microsoft.Data.SqlClient.SqlBulkCopy.ConvertValue(Object value, _SqlMetaData metadata, Boolean isNull, Boolean& isSqlType, Boolean& coercedToDataFeed) --- End of inner exception stack trace --- at Microsoft.Data.SqlClient.SqlBulkCopy.ConvertValue(Object value, _SqlMetaData metadata, Boolean isNull, Boolean& isSqlType, Boolean& coercedToDataFeed) at Microsoft.Data.SqlClient.SqlBulkCopy.ReadWriteColumnValueAsync(Int32 col) at Microsoft.Data.SqlClient.SqlBulkCopy.CopyColumnsAsync(Int32 col, TaskCompletionSource`1 source) at Microsoft.Data.SqlClient.SqlBulkCopy.CopyRowsAsync(Int32 rowsSoFar, Int32 totalRows, CancellationToken cts, TaskCompletionSource`1 source) at Microsoft.Data.SqlClient.SqlBulkCopy.CopyBatchesAsyncContinued(BulkCopySimpleResultSet internalResults, String updateBulkCommandText, CancellationToken cts, TaskCompletionSource`1 source) at Microsoft.Data.SqlClient.SqlBulkCopy.CopyBatchesAsync(BulkCopySimpleResultSet internalResults, String updateBulkCommandText, CancellationToken cts, TaskCompletionSource`1 source) at Microsoft.Data.SqlClient.SqlBulkCopy.WriteToServerInternalRestContinuedAsync(BulkCopySimpleResultSet internalResults, CancellationToken cts, TaskCompletionSource`1 source) at Microsoft.Data.SqlClient.SqlBulkCopy.WriteToServerInternalRestAsync(CancellationToken cts, TaskCompletionSource`1 source) at Microsoft.Data.SqlClient.SqlBulkCopy.WriteToServerInternalAsync(CancellationToken ctoken) at Microsoft.Data.SqlClient.SqlBulkCopy.WriteRowSourceToServerAsync(Int32 columnCount, CancellationToken ctoken) at Microsoft.Data.SqlClient.SqlBulkCopy.WriteToServer(DataTable table, DataRowState rowState) at Microsoft.Data.SqlClient.SqlBulkCopy.WriteToServer(DataTable table) at Program.Main() in C:\Users\Michael\source\repos\ConsoleApp1\ConsoleApp1\Program.cs:line 38 ```

This repro is rather minimal, but in my usual use case I'm dealing with ETLs of dozens of columns. Trying to figure out which column is the offending one that is being truncated and why is time consuming and it is much easier when I know the actual datatype and how long the string is. (Usually I don't get row number as sqlbulkcopy only gives you that for DataTable sources. Although, I really think it would be a nice addition for DbDataReader sources).

Repro ```cs using System.Data; using Microsoft.Data.SqlClient; public class Program { public static void Main() { var localdb = @"Data Source=(localDb)\.;Initial Catalog =Master;Integrated Security=true;"; using var table = new DataTable { Columns = { new DataColumn("a", typeof(string)) }, Rows = { "12" } }; using var conn = new SqlConnection(localdb); conn.Open(); using var cmd = new SqlCommand("create table #t(a varchar(1), b nvarchar(1))", conn); cmd.ExecuteNonQuery(); using var bulkCopy = new SqlBulkCopy(conn) { DestinationTableName = "#t", ColumnMappings = { new("a", "a") }, }; try { bulkCopy.WriteToServer(table); } catch(SqlException e) { Console.WriteLine($"varchar:"); Console.WriteLine(e); } try { bulkCopy.ColumnMappings.Clear(); bulkCopy.ColumnMappings.Add(new("a", "b")); bulkCopy.WriteToServer(table); } catch(InvalidOperationException e) { Console.WriteLine($"nvarchar:"); Console.WriteLine(e); } } } ```

Expected behavior

I would expect the error message would be the same datatype and have the same exact error message modulo the part where it says "nvarchar" replaced with "varchar".

Further technical details

Microsoft.Data.SqlClient version: 3.0.0 .NET target: 5.0.4 SQL Server version: Sql Server 2019 localdb (but irrelevant) Operating system: Windows 10 21H1

JRahnama commented 3 years ago

@mburbea the exceptions are indeed different. When varchar is passed SqlException is thrown from the server, which I think is the correct way to do so, as Microsoft.Data.SqlClient.SqlException (0x80131904): Received an invalid column length from the bcp client for colid 1. However when nvarchar is passed the driver tests for string length and throws InvalidOperationException which in my opinion is wrong and the error message is misleading as well.

The idea that this check has been implemented in the driver comes from the fact that nvarchar and nchar with size n are taking two times n plus 2 bytes of space for storage. We will talk internally and get back to you soon.

JRahnama commented 3 years ago

for those who want to follow the issue happens at SqlBulkCopy.ConvertValue at

                    case TdsEnums.SQLNCHAR:
                    case TdsEnums.SQLNVARCHAR:
                    case TdsEnums.SQLNTEXT:
                        mt = MetaType.GetMetaTypeFromSqlDbType(type.SqlDbType, false);
                        value = SqlParameter.CoerceValue(value, mt, out coercedToDataFeed, out typeChanged, false);
                        if (!coercedToDataFeed)
                        {   // We do not need to test for TextDataFeed as it is only assigned to (N)VARCHAR(MAX)
                            string str = ((isSqlType) && (!typeChanged)) ? ((SqlString)value).Value : ((string)value);
                            int maxStringLength = length / 2;
                            if (str.Length > maxStringLength)
                            {
                                if (metadata.isEncrypted)
                                {
                                    str = "<encrypted>";
                                }
                                else
                                {
                                    // We truncate to at most 100 characters to match SQL Servers behavior as described in
                                    // https://blogs.msdn.microsoft.com/sql_server_team/string-or-binary-data-would-be-truncated-replacing-the-infamous-error-8152/
                                    str = str.Remove(Math.Min(maxStringLength, 100));
                                }
                                throw SQL.BulkLoadStringTooLong(_destinationTableName, metadata.column, str);
                            }

Also the mentioned link to MS Document in the code comments is deprecated and not working.

mburbea commented 3 years ago

I think thats not correct. The article is still here on the new blog, and t-sql does indeed show the better error message on sql server 2017 if you turn on the traceflag 460. (on by default in 2019 iirc).

Here is the correct link for that blog post: https://techcommunity.microsoft.com/t5/sql-server/string-or-binary-data-would-be-truncated-replacing-the-infamous/ba-p/386014

create table #t(a varchar(1));
go
insert into #t(a) select '12'

yields this error:

Msg 2628 Level 16 State 1 Line 1
String or binary data would be truncated in table 'tempdb.dbo.#t_..._00000000001F', column 'a'. Truncated value: '1'.
Msg 3621 Level 0 State 0 Line 1
The statement has been terminated.
JRahnama commented 3 years ago

@mburbea what part is not correct? The part that driver checks the string length or the part that server should throw the exception in this case not the driver?

JRahnama commented 3 years ago

If you meant MS document you are right. what I meant was the deprecated link. I have fixed that. thanks.