From 4cf5e7126e878d6765096eb08007e07c2b0969ad Mon Sep 17 00:00:00 2001 From: lutovich Date: Wed, 17 Jan 2018 12:15:40 +0100 Subject: [PATCH 1/5] Introduce 'useNativeNumbers' config option This boolean option allows users to make driver accept and represent integer values as native JavaScript `Number`s instead of driver's special `Integer`s. When this option is set to `true` driver will fail to encode `Integer` values passed as query parameters. All returned `Record`, `Node`, `Relationship`, etc. will have their integer fields as native `Number`s. Object returned by `Record#toObject()` will also contain native numbers. **Warning:** enabling this setting can potentially make driver return lossy integer values. This would be the case when database contain integer values which do not fit in `[Number.MIN_SAFE_INTEGER, Number.MAX_SAFE_INTEGER]` range. Such integers will then be represented as `Number.NEGATIVE_INFINITY` or `Number.POSITIVE_INFINITY`, which is lossy and different from what is actually stored in the database. That is why this option is set to `false` by default. Driver's `Integer` class is able to represent integer values beyond `[Number.MIN_SAFE_INTEGER, Number.MAX_SAFE_INTEGER]` and thus is a much safer option in the general case. --- package-lock.json | 2 +- src/v1/index.js | 11 +++++++ src/v1/integer.js | 15 +++++++++ src/v1/internal/connector.js | 15 +++++---- src/v1/internal/packstream.js | 53 ++++++++++++++++++++++++++---- test/types/v1/driver.test.ts | 3 ++ test/v1/driver.test.js | 60 ++++++++++++++++++++++++++++++++++ test/v1/integer.test.js | 61 ++++++++++++++++++++++++----------- types/v1/driver.d.ts | 1 + 9 files changed, 188 insertions(+), 33 deletions(-) diff --git a/package-lock.json b/package-lock.json index a6fca3852..fd0262cfe 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,6 +1,6 @@ { "name": "neo4j-driver", - "version": "1.5.0-dev", + "version": "1.6.0-dev", "lockfileVersion": 1, "requires": true, "dependencies": { diff --git a/src/v1/index.js b/src/v1/index.js index b42314d44..2513b4a15 100644 --- a/src/v1/index.js +++ b/src/v1/index.js @@ -157,6 +157,17 @@ const USER_AGENT = "neo4j-javascript/" + VERSION; * // result in no timeout being applied. Connection establishment will be then bound by the timeout configured * // on the operating system level. Default value is 5000, which is 5 seconds. * connectionTimeout: 5000, // 5 seconds + * + * // Make this driver always return and accept native JavaScript number for integer values, instead of the + * // dedicated {@link Integer} class. Values that do not fit in native number bit range will be represented as + * // Number.NEGATIVE_INFINITY or Number.POSITIVE_INFINITY. Driver will fail to encode + * // {@link Integer} values passed as query parameters when this setting is set to true. + * // Warning: It is not safe to enable this setting when JavaScript applications are not the only ones + * // interacting with the database. Stored numbers might in such case be not representable by native + * // {@link Number} type and thus driver will return lossy values. For example, this might happen when data was + * // initially imported using neo4j import tool and contained numbers larger than + * // Number.MAX_SAFE_INTEGER. Driver will then return positive infinity, which is lossy. + * useNativeNumbers: false, * } * * @param {string} url The URL for the Neo4j database, for instance "bolt://localhost" diff --git a/src/v1/integer.js b/src/v1/integer.js index 573b7084a..7fe006c15 100644 --- a/src/v1/integer.js +++ b/src/v1/integer.js @@ -88,6 +88,21 @@ class Integer { */ toNumber(){ return this.high * TWO_PWR_32_DBL + (this.low >>> 0); } + /** + * Converts the Integer to native number or -Infinity/+Infinity when it does not fit. + * @return {number} + * @package + */ + toNumberOrInfinity() { + if (this.lessThan(Integer.MIN_SAFE_VALUE)) { + return Number.NEGATIVE_INFINITY; + } else if (this.greaterThan(Integer.MAX_SAFE_VALUE)) { + return Number.POSITIVE_INFINITY; + } else { + return this.toNumber(); + } + } + /** * Converts the Integer to a string written in the specified radix. * @param {number=} radix Radix (2-36), defaults to 10 diff --git a/src/v1/internal/connector.js b/src/v1/internal/connector.js index e3d53f008..8218411c4 100644 --- a/src/v1/internal/connector.js +++ b/src/v1/internal/connector.js @@ -162,11 +162,12 @@ class Connection { /** * @constructor - * @param channel - channel with a 'write' function and a 'onmessage' - * callback property - * @param url - url to connect to + * @param {NodeChannel|WebSocketChannel} channel - channel with a 'write' function and a 'onmessage' callback property. + * @param {string} url - the hostname and port to connect to. + * @param {boolean} useNativeNumbers if this connection should treat/convert all received numbers + * (including native {@link Number} type or our own {@link Integer}) as native {@link Number}. */ - constructor (channel, url) { + constructor(channel, url, useNativeNumbers = false) { /** * An ordered queue of observers, each exchange response (zero or more * RECORD messages followed by a SUCCESS message) we recieve will be routed @@ -180,8 +181,8 @@ class Connection { this._ch = channel; this._dechunker = new Dechunker(); this._chunker = new Chunker( channel ); - this._packer = new Packer( this._chunker ); - this._unpacker = new Unpacker(); + this._packer = new Packer(this._chunker, useNativeNumbers); + this._unpacker = new Unpacker(useNativeNumbers); this._isHandlingFailure = false; this._currentFailure = null; @@ -588,7 +589,7 @@ function connect(url, config = {}, connectionErrorCode = null) { const Ch = config.channel || Channel; const parsedUrl = urlUtil.parseBoltUrl(url); const channelConfig = new ChannelConfig(parsedUrl, config, connectionErrorCode); - return new Connection(new Ch(channelConfig), parsedUrl.hostAndPort); + return new Connection(new Ch(channelConfig), parsedUrl.hostAndPort, config.useNativeNumbers); } export { diff --git a/src/v1/internal/packstream.js b/src/v1/internal/packstream.js index f62ed8bde..946a38d8e 100644 --- a/src/v1/internal/packstream.js +++ b/src/v1/internal/packstream.js @@ -19,6 +19,7 @@ import utf8 from './utf8'; import Integer, {int, isInt} from '../integer'; import {newError} from './../error'; +import {Chunker} from './chunking'; const TINY_STRING = 0x80; const TINY_LIST = 0x90; @@ -75,9 +76,17 @@ class Structure { * @access private */ class Packer { - constructor (channel) { + + /** + * @constructor + * @param {Chunker} channel the chunker backed by a network channel. + * @param {boolean} useNativeNumbers if this packer should treat/convert all received numbers + * (including native {@link Number} type or our own {@link Integer}) as native {@link Number}. + */ + constructor(channel, useNativeNumbers = false) { this._ch = channel; this._byteArraysSupported = true; + this._useNativeNumbers = useNativeNumbers; } /** @@ -98,7 +107,7 @@ class Packer { } else if (typeof(x) == "string") { return () => this.packString(x, onError); } else if (isInt(x)) { - return () => this.packInteger( x ); + return this._packableInteger(x, onError); } else if (x instanceof Int8Array) { return () => this.packBytes(x, onError); } else if (x instanceof Array) { @@ -141,6 +150,28 @@ class Packer { } } + /** + * Creates a packable function out of the provided {@link Integer} value. + * @param {Integer} x the value to pack. + * @param {function} onError the callback for the case when value cannot be packed. + * @return {function} + * @private + */ + _packableInteger(x, onError) { + if (this._useNativeNumbers) { + // pack Integer objects only when native numbers are not used, fail otherwise + // Integer can't represent special values like Number.NEGATIVE_INFINITY + // and should not be used at all when native numbers are enabled + if (onError) { + onError(newError(`Cannot pack Integer value ${x} (${JSON.stringify(x)}) when native numbers are enabled. ` + + `Please use native Number instead or disable native number support on the driver level.`)); + } + return () => undefined; + } else { + return () => this.packInteger(x); + } + } + /** * Packs a struct * @param signature the signature of the struct @@ -309,11 +340,18 @@ class Packer { * @access private */ class Unpacker { - constructor () { + + /** + * @constructor + * @param {boolean} useNativeNumbers if this unpacker should treat/convert all received numbers + * (including native {@link Number} type or our own {@link Integer}) as native {@link Number}. + */ + constructor(useNativeNumbers = false) { // Higher level layers can specify how to map structs to higher-level objects. - // If we recieve a struct that has a signature that does not have a mapper, + // If we receive a struct that has a signature that does not have a mapper, // we simply return a Structure object. this.structMappers = {}; + this._useNativeNumbers = useNativeNumbers; } unpack(buffer) { @@ -388,9 +426,10 @@ class Unpacker { let b = buffer.readInt32(); return int(b); } else if (marker == INT_64) { - let high = buffer.readInt32(); - let low = buffer.readInt32(); - return new Integer(low, high); + const high = buffer.readInt32(); + const low = buffer.readInt32(); + const integer = new Integer(low, high); + return this._useNativeNumbers ? integer.toNumberOrInfinity() : integer; } else { return null; } diff --git a/test/types/v1/driver.test.ts b/test/types/v1/driver.test.ts index 0a1660710..d6cb91e89 100644 --- a/test/types/v1/driver.test.ts +++ b/test/types/v1/driver.test.ts @@ -59,6 +59,9 @@ const connectionPoolSize: undefined | number = config.connectionPoolSize; const maxTransactionRetryTime: undefined | number = config.maxTransactionRetryTime; const loadBalancingStrategy1: undefined | LoadBalancingStrategy = config.loadBalancingStrategy; const loadBalancingStrategy2: undefined | string = config.loadBalancingStrategy; +const maxConnectionLifetime: undefined | number = config.maxConnectionLifetime; +const connectionTimeout: undefined | number = config.connectionTimeout; +const useNativeNumbers: undefined | boolean = config.useNativeNumbers; const sessionMode: SessionMode = dummy; const sessionModeStr: string = sessionMode; diff --git a/test/v1/driver.test.js b/test/v1/driver.test.js index 954f53704..2d45ef211 100644 --- a/test/v1/driver.test.js +++ b/test/v1/driver.test.js @@ -325,6 +325,43 @@ describe('driver', () => { testIPv6Connection('bolt://[::1]:7687', done); }); + const nativeNumbers = [ + Number.NEGATIVE_INFINITY, Number.POSITIVE_INFINITY, + Number.MIN_SAFE_INTEGER, Number.MAX_SAFE_INTEGER, + -0, 0, + -42, 42, + -999, 999, + -1000, 1000, + -9000000, 9000000, + Number.MIN_SAFE_INTEGER + 1, Number.MAX_SAFE_INTEGER - 1 + ]; + + nativeNumbers.forEach(number => { + + it(`should return native number ${number} when useNativeNumbers=true`, done => { + testNativeNumberInReturnedRecord(number, done); + }); + + }); + + it('should fail to pack Integer when useNativeNumbers=true', done => { + driver = neo4j.driver('bolt://localhost', sharedNeo4j.authToken, {useNativeNumbers: true}); + const session = driver.session(); + + session.run('RETURN $number', {number: neo4j.int(42)}) + .then(result => { + done.fail(`Was somehow able to pack Integer and received result ${JSON.stringify(result)}`); + }) + .catch(error => { + const message = error.message; + if (message.indexOf('Cannot pack Integer value') === -1) { + done.fail(`Unexpected error message: ${message}`); + } else { + done(); + } + }); + }); + function testIPv6Connection(url, done) { if (serverVersion.compareTo(VERSION_3_1_0) < 0) { // IPv6 listen address only supported starting from neo4j 3.1, so let's ignore the rest @@ -341,6 +378,29 @@ describe('driver', () => { }); } + function testNativeNumberInReturnedRecord(number, done) { + driver = neo4j.driver('bolt://localhost', sharedNeo4j.authToken, {useNativeNumbers: true}); + + const session = driver.session(); + session.run('RETURN $number AS n0, $number AS n1', {number: number}).then(result => { + session.close(); + + const records = result.records; + expect(records.length).toEqual(1); + const record = records[0]; + + expect(record.get('n0')).toEqual(number); + expect(record.get('n1')).toEqual(number); + + expect(record.get(0)).toEqual(number); + expect(record.get(1)).toEqual(number); + + expect(record.toObject()).toEqual({n0: number, n1: number}); + + done(); + }); + } + /** * Starts new transaction to force new network connection. * @param {Driver} driver - the driver to use. diff --git a/test/v1/integer.test.js b/test/v1/integer.test.js index 6f3e19e60..1809f1181 100644 --- a/test/v1/integer.test.js +++ b/test/v1/integer.test.js @@ -17,27 +17,52 @@ * limitations under the License. */ -var v1 = require('../../lib/v1'); -var int = v1.int; -var integer = v1.integer; - -describe('Pool', function() { - it('exposes inSafeRange function', function () { - expect(integer.inSafeRange(int("9007199254740991"))).toBeTruthy(); - expect(integer.inSafeRange(int("9007199254740992"))).toBeFalsy(); - expect(integer.inSafeRange(int("-9007199254740991"))).toBeTruthy(); - expect(integer.inSafeRange(int("-9007199254740992"))).toBeFalsy(); +import neo4j from '../../src/v1'; +import Integer from '../../src/v1/integer'; + +const int = neo4j.int; +const integer = neo4j.integer; + +describe('Integer', () => { + + it('exposes inSafeRange function', () => { + expect(integer.inSafeRange(int('9007199254740991'))).toBeTruthy(); + expect(integer.inSafeRange(int('9007199254740992'))).toBeFalsy(); + expect(integer.inSafeRange(int('-9007199254740991'))).toBeTruthy(); + expect(integer.inSafeRange(int('-9007199254740992'))).toBeFalsy(); + }); + + it('exposes toNumber function', () => { + expect(integer.toNumber(int('9007199254740991'))).toEqual(9007199254740991); + expect(integer.toNumber(int('-9007199254740991'))).toEqual(-9007199254740991); }); - it('exposes toNumber function', function () { - expect(integer.toNumber(int("9007199254740991"))).toEqual(9007199254740991); - expect(integer.toNumber(int("-9007199254740991"))).toEqual(-9007199254740991); + it('exposes toString function', () => { + expect(integer.toString(int('9007199254740991'))).toEqual('9007199254740991'); + expect(integer.toString(int('9007199254740992'))).toEqual('9007199254740992'); + expect(integer.toString(int('-9007199254740991'))).toEqual('-9007199254740991'); + expect(integer.toString(int('-9007199254740992'))).toEqual('-9007199254740992'); }); - it('exposes toString function', function () { - expect(integer.toString(int("9007199254740991"))).toEqual("9007199254740991"); - expect(integer.toString(int("9007199254740992"))).toEqual("9007199254740992"); - expect(integer.toString(int("-9007199254740991"))).toEqual("-9007199254740991"); - expect(integer.toString(int("-9007199254740992"))).toEqual("-9007199254740992"); + it('converts to number when safe', () => { + expect(int('42').toNumberOrInfinity()).toEqual(42); + expect(int('4242').toNumberOrInfinity()).toEqual(4242); + expect(int('-999').toNumberOrInfinity()).toEqual(-999); + expect(int('1000000000').toNumberOrInfinity()).toEqual(1000000000); + expect(Integer.MIN_SAFE_VALUE.toNumberOrInfinity()).toEqual(Integer.MIN_SAFE_VALUE.toNumber()); + expect(Integer.MAX_SAFE_VALUE.toNumberOrInfinity()).toEqual(Integer.MAX_SAFE_VALUE.toNumber()); }); + + it('converts to negative infinity when too small', () => { + expect(Integer.MIN_SAFE_VALUE.subtract(1).toNumberOrInfinity()).toEqual(Number.NEGATIVE_INFINITY); + expect(Integer.MIN_SAFE_VALUE.subtract(42).toNumberOrInfinity()).toEqual(Number.NEGATIVE_INFINITY); + expect(Integer.MIN_SAFE_VALUE.subtract(100).toNumberOrInfinity()).toEqual(Number.NEGATIVE_INFINITY); + }); + + it('converts to positive infinity when too large', () => { + expect(Integer.MAX_SAFE_VALUE.add(1).toNumberOrInfinity()).toEqual(Number.POSITIVE_INFINITY); + expect(Integer.MAX_SAFE_VALUE.add(24).toNumberOrInfinity()).toEqual(Number.POSITIVE_INFINITY); + expect(Integer.MAX_SAFE_VALUE.add(999).toNumberOrInfinity()).toEqual(Number.POSITIVE_INFINITY); + }); + }); diff --git a/types/v1/driver.d.ts b/types/v1/driver.d.ts index a20da7d59..97c5141b4 100644 --- a/types/v1/driver.d.ts +++ b/types/v1/driver.d.ts @@ -54,6 +54,7 @@ declare interface Config { loadBalancingStrategy?: LoadBalancingStrategy; maxConnectionLifetime?: number; connectionTimeout?: number; + useNativeNumbers?: boolean; } declare type SessionMode = "READ" | "WRITE"; From c3fb512e4c831c0f96f7b7e252a7c48d10a55c9c Mon Sep 17 00:00:00 2001 From: lutovich Date: Mon, 29 Jan 2018 15:56:38 +0100 Subject: [PATCH 2/5] Renamed 'useNativeNumbers' to 'disableLosslessIntegers' New name seems more descriptive and emphasizes that numbers returned from driver with this setting set to `true` might result in loss of precision. --- src/v1/index.js | 13 +++++++------ src/v1/internal/connector.js | 10 +++++----- src/v1/internal/packstream.js | 16 ++++++++-------- test/types/v1/driver.test.ts | 13 ++----------- test/v1/driver.test.js | 8 ++++---- types/v1/driver.d.ts | 2 +- 6 files changed, 27 insertions(+), 35 deletions(-) diff --git a/src/v1/index.js b/src/v1/index.js index 2513b4a15..a9d0e0b9a 100644 --- a/src/v1/index.js +++ b/src/v1/index.js @@ -158,16 +158,17 @@ const USER_AGENT = "neo4j-javascript/" + VERSION; * // on the operating system level. Default value is 5000, which is 5 seconds. * connectionTimeout: 5000, // 5 seconds * - * // Make this driver always return and accept native JavaScript number for integer values, instead of the + * // Make this driver always return native JavaScript numbers for integer values, instead of the * // dedicated {@link Integer} class. Values that do not fit in native number bit range will be represented as - * // Number.NEGATIVE_INFINITY or Number.POSITIVE_INFINITY. Driver will fail to encode - * // {@link Integer} values passed as query parameters when this setting is set to true. - * // Warning: It is not safe to enable this setting when JavaScript applications are not the only ones + * // Number.NEGATIVE_INFINITY or Number.POSITIVE_INFINITY. + * // Warning: It is not always safe to enable this setting when JavaScript applications are not the only ones * // interacting with the database. Stored numbers might in such case be not representable by native - * // {@link Number} type and thus driver will return lossy values. For example, this might happen when data was + * // {@link Number} type and thus driver will return lossy values. This might also happen when data was * // initially imported using neo4j import tool and contained numbers larger than * // Number.MAX_SAFE_INTEGER. Driver will then return positive infinity, which is lossy. - * useNativeNumbers: false, + * // Default value for this option is false because native JavaScript numbers might result + * // in loss of precision in the general case. + * disableLosslessIntegers: false, * } * * @param {string} url The URL for the Neo4j database, for instance "bolt://localhost" diff --git a/src/v1/internal/connector.js b/src/v1/internal/connector.js index 8218411c4..54414c623 100644 --- a/src/v1/internal/connector.js +++ b/src/v1/internal/connector.js @@ -164,10 +164,10 @@ class Connection { * @constructor * @param {NodeChannel|WebSocketChannel} channel - channel with a 'write' function and a 'onmessage' callback property. * @param {string} url - the hostname and port to connect to. - * @param {boolean} useNativeNumbers if this connection should treat/convert all received numbers + * @param {boolean} disableLosslessIntegers if this connection should convert all received integers to native JS numbers * (including native {@link Number} type or our own {@link Integer}) as native {@link Number}. */ - constructor(channel, url, useNativeNumbers = false) { + constructor(channel, url, disableLosslessIntegers = false) { /** * An ordered queue of observers, each exchange response (zero or more * RECORD messages followed by a SUCCESS message) we recieve will be routed @@ -181,8 +181,8 @@ class Connection { this._ch = channel; this._dechunker = new Dechunker(); this._chunker = new Chunker( channel ); - this._packer = new Packer(this._chunker, useNativeNumbers); - this._unpacker = new Unpacker(useNativeNumbers); + this._packer = new Packer(this._chunker, disableLosslessIntegers); + this._unpacker = new Unpacker(disableLosslessIntegers); this._isHandlingFailure = false; this._currentFailure = null; @@ -589,7 +589,7 @@ function connect(url, config = {}, connectionErrorCode = null) { const Ch = config.channel || Channel; const parsedUrl = urlUtil.parseBoltUrl(url); const channelConfig = new ChannelConfig(parsedUrl, config, connectionErrorCode); - return new Connection(new Ch(channelConfig), parsedUrl.hostAndPort, config.useNativeNumbers); + return new Connection(new Ch(channelConfig), parsedUrl.hostAndPort, config.disableLosslessIntegers); } export { diff --git a/src/v1/internal/packstream.js b/src/v1/internal/packstream.js index 946a38d8e..0856924ea 100644 --- a/src/v1/internal/packstream.js +++ b/src/v1/internal/packstream.js @@ -80,13 +80,13 @@ class Packer { /** * @constructor * @param {Chunker} channel the chunker backed by a network channel. - * @param {boolean} useNativeNumbers if this packer should treat/convert all received numbers + * @param {boolean} disableLosslessIntegers if this packer should convert all received integers to native JS numbers * (including native {@link Number} type or our own {@link Integer}) as native {@link Number}. */ - constructor(channel, useNativeNumbers = false) { + constructor(channel, disableLosslessIntegers = false) { this._ch = channel; this._byteArraysSupported = true; - this._useNativeNumbers = useNativeNumbers; + this._disableLosslessIntegers = disableLosslessIntegers; } /** @@ -158,7 +158,7 @@ class Packer { * @private */ _packableInteger(x, onError) { - if (this._useNativeNumbers) { + if (this._disableLosslessIntegers) { // pack Integer objects only when native numbers are not used, fail otherwise // Integer can't represent special values like Number.NEGATIVE_INFINITY // and should not be used at all when native numbers are enabled @@ -343,15 +343,15 @@ class Unpacker { /** * @constructor - * @param {boolean} useNativeNumbers if this unpacker should treat/convert all received numbers + * @param {boolean} disableLosslessIntegers if this unpacker should convert all received integers to native JS numbers * (including native {@link Number} type or our own {@link Integer}) as native {@link Number}. */ - constructor(useNativeNumbers = false) { + constructor(disableLosslessIntegers = false) { // Higher level layers can specify how to map structs to higher-level objects. // If we receive a struct that has a signature that does not have a mapper, // we simply return a Structure object. this.structMappers = {}; - this._useNativeNumbers = useNativeNumbers; + this._disableLosslessIntegers = disableLosslessIntegers; } unpack(buffer) { @@ -429,7 +429,7 @@ class Unpacker { const high = buffer.readInt32(); const low = buffer.readInt32(); const integer = new Integer(low, high); - return this._useNativeNumbers ? integer.toNumberOrInfinity() : integer; + return this._disableLosslessIntegers ? integer.toNumberOrInfinity() : integer; } else { return null; } diff --git a/test/types/v1/driver.test.ts b/test/types/v1/driver.test.ts index d6cb91e89..cb332128b 100644 --- a/test/types/v1/driver.test.ts +++ b/test/types/v1/driver.test.ts @@ -17,16 +17,7 @@ * limitations under the License. */ -import Driver, { - AuthToken, - Config, - EncryptionLevel, - LoadBalancingStrategy, - READ, - SessionMode, - TrustStrategy, - WRITE -} from "../../../types/v1/driver"; +import Driver, {AuthToken, Config, EncryptionLevel, LoadBalancingStrategy, READ, SessionMode, TrustStrategy, WRITE} from "../../../types/v1/driver"; import {Parameters} from "../../../types/v1/statement-runner"; import Session from "../../../types/v1/session"; import {Neo4jError} from "../../../types/v1/error"; @@ -61,7 +52,7 @@ const loadBalancingStrategy1: undefined | LoadBalancingStrategy = config.loadBal const loadBalancingStrategy2: undefined | string = config.loadBalancingStrategy; const maxConnectionLifetime: undefined | number = config.maxConnectionLifetime; const connectionTimeout: undefined | number = config.connectionTimeout; -const useNativeNumbers: undefined | boolean = config.useNativeNumbers; +const disableLosslessIntegers: undefined | boolean = config.disableLosslessIntegers; const sessionMode: SessionMode = dummy; const sessionModeStr: string = sessionMode; diff --git a/test/v1/driver.test.js b/test/v1/driver.test.js index 2d45ef211..000cfa065 100644 --- a/test/v1/driver.test.js +++ b/test/v1/driver.test.js @@ -338,14 +338,14 @@ describe('driver', () => { nativeNumbers.forEach(number => { - it(`should return native number ${number} when useNativeNumbers=true`, done => { + it(`should return native number ${number} when disableLosslessIntegers=true`, done => { testNativeNumberInReturnedRecord(number, done); }); }); - it('should fail to pack Integer when useNativeNumbers=true', done => { - driver = neo4j.driver('bolt://localhost', sharedNeo4j.authToken, {useNativeNumbers: true}); + it('should fail to pack Integer when disableLosslessIntegers=true', done => { + driver = neo4j.driver('bolt://localhost', sharedNeo4j.authToken, {disableLosslessIntegers: true}); const session = driver.session(); session.run('RETURN $number', {number: neo4j.int(42)}) @@ -379,7 +379,7 @@ describe('driver', () => { } function testNativeNumberInReturnedRecord(number, done) { - driver = neo4j.driver('bolt://localhost', sharedNeo4j.authToken, {useNativeNumbers: true}); + driver = neo4j.driver('bolt://localhost', sharedNeo4j.authToken, {disableLosslessIntegers: true}); const session = driver.session(); session.run('RETURN $number AS n0, $number AS n1', {number: number}).then(result => { diff --git a/types/v1/driver.d.ts b/types/v1/driver.d.ts index 97c5141b4..17340e762 100644 --- a/types/v1/driver.d.ts +++ b/types/v1/driver.d.ts @@ -54,7 +54,7 @@ declare interface Config { loadBalancingStrategy?: LoadBalancingStrategy; maxConnectionLifetime?: number; connectionTimeout?: number; - useNativeNumbers?: boolean; + disableLosslessIntegers?: boolean; } declare type SessionMode = "READ" | "WRITE"; From bd075b727c210fdbef156a4808874874de1f3196 Mon Sep 17 00:00:00 2001 From: lutovich Date: Mon, 29 Jan 2018 17:01:00 +0100 Subject: [PATCH 3/5] Allow storing of Integer when 'disableLosslessIntegers=true' `Integer` will now be allowed in query parameters even when native JS numbers are configured on the driver level. This is done to not cut away a valid neo4j integer type. It will also result in better backwards compatibility - existing applications will not need to change all `Integer` query parameters when they set `disableLosslessIntegers=true`. --- src/v1/internal/connector.js | 5 ++-- src/v1/internal/packstream.js | 47 +++++++++----------------------- test/v1/driver.test.js | 51 +++++++++++++++++++---------------- 3 files changed, 42 insertions(+), 61 deletions(-) diff --git a/src/v1/internal/connector.js b/src/v1/internal/connector.js index 54414c623..8d4965063 100644 --- a/src/v1/internal/connector.js +++ b/src/v1/internal/connector.js @@ -164,8 +164,7 @@ class Connection { * @constructor * @param {NodeChannel|WebSocketChannel} channel - channel with a 'write' function and a 'onmessage' callback property. * @param {string} url - the hostname and port to connect to. - * @param {boolean} disableLosslessIntegers if this connection should convert all received integers to native JS numbers - * (including native {@link Number} type or our own {@link Integer}) as native {@link Number}. + * @param {boolean} disableLosslessIntegers if this connection should convert all received integers to native JS numbers. */ constructor(channel, url, disableLosslessIntegers = false) { /** @@ -181,7 +180,7 @@ class Connection { this._ch = channel; this._dechunker = new Dechunker(); this._chunker = new Chunker( channel ); - this._packer = new Packer(this._chunker, disableLosslessIntegers); + this._packer = new Packer(this._chunker); this._unpacker = new Unpacker(disableLosslessIntegers); this._isHandlingFailure = false; diff --git a/src/v1/internal/packstream.js b/src/v1/internal/packstream.js index 0856924ea..4951df9c1 100644 --- a/src/v1/internal/packstream.js +++ b/src/v1/internal/packstream.js @@ -80,13 +80,10 @@ class Packer { /** * @constructor * @param {Chunker} channel the chunker backed by a network channel. - * @param {boolean} disableLosslessIntegers if this packer should convert all received integers to native JS numbers - * (including native {@link Number} type or our own {@link Integer}) as native {@link Number}. */ - constructor(channel, disableLosslessIntegers = false) { + constructor(channel) { this._ch = channel; this._byteArraysSupported = true; - this._disableLosslessIntegers = disableLosslessIntegers; } /** @@ -107,7 +104,7 @@ class Packer { } else if (typeof(x) == "string") { return () => this.packString(x, onError); } else if (isInt(x)) { - return this._packableInteger(x, onError); + return () => this.packInteger(x); } else if (x instanceof Int8Array) { return () => this.packBytes(x, onError); } else if (x instanceof Array) { @@ -150,28 +147,6 @@ class Packer { } } - /** - * Creates a packable function out of the provided {@link Integer} value. - * @param {Integer} x the value to pack. - * @param {function} onError the callback for the case when value cannot be packed. - * @return {function} - * @private - */ - _packableInteger(x, onError) { - if (this._disableLosslessIntegers) { - // pack Integer objects only when native numbers are not used, fail otherwise - // Integer can't represent special values like Number.NEGATIVE_INFINITY - // and should not be used at all when native numbers are enabled - if (onError) { - onError(newError(`Cannot pack Integer value ${x} (${JSON.stringify(x)}) when native numbers are enabled. ` + - `Please use native Number instead or disable native number support on the driver level.`)); - } - return () => undefined; - } else { - return () => this.packInteger(x); - } - } - /** * Packs a struct * @param signature the signature of the struct @@ -209,6 +184,7 @@ class Packer { this._ch.writeInt32(low); } } + packFloat(x) { this._ch.writeUInt8(FLOAT_64); this._ch.writeFloat64(x); @@ -343,8 +319,7 @@ class Unpacker { /** * @constructor - * @param {boolean} disableLosslessIntegers if this unpacker should convert all received integers to native JS numbers - * (including native {@link Number} type or our own {@link Integer}) as native {@link Number}. + * @param {boolean} disableLosslessIntegers if this unpacker should convert all received integers to native JS numbers. */ constructor(disableLosslessIntegers = false) { // Higher level layers can specify how to map structs to higher-level objects. @@ -368,9 +343,12 @@ class Unpacker { return boolean; } - const number = this._unpackNumber(marker, buffer); - if (number !== null) { - return number; + const numberOrInteger = this._unpackNumberOrInteger(marker, buffer); + if (numberOrInteger !== null) { + if (this._disableLosslessIntegers && isInt(numberOrInteger)) { + return numberOrInteger.toNumberOrInfinity(); + } + return numberOrInteger; } const string = this._unpackString(marker, markerHigh, markerLow, buffer); @@ -411,7 +389,7 @@ class Unpacker { } } - _unpackNumber(marker, buffer) { + _unpackNumberOrInteger(marker, buffer) { if (marker == FLOAT_64) { return buffer.readFloat64(); } else if (marker >= 0 && marker < 128) { @@ -428,8 +406,7 @@ class Unpacker { } else if (marker == INT_64) { const high = buffer.readInt32(); const low = buffer.readInt32(); - const integer = new Integer(low, high); - return this._disableLosslessIntegers ? integer.toNumberOrInfinity() : integer; + return new Integer(low, high); } else { return null; } diff --git a/test/v1/driver.test.js b/test/v1/driver.test.js index 000cfa065..d88091715 100644 --- a/test/v1/driver.test.js +++ b/test/v1/driver.test.js @@ -339,27 +339,32 @@ describe('driver', () => { nativeNumbers.forEach(number => { it(`should return native number ${number} when disableLosslessIntegers=true`, done => { - testNativeNumberInReturnedRecord(number, done); + testNumberInReturnedRecord(number, number, done); }); }); - it('should fail to pack Integer when disableLosslessIntegers=true', done => { - driver = neo4j.driver('bolt://localhost', sharedNeo4j.authToken, {disableLosslessIntegers: true}); - const session = driver.session(); + const integersWithNativeNumberEquivalent = [ + [neo4j.int(0), 0], + [neo4j.int(42), 42], + [neo4j.int(-100), -100], + [neo4j.int(Number.MIN_SAFE_INTEGER), Number.MIN_SAFE_INTEGER], + [neo4j.int(Number.MAX_SAFE_INTEGER), Number.MAX_SAFE_INTEGER], + [neo4j.int('-9007199254740992'), Number.NEGATIVE_INFINITY], // Number.MIN_SAFE_INTEGER - 1 + [neo4j.int('9007199254740992'), Number.POSITIVE_INFINITY], // Number.MAX_SAFE_INTEGER + 1 + [neo4j.int('-9007199254749999'), Number.NEGATIVE_INFINITY], // Number.MIN_SAFE_INTEGER - 9007 + [neo4j.int('9007199254749999'), Number.POSITIVE_INFINITY], // Number.MAX_SAFE_INTEGER + 9008 + ]; + + integersWithNativeNumberEquivalent.forEach(integerWithNativeNumber => { + + const integer = integerWithNativeNumber[0]; + const nativeNumber = integerWithNativeNumber[1]; + + it(`should send Integer ${integer.toString()} and return native number ${nativeNumber} when disableLosslessIntegers=true`, done => { + testNumberInReturnedRecord(integer, nativeNumber, done); + }); - session.run('RETURN $number', {number: neo4j.int(42)}) - .then(result => { - done.fail(`Was somehow able to pack Integer and received result ${JSON.stringify(result)}`); - }) - .catch(error => { - const message = error.message; - if (message.indexOf('Cannot pack Integer value') === -1) { - done.fail(`Unexpected error message: ${message}`); - } else { - done(); - } - }); }); function testIPv6Connection(url, done) { @@ -378,24 +383,24 @@ describe('driver', () => { }); } - function testNativeNumberInReturnedRecord(number, done) { + function testNumberInReturnedRecord(inputNumber, expectedNumber, done) { driver = neo4j.driver('bolt://localhost', sharedNeo4j.authToken, {disableLosslessIntegers: true}); const session = driver.session(); - session.run('RETURN $number AS n0, $number AS n1', {number: number}).then(result => { + session.run('RETURN $number AS n0, $number AS n1', {number: inputNumber}).then(result => { session.close(); const records = result.records; expect(records.length).toEqual(1); const record = records[0]; - expect(record.get('n0')).toEqual(number); - expect(record.get('n1')).toEqual(number); + expect(record.get('n0')).toEqual(expectedNumber); + expect(record.get('n1')).toEqual(expectedNumber); - expect(record.get(0)).toEqual(number); - expect(record.get(1)).toEqual(number); + expect(record.get(0)).toEqual(expectedNumber); + expect(record.get(1)).toEqual(expectedNumber); - expect(record.toObject()).toEqual({n0: number, n1: number}); + expect(record.toObject()).toEqual({n0: expectedNumber, n1: expectedNumber}); done(); }); From 8edd2f6c0a39fed9b5e5753d52613615b9bc333c Mon Sep 17 00:00:00 2001 From: lutovich Date: Mon, 29 Jan 2018 18:50:07 +0100 Subject: [PATCH 4/5] TS declarations support for 'disableLosslessIntegers=true' Previously TS declaration files only allowed returned integer types to be `neo4j.Integer`. This was not compatible with newly introduced `disableLosslessIntegers` config, which makes driver always return native JS numbers. This commit allows all integer properties to either be `neo4j.Integer` or `number`. `Integer` is default. Existing TS users will not need to change their code. TS users who set 'disableLosslessIntegers=true' will have to specify generic type explicitly like `Node`. Property `identity` of such node will have type `number`. --- test/types/v1/graph-types.test.ts | 25 +++++++++++++ test/types/v1/result-summary.test.ts | 41 ++++++++++---------- types/v1/graph-types.d.ts | 56 ++++++++++++++-------------- types/v1/result-summary.d.ts | 6 +-- 4 files changed, 77 insertions(+), 51 deletions(-) diff --git a/test/types/v1/graph-types.test.ts b/test/types/v1/graph-types.test.ts index 391bb4395..20ecc252d 100644 --- a/test/types/v1/graph-types.test.ts +++ b/test/types/v1/graph-types.test.ts @@ -26,6 +26,9 @@ const node1Id: Integer = node1.identity; const node1Labels: string[] = node1.labels; const node1Props: object = node1.properties; +const node2: Node = new Node(2, ["Person", "Employee"], {name: "Alice"}); +const node2Id: number = node2.identity; + const rel1: Relationship = new Relationship(int(1), int(2), int(3), "KNOWS", {since: 12345}); const rel1String: string = rel1.toString(); const rel1Id: Integer = rel1.identity; @@ -41,13 +44,35 @@ const rel2Id: Integer = rel2.identity; const rel2Type: string = rel2.type; const rel2Props: object = rel2.properties; +const rel4: Relationship = new Relationship(2, 3, 4, "KNOWS", {since: 12345}); +const rel4Id: number = rel4.identity; +const rel4Start: number = rel4.start; +const rel4End: number = rel4.end; + +const rel5: UnboundRelationship = new UnboundRelationship(5, "KNOWS", {since: 12345}); +const rel5Id: number = rel5.identity; +const rel6 = rel5.bind(24, 42); +const rel6Id: number = rel6.identity; +const rel6Start: number = rel6.start; +const rel6End: number = rel6.end; + const pathSegment1: PathSegment = new PathSegment(node1, rel1, node1); const pathSegment1Start: Node = pathSegment1.start; const pathSegment1Rel: Relationship = pathSegment1.relationship; const pathSegment1End: Node = pathSegment1.end; +const pathSegment2: PathSegment = new PathSegment(node2, rel4, node2); +const pathSegment2Start: Node = pathSegment2.start; +const pathSegment2Rel: Relationship = pathSegment2.relationship; +const pathSegment2End: Node = pathSegment2.end; + const path1: Path = new Path(node1, node1, [pathSegment1]); const path1Start: Node = path1.start; const path1End: Node = path1.end; const path1Segments: PathSegment[] = path1.segments; const path1Length: number = path1.length; + +const path2: Path = new Path(node2, node2, [pathSegment2]); +const path2Start: Node = path2.start; +const path2End: Node = path2.end; +const path2Segments: PathSegment[] = path2.segments; diff --git a/test/types/v1/result-summary.test.ts b/test/types/v1/result-summary.test.ts index bef455309..e03538d4b 100644 --- a/test/types/v1/result-summary.test.ts +++ b/test/types/v1/result-summary.test.ts @@ -17,27 +17,20 @@ * limitations under the License. */ -import ResultSummary, { - Notification, - NotificationPosition, - Plan, - ProfiledPlan, - ServerInfo, - StatementStatistic -} from "../../../types/v1/result-summary"; +import ResultSummary, {Notification, NotificationPosition, Plan, ProfiledPlan, ServerInfo, StatementStatistic} from "../../../types/v1/result-summary"; import Integer from "../../../types/v1/integer"; const dummy: any = null; -const sum: ResultSummary = dummy; +const sum1: ResultSummary = dummy; -const stmt = sum.statement; +const stmt = sum1.statement; const stmtText: string = stmt.text; const stmtParams: object = stmt.parameters; -const str: string = sum.statementType; +const str: string = sum1.statementType; -const counters: StatementStatistic = sum.counters; +const counters: StatementStatistic = sum1.counters; const containsUpdates: boolean = counters.containsUpdates(); const nodesCreated: number = counters.nodesCreated(); @@ -52,13 +45,13 @@ const indexesRemoved: number = counters.indexesRemoved(); const constraintsAdded: number = counters.constraintsAdded(); const constraintsRemoved: number = counters.constraintsRemoved(); -const plan: Plan = sum.plan; +const plan: Plan = sum1.plan; const planOperatorType: string = plan.operatorType; const planIdentifiers: string[] = plan.identifiers; const planArguments: { [key: string]: string } = plan.arguments; const planChildren: Plan[] = plan.children; -const profile: ProfiledPlan = sum.profile; +const profile: ProfiledPlan = sum1.profile; const profileOperatorType: string = profile.operatorType; const profileIdentifiers: string[] = profile.identifiers; const profileArguments: { [key: string]: string } = profile.arguments; @@ -66,7 +59,7 @@ const profileDbHits: number = profile.dbHits; const profileRows: number = profile.rows; const profileChildren: ProfiledPlan[] = profile.children; -const notifications: Notification[] = sum.notifications; +const notifications: Notification[] = sum1.notifications; const notification: Notification = notifications[0]; const code: string = notification.code; const title: string = notification.title; @@ -78,12 +71,20 @@ const offset: number = position2.offset; const line: number = position2.line; const column: number = position2.column; -const server: ServerInfo = sum.server; +const server: ServerInfo = sum1.server; const address: string = server.address; const version: string = server.version; -const resultConsumedAfter: Integer = sum.resultConsumedAfter; -const resultAvailableAfter: Integer = sum.resultAvailableAfter; +const resultConsumedAfter1: Integer = sum1.resultConsumedAfter; +const resultAvailableAfter1: Integer = sum1.resultAvailableAfter; -const hasPlan: boolean = sum.hasPlan(); -const hasProfile: boolean = sum.hasProfile(); +const hasPlan: boolean = sum1.hasPlan(); +const hasProfile: boolean = sum1.hasProfile(); + +const sum2: ResultSummary = dummy; +const resultConsumedAfter2: number = sum2.resultConsumedAfter; +const resultAvailableAfter2: number = sum2.resultAvailableAfter; + +const sum3: ResultSummary = dummy; +const resultConsumedAfter3: Integer = sum3.resultConsumedAfter; +const resultAvailableAfter3: Integer = sum3.resultAvailableAfter; diff --git a/types/v1/graph-types.d.ts b/types/v1/graph-types.d.ts index 586dce822..11ca1a809 100644 --- a/types/v1/graph-types.d.ts +++ b/types/v1/graph-types.d.ts @@ -19,67 +19,67 @@ import Integer from "./integer"; -declare class Node { - identity: Integer; +declare class Node { + identity: T; labels: string[]; properties: object; - constructor(identity: Integer, + constructor(identity: T, labels: string[], properties: object) toString(): string; } -declare class Relationship { - identity: Integer; - start: Integer; - end: Integer; +declare class Relationship { + identity: T; + start: T; + end: T; type: string; properties: object; - constructor(identity: Integer, - start: Integer, - end: Integer, + constructor(identity: T, + start: T, + end: T, type: string, properties: object); toString(): string; } -declare class UnboundRelationship { - identity: Integer; +declare class UnboundRelationship { + identity: T; type: string; properties: object; - constructor(identity: Integer, + constructor(identity: T, type: string, properties: object); - bind(start: Integer, end: Integer): Relationship; + bind(start: T, end: T): Relationship; toString(): string; } -declare class PathSegment { - start: Node; - relationship: Relationship; - end: Node; +declare class PathSegment { + start: Node; + relationship: Relationship; + end: Node; - constructor(start: Node, - rel: Relationship, - end: Node); + constructor(start: Node, + rel: Relationship, + end: Node); } -declare class Path { - start: Node; - end: Node; - segments: PathSegment[]; +declare class Path { + start: Node; + end: Node; + segments: PathSegment[]; length: number; - constructor(start: Node, - end: Node, - segments: PathSegment[]); + constructor(start: Node, + end: Node, + segments: PathSegment[]); } export { diff --git a/types/v1/result-summary.d.ts b/types/v1/result-summary.d.ts index 6e615f9d3..e227cfb75 100644 --- a/types/v1/result-summary.d.ts +++ b/types/v1/result-summary.d.ts @@ -19,7 +19,7 @@ import Integer from "./integer"; -declare interface ResultSummary { +declare interface ResultSummary { statement: { text: string, parameters: { [key: string]: any } }; statementType: string; counters: StatementStatistic; @@ -27,8 +27,8 @@ declare interface ResultSummary { profile: ProfiledPlan; notifications: Notification[]; server: ServerInfo; - resultConsumedAfter: Integer; - resultAvailableAfter: Integer; + resultConsumedAfter: T; + resultAvailableAfter: T; hasPlan(): boolean; From 7cc68d8e380cb5cdcc40df8efd0b6e685b6a5522 Mon Sep 17 00:00:00 2001 From: lutovich Date: Tue, 30 Jan 2018 10:16:52 +0100 Subject: [PATCH 5/5] Extract NumberOrInteger type in TS declarations To deduplicate and reuse `(number | Integer)` generic type declaration. --- types/v1/graph-types.d.ts | 15 +++++++++------ types/v1/result-summary.d.ts | 3 ++- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/types/v1/graph-types.d.ts b/types/v1/graph-types.d.ts index 11ca1a809..da8b52bad 100644 --- a/types/v1/graph-types.d.ts +++ b/types/v1/graph-types.d.ts @@ -19,7 +19,9 @@ import Integer from "./integer"; -declare class Node { +declare type NumberOrInteger = number | Integer; + +declare class Node { identity: T; labels: string[]; properties: object; @@ -31,7 +33,7 @@ declare class Node { toString(): string; } -declare class Relationship { +declare class Relationship { identity: T; start: T; end: T; @@ -47,7 +49,7 @@ declare class Relationship { toString(): string; } -declare class UnboundRelationship { +declare class UnboundRelationship { identity: T; type: string; properties: object; @@ -61,7 +63,7 @@ declare class UnboundRelationship { toString(): string; } -declare class PathSegment { +declare class PathSegment { start: Node; relationship: Relationship; end: Node; @@ -71,7 +73,7 @@ declare class PathSegment { end: Node); } -declare class Path { +declare class Path { start: Node; end: Node; segments: PathSegment[]; @@ -87,5 +89,6 @@ export { Relationship, UnboundRelationship, Path, - PathSegment + PathSegment, + NumberOrInteger } diff --git a/types/v1/result-summary.d.ts b/types/v1/result-summary.d.ts index e227cfb75..f6c452ee6 100644 --- a/types/v1/result-summary.d.ts +++ b/types/v1/result-summary.d.ts @@ -18,8 +18,9 @@ */ import Integer from "./integer"; +import {NumberOrInteger} from "./graph-types"; -declare interface ResultSummary { +declare interface ResultSummary { statement: { text: string, parameters: { [key: string]: any } }; statementType: string; counters: StatementStatistic;