From a454f26572b664ad71b7b8dfa4097d0da6fcfc74 Mon Sep 17 00:00:00 2001 From: lutovich Date: Wed, 15 Feb 2017 14:51:54 +0100 Subject: [PATCH 1/2] Move connector tests to ES6 --- test/internal/connector.test.js | 67 ++++++++++++++++----------------- 1 file changed, 33 insertions(+), 34 deletions(-) diff --git a/test/internal/connector.test.js b/test/internal/connector.test.js index a99cb9ce2..126a486ff 100644 --- a/test/internal/connector.test.js +++ b/test/internal/connector.test.js @@ -16,43 +16,43 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -var DummyChannel = require('../../lib/v1/internal/ch-dummy.js'); -var connect = require("../../lib/v1/internal/connector.js").connect; -describe('connector', function() { +import * as DummyChannel from "../../src/v1/internal/ch-dummy"; +import {connect} from "../../src/v1/internal/connector"; - it('should read/write basic messages', function(done) { +describe('connector', () => { + + it('should read/write basic messages', done => { // Given - var conn = connect("bolt://localhost") + const conn = connect("bolt://localhost"); // When - conn.initialize( "mydriver/0.0.0", {scheme: "basic", principal: "neo4j", credentials: "neo4j"}, { - onCompleted: function( msg ) { - expect( msg ).not.toBeNull(); + conn.initialize("mydriver/0.0.0", {scheme: "basic", principal: "neo4j", credentials: "neo4j"}, { + onCompleted: msg => { + expect(msg).not.toBeNull(); conn.close(); done(); }, - onError: function(err) { - console.log(err); - } + onError: console.log }); conn.sync(); }); - it('should retrieve stream', function(done) { + + it('should retrieve stream', done => { // Given - var conn = connect("bolt://localhost") + const conn = connect("bolt://localhost"); // When - var records = []; - conn.initialize( "mydriver/0.0.0", {scheme: "basic", principal: "neo4j", credentials: "neo4j"} ); - conn.run( "RETURN 1.0", {} ); - conn.pullAll( { - onNext: function( record ) { - records.push( record ); + const records = []; + conn.initialize("mydriver/0.0.0", {scheme: "basic", principal: "neo4j", credentials: "neo4j"}); + conn.run("RETURN 1.0", {}); + conn.pullAll({ + onNext: record => { + records.push(record); }, - onCompleted: function( tail ) { - expect( records[0][0] ).toBe( 1 ); + onCompleted: () => { + expect(records[0][0]).toBe(1); conn.close(); done(); } @@ -60,31 +60,30 @@ describe('connector', function() { conn.sync(); }); - it('should use DummyChannel to read what gets written', function(done) { + it('should use DummyChannel to read what gets written', done => { // Given - var observer = DummyChannel.observer; - var conn = connect("bolt://localhost", {channel:DummyChannel.channel}); + const observer = DummyChannel.observer; + const conn = connect("bolt://localhost", {channel: DummyChannel.channel}); // When - var records = []; - conn.initialize( "mydriver/0.0.0", {scheme: "basic", principal: "neo4j", credentials: "neo4j"} ); - conn.run( "RETURN 1", {} ); + conn.initialize("mydriver/0.0.0", {scheme: "basic", principal: "neo4j", credentials: "neo4j"}); + conn.run("RETURN 1", {}); conn.sync(); - expect( observer.instance.toHex() ).toBe( '60 60 b0 17 00 00 00 01 00 00 00 00 00 00 00 00 00 00 00 00 00 41 b2 01 8e 6d 79 64 72 69 76 65 72 2f 30 2e 30 2e 30 a3 86 73 63 68 65 6d 65 85 62 61 73 69 63 89 70 72 69 6e 63 69 70 61 6c 85 6e 65 6f 34 6a 8b 63 72 65 64 65 6e 74 69 61 6c 73 85 6e 65 6f 34 6a 00 00 00 0c b2 10 88 52 45 54 55 52 4e 20 31 a0 00 00 ' ); + expect(observer.instance.toHex()).toBe('60 60 b0 17 00 00 00 01 00 00 00 00 00 00 00 00 00 00 00 00 00 41 b2 01 8e 6d 79 64 72 69 76 65 72 2f 30 2e 30 2e 30 a3 86 73 63 68 65 6d 65 85 62 61 73 69 63 89 70 72 69 6e 63 69 70 61 6c 85 6e 65 6f 34 6a 8b 63 72 65 64 65 6e 74 69 61 6c 73 85 6e 65 6f 34 6a 00 00 00 0c b2 10 88 52 45 54 55 52 4e 20 31 a0 00 00 '); done(); }); - it('should provide error message when connecting to http-port', function(done) { + it('should provide error message when connecting to http-port', done => { // Given - var conn = connect("bolt://localhost:7474", {encrypted:false}); + const conn = connect("bolt://localhost:7474", {encrypted: false}); // When - conn.initialize( "mydriver/0.0.0", {scheme: "basic", principal: "neo4j", credentials: "neo4j"}, { - onCompleted: function( msg ) { + conn.initialize("mydriver/0.0.0", {scheme: "basic", principal: "neo4j", credentials: "neo4j"}, { + onCompleted: msg => { }, - onError: function(err) { + onError: err => { //only node gets the pretty error message - if( require('../../lib/v1/internal/ch-node.js').available ) { + if (require('../../lib/v1/internal/ch-node.js').available) { expect(err.message).toBe("Server responded HTTP. Make sure you are not trying to connect to the http endpoint " + "(HTTP defaults to port 7474 whereas BOLT defaults to port 7687)"); } From 517e47dad1a0966740ea704de0189551e4b8e1ea Mon Sep 17 00:00:00 2001 From: lutovich Date: Thu, 16 Feb 2017 11:41:10 +0100 Subject: [PATCH 2/2] Correctly unpack failure message to Neo4jError Previously raw FAILURE message was passed to `onError`-like callbacks and thrown as is. This means thrown error contained packstream related fields and actually was a packstream structure. Example: ``` Structure { signature: 127, fields: [ { code: 'Neo.ClientError.Schema.ConstraintValidationFailed', message: 'Node 0 already exists with label A and property "a"=[42]' } ] } ``` This commit makes sure we unpack/convert FAILURE messages to `Neo4jError`s and pass those further to callbacks. --- src/v1/internal/buf.js | 3 +- src/v1/internal/connector.js | 40 ++++++++++--------- src/v1/internal/get-servers-util.js | 14 +------ src/v1/routing-driver.js | 32 +++------------ test/internal/connector.test.js | 52 ++++++++++++++++++++++++- test/v1/driver.test.js | 2 +- test/v1/examples.test.js | 2 +- test/v1/session.test.js | 8 ++-- test/v1/tck/steps/authsteps.js | 4 +- test/v1/tck/steps/erroreportingsteps.js | 2 +- test/v1/transaction.test.js | 15 ++++--- 11 files changed, 96 insertions(+), 78 deletions(-) diff --git a/src/v1/internal/buf.js b/src/v1/internal/buf.js index 057f45bf7..4c88a2062 100644 --- a/src/v1/internal/buf.js +++ b/src/v1/internal/buf.js @@ -22,7 +22,6 @@ *(via Buffer API). */ -import {newError} from './../error'; let _node = require("buffer"); /** * Common base with default implementation for most buffer methods. @@ -551,7 +550,7 @@ class NodeBuffer extends BaseBuffer { val.position + bytesToCopy ); val.position += bytesToCopy; } else { - throw newError("Copying not yet implemented."); + super.putBytes(position, val); } }; diff --git a/src/v1/internal/connector.js b/src/v1/internal/connector.js index e922f2aff..901abeefc 100644 --- a/src/v1/internal/connector.js +++ b/src/v1/internal/connector.js @@ -18,11 +18,11 @@ */ import WebSocketChannel from './ch-websocket'; import NodeChannel from './ch-node'; -import {Dechunker, Chunker} from "./chunking"; +import {Chunker, Dechunker} from './chunking'; import hasFeature from './features'; import {Packer, Unpacker} from './packstream'; import {alloc} from './buf'; -import {Node, Relationship, UnboundRelationship, Path, PathSegment} from '../graph-types' +import {Node, Path, PathSegment, Relationship, UnboundRelationship} from '../graph-types'; import {newError} from './../error'; let Channel; @@ -201,7 +201,9 @@ class Connection { this._chunker = new Chunker( channel ); this._packer = new Packer( this._chunker ); this._unpacker = new Unpacker(); + this._isHandlingFailure = false; + this._currentFailure = null; // Set to true on fatal errors, to get this out of session pool. this._isBroken = false; @@ -288,15 +290,17 @@ class Connection { } _handleMessage( msg ) { + const payload = msg.fields[0]; + switch( msg.signature ) { case RECORD: - log("S", "RECORD", msg.fields[0]); - this._currentObserver.onNext( msg.fields[0] ); + log("S", "RECORD", msg); + this._currentObserver.onNext( payload ); break; case SUCCESS: - log("S", "SUCCESS", msg.fields[0]); + log("S", "SUCCESS", msg); try { - this._currentObserver.onCompleted( msg.fields[0] ); + this._currentObserver.onCompleted( payload ); } finally { this._currentObserver = this._pendingObservers.shift(); } @@ -304,15 +308,14 @@ class Connection { case FAILURE: log("S", "FAILURE", msg); try { - this._currentObserver.onError( msg ); - this._errorMsg = msg; + this._currentFailure = newError(payload.message, payload.code); + this._currentObserver.onError( this._currentFailure ); } finally { this._currentObserver = this._pendingObservers.shift(); // Things are now broken. Pending observers will get FAILURE messages routed until // We are done handling this failure. if( !this._isHandlingFailure ) { this._isHandlingFailure = true; - let self = this; // isHandlingFailure was false, meaning this is the first failure message // we see from this failure. We may see several others, one for each message @@ -320,22 +323,23 @@ class Connection { // We only want to and need to ACK the first one, which is why we are tracking // this _isHandlingFailure thing. this._ackFailure({ - onNext: NO_OP, - onError: NO_OP, - onCompleted: () => { - self._isHandlingFailure = false; - } + onNext: NO_OP, + onError: NO_OP, + onCompleted: () => { + this._isHandlingFailure = false; + this._currentFailure = null; + } }); } } break; case IGNORED: - log("S", "IGNORED"); + log("S", "IGNORED", msg); try { - if (this._errorMsg && this._currentObserver.onError) - this._currentObserver.onError(this._errorMsg); + if (this._currentFailure && this._currentObserver.onError) + this._currentObserver.onError(this._currentFailure); else if(this._currentObserver.onError) - this._currentObserver.onError(msg); + this._currentObserver.onError(payload); } finally { this._currentObserver = this._pendingObservers.shift(); } diff --git a/src/v1/internal/get-servers-util.js b/src/v1/internal/get-servers-util.js index 70e4ed9ed..0591f8b29 100644 --- a/src/v1/internal/get-servers-util.js +++ b/src/v1/internal/get-servers-util.js @@ -31,7 +31,7 @@ export default class GetServersUtil { session.close(); return result.records; }).catch(error => { - if (this._isProcedureNotFoundError(error)) { + if (error.code === PROCEDURE_NOT_FOUND_CODE) { // throw when getServers procedure not found because this is clearly a configuration issue throw newError('Server ' + routerAddress + ' could not perform routing. ' + 'Make sure you are connecting to a causal cluster', SERVICE_UNAVAILABLE); @@ -92,16 +92,4 @@ export default class GetServersUtil { PROTOCOL_ERROR); } } - - _isProcedureNotFoundError(error) { - let errorCode = error.code; - if (!errorCode) { - try { - errorCode = error.fields[0].code; - } catch (e) { - errorCode = 'UNKNOWN'; - } - } - return errorCode === PROCEDURE_NOT_FOUND_CODE; - } } diff --git a/src/v1/routing-driver.js b/src/v1/routing-driver.js index 62431a220..bebf667a9 100644 --- a/src/v1/routing-driver.js +++ b/src/v1/routing-driver.js @@ -36,28 +36,8 @@ class RoutingDriver extends Driver { } _createSession(connectionPromise, cb) { - return new RoutingSession(connectionPromise, cb, (err, conn) => { - let code = err.code; - let msg = err.message; - if (!code) { - try { - code = err.fields[0].code; - } catch (e) { - code = 'UNKNOWN'; - } - } - if (!msg) { - try { - msg = err.fields[0].message; - } catch (e) { - msg = 'Unknown failure occurred'; - } - } - //just to simplify later error handling - err.code = code; - err.message = msg; - - if (code === SERVICE_UNAVAILABLE || code === SESSION_EXPIRED) { + return new RoutingSession(connectionPromise, cb, (error, conn) => { + if (error.code === SERVICE_UNAVAILABLE || error.code === SESSION_EXPIRED) { if (conn) { this._forget(conn.url) } else { @@ -65,8 +45,8 @@ class RoutingDriver extends Driver { this._forget(conn.url); }).catch(() => {/*ignore*/}); } - return err; - } else if (code === 'Neo.ClientError.Cluster.NotALeader') { + return error; + } else if (error.code === 'Neo.ClientError.Cluster.NotALeader') { let url = 'UNKNOWN'; if (conn) { url = conn.url; @@ -76,9 +56,9 @@ class RoutingDriver extends Driver { this._routingTable.forgetWriter(conn.url); }).catch(() => {/*ignore*/}); } - return newError("No longer possible to write to server at " + url, SESSION_EXPIRED); + return newError('No longer possible to write to server at ' + url, SESSION_EXPIRED); } else { - return err; + return error; } }); } diff --git a/test/internal/connector.test.js b/test/internal/connector.test.js index 126a486ff..746375075 100644 --- a/test/internal/connector.test.js +++ b/test/internal/connector.test.js @@ -17,8 +17,12 @@ * limitations under the License. */ -import * as DummyChannel from "../../src/v1/internal/ch-dummy"; -import {connect} from "../../src/v1/internal/connector"; +import * as DummyChannel from '../../src/v1/internal/ch-dummy'; +import {connect, Connection} from '../../src/v1/internal/connector'; +import {Packer} from '../../src/v1/internal/packstream'; +import {Chunker} from '../../src/v1/internal/chunking'; +import {alloc} from '../../src/v1/internal/buf'; +import {Neo4jError} from '../../src/v1/error'; describe('connector', () => { @@ -94,4 +98,48 @@ describe('connector', () => { }); + it('should convert failure messages to errors', done => { + const channel = new DummyChannel.channel; + const connection = new Connection(channel, 'bolt://localhost'); + + const errorCode = 'Neo.ClientError.Schema.ConstraintValidationFailed'; + const errorMessage = 'Node 0 already exists with label User and property "email"=[john@doe.com]'; + + connection._queueObserver({ + onError: error => { + expectNeo4jError(error, errorCode, errorMessage); + done(); + } + }); + + channel.onmessage(packedHandshakeMessage()); + channel.onmessage(packedFailureMessage(errorCode, errorMessage)); + }); + + function packedHandshakeMessage() { + const result = alloc(4); + result.putInt32(0, 1); + result.reset(); + return result; + } + + function packedFailureMessage(code, message) { + const channel = new DummyChannel.channel; + const chunker = new Chunker(channel); + const packer = new Packer(chunker); + packer.packStruct(0x7F, [packer.packable({code: code, message: message})]); + chunker.messageBoundary(); + chunker.flush(); + const data = channel.toBuffer(); + const result = alloc(data.length); + result.putBytes(0, data); + return result; + } + + function expectNeo4jError(error, expectedCode, expectedMessage) { + expect(() => { + throw error; + }).toThrow(new Neo4jError(expectedMessage, expectedCode)); + } + }); diff --git a/test/v1/driver.test.js b/test/v1/driver.test.js index 928ebd3b8..906750da5 100644 --- a/test/v1/driver.test.js +++ b/test/v1/driver.test.js @@ -79,7 +79,7 @@ describe('driver', function() { // Expect driver.onError = function (err) { //the error message is different whether in browser or node - expect(err.fields[0].code).toEqual('Neo.ClientError.Security.Unauthorized'); + expect(err.code).toEqual('Neo.ClientError.Security.Unauthorized'); done(); }; diff --git a/test/v1/examples.test.js b/test/v1/examples.test.js index 99148c3a4..c41db24e8 100644 --- a/test/v1/examples.test.js +++ b/test/v1/examples.test.js @@ -306,7 +306,7 @@ describe('examples', function() { // end::handle-cypher-error[] testResultPromise.then(function(loggedError){ - expect(loggedError.fields[0].code).toBe( "Neo.ClientError.Statement.SyntaxError" ); + expect(loggedError.code).toBe( 'Neo.ClientError.Statement.SyntaxError' ); done(); }); }); diff --git a/test/v1/session.test.js b/test/v1/session.test.js index 94aae44c0..940c368ac 100644 --- a/test/v1/session.test.js +++ b/test/v1/session.test.js @@ -94,9 +94,9 @@ describe('session', function () { it('should call observers onError on error ', function (done) { // When & Then - session.run("RETURN 1 AS").subscribe({ + session.run('RETURN 1 AS').subscribe({ onError: function (error) { - expect(error.fields.length).toBe(1); + expect(error.code).toEqual('Neo.ClientError.Statement.SyntaxError'); done(); } }); @@ -139,9 +139,9 @@ describe('session', function () { it('should expose basic run/catch ', function (done) { // When & Then - session.run("RETURN 1 AS").catch( + session.run('RETURN 1 AS').catch( function (error) { - expect(error.fields.length).toBe(1); + expect(error.code).toEqual('Neo.ClientError.Statement.SyntaxError'); done(); } ) diff --git a/test/v1/tck/steps/authsteps.js b/test/v1/tck/steps/authsteps.js index 2796a523a..23149cb71 100644 --- a/test/v1/tck/steps/authsteps.js +++ b/test/v1/tck/steps/authsteps.js @@ -64,8 +64,8 @@ module.exports = function () { }); this.Then(/^a `Protocol Error` is raised$/, function () { - var message = this.err.fields[0].message; - var code = this.err.fields[0].code; + var message = this.err.message; + var code = this.err.code; var expectedStartOfMessage = 'The client is unauthorized due to authentication failure.'; var expectedCode = 'Neo.ClientError.Security.Unauthorized'; diff --git a/test/v1/tck/steps/erroreportingsteps.js b/test/v1/tck/steps/erroreportingsteps.js index 988f221ee..2c75dfe9d 100644 --- a/test/v1/tck/steps/erroreportingsteps.js +++ b/test/v1/tck/steps/erroreportingsteps.js @@ -58,7 +58,7 @@ module.exports = function () { this.When(/^I run a non valid cypher statement$/, function (callback) { var self = this; - this.session.run("CRETE (:n)").then(function(result) {callback()}).catch(function(err) {self.error = err.fields[0]; callback()}) + this.session.run("CRETE (:n)").then(function(result) {callback()}).catch(function(err) {self.error = err; callback()}) }); this.When(/^I set up a driver to an incorrect port$/, function (callback) { diff --git a/test/v1/transaction.test.js b/test/v1/transaction.test.js index ccf5af390..c40f94dfc 100644 --- a/test/v1/transaction.test.js +++ b/test/v1/transaction.test.js @@ -77,9 +77,9 @@ describe('transaction', () => { it('should handle failures with subscribe', done => { const tx = session.beginTransaction(); - tx.run("THIS IS NOT CYPHER") + tx.run('THIS IS NOT CYPHER') .catch(error => { - expect(error.fields.length).toBe(1); + expect(error.code).toEqual('Neo.ClientError.Statement.SyntaxError'); driver.close(); done(); }); @@ -87,10 +87,10 @@ describe('transaction', () => { it('should handle failures with catch', done => { const tx = session.beginTransaction(); - tx.run("THIS IS NOT CYPHER") + tx.run('THIS IS NOT CYPHER') .subscribe({ onError: error => { - expect(error.fields.length).toBe(1); + expect(error.code).toEqual('Neo.ClientError.Statement.SyntaxError'); driver.close(); done(); } @@ -297,7 +297,7 @@ describe('transaction', () => { const invalidBookmark = 'hi, this is an invalid bookmark'; const tx = session.beginTransaction(invalidBookmark); tx.run('RETURN 1').catch(error => { - expect(error.fields[0].code).toBe('Neo.ClientError.Transaction.InvalidBookmark'); + expect(error.code).toBe('Neo.ClientError.Transaction.InvalidBookmark'); done(); }); }); @@ -317,7 +317,7 @@ describe('transaction', () => { const unreachableBookmark = session.lastBookmark() + "0"; const tx2 = session.beginTransaction(unreachableBookmark); tx2.run('CREATE ()').catch(error => { - const message = error.fields[0].message; + const message = error.message; const expectedPrefix = message.indexOf('Database not up to the requested version') === 0; expect(expectedPrefix).toBeTruthy(); done(); @@ -445,8 +445,7 @@ describe('transaction', () => { }); function expectSyntaxError(error) { - const code = error.fields[0].code; - expect(code).toBe('Neo.ClientError.Statement.SyntaxError'); + expect(error.code).toBe('Neo.ClientError.Statement.SyntaxError'); } function expectValidLastBookmark(session) {