-
Notifications
You must be signed in to change notification settings - Fork 316
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?
Conversation
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.
Pull Request Overview
This pull request adds a null check for systemParams before accessing its Length property in the parameter encryption metadata retrieval logic. This defensive programming change prevents a potential NullReferenceException when processing RPC batch commands.
Key Changes
- Added null check for
_RPCList[i].systemParamsbefore accessing itsLengthproperty inTryFetchInputParameterEncryptionInfomethod
Comments suppressed due to low confidence (1)
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlCommand.Encryption.cs:1296
- The added null check for
systemParamslacks test coverage. Based on the code analysis,systemParamsis initialized throughGetRPCObject()which always allocates and initializes the array whensystemParamCount > 0. However, a newly constructed_SqlRPCobject (as seen inAddBatchCommandline 21) hassystemParamsinitially null. A test case should be added that exercises the batch RPC mode encryption path with an RPC object that has not yet had itssystemParamsinitialized to verify this defensive check works correctly.
if (_RPCList[i].systemParams != null && _RPCList[i].systemParams.Length > 1)
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlCommand.Encryption.cs
Show resolved
Hide resolved
paulmedynski
left a comment
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.
Need some tests for this.
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.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
...crosoft.Data.SqlClient/tests/ManualTests/Microsoft.Data.SqlClient.ManualTesting.Tests.csproj
Outdated
Show resolved
Hide resolved
...Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/SqlDataAdapterBatchUpdateTests.cs
Show resolved
Hide resolved
...Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/SqlDataAdapterBatchUpdateTests.cs
Show resolved
Hide resolved
...Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/SqlDataAdapterBatchUpdateTests.cs
Show resolved
Hide resolved
apoorvdeshmukh
left a comment
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.
There are build failures while compiling the tests.
...crosoft.Data.SqlClient/tests/ManualTests/Microsoft.Data.SqlClient.ManualTesting.Tests.csproj
Outdated
Show resolved
Hide resolved
…SqlClient.ManualTesting.Tests.csproj Co-authored-by: Copilot <[email protected]>
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.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
...Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/SqlDataAdapterBatchUpdateTests.cs
Show resolved
Hide resolved
| // Mutate values for update | ||
| MutateForUpdate(dataTable); | ||
|
|
||
| // 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.
Missing space after 'previously' in the comment.
| // 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) |
...Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/SqlDataAdapterBatchUpdateTests.cs
Outdated
Show resolved
Hide resolved
…dotnet/SqlClient into dev/prtiwar/SqlDataAdapterIssue
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.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
...Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/SqlDataAdapterBatchUpdateTests.cs
Show resolved
Hide resolved
...Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/SqlDataAdapterBatchUpdateTests.cs
Outdated
Show resolved
Hide resolved
...crosoft.Data.SqlClient/tests/ManualTests/Microsoft.Data.SqlClient.ManualTesting.Tests.csproj
Outdated
Show resolved
Hide resolved
...Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/SqlDataAdapterBatchUpdateTests.cs
Show resolved
Hide resolved
...Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/SqlDataAdapterBatchUpdateTests.cs
Outdated
Show resolved
Hide resolved
…/SqlDataAdapterBatchUpdateTests.cs Co-authored-by: Copilot <[email protected]>
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.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
...crosoft.Data.SqlClient/tests/ManualTests/Microsoft.Data.SqlClient.ManualTesting.Tests.csproj
Show resolved
Hide resolved
...Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/SqlDataAdapterBatchUpdateTests.cs
Outdated
Show resolved
Hide resolved
…/SqlDataAdapterBatchUpdateTests.cs Co-authored-by: Copilot <[email protected]>
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.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
...crosoft.Data.SqlClient/tests/ManualTests/Microsoft.Data.SqlClient.ManualTesting.Tests.csproj
Outdated
Show resolved
Hide resolved
...Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/SqlDataAdapterBatchUpdateTests.cs
Show resolved
Hide resolved
...Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/SqlDataAdapterBatchUpdateTests.cs
Show resolved
Hide resolved
...Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/SqlDataAdapterBatchUpdateTests.cs
Show resolved
Hide resolved
...Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/SqlDataAdapterBatchUpdateTests.cs
Show resolved
Hide resolved
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.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| { | ||
| using var connection = new SqlConnection(GetOpenConnectionString(connectionString, encryptionEnabled: true)); | ||
| connection.Open(); | ||
| SilentRunCommand($@"TRUNCATE TABLE [dbo].[{SqlCommandHelper.EscapeIdentifier(tableNames[tableName])}]", connection); |
Copilot
AI
Nov 19, 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 SqlCommandHelper class does not exist in the codebase. The correct class to use is SqlServerEscapeHelper which is available in the Microsoft.Data.SqlClient namespace.
Replace SqlCommandHelper.EscapeIdentifier with SqlServerEscapeHelper.EscapeIdentifier.
| foreach (var (id, s1, s2) in rows) | ||
| { | ||
| using var cmd = new SqlCommand( | ||
| $@"INSERT INTO [dbo].[{SqlCommandHelper.EscapeIdentifier(tableNames[tableName])}] (BuyerSellerID, SSN1, SSN2) VALUES (@id, @s1, @s2)", |
Copilot
AI
Nov 19, 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 SqlCommandHelper class does not exist in the codebase. The correct class to use is SqlServerEscapeHelper which is available in the Microsoft.Data.SqlClient namespace.
Replace SqlCommandHelper.EscapeIdentifier with SqlServerEscapeHelper.EscapeIdentifier.
| { | ||
| using var connection = new SqlConnection(GetOpenConnectionString(connectionString, encryptionEnabled: true)); | ||
| connection.Open(); | ||
| SilentRunCommand($"DELETE FROM [dbo].[{SqlCommandHelper.EscapeIdentifier(tableNames["BuyerSeller"])}]", connection); |
Copilot
AI
Nov 19, 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 SqlCommandHelper class does not exist in the codebase. The correct class to use is SqlServerEscapeHelper which is available in the Microsoft.Data.SqlClient namespace.
Replace SqlCommandHelper.EscapeIdentifier with SqlServerEscapeHelper.EscapeIdentifier.
...Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/SqlDataAdapterBatchUpdateTests.cs
Show resolved
Hide resolved
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.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
...crosoft.Data.SqlClient/tests/ManualTests/Microsoft.Data.SqlClient.ManualTesting.Tests.csproj
Outdated
Show resolved
Hide resolved
|
@priyankatiwari08 please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement ( “Agreement” ) is agreed to by the party signing below ( “You” ), 1. Definitions. “Code” means the computer software code, whether in human-readable or machine-executable form, “Project” means any of the projects owned or managed by .NET Foundation and offered under a license “Submit” is the act of uploading, submitting, transmitting, or distributing code or other content to any “Submission” means the Code and any other copyrightable material Submitted by You, including any 2. Your Submission. You must agree to the terms of this Agreement before making a Submission to any 3. Originality of Work. You represent that each of Your Submissions is entirely Your 4. Your Employer. References to “employer” in this Agreement include Your employer or anyone else 5. Licenses. a. Copyright License. You grant .NET Foundation, and those who receive the Submission directly b. Patent License. You grant .NET Foundation, and those who receive the Submission directly or c. Other Rights Reserved. Each party reserves all rights not expressly granted in this Agreement. 6. Representations and Warranties. You represent that You are legally entitled to grant the above 7. Notice to .NET Foundation. You agree to notify .NET Foundation in writing of any facts or 8. Information about Submissions. You agree that contributions to Projects and information about 9. Governing Law/Jurisdiction. This Agreement is governed by the laws of the State of Washington, and 10. Entire Agreement/Assignment. This Agreement is the entire agreement between the parties, and .NET Foundation dedicates this Contribution License Agreement to the public domain according to the Creative Commons CC0 1. |
…apterIssue r Please enter a commit message to explain why this merge is necessary, Merging with main
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.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| // 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. |
Copilot
AI
Dec 2, 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 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.
| // 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. |
| // 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); | ||
|
|
Copilot
AI
Dec 2, 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 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).
| // Act (should not throw) | ||
| var updatedRows = await Task.Run(() => adapter.Update(dataTable)); | ||
|
|
||
| // Assert | ||
| Assert.Equal(dataTable.Rows.Count, updatedRows); |
Copilot
AI
Dec 2, 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 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.
| 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}"; |
Copilot
AI
Dec 2, 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.
[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";
| 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"; |
| // 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 | ||
| } |
Copilot
AI
Dec 2, 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 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.
| tableNames["BuyerSeller"] = "BuyerSeller"; | ||
| tableNames["ProcInsertBuyerSeller"] = "InsertBuyerSeller"; | ||
| tableNames["ProcUpdateBuyerSeller"] = "UpdateBuyerSeller"; |
Copilot
AI
Dec 2, 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 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:
- Add the BuyerSeller table and stored procedure creation to the
SQLSetupStrategyCertStoreProvider.SetupDatabase()method or the baseCreateTables()method, similar to how other test tables likeApiTestTableare created. - Create a dedicated table class like
BuyerSellerTablein theAlwaysEncrypted/TestFixtures/Setup/directory that implements theICreatableandIDroppableinterfaces.
Description
This Pull Request addresses issue #3716 by introducing a null check for systemParams, which stores the system-level parameters for SQL RPC (Remote Procedure Call) operations. In batch scenarios, certain SQL RPC calls may not include system parameters, and this change ensures proper handling in such cases.
Issues
#3716