Add API to configure database types for DbFunction's parameters

[ajcvickers]

See #13736 (comment) for the setup and the conversion.

I attempted to map my own function like this:

    public static class GeoExtensions
    {
        public static double Distance(this GeoPoint x, GeoPoint y)
            => throw new NotImplementedException();
    }

which is based on the GeoPoint mapping in the comment linked above.

My attempt was this:

modelBuilder.HasDbFunction(
    typeof(GeoExtensions).GetMethod(nameof(GeoExtensions.Distance)),
    b => b.HasTranslation(
        e => new SqlFunctionExpression(
            e.First(),
            "STDistance",
            typeof(double),
            e.Skip(1))));

This failed here because there is no mapping registered for GeoPoint.

This would likely be fixed by #10784, but even then it probably should be possible to make work with a custom mapping.

[ajcvickers]

Triage: this is too risky to try to fix for 2.12.2 and probably requires better type inference and/or the ability to explicitly associate type mappings with parameters in translation--the current code only allows type mappings to be applied for SQL generation if translation can succeed without them.

A workaround is below, but it won't work with preview3 due to other bugs to be fixed for RTM. Also, the workaround requires using internal code, which make break in subsequent releases.

public static class GeoExtensions
{
    public static double Distance(this GeoPoint x, GeoPoint y)
        => throw new NotImplementedException();
}

public struct GeoPoint
{
    public GeoPoint(double lat, double lon)
    {
        Lat = lat;
        Lon = lon;
    }
    public double Lat { get; }
    public double Lon { get; }
}

public class House
{
    public int Id { get; set; }
    public GeoPoint  Location  { get; set; }
}

public class GeoPointConverter : ValueConverter<GeoPoint, IPoint>
{
    public static readonly GeoPointConverter Instance = new GeoPointConverter();

    private static readonly IGeometryFactory _geoFactory
        = NtsGeometryServices.Instance.CreateGeometryFactory(srid: 4326);

    public GeoPointConverter()
        : base(
            v => _geoFactory.CreatePoint(new Coordinate(v.Lon, v.Lat)),
            v => new GeoPoint(v.Y, v.X))
    {
    }
}

public class BloggingContext : DbContext
{
    protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
        => optionsBuilder
            .ReplaceService<IRelationalTypeMappingSource, ReplacementTypeMappingSource>()
            .UseSqlServer(
                @"Server=(localdb)\mssqllocaldb;Database=Test;ConnectRetryCount=0",
                b => b.UseNetTopologySuite());

    protected override void OnModelCreating(ModelBuilder modelBuilder)
    {
        modelBuilder
            .Entity<House>()
            .Property(e => e.Location)
            .HasConversion(GeoPointConverter.Instance);

        modelBuilder.HasDbFunction(
            typeof(GeoExtensions).GetMethod(nameof(GeoExtensions.Distance)),
            b => b.HasTranslation(
                e => new SqlFunctionExpression(
                    e.First(),
                    "STDistance",
                    typeof(double),
                    e.Skip(1))));
    }
}

public class ReplacementTypeMappingSource : SqlServerTypeMappingSource
{
    public ReplacementTypeMappingSource(
        TypeMappingSourceDependencies dependencies,
        RelationalTypeMappingSourceDependencies relationalDependencies)
        : base(dependencies, relationalDependencies)
    {
    }

    protected override RelationalTypeMapping FindMapping(in RelationalTypeMappingInfo mappingInfo)
        => mappingInfo.ClrType == typeof(GeoPoint)
           || "geography".Equals(mappingInfo.StoreTypeName, StringComparison.OrdinalIgnoreCase)
            ? (RelationalTypeMapping)base.FindMapping(typeof(IGeometry))
                .Clone(GeoPointConverter.Instance)
            : base.FindMapping(mappingInfo);
}

public class Program
{
    public static void Main()
    {
        using (var context = new BloggingContext())
        {
            context.Database.EnsureDeleted();
            context.Database.EnsureCreated();

            context.Add(new House { Location = new GeoPoint(-122.34877, 47.6233355) });
            context.Add(new House { Location = new GeoPoint(-122.3308366, 47.5978429) });

            context.SaveChanges();
        }

        using (var context = new BloggingContext())
        {
            var distances = context
                .Set<House>().Select(e => e.Location.Distance(new GeoPoint(-121.3308366, 46.5978429)))
                .ToList();
        }
    }
}

[andriysavin]

this is too risky to try to fix for 2.1

Did you mean 2.2?
Thanks for the workaround.

[andriysavin]

@ajcvickers Since 2.2 is out I tried your workaround. Unfortunately, I get the following exception:

Unhandled Exception: Microsoft.EntityFrameworkCore.DbUpdateException: An error occurred while updating the entries. See the inner exception for details. ---> System.Data.SqlClient.SqlException: A .NET Framework error occurred during execution of user-defined routine or aggregate "geography":
System.FormatException: One of the identified items was in an invalid format.
System.FormatException:
.
The statement has been terminated.
   at System.Data.SqlClient.SqlConnection.OnError(SqlException exception, Boolean breakConnection, Action`1 wrapCloseInAction)
   at System.Data.SqlClient.TdsParser.ThrowExceptionAndWarning(TdsParserStateObject stateObj, Boolean callerHasConnectionLock, Boolean asyncClose)
   at System.Data.SqlClient.TdsParser.TryRun(RunBehavior runBehavior, SqlCommand cmdHandler, SqlDataReader dataStream, BulkCopySimpleResultSet bulkCopyHandler, TdsParserStateObject stateObj, Boolean& dataReady)
   at System.Data.SqlClient.SqlDataReader.TryConsumeMetaData()
   at System.Data.SqlClient.SqlDataReader.get_MetaData()
   at System.Data.SqlClient.SqlCommand.FinishExecuteReader(SqlDataReader ds, RunBehavior runBehavior, String resetOptionsString)
   at System.Data.SqlClient.SqlCommand.RunExecuteReaderTds(CommandBehavior cmdBehavior, RunBehavior runBehavior, Boolean returnStream, Boolean async, Int32 timeout, Task& task, Boolean asyncWrite, SqlDataReader ds)
   at System.Data.SqlClient.SqlCommand.ExecuteReader(CommandBehavior behavior)
   at Microsoft.EntityFrameworkCore.Storage.Internal.RelationalCommand.Execute(IRelationalConnection connection, DbCommandMethod executeMethod, IReadOnlyDictionary`2 parameterValues)
   at Microsoft.EntityFrameworkCore.Storage.Internal.RelationalCommand.ExecuteReader(IRelationalConnection connection, IReadOnlyDictionary`2 parameterValues)
   at Microsoft.EntityFrameworkCore.Update.ReaderModificationCommandBatch.Execute(IRelationalConnection connection)
   --- End of inner exception stack trace ---
   at Microsoft.EntityFrameworkCore.Update.ReaderModificationCommandBatch.Execute(IRelationalConnection connection)
   at Microsoft.EntityFrameworkCore.Update.Internal.BatchExecutor.Execute(DbContext _, ValueTuple`2 parameters)
   at Microsoft.EntityFrameworkCore.SqlServer.Storage.Internal.SqlServerExecutionStrategy.Execute[TState,TResult](TState state, Func`3 operation, Func`3 verifySucceeded)
   at Microsoft.EntityFrameworkCore.Update.Internal.BatchExecutor.Execute(IEnumerable`1 commandBatches, IRelationalConnection connection)
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.StateManager.SaveChanges(IReadOnlyList`1 entriesToSave)
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.StateManager.SaveChanges(Boolean acceptAllChangesOnSuccess)
   at Microsoft.EntityFrameworkCore.DbContext.SaveChanges(Boolean acceptAllChangesOnSuccess)
   at TestSQLSpatialQueries.Program.Main() in C:\Users\andsav\Code\Projects\Temp\TestSQLSpatialQueries\TestSQLSpatialQueries\Program.cs:line 107

[ajcvickers]

@andriysavin Unfortunately there were additional complexities in the function mapping design that meant we were not able to make this work for 2.2.

[andriysavin]

@ajcvickers You mean even the workaround?

[ajcvickers]

@andriysavin Yes.

[smitpatel]

@pmiddleton - I vaguely remember you had API to configure parameters in UDFs which we stripped out before committing. Do you remember or should I find the PR?

[pmiddleton]

@smitpatel - I originally did have the ability to add parameters. I can't find the version that was part of that PR. I best I have been able to find is an older version when the code was still in the main EFCore assembly and

You can see that here.

https://github.com/pmiddleton/EntityFramework/blob/udfSlim2/src/EFCore/Metadata/Internal/DbFunctionParameter.cs

I think the version that got converted to be in EFCore.Relational might be lost.

Is there something I can help you with?

[smitpatel]

That should be fine. Basically, for this issue, we need ability to configure parameter's column type on server side. Something like, modelBuilder.HasDbFunction(...).HasParameter(...).HasColumnType("geography") so that when translating we can use appropriate type mapping.
This is certainly not urgent issue, I just wanted to get thoughts of yours if you have any other ideas in mind. Implementation wise, we would do it when we get there in new pipeline.

[ajcvickers]

@pmiddleton We're still interested in this functionality, so if you have ideas we would be happy to discuss/consider PRs.

[pmiddleton]

@ajcvickers - I dug around on my pc and found the rest of the db function parameter code. I will work on a PR to get the parameter logic added back in. Assuming I ever thaw out again.... -28 degrees here.

[pmiddleton]

@smitpatel @ajcvickers, @divega

I just pulled latest and it appears the build system has changed a bit for 3.0 and I can't get the build script to work.

When I try to run the build script I just get a "build failure" error with no reason why. I tried adding the -v flag but that just spit out the command line that failed. KoreBuild is installing the 3.0.100-preview-010010 sdk to a .dotnet directory local to the project (not in userprofile as it use to).

I also tried building in VS and get the error "Unable to locate the .NET Core SDK. Check that it is installed and that the version specified in global.json (if any) matches the installed version."

I've also tried making a fresh clone but that has the same issue.

Is there something else that I need to setup, or a way to get a better error message to know what is failing?

[AndriySvyryd]

@pmiddleton Try running git clean -xdf before running build.cmd.
Afterwards you can open the solution using startvs.cmd EFCore.Runtime.sln

[pmiddleton]

@andriysavin - Still no go on the build script. However opening EFCore.Runtime.sln did get me a different error.

Version 3.0.100-preview-010010 of the .NET Core SDK requires at least version 16.0.0 of MSBuild. The current available version of MSBuild is 15.9.21.664.

The version of msbuild.dll included in the 3.0 framework is 16. However it seems VS isn't wanting to load that version up.

Maybe everything is just too frozen still from the polar vortex :)

[pmiddleton]

Do I need VS 2019 or will 2017 still work?

[pmiddleton]

Ok I got it. This was strange - I was missing %USERPROFILE%/.dotnet/NuGetFallbackFolder Once I added that empty folder the build script started working and then VS would build after that.

[AndriySvyryd]

VS 2019 is preferred, but VS 2017 should still work for a little while.

[andriysavin]

Hi folks. I'm wondering if this was finally fixed in preview 7? Because I tried this sample project (after fixing NTS API breaking changes in latest version) without that ReplacementTypeMappingSource and I'm still getting this error:

System.InvalidOperationException: 'The parameter 'x' for the DbFunction 'GeoExtensions.Distance' has an invalid type 'GeoPoint'. Ensure the parameter type can be mapped by the current provider.'

This is with preview 7.

It might be important to quote this part which was affected by api change:

  modelBuilder.HasDbFunction(
                typeof(GeoExtensions).GetMethod(nameof(GeoExtensions.Distance)),
                b => b.HasTranslation(
                    e => new SqlFunctionExpression(
                        e.First(),
                        "STDistance",
                        e.Skip(1), 
                        typeof(double),
                        typeMapping: null)));

Pay attention to typeMapping: null. I couldn't find better overload.

BTW, the workaround still doesn't work as well, returning me some SQL Server-side error on saving changes:

Microsoft.Data.SqlClient.SqlException (0x80131904): A .NET Framework error occurred during execution of user-defined routine or aggregate "geography":
System.FormatException: One of the identified items was in an invalid format.
System.FormatException:
.
The statement has been terminated.

[divega]

@smitpatel any comments on #13752 (comment) is this something that was supposed to be addressed by the fix or are additional calls required, e.g. to explicitly configure the store type.

[smitpatel]

HasDbFunction(..).HasParameter("name").HasStoreType("geography or whatever") and it will work.

[andriysavin]

@smitpatel Wow, that function mapping works (doesn't fail during configuration), thanks! I have one usability question, though.
First of all I'm curious how does EF know how to convert my GeoPoint type to NTS Point to then convert to geoprgahy when we call that function? We're not setting up any conversion for the parameters (and in the workaround we did). But if it's able to do so, then why do I have to configure store type for each function parameter while EF already knows that GeoPoint=>Point=>geography and store type could be inferred automatically? Not that I'm too lazy to specify store type, but I have to reference function parameters by name, and this is what makes me uncomfortable. This is my code:

modelBuilder.HasDbFunction(
                typeof(GeoExtensions).GetMethod(nameof(GeoExtensions.Distance)),
                builder =>
                {
                    builder.HasName("STDistance");
                    builder.HasParameter("x").HasStoreType("geography");
                    builder.HasParameter("y").HasStoreType("geography");
                });

Now, the bad part. When I execute the following simple query (from the sample code above)

var distances = context
                    .Houses.Select(e => e.Location.Distance(new GeoPoint(-121.3308366, 46.5978429)))
                    .ToList();

on a DB without any rows I get this error:

System.InvalidOperationException: 'Null TypeMapping in Sql Tree'

I feel that this is related to the GeopPoint=>Point=>geography conversion I mentioned earlier.

The place where it happens:

The stack trace:

   at Microsoft.EntityFrameworkCore.Relational.Query.Pipeline.RelationalSqlTranslatingExpressionVisitor.SqlTypeMappingVerifyingExpressionVisitor.VisitExtension(Expression node)
   at System.Linq.Expressions.Expression.Accept(ExpressionVisitor visitor)
   at System.Linq.Expressions.ExpressionVisitor.Visit(Expression node)
   at Microsoft.EntityFrameworkCore.Relational.Query.Pipeline.SqlExpressions.SqlFunctionExpression.VisitChildren(ExpressionVisitor visitor)
   at System.Linq.Expressions.ExpressionVisitor.VisitExtension(Expression node)
   at Microsoft.EntityFrameworkCore.Relational.Query.Pipeline.RelationalSqlTranslatingExpressionVisitor.SqlTypeMappingVerifyingExpressionVisitor.VisitExtension(Expression node)
   at System.Linq.Expressions.Expression.Accept(ExpressionVisitor visitor)
   at System.Linq.Expressions.ExpressionVisitor.Visit(Expression node)
   at Microsoft.EntityFrameworkCore.Relational.Query.Pipeline.RelationalSqlTranslatingExpressionVisitor.Translate(Expression expression)
   at Microsoft.EntityFrameworkCore.Relational.Query.Pipeline.RelationalProjectionBindingExpressionVisitor.Visit(Expression expression)
   at Microsoft.EntityFrameworkCore.Relational.Query.Pipeline.RelationalProjectionBindingExpressionVisitor.Translate(SelectExpression selectExpression, Expression expression)
   at Microsoft.EntityFrameworkCore.Relational.Query.Pipeline.RelationalQueryableMethodTranslatingExpressionVisitor.TranslateSelect(ShapedQueryExpression source, LambdaExpression selector)
   at Microsoft.EntityFrameworkCore.Query.Pipeline.QueryableMethodTranslatingExpressionVisitor.VisitMethodCall(MethodCallExpression methodCallExpression)
   at Microsoft.EntityFrameworkCore.Relational.Query.Pipeline.RelationalQueryableMethodTranslatingExpressionVisitor.VisitMethodCall(MethodCallExpression methodCallExpression)
   at System.Linq.Expressions.MethodCallExpression.Accept(ExpressionVisitor visitor)
   at System.Linq.Expressions.ExpressionVisitor.Visit(Expression node)
   at Microsoft.EntityFrameworkCore.Query.Pipeline.QueryCompilationContext.CreateQueryExecutor[TResult](Expression query)
   at Microsoft.EntityFrameworkCore.Storage.Database.CompileQuery[TResult](Expression query, Boolean async)
   at Microsoft.EntityFrameworkCore.Query.Internal.QueryCompiler.CompileQueryCore[TResult](IDatabase database, Expression query, IModel model, Boolean async)
   at Microsoft.EntityFrameworkCore.Query.Internal.QueryCompiler.<>c__DisplayClass9_0`1.<Execute>b__0()
   at Microsoft.EntityFrameworkCore.Query.Internal.CompiledQueryCache.GetOrAddQueryCore[TFunc](Object cacheKey, Func`1 compiler)
   at Microsoft.EntityFrameworkCore.Query.Internal.CompiledQueryCache.GetOrAddQuery[TResult](Object cacheKey, Func`1 compiler)
   at Microsoft.EntityFrameworkCore.Query.Internal.QueryCompiler.Execute[TResult](Expression query)
   at Microsoft.EntityFrameworkCore.Query.Internal.EntityQueryProvider.Execute[TResult](Expression expression)
   at Microsoft.EntityFrameworkCore.Query.Internal.EntityQueryable`1.GetEnumerator()
   at System.Collections.Generic.List`1..ctor(IEnumerable`1 collection)
   at System.Linq.Enumerable.ToList[TSource](IEnumerable`1 source)

[smitpatel]

The thing is EF Core does not know how to do the conversion.
The original issue was, we did not know how to map parameter of any type unless mapped by provider. There are 2 ways to resolve it fully.

• Add global level additional type mapping to typemapper used by provider. See Mechanism/API to specify a default conversion for any property of a given type in the model #10784
• Allow all facets of type mapping (including valueconverter) to be configured on any functions parameter. Essentially mapping the parameter same way mapping any property.

As you noted, 2nd option is not great experience compared to the first.
Now, EF Core needs to know everything about how to map given parameter's ClrType if it has to send constant/parameter to server for such function. As an escape hatch or to unblock certain users, if you are using your DbFunction with one of your column as parameter (and never passing constant/parameter), then your column already has been already converted and stored on server side and we don't need any additional info while querying. So we added API to configure store type, which you specify when you are going to use a column only and you tell us, you will find the column to be this particular database type on server side. This would pass validation check and we will happily pass your column in the DbFunction and run query for you. Though since it does not work for parameter/constant, you hit the second issue.

[andriysavin]

@smitpatel Thanks for explaining! I think that both ways you mentioned would be helpful depending on situation. Any hope to see at least one of them implemented in 3.0? (I noted that the referenced issue mentions properties only, which I believe is not exactly what's needed).

Also with using the syntax like builder.HasName("STDistance"); I'm getting this error:

Microsoft.Data.SqlClient.SqlException: 'Cannot find either column "dbo" or the user-defined function or aggregate "dbo.STDistance", or the name is ambiguous.'

While using the SqlFunctionExpression just works...

[andriysavin]

Regarding the second error I was mentioning, SQL Server-side error on saving changes or on a query (with using the workaround):

Microsoft.Data.SqlClient.SqlException (0x80131904): A .NET Framework error occurred during execution of user-defined routine or aggregate "geography":
System.FormatException: One of the identified items was in an invalid format.
System.FormatException:
.
The statement has been terminated.

I finally found out that it was result of incorrectly specifying latitude and longitude. After swapping them the error doesn't appear anymore. Though I'm surprised that there was no validation error that latitude was out of allowed range (-120).

[smitpatel]

• The referenced issue (Mechanism/API to specify a default conversion for any property of a given type in the model #10784) has actually been expanded to include any type being used in property or anywhere else.
• The later has been partially implemented. And we are not planning to expand it further for 3.0
• HasName configures name of the function as UDF. Hence it will try to add dbo.STDistance. If you are mapping function to built-in function, then you would need to use HasTranslation API to construct SqlFunctionExpression

[andriysavin]

Thank you once more :)

BTW, regarding configuring method parameters, to avoid referencing parameter names a good alternative could be using an approach similar to what different test mock frameworks do, e.g. with Moq:

mock.Setup(foo => foo.Add(It.Is<int>(i => i % 2 == 0))).Returns(true); 

E.g:

modelBuilder.HasDbFunction(() => GeoExtensions.Distance(
     Has.Parameter<GeoPoint>().HasStoreType("geography"),
     Has.Parameter<GeoPoint>().HasStoreType("geography"))); 

or

modelBuilder.HasDbFunction(() => GeoExtensions.Distance(
    Has.Parameter<GeoPoint>(pb => pb.HasStoreType("geography")),
    Has.Parameter<GeoPoint>(pb => pb.HasStoreType("geography"))));