[Breaking change]: SqlDataReader.GetString() behavior change on null

[jhudsoncedaron]

Description

When upgrading from .NET 6 to .NET 7 we encountered a breaking change in SqlDataReader.

For some reason, upgrading runtime from 6 to 7 changes the behavior of System.Data.SqlDataReader.GetString() to throw when asked to read a null rather than return a null. This is particularly weird because I hadn't gotten around to upgrading SqlDataReader yet. I'm still using the version number from .NET 6.

      IDataReader source;
-    string IDataRow.GetString(int index) => source.GetString(index);
+    // NET 6 behavior: GetString(null column) -> null
+    // NET 7 behavior: GetString(null column) -> throw
+    // BSONReader behavior -> null
+    // Thus expected itnerface behavior is null so we do that
+    string IDataRow.GetString(int index) => source.IsDBNull(index) ? null : source.GetString(index);

Version

.NET 7

Previous behavior

GetString(null column id) => null

New behavior

GetString(null column id) => throw

Type of breaking change

• Binary incompatible: Existing binaries may encounter a breaking change in behavior, such as failure to load/execute or different run-time behavior.
• Source incompatible: Source code may encounter a breaking change in behavior when targeting the new runtime/component/SDK, such as compile errors or different run-time behavior.

Reason for change

unknown

Recommended action

Document change

Feature area

Other (please put exact area in description textbox)

Affected APIs

System.Data.SqlClient.GetString()

[gewarren]

@DavoudEshtehari @David-Engel @JRahnama Can one of you comment on this .NET 7 behavior?

[jhudsoncedaron]

Update: turns out the breaking change wasn't actually in .NET 7. Just started getting it in our .NET 6 codebase today after applying security updates.

It bisects to this change:

-    <PackageReference Include="System.Data.SqlClient" Version="4.8.3" />
+    <PackageReference Include="System.Data.SqlClient" Version="4.8.5" />

The new behavior is most likely the correct behavior; however it was a surprising breaking change and deserves to be documented.

To fully understand how this got found and why it was surprising; we must take a bit of a diversion. We have an interface for a platform-independent data reader. The way the application works is the web server calls the web API and gets back a data reader that isn't even a SqlDataReader but is reading from the response stream on the web API. On the other side, the same interface wraps a SqlDataReader and the controller just runs foreach { foreach { to output all the rows into the stream. But there's this one caller that runs directly inside the application server and gets the same interface wrapping from a database call without making an API call (because it's already inside the API server). In one place it does GetString(column) ?? "default text" where "default text" should be regarded as more like error recovery text than expected behavior. And starting with the security update, GetString() started throwing on one of our development databases.