-
Notifications
You must be signed in to change notification settings - Fork 317
Fixing NullReferenceException issue with SqlDataAdapter #3749
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,228 @@ | ||||||||||||||||||||||||||||||||||||||||||
| // Licensed to the .NET Foundation under one or more agreements. | ||||||||||||||||||||||||||||||||||||||||||
| // The .NET Foundation licenses this file to you under the MIT license. | ||||||||||||||||||||||||||||||||||||||||||
| // See the LICENSE file in the project root for more information. | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| using System; | ||||||||||||||||||||||||||||||||||||||||||
| using System.Data; | ||||||||||||||||||||||||||||||||||||||||||
| using System.Threading.Tasks; | ||||||||||||||||||||||||||||||||||||||||||
| using System.Collections.Generic; | ||||||||||||||||||||||||||||||||||||||||||
| using Microsoft.Data.SqlClient; | ||||||||||||||||||||||||||||||||||||||||||
| using Microsoft.Data.SqlClient.ManualTesting.Tests.AlwaysEncrypted.Setup; | ||||||||||||||||||||||||||||||||||||||||||
| using Xunit; | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| namespace Microsoft.Data.SqlClient.ManualTesting.Tests.AlwaysEncrypted | ||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||
| public class SqlDataAdapterBatchUpdateTests : IClassFixture<SQLSetupStrategyCertStoreProvider> | ||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||
| private readonly SQLSetupStrategy _fixture; | ||||||||||||||||||||||||||||||||||||||||||
| private readonly Dictionary<string, string> tableNames = new(); | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| public SqlDataAdapterBatchUpdateTests(SQLSetupStrategyCertStoreProvider context) | ||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||
| _fixture = context; | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| // Provide table names to mirror repo patterns. | ||||||||||||||||||||||||||||||||||||||||||
| // If your fixture already exposes specific names for BuyerSeller and procs, wire them here. | ||||||||||||||||||||||||||||||||||||||||||
| // Otherwise use literal names as below. | ||||||||||||||||||||||||||||||||||||||||||
| tableNames["BuyerSeller"] = "BuyerSeller"; | ||||||||||||||||||||||||||||||||||||||||||
| tableNames["ProcInsertBuyerSeller"] = "InsertBuyerSeller"; | ||||||||||||||||||||||||||||||||||||||||||
| tableNames["ProcUpdateBuyerSeller"] = "UpdateBuyerSeller"; | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| // ---------- TESTS ---------- | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| [ConditionalTheory(typeof(DataTestUtility), nameof(DataTestUtility.IsTargetReadyForAeWithKeyStore))] | ||||||||||||||||||||||||||||||||||||||||||
| [ClassData(typeof(AEConnectionStringProvider))] | ||||||||||||||||||||||||||||||||||||||||||
| public async Task dapterUpdate_BatchSizeGreaterThanOne_Succeeds(string connectionString) | ||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||
| // Arrange | ||||||||||||||||||||||||||||||||||||||||||
| // Ensure baseline rows exist | ||||||||||||||||||||||||||||||||||||||||||
| TruncateTables("BuyerSeller", connectionString); | ||||||||||||||||||||||||||||||||||||||||||
| PopulateTable("BuyerSeller", new (int id, string s1, string s2)[] { | ||||||||||||||||||||||||||||||||||||||||||
| (1, "123-45-6789", "987-65-4321"), | ||||||||||||||||||||||||||||||||||||||||||
| (2, "234-56-7890", "876-54-3210"), | ||||||||||||||||||||||||||||||||||||||||||
| (3, "345-67-8901", "765-43-2109"), | ||||||||||||||||||||||||||||||||||||||||||
| (4, "456-78-9012", "654-32-1098"), | ||||||||||||||||||||||||||||||||||||||||||
| }, connectionString); | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| using var conn = new SqlConnection(GetOpenConnectionString(connectionString, encryptionEnabled: true)); | ||||||||||||||||||||||||||||||||||||||||||
| await conn.OpenAsync(); | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| using var adapter = CreateAdapter(conn, updateBatchSize: 10); // failure repro: > 1 | ||||||||||||||||||||||||||||||||||||||||||
| var dataTable = BuildBuyerSellerDataTable(); | ||||||||||||||||||||||||||||||||||||||||||
| LoadCurrentRowsIntoDataTable(dataTable, conn); | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| // Mutate values for update | ||||||||||||||||||||||||||||||||||||||||||
| MutateForUpdate(dataTable); | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| // Act - This is where NullReferenceException was being thrown previously(which is now fixed) | ||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
| // Act - This is where NullReferenceException was being thrown previously(which is now fixed) | |
| // Act - This is where NullReferenceException was being thrown previously (which is now fixed) |
Copilot
AI
Nov 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Disposable 'DataColumn' is created but not disposed.
Copilot
AI
Nov 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Disposable 'DataColumn' is created but not disposed.
Copilot
AI
Nov 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Disposable 'DataColumn' is created but not disposed.
Copilot
AI
Nov 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential SQL injection vulnerability: The test data values are directly concatenated into the INSERT statement without parameterization. While this is test code and values are controlled, it violates security best practices and could cause issues if the test data contains quotes or special characters. Consider using parameterized queries even in test code to follow best practices and avoid potential issues.
| ExecuteQuery(connection, | |
| $@"INSERT INTO [dbo].[{tableNames[tableName]}] (BuyerSellerID, SSN1, SSN2) VALUES ({id}, '{s1}', '{s2}')"); | |
| } | |
| } | |
| ExecuteParameterizedInsert(connection, tableNames[tableName], id, s1, s2); | |
| } | |
| } | |
| private void ExecuteParameterizedInsert(SqlConnection connection, string tableName, int id, string s1, string s2) | |
| { | |
| using var cmd = new SqlCommand( | |
| $@"INSERT INTO [dbo].[{tableName}] (BuyerSellerID, SSN1, SSN2) VALUES (@id, @s1, @s2)", | |
| connection, | |
| transaction: null, | |
| columnEncryptionSetting: SqlCommandColumnEncryptionSetting.Enabled); | |
| cmd.Parameters.Add(new SqlParameter("@id", SqlDbType.Int) { Value = id }); | |
| cmd.Parameters.Add(new SqlParameter("@s1", SqlDbType.NVarChar, 50) { Value = s1 ?? (object)DBNull.Value }); | |
| cmd.Parameters.Add(new SqlParameter("@s2", SqlDbType.NVarChar, 50) { Value = s2 ?? (object)DBNull.Value }); | |
| cmd.ExecuteNonQuery(); | |
| } |
Copilot
AI
Nov 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test class does not implement IDisposable. The Dispose() method on line 217 will not be called automatically by xUnit since the class doesn't implement IDisposable interface. Either implement IDisposable or remove the Dispose method if cleanup is handled by the fixture.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -277,6 +277,7 @@ | |
| <Compile Include="SQL\ConnectionPoolTest\ConnectionPoolTest.Debug.cs" /> | ||
| </ItemGroup> | ||
| <ItemGroup> | ||
| <Compile Include="AlwaysEncrypted\SqlDataAdapterBatchUpdateTests.cs" /> | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you have dependency on AE related setup, consider including this in |
||
| <Compile Include="DataCommon\AADUtility.cs" /> | ||
| <Compile Include="DataCommon\AssemblyResourceManager.cs" /> | ||
| <Compile Include="DataCommon\ConnectionTestParameters.cs" /> | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.