Add correct lat, lng to parsed GEOGRAPHY POINT [#1277]

CHANGELOG.txt

@@ -1,3 +1,8 @@
+v7.2.0 (2021-??-??)
+-------------------
+[new] Update Geography field parsing to provide lat/lng props from Geography Point ((#1282)[https://github.com/tediousjs/node-mssql/pull/1282])
+
+
 v7.1.3 (2021-06-11)
 -------------------
 [fix] Request timeout settings now respect value parsed from connection strings ((#1257)[https://github.com/tediousjs/node-mssql/pull/1257)

README.md

@@ -1514,32 +1514,65 @@ If you omit config path argument, mssql will try to load it from current working
 
 ## Geography and Geometry
 
-node-mssql has built-in serializer for Geography and Geometry CLR data types.
+node-mssql has built-in deserializer for Geography and Geometry CLR data types.
+
+### Geography
+
+Geography types can be constructed several different ways. Refer carefully to documentation to verify the coordinate ordering; the ST methods tend to order parameters as longitude (x) then latitude (y), while custom CLR methods tend to prefer to order them as latitude (y) then longitude (x).
+
+The query:
 
 ```sql
-select geography::STGeomFromText('LINESTRING(-122.360 47.656, -122.343 47.656 )', 4326)
-select geometry::STGeomFromText('LINESTRING (100 100 10.3 12, 20 180, 180 180)', 0)
+select geography::STGeomFromText(N'POLYGON((1 1, 3 1, 3 1, 1 1))',4326)
 ```
 
-Results in:
+results in:
 
 ```javascript
-{ srid: 4326,
-  version: 1,
-  points: [ { x: 47.656, y: -122.36 }, { x: 47.656, y: -122.343 } ],
+{
+  srid: 4326,
+  version: 2,
+  points: [
+    Point { lat: 1, lng: 1, z: null, m: null },
+    Point { lat: 1, lng: 3, z: null, m: null },
+    Point { lat: 1, lng: 3, z: null, m: null },
+    Point { lat: 1, lng: 1, z: null, m: null }
+  ],
   figures: [ { attribute: 1, pointOffset: 0 } ],
-  shapes: [ { parentOffset: -1, figureOffset: 0, type: 2 } ],
-  segments: [] }
+  shapes: [ { parentOffset: -1, figureOffset: 0, type: 3 } ],
+  segments: []
+}
+```
+
+**NOTE:** You will also see `x` and `y` coordinates in parsed Geography points,
+they are not recommended for use. They have thus been omitted from this example.
+For compatibility, they remain flipped (x, the horizontal offset, is instead used for latitude, the vertical), and thus risk misleading you.
+Prefer instead to use the `lat` and `lng` properties.
+
+### Geometry
 
-{ srid: 0,
+Geometry types can also be constructed in several ways. Unlike Geographies, they are consistent in always placing x before y. node-mssql decodes the result of this query:
+
+```sql
+select geometry::STGeomFromText(N'POLYGON((1 1, 3 1, 3 7, 1 1))',4326)
+```
+
+into the JavaScript object:
+
+```javascript
+{
+  srid: 4326,
   version: 1,
-  points:
-   [ { x: 100, y: 100, z: 10.3, m: 12 },
-     { x: 20, y: 180, z: NaN, m: NaN },
-     { x: 180, y: 180, z: NaN, m: NaN } ],
-  figures: [ { attribute: 1, pointOffset: 0 } ],
-  shapes: [ { parentOffset: -1, figureOffset: 0, type: 2 } ],
-  segments: [] }
+  points: [
+    Point { x: 1, y: 1, z: null, m: null },
+    Point { x: 1, y: 3, z: null, m: null },
+    Point { x: 7, y: 3, z: null, m: null },
+    Point { x: 1, y: 1, z: null, m: null }
+  ],
+  figures: [ { attribute: 2, pointOffset: 0 } ],
+  shapes: [ { parentOffset: -1, figureOffset: 0, type: 3 } ],
+  segments: []
+}
 ```
 
 ## Table-Valued Parameter (TVP)

lib/tedious/request.js

@@ -268,11 +268,11 @@ class Request extends BaseRequest {
       this.parent.acquire(this, (err, connection) => {
         const callbackWithRelease = (err, ...args) => {
           try {
-            this.parent.release(connection);
+            this.parent.release(connection)
           } catch (e) {
             // noop
           }
-          callback(err, ...args);
+          callback(err, ...args)
         }
         if (err) return callbackWithRelease(err)
 

lib/udt.js

@@ -53,20 +53,42 @@ class Point {
   }
 }
 
-const parsePoints = (buffer, count) => {
+const parsePoints = (buffer, count, isGeometryPoint) => {
   // s2.1.5 + s2.1.6
+  // The key distinction for parsing is that a GEOGRAPHY POINT is ordered Lat (y) then Long (x),
+  // while a GEOMETRY POINT is ordered x then y.
+  // Further, there are additional range constraints on GEOGRAPHY POINT that are useful for testing that the coordinate order has not been flipped, such as that Lat must be in the range [-90, +90].
 
   const points = []
   if (count < 1) {
     return points
   }
 
-  for (let i = 1; i <= count; i++) {
-    const point = new Point()
-    points.push(point)
-    point.x = buffer.readDoubleLE(buffer.position)
-    point.y = buffer.readDoubleLE(buffer.position + 8)
-    buffer.position += 16
+  if (isGeometryPoint) {
+    // GEOMETRY POINT (s2.1.6): x then y.
+    for (let i = 1; i <= count; i++) {
+      const point = new Point()
+      points.push(point)
+      point.x = buffer.readDoubleLE(buffer.position)
+      point.y = buffer.readDoubleLE(buffer.position + 8)
+      buffer.position += 16
+    }
+  } else {
+    // GEOGRAPHY POINT (s2.1.5): Lat (y) then Long (x).
+    for (let i = 1; i <= count; i++) {
+      const point = new Point()
+      points.push(point)
+      point.lat = buffer.readDoubleLE(buffer.position)
+      point.lng = buffer.readDoubleLE(buffer.position + 8)
+
+      // For backwards compatibility, preserve the coordinate inversion in x and y.
+      // A future breaking change likely eliminate x and y for geography points in favor of just the lat and lng fields, as they've proven marvelously confusing.
+      // See discussion at: https://github.com/tediousjs/node-mssql/pull/1282#discussion_r677769531
+      point.x = point.lat
+      point.y = point.lng
+
+      buffer.position += 16
+    }
   }
 
   return points
@@ -182,7 +204,7 @@ const parseSegments = (buffer, count) => {
   return segments
 }
 
-const parseGeography = buffer => {
+const parseGeography = (buffer, isUsingGeometryPoints) => {
   // s2.1.1 + s.2.1.2
 
   const srid = buffer.readInt32LE(0)
@@ -227,7 +249,7 @@ const parseGeography = buffer => {
 
   // console.log("numberOfPoints", numberOfPoints)
 
-  value.points = parsePoints(buffer, numberOfPoints)
+  value.points = parsePoints(buffer, numberOfPoints, isUsingGeometryPoints)
 
   if (properties.Z) {
     parseZ(buffer, value.points)
@@ -289,10 +311,10 @@ const parseGeography = buffer => {
 
 module.exports.PARSERS = {
   geography (buffer) {
-    return parseGeography(buffer)
+    return parseGeography(buffer, /* isUsingGeometryPoints: */false)
   },
 
   geometry (buffer) {
-    return parseGeography(buffer)
+    return parseGeography(buffer, /* isUsingGeometryPoints: */true)
   }
 }

test/common/tests.js

@@ -3,6 +3,7 @@
 const assert = require('assert')
 const stream = require('stream')
 const { join } = require('path')
+const { format } = require('util')
 const ISOLATION_LEVELS = require('../../lib/isolationlevel')
 const BaseTransaction = require('../../lib/base/transaction')
 const versionHelper = require('./versionhelper')
@@ -27,6 +28,10 @@ class WritableStream extends stream.Writable {
   }
 }
 
+const readConfig = () => {
+  return require(join(__dirname, '../.mssql.json'))
+}
+
 module.exports = (sql, driver) => {
   class TestRequest extends sql.Request {
     execute (method) {
@@ -173,39 +178,67 @@ module.exports = (sql, driver) => {
         // assert.deepStrictEqual rst[0].geography, sample1
         // assert.deepStrictEqual rst[0].geometry, sample2
 
+        // GEOGRAPHY
         assert.strictEqual(result.recordset[0].geography.srid, 4326)
         assert.strictEqual(result.recordset[0].geography.version, 1)
+
         assert.strictEqual(result.recordset[0].geography.points.length, 2)
+        assert.strictEqual(result.recordset[0].geography.points[0].lng, -122.360)
+        assert.strictEqual(result.recordset[0].geography.points[0].lat, 47.656)
+        assert.strictEqual(result.recordset[0].geography.points[1].lng, -122.343)
+        assert.strictEqual(result.recordset[0].geography.points[1].lat, 47.656)
+
+        // Backwards compatibility: Preserve flipped x/y.
+        assert.strictEqual(result.recordset[0].geography.points[0].y, -122.360)
         assert.strictEqual(result.recordset[0].geography.points[0].x, 47.656)
         assert.strictEqual(result.recordset[0].geography.points[1].y, -122.343)
+        assert.strictEqual(result.recordset[0].geography.points[1].x, 47.656)
+
         assert.strictEqual(result.recordset[0].geography.figures.length, 1)
         assert.strictEqual(result.recordset[0].geography.figures[0].attribute, 0x01)
+
         assert.strictEqual(result.recordset[0].geography.shapes.length, 1)
         assert.strictEqual(result.recordset[0].geography.shapes[0].type, 0x02)
+
         assert.strictEqual(result.recordset[0].geography.segments.length, 0)
 
+        // GEOMETRY
         assert.strictEqual(result.recordset[0].geometry.srid, 0)
         assert.strictEqual(result.recordset[0].geometry.version, 1)
+
         assert.strictEqual(result.recordset[0].geometry.points.length, 3)
         assert.strictEqual(result.recordset[0].geometry.points[0].z, 10.3)
         assert.strictEqual(result.recordset[0].geometry.points[0].m, 12)
         assert.strictEqual(result.recordset[0].geometry.points[1].x, 20)
         assert.strictEqual(result.recordset[0].geometry.points[2].y, 180)
         assert(isNaN(result.recordset[0].geometry.points[2].z))
         assert(isNaN(result.recordset[0].geometry.points[2].m))
+
         assert.strictEqual(result.recordset[0].geometry.figures.length, 1)
         assert.strictEqual(result.recordset[0].geometry.figures[0].attribute, 0x01)
+
         assert.strictEqual(result.recordset[0].geometry.shapes.length, 1)
         assert.strictEqual(result.recordset[0].geometry.shapes[0].type, 0x02)
+
         assert.strictEqual(result.recordset[0].geometry.segments.length, 0)
 
         assert(result.recordset.columns.geography.type === sql.Geography)
         assert(result.recordset.columns.geometry.type === sql.Geometry)
         assert.strictEqual(result.recordset.columns.geography.udt.name, 'geography')
         assert.strictEqual(result.recordset.columns.geometry.udt.name, 'geometry')
+      }).then(() =>
+        new TestRequest().query('DECLARE @geo GEOGRAPHY = geography::Point(90, 180, 4326); SELECT @geo AS geo, @geo.Lat AS expectedLat, @geo.Long AS expectedLng')
+      ).then(result => {
+        // Our notion of lat and lng should agree with SQL Server's notion.
+        const record = result.recordset[0]
+        const parsedPoint = record.geo.points[0]
+        assert.strictEqual(parsedPoint.lat, record.expectedLat)
+        assert.strictEqual(parsedPoint.lng, record.expectedLng)
 
-        done()
-      }).catch(done)
+        // Backwards compatibility: Preserve flipped x/y.
+        assert.strictEqual(parsedPoint.x, record.expectedLat)
+        assert.strictEqual(parsedPoint.y, record.expectedLng)
+      }).then(done, done)
     },
 
     'binary data' (done) {
@@ -971,7 +1004,10 @@ module.exports = (sql, driver) => {
             assert.ok(err)
 
             if (isSQLServer2019OrNewer) {
-              expectedMessage = "String or binary data would be truncated in table 'master.dbo.tran_test', column 'data'. Truncated value: 'asdfasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfas'."
+              const config = readConfig()
+              const configDatabase = config.database
+              const databaseName = configDatabase || 'master'
+              expectedMessage = "String or binary data would be truncated in table '" + databaseName + ".dbo.tran_test', column 'data'. Truncated value: 'asdfasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfas'."
             } else {
               expectedMessage = 'String or binary data would be truncated.'
             }
@@ -984,11 +1020,15 @@ module.exports = (sql, driver) => {
               if (!rollbackHandled) { return done(new Error("Rollback event didn't fire.")) }
 
               done()
-            })
-          })
+            }).catch(done)
+          }).catch(done)
 
           tran.on('rollback', function (aborted) {
-            assert.strictEqual(aborted, true)
+            try {
+              assert.strictEqual(aborted, true)
+            } catch (err) {
+              done(err)
+            }
 
             rollbackHandled = true
           })
@@ -1097,7 +1137,7 @@ module.exports = (sql, driver) => {
     },
 
     'request timeout' (done, driver, message) {
-      const config = JSON.parse(require('fs').readFileSync(join(__dirname, '../.mssql.json')))
+      const config = readConfig()
       config.driver = driver
       config.requestTimeout = 1000 // note: msnodesqlv8 doesn't support timeouts less than 1 second
 
@@ -1235,7 +1275,7 @@ module.exports = (sql, driver) => {
     },
 
     'login failed' (done, message) {
-      const config = JSON.parse(require('fs').readFileSync(join(__dirname, '../.mssql.json')))
+      const config = readConfig()
       config.user = '__notexistinguser__'
 
       // eslint-disable-next-line no-new
@@ -1255,7 +1295,17 @@ module.exports = (sql, driver) => {
         connectionTimeout: 1000,
         pool: { idleTimeoutMillis: 500 }
       }, (err) => {
-        assert.strictEqual((message ? (message.exec(err.message) != null) : (err instanceof sql.ConnectionPoolError)), true)
+        const isRunningUnderCI = process.env.CI && process.env.CI.toLowerCase() === 'true'
+        if (!isRunningUnderCI) {
+          // Skipping outside CI as this test relies on a controlled network environment.
+          // See discussion at: https://github.com/tediousjs/node-mssql/issues/1277#issuecomment-886638039
+          this.skip()
+          return
+        }
+
+        const match = message.exec(err.message)
+        assert.notStrictEqual(match, null, format('Expected timeout error message to match regexp', message, 'but instead received error message:', err.message))
+
         conn.close()
         done()
       })
@@ -1428,7 +1478,7 @@ module.exports = (sql, driver) => {
       }
 
       __range__(1, peak, true).forEach((i) => {
-        const c = new sql.ConnectionPool(JSON.parse(require('fs').readFileSync(join(__dirname, '../.mssql.json'))))
+        const c = new sql.ConnectionPool(readConfig())
         c.connect(connected)
         conns.push(c)
       })
@@ -1437,7 +1487,7 @@ module.exports = (sql, driver) => {
     'concurrent requests' (done, driver) {
       console.log('')
 
-      const config = JSON.parse(require('fs').readFileSync(join(__dirname, '../.mssql.json')))
+      const config = readConfig()
       config.driver = driver
       config.pool = { min: 0, max: 50 }