Drizin / InterpolatedSql

Sql Builder using Interpolated Strings
MIT License
117 stars 7 forks source link

Feature Request for parameters specified with :text, :varchar, :nvarchar, etc. #8

Closed terryaney closed 11 months ago

terryaney commented 11 months ago

If I try the following:

var properties = XElement.Parse( """<Properties RBLFramework="Evolution"><Live Key="12359" ApprovedOn="2021-10-11T18:41:47.98" ApprovedBy="local.environment@samplelogin.com" /></Properties>""" );

var cn = new SqlConnection(this.Connection.ConnectionString);
var qb = cn.QueryBuilder($"UPDATE [File] SET Properties = {properties:text} WHERE UID = {11670}" );
await qb.ExecuteAsync();

I get the following SqlException: Operand type clash: xml is incompatible with text.

To get around this, I can simply put {properties.ToString()} but of course the IDE dims out .ToString() saying I don't need it (since it usually would do a .ToString() during normal string interpolation. So I have to make sure other users who don't know issue, don't take IDE suggestion of removing it. Additionally, I have to wrap it with:

#pragma warning disable IDE0071 // Interpoliation can be simplified
...
#pragma warning restore IDE0071 // Interpoliation can be simplified

Maybe be far fetched request, but curious on your thoughts. If a DB data type is provided and it is a 'string' type (text, ntext, char, varchar, nvarchar, etc.) you first do a .ToString() on the object before creating parameter? I could take a stab and a PR if you agree and don't have time to implement. I haven't looked at code base yet, but assume I'd find it pretty easily.

Drizin commented 11 months ago

I think what you're suggesting makes sense, but I'm afraid that those implicit conversions can bite us.

So for now I think the easiest (and extensible/maintainable) approach here would be subclassing SqlParameterMapper (you'll have to set the static SqlParameterMapper.Default to an instance of your new subclass), and in this subclass you would modify the way that AddToDynamicParameters passes the parameters to Dapper. You would want to check the parameter type and depending on the type you can use the ToString() or not.

The major extensibility points to customize how parameters are captured and passed to Dapper are InterpolatedSqlParser (mostly to override the TransformArgument method) and SqlParameterMapper (mostly to override AddToDynamicParameters) - probably any of those could be used for what you need, but SqlParameterMapper is probably easier since it's smaller and more specific.

Drizin commented 11 months ago

Please disregard my previous suggestion. I checked the code, and this is how explicitly-described string dbtypes are supposed to work:

So basically since your XElement is not previously converted to string (which would also be a quick solution, var properties = XElement.Parse("etc").ToString()), the "text" specifier is just being discarded, and I suppose Dapper passes your XElement as if it was a XML type but then it doesn't match what SQL expects for the field.

So I think the right solution is exactly what you suggested:

Would you like to create a PR?

terryaney commented 11 months ago

Digging into it. I've usually just use vscode/tasks.json to do builds. In attempting to run your build.ps1, I get the following errors:

C:\Program Files\dotnet\sdk\7.0.403\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.TargetFrameworkInference.targets(92,5): error NETSDK1013: The TargetFramework value '"netstandard2.0' was not recognized. It may be misspelled. If not, then the TargetFrameworkIdentifier and/or TargetFrameworkVersion properties must be s pecified explicitly. [C:\BTR\OpenSource\InterpolatedSql\src\InterpolatedSql\InterpolatedSql.csproj::TargetFramework="netstandard2.0]

C:\Program Files\dotnet\sdk\7.0.403\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.TargetFrameworkInference.targets(92,5): error NETSDK1013: The TargetFramework value 'net7.0"' was not recognized. It may be misspelled. If not, then the TargetFrameworkIdentifier and/or TargetFrameworkVersion properties must be specified explicitly. [C:\BTR\OpenSource\InterpolatedSql\src\InterpolatedSql\InterpolatedSql.csproj::TargetFramework=net7.0"]

Second error is definitely strange to me as I have .net 7.0 installed (it's current version we are using in my work project).

Then I removed netstandard2.0; and net7.0 and get:

C:\Program Files\dotnet\sdk\7.0.403\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.TargetFrameworkInference.targets(92,5): error NETSDK1013: The TargetFramework value '"net462' was not recognized. It may be misspelled. If not, then the TargetFrameworkIdentifier and/or TargetFrameworkVersion properties must be specified explicitly. [C:\BTR\OpenSource\InterpolatedSql\src\InterpolatedSql\InterpolatedSql.csproj::TargetFramework="net462]

C:\Program Files\dotnet\sdk\7.0.403\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.TargetFrameworkInference.targets(92,5): error NETSDK1013: The TargetFramework value 'net6.0"' was not recognized. It may be misspelled. If not, then the TargetFrameworkIdentifier and/or TargetFrameworkVersion properties must be specified explicitly. [C:\BTR\OpenSource\InterpolatedSql\src\InterpolatedSql\InterpolatedSql.csproj::TargetFramework=net6.0"]

So I don't think I have all the .net legacy/current versions installed to support using build.ps1. I was just going to manually run:

    dotnet build -c release InterpolatedSql.Tests\InterpolatedSql.Tests.csproj
    dotnet test  InterpolatedSql.Tests\InterpolatedSql.Tests.csproj

And I can successfully build the project, but trying to run the tests returns:

Testhost process for source(s) 'C:\BTR\OpenSource\InterpolatedSql\src\InterpolatedSql.Tests\bin\Debug\net5.0\InterpolatedSql.Tests.dll' exited with error: You must install or update .NET to run this application.
App: C:\BTR\NugetPackages\microsoft.testplatform.testhost\15.8.0\lib\netstandard1.5\testhost.dll
Architecture: x64
Framework: 'Microsoft.NETCore.App', version '5.0.0' (x64)
.NET location: C:\Program Files\dotnet\
The following frameworks were found:
  6.0.18 at [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  6.0.20 at [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  6.0.24 at [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  7.0.13 at [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Learn about framework resolution:
https://aka.ms/dotnet/app-launch-failed
To install missing framework, download:
https://aka.ms/dotnet-core-applaunch?framework=Microsoft.NETCore.App&framework_version=5.0.0&arch=x64&rid=win10-x64
. Please check the diagnostic logs for more information.

Test Run Aborted.

Can I just modify the test *.csproj temporarily to remove all target frameworks except for 6?

terryaney commented 11 months ago

I've created a PR as you've probably seen. I was able to temporarily disable the target frameworks that I don't have installed on my laptop to build/test the project without commiting those changes obviously. Let me know what you think.