Skip to content

Fix Driver.onCompleted callback #298

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

Merged
merged 1 commit into from
Oct 4, 2017
Merged
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
13 changes: 1 addition & 12 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -61,23 +61,12 @@ driver.close();

## Usage examples

Driver creation:
Driver lifecycle:
```javascript
// Create a driver instance, for the user neo4j with password neo4j.
// It should be enough to have a single driver per database per application.
var driver = neo4j.driver("bolt://localhost", neo4j.auth.basic("neo4j", "neo4j"));

// Register a callback to know if driver creation was successful:
driver.onCompleted = function () {
// proceed with using the driver, it was successfully instantiated
};

// Register a callback to know if driver creation failed.
// This could happen due to wrong credentials or database unavailability:
driver.onError = function (error) {
console.log('Driver instantiation failed', error);
};

// Close the driver when application exits.
// This closes all used network connections.
driver.close();
Expand Down
26 changes: 16 additions & 10 deletions src/v1/driver.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import StreamObserver from './internal/stream-observer';
import {newError, SERVICE_UNAVAILABLE} from './error';
import {DirectConnectionProvider} from './internal/connection-providers';
import Bookmark from './internal/bookmark';
import ConnectivityVerifier from './internal/connectivity-verifier';

const READ = 'READ', WRITE = 'WRITE';
/**
Expand Down Expand Up @@ -72,6 +73,21 @@ class Driver {
* @protected
*/
this._connectionProvider = null;

this._onCompleted = null;
}

get onCompleted() {
return this._onCompleted;
}

set onCompleted(callback) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Setting an event handler invoking actual verification confused me a bit. What happens if verification fails? Maybe an explicit method verifyConnectivity() returning a promise would be a better option, can be elaborated for upcoming versions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Failed verification will result in Driver.onCompleted not being invoked. Instead Driver.onError will be invoked. I agree, these callbacks are not ideal and rather ugly. That's why they are removed from README. Users can verify connectivity by running a dummy query like RETURN 1. So this fix is only for backwards compatibility.

this._onCompleted = callback;
if (this._onCompleted) {
const connectionProvider = this._getOrCreateConnectionProvider();
const connectivityVerifier = new ConnectivityVerifier(connectionProvider, this._onCompleted);
connectivityVerifier.verify();
}
}

/**
Expand Down Expand Up @@ -219,16 +235,6 @@ class _ConnectionStreamObserver extends StreamObserver {
this._hasFailed = true;
}
}

onCompleted(message) {
if (this._driver.onCompleted) {
this._driver.onCompleted(message);
}

if (this._observer && this._observer.onComplete) {
this._observer.onCompleted(message);
}
}
}

/**
Expand Down
71 changes: 71 additions & 0 deletions src/v1/internal/connectivity-verifier.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
/**
* Copyright (c) 2002-2017 "Neo Technology,","
* Network Engine for Objects in Lund AB [http://neotechnology.com]
*
* This file is part of Neo4j.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

import ConnectionHolder from './connection-holder';
import {READ} from '../driver';
import StreamObserver from './stream-observer';

/**
* Verifies connectivity using the given connection provider.
*/
export default class ConnectivityVerifier {

/**
* @constructor
* @param {ConnectionProvider} connectionProvider the provider to obtain connections from.
* @param {function} successCallback a callback to invoke when verification succeeds.
*/
constructor(connectionProvider, successCallback) {
this._connectionProvider = connectionProvider;
this._successCallback = successCallback;
}

verify() {
acquireAndReleaseDummyConnection(this._connectionProvider).then(serverInfo => {
if (this._successCallback) {
this._successCallback(serverInfo);
}
}).catch(ignoredError => {
});
}
}

/**
* @private
* @param {ConnectionProvider} connectionProvider the provider to obtain connections from.
* @return {Promise<object>} promise resolved with server info or rejected with error.
*/
function acquireAndReleaseDummyConnection(connectionProvider) {
const connectionHolder = new ConnectionHolder(READ, connectionProvider);
connectionHolder.initializeConnection();
const dummyObserver = new StreamObserver();
const connectionPromise = connectionHolder.getConnection(dummyObserver);

return connectionPromise.then(connection => {
// able to establish a connection
return connectionHolder.close().then(() => connection.server);
}).catch(error => {
// failed to establish a connection
return connectionHolder.close().catch(ignoredError => {
// ignore connection release error
}).then(() => {
return Promise.reject(error);
});
});
}
2 changes: 2 additions & 0 deletions src/v1/internal/server-version.js
Original file line number Diff line number Diff line change
Expand Up @@ -91,10 +91,12 @@ function compareInts(x, y) {
return (x < y) ? -1 : ((x === y) ? 0 : 1);
}

const VERSION_3_1_0 = new ServerVersion(3, 1, 0);
const VERSION_3_2_0 = new ServerVersion(3, 2, 0);

export {
ServerVersion,
VERSION_3_1_0,
VERSION_3_2_0
};

Expand Down
37 changes: 37 additions & 0 deletions test/internal/connectivity-verifier.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
/**
* Copyright (c) 2002-2017 "Neo Technology,","
* Network Engine for Objects in Lund AB [http://neotechnology.com]
*
* This file is part of Neo4j.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

import ConnectivityVerifier from '../../src/v1/internal/connectivity-verifier';
import {SingleConnectionProvider} from '../../src/v1/internal/connection-providers';
import FakeConnection from './fake-connection';

describe('ConnectivityVerifier', () => {

it('should call success callback when able to acquire and release a connection', done => {
const connectionPromise = Promise.resolve(new FakeConnection());
const connectionProvider = new SingleConnectionProvider(connectionPromise);

const verifier = new ConnectivityVerifier(connectionProvider, () => {
done();
});

verifier.verify();
});

});
8 changes: 5 additions & 3 deletions test/types/v1/driver.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import Driver, {
import {Parameters} from "../../../types/v1/statement-runner";
import Session from "../../../types/v1/session";
import {Neo4jError} from "../../../types/v1/error";
import {ServerInfo} from "../../../types/v1/result-summary";

const dummy: any = null;

Expand Down Expand Up @@ -87,11 +88,12 @@ session1.run("RETURN 1").then(result => {

const close: void = driver.close();

driver.onCompleted = (metadata: { server: string }) => {
console.log(metadata.server);
driver.onCompleted = (serverInfo: ServerInfo) => {
console.log(serverInfo.version);
console.log(serverInfo.address);
};

driver.onCompleted({server: "Neo4j/3.2.0"});
driver.onCompleted({version: "Neo4j/3.2.0", address: "localhost:7687"});

driver.onError = (error: Neo4jError) => {
console.log(error);
Expand Down
24 changes: 8 additions & 16 deletions test/v1/driver.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -112,51 +112,43 @@ describe('driver', () => {
driver = neo4j.driver("bolt://localhost", sharedNeo4j.authToken);

// Expect
driver.onCompleted = meta => {
driver.onCompleted = server => {
expect(server.address).toBeDefined();
done();
};

// When
startNewTransaction(driver);
});

it('should be possible to pass a realm with basic auth tokens', done => {
// Given
driver = neo4j.driver("bolt://localhost", neo4j.auth.basic(sharedNeo4j.username, sharedNeo4j.password, "native"));

// Expect
driver.onCompleted = meta => {
driver.onCompleted = server => {
expect(server.address).toBeDefined();
done();
};

// When
startNewTransaction(driver);
});

it('should be possible to create custom auth tokens', done => {
// Given
driver = neo4j.driver("bolt://localhost", neo4j.auth.custom(sharedNeo4j.username, sharedNeo4j.password, "native", "basic"));

// Expect
driver.onCompleted = meta => {
driver.onCompleted = server => {
expect(server.address).toBeDefined();
done();
};

// When
startNewTransaction(driver);
});

it('should be possible to create custom auth tokens with additional parameters', done => {
// Given
driver = neo4j.driver("bolt://localhost", neo4j.auth.custom(sharedNeo4j.username, sharedNeo4j.password, "native", "basic", {secret: 42}));

// Expect
driver.onCompleted = () => {
driver.onCompleted = server => {
expect(server.address).toBeDefined();
done();
};

// When
startNewTransaction(driver);
});

it('should fail nicely when connecting with routing to standalone server', done => {
Expand Down
42 changes: 6 additions & 36 deletions test/v1/examples.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -102,14 +102,9 @@ describe('examples', () => {
// end::basic-auth[]

driver.onCompleted = () => {
driver.close();
done();
};

const session = driver.session();
session.run('RETURN 1').then(() => {
session.close();
driver.close();
});
});

it('config max retry time example', done => {
Expand All @@ -123,14 +118,9 @@ describe('examples', () => {
// end::config-max-retry-time[]

driver.onCompleted = () => {
driver.close();
done();
};

const session = driver.session();
session.run('RETURN 1').then(() => {
session.close();
driver.close();
});
});

it('config trust example', done => {
Expand All @@ -144,19 +134,9 @@ describe('examples', () => {
// end::config-trust[]

driver.onCompleted = () => {
driver.close();
done();
};

driver.onError = error => {
console.log(error);
};

const session = driver.session();
session.run('RETURN 1').then(() => {
session.close();
driver.close();
}).catch(error => {
});
});

it('config unencrypted example', done => {
Expand All @@ -169,14 +149,9 @@ describe('examples', () => {
// end::config-unencrypted[]

driver.onCompleted = () => {
driver.close();
done();
};

const session = driver.session();
session.run('RETURN 1').then(() => {
session.close();
driver.close();
});
});

it('custom auth example', done => {
Expand All @@ -191,14 +166,9 @@ describe('examples', () => {
// end::custom-auth[]

driver.onCompleted = () => {
driver.close();
done();
};

const session = driver.session();
session.run('RETURN 1').then(() => {
session.close();
driver.close();
});
});

it('kerberos auth example', () => {
Expand Down Expand Up @@ -239,7 +209,7 @@ describe('examples', () => {
// tag::driver-lifecycle[]
const driver = neo4j.driver(uri, neo4j.auth.basic(user, password));

driver.onCompleted = metadata => {
driver.onCompleted = () => {
console.log('Driver created');
};

Expand Down
Loading