Add flag var lazyConnect to ds config by jannyHou · Pull Request #185 · strongloop/loopback-boot

jannyHou · 2016-04-14T20:40:12Z

Connect to strongloop/loopback#2242
Add flag var lazyConnect to ds config

raymondfeng · 2016-04-14T21:12:01Z

lib/executor.js

      useEnvVars: true,
    };
    obj = getUpdatedConfigObject(app, obj, opts);
+    obj.lazyConnect = true;


Now we defer connect to a later phase. When will connect be triggered?

I tried with local db, I think the connect will be triggered when data query happens, like hitting endpoint from Explorer. And also be triggered when datasource.ping() is called.

Hmm...actually I am confused when it should be triggered?

@raymondfeng I think my previous answer is for "when connect will be called" but not "when connect will be triggered in initialize".
So, for "when connect will be triggered in initialize", for example if user create a datasource from boot script, it will be triggered, since lazyConnect is undefined.

var myDS = new DataSource(dsName, dsConfig); // will trigger connect in initialize

I think boot should trigger connect by default. We can set an option to disable it so that swagger generation doesn't depend on live database connections.

bajtos · 2016-04-20T14:49:09Z

lib/executor.js

      useEnvVars: true,
    };
    obj = getUpdatedConfigObject(app, obj, opts);
+    obj.lazyConnect = process.env.LB_LAZYCONNECT_DATASOURCES;


When the env var is not set, this ads lazyConnect variable with undefined value. I think this may surprise users when debugging. Also if lazyConnect was set in datasources.json, then you will re-set it back to whatever is provided by the env variable.

I am proposing a safer approach: 1) change the value only when the ENV var is set and also 2) handle "false" and "0" values as false

var lazyConnect = process.env.LB_LAZYCONNECT_DATASOURCES; if (lazyConnect) { obj.lazyConnect = lazyConnect === 'false' || lazyConnect === '0' ? false : true; }

makes sense. I should take care of undefined. Thanks!

bajtos · 2016-04-20T14:49:35Z

@jannyHou please add unit-tests to verify the implementation works as intended. I'd like to review & approve this pull request myself.

bajtos · 2016-05-02T14:28:21Z

test/executor.test.js

  });

+  describe('when booting with lazy connect', function() {
+    it.only('should report error', function(done) {


Forgotten it.only, please fix to it.

bajtos · 2016-05-02T14:31:59Z

IMO, depending on loopback-connector-mongodb will make the tests slow and brittle. I am proposing to create a custom connector used by tests only, this connector should be very simple - throw when lazyConnect is not set, do no-op otherwise.

bajtos · 2016-05-02T14:37:21Z

You can register a custom connector by calling app.connector(name, connectorObject). A mock-up implementation which may not work OOTB:

// this can be configured in a beforeEach hook
app.connector('throwOnConnect', {
  initialize: function(dataSource, callback) {
    if (dataSource.settings.lazyConnect) callback();
    else callback(new Error('CANNOT CONNECT'));
  }
});

// and then use the connector in tests:
app.boot(someInstructions({ dataSources: {
  'db': { connector: 'throwOnConnect', lazyConnect: false }
}});

jannyHou · 2016-05-03T01:05:06Z

@bajtos I create a custom connector for test, and simply throw new error in initialize to avoid executing callback function postInit.
If we trigger postInit, it will always check connector instance valid before check connection, giving an error:

Connector is not defined correctly: it should create connector member of dataSource'

The error emitted in postInit is already checked in each connector's test case, so I think here we only need to make sure that the flag ENV is passed to connector properly.

bajtos · 2016-05-03T14:52:11Z

test/executor.test.js

+    var DATASOURCE_THROWING_CONNECTION_ERROR = {
+      lazyConnector: {
+        host: 'invalid-host',
+        port: 80,


I believe host and port are not needed by tests, please remove.

bajtos · 2016-05-03T14:58:20Z

I agree it should be enough to test that LB_LAZYCONNECT_DATASOURCES is converted into lazyConnect setting. I think throwing an exception is an overkill for that, you can set a bool variable instead.

    var lazyConnect;
    beforeEach(function() {
      app.connector('reportLazyConnect', {
        initialize: function(dataSource, callback) {
          lazyConnect = dataSource.settings.lazyConnect; 
        },
      });
    });

    var REPORT_LAZY_CONNECT_INSTRUCTIONS = someInstructions({ ... });

  it('does not set lazyConnect by default', function() {
    delete process.env.LB_LAZYCONNECT_DATASOURCES;
    boot.execute(app, REPORT_LAZY_CONNECT_INSTRUCTIONS);
    expect(lazyConnect).to.equal(undefined);
  });

  it('sets lazyConnect:true when LB_LAZYCONNECT_DATASOURCES=true', function() {
    process.env.LB_LAZYCONNECT_DATASOURCES = 'true';
    boot.execute(app, REPORT_LAZY_CONNECT_INSTRUCTIONS);
    expect(lazyConnect).to.equal(undefined);
  });

  // etc, check for cases when lazyConnect is set to "false"
});

jannyHou · 2016-05-04T03:30:58Z

@bajtos I applied the boolean flag to test cases and copied your question from slack:

so what would happen if we callback and let postInit run?

It will execute _setupConnector() which makes things complex. I am afraid we need to create a connector object to get it pass, otherwise it will throw undefined connector before checking connection. I need to investigate what to be included in the connector object if it's worth.

DataSource.prototype._setupConnector = function() {
  this.connector = this.connector || this.adapter; // The legacy JugglingDB adapter will set up `adapter` property
  this.adapter = this.connector; // Keep the adapter as an alias to connector
  if (this.connector) {
    if (!this.connector.dataSource) {
      // Set up the dataSource if the connector doesn't do so
      this.connector.dataSource = this;
    }
    var dataSource = this;
    this.connector.log = function(query, start) {
      dataSource.log(query, start);
    };

    this.connector.logger = function(query) {
      var t1 = Date.now();
      var log = this.log;
      return function(q) {
        log(q || query, t1);
      };
    };
    // Configure the connector instance to mix in observer functions
    jutil.mixin(this.connector, OberserverMixin);
  }
};

bajtos · 2016-05-04T07:29:12Z

test/executor.test.js

    });
  });

+  describe.only('when booting with lazy connect', function() {


Forgotten only, please remove. BTW if you want to run a subset of tests only, you can use mocha's command-line flag -g:

$ mocha -g "lazy connect"

That way you don't need to change code.

bajtos · 2016-05-04T07:30:48Z

test/executor.test.js

+    it('should trigger connect', function(done) {
+      boot.execute(app, someInstructions({
+        dataSources: DATASOURCE_THROWING_CONNECTION_ERROR,
+      }));


To avoid repetition, please extract someInstructions({ dataSources: DATASOURCE_THROWING_CONNECTION_ERROR }); into a shared constant. Then you can remove DATASOURCE_THROWING_CONNECTION_ERROR entirely (inline it in the new constant).

bajtos · 2016-05-04T07:41:22Z

It will execute _setupConnector() which makes things complex. I am afraid we need to create a connector object to get it pass, otherwise it will throw undefined connector before checking connection. I need to investigate what to be included in the connector object if it's worth.

Let's try the following and if you cannot get it working in under 30 minutes then leave the code as it is now and move on.

// somewhere on the top of the file, assuming var loopback = require('loopback')
var Connector = loopback.Connector;

// in your initialize function
initialize: function(dataSource, callback) {
  dataSource.connector = new Connector(dataSource.options);

  if (dataSource.settings.lazyConnect) {
    connectTriggered = false;
  }
}

jannyHou · 2016-05-05T02:48:33Z

@bajtos i add the other two test cases. And i think let's just don't trigger postInit this time:( Thanks for understanding. I will look into it if i have plenty time this sprint or in the near future. Probably still need to touch it when i learn oracle with raymond's pr.

bajtos · 2016-05-05T08:21:13Z

test/executor.test.js

  });

+  describe('when booting with lazy connect', function() {
+    var sampleInstruction = someInstructions({


Please rename to indicate that this should be treated as an immutable constant, e.g. SAMPLE_INSTRUCTIONS.

bajtos · 2016-05-05T08:24:28Z

Few more details to improve, the patch LGTM otherwise. No more reviews are needed.

jannyHou added the #review label Apr 14, 2016

raymondfeng reviewed Apr 14, 2016
View reviewed changes

jannyHou mentioned this pull request Apr 14, 2016

Spike: Lazy connect datasources on app start strongloop/loopback#2176

Closed

jannyHou self-assigned this Apr 15, 2016

jannyHou force-pushed the feature/lazy-connect branch 2 times, most recently from 7e5befb to 1793276 Compare April 19, 2016 20:44

jannyHou assigned raymondfeng and unassigned jannyHou Apr 19, 2016

bajtos reviewed Apr 20, 2016
View reviewed changes

bajtos assigned jannyHou and unassigned raymondfeng Apr 20, 2016

jannyHou mentioned this pull request Apr 27, 2016

Lazy connect datasources on app start strongloop/loopback#2242

Closed

bajtos reviewed May 2, 2016
View reviewed changes

jannyHou force-pushed the feature/lazy-connect branch from c6e46fd to fc0f9a9 Compare May 2, 2016 21:58

jannyHou assigned bajtos and unassigned jannyHou May 3, 2016

bajtos reviewed May 3, 2016
View reviewed changes

bajtos assigned jannyHou and unassigned bajtos May 3, 2016

jannyHou assigned bajtos and unassigned jannyHou May 3, 2016

bajtos reviewed May 4, 2016
View reviewed changes

bajtos assigned jannyHou and unassigned bajtos May 4, 2016

jannyHou force-pushed the feature/lazy-connect branch 3 times, most recently from b48a73f to 7404125 Compare May 5, 2016 01:32

jannyHou assigned bajtos and unassigned jannyHou May 5, 2016

bajtos reviewed May 5, 2016
View reviewed changes

bajtos assigned jannyHou and unassigned bajtos May 5, 2016

Add flag var lazyConnect to ds config

d334425

jannyHou force-pushed the feature/lazy-connect branch from 388002a to d334425 Compare May 5, 2016 21:33

jannyHou merged commit defe5f4 into master May 5, 2016

jannyHou removed the #review label May 5, 2016

jannyHou mentioned this pull request Jun 20, 2016

Support lazy connection for data sources loopbackio/loopback-datasource-juggler#463

Closed

Conversation

jannyHou commented Apr 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jannyHou Apr 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jannyHou Apr 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bajtos commented Apr 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bajtos commented May 2, 2016

Uh oh!

bajtos commented May 2, 2016

Uh oh!

jannyHou commented May 3, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bajtos commented May 3, 2016

Uh oh!

jannyHou commented May 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bajtos commented May 4, 2016

Uh oh!

jannyHou commented May 5, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bajtos commented May 5, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jannyHou commented Apr 14, 2016 •

edited

Loading

jannyHou Apr 15, 2016 •

edited

Loading

jannyHou Apr 15, 2016 •

edited

Loading

bajtos commented Apr 20, 2016 •

edited

Loading

jannyHou commented May 4, 2016 •

edited

Loading