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) 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) 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) diff --git a/lib/udt.js b/lib/udt.js index 761f0025..c53336b3 100644 --- a/lib/udt.js +++ b/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) } } diff --git a/test/common/tests.js b/test/common/tests.js index 4a29de2d..0ce233a1 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 { 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,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].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) @@ -193,19 +213,32 @@ 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) 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 } diff --git a/test/common/unit.js b/test/common/unit.js index cd10049c..76a9ffcf 100644 --- a/test/common/unit.js +++ b/test/common/unit.js @@ -290,15 +290,110 @@ 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) + + /* +{ + 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) + 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) + + /* +{ + 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) + 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) 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, 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 } ], + segments: [] +} + */ assert.strictEqual(geo.version, 1) assert.strictEqual(geo.srid, 4326) assert.strictEqual(geo.points.length, 4) + + 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[0].x, 1) assert.strictEqual(geo.points[1].y, 3) @@ -314,9 +409,35 @@ 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, 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 } ], + segments: [] +} + */ assert.strictEqual(geo.version, 2) assert.strictEqual(geo.srid, 4326) assert.strictEqual(geo.points.length, 4) + + 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[0].x, 1) assert.strictEqual(geo.points[1].y, 3) diff --git a/test/msnodesqlv8/msnodesqlv8.js b/test/msnodesqlv8/msnodesqlv8.js index 51a42c43..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)) + 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/)) })