-
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?
Changes from all commits
0488e8c
b6bdbaa
92fc6cb
ae05d9d
a8bda9a
373693f
e1ace34
3696a19
cbec7a7
522ff97
ed45b19
e926fb5
6137517
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,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; | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
priyankatiwari08 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||
| // 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
|
||||||||||||||||||||||||||
| // 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
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.
priyankatiwari08 marked this conversation as resolved.
Show resolved
Hide resolved
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).
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.
priyankatiwari08 marked this conversation as resolved.
Show resolved
Hide resolved
priyankatiwari08 marked this conversation as resolved.
Show resolved
Hide resolved
priyankatiwari08 marked this conversation as resolved.
Show resolved
Hide resolved
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"; |
paulmedynski marked this conversation as resolved.
Show resolved
Hide resolved
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.
Uh oh!
There was an error while loading. Please reload this page.