Remove SkipGeographyFixup and SkipGeographyChecks

[bricelam]

After thinking about this a lot, I think we can remove the SqlServerBytesReader.SkipGeographyFixup and SqlServerBytesWriter.SkipGeographyChecks properties.

The other IO module always fixed up geography polygon rings on read. Doing so provides a much better developer experience, and I can't imagine the perf gains of disabling it would be significant.

Allowing you to read geography polygons based on the full globe probably isn't smart since NTS can't actually handle FullGlobe so all of the operation results will be wrong.

Always validating geography polygon rings is good as it will educate users. Disabling this check has the same dangers as the previous point. The resulting instance is different and will give very different results.

[airbreather]

The other IO module always fixed up geography polygon rings on read.

I tried to confirm this, but I could not... am I doing something wrong?

ConsoleApp0.csproj

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

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net472</TargetFramework>
  </PropertyGroup>

  <ItemGroup>
    <!-- The implicitly referenced version doesn't work on my machine. -->
    <PackageReference Include="Microsoft.SqlServer.Types" Version="14.0.1016.290" />
    <PackageReference Include="NetTopologySuite.IO.SqlServer" Version="1.15.0" />
    <PackageReference Include="NetTopologySuite.IO.SqlServerBytes" Version="1.15.0-pre001" />
  </ItemGroup>

</Project>

Program.cs

using System;
using System.Data.SqlTypes;
using System.IO;

using Microsoft.SqlServer.Types;

using NetTopologySuite.IO;

static class Program
{
    static void Main()
    {
        var ntsGeometry = new WKTReader().Read("POLYGON ((0 0, 3 0, 3 3, 0 3, 0 0), (1 1, 1 2, 2 2, 2 1, 1 1))");
        ntsGeometry.SRID = 4326;

        var originalBytes = new SqlServerBytesWriter().Write(ntsGeometry);
        var originalSql = SqlGeography.Deserialize(new SqlBytes(originalBytes));
        var oldVersionOriginal = new MsSql2008GeographyReader().Read(originalBytes);
        var newVersionOriginal = new SqlServerBytesReader().Read(originalBytes);

        var reorientSql = originalSql.ReorientObject();
        var reorientBytes = ToBytes(reorientSql);
        var oldVersionReorient = new MsSql2008GeographyReader().Read(reorientBytes);
        var newVersionReorient = new SqlServerBytesReader().Read(reorientBytes);

        Console.WriteLine($"ntsGeometry:        {ntsGeometry}");                // POLYGON ((0 0, 3 0, 3 3, 0 3, 0 0), (1 1, 1 2, 2 2, 2 1, 1 1))
        Console.WriteLine($"originalBytes:      {ToHexString(originalBytes)}"); // E610000001040A0000000000000000000000000000000000000000000000000008400000000000000000000000000000084000000000000008400000000000000000000000000000084000000000000000000000000000000000000000000000F03F000000000000F03F000000000000F03F0000000000000040000000000000004000000000000000400000000000000040000000000000F03F000000000000F03F000000000000F03F020000000200000000000500000001000000FFFFFFFF0000000003
        Console.WriteLine($"reorientBytes:      {ToHexString(reorientBytes)}"); // E610000001040A00000000000000000000000000000000000000000000000000000000000000000008400000000000000840000000000000084000000000000008400000000000000000000000000000000000000000000000000000000000000040000000000000F03F00000000000000400000000000000040000000000000F03F0000000000000040000000000000F03F000000000000F03F0000000000000040000000000000F03F020000000200000000000500000001000000FFFFFFFF0000000003

        Console.WriteLine();

        Console.WriteLine($"originalSql:        {originalSql}");        // POLYGON ((0 0, 0 3, 3 3, 3 0, 0 0), (1 1, 2 1, 2 2, 1 2, 1 1))
        Console.WriteLine($"oldVersionOriginal: {oldVersionOriginal}"); // POLYGON ((0 0, 0 3, 3 3, 3 0, 0 0), (1 1, 2 1, 2 2, 1 2, 1 1))
        Console.WriteLine($"newVersionOriginal: {newVersionOriginal}"); // POLYGON ((0 0, 3 0, 3 3, 0 3, 0 0), (1 1, 1 2, 2 2, 2 1, 1 1))

        Console.WriteLine();

        Console.WriteLine($"reorientSql:        {reorientSql}");        // POLYGON ((0 0, 3 0, 3 3, 0 3, 0 0), (1 2, 2 2, 2 1, 1 1, 1 2))
        Console.WriteLine($"oldVersionReorient: {oldVersionReorient}"); // POLYGON ((0 0, 3 0, 3 3, 0 3, 0 0), (1 2, 2 2, 2 1, 1 1, 1 2))
        Console.WriteLine($"newVersionReorient: {newVersionReorient}"); // POLYGON ((0 0, 0 3, 3 3, 3 0, 0 0), (2 1, 2 2, 1 2, 1 1, 2 1))
    }

    static byte[] ToBytes(SqlGeography geog)
    {
        using (var ms = new MemoryStream())
        {
            using (var wr = new BinaryWriter(ms))
            {
                geog.Write(wr);
            }

            return ms.ToArray();
        }
    }

    static string ToHexString(byte[] bytes) => BitConverter.ToString(bytes).Replace("-", string.Empty);
}

[bricelam]

Ah, I got confused. It silently fixes them on write but doesn't do anything on read.

After the PR, we'd check them on write, and use the CCW one as the shell on read.

[PabodhaWimalasuriya123]

@bricelam Hi I get this exception when saving multiple polygons. How can I set SkipGeographyChecks to true ?

When writing a SQL Server geography value, the shell of a polygon must be oriented counter-clockwise. To write polygons without a shell, set SkipGeographyChecks

Code sample :

var geometryFactory = NtsGeometryServices.Instance.CreateGeometryFactory(srid: 4326);
var poly = new Polygon[] {
new Polygon(new LinearRing(new Coordinate[]
{
        new Coordinate(19.2385498607974, -51.50390625,0),
        new Coordinate(24.1367281697474, -37.6171875,0),
        new Coordinate(13.8487471475372, -18.10546875,0),
        new Coordinate(19.2385498607974, -51.50390625,0),
                    })),
new Polygon(new LinearRing(new Coordinate[]
{
        new Coordinate(-10.0445849842118, -53.0859375,0),
        new Coordinate(4.13824308398371, -58.7109375,0),
        new Coordinate(2.20770545570541, -68.73046875,0),
        new Coordinate(-8.83079518432893, -79.1015625,0),
        new Coordinate(-17.3820949478775, -81.2109375,0),
        new Coordinate(-21.0332372344673, -51.328125,0),
        new Coordinate(-10.0445849842118, -53.0859375,0),
})) 
};

var currentLocation = geometryFactory.CreateMultiPolygon(poly) as MultiPolygon;
dbset.Polygons = currentLocation;
_context.Add(dbset);
await _context.SaveChangesAsync();

[airbreather]

Hi I get this exception when saving multiple polygons. How can I set SkipGeographyChecks to true ?

To give a little background, SQL Server has a stricter requirement about the orientation of your polygon rings than NTS does: the outer rings need to be counter-clockwise, and the inner rings need to be clockwise. NTS, on the other hand, doesn't care: as far as I know, everything would work just fine no matter which way your outer / inner rings are oriented.

So the problem we have here is what to do when you take polygons which are perfectly valid for NTS and try to save them to SQL Server, where they're not valid. One idea was to silently rewrite your input data to represent the same area in a way that SQL Server accepts. That idea felt dirty, because when you ask us to save something, you're probably not expecting it to look different when you pull it back out (and who knows, maybe you have a reason for them to be oriented that way!).

Instead, we decided (see more discussion on #5) not to fix the issue for you automatically, at least at this step of the process. Rather, if your polygons are valid for NTS but not for SQL Server, we would raise an error telling you about the issue, and leave it up to you to decide how to deal with it (and any other steps you might need to take that flow from this).

So you would have to do something like this (note a few other changes I've made: coordinates are 2d now since it doesn't look like you're using Z, and we're using the IGeometryFactory for everything):

var geometryFactory = NtsGeometryServices.Instance.CreateGeometryFactory(srid: 4326);
var ring1Coordinates = new[]
{
    new Coordinate(19.2385498607974, -51.50390625),
    new Coordinate(24.1367281697474, -37.6171875),
    new Coordinate(13.8487471475372, -18.10546875),
    new Coordinate(19.2385498607974, -51.50390625),
};
var ring2Coordinates = new[]
{
    new Coordinate(-10.0445849842118, -53.0859375),
    new Coordinate(4.13824308398371, -58.7109375),
    new Coordinate(2.20770545570541, -68.73046875),
    new Coordinate(-8.83079518432893, -79.1015625),
    new Coordinate(-17.3820949478775, -81.2109375),
    new Coordinate(-21.0332372344673, -51.328125),
    new Coordinate(-10.0445849842118, -53.0859375),
};
var multiPolygon = geometryFactory.CreateMultiPolygon(new[]
{
    geometryFactory.CreatePolygon(ring1Coordinates),
    geometryFactory.CreatePolygon(ring2Coordinates),
});

// ensure that the ring orientation matches what SQL Server requires.
var forSqlServer = multiPolygon.Normalized();
forSqlServer.Reverse();

// now do your stuff
dbset.Polygons = forSqlServer;
_context.Add(dbset);
await _context.SaveChangesAsync();

[airbreather]

Going back to your specific question, I've opened #6 to remove that confusing part of the error message.

[PabodhaWimalasuriya123]

@airbreather : Thank you.. It worked.. 👍

[IanKemp]

Rather, if your polygons are valid for NTS but not for SQL Server, we would raise an error telling you about the issue, and leave it up to you to decide how to deal with it (and any other steps you might need to take that flow from this).

And in 99.99999% of cases the answer to "how do you want to deal with it" is "I want it to just f**king work because I shouldn't need to care about the fact that SQL Server needs its geography in a specific format, because this package is supposed to take care of that for me".

In other words, in 99.99999% of cases, devs are now going to be forced to write:

var forSqlServer = myGeometryObject.Normalized();
forSqlServer.Reverse();

Forcing extra code to be written for the common case can best be described as a poor choice.

---

To make this case general and ensure that any object with a Geometry subtype is being written correctly, I now have to override SaveChanges in my DbContext, enumerate every added/modified entity and update their Geometry properties:

private IEnumerable<EntityEntry> GetAddedOrModifiedEntities()
    => ChangeTracker.Entries().ToList().Where(e => e.State == EntityState.Added || e.State == EntityState.Modified);

private IEnumerable<PropertyEntry> GetWritablePropertiesAssignableFrom<T>(EntityEntry entity)
    => entity.Properties.Where(p => typeof(T).IsAssignableFrom(p.Metadata.PropertyInfo.PropertyType)
        && p.Metadata.PropertyInfo.CanWrite);

public async override Task<int> SaveChangesAsync(bool acceptAllChangesOnSuccess, CancellationToken cancellationToken = default(CancellationToken))
{
    foreach (var entity in GetAddedOrModifiedEntities())
    {
        foreach (var property in GetWritablePropertiesAssignableFrom<Geometry>(entity))
        {
            property.CurrentValue = ((Geometry)property.CurrentValue).Normalized().Reverse();
        }
    }
}

Except... the above does nothing because the CurrentValue setter internally calls GeometryValueComparer<Polygon>.Equals, which calls Polygon.EqualsTopologically(oldValue, newValue) which returns true for a normalized reversed polygon, which means the new value is never set.

Fantastic.

@bricelam @airbreather Would you be kind enough to inform me how I can make this work as I expect?

[airbreather]

@bricelam @airbreather Would you be kind enough to inform me how I can make this work as I expect?

@IanKemp my recommendation would be to orient your polygon rings earlier than this, ideally as the last step of whatever processing you do when building the polygon (or, if possible, do the processing in a way that will naturally result in them being oriented this way). The polygon data you got had to come from somewhere. If it came from SQL Server and you haven't touched it since, then it should be good already. Otherwise, wherever you touched it, that's where the reorientation should happen.

SQL Server's geography type uses ring orientation, rather than defining rings as "exterior / interior" like NTS does, to determine which side of a ring is "inside" vs. "outside". When you ask us to send along a polygon with a particular "exterior" ring (as marked by NTS), but its coordinates are oriented in the "wrong" way, there are only so many ways we can respond:

1. Ignore the issue and just send your coordinates exactly as they appear in your polygon. .STAsText() in SQL Server should always match what NTS reports, and they will look the same after a round-trip, but depending on what seemingly "random" processes you're using to build the polygons in the first place, queries using that polygon may or may not sometimes report exactly the inverse of what they would do in NTS (and, probably, the inverse of what you expect).
2. Automatically "repair" the issue for you, by detecting this case and reorienting the rings to match what SQL Server's geography type expects them to be. This will make the queries work correctly, and they will be considered topologically equal, but original.EqualsExact(roundTrip) tests will fail, .STAsText() will report something different than what NTS would have reported before saving, and perhaps some other computed values might differ slightly (e.g., .InteriorPoint). Plus, rewriting the user's data just feels sketchy.
3. Detect the issue like we would do in the "repair" solution, but instead of rewriting anything, tell the user about the problem so that they can fix it in their data.

We went with the last one. Perhaps it could make sense for EF Core to allow the user to inject a handler, or even set an "opt-in" flag, to reorient the rings on-the-fly when we detect an issue, but IMO, it's primarily the user's responsibility to conform their data.

I suspect that even if we implemented an "opt in" flag, we'd still get some confusion about "why are my polygons different after saving / loading" because frankly it seems just really easy to forget that "5 months ago, I copy-pasted a line of code that I found on StackOverflow somewhere that made everything work".

[IanKemp]

@IanKemp my recommendation would be to orient your polygon rings earlier than this, ideally as the last step of whatever processing you do when building the polygon (or, if possible, do the processing in a way that will naturally result in them being oriented this way). The polygon data you got had to come from somewhere. If it came from SQL Server and you haven't touched it since, then it should be good already. Otherwise, wherever you touched it, that's where the reorientation should happen.

I strongly disagree with this argument. That SQL server requires polygons to be stored as it does is SQL server's problem, i.e. a concern of the persistence layer - what you are suggesting is to make it a concern of layers higher up the stack, which is completely the wrong thing to do because those layers don't and shouldn't need to know or care about the nitty-gritty of data persistence and retrieval.

And fundamentally, by throwing an exception what you are saying is "your data is wrong". But it's not - it's just that SQL Server chooses to store it in a way that may make it look wrong when you later retrieve it. Which, again, is an issue that no other RDMBS would have.

An EF hook to transform data on the way in/out of the persistence provider would solve this problem, because then I could just do the geo normalisation + reverse in a general manner there. (Which is essentially what I'm attempting to do in my DbContext above.) But it would be a very edge-case scenario and as such seems like something the EF team wouldn't be particularly interested in prioritising, which means I can't rely on it happening anytime soon.

SQL Server's geography type uses ring orientation, rather than defining rings as "exterior / interior" like NTS does, to determine which side of a ring is "inside" vs. "outside".

God that documentation is useless. The only reference to the storage format is the vague sentence "The interior of the polygon in an ellipsoidal system is defined by the left-hand rule" - there is nothing that says "your polygon has to be defined counterclockwise or SQL Server's EF provider will refuse to save it".

I suspect that even if we implemented an "opt in" flag, we'd still get some confusion about "why are my polygons different after saving / loading" because frankly it seems just really easy to forget that "5 months ago, I copy-pasted a line of code that I found on StackOverflow somewhere that made everything work".

I suspect rather less confusion with the option, as opposed to a lot of unhappiness without it, as currently.

[FObermaier]

I think @IanKemp has a point here. I think it can be easily solved by making NtsGeometryServices configurable in the sense that it can create OgcCompliantGeometryFactorys instead of the default ones. Unfortunatly there is no interface anymore ;-).

This would be a new issue over at NetTopologySuite, though.

For the time being, @IanKemp, can you force the Geometry.Factory being an OgcCompliantGeometryFactory. I suppose that this would solve your issue. That is, before you do anything with the geometry, so right after reading it.

[airbreather]

I think it can be easily solved by making NtsGeometryServices configurable in the sense that it can create OgcCompliantGeometryFactorys instead of the default ones. Unfortunatly there is no interface anymore ;-).

This would be a new issue over at NetTopologySuite, though.

NetTopologySuite/NetTopologySuite#354 logged, I'll get it fixed. Sorry, blame me for this one.

[FObermaier]

Sorry, blame me for this one.

@airbreather, just kidding, I really appreciate how you pushed NTS v2.

[airbreather]

I've opened #9 for further discussion on the topic of reconsidering the decision not to reorient the rings at this stage of the process. This is to give the issue more visibility than a comment thread on an issue that was resolved almost a year ago.

[3263927]

but how to deal with it if i have user defined polygons, and i dont know is it clockwise or anticlockwise? how i can check it out and reverse only clockwise polygons?

[galvesribeiro]

Just to drop my 2c here. It is 2021, almost 3 years and the same problem exist on .Net 5.

I'm pulling data from OpenStreetMaps like this: https://nominatim.openstreetmap.org/details.php?polygon_geojson=1&format=json&osmtype=R&osmid=113314

And then creating Polygon or MultiPolygon depending on the Country/City/State areas and trying to persist with NTS + EFCore 5.

The exact same code to write/read from the database those polygons works perfectly on PostgreSQL/PostGIS but it fail miserably on SQL Server.

If I try to insert the polygon without any transformation, I get an exception saying it should be Counter Clockwise on SaveChangesAsync() call.

If I call .Normalized().Reverse() as suggested here, the SaveChangesAsync() works, however, when querying the data like this:

var austin = geoFactory.CreatePoint(new Coordinate(lon, lat));
...
var entries = await ctx.Map.AsNoTracking().Where(m => m.Area.Contains(austin)).ToListAsync();

It fail miserably when reading saying that I should call MakeValid().

Tried other approaches like creating the geometry factory this:

var geoFactory = new GeometryFactoryEx(new PrecisionModel(PrecisionModels.Floating), 4326) { OrientationOfExteriorRing = LinearRingOrientation.CCW };

Same thing...

I wonder if that would ever get a fix or a least a clear a precise workaround for SQL Server users...

[FObermaier]

@galvesribeiro, I'm not familiar with EF at all so it would be very helpful if you could set up a minimal unit test to show us what you are doing and reproduce the failure you are getting. Thanks.