From d3336e44a1f8359d0904b39b399cc6cb36675c0f Mon Sep 17 00:00:00 2001 From: Wraith2 Date: Fri, 2 Apr 2021 21:12:18 +0100 Subject: [PATCH 1/3] fix derived parameters containing typename incorrectly and add tests --- .../src/Microsoft/Data/Common/AdapterUtil.cs | 21 ++- .../Microsoft/Data/SqlClient/SqlCommand.cs | 48 ++++++- .../Microsoft/Data/SqlClient/SqlParameter.cs | 8 +- .../tests/ManualTests/SQL/UdtTest/UdtTest2.cs | 122 ++++++++++++++++++ tools/testsql/createUdtTestDb.sql | Bin 199880 -> 200366 bytes 5 files changed, 191 insertions(+), 8 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/netcore/src/Common/src/Microsoft/Data/Common/AdapterUtil.cs b/src/Microsoft.Data.SqlClient/netcore/src/Common/src/Microsoft/Data/Common/AdapterUtil.cs index 396d43caa6..19a9d2447b 100644 --- a/src/Microsoft.Data.SqlClient/netcore/src/Common/src/Microsoft/Data/Common/AdapterUtil.cs +++ b/src/Microsoft.Data.SqlClient/netcore/src/Common/src/Microsoft/Data/Common/AdapterUtil.cs @@ -379,24 +379,33 @@ internal static Exception StreamClosed([CallerMemberName] string method = "") internal static string BuildQuotedString(string quotePrefix, string quoteSuffix, string unQuotedString) { - var resultString = new StringBuilder(); + var resultString = new StringBuilder(unQuotedString.Length + quoteSuffix.Length + quoteSuffix.Length); + AppendQuotedString(resultString, quotePrefix, quoteSuffix, unQuotedString); + return resultString.ToString(); + } + + internal static string AppendQuotedString(StringBuilder buffer, string quotePrefix, string quoteSuffix, string unQuotedString) + { + if (!string.IsNullOrEmpty(quotePrefix)) { - resultString.Append(quotePrefix); + buffer.Append(quotePrefix); } // Assuming that the suffix is escaped by doubling it. i.e. foo"bar becomes "foo""bar". if (!string.IsNullOrEmpty(quoteSuffix)) { - resultString.Append(unQuotedString.Replace(quoteSuffix, quoteSuffix + quoteSuffix)); - resultString.Append(quoteSuffix); + int start = buffer.Length; + buffer.Append(unQuotedString); + buffer.Replace(quoteSuffix, quoteSuffix + quoteSuffix, start, unQuotedString.Length); + buffer.Append(quoteSuffix); } else { - resultString.Append(unQuotedString); + buffer.Append(unQuotedString); } - return resultString.ToString(); + return buffer.ToString(); } static internal string BuildMultiPartName(string[] strings) diff --git a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlCommand.cs b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlCommand.cs index 271f76ea4a..552768effb 100644 --- a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlCommand.cs +++ b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlCommand.cs @@ -3084,6 +3084,12 @@ internal void DeriveParameters() p.TypeName = r[colNames[(int)ProcParamsColIndex.TypeCatalogName]] + "." + r[colNames[(int)ProcParamsColIndex.TypeSchemaName]] + "." + r[colNames[(int)ProcParamsColIndex.TypeName]]; + + // the constructed type name above is incorrectly formatted, it should be a 2 part name not 3 + // for compatibility we can't change this because the bug has existed for a long time and been + // worked around by users, so identify that it is present and catch it later in the execution + // process once users can no longer interact with with the parameter type name + p.IsDerivedParameterTypeName = true; } // XmlSchema name for Xml types @@ -5534,6 +5540,23 @@ private void SetUpRPCParameters(_SqlRPC rpc, bool inSchema, SqlParameterCollecti { options |= TdsEnums.RPC_PARAM_DEFAULT; } + + // detect incorrectly derived type names unchanged by the caller and fix them + if (parameter.IsDerivedParameterTypeName) + { + string[] parts = MultipartIdentifier.ParseMultipartIdentifier(parameter.TypeName, "[\"", "]\"", Strings.SQL_TDSParserTableName, false); + if (parts != null && parts.Length == 4) // will always return int[4] right justified + { + if ( + parts[3] != null && // name must not be null + parts[2] != null && // schema must not be null + parts[1] != null // server should not be null or we don't need to remove it + ) + { + parameter.TypeName = QuoteIdentifier(parts.AsSpan(2, 2)); + } + } + } } rpc.userParamMap[userParamCount] = ((((long)options) << 32) | (long)index); @@ -5974,7 +5997,30 @@ internal string BuildParamList(TdsParser parser, SqlParameterCollection paramete private static string ParseAndQuoteIdentifier(string identifier, bool isUdtTypeName) { string[] strings = SqlParameter.ParseTypeName(identifier, isUdtTypeName); - return ADP.BuildMultiPartName(strings); + return QuoteIdentifier(strings); + } + + private static string QuoteIdentifier(ReadOnlySpan strings) + { + StringBuilder bld = new StringBuilder(); + + // Stitching back together is a little tricky. Assume we want to build a full multi-part name + // with all parts except trimming separators for leading empty names (null or empty strings, + // but not whitespace). Separators in the middle should be added, even if the name part is + // null/empty, to maintain proper location of the parts. + for (int i = 0; i < strings.Length; i++) + { + if (0 < bld.Length) + { + bld.Append('.'); + } + if (null != strings[i] && 0 != strings[i].Length) + { + ADP.AppendQuotedString(bld, "[", "]", strings[i]); + } + } + + return bld.ToString(); } // returns set option text to turn on format only and key info on and off diff --git a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlParameter.cs b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlParameter.cs index da1114468e..2bab7e4dbd 100644 --- a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlParameter.cs +++ b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlParameter.cs @@ -669,7 +669,11 @@ public string UdtTypeName public string TypeName { get => _typeName ?? ADP.StrEmpty; - set => _typeName = value; + set + { + _typeName = value; + IsDerivedParameterTypeName = false; + } } /// @@ -964,6 +968,8 @@ internal string ParameterNameFixed internal INullable ValueAsINullable => _valueAsINullable; + internal bool IsDerivedParameterTypeName { get; set; } + private void CloneHelper(SqlParameter destination) { // NOTE: _parent is not cloned diff --git a/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/UdtTest/UdtTest2.cs b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/UdtTest/UdtTest2.cs index f34dc2833a..098c5e7736 100644 --- a/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/UdtTest/UdtTest2.cs +++ b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/UdtTest/UdtTest2.cs @@ -588,6 +588,128 @@ Func create () => create(-2), errorMessage); } + + [ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.IsUdtTestDatabasePresent), nameof(DataTestUtility.AreConnStringsSetup))] + public void UDTParams_DeriveParameters_CheckAutoFixSuccess() + { + // the type and sproc must be commited to the database or this test will deadlock with a schema lock violation + // if you are missing these database entities then you should look for an updated version of the database creation script + + string sprocName = "sp_insert_customers"; + string typeName = "CustomerAddress"; + string customerAddressTypeIncorrectName = $"{DataTestUtility.UdtTestDbName}.dbo.{typeName.Trim('[', ']')}"; + string customerAddressTypeCorrectedName = $"[dbo].[{typeName.Trim('[', ']')}]"; + string customerParameterName = "@customers"; + + Address addr = Address.Parse("123 baker st || Redmond"); + DataTable table = new DataTable(); + table.Columns.Add(); + table.Columns.Add(); + table.Rows.Add("john", addr); + + using (SqlConnection connection = new SqlConnection(_connStr)) + { + connection.Open(); + using (SqlTransaction transaction = connection.BeginTransaction()) + using (SqlCommand cmd = new SqlCommand(sprocName, connection, transaction)) + { + try + { + cmd.CommandType = CommandType.StoredProcedure; + + SqlCommandBuilder.DeriveParameters(cmd); + + Assert.NotNull(cmd.Parameters); + Assert.Equal(2, cmd.Parameters.Count); // [return_value, table] + + SqlParameter p = cmd.Parameters[1]; + + Assert.Equal(customerParameterName, p.ParameterName); + Assert.Equal(SqlDbType.Structured, p.SqlDbType); + Assert.Equal(customerAddressTypeIncorrectName, p.TypeName); // the 3 part name is incorrect but needs to be maintained for compatibility + p.Value = table; + + cmd.ExecuteNonQuery(); + + Assert.Equal(customerAddressTypeCorrectedName, p.TypeName); // check that the auto fix has been applied correctly + } + finally + { + try + { + transaction.Rollback(); + } + catch + { + // ignore rollback failure exceptions to preserve original thrown error in test result + } + } + } + + } + } + + [ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.IsUdtTestDatabasePresent), nameof(DataTestUtility.AreConnStringsSetup))] + public void UDTParams_DeriveParameters_CheckAutoFixOverride() + { + // the type and sproc must be commited to the database or this test will deadlock with a schema lock violation + // if you are missing these database entities then you should look for an updated version of the database creation script + + string sprocName = "sp_insert_customers"; + string typeName = "CustomerAddress"; + string customerAddressTypeIncorrectName = $"{DataTestUtility.UdtTestDbName}.dbo.{typeName.Trim('[', ']')}"; + string customerAddressTypeCorrectedName = $"[dbo].[{typeName.Trim('[', ']')}]"; + string customerParameterName = "@customers"; + + Address addr = Address.Parse("123 baker st || Redmond"); + DataTable table = new DataTable(); + table.Columns.Add(); + table.Columns.Add(); + table.Rows.Add("john", addr); + + using (SqlConnection connection = new SqlConnection(_connStr)) + { + connection.Open(); + using (SqlTransaction transaction = connection.BeginTransaction()) + using (SqlCommand cmd = new SqlCommand(sprocName, connection, transaction)) + { + try + { + cmd.CommandType = CommandType.StoredProcedure; + + SqlCommandBuilder.DeriveParameters(cmd); + + Assert.NotNull(cmd.Parameters); + Assert.Equal(2, cmd.Parameters.Count); // [return_value, table] + + SqlParameter p = cmd.Parameters[1]; + + Assert.Equal(customerParameterName, p.ParameterName); + Assert.Equal(SqlDbType.Structured, p.SqlDbType); + Assert.Equal(customerAddressTypeIncorrectName, p.TypeName); // the 3 part name is incorrect but needs to be maintained for compatibility + p.Value = table; + + p.TypeName = customerAddressTypeIncorrectName; // force using the incorrect name by manually setting it + + Assert.Throws( + () => cmd.ExecuteNonQuery() + ); + } + finally + { + try + { + transaction.Rollback(); + } + catch + { + // ignore rollback failure exceptions to preserve original thrown error in test result + } + } + } + + } + } } } diff --git a/tools/testsql/createUdtTestDb.sql b/tools/testsql/createUdtTestDb.sql index 61052c75487154b3286afc8cbcf40033753108a2..5511f5a680c59982c58d5c8e188f06df16d7f86b 100644 GIT binary patch delta 337 zcmX>xlV{yro`x32EleH7(|HP+IED Y1#B*x-q^t;KRqv>QE2Pxo`x32EleH7(?75>b8Yu2VNw&{KCg#Kpa}rH?h8Kv From e0542227650f663d9a22b105fd7145b4200a7193 Mon Sep 17 00:00:00 2001 From: Wraith2 Date: Wed, 7 Apr 2021 01:47:03 +0100 Subject: [PATCH 2/3] remove layer of error handlering and test --- .../tests/ManualTests/SQL/UdtTest/UdtTest2.cs | 23 ++++--------------- 1 file changed, 4 insertions(+), 19 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/UdtTest/UdtTest2.cs b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/UdtTest/UdtTest2.cs index 098c5e7736..3437736adf 100644 --- a/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/UdtTest/UdtTest2.cs +++ b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/UdtTest/UdtTest2.cs @@ -635,17 +635,9 @@ public void UDTParams_DeriveParameters_CheckAutoFixSuccess() } finally { - try - { - transaction.Rollback(); - } - catch - { - // ignore rollback failure exceptions to preserve original thrown error in test result - } + transaction.Rollback(); } } - } } @@ -691,23 +683,16 @@ public void UDTParams_DeriveParameters_CheckAutoFixOverride() p.TypeName = customerAddressTypeIncorrectName; // force using the incorrect name by manually setting it - Assert.Throws( + SqlException exception = Assert.Throws( () => cmd.ExecuteNonQuery() ); + Assert.Contains("Database name is not allowed", exception.Message); } finally { - try - { - transaction.Rollback(); - } - catch - { - // ignore rollback failure exceptions to preserve original thrown error in test result - } + transaction.Rollback(); } } - } } } From 78fda4ef8396c1e3bbe56c507d53156c251125c7 Mon Sep 17 00:00:00 2001 From: Wraith2 Date: Fri, 9 Apr 2021 09:23:47 +0100 Subject: [PATCH 3/3] implement netfx version --- .../src/Microsoft/Data/Common/AdapterUtil.cs | 26 ++++++---- .../Microsoft/Data/SqlClient/SqlCommand.cs | 47 +++++++++++++++++++ .../Microsoft/Data/SqlClient/SqlParameter.cs | 8 +++- 3 files changed, 71 insertions(+), 10 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/Common/AdapterUtil.cs b/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/Common/AdapterUtil.cs index 661f49dd60..eba346f34a 100644 --- a/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/Common/AdapterUtil.cs +++ b/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/Common/AdapterUtil.cs @@ -2334,26 +2334,34 @@ static internal string MachineName() return Environment.MachineName; } - static internal string BuildQuotedString(string quotePrefix, string quoteSuffix, string unQuotedString) + internal static string BuildQuotedString(string quotePrefix, string quoteSuffix, string unQuotedString) { - StringBuilder resultString = new StringBuilder(); - if (ADP.IsEmpty(quotePrefix) == false) + var resultString = new StringBuilder(unQuotedString.Length + quoteSuffix.Length + quoteSuffix.Length); + AppendQuotedString(resultString, quotePrefix, quoteSuffix, unQuotedString); + return resultString.ToString(); + } + + internal static string AppendQuotedString(StringBuilder buffer, string quotePrefix, string quoteSuffix, string unQuotedString) + { + if (!string.IsNullOrEmpty(quotePrefix)) { - resultString.Append(quotePrefix); + buffer.Append(quotePrefix); } // Assuming that the suffix is escaped by doubling it. i.e. foo"bar becomes "foo""bar". - if (ADP.IsEmpty(quoteSuffix) == false) + if (!string.IsNullOrEmpty(quoteSuffix)) { - resultString.Append(unQuotedString.Replace(quoteSuffix, quoteSuffix + quoteSuffix)); - resultString.Append(quoteSuffix); + int start = buffer.Length; + buffer.Append(unQuotedString); + buffer.Replace(quoteSuffix, quoteSuffix + quoteSuffix, start, unQuotedString.Length); + buffer.Append(quoteSuffix); } else { - resultString.Append(unQuotedString); + buffer.Append(unQuotedString); } - return resultString.ToString(); + return buffer.ToString(); } static internal string BuildMultiPartName(string[] strings) diff --git a/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlCommand.cs b/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlCommand.cs index e276929dff..aa054ae5a1 100644 --- a/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlCommand.cs +++ b/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlCommand.cs @@ -19,6 +19,7 @@ using System.Threading; using System.Threading.Tasks; using System.Xml; +using System.Buffers; using Microsoft.Data.Common; using Microsoft.Data.Sql; using Microsoft.Data.SqlClient.Server; @@ -3538,6 +3539,12 @@ internal void DeriveParameters() p.TypeName = r[colNames[(int)ProcParamsColIndex.TypeCatalogName]] + "." + r[colNames[(int)ProcParamsColIndex.TypeSchemaName]] + "." + r[colNames[(int)ProcParamsColIndex.TypeName]]; + + // the constructed type name above is incorrectly formatted, it should be a 2 part name not 3 + // for compatibility we can't change this because the bug has existed for a long time and been + // worked around by users, so identify that it is present and catch it later in the execution + // process once users can no longer interact with with the parameter type name + p.IsDerivedParameterTypeName = true; } // XmlSchema name for Xml types @@ -6464,6 +6471,23 @@ private void SetUpRPCParameters(_SqlRPC rpc, int startCount, bool inSchema, SqlP { rpc.paramoptions[j] |= TdsEnums.RPC_PARAM_DEFAULT; } + + // detect incorrectly derived type names unchanged by the caller and fix them + if (parameter.IsDerivedParameterTypeName) + { + string[] parts = MultipartIdentifier.ParseMultipartIdentifier(parameter.TypeName, "[\"", "]\"", Strings.SQL_TDSParserTableName, false); + if (parts != null && parts.Length == 4) // will always return int[4] right justified + { + if ( + parts[3] != null && // name must not be null + parts[2] != null && // schema must not be null + parts[1] != null // server should not be null or we don't need to remove it + ) + { + parameter.TypeName = QuoteIdentifier(parts, 2, 2); + } + } + } } // Must set parameter option bit for LOB_COOKIE if unfilled LazyMat blob @@ -6937,6 +6961,29 @@ private string ParseAndQuoteIdentifier(string identifier, bool isUdtTypeName) return ADP.BuildMultiPartName(strings); } + private static string QuoteIdentifier(string[] strings, int offset, int length) + { + StringBuilder bld = new StringBuilder(); + + // Stitching back together is a little tricky. Assume we want to build a full multi-part name + // with all parts except trimming separators for leading empty names (null or empty strings, + // but not whitespace). Separators in the middle should be added, even if the name part is + // null/empty, to maintain proper location of the parts. + for (int i = offset; i < (offset + length); i++) + { + if (0 < bld.Length) + { + bld.Append('.'); + } + if (null != strings[i] && 0 != strings[i].Length) + { + ADP.AppendQuotedString(bld, "[", "]", strings[i]); + } + } + + return bld.ToString(); + } + // returns set option text to turn on format only and key info on and off // When we are executing as a text command, then we never need // to turn off the options since they command text is executed in the scope of sp_executesql. diff --git a/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlParameter.cs b/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlParameter.cs index 231a635dfe..26174a9b67 100644 --- a/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlParameter.cs +++ b/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlParameter.cs @@ -656,7 +656,11 @@ public string UdtTypeName public string TypeName { get => _typeName ?? ADP.StrEmpty; - set => _typeName = value; + set + { + _typeName = value; + IsDerivedParameterTypeName = false; + } } /// @@ -955,6 +959,8 @@ internal string ParameterNameFixed internal INullable ValueAsINullable => _valueAsINullable; + internal bool IsDerivedParameterTypeName { get; set; } + private void CloneHelper(SqlParameter destination) { // NOTE: _parent is not cloned