dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.16k stars 4.71k forks source link

new SqlDouble(NaN) is only allowed on NETFX #25912

Closed dotMorten closed 4 years ago

dotMorten commented 6 years ago

SqlDouble has a weird platform difference:

        public SqlDouble(double value)
        {
#if !netfx
            if (!double.IsFinite(value))
#else
            if (double.IsInfinity(value) || double.IsNaN(value))
#endif
            {
                throw new OverflowException(SQLResource.ArithOverflowMessage);
            }
            else
            {
                m_value = value;
                m_fNotNull = true;
            }
        }

https://github.com/dotnet/corefx/blob/master/src/System.Data.Common/src/System/Data/SQLTypes/SQLDouble.cs#L40

This means I can insert NaN values when not using the .NET Framework, but when running the same code on a different platform, I get a crash here. Easy to reproduce:

new SqlDouble(double.NaN); //Crashes on NETFX

This is rather problematic because NaN is commonly used in the SqlGeometry and SqlGeography datatypes to represent Z and M values, where for instance a line generally contains Z measurements, but on some vertices they might not be available. The behavior is described in Microsoft SQL Server CLR Types Serialization Formats on Page 9:

image

dotMorten commented 6 years ago

@tannergooding Can you share the reasoning behind this commit: https://github.com/dotnet/corefx/commit/1194cdd15a91f0431c19cb2a03d5959aa698e658 ?

karelz commented 6 years ago

@keeratsingh @David-Engel @AfsanehR @saurabh500 @divega looks like a breaking change with impact on SqlClient ...

saurabh500 commented 6 years ago

cc @tannergooding who made this change in https://github.com/dotnet/corefx/commit/1194cdd15a91f0431c19cb2a03d5959aa698e658 as part of https://github.com/dotnet/corefx/pull/22490

dotMorten commented 6 years ago

Brainfart! It says 'IsFinite' not 'IsInfinite' and the behavior is the same on both platforms.

tannergooding commented 6 years ago

Right, the resulting behavior should be the same, but using a single combined check, rather than two separate checks.