SqlDataReader.GetXmlReader(int) returns unusable reader rather than throwing on DBNull

[madelson]

Describe the bug

Most GetXYZ(int) (e. g. GetInt32, GetString, etc) methods on SqlDataReader will throw if they encounter a DBNull.Value and cannot return it. The exception is GetValue which can just return DBNull.Value directly.

GetXmlReader is another exception but with (in my opinion) less reasonable behavior. When called on a column whose value is SQL NULL, it returns a non-null XmlReader object which fails when read from.

To reproduce

Include a complete code listing (or project/solution) that we can run to reproduce the issue.

var connectionString = "...";
using var conn = new Microsoft.Data.SqlClient.SqlConnection(connectionString);
conn.Open();
using var cmd = conn.CreateCommand();
cmd.CommandText = "select cast(null as xml)";
var reader = cmd.ExecuteReader();
reader.Read();

Console.WriteLine(reader.IsDBNull(0)); // true
using var xmlReader = reader.GetXmlReader(0); // works, does not return null
Console.WriteLine(XDocument.Load(xmlReader)); // throws InvalidOperationException "The XmlReader state should be interactive"
//Console.WriteLine(reader.GetString(0)); // throws SqlNullValueException "Data is Null. This method or property cannot be called on Null values."

Expected behavior

For consistency with the other get methods, I think GetXmlReader should throw SqlNullValueException when called on a DBNull column.

This behavior would be useful when processing query results because any errors due to expected non-null values fail fast early rather than manifesting later as a different-seeming exception.

Further technical details

Microsoft.Data.SqlClient version: 4.0.0
.NET target: .NET 5
SQL Server version: SQL Server 2017
Operating system: Windows 10

[Wraith2]

I found this behaviour when I was working to add GetFieldValue<TextReader> etc. I think the original reason was so that you could always use the returned reader as part of a using statement without throwing in the using code but that means that the user should always call IsDBNull beforehand or check the state of the reader once they've asked for and received it.

Unfortunately I think that changing the default behaviour of the existing methods to throw on null value is likely to break a lot of old code. The pattern I recommend is to always check IsDBNull before you ask for the reader because the null check is part of the row metadata it's essentially free. Programming assuming that null will throw exceptions is a poor choice because it's using exception for control flow which limits scalability.

[madelson]

I think the original reason was so that you could always use the returned reader as part of a using statement without throwing

Maybe this wasn't always true, but using blocks are null-safe, right? E. g. you can do:

using var y = SomeFunctionThatMightReturnNull();

And it will just noop when it gets to the implicit finally block.

Unfortunately I think that changing the default behaviour of the existing methods to throw on null value is likely to break a lot of old code

Perhaps, but:

(1) Microsoft.Data.SqlClient is relatively new compared to System.Data.SqlClient and it feels like stuff like this ought to be able to be fixed. The ability to be versioned independently of .NET gives consumers more control around upgrading as well.

(2) given that using blocks won't break and actually trying to do anything with the returned XmlReader throws an exception, I struggle to imagine that there's much code working well today under the old behavior that would be broken under the proposed new behavior. Obviously something can be contrived but it doesn't seem likely to be commonplace.

The pattern I recommend is to always check IsDBNull before you ask for the reader because the null check is part of the row metadata it's essentially free

I agree with this if the data might actually be null. However, in most cases you know whether the data coming back ought to be null or not based on the underlying column types, check constraints, business rules, etc. If you don't think it should ever be null, then I think it is the right move to just call GetInt32() or what have you and rely on the internal check to fail fast if your assumption was invalid (likely due to a bug elsewhere in the system) rather than having to insert an explicit if-null-throw check before every column access. It's like the cases where you'd use .Single() or .First() instead of the OrDefault() versions of those methods.

[JRahnama]

@madelson we will look into this and will decide about the breaking change internally and will update you soon.

[madelson]

Thanks @JRahnama . Even if it isn’t possible to make the change, it would be great to call out the inconsistency in the documentation.

[Wraith2]

Perhaps changing the default behaviour to throw and using an AppContext switch to turn back the previous behaviour like we have in other places would work here.

[lcheunglci]

We would support an API doc change. However, the implementation of the breaking change is not likely to happen.