Merge pull request #364 from lutovich/1.6-port-80

Fix handling of bolt URL with port 80

Author: ali-ince

package-lock.json

@@ -73,6 +73,6 @@
   },
   "dependencies": {
     "babel-runtime": "^6.18.0",
-    "url-parse": "^1.2.0"
+    "uri-js": "^4.2.1"
   }
 }

package.json

@@ -56,16 +56,16 @@ class Driver {
   /**
    * You should not be calling this directly, instead use {@link driver}.
    * @constructor
-   * @param {string} url
+   * @param {string} hostPort
    * @param {string} userAgent
    * @param {object} token
    * @param {object} config
    * @protected
    */
-  constructor(url, userAgent, token = {}, config = {}) {
+  constructor(hostPort, userAgent, token = {}, config = {}) {
     sanitizeConfig(config);
 
-    this._url = url;
+    this._hostPort = hostPort;
     this._userAgent = userAgent;
     this._openSessions = {};
     this._sessionIdGenerator = 0;
@@ -116,13 +116,13 @@ class Driver {
    * @return {Connection} new connector-api session instance, a low level session API.
    * @access private
    */
-  _createConnection(url, release) {
+  _createConnection(hostPort, release) {
     let sessionId = this._sessionIdGenerator++;
-    let conn = connect(url, this._config, this._connectionErrorCode());
+    let conn = connect(hostPort, this._config, this._connectionErrorCode());
     let streamObserver = new _ConnectionStreamObserver(this, conn);
     conn.initialize(this._userAgent, this._token, streamObserver);
     conn._id = sessionId;
-    conn._release = () => release(url, conn);
+    conn._release = () => release(hostPort, conn);
 
     this._openSessions[sessionId] = conn;
     return conn;
@@ -185,8 +185,8 @@ class Driver {
   }
 
   // Extension point
-  _createConnectionProvider(address, connectionPool, driverOnErrorCallback) {
-    return new DirectConnectionProvider(address, connectionPool, driverOnErrorCallback);
+  _createConnectionProvider(hostPort, connectionPool, driverOnErrorCallback) {
+    return new DirectConnectionProvider(hostPort, connectionPool, driverOnErrorCallback);
   }
 
   // Extension point
@@ -203,7 +203,7 @@ class Driver {
   _getOrCreateConnectionProvider() {
     if (!this._connectionProvider) {
       const driverOnErrorCallback = this._driverOnErrorCallback.bind(this);
-      this._connectionProvider = this._createConnectionProvider(this._url, this._pool, driverOnErrorCallback);
+      this._connectionProvider = this._createConnectionProvider(this._hostPort, this._pool, driverOnErrorCallback);
     }
     return this._connectionProvider;
   }

src/v1/driver.js

@@ -44,24 +44,24 @@ class ConnectionProvider {
 
 export class DirectConnectionProvider extends ConnectionProvider {
 
-  constructor(address, connectionPool, driverOnErrorCallback) {
+  constructor(hostPort, connectionPool, driverOnErrorCallback) {
     super();
-    this._address = address;
+    this._hostPort = hostPort;
     this._connectionPool = connectionPool;
     this._driverOnErrorCallback = driverOnErrorCallback;
   }
 
   acquireConnection(mode) {
-    const connectionPromise = this._connectionPool.acquire(this._address);
+    const connectionPromise = this._connectionPool.acquire(this._hostPort);
     return this._withAdditionalOnErrorCallback(connectionPromise, this._driverOnErrorCallback);
   }
 }
 
 export class LoadBalancer extends ConnectionProvider {
 
-  constructor(address, routingContext, connectionPool, loadBalancingStrategy, driverOnErrorCallback) {
+  constructor(hostPort, routingContext, connectionPool, loadBalancingStrategy, driverOnErrorCallback) {
     super();
-    this._seedRouter = address;
+    this._seedRouter = hostPort;
     this._routingTable = new RoutingTable([this._seedRouter]);
     this._rediscovery = new Rediscovery(new RoutingUtil(routingContext));
     this._connectionPool = connectionPool;

src/v1/internal/connection-providers.js

@@ -96,17 +96,17 @@ 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 {string} hostPort - the hostname and port to connect to.
    * @param {boolean} disableLosslessIntegers if this connection should convert all received integers to native JS numbers.
    */
-  constructor(channel, url, disableLosslessIntegers = false) {
+  constructor(channel, hostPort, disableLosslessIntegers = false) {
     /**
      * An ordered queue of observers, each exchange response (zero or more
      * RECORD messages followed by a SUCCESS message) we receive will be routed
      * to the next pending observer.
      */
-    this.url = url;
-    this.server = {address: url};
+    this.hostPort = hostPort;
+    this.server = {address: hostPort};
     this.creationTimestamp = Date.now();
     this._disableLosslessIntegers = disableLosslessIntegers;
     this._pendingObservers = [];
@@ -516,18 +516,18 @@ class ConnectionState {
 }
 
 /**
- * Crete new connection to the provided url.
+ * Crete new connection to the provided address.
  * @access private
- * @param {string} url - 'neo4j'-prefixed URL to Neo4j Bolt endpoint
+ * @param {string} hostPort - the Bolt endpoint to connect to
  * @param {object} config - this driver configuration
  * @param {string=null} connectionErrorCode - error code for errors raised on connection errors
  * @return {Connection} - New connection
  */
-function connect(url, config = {}, connectionErrorCode = null) {
+function connect(hostPort, config = {}, connectionErrorCode = null) {
   const Ch = config.channel || Channel;
-  const parsedUrl = urlUtil.parseDatabaseUrl(url);
-  const channelConfig = new ChannelConfig(parsedUrl, config, connectionErrorCode);
-  return new Connection(new Ch(channelConfig), parsedUrl.hostAndPort, config.disableLosslessIntegers);
+  const parsedAddress = urlUtil.parseDatabaseUrl(hostPort);
+  const channelConfig = new ChannelConfig(parsedAddress, config, connectionErrorCode);
+  return new Connection(new Ch(channelConfig), parsedAddress.hostAndPort, config.disableLosslessIntegers);
 }
 
 export {

src/v1/internal/connector.js

@@ -22,13 +22,13 @@ import HttpSessionTracker from './http-session-tracker';
 
 export default class HttpDriver extends Driver {
 
-  constructor(url, userAgent, token, config) {
-    super(url, userAgent, token, config);
+  constructor(hostPort, userAgent, token, config) {
+    super(hostPort, userAgent, token, config);
     this._sessionTracker = new HttpSessionTracker();
   }
 
   session() {
-    return new HttpSession(this._url, this._token, this._config, this._sessionTracker);
+    return new HttpSession(this._hostPort, this._token, this._config, this._sessionTracker);
   }
 
   close() {

src/v1/internal/http/http-driver.js

@@ -16,7 +16,7 @@
  * limitations under the License.
  */
 
-import ParsedUrl from 'url-parse';
+import {parse as uriJsParse} from 'uri-js';
 import {assertString} from './util';
 
 const DEFAULT_BOLT_PORT = 7687;
@@ -68,14 +68,14 @@ function parseDatabaseUrl(url) {
   assertString(url, 'URL');
 
   const sanitized = sanitizeUrl(url);
-  const parsedUrl = new ParsedUrl(sanitized.url, {}, query => extractQuery(query, url));
+  const parsedUrl = uriJsParse(sanitized.url);
 
-  const scheme = sanitized.schemeMissing ? null : extractScheme(parsedUrl.protocol);
-  const rawHost = extractHost(parsedUrl.hostname); // has square brackets for IPv6
-  const host = unescapeIPv6Address(rawHost); // no square brackets for IPv6
+  const scheme = sanitized.schemeMissing ? null : extractScheme(parsedUrl.scheme);
+  const host = extractHost(parsedUrl.host); // no square brackets for IPv6
+  const formattedHost = formatHost(host); // has square brackets for IPv6
   const port = extractPort(parsedUrl.port, scheme);
-  const hostAndPort = `${rawHost}:${port}`;
-  const query = parsedUrl.query;
+  const hostAndPort = `${formattedHost}:${port}`;
+  const query = extractQuery(parsedUrl.query, url);
 
   return new Url(scheme, host, port, hostAndPort, query);
 }
@@ -84,8 +84,8 @@ function sanitizeUrl(url) {
   url = url.trim();
 
   if (url.indexOf('://') === -1) {
-    // url does not contain scheme, add dummy 'http://' to make parser work correctly
-    return {schemeMissing: true, url: `http://${url}`};
+    // url does not contain scheme, add dummy 'none://' to make parser work correctly
+    return {schemeMissing: true, url: `none://${url}`};
   }
 
   return {schemeMissing: false, url: url};
@@ -168,17 +168,12 @@ function escapeIPv6Address(address) {
   }
 }
 
-function unescapeIPv6Address(address) {
-  const startsWithSquareBracket = address.charAt(0) === '[';
-  const endsWithSquareBracket = address.charAt(address.length - 1) === ']';
-
-  if (!startsWithSquareBracket && !endsWithSquareBracket) {
-    return address;
-  } else if (startsWithSquareBracket && endsWithSquareBracket) {
-    return address.substring(1, address.length - 1);
-  } else {
-    throw new Error(`Illegal IPv6 address ${address}`);
+function formatHost(host) {
+  if (!host) {
+    throw new Error(`Illegal host ${host}`);
   }
+  const isIPv6Address = host.indexOf(':') >= 0;
+  return isIPv6Address ? escapeIPv6Address(host) : host;
 }
 
 function formatIPv4Address(address, port) {

src/v1/internal/url-util.js

@@ -24,19 +24,19 @@ import LeastConnectedLoadBalancingStrategy, {LEAST_CONNECTED_STRATEGY_NAME} from
 import RoundRobinLoadBalancingStrategy, {ROUND_ROBIN_STRATEGY_NAME} from './internal/round-robin-load-balancing-strategy';
 
 /**
- * A driver that supports routing in a core-edge cluster.
+ * A driver that supports routing in a causal cluster.
  * @private
  */
 class RoutingDriver extends Driver {
 
-  constructor(url, routingContext, userAgent, token = {}, config = {}) {
-    super(url, userAgent, token, validateConfig(config));
+  constructor(hostPort, routingContext, userAgent, token = {}, config = {}) {
+    super(hostPort, userAgent, token, validateConfig(config));
     this._routingContext = routingContext;
   }
 
-  _createConnectionProvider(address, connectionPool, driverOnErrorCallback) {
+  _createConnectionProvider(hostPort, connectionPool, driverOnErrorCallback) {
     const loadBalancingStrategy = RoutingDriver._createLoadBalancingStrategy(this._config, connectionPool);
-    return new LoadBalancer(address, this._routingContext, connectionPool, loadBalancingStrategy, driverOnErrorCallback);
+    return new LoadBalancer(hostPort, this._routingContext, connectionPool, loadBalancingStrategy, driverOnErrorCallback);
   }
 
   _createSession(mode, connectionProvider, bookmark, config) {
@@ -46,14 +46,14 @@ class RoutingDriver extends Driver {
         return error;
       }
 
-      const url = conn.url;
+      const hostPort = conn.hostPort;
 
       if (error.code === SESSION_EXPIRED || isDatabaseUnavailable(error)) {
-        this._connectionProvider.forget(url);
+        this._connectionProvider.forget(hostPort);
         return error;
       } else if (isFailureToWrite(error)) {
-        this._connectionProvider.forgetWriter(url);
-        return newError('No longer possible to write to server at ' + url, SESSION_EXPIRED);
+        this._connectionProvider.forgetWriter(hostPort);
+        return newError('No longer possible to write to server at ' + hostPort, SESSION_EXPIRED);
       } else {
         return error;
       }

src/v1/routing-driver.js

@@ -722,6 +722,31 @@ describe('url-util', () => {
     expect(parse('https://localhost').port).toEqual(urlUtil.defaultPortForScheme('https'));
   });
 
+  it('should parse URLs with port 80', () => {
+    ['http', 'https', 'ws', 'wss', 'bolt', 'bolt+routing'].forEach(scheme => {
+      verifyUrl(`${scheme}://localhost:80`, {
+        scheme: scheme,
+        host: 'localhost',
+        port: 80
+      });
+    });
+
+    ['localhost', '127.0.0.1', '192.168.10.29'].forEach(host => {
+      verifyUrl(`${host}:80`, {
+        host: host,
+        port: 80
+      });
+    });
+
+    ['::1', '1afc:0:a33:85a3::ff2f'].forEach(host => {
+      verifyUrl(`[${host}]:80`, {
+        host: host,
+        port: 80,
+        ipv6: true
+      });
+    });
+  });
+
   function verifyUrl(urlString, expectedUrl) {
     const url = parse(urlString);
 

test/internal/url-util.test.js

@@ -22,6 +22,7 @@ import FakeConnection from '../internal/fake-connection';
 import lolex from 'lolex';
 import {DEFAULT_ACQUISITION_TIMEOUT, DEFAULT_MAX_SIZE} from '../../src/v1/internal/pool-config';
 import {ServerVersion, VERSION_3_1_0} from '../../src/v1/internal/server-version';
+import testUtils from '../internal/test-utils';
 
 describe('driver', () => {
 
@@ -77,6 +78,28 @@ describe('driver', () => {
     startNewTransaction(driver);
   }, 10000);
 
+  it('should fail with correct error message when connecting to port 80', done => {
+    if (testUtils.isClient()) {
+      // good error message is not available in browser
+      done();
+      return;
+    }
+
+    driver = neo4j.driver('bolt://localhost:80', sharedNeo4j.authToken);
+
+    driver.session().run('RETURN 1').then(result => {
+      done.fail('Should not be able to connect. Result: ' + JSON.stringify(result));
+    }).catch(error => {
+      const doesNotContainAddress = error.message.indexOf(':80') < 0;
+      if (doesNotContainAddress) {
+        done.fail(`Expected to contain ':80' but was: ${error.message}`);
+      } else {
+        expect(error.code).toEqual(neo4j.error.SERVICE_UNAVAILABLE);
+        done();
+      }
+    });
+  });
+
   it('should handle wrong scheme', () => {
     expect(() => neo4j.driver("tank://localhost", sharedNeo4j.authToken))
       .toThrow(new Error('Unknown scheme: tank'));