From 016dd58147afee5489f62efcf19ca05b3d3dc4cb 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 4e92d96c4..e913b86b7 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 7ab5adb77..4f6dcafc9 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('automigrate 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; +}