lib/udt.js: parsePoints() flips latitude and longitude

jeremy-w commented 3 years ago

https://github.com/tediousjs/node-mssql/blob/5b456f8f3e4906d9bd7451adcdd8c599d20b54e3/lib/udt.js#L67-L68 shows it reading the values into X then Y.

but X is longitude, and Y is latitude.

and a geography::Point orders its values as latitude (Y), and then longitude (X). https://docs.microsoft.com/en-us/openspecs/sql_server_protocols/ms-ssclrt/9911c186-a699-485e-ac77-6800a11da361

(unlike a geometry::Point, which does do X then Y. https://docs.microsoft.com/en-us/openspecs/sql_server_protocols/ms-ssclrt/47f5ad86-257d-4134-bc90-9129e2f594e9 )

Expected behaviour:

If I insert a value as geography::Point(90, 180, 4120) with latitude y=90, longitude x=+180, and then select the field back, the parsed UDT should look something like:

{
  srid: 4120,
  points: [
    { y: 90, x: 180 }
  ]
}

Actual behaviour:

The x and y are flipped, even though 180 is not a valid value for y (aka northing aka latitude).

Configuration:

(I don't know what you expect here.)

Software versions

NodeJS: v12.19.0
mssql: 6.2.3
SQL Server: 2019 developer edition
Knex: 0.21.12

dhensby commented 3 years ago

As an initial point, your knex version is old and knex no longer makes use of mssql lib as a driver.

I'm a bit confused looking at the docs and reconciling them with this bug report.

Latitude slice the globe from the equator going round the globe, longitude are the lines that go from pole-to-pole on the globe. From a geomatory/graphing perspective, if you use the poles as the "top" and "bottom" of the "graph" then the x-axis is latitude and the y-axis is longitude.

In the docs I'm seeing geometry stored as (x,y) and [geography stored as (lat,long) ie: (x,y). So it doesn't seem like the output of the query is incorrect to me.

It would be wholly bizarre design choice for MS to have decided that Geography points would be stored (y,x) but geometry as (x,y) - though you can't rule that out, of course - but if it is as documented then everything seems in order.

jeremy-w commented 3 years ago

Latitude is the Y axis (northing). One mnemonic is that latitude is like a ladder going up and down.

X and Y are reversed between the Geometry vs Geography structs. It is confusing, and easy to reverse, and different point formats switch them all the time: compare the argument order of geography::Point https://docs.microsoft.com/en-us/sql/t-sql/spatial-geography/point-geography-data-type?view=sql-server-ver15 against the WKT Point ordering used by geography::STPointFromText. (AFAICT, the OGC API is uniform in using x then y, but Microsoft’s CLR types appear to have gone with Lat then Long ordering for Geography.)

The best way I’ve found to detect reversal when inputting points is to take advantage of Longitude’s range exceeding the valid range for Latitude by inserting a maximal test point of 90 N, 180 E. If the two arguments are reversed, then you get a convenient range error from the CLR assembly. (Accidentally tripping a range error is how I originally noticed the inversion.)

You can confirm the reversal for yourself by comparing lib/udt’s parsed geo struct repr against the result of using the accessor properties provided by SQL Server by selecting geo.STPointN(1).Lat as Lat and geo.STPointN(1).Long as Long.

(I am aware my Knex version is older; it was long pinned by Objection, which only in the last week released a new alpha major version that unlocks upgrading https://github.com/Vincit/objection.js/pull/1992#issuecomment-883906897.)

Jeremy W. Sherman http://jeremywsherman.com/

On Jul 24, 2021, at 09:41, Daniel Hensby @.***> wrote:

As an initial point, your knex version is old and knex no longer makes use of mssql lib as a driver.

I'm a bit confused looking at the docs and reconciling them with this bug report.

Latitude slice the globe from the equator going round the globe, longitude are the lines that go from pole-to-pole on the globe. From a geomatory/graphing perspective, if you use the poles as the "top" and "bottom" of the "graph" then the x-axis is latitude and the y-axis is longitude.

In the docs I'm seeing geometry stored as (x,y) and [geography stored as (lat,long) ie: (x,y). So it doesn't seem like the output of the query is incorrect to me.

It would be wholly bizarre design choice for MS to have decided that Geography points would be stored (y,x) but geometry as (x,y) - though you can't rule that out, of course - but if it is as documented then everything seems in order.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe.

dhensby commented 3 years ago

Ok right, so the lines of longitude run vertically, which denotes a point along an x-axis and vice versa for latitude.

I think the "fixing" of this problem would need to come in a new major release, though because it's pretty reasonable to expect people would have simply mapped X to long and y to lat.

Are you prepared to author a PR for this?

jeremy-w commented 3 years ago

Yes. I’ll give it a shot Monday. 👍🏽

Looks like aside from the logic change in lib/udt, I’ll also need to update:

the README example https://github.com/tediousjs/node-mssql/blob/master/README.md#geography-and-geometry
the Geography section of the common unit tests https://github.com/tediousjs/node-mssql/blob/master/test/common/unit.js#L293-L329

Are there any other files (test or implementation) impacted that I’m missing?

Jeremy W. Sherman http://jeremywsherman.com/

On Jul 24, 2021, at 13:47, Daniel Hensby @.***> wrote:

Ok right, so the lines of longitude run vertically, which denotes a point along an x-axis and vice versa for latitude.

I think the "fixing" of this problem would need to come in a new major release, though because it's pretty reasonable to expect people would have simply mapped X to long and y to lat.

Are you prepared to author a PR for this?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe.

dhensby commented 3 years ago

I think that will be it 👍

jeremy-w commented 3 years ago

Working on getting a happy test suite first.

It looks like, if an assertion fails in 'transaction with error', it doesn't get fed into Mocha, but instead trips an unhandled rejection + an eventual mocha timeout.

I originally "fixed" it by switching the catch(done) to finally(done), but that just led to missing the test failure. The failure was due to my configuring it with a custom DB (test_node_mssql) rather than master in the test config, so the string (which embeds the DB name on newer MSSQL) failed to match.

I plan to:

Update the assertion so it doesn't fail due to DB name variation: Several options here. Switch the string to use a regex to ignore the specific database name, or else grab the currently connected db, or perhaps override the database spec'd in the config file to always be master. ~For simplicity, for now, I'll match any DB name with a regex.~ Edit: This would provide poor test feedback on failure without assert.match (unavailable since added after Node 10), so instead, I'm using the database from config.
Feed Mocha any failed assertions: Try/catch around where assertions are being made, and return done(err) in the catch.

jeremy-w commented 3 years ago

I'm getting a different error that I'm not sure how to interpret that's leading the connection errors : timeout test to fail.

The test itself asserts against a Boolean, so I added a log to see what it's dealing with. The test body is expecting to match the regex passed in from tedious.js:

it('timeout', done => TESTS.timeout(done, /Failed to connect to 10.0.0.1:1433 in 1000ms/))

But the error message I'm observing is not actually a timeout error, but something about sequence causing it not to connect successfully:

TEST: err.message Failed to connect to 10.0.0.1:1433 - Could not connect (sequence) err ConnectionError: Failed to connect to 10.0.0.1:1433 - Could not connect (sequence)

I'm guessing this is because 10.0.0.1 is actually a device on my local network. I tried another IP address, which is not assigned, and got the same error message.

I'm not clear on how this test actually works. What is expected to be running on 10.0.0.1 that this hangs until timeout?

(I'll be ignoring the test failure WRT my local tests of the UDT changes.)

dhensby commented 3 years ago

I have also noticed that I get one of these tests failing locally that passes in CI - I've never had the inclination to really look into it in detail.

Attempting to connect to an IP like that for a test feels really bad (this was authored before I took on maintenance of the project). I'd much rather just try to connect to a different port on localhost or something like that. Possibly even just mocking the tedious error because forcing a timeout like this is only really possible if we can have control over shutting down the sql server via the test suite (probably possible, but a pain).

jeremy-w commented 3 years ago

Ah OK. I will teach it to skip outside CI for now then.

jeremy-w commented 3 years ago

Probably starting a server listening on a local port that then never accepts connections would do the trick to make this repeatable easily, but I'm stopping with all green by skipping for now. ;)

tediousjs / node-mssql