Fix IsDBNull + stream read bug that skips column data by paulmedynski · Pull Request #4082 · dotnet/SqlClient

paulmedynski · 2026-03-24T18:43:16Z

Fix: IsDBNull/IsDBNullAsync breaks subsequent streaming reads in SequentialAccess mode

Problem

When using CommandBehavior.SequentialAccess, calling IsDBNull() or IsDBNullAsync() on a column before reading it via a streaming type (GetFieldValue<Stream>, GetFieldValue<TextReader>, GetFieldValue<XmlReader>, or their async variants) causes:

Debug builds: DebugAssertException — "Already read past column"
Release builds: Silent data loss — the stream returns 0 bytes

Root Cause

TryReadColumnInternal in SqlDataReader.cs has two code paths for sequential access depending on whether the column header has already been read:

Path A — header already read (i < _nextColumnHeaderToRead):

if ((i == _sharedState._nextColumnDataToRead) && (!readHeaderOnly))
{
    return TryReadColumnData();  // consumes ALL column data from the wire
}

Path B — header not yet read (_nextColumnHeaderToRead == i):

if (!readHeaderOnly && !forStreaming)
{
    result = _parser.TryReadSqlValue(…);
    _sharedState._nextColumnDataToRead++;
}

Path B correctly checks both !readHeaderOnly and !forStreaming before consuming data. Path A only checks !readHeaderOnly, ignoring forStreaming.

When IsDBNull/IsDBNullAsync is called first, it reads the column header (advancing _nextColumnHeaderToRead) but leaves data on the wire. The subsequent streaming GetFieldValue<Stream> call enters Path A (since the header is already read) and eagerly consumes all data via TryReadColumnData(), leaving nothing for the SqlSequentialStream to read.

Fix

Add && (!forStreaming) to Path A's condition to match Path B's behavior:

-if ((i == _sharedState._nextColumnDataToRead) && (!readHeaderOnly))
+if ((i == _sharedState._nextColumnDataToRead) && (!readHeaderOnly) && (!forStreaming))

This ensures that when a streaming type is requested after IsDBNull has already read the column header, the column data stays on the wire for the sequential stream to consume incrementally.

Compatibility

The fix only affects calls where forStreaming == true, which is limited to GetFieldValue<T>/GetFieldValueAsync<T> with T being Stream, TextReader, or XmlReader.
All other callers pass forStreaming: false (the default) — zero behavior change.
The previous behavior was always broken (crash or silent data loss), so no application could depend on it.

Tests

Added 7 new test methods (28 test cases) covering all combinations of:

IsDBNull variant	GetFieldValue variant	Data type
`IsDBNullAsync`	`GetFieldValueAsync<TextReader>`	nvarchar
`IsDBNullAsync`	`GetFieldValueAsync<XmlReader>`	xml
`IsDBNull` (sync)	`GetFieldValue<Stream>` (sync)	varbinary
`IsDBNull` (sync)	`GetFieldValue<TextReader>` (sync)	nvarchar
`IsDBNull` (sync)	`GetFieldValue<XmlReader>` (sync)	xml
`IsDBNull` (sync)	`GetFieldValueAsync<Stream>` (async)	varbinary
`IsDBNullAsync`	`GetFieldValue<Stream>` (sync)	varbinary

Each test method has 4 inline data cases: null/non-null × checkNull true/false.

Verification:

Condition	Without fix	With fix
`checkNull: True` (bug path)	16 failed	0 failed
`checkNull: False` (control)	17 passed	33 passed

Checklist

Tests added or updated
Verified against customer repro
No breaking changes introduced
Public API changes documented (N/A — no public API changes)

Copilot

Pull request overview

Fixes a SqlDataReader sequential-access streaming regression where calling IsDBNull/IsDBNullAsync before GetFieldValue<T> for streaming types could consume the column payload and cause empty streams / asserts.

Changes:

Updates SqlDataReader.TryReadColumnInternal to avoid eagerly consuming column data when forStreaming == true and the column header was already read.
Adds new manual test coverage for IsDBNull/IsDBNullAsync + streaming GetFieldValue<T> combinations under CommandBehavior.SequentialAccess.
Adds test-utility configurability (env var override) and hardens an Always Encrypted CSP test’s data discovery on non-Windows.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

File	Description
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlDataReader.cs	Adjusts sequential-access read logic to preserve wire data for streaming readers after `IsDBNull*`.
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderStreamsTest.cs	Adds manual regression tests for streaming reads after `IsDBNull/IsDBNullAsync` in sequential access.
src/Microsoft.Data.SqlClient/tests/tools/Microsoft.Data.SqlClient.TestUtilities/Config.cs	Adds env-var override support for test config values.
src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/CspProviderExt.cs	Prevents registry access during test discovery on non-Windows.

src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderStreamsTest.cs

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlDataReader.cs

src/Microsoft.Data.SqlClient/tests/tools/Microsoft.Data.SqlClient.TestUtilities/Config.cs

codecov · 2026-03-24T20:09:06Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 66.49%. Comparing base (60d4b92) to head (71b9bfa).
⚠️ Report is 13 commits behind head on main.

❗ There is a different number of reports uploaded between BASE (60d4b92) and HEAD (71b9bfa). Click for more details.

HEAD has 1 upload less than BASE

Flag BASE (60d4b92) HEAD (71b9bfa)

CI-SqlClient 1 0

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4082      +/-   ##
==========================================
- Coverage   73.22%   66.49%   -6.73%     
==========================================
  Files         280      275       -5     
  Lines       43000    65808   +22808     
==========================================
+ Hits        31486    43758   +12272     
- Misses      11514    22050   +10536

Flag	Coverage Δ
CI-SqlClient	`?`
PR-SqlClient-Project	`66.49% <100.00%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

paulmedynski · 2026-03-25T11:37:56Z

src/Microsoft.Data.SqlClient/tests/Common/Microsoft.Data.SqlClient.TestCommon.csproj

  <ItemGroup Condition="'$(TargetFramework)' == 'net462'">
    <PackageReference Include="Azure.Identity" />
    <PackageReference Include="Azure.Security.KeyVault.Keys" />
-    <PackageReference Include="Microsoft.DotNet.XUnitExtensions" />


This project doesn't use xUnit. Including these was causing VS Code test explorer to attempt test discovery, which failed consistently when builds occurred elsewhere.

paulmedynski · 2026-03-25T11:39:54Z

src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/CspProviderExt.cs

            {
+                // xUnit evaluates MemberData during discovery, before [PlatformSpecific]
+                // can skip execution. Guard here to prevent Registry access on non-Windows.
+                if (!RuntimeInformation.IsOSPlatform(OSPlatform.Windows))


The test class is gated by [PlatformSpecific(TestPlatforms.Windows)], but that only applies at test execution time. Test discovery occurs regardless of attributes like these, and in this case would fail on Unix due to the Windows-specific registry calls below.

paulmedynski · 2026-03-25T11:41:03Z

src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderStreamsTest.cs

-                        Assert.True(reader.IsDBNull(1));
-                        Assert.True(reader.IsDBNull(2));
-                    }
+                    Assert.True(reader.Read());


Fixed broken conditions here, that would pass the test if the Read() failed.

paulmedynski · 2026-03-25T11:42:10Z

src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderStreamsTest.cs

        }
 #endif

+#nullable enable


New tests that expose the bug behaviour. All of these tests fail without the change to SqlDataReader.

paulmedynski · 2026-03-25T11:44:48Z

src/Microsoft.Data.SqlClient/tests/tools/Microsoft.Data.SqlClient.TestUtilities/Config.cs

+            }
+
+            // Allow environment variables to override individual config values.
+            SetFromEnv("MDS_TCPConnectionString", ref config.TCPConnectionString);


This makes it easy to run many tests (like the new ones above) with a single command:

MDS_TCPConnectionString="Server=tcp:127.0.0.1,1434;Database=Northwind;User ID=sa;Password=****;Encrypt=false;TrustServerCertificate=true" dotnet test --no-build src/Microsoft.Data.SqlClient/tests/ManualTests/ -f net10.0 --filter IsDBNull

No need to jump through the config file hoops.

Please document the same in BUILDGUIDE so its easier for developers to know about these variables that can be used for local development.

- Added a unit test that exposes the IsDBNullAsync() and GetFieldValueAsync() interaction problem.

…mlReader]>() that would skip column data.

- Fixed a long-standing issue with test discovery failures in VS Code with the TestCommon project.

Copilot

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

src/Microsoft.Data.SqlClient/tests/tools/Microsoft.Data.SqlClient.TestUtilities/Config.cs

src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderStreamsTest.cs

paulmedynski · 2026-03-27T11:18:06Z

/azp run

azure-pipelines · 2026-03-27T11:18:31Z

Azure Pipelines successfully started running 2 pipeline(s).

paulmedynski added this to the 7.1.0-preview1 milestone Mar 24, 2026

paulmedynski requested a review from a team as a code owner March 24, 2026 18:43

paulmedynski added the Bug label Mar 24, 2026

Copilot AI review requested due to automatic review settings March 24, 2026 18:43

github-project-automation bot added this to SqlClient Board Mar 24, 2026

github-project-automation bot moved this to To triage in SqlClient Board Mar 24, 2026

Copilot started reviewing on behalf of paulmedynski March 24, 2026 18:43 View session

paulmedynski moved this from To triage to In review in SqlClient Board Mar 24, 2026

Copilot AI reviewed Mar 24, 2026

View reviewed changes

cheenamalhotra removed the Bug label Mar 24, 2026

paulmedynski commented Mar 25, 2026

View reviewed changes

cheenamalhotra previously approved these changes Mar 25, 2026

View reviewed changes

paulmedynski added 4 commits March 26, 2026 14:37

Task 33220: Add unit tests to expose the behaviour

3cb254e

- Added a unit test that exposes the IsDBNullAsync() and GetFieldValueAsync() interaction problem.

Fix bug with IsDbNull[Async]() and GetFieldValue<[Stream,TextReader,X…

a380085

…mlReader]>() that would skip column data.

- Addressed Copilot comments in the PR.

ea003d3

- Fixed a long-standing issue with test discovery failures in VS Code with the TestCommon project.

Addressed review comments.

71b9bfa

Copilot AI review requested due to automatic review settings March 26, 2026 17:39

paulmedynski dismissed cheenamalhotra’s stale review via 71b9bfa March 26, 2026 17:39

paulmedynski force-pushed the dev/paul/issue-3057 branch from c2a9aee to 71b9bfa Compare March 26, 2026 17:39

Copilot started reviewing on behalf of paulmedynski March 26, 2026 17:40 View session

Copilot AI reviewed Mar 26, 2026

View reviewed changes

src/Microsoft.Data.SqlClient/tests/tools/Microsoft.Data.SqlClient.TestUtilities/Config.cs Show resolved Hide resolved

src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderStreamsTest.cs Show resolved Hide resolved

cheenamalhotra approved these changes Mar 28, 2026

View reviewed changes

paulmedynski added Hotfix 7.0.1 When this PR merges, automatically open a PR to cherry-pick to the 7.0.1 branch and removed Hotfix 7.0.1 When this PR merges, automatically open a PR to cherry-pick to the 7.0.1 branch labels Apr 1, 2026

Conversation

paulmedynski commented Mar 24, 2026

Fix: IsDBNull/IsDBNullAsync breaks subsequent streaming reads in SequentialAccess mode

Problem

Root Cause

Fix

Compatibility

Tests

Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Mar 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

paulmedynski Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

paulmedynski Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

paulmedynski Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

paulmedynski Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

paulmedynski Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

cheenamalhotra Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

paulmedynski commented Mar 27, 2026

Uh oh!

azure-pipelines bot commented Mar 27, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Mar 24, 2026 •

edited

Loading