From 40286fcd28cc0119e8530d56ff0d82bc55f4517d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miroslav=20Bajto=C5=A1?= Date: Mon, 1 Jul 2019 16:54:50 +0200 Subject: [PATCH] fix: report errors from automigrate/autoupdate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Defer automigrate/autoupdate until we are connected, so that connection errors can be reported back to callers. Fix postInit handler to not report connection error to console.log and via dataSource "error" event in case there is already an operation queued. When this happens, we want the error to be handled by the queued operation and reported to its caller. Signed-off-by: Miroslav Bajtoš --- lib/datasource.js | 48 +++++++++++++++++++++----- test/datasource.test.js | 76 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 115 insertions(+), 9 deletions(-) diff --git a/lib/datasource.js b/lib/datasource.js index 4e92d96c..29249b9d 100644 --- a/lib/datasource.js +++ b/lib/datasource.js @@ -136,6 +136,7 @@ function DataSource(name, settings, modelBuilder) { this.models = this.modelBuilder.models; this.definitions = this.modelBuilder.definitions; this.juggler = juggler; + this._queuedInvocations = 0; // operation metadata // Initialize it before calling setup as the connector might register operations @@ -477,18 +478,22 @@ DataSource.prototype.setup = function(dsName, settings) { if (this.connected) { debug('DataSource %s is now connected to %s', this.name, this.connector.name); this.emit('connected'); - } else { + } else if (err) { // The connection fails, let's report it and hope it will be recovered in the next call - if (err) { - // Reset the connecting to `false` - this.connecting = false; + // Reset the connecting to `false` + this.connecting = false; + if (this._queuedInvocations) { + // Another operation is already waiting for connect() result, + // let them handle the connection error. + debug('Connection fails: %s\nIt will be retried for the next request.', err); + } else { g.error('Connection fails: %s\nIt will be retried for the next request.', err); this.emit('error', err); - } else { - // Either lazyConnect or connector initialize() defers the connection - debug('DataSource %s will be connected to connector %s', this.name, - this.connector.name); } + } else { + // Either lazyConnect or connector initialize() defers the connection + debug('DataSource %s will be connected to connector %s', this.name, + this.connector.name); } }.bind(this); @@ -1053,6 +1058,14 @@ DataSource.prototype.automigrate = function(models, cb) { } } + const args = [models, cb]; + args.callee = this.automigrate; + const queued = this.ready(this, args); + if (queued) { + // waiting to connect + return cb.promise; + } + this.connector.automigrate(models, cb); return cb.promise; }; @@ -1114,6 +1127,14 @@ DataSource.prototype.autoupdate = function(models, cb) { } } + const args = [models, cb]; + args.callee = this.automigrate; + const queued = this.ready(this, args); + if (queued) { + // waiting to connect + return cb.promise; + } + this.connector.autoupdate(models, cb); return cb.promise; }; @@ -2514,12 +2535,15 @@ function(obj, args) { return false; } + this._queuedInvocations++; + const method = args.callee; // Set up a callback after the connection is established to continue the method call let onConnected = null, onError = null, timeoutHandle = null; onConnected = function() { debug('Datasource %s is now connected - executing method %s', self.name, method.name); + this._queuedInvocations--; // Remove the error handler self.removeListener('error', onError); if (timeoutHandle) { @@ -2542,6 +2566,7 @@ function(obj, args) { }; onError = function(err) { debug('Datasource %s fails to connect - aborting method %s', self.name, method.name); + this._queuedInvocations--; // Remove the connected listener self.removeListener('connected', onConnected); if (timeoutHandle) { @@ -2563,6 +2588,7 @@ function(obj, args) { timeoutHandle = setTimeout(function() { debug('Datasource %s fails to connect due to timeout - aborting method %s', self.name, method.name); + this._queuedInvocations--; self.connecting = false; self.removeListener('error', onError); self.removeListener('connected', onConnected); @@ -2575,7 +2601,11 @@ function(obj, args) { if (!this.connecting) { debug('Connecting datasource %s to connector %s', this.name, this.connector.name); - this.connect(); + // When no callback is provided to `connect()`, it returns a Promise. + // We are not handling that promise and thus UnhandledPromiseRejection + // warning is triggered when the connection could not be established. + // We are avoiding this problem by providing a no-op callback. + this.connect(() => {}); } return true; }; diff --git a/test/datasource.test.js b/test/datasource.test.js index 7ab5adb7..adb63282 100644 --- a/test/datasource.test.js +++ b/test/datasource.test.js @@ -457,4 +457,80 @@ describe('DataSource', function() { }); }); }); + + describe('automigrate', () => { + it('reports connection errors (immediate connect)', async () => { + const dataSource = new DataSource({ + connector: givenConnectorFailingOnConnect(), + }); + dataSource.define('MyModel'); + await dataSource.automigrate().should.be.rejectedWith(/test failure/); + }); + + it('reports connection errors (lazy connect)', () => { + const dataSource = new DataSource({ + connector: givenConnectorFailingOnConnect(), + lazyConnect: true, + }); + dataSource.define('MyModel'); + return dataSource.automigrate().should.be.rejectedWith(/test failure/); + }); + + function givenConnectorFailingOnConnect() { + return givenMockConnector({ + connect: function(cb) { + process.nextTick(() => cb(new Error('test failure'))); + }, + automigrate: function(models, cb) { + cb(new Error('automigrate should not have been called')); + }, + }); + } + }); + + describe('autoupdate', () => { + it('reports connection errors (immediate connect)', async () => { + const dataSource = new DataSource({ + connector: givenConnectorFailingOnConnect(), + }); + dataSource.define('MyModel'); + await dataSource.autoupdate().should.be.rejectedWith(/test failure/); + }); + + it('reports connection errors (lazy connect)', () => { + const dataSource = new DataSource({ + connector: givenConnectorFailingOnConnect(), + lazyConnect: true, + }); + dataSource.define('MyModel'); + return dataSource.autoupdate().should.be.rejectedWith(/test failure/); + }); + + function givenConnectorFailingOnConnect() { + return givenMockConnector({ + connect: function(cb) { + process.nextTick(() => cb(new Error('test failure'))); + }, + autoupdate: function(models, cb) { + cb(new Error('autoupdate should not have been called')); + }, + }); + } + }); }); + +function givenMockConnector(props) { + const connector = { + name: 'loopback-connector-mock', + initialize: function(ds, cb) { + ds.connector = connector; + if (ds.settings.lazyConnect) { + cb(null, false); + } else { + connector.connect(cb); + } + }, + ...props, + }; + return connector; +}