jonwagner / Insight.Database

Fast, lightweight .NET micro-ORM
Other
856 stars 145 forks source link

Data loss when using table valued parameters in .NET 6 #475

Closed nzbart closed 2 years ago

nzbart commented 2 years ago

When using a table valued parameter with SQL Server type Money and C# type decimal, the decimal places are lost when sent to the database.

This problem does not appear to be present when running under the .NET Framework. Note that I have not attempted to reproduce this issue in any other .NET versions, so can't comment on which versions may have this problem. .NET 6 is the latest production .NET version.

Steps to reproduce

Create a C# project with the following files:

Program.cs

using System.Data.SqlClient;
using Insight.Database;

var masterCon = new SqlConnection("Server=(local);trusted_connection=true");
await masterCon.ExecuteSqlAsync("drop database if exists bugdemo; create database bugdemo");
var con = new SqlConnection(masterCon.ConnectionString + ";Database=bugdemo");
await con.ExecuteSqlAsync("create type MoneyDemoTable as table (Value money)");
var original = new MoneyDemo(1.234m); 
var actual = await con.SingleSqlAsync<decimal>("select * from @Value", new { Value = new[] { original } });

Console.WriteLine("Original: {0}", original.Value);
Console.WriteLine("Actual: {0}", actual);

record MoneyDemo(decimal Value);

repro.csproj

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net6.0</TargetFramework>
    <ImplicitUsings>enable</ImplicitUsings>
    <Nullable>enable</Nullable>
    <LangVersion>default</LangVersion>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="Insight.Database.Providers.Default" Version="6.3.9" />
  </ItemGroup>

</Project>

Update the connection string to point to a SQL Server instance if you haven't got SQL Server running locally, and run the app using dotnet run. It will output the following:

Original: 1.234
Actual: 1.0000

Expected behavior

I expect the output to be as follows, since decimal places should not be lost:

Original: 1.234
Actual: 1.234
nzbart commented 2 years ago

This change appears to fix the problem for me:

diff --git a/Insight.Database.Providers.Default/SqlDataRecordAdapter.cs b/Insight.Database.Providers.Default/SqlDataRecordAdapter.cs
index 442cfad..3d8c416 100644
--- a/Insight.Database.Providers.Default/SqlDataRecordAdapter.cs
+++ b/Insight.Database.Providers.Default/SqlDataRecordAdapter.cs
@@ -114,6 +114,8 @@ namespace Insight.Database.Providers.Default
                        return SqlDbType.DateTime;
                    case "datetime2":
                        return SqlDbType.DateTime2;
+                   case "money":
+                       return SqlDbType.Money;
                }
            }

I'm happy to create a pull request, but could do with some advice about where to write the tests. The closest, similar, test I can see is Datetime2FieldsShouldConvertInTVP. Should I create one below that?

jonwagner commented 2 years ago

Yeah that’s a good place for a test. If you put in a pr i can do a build this weekend (along with the sql driver version update)

On May 5, 2022, at 9:11 PM, nzbart @.***> wrote:



This change appears to fix the problem for me:

diff --git a/Insight.Database.Providers.Default/SqlDataRecordAdapter.cs b/Insight.Database.Providers.Default/SqlDataRecordAdapter.cs index 442cfad..3d8c416 100644 --- a/Insight.Database.Providers.Default/SqlDataRecordAdapter.cs +++ b/Insight.Database.Providers.Default/SqlDataRecordAdapter.cs @@ -114,6 +114,8 @@ namespace Insight.Database.Providers.Default return SqlDbType.DateTime; case "datetime2": return SqlDbType.DateTime2;

I'm happy to create a pull request, but could do with some advice about where to write the tests. The closest, similar, test I can see is Datetime2FieldsShouldConvertInTVP. Should I create one below that?

— Reply to this email directly, view it on GitHubhttps://github.com/jonwagner/Insight.Database/issues/475#issuecomment-1119198991, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AAMTO5CFZ5CJAXECGS7YXWLVIR5UBANCNFSM5VARC3NQ. You are receiving this because you are subscribed to this thread.Message ID: @.***>