LegacyRowVersionNullBehavior causes InvalidCastException when using SqlDataReader.GetFieldValue[T]

[andygjp]

Describe the bug

I updated my project, which uses EF Core 5.0.8, to use the latest version SqlClient, 3.0.0, and got an InvalidCastException exception. Turning off SqlServerDbContextOptionsBuilder.EnableRetryOnFailure() prevents the error, but I need it on.

Investigating further, the issue seems to come from SqlDataReader.GetFieldValue[T](Int32 i).

Exception message: Unable to cast object of type 'System.DBNull' to type 'System.Byte[]'.

Stack trace: 
System.InvalidCastException: Unable to cast object of type 'System.DBNull' to type 'System.Byte[]'.
   at Microsoft.Data.SqlClient.SqlDataReader.GetFieldValueFromSqlBufferInternal[T](SqlBuffer data, _SqlMetaData metaData)
   at Microsoft.Data.SqlClient.SqlDataReader.GetFieldValueInternal[T](Int32 i)
   at Microsoft.Data.SqlClient.SqlDataReader.GetFieldValue[T](Int32 i)

To reproduce

I've recreated the issue in a repo: https://github.com/andygjp/NullRowVersionIssue

Expected behavior

It should not throw an exception - the GetFieldValueFromSqlBufferInternal() function needs to have the same behaviour as TryReadColumnInternal(Int32, Bool), ie respect LocalAppContextSwitches.LegacyRowVersionNullBehavior

Further technical details

dotnet --info
.NET SDK (reflecting any global.json):
 Version:   5.0.204
 Commit:    84d1fe1bb7

Runtime Environment:
 OS Name:     fedora
 OS Version:  34
 OS Platform: Linux
 RID:         fedora.34-x64
 Base Path:   /usr/lib64/dotnet/sdk/5.0.204/

Host (useful for support):
  Version: 5.0.7
  Commit:  556582d964

.NET SDKs installed:
  3.1.116 [/usr/lib64/dotnet/sdk]
  5.0.204 [/usr/lib64/dotnet/sdk]

.NET runtimes installed:
  Microsoft.AspNetCore.App 3.1.16 [/usr/lib64/dotnet/shared/Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 5.0.7 [/usr/lib64/dotnet/shared/Microsoft.AspNetCore.App]
  Microsoft.NETCore.App 3.1.16 [/usr/lib64/dotnet/shared/Microsoft.NETCore.App]
  Microsoft.NETCore.App 5.0.7 [/usr/lib64/dotnet/shared/Microsoft.NETCore.App]

[andygjp]

I've added another test case that uses SqlDataReader.GetValues(Object[]) (andygjp/NullRowVersionIssue@8a93ea3) and that fails too.

I don't think the Switch.Microsoft.Data.SqlClient.LegacyRowVersionNullBehavior switch actually maintains backwards compatibility. If this is enabled, I would expect an empty byte array to be returned, but it is not - I get DBNull.

[Wraith2]

I did add tests with the feature but if something has been missed can you take a look and see if you can help extend the test coverage with your failing scenarios? #998

I'll take a look when I can get time and see if there's a simple fix.

[andygjp]

Hi,

I'm struggling to get the solution to build on my Linux development machine. I'll see if I can get it to build on Windows machine - I'll report back once I've done that.

However, I've been looking at the change to SqlDataReader, assuming the problem lies in this class, I think the change that should've been made to maintain backwards compatibility is this:

if (isNull && columnMetaData.type != SqlDbType.Timestamp)
{
    TdsParser.GetNullSqlValue(_data[_sharedState._nextColumnDataToRead],
        columnMetaData,
        _command != null ? _command.ColumnEncryptionSetting : SqlCommandColumnEncryptionSetting.UseConnectionSetting,
        _parser.Connection);

    if (!readHeaderOnly)
    {
        _sharedState._nextColumnDataToRead++;
    }
}
else
{
    if (isNull && columnMetaData.type == SqlDbType.Timestamp && !LocalAppContextSwitches.LegacyRowVersionNullBehavior)
    {
        _data[i].SetToNullOfType(SqlBuffer.StorageType.SqlBinary);
    }
    else if ((i > _sharedState._nextColumnDataToRead) || (!readHeaderOnly))
    {
        // If we're not in sequential access mode, we have to
        // save the data we skip over so that the consumer
        // can read it out of order
        if (!_parser.TryReadSqlValue(_data[_sharedState._nextColumnDataToRead], columnMetaData, (int)dataLength, _stateObj,
            _command != null ? _command.ColumnEncryptionSetting : SqlCommandColumnEncryptionSetting.UseConnectionSetting,
            columnMetaData.column))
        { // will read UDTs as VARBINARY.
            return false;
        }
        _sharedState._nextColumnDataToRead++;
    }
    else
    {
        _sharedState._columnDataBytesRemaining = (long)dataLength;
    }
}

This way, the code execution heads into the else block allowing it to do what it previously did, but with the addition of the if (isNull && columnMetaData.type == SqlDbType.Timestamp && !LocalAppContextSwitches.LegacyRowVersionNullBehavior) to execute the non-legacy functionality.

[Wraith2]

Building can be hard work to setup and isn't truly required if you can just work up tests to add alongside the ones already present trying to cover this feature. For example take

[ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup))]
        public static void CheckNullRowVersionIsBDNull()
        {
            lock (s_rowVersionLock)
            {
                bool? originalValue = SetLegacyRowVersionNullBehaviour(false);
                try
                {
                    using (SqlConnection con = new SqlConnection(DataTestUtility.TCPConnectionString))
                    {
                        con.Open();
                        using (SqlCommand command = con.CreateCommand())
                        {
                            command.CommandText = "select cast(null as rowversion) rv";
                            using (SqlDataReader reader = command.ExecuteReader())
                            {
                                reader.Read();
                                Assert.True(reader.IsDBNull(0));
                                Assert.Equal(reader[0], DBNull.Value);
                            }
                        }
                    }
                }
                finally
                {
                    SetLegacyRowVersionNullBehaviour(originalValue);
                }
            }
        }

As a template and change it to cover the things that currently aren't working, positive and negative cases for both legacy and new behaviour if possible. I can then drop those in and use them to explore and alter the behaviour.

[andygjp]

Hi,

I've come up with these three tests:

[ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup))]
public static void CheckGetValuesNullRowVersionReturnsEmptyByteArray()
{
    lock (s_rowVersionLock)
    {
        bool? originalValue = SetLegacyRowVersionNullBehaviour(true);
        try
        {
            using (SqlConnection con = new SqlConnection(DataTestUtility.TCPConnectionString))
            {
                con.Open();
                using (SqlCommand command = con.CreateCommand())
                {
                    command.CommandText = "select cast(null as rowversion) rv";
                    using (SqlDataReader reader = command.ExecuteReader())
                    {
                        reader.Read();
                        var data = new object[1];
                        reader.GetValues(data);
                        Assert.True(data[0] is byte[] {Length: 0});
                    }
                }
            }
        }
        finally
        {
            SetLegacyRowVersionNullBehaviour(originalValue);
        }
    }
}

[ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup))]
public static void CheckGetValueNullRowVersionReturnsEmptyByteArray()
{
    lock (s_rowVersionLock)
    {
        bool? originalValue = SetLegacyRowVersionNullBehaviour(true);
        try
        {
            using (SqlConnection con = new SqlConnection(DataTestUtility.TCPConnectionString))
            {
                con.Open();
                using (SqlCommand command = con.CreateCommand())
                {
                    command.CommandText = "select cast(null as rowversion) rv";
                    using (SqlDataReader reader = command.ExecuteReader())
                    {
                        reader.Read();
                        var value = reader.GetValue(0);
                        Assert.True(value is byte[] {Length: 0});
                    }
                }
            }
        }
        finally
        {
            SetLegacyRowVersionNullBehaviour(originalValue);
        }
    }
}

[ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup))]
public static void CheckGetFieldValueNullRowVersionDoesNotThrow()
{
    lock (s_rowVersionLock)
    {
        bool? originalValue = SetLegacyRowVersionNullBehaviour(true);
        try
        {
            using (SqlConnection con = new SqlConnection(DataTestUtility.TCPConnectionString))
            {
                con.Open();
                using (SqlCommand command = con.CreateCommand())
                {
                    command.CommandText = "select cast(null as rowversion) rv";
                    using (SqlDataReader reader = command.ExecuteReader())
                    {
                        reader.Read();
                        var ex = Record.Exception(() => reader.GetFieldValue<byte[]>(0));
                        Assert.Null(ex);
                    }
                }
            }
        }
        finally
        {
            SetLegacyRowVersionNullBehaviour(originalValue);
        }
    }
}

I've added the equivalent tests to my repo: andygjp/NullRowVersionIssue@f282a1f

[JRahnama]

@andygjp thanks for the repro tests. I spent a short time yesterday on the failures and the behavior is as you described with or without the switch as far as I could see. We will get back to you shortly with an explanation if this is by design or a fix to address your concern.

@Wraith2 is this something you like to work on or should we continue looking into this?

[Wraith2]

I intend to look at it so you can assign it to me.

[JRahnama]

@Wraith2 thanks for the continuous support.

[Wraith2]

When I revert the change CheckNullRowVersionIsDBNull still fails, can you check the logic on that one please. I'll debug the other two in the meantime.
[edit]
Nm, not a new one. Ignore me,

[Wraith2]

Ok, prospective fix is:

switch (columnMetaData.type)
{
    case SqlDbType.Timestamp: // rowversion has special behavior
        if (isNull && !LocalAppContextSwitches.LegacyRowVersionNullBehavior)
        {
            _data[i].SetToNullOfType(SqlBuffer.StorageType.SqlBinary);
        }
        else
        {
            goto ReadDataValue;
        }
        break;

    default: 
        if (isNull)
        {
            goto GetNullValue;
        }
        else
        {
            goto ReadDataValue;
        }

    ReadDataValue: // read the data value
        if ((i > _sharedState._nextColumnDataToRead) || (!readHeaderOnly))
        {
            // If we're not in sequential access mode, we have to
            // save the data we skip over so that the consumer
            // can read it out of order
            if (!_parser.TryReadSqlValue(_data[_sharedState._nextColumnDataToRead], columnMetaData, (int)dataLength, _stateObj,
                _command != null ? _command.ColumnEncryptionSetting : SqlCommandColumnEncryptionSetting.UseConnectionSetting,
                columnMetaData.column, _command))
            { // will read UDTs as VARBINARY.
                return false;
            }
            _sharedState._nextColumnDataToRead++;
        }
        else
        {
            _sharedState._columnDataBytesRemaining = (long)dataLength;
        }
        break;

    GetNullValue: // fetch a correctly typed null value
        TdsParser.GetNullSqlValue(_data[_sharedState._nextColumnDataToRead],
            columnMetaData,
            _command != null ? _command.ColumnEncryptionSetting : SqlCommandColumnEncryptionSetting.UseConnectionSetting,
            _parser.Connection);

        if (!readHeaderOnly)
        {
            _sharedState._nextColumnDataToRead++;
        }
        break;
}

but I'm not sure how other people feel about using gotos in switches. Done this way each value is only checked once so we don't waste time comparing things. It's a bit jumpy but cleaner than trying to express the same thing in pure if statements, you can clearly see the decisions being made and then named areas to implement them. What do you think @JRahnama ?

[ErikEJ]

Goto !?

[ErikEJ]

Maybe move GetNullValue up inline?

[Wraith2]

Goto !?

Yes. Sometimes I find that it's the cleanest way to express things.
This approach doesn't duplicate work and clearly separates decision and action. I could break the two action blocks into location functions or dedicated functions but it feels wasteful doing all that work setting up a stackframe and pushing/popping args for such small reasons.

[ErikEJ]

Makes sense, and this code must be highly performant...