dotnet / efcore

EF Core is a modern object-database mapper for .NET. It supports LINQ queries, change tracking, updates, and schema migrations.
https://docs.microsoft.com/ef/
MIT License
13.73k stars 3.18k forks source link

Spatial types support without third-party dependencies in model #13736

Closed andriysavin closed 2 years ago

andriysavin commented 5 years ago

Announced spatial types support is a huge step forward. However, I find it not ideal at least in one way. The problem is that to use this feature my model has to take a dependency on a third-party library like NTS TopologySuite.

While one may find this acceptable if heavy spatial calculations are used in business code (compared to data access layer), often you need only very basic functionality or even just latitude/longitude pair of values to store some location. Then you want to be able to do spatial queries against a DB and that's it.

In such cases I would prefer having my own type for representing spatial location in my model, and even my own spatial math implementation. Other approach would be to define first-party abstraction in my model, and then have implementation separately based on existing libraries (like NTS TopologySuite).

So what I'd like to see from EF side is at least support for value conversion from my own spatial type to one of available spatial type implementations. I would expect some way of writing queries against my own custom types. Just as one of possible examples, suppose I have my GeoPoint class in my model, and I define some (empty) extension methods (somewhat similar to Database scalar function mapping) at repository implementation level which match (e.g. by signature) to methods in TopologySuite (e.g. double Distance(this GeoPoint x, GeoPoint y). In that case EF could by convention map such calls to TopologySuite method calls (with type conversion), and then translate to SQL as it already does.

Again, that's just one idea, and I'm sure EF team can invent more elegant way to configure such mapping. The only requirement is that my model doesn't have to know about any of the libraries EF uses to support spatial types (and about EF itself, of course).

bricelam commented 5 years ago

The GeoAPI.Core package contains only interfaces (no spatial implementation). You can depend on this in your domain model so only the library containing your DbContext class needs to depend on the full NetTopologySuite library.

public class Customer
{
    public int Id { get; set; }
    public IPoint Location { get; set; }
}

This lets you provide your own implementations of these types for testing or client-side code where full spatial support isn't required.

andriysavin commented 5 years ago

@bricelam Good point, but take a look at the IPoint interface and all interfaces it inherits. IGeometry forces me to implement huge number of members, and many of them reference other interfaces. I could talk about Interface Segregation Principle here, but it's obvious it's unreal to provide custom implementation here in acceptable time, and most of that implementation wouldn't be used.

So even if I try to forget (and I don't want to!) about need to reference a third-party library, this doesn't help me. Or am I missing something?

ajcvickers commented 5 years ago

@andriysavin Also, the architecture is such that you could provide your own types and your own implementation that mirrors how NTS interacts with EF but with different types. There are challenges with this for SQL Server especially because SqlClient cannot handle SQL Server spatial types on .NET Core, so there needs to be low-level code to read to and from the database. But the code to do that exists and could be copied by you into your own implementation.

andriysavin commented 5 years ago

@ajcvickers To explain my point better: what I like EF Core for (compared to EF6) is that it provides much better decoupling of model from infrastructural (read: DB or ORM-specific) code. In EF6 we had a DbGeography, which was coupling model to EF. With EF Core it's now a chance to do that differently. What current implementation provides is semantically the same as in EF6, but it gives (theoretically) broader choice of spatial implementations and makes it cross-platform. But my model still has to reference external types with huge amount of functionality I won't ever need in business code.

Now, I'm not suggesting to make possible my implementation in my model to replace all the infrastructure NTS provides and at the same time make no dependency on EF.

What I want is my model to just provide coordinates to EF, EF maps (e.g. via value conversion) them to NTS types, NTS does its hard work. I understand that I don't know all the hard work behind current spatial support, and what looks simple and elegant to me can be difficult to implement. But I can't not suggest it at least.

ajcvickers commented 5 years ago

@andriysavin Can you write some example code showing what the application code would look like using your approach? Also, please show what you would expect it to map to in the database.

andriysavin commented 5 years ago

Ok, will try to provide something.

ajcvickers commented 5 years ago

@andriysavin We talked about this some more; are you meaning something like this:

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
            .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);
    }
}

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 houses = context
                .Set<House>()
                .ToList();
        }
    }
}

If so, then we can make this work, but in trying it I found some bugs that need fixing. Also, I tried the function mapping too, but ran into more issue. I'll file some issues for these things and reference them here.

andriysavin commented 5 years ago

@ajcvickers Yes, you're reading my mind :) The only thing you're missing is doing spatial queries. Something like:

    // This is a set of operations to enable 
    // on GeoPoint when doing spatial queries.
    // This class is specific to spatial services 
    // provider (e.g. NTS) and should be defined at DbContext 
    // level. It's my responsibility to create such class.
    static class GeoPositionQueryExtensions
    {
        // This somehow gets translated to 
        // double GeoAPI.Geometries.IGeometry.Distance(IGeometry g)
        public static double Distance(
            this GeoPoint x, // Converted to IGeometry
            GeoPoint y) // Converted to IGeometry
        {
            throw new NotImplementedException("This is stub");
        }
    }

    using (var context = new BloggingContext())
            {
                var myLocation = new GeoPoint(30, 50);

                var houses = context
                    .Set<House>()
                   // Distance is the extension method defined above
                    .Where(house=> house.Location.Distance(myLocation) < 1000)
                    .ToList();
            }

Of course, direct translation to DB spatial functions can be done for Distance method, but I'm not sure if that's better or worse. Direct translation looks like another fundamental implementation of spatial functionality to me (without NTS). This approach would free me from NTS at all, but I'm not sure what are the drawbacks.

ajcvickers commented 5 years ago

@andriysavin For functions, see #13752,

For doing this without NTS, this is possible, but requires copying any modifying the binary serialization code to go from SQL Server native bytes to your types.

andriysavin commented 5 years ago

@ajcvickers Should I follow referenced issues to track progress on this?

ajcvickers commented 5 years ago

@andriysavin I guess that's up to you. 😄

andriysavin commented 5 years ago

@ajcvickers :) I meant since you're closing this where to watch progress ;)

ajcvickers commented 5 years ago

13750 is fixed and checked in. So, for the function mapping part you can follow #13752

andriysavin commented 5 years ago

Thanks!

MithrilMan commented 2 years ago

what's the current way to have a simple converter for a custom Point class? I've seen this https://github.com/dotnet/efcore/blob/6054beea26eb609b36709ab07276e073f5600842/test/EFCore.Specification.Tests/TestModels/SpatialModel/GeoPointConverter.cs

but it makes use of NetTopologySuite, isn't there a best way to just have a converter between geographic spatial point db type and a custom value object, without making use of 3rd party library?

(I don't need any spatial function in my query, I just need to store spatial data)

ajcvickers commented 2 years ago

@MithrilMan It's theoretically possible, but not trivial. It would need to use similar techniques to those used by the NTS plug-ins to deserialize raw SQL bytes into something else.

bricelam commented 2 years ago

@MithrilMan If you'd like to delve deeper, here's the library that reads and writes the bytes: NetTopologySuite.IO.SqlServerBytes.