{"action":"created","issue":{"url":"https://api.github.com/repos/npgsql/npgsql/issues/2255","repository_url":"https://api.github.com/repos/npgsql/npgsql","labels_url":"https://api.github.com/repos/npgsql/npgsql/issues/2255/labels{/name}","comments_url":"https://api.github.com/repos/npgsql/npgsql/issues/2255/comments","events_url":"https://api.github.com/repos/npgsql/npgsql/issues/2255/events","html_url":"https://github.com/npgsql/npgsql/issues/2255","id":388612872,"node_id":"MDU6SXNzdWUzODg2MTI4NzI=","number":2255,"title":"NetTopologySuite behavior change in 4.0.4","user":{"login":"roji","id":1862641,"node_id":"MDQ6VXNlcjE4NjI2NDE=","avatar_url":"https://avatars0.githubusercontent.com/u/1862641?v=4","gravatar_id":"","url":"https://api.github.com/users/roji","html_url":"https://github.com/roji","followers_url":"https://api.github.com/users/roji/followers","following_url":"https://api.github.com/users/roji/following{/other_user}","gists_url":"https://api.github.com/users/roji/gists{/gist_id}","starred_url":"https://api.github.com/users/roji/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/roji/subscriptions","organizations_url":"https://api.github.com/users/roji/orgs","repos_url":"https://api.github.com/users/roji/repos","events_url":"https://api.github.com/users/roji/events{/privacy}","received_events_url":"https://api.github.com/users/roji/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":4,"created_at":"2018-12-07T11:19:50Z","updated_at":"2018-12-07T13:49:06Z","closed_at":null,"author_association":"MEMBER","body":"Yesterday @austindrenski and I worked on releasing Npgsql 4.0.4 and EFCore.PG 2.2.0. 4.0.4 is now out and available on nuget.org, but we hit some spatial issues. Sorry for the length of the write-up, I just wanted to sum everything up...\r\n\r\nWhen `INpgsqlTypeMapper.UseNetTopologySuite()` is called a null `coordinateSequenceFactory`, the default coordinate sequence factory is `CoordinateArraySequenceFactory` (this is how `NtsGeometryService` is set up). When `handleOrdinates` is null as well, the ordinates are taken from `coordinateSequenceFactory`, and for `CoordinateArraySequenceFactory` that's `XYZ` by default. So it would seem that setting up NetTopologySuite with the absolute default settings leads to 3D geometry rather than the more common 2D geometry.\r\n\r\nNote that [in Npgsql's NetTopologySuite tests](https://github.com/npgsql/Npgsql/blob/de6b6721413e671629d027cdaf7e14c83e22c4f3/test/Npgsql.PluginTests/NetTopologySuiteTests.cs#L68), we explicitly specify to use `DotSpatialAffineCoordinateSequenceFactory` instead of the default `CoordinateArraySequenceFactory`, and we always specify ordinates explicitly as well. This is why we haven't experienced any unexpected behavior there.\r\n\r\nNow, Npgsql.NetTopologySuite 4.0.4 includes @YohDeadfall commit, a3c25eb613d6cc9b98164cf4bd9913a533649c66, which flows the user-provided ordinates (or the default one) into the PostgisWriter. Before this commit, PostgisWriter always had `Ordinates.None`, which made it [look at the actually geometry to be written](https://github.com/NetTopologySuite/NetTopologySuite.IO.PostGis/blob/master/NetTopologySuite.IO.PostGis/PostGisWriter.cs#L506), and see which ordinates it has. In other words, it seems like setting the Ordinates on PostGisWriter is more of a forced override - the class knows how to write the correct number of ordinates based on the geometry objects it receives. Even worse, if you give it an XY point, but you manually set ordinates to XYZ, [it will send a zero (?) for the missing Z coordinate](https://github.com/NetTopologySuite/NetTopologySuite.IO.PostGis/blob/master/NetTopologySuite.IO.PostGis/PostGisWriter.cs#L506).\r\n\r\nOn the read side we have some seemingly odd behavior... If we set PostGisReader's Ordinates to XYZ, it will always return geometry objects with XYZ, and leave the Z null if it was not sent by the server. I would have expected to return objects with ordinates matching what was sent from the server.\r\n\r\nCombining the above we have the following bad behavior:\r\n\r\n```c#\r\nusing (var cmd = new NpgsqlCommand(\"SELECT ST_AsText(@p)\", conn))\r\n{\r\n    cmd.Parameters.AddWithValue(\"p\", new Point(1, 2400));\r\n    Console.WriteLine(cmd.ExecuteScalar());\r\n    // Shows POINT(1 2400) before a3c25eb613, POINT Z (1 2400 0) after\r\n}\r\n\r\nusing (var cmd = new NpgsqlCommand(\"SELECT GEOMETRY 'POINT(1 2400)'\", conn))\r\n{\r\n    var point = (Point)cmd.ExecuteScalar();\r\n\r\n    Console.WriteLine(point);  // Shows POINT (1 2400)\r\n    Console.WriteLine(point.CoordinateSequence.Ordinates);  // Shows XYZ :/\r\n}\r\n\r\nusing (var cmd = new NpgsqlCommand(\"SELECT @p\", conn))\r\n{\r\n    cmd.Parameters.AddWithValue(\"p\", new Point(1, 2400));\r\n    var point = (Point)cmd.ExecuteScalar();\r\n\r\n    Console.WriteLine(point);  // Shows POINT (1 2400 0), note the zero!\r\n    Console.WriteLine(point.CoordinateSequence.Ordinates);  // Shows XYZ\r\n}\r\n```\r\n\r\nTo summarize, I think we should consider rolling back a3c25eb613 and releasing version 4.0.4.1 of Npgsql.NetTopologySuite. @YohDeadfall is it possible to get some feedback from you on this, as soon as possible? Same for @austindrenski...\r\n"},"comment":{"url":"https://api.github.com/repos/npgsql/npgsql/issues/comments/445237905","html_url":"https://github.com/npgsql/npgsql/issues/2255#issuecomment-445237905","issue_url":"https://api.github.com/repos/npgsql/npgsql/issues/2255","id":445237905,"node_id":"MDEyOklzc3VlQ29tbWVudDQ0NTIzNzkwNQ==","user":{"login":"roji","id":1862641,"node_id":"MDQ6VXNlcjE4NjI2NDE=","avatar_url":"https://avatars0.githubusercontent.com/u/1862641?v=4","gravatar_id":"","url":"https://api.github.com/users/roji","html_url":"https://github.com/roji","followers_url":"https://api.github.com/users/roji/followers","following_url":"https://api.github.com/users/roji/following{/other_user}","gists_url":"https://api.github.com/users/roji/gists{/gist_id}","starred_url":"https://api.github.com/users/roji/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/roji/subscriptions","organizations_url":"https://api.github.com/users/roji/orgs","repos_url":"https://api.github.com/users/roji/repos","events_url":"https://api.github.com/users/roji/events{/privacy}","received_events_url":"https://api.github.com/users/roji/received_events","type":"User","site_admin":false},"created_at":"2018-12-07T13:49:06Z","updated_at":"2018-12-07T13:49:06Z","author_association":"MEMBER","body":"Upon reexamination, PostGisWriter seem to have a bug - the constructor sets `HandleOrdinates` to `None`, which should make subsequent writes detect the ordinates on the given geometry object with `CheckOrdinates()`, as I wrote above. However, `HandleOrdinates` is a property which always ORs XY to the given value, so it's impossible for the ordinates to be `None`, and `CheckOrdinates()` never gets called.\r\n\r\nI am copying the source of PostGisWriter into our plugin so it can be patched, and will submit PRs to the NTS folks to fix this upstream."}}
{"actor":{"display_login":"roji"}}