Skip to content

Add preferNativeInt for unpack operation #94

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 35 additions & 0 deletions src/v1/integer.js
Original file line number Diff line number Diff line change
Expand Up @@ -718,6 +718,23 @@ Integer.fromValue = function(val) {
return new Integer(val.low, val.high);
};

/**
* Returns a number or Integer for a given pair of low and high bits, preferring number when in scope.
* @access private
* @param {number} lowBits The low 32 bits
* @param {number} highBits The high 32 bits
* @returns {!Integer|number} The corresponding Integer value
* @expose
*/
Integer.asNative = function(lowBits, highBits) {
var val = new Integer(lowBits, highBits);
if (val.greaterThan(MIN_NATIVE) && val.lessThan(MAX_NATIVE)) {
return val.toNumber();
} else {
return val;
}
};

/**
* @type {number}
* @const
Expand Down Expand Up @@ -801,6 +818,24 @@ Integer.MAX_VALUE = Integer.fromBits(0xFFFFFFFF|0, 0x7FFFFFFF|0, false);
*/
Integer.MIN_VALUE = Integer.fromBits(0, 0x80000000|0, false);

/**
* Minimum safe native value.
* @type {!Integer}
* @const
* @inner
* @private
*/
var MIN_NATIVE = Integer.fromValue(Number.MIN_SAFE_INTEGER);

/**
* Maximum safe native value.
* @type {!Integer}
* @const
* @inner
* @private
*/
var MAX_NATIVE = Integer.fromValue(Number.MAX_SAFE_INTEGER);

Copy link

@swist swist Aug 12, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason you have added these as variables in module scope? Perhaps you could attach them to the Integer constructor like so:

Integer.MIN_NATIVE = Integer.fromValue(Number.MIN_SAFE_INTEGER)
Integer.MAX_NATIVE = Integer.fromValue(Number.MAX_SAFE_INTEGER)

I'd do it myself, but I'm not sure if mucking about with someone's PR is frowned upon :)

It would be nice to have these available in the same place like other MIN_MAX values, perhaps you could even reuse the others, but I don't have the driver on hand to test myself. Might try over lunch

[edit] d'oh, just saw @pontusmelke's comment...

/**
* Cast value to Integer type.
* @access public
Expand Down
6 changes: 3 additions & 3 deletions src/v1/internal/connector.js
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ class Connection {
* @param channel - channel with a 'write' function and a 'onmessage'
* callback property
*/
constructor (channel) {
constructor (channel, config) {
/**
* An ordered queue of observers, each exchange response (zero or more
* RECORD messages followed by a SUCCESS message) we recieve will be routed
Expand All @@ -173,7 +173,7 @@ class Connection {
this._dechunker = new chunking.Dechunker();
this._chunker = new chunking.Chunker( channel );
this._packer = new packstream.Packer( this._chunker );
this._unpacker = new packstream.Unpacker();
this._unpacker = new packstream.Unpacker( config );
this._isHandlingFailure = false;

// Set to true on fatal errors, to get this out of session pool.
Expand Down Expand Up @@ -428,7 +428,7 @@ function connect( url, config = {}) {
trust : config.trust || (hasFeature("trust_on_first_use") ? "TRUST_ON_FIRST_USE" : "TRUST_SIGNED_CERTIFICATES"),
trustedCertificates : config.trustedCertificates || [],
knownHosts : config.knownHosts
}));
}), config);
}

export default {
Expand Down
38 changes: 27 additions & 11 deletions src/v1/internal/packstream.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/

import {debug} from "./log";
import {alloc} from "./buf";
import utf8 from "./utf8";
Expand Down Expand Up @@ -244,7 +244,9 @@ class Packer {
* @access private
*/
class Unpacker {
constructor () {
constructor (config) {
this._config = config || {};

// 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,
// we simply return a Structure object.
Expand All @@ -255,7 +257,7 @@ class Unpacker {
let value = [];
for(let i = 0; i < size; i++) {
value.push( this.unpack( buffer ) );
}
}
return value;
}

Expand All @@ -277,8 +279,23 @@ class Unpacker {
let value = new Structure(signature, []);
for(let i = 0; i < size; i++) {
value.fields.push(this.unpack(buffer));
}
}
return value;
}
}

unpackSmallInt (value) {
if( this._config.preferNativeInt ) {
return value;
} else {
return int(value);
}
}
unpackLargeInt(low, high) {
if( this._config.preferNativeInt ) {
return Integer.asNative(low, high);
} else {
return Integer.fromBits(low, high);
}
}

Expand All @@ -293,20 +310,19 @@ class Unpacker {
} else if (marker == FLOAT_64) {
return buffer.readFloat64();
} else if (marker >= 0 && marker < 128) {
return int(marker);
return this.unpackSmallInt(marker);
} else if (marker >= 240 && marker < 256) {
return int(marker - 256);
return this.unpackSmallInt(marker - 256);
} else if (marker == INT_8) {
return int(buffer.readInt8());
return this.unpackSmallInt(buffer.readInt8());
} else if (marker == INT_16) {
return int(buffer.readInt16());
return this.unpackSmallInt(buffer.readInt16());
} else if (marker == INT_32) {
let b = buffer.readInt32();
return int(b);
return this.unpackSmallInt(buffer.readInt32());
} else if (marker == INT_64) {
let high = buffer.readInt32();
let low = buffer.readInt32();
return new Integer( low, high );
return this.unpackLargeInt( low, high );
} else if (marker == STRING_8) {
return utf8.decode( buffer, buffer.readUInt8());
} else if (marker == STRING_16) {
Expand Down
11 changes: 7 additions & 4 deletions test/internal/packstream.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/

var alloc = require('../../lib/v1/internal/buf').alloc,
packstream = require("../../lib/v1/internal/packstream.js"),
integer = require("../../lib/v1/integer.js"),
Expand All @@ -32,16 +32,19 @@ describe('packstream', function() {
for(n = -999; n <= 999; n += 1) {
i = Integer.fromNumber(n);
expect( packAndUnpack( i ).toString() ).toBe( i.toString() );
expect( packAndUnpack( i, undefined, {preferNativeInt: true} ) ).toBe( i.toNumber() );
}
// positive numbers
for(n = 16; n <= 16 ; n += 1) {
i = Integer.fromNumber(Math.pow(2, n));
expect( packAndUnpack( i ).toString() ).toBe( i.toString() );
expect( packAndUnpack( i, undefined, {preferNativeInt: true} ) ).toBe( i.toNumber() );
}
// negative numbers
for(n = 0; n <= 63 ; n += 1) {
i = Integer.fromNumber(-Math.pow(2, n));
expect( packAndUnpack( i ).toString() ).toBe( i.toString() );
expect( packAndUnpack( i, undefined, {preferNativeInt: true} ).toString() ).toEqual( i.toString() );
}
});
it('should pack strings', function() {
Expand All @@ -51,7 +54,7 @@ describe('packstream', function() {
expect( packAndUnpack(str, str.length + 8)).toBe(str);
});
it('should pack structures', function() {
expect( packAndUnpack( new Structure(1, ["Hello, world!!!"] ) ).fields[0] )
expect( packAndUnpack( new Structure(1, ["Hello, world!!!"] ) ).fields[0] )
.toBe( "Hello, world!!!" );
});
it('should pack lists', function() {
Expand All @@ -73,10 +76,10 @@ describe('packstream', function() {
});
});

function packAndUnpack( val, bufferSize ) {
function packAndUnpack( val, bufferSize, options ) {
bufferSize = bufferSize || 128;
var buffer = alloc(bufferSize);
new Packer( buffer ).pack( val );
buffer.reset();
return new Unpacker().unpack( buffer );
return new Unpacker(options).unpack( buffer );
}
8 changes: 5 additions & 3 deletions test/v1/types.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ describe('integer values', function() {
it('should support integer -1 ', testVal( neo4j.int(-1) ) );
it('should support integer larger than JS Numbers can model', testVal( neo4j.int("0x7fffffffffffffff") ) );
it('should support integer smaller than JS Numbers can model', testVal( neo4j.int("0x8000000000000000") ) );

it('should support integer 1 casting to native ', testVal( neo4j.int(1), 1, {preferNativeInt: true} ) );
});

describe('boolean values', function() {
Expand Down Expand Up @@ -131,14 +133,14 @@ describe('path values', function() {
});
});

function testVal( val ) {
function testVal( val, expected, config ) {
return function( done ) {
var driver = neo4j.driver("bolt://localhost", neo4j.auth.basic("neo4j", "neo4j"));
var driver = neo4j.driver("bolt://localhost", neo4j.auth.basic("neo4j", "neo4j"), config);
var session = driver.session();

session.run("RETURN {val} as v", {val: val})
.then( function( result ) {
expect( result.records[0].get('v') ).toEqual( val );
expect( result.records[0].get('v') ).toEqual( expected || val );
driver.close();
done();
}).catch(function(err) { console.log(err); });
Expand Down