From 096cab931ff60dff33fc02923e3237e8b2ff17f2 Mon Sep 17 00:00:00 2001 From: "Jeremy W. Sherman" Date: Mon, 26 Jul 2021 06:55:24 -0400 Subject: [PATCH 01/17] standard --fix Since standard is part of the test script, it was failing before tests even ran. ``` standard: Use JavaScript Standard Style (https://standardjs.com) standard: Run `standard --fix` to automatically fix some problems. node-mssql/lib/tedious/request.js:271:44: Extra semicolon. node-mssql/lib/tedious/request.js:275:33: Extra semicolon. ``` --- lib/tedious/request.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/tedious/request.js b/lib/tedious/request.js index eac531c2..325f32fc 100644 --- a/lib/tedious/request.js +++ b/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) From 698d12b58d48e57fe8bdebb797efe49f850c29b4 Mon Sep 17 00:00:00 2001 From: "Jeremy W. Sherman" Date: Mon, 26 Jul 2021 07:40:58 -0400 Subject: [PATCH 02/17] Route failed assertions in callbacks to Mocha Before, the test would timeout if an assertion tripped. Alongside that, Node would log an error about an unhandled rejection. Now, Mocha rapidly fails the test on the first failed assertion. --- test/common/tests.js | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/test/common/tests.js b/test/common/tests.js index 4a29de2d..64abcef5 100644 --- a/test/common/tests.js +++ b/test/common/tests.js @@ -984,11 +984,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 }) From 0fd823b465ea57d64170599c8124a6c6b56d00c3 Mon Sep 17 00:00:00 2001 From: "Jeremy W. Sherman" Date: Mon, 26 Jul 2021 07:55:54 -0400 Subject: [PATCH 03/17] Prevent failed test due to different DB name Before, this one test embedded an assumption that the tests would only run against the `master` database. Now, that assumption is removed, by reading the database name from the config file. --- test/common/tests.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/test/common/tests.js b/test/common/tests.js index 64abcef5..c3e00c57 100644 --- a/test/common/tests.js +++ b/test/common/tests.js @@ -971,7 +971,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 = JSON.parse(require('fs').readFileSync(join(__dirname, '../.mssql.json'))) + 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.' } From 089ce5b902ec34e6cd4891493374f1b8e987c7ee Mon Sep 17 00:00:00 2001 From: "Jeremy W. Sherman" Date: Mon, 26 Jul 2021 09:21:54 -0400 Subject: [PATCH 04/17] Add tests for existing Geometry parsing Since GEOGRAPHY POINT and GEOMETRY POINT are different, we need separate tests for these two UDTs. As 2.1 notes: > The term "GEOGRAPHY POINT structure" does not also refer to the GEOMETRY POINT structure in this document. ([MS-SSCLRT 2.1][]) Other than that fundamental unit of structure, they're identical. [MS-SSCLRT 2.1]: https://docs.microsoft.com/en-us/openspecs/sql_server_protocols/ms-ssclrt/8ce82728-6582-4e7b-96a0-8d0379767877 --- test/common/unit.js | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/test/common/unit.js b/test/common/unit.js index cd10049c..be6ed3e8 100644 --- a/test/common/unit.js +++ b/test/common/unit.js @@ -290,6 +290,45 @@ describe('Unit', () => { }) }) +describe('Geometry Parsing', () => { + it('polygon v1', () => { + // select geometry::STGeomFromText(N'POLYGON((1 1, 3 1, 3 7, 1 1))',4326) + const buffer = Buffer.from('E6100000010404000000000000000000F03F000000000000F03F0000000000000840000000000000F03F00000000000008400000000000001C40000000000000F03F000000000000F03F01000000020000000001000000FFFFFFFF0000000003', 'hex') + const geom = udt.PARSERS.geometry(buffer) + + assert.strictEqual(geom.version, 1) + assert.strictEqual(geom.srid, 4326) + assert.strictEqual(geom.points.length, 4) + assert.strictEqual(geom.points[0].x, 1) + assert.strictEqual(geom.points[0].y, 1) + assert.strictEqual(geom.points[1].x, 3) + assert.strictEqual(geom.points[1].y, 1) + assert.strictEqual(geom.points[2].x, 3) + assert.strictEqual(geom.points[2].y, 7) + assert.strictEqual(geom.points[3].x, 1) + assert.strictEqual(geom.points[3].y, 1) + }) + + it('polygon v2', () => { + // select geometry::STGeomFromText(N'POLYGON((1 1, 3 1, 3 1, 1 1))',4326) + // (then tweak it to switch to v2: s/010/020/, though without any segments, it's kind of a moot point.) + const buffer = Buffer.from('E6100000020004000000000000000000F03F000000000000F03F0000000000000840000000000000F03F0000000000000840000000000000F03F000000000000F03F000000000000F03F01000000020000000001000000FFFFFFFF0000000003', 'hex') + const geom = udt.PARSERS.geometry(buffer) + + assert.strictEqual(geom.version, 2) + assert.strictEqual(geom.srid, 4326) + assert.strictEqual(geom.points.length, 4) + assert.strictEqual(geom.points[0].x, 1) + assert.strictEqual(geom.points[0].y, 1) + assert.strictEqual(geom.points[1].x, 3) + assert.strictEqual(geom.points[1].y, 1) + assert.strictEqual(geom.points[2].x, 3) + assert.strictEqual(geom.points[2].y, 1) + assert.strictEqual(geom.points[3].x, 1) + assert.strictEqual(geom.points[3].y, 1) + }) +}) + describe('Geography Parsing', () => { it('polygon v1', () => { // select geography::STGeomFromText(N'POLYGON((1 1, 3 1, 3 7, 1 1))',4326) From 110dbb18a7d4affbf053f1bed684bf6b1baf4d77 Mon Sep 17 00:00:00 2001 From: "Jeremy W. Sherman" Date: Mon, 26 Jul 2021 09:23:04 -0400 Subject: [PATCH 05/17] Show overall structure asserted against in comment --- test/common/unit.js | 60 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/test/common/unit.js b/test/common/unit.js index be6ed3e8..30f3d3d8 100644 --- a/test/common/unit.js +++ b/test/common/unit.js @@ -296,6 +296,21 @@ describe('Geometry Parsing', () => { const buffer = Buffer.from('E6100000010404000000000000000000F03F000000000000F03F0000000000000840000000000000F03F00000000000008400000000000001C40000000000000F03F000000000000F03F01000000020000000001000000FFFFFFFF0000000003', 'hex') const geom = udt.PARSERS.geometry(buffer) + /* +{ + srid: 4326, + version: 1, + 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: [] +} + */ assert.strictEqual(geom.version, 1) assert.strictEqual(geom.srid, 4326) assert.strictEqual(geom.points.length, 4) @@ -315,6 +330,21 @@ describe('Geometry Parsing', () => { const buffer = Buffer.from('E6100000020004000000000000000000F03F000000000000F03F0000000000000840000000000000F03F0000000000000840000000000000F03F000000000000F03F000000000000F03F01000000020000000001000000FFFFFFFF0000000003', 'hex') const geom = udt.PARSERS.geometry(buffer) + /* +{ + srid: 4326, + version: 2, + points: [ + Point { x: 1, y: 1, z: null, m: null }, + Point { x: 1, y: 3, z: null, m: null }, + Point { x: 1, y: 3, z: null, m: null }, + Point { x: 1, y: 1, z: null, m: null } + ], + figures: [ { attribute: 1, pointOffset: 0 } ], + shapes: [ { parentOffset: -1, figureOffset: 0, type: 3 } ], + segments: [] +} + */ assert.strictEqual(geom.version, 2) assert.strictEqual(geom.srid, 4326) assert.strictEqual(geom.points.length, 4) @@ -335,6 +365,21 @@ describe('Geography Parsing', () => { const buffer = Buffer.from('E6100000010404000000000000000000F03F000000000000F03F000000000000F03F00000000000008400000000000001C400000000000000840000000000000F03F000000000000F03F01000000020000000001000000FFFFFFFF0000000003', 'hex') const geo = udt.PARSERS.geography(buffer) + /* +{ + srid: 4326, + version: 1, + 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: [] +} + */ assert.strictEqual(geo.version, 1) assert.strictEqual(geo.srid, 4326) assert.strictEqual(geo.points.length, 4) @@ -353,6 +398,21 @@ describe('Geography Parsing', () => { const buffer = Buffer.from('E6100000020004000000000000000000F03F000000000000F03F000000000000F03F0000000000000840000000000000F03F0000000000000840000000000000F03F000000000000F03F01000000010000000001000000FFFFFFFF0000000003', 'hex') const geo = udt.PARSERS.geography(buffer) + /* +{ + srid: 4326, + version: 2, + points: [ + Point { x: 1, y: 1, z: null, m: null }, + Point { x: 1, y: 3, z: null, m: null }, + Point { x: 1, y: 3, z: null, m: null }, + Point { x: 1, y: 1, z: null, m: null } + ], + figures: [ { attribute: 1, pointOffset: 0 } ], + shapes: [ { parentOffset: -1, figureOffset: 0, type: 3 } ], + segments: [] +} + */ assert.strictEqual(geo.version, 2) assert.strictEqual(geo.srid, 4326) assert.strictEqual(geo.points.length, 4) From 047af08b84ab68224faf24ed103fabec1f2279cb Mon Sep 17 00:00:00 2001 From: "Jeremy W. Sherman" Date: Mon, 26 Jul 2021 10:40:43 -0400 Subject: [PATCH 06/17] Fix lat/lon inversion when parsing GEOGRAPHY POINT Fixes #1277. --- lib/udt.js | 37 ++++++++++++++++++++++++++----------- test/common/tests.js | 4 ++-- test/common/unit.js | 32 ++++++++++++++++---------------- 3 files changed, 44 insertions(+), 29 deletions(-) diff --git a/lib/udt.js b/lib/udt.js index 761f0025..e3faf74a 100644 --- a/lib/udt.js +++ b/lib/udt.js @@ -53,20 +53,35 @@ 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) { + // GEOGRAPHY 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.y = buffer.readDoubleLE(buffer.position) + point.x = buffer.readDoubleLE(buffer.position + 8) + buffer.position += 16 + } } return points @@ -182,7 +197,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 +242,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 +304,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) } } diff --git a/test/common/tests.js b/test/common/tests.js index c3e00c57..76db2c7a 100644 --- a/test/common/tests.js +++ b/test/common/tests.js @@ -176,8 +176,8 @@ module.exports = (sql, driver) => { 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].x, 47.656) - assert.strictEqual(result.recordset[0].geography.points[1].y, -122.343) + assert.strictEqual(result.recordset[0].geography.points[0].y, 47.656) + assert.strictEqual(result.recordset[0].geography.points[1].x, -122.343) 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) diff --git a/test/common/unit.js b/test/common/unit.js index 30f3d3d8..b1c13973 100644 --- a/test/common/unit.js +++ b/test/common/unit.js @@ -371,8 +371,8 @@ describe('Geography Parsing', () => { version: 1, 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: 3, y: 1, z: null, m: null }, + Point { x: 3, y: 7, z: null, m: null }, Point { x: 1, y: 1, z: null, m: null } ], figures: [ { attribute: 2, pointOffset: 0 } ], @@ -383,14 +383,14 @@ describe('Geography Parsing', () => { assert.strictEqual(geo.version, 1) assert.strictEqual(geo.srid, 4326) assert.strictEqual(geo.points.length, 4) - assert.strictEqual(geo.points[0].y, 1) assert.strictEqual(geo.points[0].x, 1) - assert.strictEqual(geo.points[1].y, 3) - assert.strictEqual(geo.points[1].x, 1) - assert.strictEqual(geo.points[2].y, 3) - assert.strictEqual(geo.points[2].x, 7) - assert.strictEqual(geo.points[3].y, 1) + assert.strictEqual(geo.points[0].y, 1) + assert.strictEqual(geo.points[1].x, 3) + assert.strictEqual(geo.points[1].y, 1) + assert.strictEqual(geo.points[2].x, 3) + assert.strictEqual(geo.points[2].y, 7) assert.strictEqual(geo.points[3].x, 1) + assert.strictEqual(geo.points[3].y, 1) }) it('polygon v2', () => { @@ -404,8 +404,8 @@ describe('Geography Parsing', () => { version: 2, points: [ Point { x: 1, y: 1, z: null, m: null }, - Point { x: 1, y: 3, z: null, m: null }, - Point { x: 1, y: 3, z: null, m: null }, + Point { x: 3, y: 1, z: null, m: null }, + Point { x: 3, y: 1, z: null, m: null }, Point { x: 1, y: 1, z: null, m: null } ], figures: [ { attribute: 1, pointOffset: 0 } ], @@ -416,14 +416,14 @@ describe('Geography Parsing', () => { assert.strictEqual(geo.version, 2) assert.strictEqual(geo.srid, 4326) assert.strictEqual(geo.points.length, 4) - assert.strictEqual(geo.points[0].y, 1) assert.strictEqual(geo.points[0].x, 1) - assert.strictEqual(geo.points[1].y, 3) - assert.strictEqual(geo.points[1].x, 1) - assert.strictEqual(geo.points[2].y, 3) - assert.strictEqual(geo.points[2].x, 1) - assert.strictEqual(geo.points[3].y, 1) + assert.strictEqual(geo.points[0].y, 1) + assert.strictEqual(geo.points[1].x, 3) + assert.strictEqual(geo.points[1].y, 1) + assert.strictEqual(geo.points[2].x, 3) + assert.strictEqual(geo.points[2].y, 1) assert.strictEqual(geo.points[3].x, 1) + assert.strictEqual(geo.points[3].y, 1) }) }) From ceee98035ac59b5c5e008df9171d8fdb5b5d1a4f Mon Sep 17 00:00:00 2001 From: "Jeremy W. Sherman" Date: Mon, 26 Jul 2021 10:41:33 -0400 Subject: [PATCH 07/17] Improve timeout test failure diagnostics Before, it said false was not true. Now, it provides enough detail to understand why false was not true. --- test/common/tests.js | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/test/common/tests.js b/test/common/tests.js index 76db2c7a..e6baa4b8 100644 --- a/test/common/tests.js +++ b/test/common/tests.js @@ -3,6 +3,7 @@ const assert = require('assert') const stream = require('stream') const { join } = require('path') +const util = require('util') const ISOLATION_LEVELS = require('../../lib/isolationlevel') const BaseTransaction = require('../../lib/base/transaction') const versionHelper = require('./versionhelper') @@ -1262,7 +1263,12 @@ module.exports = (sql, driver) => { connectionTimeout: 1000, pool: { idleTimeoutMillis: 500 } }, (err) => { - assert.strictEqual((message ? (message.exec(err.message) != null) : (err instanceof sql.ConnectionPoolError)), true) + if (message) { + const match = message.exec(err.message) + assert.notStrictEqual(match, null, util.format('Expected timeout error message to match', message, 'but instead received error message:', err.message)) + } else { + assert.strictEqual(err instanceof sql.ConnectionPoolError, true, util.format('Expected timeout error to be an instance of ConnectionPoolError, but instead received an instance of', Object.getPrototypeOf(err), err)) + } conn.close() done() }) From db746cdae5ac7638c6da393986b1c8549ce38b3f Mon Sep 17 00:00:00 2001 From: "Jeremy W. Sherman" Date: Mon, 26 Jul 2021 10:45:50 -0400 Subject: [PATCH 08/17] Remove unused test variation Before, it checked for instanceof when no message was provided. But it is never called without a message regexp, so this is unnecessary. (In case the unrunnable-test variation using the msnodesqlv8 driver ever gets un-skipped, I have adjusted the parameters to agree in providing a regexp, however.) --- test/common/tests.js | 9 +++------ test/msnodesqlv8/msnodesqlv8.js | 2 +- 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/test/common/tests.js b/test/common/tests.js index e6baa4b8..ec16c777 100644 --- a/test/common/tests.js +++ b/test/common/tests.js @@ -1263,12 +1263,9 @@ module.exports = (sql, driver) => { connectionTimeout: 1000, pool: { idleTimeoutMillis: 500 } }, (err) => { - if (message) { - const match = message.exec(err.message) - assert.notStrictEqual(match, null, util.format('Expected timeout error message to match', message, 'but instead received error message:', err.message)) - } else { - assert.strictEqual(err instanceof sql.ConnectionPoolError, true, util.format('Expected timeout error to be an instance of ConnectionPoolError, but instead received an instance of', Object.getPrototypeOf(err), err)) - } + const match = message.exec(err.message) + assert.notStrictEqual(match, null, util.format('Expected timeout error message to match regexp', message, 'but instead received error message:', err.message)) + conn.close() done() }) diff --git a/test/msnodesqlv8/msnodesqlv8.js b/test/msnodesqlv8/msnodesqlv8.js index 51a42c43..8f3fc1ba 100644 --- a/test/msnodesqlv8/msnodesqlv8.js +++ b/test/msnodesqlv8/msnodesqlv8.js @@ -178,7 +178,7 @@ describe('msnodesqlv8', function () { describe('msnodesqlv8 connection errors', function () { it('login failed', done => TESTS['login failed'](done, /Login failed for user '(.*)'\./)) - it.skip('timeout (not supported by msnodesqlv8)', done => TESTS.timeout(done)) + it.skip('timeout (not supported by msnodesqlv8)', done => TESTS.timeout(done, /1000ms/)) it.skip('network error (not supported by msnodesqlv8)', done => TESTS['network error'](done)) }) From 4f973296a8e9731242925c44828d21ddca91297f Mon Sep 17 00:00:00 2001 From: "Jeremy W. Sherman" Date: Mon, 26 Jul 2021 10:52:17 -0400 Subject: [PATCH 09/17] Skip connection timeout test outside CI --- test/common/tests.js | 8 ++++++++ test/msnodesqlv8/msnodesqlv8.js | 2 +- test/tedious/tedious.js | 3 ++- 3 files changed, 11 insertions(+), 2 deletions(-) diff --git a/test/common/tests.js b/test/common/tests.js index ec16c777..7f2fa915 100644 --- a/test/common/tests.js +++ b/test/common/tests.js @@ -1263,6 +1263,14 @@ module.exports = (sql, driver) => { connectionTimeout: 1000, pool: { idleTimeoutMillis: 500 } }, (err) => { + 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, util.format('Expected timeout error message to match regexp', message, 'but instead received error message:', err.message)) diff --git a/test/msnodesqlv8/msnodesqlv8.js b/test/msnodesqlv8/msnodesqlv8.js index 8f3fc1ba..51c5c00d 100644 --- a/test/msnodesqlv8/msnodesqlv8.js +++ b/test/msnodesqlv8/msnodesqlv8.js @@ -178,7 +178,7 @@ describe('msnodesqlv8', function () { describe('msnodesqlv8 connection errors', function () { it('login failed', done => TESTS['login failed'](done, /Login failed for user '(.*)'\./)) - it.skip('timeout (not supported by msnodesqlv8)', done => TESTS.timeout(done, /1000ms/)) + it.skip('timeout (not supported by msnodesqlv8)', done => TESTS.timeout.call(this, done, /1000ms/)) it.skip('network error (not supported by msnodesqlv8)', done => TESTS['network error'](done)) }) diff --git a/test/tedious/tedious.js b/test/tedious/tedious.js index 291373df..b9c0d416 100644 --- a/test/tedious/tedious.js +++ b/test/tedious/tedious.js @@ -229,7 +229,8 @@ describe('tedious', () => { describe('connection errors', function () { it('login failed', done => TESTS['login failed'](done, /Login failed for user '(.*)'/)) - it('timeout', done => TESTS.timeout(done, /Failed to connect to 10.0.0.1:1433 in 1000ms/)) + // call(this) to enable the test to skip itself. + it('timeout', function (done) { TESTS.timeout.call(this, done, /Failed to connect to 10.0.0.1:1433 in 1000ms/) }) it('network error', done => TESTS['network error'](done, /Failed to connect to \.\.\.:1433 - getaddrinfo ENOTFOUND/)) }) From dd3f0e674b6bf0728ff853f83a24e544b5b00095 Mon Sep 17 00:00:00 2001 From: "Jeremy W. Sherman" Date: Wed, 28 Jul 2021 09:57:26 -0400 Subject: [PATCH 10/17] Bring only util.format into scope --- test/common/tests.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/common/tests.js b/test/common/tests.js index 7f2fa915..49d73e61 100644 --- a/test/common/tests.js +++ b/test/common/tests.js @@ -3,7 +3,7 @@ const assert = require('assert') const stream = require('stream') const { join } = require('path') -const util = require('util') +const { format } = require('util') const ISOLATION_LEVELS = require('../../lib/isolationlevel') const BaseTransaction = require('../../lib/base/transaction') const versionHelper = require('./versionhelper') @@ -1272,7 +1272,7 @@ module.exports = (sql, driver) => { } const match = message.exec(err.message) - assert.notStrictEqual(match, null, util.format('Expected timeout error message to match regexp', message, 'but instead received error message:', 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() From 8b4d9222ce108e246f4802c7917bb9150e6b5860 Mon Sep 17 00:00:00 2001 From: "Jeremy W. Sherman" Date: Wed, 28 Jul 2021 09:58:40 -0400 Subject: [PATCH 11/17] Fix comment to refer to geometry, not geography Section ref and details were correct, but "header" was wrong. --- lib/udt.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/udt.js b/lib/udt.js index e3faf74a..8fa5764f 100644 --- a/lib/udt.js +++ b/lib/udt.js @@ -65,7 +65,7 @@ const parsePoints = (buffer, count, isGeometryPoint) => { } if (isGeometryPoint) { - // GEOGRAPHY POINT (s2.1.6): x then y. + // GEOMETRY POINT (s2.1.6): x then y. for (let i = 1; i <= count; i++) { const point = new Point() points.push(point) From e9a9bbe99b95efb5157d4a93189a773598f71b6b Mon Sep 17 00:00:00 2001 From: "Jeremy W. Sherman" Date: Wed, 28 Jul 2021 10:16:46 -0400 Subject: [PATCH 12/17] tests: Extract readConfig() helper --- test/common/tests.js | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/test/common/tests.js b/test/common/tests.js index 49d73e61..9cebd4fb 100644 --- a/test/common/tests.js +++ b/test/common/tests.js @@ -28,6 +28,10 @@ class WritableStream extends stream.Writable { } } +const readConfig = () => { + return JSON.parse(require('fs').readFileSync(join(__dirname, '../.mssql.json'))) +} + module.exports = (sql, driver) => { class TestRequest extends sql.Request { execute (method) { @@ -972,7 +976,7 @@ module.exports = (sql, driver) => { assert.ok(err) if (isSQLServer2019OrNewer) { - const config = JSON.parse(require('fs').readFileSync(join(__dirname, '../.mssql.json'))) + 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'." @@ -1105,7 +1109,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 @@ -1243,7 +1247,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 @@ -1446,7 +1450,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) }) @@ -1455,7 +1459,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 } From eaee8750c7f366a6f8aa305ae0f5b72c1129bbc0 Mon Sep 17 00:00:00 2001 From: "Jeremy W. Sherman" Date: Wed, 28 Jul 2021 10:26:48 -0400 Subject: [PATCH 13/17] Read config by requiring it as JSON module --- test/common/tests.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/common/tests.js b/test/common/tests.js index 9cebd4fb..935c2175 100644 --- a/test/common/tests.js +++ b/test/common/tests.js @@ -29,7 +29,7 @@ class WritableStream extends stream.Writable { } const readConfig = () => { - return JSON.parse(require('fs').readFileSync(join(__dirname, '../.mssql.json'))) + return require(join(__dirname, '../.mssql.json')) } module.exports = (sql, driver) => { From b4be2bc176ba0b4f441a6f9ace8395a15e8d03cc Mon Sep 17 00:00:00 2001 From: "Jeremy W. Sherman" Date: Wed, 28 Jul 2021 11:12:12 -0400 Subject: [PATCH 14/17] Geography: Re-flip x/y, add correct lat/lng This makes this not-a-breaking-change, since it is purely additive. --- lib/udt.js | 11 ++++++-- test/common/tests.js | 22 ++++++++++++++-- test/common/unit.js | 62 ++++++++++++++++++++++++++++++-------------- 3 files changed, 71 insertions(+), 24 deletions(-) diff --git a/lib/udt.js b/lib/udt.js index 8fa5764f..c53336b3 100644 --- a/lib/udt.js +++ b/lib/udt.js @@ -78,8 +78,15 @@ const parsePoints = (buffer, count, isGeometryPoint) => { for (let i = 1; i <= count; i++) { const point = new Point() points.push(point) - point.y = buffer.readDoubleLE(buffer.position) - point.x = buffer.readDoubleLE(buffer.position + 8) + 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 } } diff --git a/test/common/tests.js b/test/common/tests.js index 935c2175..4e68ef24 100644 --- a/test/common/tests.js +++ b/test/common/tests.js @@ -178,19 +178,34 @@ 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].y, 47.656) - assert.strictEqual(result.recordset[0].geography.points[1].x, -122.343) + 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) @@ -198,10 +213,13 @@ module.exports = (sql, driver) => { 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) diff --git a/test/common/unit.js b/test/common/unit.js index b1c13973..76a9ffcf 100644 --- a/test/common/unit.js +++ b/test/common/unit.js @@ -370,10 +370,10 @@ describe('Geography Parsing', () => { srid: 4326, version: 1, points: [ - Point { x: 1, y: 1, z: null, m: null }, - Point { x: 3, y: 1, z: null, m: null }, - Point { x: 3, y: 7, z: null, m: null }, - Point { x: 1, y: 1, z: null, m: null } + Point { x: 1, y: 1, z: null, m: null, lat: 1, lng: 1 }, + Point { x: 1, y: 3, z: null, m: null, lat: 1, lng: 3 }, + Point { x: 7, y: 3, z: null, m: null, lat: 7, lng: 3 }, + Point { x: 1, y: 1, z: null, m: null, lat: 1, lng: 1 } ], figures: [ { attribute: 2, pointOffset: 0 } ], shapes: [ { parentOffset: -1, figureOffset: 0, type: 3 } ], @@ -383,14 +383,25 @@ describe('Geography Parsing', () => { assert.strictEqual(geo.version, 1) assert.strictEqual(geo.srid, 4326) assert.strictEqual(geo.points.length, 4) - assert.strictEqual(geo.points[0].x, 1) + + assert.strictEqual(geo.points[0].lng, 1) + assert.strictEqual(geo.points[0].lat, 1) + assert.strictEqual(geo.points[1].lng, 3) + assert.strictEqual(geo.points[1].lat, 1) + assert.strictEqual(geo.points[2].lng, 3) + assert.strictEqual(geo.points[2].lat, 7) + assert.strictEqual(geo.points[3].lng, 1) + assert.strictEqual(geo.points[3].lat, 1) + + // Backwards compatibility: Preserve flipped x and y. assert.strictEqual(geo.points[0].y, 1) - assert.strictEqual(geo.points[1].x, 3) - assert.strictEqual(geo.points[1].y, 1) - assert.strictEqual(geo.points[2].x, 3) - assert.strictEqual(geo.points[2].y, 7) - assert.strictEqual(geo.points[3].x, 1) + assert.strictEqual(geo.points[0].x, 1) + assert.strictEqual(geo.points[1].y, 3) + assert.strictEqual(geo.points[1].x, 1) + assert.strictEqual(geo.points[2].y, 3) + assert.strictEqual(geo.points[2].x, 7) assert.strictEqual(geo.points[3].y, 1) + assert.strictEqual(geo.points[3].x, 1) }) it('polygon v2', () => { @@ -403,10 +414,10 @@ describe('Geography Parsing', () => { srid: 4326, version: 2, points: [ - Point { x: 1, y: 1, z: null, m: null }, - Point { x: 3, y: 1, z: null, m: null }, - Point { x: 3, y: 1, z: null, m: null }, - Point { x: 1, y: 1, z: null, m: null } + Point { x: 1, y: 1, z: null, m: null, lat: 1, lng: 1 }, + Point { x: 1, y: 3, z: null, m: null, lat: 1, lng: 3 }, + Point { x: 1, y: 3, z: null, m: null, lat: 1, lng: 3 }, + Point { x: 1, y: 1, z: null, m: null, lat: 1, lng: 1 } ], figures: [ { attribute: 1, pointOffset: 0 } ], shapes: [ { parentOffset: -1, figureOffset: 0, type: 3 } ], @@ -416,14 +427,25 @@ describe('Geography Parsing', () => { assert.strictEqual(geo.version, 2) assert.strictEqual(geo.srid, 4326) assert.strictEqual(geo.points.length, 4) - assert.strictEqual(geo.points[0].x, 1) + + assert.strictEqual(geo.points[0].lng, 1) + assert.strictEqual(geo.points[0].lat, 1) + assert.strictEqual(geo.points[1].lng, 3) + assert.strictEqual(geo.points[1].lat, 1) + assert.strictEqual(geo.points[2].lng, 3) + assert.strictEqual(geo.points[2].lat, 1) + assert.strictEqual(geo.points[3].lng, 1) + assert.strictEqual(geo.points[3].lat, 1) + + // Backwards compatibility: Preserve flipped x and y. assert.strictEqual(geo.points[0].y, 1) - assert.strictEqual(geo.points[1].x, 3) - assert.strictEqual(geo.points[1].y, 1) - assert.strictEqual(geo.points[2].x, 3) - assert.strictEqual(geo.points[2].y, 1) - assert.strictEqual(geo.points[3].x, 1) + assert.strictEqual(geo.points[0].x, 1) + assert.strictEqual(geo.points[1].y, 3) + assert.strictEqual(geo.points[1].x, 1) + assert.strictEqual(geo.points[2].y, 3) + assert.strictEqual(geo.points[2].x, 1) assert.strictEqual(geo.points[3].y, 1) + assert.strictEqual(geo.points[3].x, 1) }) }) From 1469ec90f1f0ebdb7a10a231818b385ab44fc33e Mon Sep 17 00:00:00 2001 From: "Jeremy W. Sherman" Date: Wed, 28 Jul 2021 11:55:07 -0400 Subject: [PATCH 15/17] Verify our lat,lng agree with SQL Server's Done by querying for the point's Lat and Long directly and comparing to our parsed fields. --- test/common/tests.js | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/test/common/tests.js b/test/common/tests.js index 4e68ef24..0ce233a1 100644 --- a/test/common/tests.js +++ b/test/common/tests.js @@ -226,9 +226,19 @@ module.exports = (sql, driver) => { 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) { From f1993c5d8c1575bf19dd6512f0be5aa689e56242 Mon Sep 17 00:00:00 2001 From: "Jeremy W. Sherman" Date: Wed, 28 Jul 2021 13:51:20 -0400 Subject: [PATCH 16/17] README: Describe new lat and lng fields --- README.md | 67 +++++++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 50 insertions(+), 17 deletions(-) diff --git a/README.md b/README.md index 4a3e2a60..b5034073 100644 --- a/README.md +++ b/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) From d96cb54acb96785755445642db598dff73004c10 Mon Sep 17 00:00:00 2001 From: "Jeremy W. Sherman" Date: Wed, 28 Jul 2021 20:23:47 -0400 Subject: [PATCH 17/17] Add changelog entry for Geography point lat/lng --- CHANGELOG.txt | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/CHANGELOG.txt b/CHANGELOG.txt index a3487ffd..b048ce21 100644 --- a/CHANGELOG.txt +++ b/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)