From 832e2c391c8bc7ab4bd9ccb27e016c5f876b1c87 Mon Sep 17 00:00:00 2001 From: gunjpan Date: Fri, 3 Jun 2016 16:51:48 -0400 Subject: [PATCH] Discard sugar method for model creation Current implementation of `app.model(modelName, settings)` works as a sugar for model creation. In 3.0, this is not supported anymore. This implementation reports an error when sugar is used for model creation. Includes: - Updated app.model() method - Fixed test cases reflecting the change --- 3.0-RELEASE-NOTES.md | 9 +++ example/colors/app.js | 6 +- example/replication/app.js | 8 +- lib/application.js | 38 ++++----- test/acl.test.js | 18 ++--- test/app.test.js | 89 +++++---------------- test/change-stream.test.js | 3 +- test/integration.test.js | 3 +- test/registries.test.js | 18 +++-- test/relations.integration.js | 136 ++++++++++++++++++--------------- test/remoting-coercion.test.js | 7 +- 11 files changed, 152 insertions(+), 183 deletions(-) diff --git a/3.0-RELEASE-NOTES.md b/3.0-RELEASE-NOTES.md index 29915b12..858720df 100644 --- a/3.0-RELEASE-NOTES.md +++ b/3.0-RELEASE-NOTES.md @@ -194,3 +194,12 @@ module: See also [loopback#2564](https://github.com/strongloop/loopback/pull/2564) and the official [documentation](https://docs.strongloop.com/display/APIC/Using+current+context) + +## Remove sugar for creating models + +`app.model(modelName, settings)`, a sugar for creating non-existing model, is +now removed in favor of promoting use of: +- `app.registry.createModel(modelName, properties, options)` to create new model +- `app.model(modelCtor, config)` to update existing model and attach it to app + +Please see [related code change](https://github.com/strongloop/loopback/pull/2401) here. diff --git a/example/colors/app.js b/example/colors/app.js index 0ff1541b..170de259 100644 --- a/example/colors/app.js +++ b/example/colors/app.js @@ -14,9 +14,9 @@ var schema = { name: String, }; -var Color = app.model('color', schema); - -app.dataSource('db', { adapter: 'memory' }).attach(Color); +app.dataSource('db', { connector: 'memory' }); +var Color = app.registry.createModel('color', schema); +app.model(Color, { dataSource: 'db' }); Color.create({ name: 'red' }); Color.create({ name: 'green' }); diff --git a/example/replication/app.js b/example/replication/app.js index 69c64183..2ebb1ac1 100644 --- a/example/replication/app.js +++ b/example/replication/app.js @@ -5,9 +5,11 @@ var loopback = require('../../'); var app = loopback(); -var db = app.dataSource('db', { connector: loopback.Memory }); -var Color = app.model('color', { dataSource: 'db', options: { trackChanges: true }}); -var Color2 = app.model('color2', { dataSource: 'db', options: { trackChanges: true }}); +var db = app.dataSource('db', { connector: 'memory' }); +var Color = app.registry.createModel('color', {}, { trackChanges: true }); +app.model(Color, { dataSource: 'db' }); +var Color2 = app.registry.createModel('color2', {}, { trackChanges: true }); +app.model(Color2, { dataSource: 'db' }); var target = Color2; var source = Color; var SPEED = process.env.SPEED || 100; diff --git a/lib/application.js b/lib/application.js index 0d79f170..9d7e462c 100644 --- a/lib/application.js +++ b/lib/application.js @@ -101,7 +101,7 @@ app.disuse = function(route) { * app.model(User, { dataSource: 'db' }); *``` * - * @param {Object|String} Model The model to attach. + * @param {Object} Model The model to attach. * @options {Object} config The model's configuration. * @property {String|DataSource} dataSource The `DataSource` to which to attach the model. * @property {Boolean} [public] Whether the model should be exposed via REST API. @@ -114,29 +114,15 @@ app.model = function(Model, config) { var isPublic = true; var registry = this.registry; + if (typeof Model === 'string') { + var msg = 'app.model(modelName, settings) is no longer supported. ' + + 'Use app.registry.createModel(modelName, definition) and ' + + 'app.model(ModelCtor, config) instead.'; + throw new Error(msg); + } + if (arguments.length > 1) { config = config || {}; - if (typeof Model === 'string') { - // create & attach the model - backwards compatibility - - // create config for loopback.modelFromConfig - var modelConfig = extend({}, config); - modelConfig.options = extend({}, config.options); - modelConfig.name = Model; - - // modeller does not understand `dataSource` option - delete modelConfig.dataSource; - - Model = registry.createModel(modelConfig); - - // delete config options already applied - ['relations', 'base', 'acls', 'hidden', 'methods'].forEach(function(prop) { - delete config[prop]; - if (config.options) delete config.options[prop]; - }); - delete config.properties; - } - configureModel(Model, config, this); isPublic = config.public !== false; } else { @@ -186,7 +172,7 @@ app.model = function(Model, config) { * }); * ``` * - * 2. Use `app.model` to access a model by name. + * 2. Use `app.models` to access a model by name. * `app.models` has properties for all defined models. * * The following example illustrates accessing the `Product` and `CustomerReceipt` models @@ -201,8 +187,10 @@ app.model = function(Model, config) { * } * }); * - * app.model('product', {dataSource: 'db'}); - * app.model('customer-receipt', {dataSource: 'db'}); + * var productModel = app.registry.createModel('product'); + * app.model(productModel, {dataSource: 'db'}); + * var customerReceiptModel = app.registry.createModel('customer-receipt'); + * app.model(customerReceiptModel, {dataSource: 'db'}); * * // available based on the given name * var Product = app.models.Product; diff --git a/test/acl.test.js b/test/acl.test.js index dd2ff474..b2f7d30c 100644 --- a/test/acl.test.js +++ b/test/acl.test.js @@ -393,20 +393,18 @@ describe('security ACLs', function() { }); describe('access check', function() { - var app; - before(function() { - app = loopback(); - app.set('remoting', { errorHandler: { debug: true, log: false }}); - app.use(loopback.rest()); - app.enableAuth(); - app.dataSource('test', { connector: 'memory' }); - }); - it('should occur before other remote hooks', function(done) { - var MyTestModel = app.model('MyTestModel', { base: 'PersistedModel', dataSource: 'test' }); + var app = loopback(); + var MyTestModel = app.registry.createModel('MyTestModel'); var checkAccessCalled = false; var beforeHookCalled = false; + app.use(loopback.rest()); + app.set('remoting', { errorHandler: { debug: true, log: false }}); + app.enableAuth(); + app.dataSource('test', { connector: 'memory' }); + app.model(MyTestModel, { dataSource: 'test' }); + // fake / spy on the checkAccess method MyTestModel.checkAccess = function() { var cb = arguments[arguments.length - 1]; diff --git a/test/app.test.js b/test/app.test.js index 352580e4..4ad8c97b 100644 --- a/test/app.test.js +++ b/test/app.test.js @@ -611,11 +611,12 @@ describe('app', function() { }); describe('app.model(Model)', function() { - var app, db; + var app, db, MyTestModel; beforeEach(function() { app = loopback(); app.set('remoting', { errorHandler: { debug: true, log: false }}); db = loopback.createDataSource({ connector: loopback.Memory }); + MyTestModel = app.registry.createModel('MyTestModel'); }); it('Expose a `Model` to remote clients', function() { @@ -626,7 +627,7 @@ describe('app', function() { expect(app.models()).to.eql([Color]); }); - it('uses singlar name as app.remoteObjects() key', function() { + it('uses singular name as app.remoteObjects() key', function() { var Color = PersistedModel.extend('color', { name: String }); app.model(Color); Color.attachTo(db); @@ -689,76 +690,26 @@ describe('app', function() { }); }); - it('accepts null dataSource', function() { - app.model('MyTestModel', { dataSource: null }); + it('accepts null dataSource', function(done) { + app.model(MyTestModel, { dataSource: null }); + expect(MyTestModel.dataSource).to.eql(null); + done(); }); - it('accepts false dataSource', function() { - app.model('MyTestModel', { dataSource: false }); + it('accepts false dataSource', function(done) { + app.model(MyTestModel, { dataSource: false }); + expect(MyTestModel.getDataSource()).to.eql(null); + done(); }); - it('should not require dataSource', function() { - app.model('MyTestModel', {}); - }); - }); - - describe('app.model(name, config)', function() { - var app; - - beforeEach(function() { - app = loopback(); - app.dataSource('db', { - connector: 'memory', - }); + it('does not require dataSource', function(done) { + app.model(MyTestModel); + done(); }); - it('Sugar for defining a fully built model', function() { - app.model('foo', { - dataSource: 'db', - }); - - var Foo = app.models.foo; - var f = new Foo(); - - assert(f instanceof app.registry.getModel('Model')); - }); - - it('interprets extra first-level keys as options', function() { - app.model('foo', { - dataSource: 'db', - base: 'User', - }); - - expect(app.models.foo.definition.settings.base).to.equal('User'); - }); - - it('prefers config.options.key over config.key', function() { - app.model('foo', { - dataSource: 'db', - base: 'User', - options: { - base: 'Application', - }, - }); - - expect(app.models.foo.definition.settings.base).to.equal('Application'); - }); - - it('honors config.public options', function() { - app.model('foo', { - dataSource: 'db', - public: false, - }); - expect(app.models.foo.app).to.equal(app); - expect(app.models.foo.shared).to.equal(false); - }); - - it('defaults config.public to be true', function() { - app.model('foo', { - dataSource: 'db', - }); - expect(app.models.foo.app).to.equal(app); - expect(app.models.foo.shared).to.equal(true); + it('throws error if model typeof string is passed', function() { + var fn = function() { app.model('MyTestModel'); }; + expect(fn).to.throw(/app(\.model|\.registry)/); }); }); @@ -772,7 +723,8 @@ describe('app', function() { } assert(!previousModel || !previousModel.dataSource); - app.model('TestModel', { dataSource: 'db' }); + var TestModel = app.registry.createModel('TestModel'); + app.model(TestModel, { dataSource: 'db' }); expect(app.models.TestModel.dataSource).to.equal(app.dataSources.db); }); }); @@ -780,7 +732,8 @@ describe('app', function() { describe('app.models', function() { it('is unique per app instance', function() { app.dataSource('db', { connector: 'memory' }); - var Color = app.model('Color', { dataSource: 'db' }); + var Color = app.registry.createModel('Color'); + app.model(Color, { dataSource: 'db' }); expect(app.models.Color).to.equal(Color); var anotherApp = loopback(); expect(anotherApp.models.Color).to.equal(undefined); diff --git a/test/change-stream.test.js b/test/change-stream.test.js index c2ecd56d..222a64c6 100644 --- a/test/change-stream.test.js +++ b/test/change-stream.test.js @@ -9,7 +9,8 @@ describe('PersistedModel.createChangeStream()', function() { var test = this; var app = loopback({ localRegistry: true }); var ds = app.dataSource('ds', { connector: 'memory' }); - this.Score = app.model('Score', { + var Score = app.registry.createModel('Score'); + this.Score = app.model(Score, { dataSource: 'ds', changeDataSource: false, // use only local observers }); diff --git a/test/integration.test.js b/test/integration.test.js index 14575bfe..885bc1a4 100644 --- a/test/integration.test.js +++ b/test/integration.test.js @@ -40,7 +40,8 @@ describe('loopback application', function() { loopback.ACL.attachTo(db); loopback.User.hasMany(loopback.AccessToken, { as: 'accessTokens' }); - var Streamer = app.model('Streamer', { dataSource: 'db' }); + var Streamer = app.registry.createModel('Streamer'); + app.model(Streamer, { dataSource: 'db' }); Streamer.read = function(req, res, cb) { var body = new Buffer(0); req.on('data', function(chunk) { diff --git a/test/registries.test.js b/test/registries.test.js index a4b01d56..d3600e1a 100644 --- a/test/registries.test.js +++ b/test/registries.test.js @@ -26,15 +26,17 @@ describe('Registry', function() { var dsFoo = appFoo.dataSource('dsFoo', { connector: 'memory' }); var dsBar = appFoo.dataSource('dsBar', { connector: 'memory' }); - var FooModel = appFoo.model(modelName, settings); - var FooSubModel = appFoo.model(subModelName, settings); - var BarModel = appBar.model(modelName, settings); - var BarSubModel = appBar.model(subModelName, settings); + var FooModel = appFoo.registry.createModel(modelName, {}, settings); + appFoo.model(FooModel, { dataSource: dsFoo }); - FooModel.attachTo(dsFoo); - FooSubModel.attachTo(dsFoo); - BarModel.attachTo(dsBar); - BarSubModel.attachTo(dsBar); + var FooSubModel = appFoo.registry.createModel(subModelName, {}, settings); + appFoo.model(FooSubModel, { dataSource: dsFoo }); + + var BarModel = appBar.registry.createModel(modelName, {}, settings); + appBar.model(BarModel, { dataSource: dsBar }); + + var BarSubModel = appBar.registry.createModel(subModelName, {}, settings); + appBar.model(BarSubModel, { dataSource: dsBar }); FooModel.hasMany(FooSubModel); BarModel.hasMany(BarSubModel); diff --git a/test/relations.integration.js b/test/relations.integration.js index beb33c89..a84c6f75 100644 --- a/test/relations.integration.js +++ b/test/relations.integration.js @@ -31,24 +31,14 @@ describe('relations - integration', function() { describe('polymorphicHasMany', function() { before(function defineProductAndCategoryModels() { - var Team = app.model( - 'Team', - { properties: { name: 'string' }, - dataSource: 'db', - } - ); - var Reader = app.model( - 'Reader', - { properties: { name: 'string' }, - dataSource: 'db', - } - ); - var Picture = app.model( - 'Picture', - { properties: { name: 'string', imageableId: 'number', imageableType: 'string' }, - dataSource: 'db', - } - ); + var Team = app.registry.createModel('Team', { name: 'string' }); + var Reader = app.registry.createModel('Reader', { name: 'string' }); + var Picture = app.registry.createModel('Picture', + { name: 'string', imageableId: 'number', imageableType: 'string' }); + + app.model(Team, { dataSource: 'db' }); + app.model(Reader, { dataSource: 'db' }); + app.model(Picture, { dataSource: 'db' }); Reader.hasMany(Picture, { polymorphic: { // alternative syntax as: 'imageable', // if not set, default to: reference @@ -640,15 +630,17 @@ describe('relations - integration', function() { describe('hasAndBelongsToMany', function() { beforeEach(function defineProductAndCategoryModels() { - var product = app.model( - 'product', - { properties: { id: 'string', name: 'string' }, dataSource: 'db' } - + var product = app.registry.createModel( + 'product', + { id: 'string', name: 'string' } ); - var category = app.model( + var category = app.registry.createModel( 'category', - { properties: { id: 'string', name: 'string' }, dataSource: 'db' } + { id: 'string', name: 'string' } ); + app.model(product, { dataSource: 'db' }); + app.model(category, { dataSource: 'db' }); + product.hasAndBelongsToMany(category); category.hasAndBelongsToMany(product); }); @@ -762,17 +754,18 @@ describe('relations - integration', function() { describe('embedsOne', function() { before(function defineGroupAndPosterModels() { - var group = app.model( - 'group', - { properties: { name: 'string' }, - dataSource: 'db', - plural: 'groups', - } + var group = app.registry.createModel('group', + { name: 'string' }, + { plural: 'groups' } ); - var poster = app.model( + app.model(group, { dataSource: 'db' }); + + var poster = app.registry.createModel( 'poster', - { properties: { url: 'string' }, dataSource: 'db' } + { url: 'string' } ); + app.model(poster, { dataSource: 'db' }); + group.embedsOne(poster, { as: 'cover' }); }); @@ -877,18 +870,19 @@ describe('relations - integration', function() { describe('embedsMany', function() { before(function defineProductAndCategoryModels() { - var todoList = app.model( + var todoList = app.registry.createModel( 'todoList', - { properties: { name: 'string' }, - dataSource: 'db', - plural: 'todo-lists', - } + { name: 'string' }, + { plural: 'todo-lists' } ); - var todoItem = app.model('todoItem', { - properties: { content: 'string' }, - forceId: false, - dataSource: 'db', - }); + app.model(todoList, { dataSource: 'db' }); + + var todoItem = app.registry.createModel( + 'todoItem', + { content: 'string' }, { forceId: false } + ); + app.model(todoItem, { dataSource: 'db' }); + todoList.embedsMany(todoItem, { as: 'items' }); }); @@ -1064,18 +1058,24 @@ describe('relations - integration', function() { describe('referencesMany', function() { before(function defineProductAndCategoryModels() { - var recipe = app.model( + var recipe = app.registry.createModel( 'recipe', - { properties: { name: 'string' }, dataSource: 'db' } + { name: 'string' } ); - var ingredient = app.model( + app.model(recipe, { dataSource: 'db' }); + + var ingredient = app.registry.createModel( 'ingredient', - { properties: { name: 'string' }, dataSource: 'db' } + { name: 'string' } ); - var photo = app.model( + app.model(ingredient, { dataSource: 'db' }); + + var photo = app.registry.createModel( 'photo', - { properties: { name: 'string' }, dataSource: 'db' } + { name: 'string' } ); + app.model(photo, { dataSource: 'db' }); + recipe.referencesMany(ingredient); // contrived example for test: recipe.hasOne(photo, { as: 'picture', options: { @@ -1410,31 +1410,41 @@ describe('relations - integration', function() { describe('nested relations', function() { before(function defineModels() { - var Book = app.model( + var Book = app.registry.createModel( 'Book', - { properties: { name: 'string' }, dataSource: 'db', - plural: 'books' } + { name: 'string' }, + { plural: 'books' } ); - var Page = app.model( + app.model(Book, { dataSource: 'db' }); + + var Page = app.registry.createModel( 'Page', - { properties: { name: 'string' }, dataSource: 'db', - plural: 'pages' } + { name: 'string' }, + { plural: 'pages' } ); - var Image = app.model( + app.model(Page, { dataSource: 'db' }); + + var Image = app.registry.createModel( 'Image', - { properties: { name: 'string' }, dataSource: 'db', - plural: 'images' } + { name: 'string' }, + { plural: 'images' } ); - var Note = app.model( + app.model(Image, { dataSource: 'db' }); + + var Note = app.registry.createModel( 'Note', - { properties: { text: 'string' }, dataSource: 'db', - plural: 'notes' } + { text: 'string' }, + { plural: 'notes' } ); - var Chapter = app.model( + app.model(Note, { dataSource: 'db' }); + + var Chapter = app.registry.createModel( 'Chapter', - { properties: { name: 'string' }, dataSource: 'db', - plural: 'chapters' } + { name: 'string' }, + { plural: 'chapters' } ); + app.model(Chapter, { dataSource: 'db' }); + Book.hasMany(Page); Book.hasMany(Chapter); Page.hasMany(Note); diff --git a/test/remoting-coercion.test.js b/test/remoting-coercion.test.js index 232951e0..c96ffd0b 100644 --- a/test/remoting-coercion.test.js +++ b/test/remoting-coercion.test.js @@ -12,7 +12,12 @@ describe('remoting coercion', function() { var app = loopback(); app.use(loopback.rest()); - var TestModel = app.model('TestModel', { base: 'Model', dataSource: null, public: true }); + var TestModel = app.registry.createModel('TestModel', + {}, + { base: 'Model' } + ); + app.model(TestModel, { public: true }); + TestModel.test = function(inst, cb) { called = true; assert(inst instanceof TestModel);