Merge pull request #1989 from strongloop/feature/remove-auto-attach

[SEMVER-MAJOR] Remove "loopback.autoAttach()"

common/models/role.js

@@ -145,12 +145,10 @@ module.exports = function(Role) {
   });
 
   function isUserClass(modelClass) {
-    if (modelClass) {
+    if (!modelClass) return false;
-      return modelClass === loopback.User ||
+    var User = modelClass.modelBuilder.models.User;
-        modelClass.prototype instanceof loopback.User;
+    if (!User) return false;
-    } else {
+    return modelClass == User || modelClass.prototype instanceof User;
-      return false;
-    }
   }
 
   /*!

lib/builtin-models.js

@@ -41,25 +41,6 @@ module.exports = function(registry) {
     require('../common/models/checkpoint.json'),
     require('../common/models/checkpoint.js'));
 
-  /*!
-   * Automatically attach these models to dataSources
-   */
-
-  var dataSourceTypes = {
-    DB: 'db',
-    MAIL: 'mail',
-  };
-
-  registry.Email.autoAttach = dataSourceTypes.MAIL;
-  registry.getModel('PersistedModel').autoAttach = dataSourceTypes.DB;
-  registry.User.autoAttach = dataSourceTypes.DB;
-  registry.AccessToken.autoAttach = dataSourceTypes.DB;
-  registry.Role.autoAttach = dataSourceTypes.DB;
-  registry.RoleMapping.autoAttach = dataSourceTypes.DB;
-  registry.ACL.autoAttach = dataSourceTypes.DB;
-  registry.Scope.autoAttach = dataSourceTypes.DB;
-  registry.Application.autoAttach = dataSourceTypes.DB;
-
   function createModel(definitionJson, customizeFn) {
     var Model = registry.createModel(definitionJson);
     customizeFn(Model);

lib/loopback.js

@@ -363,43 +363,6 @@ loopback.createDataSource = function(name, options) {
 loopback.memory = function(name) {
   return this.registry.memory.apply(this.registry, arguments);
 };
-
-/**
- * Set the default `dataSource` for a given `type`.
- * @param {String} type The datasource type.
- * @param {Object|DataSource} dataSource The data source settings or instance
- * @returns {DataSource} The data source instance.
- *
- * @header loopback.setDefaultDataSourceForType(type, dataSource)
- */
-
-loopback.setDefaultDataSourceForType = function(type, dataSource) {
-  return this.registry.setDefaultDataSourceForType.apply(this.registry, arguments);
-};
-
-/**
- * Get the default `dataSource` for a given `type`.
- * @param {String} type The datasource type.
- * @returns {DataSource} The data source instance
- */
-
-loopback.getDefaultDataSourceForType = function(type) {
-  return this.registry.getDefaultDataSourceForType.apply(this.registry, arguments);
-};
-
-/**
- * Attach any model that does not have a dataSource to
- * the default dataSource for the type the Model requests
- */
-
-loopback.autoAttach = function() {
-  return this.registry.autoAttach.apply(this.registry, arguments);
-};
-
-loopback.autoAttachModel = function(ModelCtor) {
-  return this.registry.autoAttachModel.apply(this.registry, arguments);
-};
-
 /*!
  * Built in models / services
  */

lib/persisted-model.js

@@ -1533,8 +1533,7 @@ module.exports = function(registry) {
       attachRelatedModels(this);
     }
 
-    // We have to attach related model whenever the datasource changes,
+    // Re-attach related models whenever our datasource is changed.
-    // this is a workaround for autoAttach called by loopback.createModel
     var self = this;
     this.on('dataSourceAttached', function() {
       attachRelatedModels(self);

lib/registry.js

@@ -118,11 +118,6 @@ Registry.prototype.createModel = function(name, properties, options) {
 
   this._defineRemoteMethods(model, options.methods);
 
-  // try to attach
-  try {
-    this.autoAttachModel(model);
-  } catch (e) {}
-
   return model;
 };
 
@@ -368,7 +363,8 @@ Registry.prototype.createDataSource = function(name, options) {
   };
 
   if (ds.settings && ds.settings.defaultForType) {
-    this.setDefaultDataSourceForType(ds.settings.defaultForType, ds);
+    var msg = 'DataSource option "defaultForType" is no longer supported';
+    throw new Error(msg);
   }
 
   return ds;
@@ -395,66 +391,3 @@ Registry.prototype.memory = function(name) {
 
   return memory;
 };
-
-/**
- * Set the default `dataSource` for a given `type`.
- * @param {String} type The datasource type.
- * @param {Object|DataSource} dataSource The data source settings or instance
- * @returns {DataSource} The data source instance.
- *
- * @header loopback.setDefaultDataSourceForType(type, dataSource)
- */
-
-Registry.prototype.setDefaultDataSourceForType = function(type, dataSource) {
-  var defaultDataSources = this.defaultDataSources;
-
-  if (!(dataSource instanceof DataSource)) {
-    dataSource = this.createDataSource(dataSource);
-  }
-
-  defaultDataSources[type] = dataSource;
-  return dataSource;
-};
-
-/**
- * Get the default `dataSource` for a given `type`.
- * @param {String} type The datasource type.
- * @returns {DataSource} The data source instance
- */
-
-Registry.prototype.getDefaultDataSourceForType = function(type) {
-  return this.defaultDataSources && this.defaultDataSources[type];
-};
-
-/**
- * Attach any model that does not have a dataSource to
- * the default dataSource for the type the Model requests
- */
-
-Registry.prototype.autoAttach = function() {
-  var models = this.modelBuilder.models;
-  assert.equal(typeof models, 'object', 'Cannot autoAttach without a models object');
-
-  Object.keys(models).forEach(function(modelName) {
-    var ModelCtor = models[modelName];
-
-    // Only auto attach if the model doesn't have an explicit data source
-    if (ModelCtor && (!(ModelCtor.dataSource instanceof DataSource))) {
-      this.autoAttachModel(ModelCtor);
-    }
-  }, this);
-};
-
-Registry.prototype.autoAttachModel = function(ModelCtor) {
-  if (ModelCtor.autoAttach) {
-    var ds = this.getDefaultDataSourceForType(ModelCtor.autoAttach);
-
-    assert(
-      ds instanceof DataSource,
-      'cannot autoAttach model "' + ModelCtor.modelName +
-      '". No dataSource found of type ' + ModelCtor.autoAttach
-    );
-
-    ModelCtor.attachTo(ds);
-  }
-};

package.json

@@ -102,5 +102,10 @@
     "depd": "loopback-datasource-juggler/lib/browser.depd.js",
     "bcrypt": false
   },
+  "config": {
+    "ci": {
+      "debug": "*,-mocha:*,-eslint:*"
+    }
+  },
   "license": "MIT"
 }

test/email.test.js

@@ -37,7 +37,11 @@ describe('Email connector', function() {
 describe('Email and SMTP', function() {
   beforeEach(function() {
     MyEmail = loopback.Email.extend('my-email');
-    loopback.autoAttach();
+    var ds = loopback.createDataSource('email', {
+      connector: loopback.Mail,
+      transports: [{ type: 'STUB' }],
+    });
+    MyEmail.attachTo(ds);
   });
 
   it('should have a send method', function() {

test/integration.test.js

@@ -24,7 +24,6 @@ describe('loopback application', function() {
     function setupAppWithStreamingMethod() {
       app.dataSource('db', {
         connector: loopback.Memory,
-        defaultForType: 'db',
       });
       var db = app.datasources.db;
 

test/loopback.test.js

@@ -53,8 +53,6 @@ describe('loopback', function() {
         'ValidationError',
         'application',
         'arguments',
-        'autoAttach',
-        'autoAttachModel',
         'bodyParser',
         'caller',
         'compress',
@@ -73,7 +71,6 @@ describe('loopback', function() {
         'faviconFile',
         'findModel',
         'getCurrentContext',
-        'getDefaultDataSourceForType',
         'getModel',
         'getModelByType',
         'isBrowser',
@@ -96,7 +93,6 @@ describe('loopback', function() {
         'rest',
         'runInContext',
         'session',
-        'setDefaultDataSourceForType',
         'static',
         'status',
         'template',
@@ -158,32 +154,6 @@ describe('loopback', function() {
     });
   });
 
-  describe('loopback.autoAttach', function() {
-    it('doesn\'t overwrite model with datasource configured', function() {
-      var ds1 = loopback.createDataSource('db1', {
-        connector: loopback.Memory,
-      });
-
-      // setup default data sources
-      loopback.setDefaultDataSourceForType('db', ds1);
-
-      var ds2 = loopback.createDataSource('db2', {
-        connector: loopback.Memory,
-      });
-
-      var model1 = ds2.createModel('m1', {});
-
-      var model2 = loopback.createModel('m2');
-      model2.autoAttach = 'db';
-
-      // auto attach data sources to models
-      loopback.autoAttach();
-
-      assert(model1.dataSource === ds2);
-      assert(model2.dataSource === ds1);
-    });
-  });
-
   describe('loopback.remoteMethod(Model, fn, [options]);', function() {
     it('Setup a remote method.', function() {
       var Product = loopback.createModel('product', { price: Number });
@@ -370,6 +340,14 @@ describe('loopback', function() {
       var db = loopback.createDataSource({ connector: loopback.Memory });
       var model = loopback.Model.extend(uniqueModelName);
 
+      // This test used to work because User model was already attached
+      // by other tests via `loopback.autoAttach()`
+      // Now that autoAttach is gone, it turns out the tested functionality
+      // does not work exactly as intended. To keep this change narrowly
+      // focused on removing autoAttach, we are attaching the User model
+      // to simulate the old test setup.
+      loopback.User.attachTo(db);
+
       loopback.configureModel(model, {
         dataSource: db,
         relations: {

test/model.application.test.js

@@ -5,6 +5,10 @@ var Application = loopback.Application;
 describe('Application', function() {
   var registeredApp = null;
 
+  before(function attachToMemory() {
+    Application.attachTo(loopback.memory());
+  });
+
   it('honors `application.register` - promise variant', function(done) {
     Application.register('rfeng', 'MyTestApp',
       { description: 'My test application' }, function(err, result) {

test/rest.middleware.test.js

@@ -1,11 +1,15 @@
 var path = require('path');
 
 describe('loopback.rest', function() {
-  var MyModel;
+  var app, MyModel;
+
   beforeEach(function() {
-    var ds = app.dataSource('db', { connector: loopback.Memory });
+    // override the global app object provided by test/support.js
-    MyModel = ds.createModel('MyModel', { name: String });
+    // and create a local one that does not share state with other tests
-    loopback.autoAttach();
+    app = loopback({ localRegistry: true, loadBuiltinModels: true });
+    var db = app.dataSource('db', { connector: 'memory' });
+    MyModel = app.registry.createModel('MyModel');
+    MyModel.attachTo(db);
   });
 
   it('works out-of-the-box', function(done) {
@@ -101,7 +105,7 @@ describe('loopback.rest', function() {
   });
 
   it('should honour `remoting.rest.supportedTypes`', function(done) {
-    var app = loopback();
+    var app = loopback({ localRegistry: true });
 
     // NOTE it is crucial to set `remoting` before creating any models
     var supportedTypes = ['json', 'application/javascript', 'text/javascript'];
@@ -117,26 +121,24 @@ describe('loopback.rest', function() {
   });
 
   it('allows models to provide a custom HTTP path', function(done) {
-    var ds = app.dataSource('db', { connector: loopback.Memory });
+    var CustomModel = app.registry.createModel('CustomModel',
-    var CustomModel = ds.createModel('CustomModel',
       { name: String },
       { http: { 'path': 'domain1/CustomModelPath' },
     });
 
-    app.model(CustomModel);
+    app.model(CustomModel, { dataSource: 'db' });
     app.use(loopback.rest());
 
     request(app).get('/domain1/CustomModelPath').expect(200).end(done);
   });
 
   it('should report 200 for url-encoded HTTP path', function(done) {
-    var ds = app.dataSource('db', { connector: loopback.Memory });
+    var CustomModel = app.registry.createModel('CustomModel',
-    var CustomModel = ds.createModel('CustomModel',
       { name: String },
       { http: { path: 'domain%20one/CustomModelPath' },
     });
 
-    app.model(CustomModel);
+    app.model(CustomModel, { dataSource: 'db' });
     app.use(loopback.rest());
 
     request(app).get('/domain%20one/CustomModelPath').expect(200).end(done);
@@ -144,12 +146,12 @@ describe('loopback.rest', function() {
 
   it('includes loopback.token when necessary', function(done) {
     givenUserModelWithAuth();
-    app.enableAuth();
+    app.enableAuth({ dataSource: 'db' });
     app.use(loopback.rest());
 
     givenLoggedInUser(function(err, token) {
       if (err) return done(err);
-      expect(token).instanceOf(app.models.accessToken);
+      expect(token).instanceOf(app.models.AccessToken);
       request(app).get('/users/' + token.userId)
         .set('Authorization', token.id)
         .expect(200)
@@ -268,14 +270,14 @@ describe('loopback.rest', function() {
 
     it('should enable context using loopback.context', function(done) {
       app.use(loopback.context({ enableHttpContext: true }));
-      app.enableAuth();
+      app.enableAuth({ dataSource: 'db' });
       app.use(loopback.rest());
 
       invokeGetToken(done);
     });
 
     it('should enable context with loopback.rest', function(done) {
-      app.enableAuth();
+      app.enableAuth({ dataSource: 'db' });
       app.set('remoting', { context: { enableHttpContext: true }});
       app.use(loopback.rest());
 
@@ -283,10 +285,10 @@ describe('loopback.rest', function() {
     });
 
     it('should support explicit context', function(done) {
-      app.enableAuth();
+      app.enableAuth({ dataSource: 'db' });
       app.use(loopback.context());
       app.use(loopback.token(
-        { model: loopback.getModelByType(loopback.AccessToken) }));
+        { model: app.registry.getModelByType('AccessToken') }));
       app.use(function(req, res, next) {
         loopback.getCurrentContext().set('accessToken', req.accessToken);
         next();
@@ -321,32 +323,26 @@ describe('loopback.rest', function() {
   });
 
   function givenUserModelWithAuth() {
-    // NOTE(bajtos) It is important to create a custom AccessToken model here,
+    var AccessToken = app.registry.getModel('AccessToken');
-    // in order to overwrite the entry created by previous tests in
+    app.model(AccessToken, { dataSource: 'db' });
-    // the global model registry
+    var User = app.registry.getModel('User');
-    app.model('accessToken', {
+    app.model(User, { dataSource: 'db' });
-      options: {
+
-        base: 'AccessToken',
+    // NOTE(bajtos) This is puzzling to me. The built-in User & AccessToken
-      },
+    // models should come with both relations already set up, i.e. the
-      dataSource: 'db',
+    // following two lines should not be neccessary.
-    });
+    // And it does behave that way when only tests in this file are run.
-    return app.model('user', {
+    // However, when I run the full test suite (all files), the relations
-      options: {
+    // get broken.
-        base: 'User',
+    AccessToken.belongsTo(User, { as: 'user', foreignKey: 'userId' });
-        relations: {
+    User.hasMany(AccessToken, { as: 'accessTokens', foreignKey: 'userId' });
-          accessTokens: {
+
-            model: 'accessToken',
+    return User;
-            type: 'hasMany',
-            foreignKey: 'userId',
-          },
-        },
-      },
-      dataSource: 'db',
-    });
   }
+
   function givenLoggedInUser(cb, done) {
     var credentials = { email: 'user@example.com', password: 'pwd' };
-    var User = app.models.user;
+    var User = app.models.User;
     User.create(credentials,
       function(err, user) {
         if (err) return done(err);

test/role.test.js

@@ -434,4 +434,28 @@ describe('role model', function() {
       });
     });
   });
+
+  describe('isOwner', function() {
+    it('supports app-local model registry', function(done) {
+      var app = loopback({ localRegistry: true, loadBuiltinModels: true });
+      app.dataSource('db', { connector: 'memory' });
+      // attach all auth-related models to 'db' datasource
+      app.enableAuth({ dataSource: 'db' });
+
+      var Role = app.models.Role;
+      var User = app.models.User;
+
+      var u = app.registry.findModel('User');
+      var credentials = { email: 'test@example.com', password: 'pass' };
+      User.create(credentials, function(err, user) {
+        if (err) return done(err);
+
+        Role.isOwner(User, user.id, user.id, function(err, result) {
+          if (err) return done(err);
+          expect(result, 'isOwner result').to.equal(true);
+          done();
+        });
+      });
+    });
+  });
 });

test/support.js

@@ -18,18 +18,6 @@ loopback.User.settings.saltWorkFactor = 4;
 
 beforeEach(function() {
   this.app = app = loopback();
-
-  // setup default data sources
-  loopback.setDefaultDataSourceForType('db', {
-    connector: loopback.Memory,
-  });
-
-  loopback.setDefaultDataSourceForType('mail', {
-    connector: loopback.Mail,
-    transports: [
-      { type: 'STUB' },
-    ],
-  });
 });
 
 assertValidDataSource = function(dataSource) {

test/user.test.js

@@ -1,11 +1,6 @@
 require('./support');
 var loopback = require('../');
 var User, AccessToken;
-var MailConnector = require('../lib/connectors/mail');
-
-var userMemory = loopback.createDataSource({
-  connector: 'memory',
-});
 
 describe('User', function() {
   var validCredentialsEmail = 'foo@bar.com';
@@ -21,44 +16,59 @@ describe('User', function() {
   var invalidCredentials = { email: 'foo1@bar.com', password: 'invalid' };
   var incompleteCredentials = { password: 'bar1' };
 
-  var defaultApp;
+  // Create a local app variable to prevent clashes with the global
+  // variable shared by all tests. While this should not be necessary if
+  // the tests were written correctly, it turns out that's not the case :(
+  var app;
 
-  beforeEach(function() {
+  beforeEach(function setupAppAndModels(done) {
-    // FIXME: [rfeng] Remove loopback.User.app so that remote hooks don't go
+    // override the global app object provided by test/support.js
-    // to the wrong app instance
+    // and create a local one that does not share state with other tests
-    defaultApp = loopback.User.app;
+    app = loopback({ localRegistry: true, loadBuiltinModels: true });
-    loopback.User.app = null;
+    app.dataSource('db', { connector: 'memory' });
-    User = loopback.User.extend('TestUser', {}, { http: { path: 'test-users' }});
+
-    AccessToken = loopback.AccessToken.extend('TestAccessToken');
+    // setup Email model, it's needed by User tests
-    User.email = loopback.Email.extend('email');
+    app.dataSource('email', {
-    loopback.autoAttach();
+      connector: loopback.Mail,
+      transports: [{ type: 'STUB' }],
+    });
+    var Email = app.registry.getModel('Email');
+    app.model(Email, { dataSource: 'email' });
+
+    // attach User and related models
+    User = app.registry.createModel('TestUser', {}, {
+      base: 'User',
+      http: { path: 'test-users' },
+    });
+    app.model(User, { dataSource: 'db' });
+
+    AccessToken = app.registry.getModel('AccessToken');
+    app.model(AccessToken, { dataSource: 'db' });
+
+    User.email = Email;
 
     // Update the AccessToken relation to use the subclass of User
     AccessToken.belongsTo(User, { as: 'user', foreignKey: 'userId' });
     User.hasMany(AccessToken, { as: 'accessTokens', foreignKey: 'userId' });
 
+    // Speed up the password hashing algorithm
+    // for tests using the built-in User model
+    User.settings.saltWorkFactor = 4;
+
     // allow many User.afterRemote's to be called
     User.setMaxListeners(0);
-  });
 
-  beforeEach(function(done) {
+    app.enableAuth({ dataSource: 'db' });
-    app.enableAuth();
     app.use(loopback.token({ model: AccessToken }));
     app.use(loopback.rest());
     app.model(User);
 
     User.create(validCredentials, function(err, user) {
+      if (err) return done(err);
       User.create(validCredentialsEmailVerified, done);
     });
   });
 
-  afterEach(function(done) {
-    loopback.User.app = defaultApp;
-    User.destroyAll(function(err) {
-      User.accessToken.destroyAll(done);
-    });
-  });
-
   describe('User.create', function() {
     it('Create a new user', function(done) {
       User.create({ email: 'f@b.com', password: 'bar' }, function(err, user) {
@@ -700,12 +710,19 @@ describe('User', function() {
   describe('User.login requiring realm', function() {
     var User, AccessToken;
 
-    before(function() {
+    beforeEach(function() {
-      User = loopback.User.extend('RealmUser', {},
+      User = app.registry.createModel('RealmUser', {}, {
-        { realmRequired: true, realmDelimiter: ':' });
+        base: 'TestUser',
-      AccessToken = loopback.AccessToken.extend('RealmAccessToken');
+        realmRequired: true,
+        realmDelimiter: ':',
+      });
 
-      loopback.autoAttach();
+      AccessToken = app.registry.createModel('RealmAccessToken', {}, {
+        base: 'AccessToken',
+      });
+
+      app.model(AccessToken, { dataSource: 'db' });
+      app.model(User, { dataSource: 'db' });
 
       // Update the AccessToken relation to use the subclass of User
       AccessToken.belongsTo(User, { as: 'user', foreignKey: 'userId' });
@@ -776,15 +793,6 @@ describe('User', function() {
       });
     });
 
-    afterEach(function(done) {
-      User.deleteAll({ realm: 'realm1' }, function(err) {
-        if (err) {
-          return done(err);
-        }
-        User.deleteAll({ realm: 'realm2' }, done);
-      });
-    });
-
     it('rejects a user by without realm', function(done) {
       User.login(credentialWithoutRealm, function(err, accessToken) {
         assert(err);
@@ -834,11 +842,11 @@ describe('User', function() {
     });
 
     describe('User.login with realmRequired but no realmDelimiter', function() {
-      before(function() {
+      beforeEach(function() {
         User.settings.realmDelimiter = undefined;
       });
 
-      after(function() {
+      afterEach(function() {
         User.settings.realmDelimiter = ':';
       });
 
@@ -1544,7 +1552,7 @@ describe('User', function() {
   describe('ctor', function() {
     it('exports default Email model', function() {
       expect(User.email, 'User.email').to.be.a('function');
-      expect(User.email.modelName, 'modelName').to.eql('email');
+      expect(User.email.modelName, 'modelName').to.eql('Email');
     });
 
     it('exports default AccessToken model', function() {