-
Notifications
You must be signed in to change notification settings - Fork 321
ArgumentNullException when reading empty/null PLP strings with new async behavior #3884
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 |
|---|---|---|
|
|
@@ -1084,5 +1084,124 @@ [Value] [nvarchar](max) NULL | |
| } | ||
| } | ||
| } | ||
|
|
||
| // Test for Issue #593 - ArgumentNullException when reading empty/null PLP strings with new async behavior | ||
| [ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup))] | ||
| public static async Task ReadEmptyAndNullPlpStringsAsyncWithNewBehavior() | ||
| { | ||
| string tableName = DataTestUtility.GenerateObjectName(); | ||
| bool? originalSwitchValue = null; | ||
|
|
||
| try | ||
| { | ||
| // Save original switch value and disable compatibility mode to use new async behavior | ||
| using (var switchHelper = new SwitchesHelper()) | ||
| { | ||
| originalSwitchValue = switchHelper.UseCompatibilityAsyncBehaviour; | ||
| switchHelper.UseCompatibilityAsyncBehaviour = false; | ||
| } | ||
|
|
||
| using (SqlConnection connection = new SqlConnection(DataTestUtility.TCPConnectionString)) | ||
| { | ||
| await connection.OpenAsync(); | ||
|
|
||
| // Create test table with PLP string columns | ||
| using (SqlCommand cmd = connection.CreateCommand()) | ||
| { | ||
| cmd.CommandText = $@" | ||
| CREATE TABLE {tableName} ( | ||
| Id INT PRIMARY KEY, | ||
| VarcharMaxCol VARCHAR(MAX), | ||
| NVarcharMaxCol NVARCHAR(MAX), | ||
| TextCol TEXT | ||
| )"; | ||
| await cmd.ExecuteNonQueryAsync(); | ||
| } | ||
|
|
||
| // Insert test data: NULL, empty string, and non-empty values | ||
| using (SqlCommand cmd = connection.CreateCommand()) | ||
| { | ||
| cmd.CommandText = $@" | ||
| INSERT INTO {tableName} (Id, VarcharMaxCol, NVarcharMaxCol, TextCol) VALUES | ||
| (1, NULL, NULL, NULL), | ||
| (2, '', '', ''), | ||
| (3, 'test', N'test', 'test'), | ||
| (4, NULL, '', 'value'), | ||
| (5, '', NULL, NULL)"; | ||
| await cmd.ExecuteNonQueryAsync(); | ||
| } | ||
|
|
||
| // Read data asynchronously - this should not throw ArgumentNullException | ||
| using (SqlCommand cmd = connection.CreateCommand()) | ||
| { | ||
| cmd.CommandText = $"SELECT Id, VarcharMaxCol, NVarcharMaxCol, TextCol FROM {tableName} ORDER BY Id"; | ||
| using (SqlDataReader reader = await cmd.ExecuteReaderAsync()) | ||
| { | ||
| // Row 1: All NULL values | ||
| Assert.True(await reader.ReadAsync()); | ||
| Assert.Equal(1, reader.GetInt32(0)); | ||
| Assert.True(reader.IsDBNull(1)); | ||
| Assert.True(reader.IsDBNull(2)); | ||
| Assert.True(reader.IsDBNull(3)); | ||
|
|
||
| // Row 2: All empty strings | ||
| Assert.True(await reader.ReadAsync()); | ||
| Assert.Equal(2, reader.GetInt32(0)); | ||
| Assert.Equal(string.Empty, reader.GetString(1)); | ||
| Assert.Equal(string.Empty, reader.GetString(2)); | ||
| Assert.Equal(string.Empty, reader.GetString(3)); | ||
|
|
||
| // Row 3: Non-empty values | ||
| Assert.True(await reader.ReadAsync()); | ||
| Assert.Equal(3, reader.GetInt32(0)); | ||
| Assert.Equal("test", reader.GetString(1)); | ||
| Assert.Equal("test", reader.GetString(2)); | ||
| Assert.Equal("test", reader.GetString(3)); | ||
|
|
||
| // Row 4: Mixed NULL and values | ||
| Assert.True(await reader.ReadAsync()); | ||
| Assert.Equal(4, reader.GetInt32(0)); | ||
| Assert.True(reader.IsDBNull(1)); | ||
| Assert.Equal(string.Empty, reader.GetString(2)); | ||
| Assert.Equal("value", reader.GetString(3)); | ||
|
|
||
| // Row 5: Mixed empty and NULL | ||
| Assert.True(await reader.ReadAsync()); | ||
| Assert.Equal(5, reader.GetInt32(0)); | ||
| Assert.Equal(string.Empty, reader.GetString(1)); | ||
| Assert.True(reader.IsDBNull(2)); | ||
| Assert.True(reader.IsDBNull(3)); | ||
|
|
||
| Assert.False(await reader.ReadAsync()); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| finally | ||
| { | ||
| // Restore original switch value | ||
| if (originalSwitchValue.HasValue) | ||
| { | ||
| using (var switchHelper = new SwitchesHelper()) | ||
| { | ||
| switchHelper.UseCompatibilityAsyncBehaviour = originalSwitchValue; | ||
| } | ||
| } | ||
|
Comment on lines
+1183
to
+1189
|
||
|
|
||
| // Clean up test table | ||
| try | ||
| { | ||
| using (SqlConnection connection = new SqlConnection(DataTestUtility.TCPConnectionString)) | ||
| { | ||
| connection.Open(); | ||
| DataTestUtility.DropTable(connection, tableName); | ||
| } | ||
| } | ||
| catch | ||
| { | ||
| // Ignore cleanup errors | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
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 SwitchesHelper usage pattern is incorrect. The helper is being disposed at line 1102 before the actual test runs, which means the UseCompatibilityAsyncBehaviour switch will be restored to its original value before the test executes.
The correct pattern is to keep the SwitchesHelper alive for the entire scope where you need the modified switch value. The helper should be created at the beginning of the try block and only disposed after all test operations that depend on the switch value are complete.
Looking at other tests in this file (e.g., CheckNullRowVersionIsDBNull at line 270), the pattern should be:
Additionally, lines 1185-1188 attempt to restore the switch value manually, but this is unnecessary and incorrect since the Dispose method of SwitchesHelper already restores all original values automatically.
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.
@copilot open a new pull request to apply changes based on this feedback