Skip to content
Open
Show file tree
Hide file tree
Changes from 11 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -1293,7 +1293,7 @@ private SqlDataReader TryFetchInputParameterEncryptionInfo(
// not present as the rpcName, as is the case with non-_batchRPCMode. So
// input parameters start at parameters[1]. parameters[0] is the actual
// T-SQL Statement. rpcName is sp_executesql.
if (_RPCList[i].systemParams.Length > 1)
if (_RPCList[i].systemParams != null && _RPCList[i].systemParams.Length > 1)
{
_RPCList[i].needsFetchParameterEncryptionMetadata = true;

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,255 @@
// 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 sealed class SqlDataAdapterBatchUpdateTests : IClassFixture<SQLSetupStrategyCertStoreProvider>, IDisposable
{
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.
Comment on lines +24 to +26
Copy link

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment states "Provide table names to mirror repo patterns" but the test is creating its own hardcoded table names that don't exist in the fixture. This comment is misleading as it suggests the fixture provides these names, when it doesn't.

Consider updating the comment to clarify that these are test-specific table names that need to be created, or better yet, define these as constants or properties in the fixture.

Suggested change
// 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.
// The following are test-specific table and procedure names required for these tests.
// The fixture does not provide these names; they must be created and managed within the test.
// For better maintainability, consider defining these as constants or exposing them via the fixture.

Copilot uses AI. Check for mistakes.
tableNames["BuyerSeller"] = "BuyerSeller";
tableNames["ProcInsertBuyerSeller"] = "InsertBuyerSeller";
tableNames["ProcUpdateBuyerSeller"] = "UpdateBuyerSeller";
Comment on lines +27 to +29
Copy link

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The BuyerSeller table and associated stored procedures (InsertBuyerSeller, UpdateBuyerSeller) referenced in this test are not created by the SQLSetupStrategyCertStoreProvider fixture.

The fixture's CreateTables method (defined in the base SQLSetupStrategy class) does not include the creation of these database objects, which means these tests will fail when executed because the table and stored procedures don't exist in the test database.

To fix this, you should either:

  1. Add the BuyerSeller table and stored procedure creation to the SQLSetupStrategyCertStoreProvider.SetupDatabase() method or the base CreateTables() method, similar to how other test tables like ApiTestTable are created.
  2. Create a dedicated table class like BuyerSellerTable in the AlwaysEncrypted/TestFixtures/Setup/ directory that implements the ICreatable and IDroppable interfaces.

Copilot uses AI. Check for mistakes.
}

// ---------- TESTS ----------

[ConditionalTheory(typeof(DataTestUtility), nameof(DataTestUtility.IsTargetReadyForAeWithKeyStore))]
[ClassData(typeof(AEConnectionStringProvider))]
public async Task AdapterUpdate_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 - With batch updates (UpdateBatchSize > 1), this previously threw NullReferenceException due to null systemParams in batch RPC mode
var updated = await Task.Run(() => adapter.Update(dataTable));

// Assert
Assert.Equal(dataTable.Rows.Count, updated);

Comment on lines +58 to +63
Copy link

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test does not verify that the data was actually updated in the database after the adapter.Update() call. It only checks that the number of rows updated matches the expected count, but doesn't confirm that the values were correctly persisted.

Consider adding an assertion that queries the database after the update and verifies the actual values match the expected mutated values (e.g., using LoadCurrentRowsIntoDataTable on a fresh DataTable and comparing values).

Copilot uses AI. Check for mistakes.
}

[ConditionalTheory(typeof(DataTestUtility), nameof(DataTestUtility.IsTargetReadyForAeWithKeyStore))]
[ClassData(typeof(AEConnectionStringProvider))]
public async Task AdapterUpdate_BatchSizeOne_Succeeds(string connectionString)
{
// Arrange
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: 1); // success path
var dataTable = BuildBuyerSellerDataTable();
LoadCurrentRowsIntoDataTable(dataTable, conn);

MutateForUpdate(dataTable);

// Act (should not throw)
var updatedRows = await Task.Run(() => adapter.Update(dataTable));

// Assert
Assert.Equal(dataTable.Rows.Count, updatedRows);
Comment on lines +88 to +92
Copy link

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test does not verify that the data was actually updated in the database after the adapter.Update() call. It only checks that the number of rows updated matches the expected count, but doesn't confirm that the values were correctly persisted.

Consider adding an assertion that queries the database after the update and verifies the actual values match the expected mutated values.

Copilot uses AI. Check for mistakes.

}

// ---------- HELPERS ----------

private SqlDataAdapter CreateAdapter(SqlConnection connection, int updateBatchSize)
{
// Insert
var insertCmd = new SqlCommand(tableNames["ProcInsertBuyerSeller"], connection)
{
CommandType = CommandType.StoredProcedure
};
insertCmd.Parameters.AddRange(new[]
{
new SqlParameter("@BuyerSellerID", SqlDbType.Int) { SourceColumn = "BuyerSellerID" },
new SqlParameter("@SSN1", SqlDbType.VarChar, 255) { SourceColumn = "SSN1" },
new SqlParameter("@SSN2", SqlDbType.VarChar, 255) { SourceColumn = "SSN2" },
});
insertCmd.UpdatedRowSource = UpdateRowSource.None;

// Update
var updateCmd = new SqlCommand(tableNames["ProcUpdateBuyerSeller"], connection)
{
CommandType = CommandType.StoredProcedure
};
updateCmd.Parameters.AddRange(new[]
{
new SqlParameter("@BuyerSellerID", SqlDbType.Int) { SourceColumn = "BuyerSellerID" },
new SqlParameter("@SSN1", SqlDbType.VarChar, 255) { SourceColumn = "SSN1" },
new SqlParameter("@SSN2", SqlDbType.VarChar, 255) { SourceColumn = "SSN2" },
});
updateCmd.UpdatedRowSource = UpdateRowSource.None;

return new SqlDataAdapter
{
InsertCommand = insertCmd,
UpdateCommand = updateCmd,
UpdateBatchSize = updateBatchSize
};
}

private DataTable BuildBuyerSellerDataTable()
{
var dt = new DataTable(tableNames["BuyerSeller"]);
dt.Columns.AddRange(new[]
{
new DataColumn("BuyerSellerID", typeof(int)),
new DataColumn("SSN1", typeof(string)),
new DataColumn("SSN2", typeof(string)),
});
dt.PrimaryKey = new[] { dt.Columns["BuyerSellerID"] };
return dt;
}

private void LoadCurrentRowsIntoDataTable(DataTable dt, SqlConnection conn)
{
using var cmd = new SqlCommand($"SELECT BuyerSellerID, SSN1, SSN2 FROM [dbo].[{tableNames["BuyerSeller"]}] ORDER BY BuyerSellerID", conn);
using var reader = cmd.ExecuteReader();
while (reader.Read())
{
dt.Rows.Add(reader.GetInt32(0), reader.GetString(1), reader.GetString(2));
}
}

private void MutateForUpdate(DataTable dt)
{
int i = 0;
var fixedTime = new DateTime(2023, 01, 01, 12, 34, 56); // Use any fixed value
string timeStr = fixedTime.ToString("HHmm");
foreach (DataRow row in dt.Rows)
{
i++;
row["SSN1"] = $"{i:000}-11-{timeStr}";
row["SSN2"] = $"{i:000}-22-{timeStr}";
Comment on lines +160 to +166
Copy link

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The fixedTime variable is commented as "Use any fixed value" but this appears to be unnecessarily complex. The mutation logic only needs to generate unique values for each row, and the timestamp-based suffix adds unnecessary complexity.

Consider simplifying this to just use the row index or a simple counter to generate unique test values, e.g., row["SSN1"] = $"{i:000}-11-1234";

Suggested change
var fixedTime = new DateTime(2023, 01, 01, 12, 34, 56); // Use any fixed value
string timeStr = fixedTime.ToString("HHmm");
foreach (DataRow row in dt.Rows)
{
i++;
row["SSN1"] = $"{i:000}-11-{timeStr}";
row["SSN2"] = $"{i:000}-22-{timeStr}";
foreach (DataRow row in dt.Rows)
{
i++;
row["SSN1"] = $"{i:000}-11-1234";
row["SSN2"] = $"{i:000}-22-1234";

Copilot uses AI. Check for mistakes.
}
}

internal void TruncateTables(string tableName, string connectionString)
{
using var connection = new SqlConnection(GetOpenConnectionString(connectionString, encryptionEnabled: true));
connection.Open();
SilentRunCommand($@"TRUNCATE TABLE [dbo].[{tableNames[tableName]}]", connection);
}

internal void ExecuteQuery(SqlConnection connection, string commandText)
{
// Mirror AE-enabled command execution style used in repo tests
using var cmd = new SqlCommand(
commandText,
connection: connection,
transaction: null,
columnEncryptionSetting: SqlCommandColumnEncryptionSetting.Enabled);
cmd.ExecuteNonQuery();
}

internal void PopulateTable(string tableName, (int id, string s1, string s2)[] rows, string connectionString)
{
using var connection = new SqlConnection(GetOpenConnectionString(connectionString, encryptionEnabled: true));
connection.Open();

foreach (var (id, s1, s2) in rows)
{
using var cmd = new SqlCommand(
$@"INSERT INTO [dbo].[{tableNames[tableName]}] (BuyerSellerID, SSN1, SSN2) VALUES (@id, @s1, @s2)",
connection,
null,
SqlCommandColumnEncryptionSetting.Enabled);

cmd.Parameters.Add(new SqlParameter("@id", SqlDbType.Int) { Value = id });
cmd.Parameters.Add(new SqlParameter("@s1", SqlDbType.VarChar, 255) { Value = s1 });
cmd.Parameters.Add(new SqlParameter("@s2", SqlDbType.VarChar, 255) { Value = s2 });

cmd.ExecuteNonQuery();
}
}

public string GetOpenConnectionString(string baseConnectionString, bool encryptionEnabled)
{
var builder = new SqlConnectionStringBuilder(baseConnectionString)
{
// TrustServerCertificate can be set based on environment; mirror repo’s AE toggling idiom
ColumnEncryptionSetting = encryptionEnabled
? SqlConnectionColumnEncryptionSetting.Enabled
: SqlConnectionColumnEncryptionSetting.Disabled
};
return builder.ToString();
}

internal void SilentRunCommand(string commandText, SqlConnection connection)
{
try
{ ExecuteQuery(connection, commandText); }
catch (SqlException ex)
{
// Only swallow "object does not exist" (error 208), log others
bool onlyObjectNotExist = true;
foreach (SqlError err in ex.Errors)
{
if (err.Number != 208)
{
onlyObjectNotExist = false;
break;
}
}
if (!onlyObjectNotExist)
{
Console.WriteLine($"SilentRunCommand: Unexpected SqlException during cleanup: {ex}");
}
// Swallow all exceptions, but log unexpected ones
}
Comment on lines +227 to +242
Copy link

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment "Only swallow 'object does not exist' (error 208), log others" at line 227 is misleading. The code actually swallows all exceptions (line 242: "Swallow all exceptions, but log unexpected ones"), not just error 208. The check for error 208 only affects whether the exception is logged to console, but all exceptions are still caught and ignored.

Consider clarifying the comment to accurately reflect the behavior, or better yet, rethrow non-208 errors instead of just logging them to ensure unexpected cleanup failures are visible.

Copilot uses AI. Check for mistakes.
}

public void Dispose()
{
foreach (string connectionString in DataTestUtility.AEConnStringsSetup)
{
using var connection = new SqlConnection(GetOpenConnectionString(connectionString, encryptionEnabled: true));
connection.Open();
SilentRunCommand($"DELETE FROM [dbo].[{tableNames["BuyerSeller"]}]", connection);
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@
<OutputPath>$(BinFolder)$(Configuration).$(Platform).$(AssemblyName)</OutputPath>
<IsTestProject>true</IsTestProject>
</PropertyGroup>
<ItemGroup Condition="'$(TestSet)' == 'AE'">
<Compile Include="AlwaysEncrypted\SqlDataAdapterBatchUpdateTests.cs" />
</ItemGroup>
<ItemGroup Condition="'$(TestSet)' == '' or '$(TestSet)' == 'AE'">
<Compile Include="AlwaysEncrypted\ConversionTests.cs" />
<Compile Include="AlwaysEncrypted\ExceptionsGenericError.cs" />
Expand Down Expand Up @@ -266,6 +269,7 @@
<Compile Include="SQL\ConnectionPoolTest\ConnectionPoolTest.Debug.cs" />
</ItemGroup>
<ItemGroup>
<Compile Include="AlwaysEncrypted\SqlDataAdapterBatchUpdateTests.cs" />
<Compile Include="DataCommon\AADUtility.cs" />
<Compile Include="DataCommon\AssemblyResourceManager.cs" />
<Compile Include="DataCommon\ConnectionTestParameters.cs" />
Expand Down
Loading