From a1d3e72046763ce9936da809682b440c3e9edd29 Mon Sep 17 00:00:00 2001 From: Rand McKinney Date: Tue, 24 Jun 2014 16:15:35 -0700 Subject: [PATCH 01/23] Update link to doc --- README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 1a14aa30..66f41ee5 100644 --- a/README.md +++ b/README.md @@ -5,8 +5,8 @@ for interacting with databases, REST APIs, and other data sources. It was initially forked from [JugglingDB](https://github.com/1602/jugglingdb). **For full documentation, see the official StrongLoop documentation**: - * [Data sources and connectors](http://docs.strongloop.com/display/DOC/Data+sources+and+connectors) - * [Data Source Juggler](http://docs.strongloop.com/display/DOC/Data+Source+Juggler). + * [Data sources and connectors](http://docs.strongloop.com/display/LB/Data+sources+and+connectors) + * [Creating data sources and connected models](http://docs.strongloop.com/display/LB/Creating+data+sources+and+connected+models). ## Installation From a3ed1a575e7dff0d8832b4634cf372b50f75957f Mon Sep 17 00:00:00 2001 From: Ritchie Martori Date: Thu, 26 Jun 2014 14:48:27 -0700 Subject: [PATCH 02/23] Add "hasOne" to relationTypes --- lib/datasource.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/datasource.js b/lib/datasource.js index 7c8b4190..e1841968 100644 --- a/lib/datasource.js +++ b/lib/datasource.js @@ -364,7 +364,7 @@ function isModelClass(cls) { return cls.prototype instanceof ModelBaseClass; } -DataSource.relationTypes = ['belongsTo', 'hasMany', 'hasAndBelongsToMany']; +DataSource.relationTypes = ['belongsTo', 'hasMany', 'hasAndBelongsToMany', 'hasOne']; function isModelDataSourceAttached(model) { return model && (!model.settings.unresolved) && (model.dataSource instanceof DataSource); From 45154913188c09a00f06e49502227343726fc4a7 Mon Sep 17 00:00:00 2001 From: Raymond Feng Date: Thu, 26 Jun 2014 23:38:04 -0700 Subject: [PATCH 03/23] Make sure type of the foreign key match the primary key See https://github.com/strongloop/loopback/issues/353 --- lib/datasource.js | 28 +++++++++------- lib/relation-definition.js | 69 +++++++++++++++++++++++++++++++++----- lib/scope.js | 4 ++- test/relations.test.js | 4 +-- 4 files changed, 81 insertions(+), 24 deletions(-) diff --git a/lib/datasource.js b/lib/datasource.js index 7c8b4190..64d0c210 100644 --- a/lib/datasource.js +++ b/lib/datasource.js @@ -1579,22 +1579,26 @@ DataSource.prototype.idProperty = function (modelName) { * @param {String} foreignClassName The foreign model name */ DataSource.prototype.defineForeignKey = function defineForeignKey(className, key, foreignClassName) { - // quit if key already defined - if (this.getModelDefinition(className).rawProperties[key]) return; - - var defaultType = Number; - if (foreignClassName) { - var foreignModel = this.getModelDefinition(foreignClassName); - var pkName = foreignModel && foreignModel.idName(); - if (pkName) { - defaultType = foreignModel.properties[pkName].type; - } + var pkType = null; + var foreignModel = this.getModelDefinition(foreignClassName); + var pkName = foreignModel && foreignModel.idName(); + if (pkName) { + pkType = foreignModel.properties[pkName].type; } + var model = this.getModelDefinition(className); + if (model.properties[key]) { + if (pkType) { + // Reset the type of the foreign key + model.rawProperties[key].type = model.properties[key].type = pkType; + } + return; + } + if (this.connector.defineForeignKey) { var cb = function (err, keyType) { if (err) throw err; // Add the foreign key property to the data source _models - this.defineProperty(className, key, {type: keyType || defaultType}); + this.defineProperty(className, key, {type: keyType || pkType}); }.bind(this); switch (this.connector.defineForeignKey.length) { case 4: @@ -1607,7 +1611,7 @@ DataSource.prototype.defineForeignKey = function defineForeignKey(className, key } } else { // Add the foreign key property to the data source _models - this.defineProperty(className, key, {type: defaultType}); + this.defineProperty(className, key, {type: pkType}); } }; diff --git a/lib/relation-definition.js b/lib/relation-definition.js index fe4a6d7d..c50cf0d6 100644 --- a/lib/relation-definition.js +++ b/lib/relation-definition.js @@ -336,7 +336,7 @@ RelationDefinition.hasMany = function hasMany(modelFrom, modelTo, params) { if (!params.through) { // obviously, modelTo should have attribute called `fk` - modelTo.dataSource.defineForeignKey(modelTo.modelName, fk, this.modelName); + modelTo.dataSource.defineForeignKey(modelTo.modelName, fk, modelFrom.modelName); } var scopeMethods = { @@ -348,6 +348,9 @@ RelationDefinition.hasMany = function hasMany(modelFrom, modelTo, params) { scopeMethods.create = scopeMethod(definition, 'create'); scopeMethods.add = scopeMethod(definition, 'add'); scopeMethods.remove = scopeMethod(definition, 'remove'); + } else { + scopeMethods.create = scopeMethod(definition, 'create'); + scopeMethods.build = scopeMethod(definition, 'build'); } // Mix the property and scoped methods into the prototype class @@ -623,6 +626,11 @@ BelongsTo.prototype.create = function(targetModelData, cb) { var pk = this.definition.keyFrom; var modelInstance = this.modelInstance; + if (typeof targetModelData === 'function' && !cb) { + cb = targetModelData; + targetModelData = {}; + } + modelTo.create(targetModelData, function(err, targetModel) { if(!err) { modelInstance[fk] = targetModel[pk]; @@ -695,7 +703,7 @@ BelongsTo.prototype.related = function (refresh, params) { return cachedValue; } } else if (params === undefined) { // acts as sync getter - return modelInstance[fk]; + return cachedValue; } else { // setter modelInstance[fk] = params; self.resetCache(); @@ -813,33 +821,77 @@ RelationDefinition.hasOne = function (modelFrom, modelTo, params) { * @param {String|Object} err Error string or object * @param {Object} The newly created target model instance */ -HasOne.prototype.create = function(targetModelData, cb) { +HasOne.prototype.create = function (targetModelData, cb) { var self = this; var modelTo = this.definition.modelTo; var fk = this.definition.keyTo; var pk = this.definition.keyFrom; var modelInstance = this.modelInstance; - var relationName = this.definition.name + if (typeof targetModelData === 'function' && !cb) { + cb = targetModelData; + targetModelData = {}; + } + targetModelData = targetModelData || {}; + targetModelData[fk] = modelInstance[pk]; + var query = {where: {}}; + query.where[fk] = targetModelData[fk] + modelTo.findOne(query, function(err, result) { + if(err) { + cb(err); + } else if(result) { + cb(new Error('HasOne relation cannot create more than one instance of ' + + modelTo.modelName)); + } else { + modelTo.create(targetModelData, function (err, targetModel) { + if (!err) { + // Refresh the cache + self.resetCache(targetModel); + cb && cb(err, targetModel); + } else { + cb && cb(err); + } + }); + } + }); +}; + +/** + * Create a target model instance + * @param {Object} targetModelData The target model data + * @callback {Function} [cb] Callback function + * @param {String|Object} err Error string or object + * @param {Object} The newly created target model instance + */ +HasMany.prototype.create = function (targetModelData, cb) { + var self = this; + var modelTo = this.definition.modelTo; + var fk = this.definition.keyTo; + var pk = this.definition.keyFrom; + var modelInstance = this.modelInstance; + + if (typeof targetModelData === 'function' && !cb) { + cb = targetModelData; + targetModelData = {}; + } targetModelData = targetModelData || {}; targetModelData[fk] = modelInstance[pk]; modelTo.create(targetModelData, function(err, targetModel) { if(!err) { // Refresh the cache - self.resetCache(targetModel); + self.addToCache(targetModel); cb && cb(err, targetModel); } else { cb && cb(err); } }); }; - /** * Build a target model instance * @param {Object} targetModelData The target model data * @returns {Object} The newly built target model instance */ -HasOne.prototype.build = function(targetModelData) { +HasMany.prototype.build = HasOne.prototype.build = function(targetModelData) { var modelTo = this.definition.modelTo; var pk = this.definition.keyFrom; var fk = this.definition.keyTo; @@ -865,7 +917,6 @@ HasOne.prototype.related = function (refresh, params) { var fk = this.definition.keyTo; var pk = this.definition.keyFrom; var modelInstance = this.modelInstance; - var relationName = this.definition.name; if (arguments.length === 1) { params = refresh; @@ -907,7 +958,7 @@ HasOne.prototype.related = function (refresh, params) { return cachedValue; } } else if (params === undefined) { // acts as sync getter - return modelInstance[pk]; + return cachedValue; } else { // setter params[fk] = modelInstance[pk]; self.resetCache(); diff --git a/lib/scope.js b/lib/scope.js index 8a4485df..5267ce8d 100644 --- a/lib/scope.js +++ b/lib/scope.js @@ -202,7 +202,9 @@ function defineScope(cls, targetClass, name, params, methods) { var prop = targetClass.definition.properties[i]; if (prop) { var val = where[i]; - if (typeof val !== 'object' || val instanceof prop.type) { + if (typeof val !== 'object' || val instanceof prop.type + || prop.type.name === 'ObjectID') // MongoDB key + { // Only pick the {propertyName: propertyValue} data[i] = where[i]; } diff --git a/test/relations.test.js b/test/relations.test.js index 4e1f5873..b8f34254 100644 --- a/test/relations.test.js +++ b/test/relations.test.js @@ -155,7 +155,7 @@ describe('relations', function () { should.not.exist(e); should.exist(l); l.should.be.an.instanceOf(List); - todo.list().should.equal(l.id); + todo.list().id.should.equal(l.id); done(); }); }); @@ -206,7 +206,7 @@ describe('relations', function () { should.not.exist(e); should.exist(act); act.should.be.an.instanceOf(Account); - supplier.account().should.equal(act.id); + supplier.account().id.should.equal(act.id); done(); }); }); From e0c76199085eb09b94411e13171a20d033ccde48 Mon Sep 17 00:00:00 2001 From: Raymond Feng Date: Thu, 26 Jun 2014 23:40:20 -0700 Subject: [PATCH 04/23] Normalize filter.order and enforce more checks --- lib/connectors/memory.js | 6 +++ lib/dao.js | 112 ++++++++++++++++++++++++++++++++------- test/memory.test.js | 58 ++++++++++++++++++++ 3 files changed, 156 insertions(+), 20 deletions(-) diff --git a/lib/connectors/memory.js b/lib/connectors/memory.js index b568a0f3..7441ddd6 100644 --- a/lib/connectors/memory.js +++ b/lib/connectors/memory.js @@ -270,6 +270,12 @@ Memory.prototype.all = function all(model, filter, callback) { }.bind(this)); if (filter) { + if (!filter.order) { + var idNames = this.idNames(model); + if (idNames && idNames.length) { + filter.order = idNames; + } + } // do we need some sorting? if (filter.order) { var orders = filter.order; diff --git a/lib/dao.js b/lib/dao.js index 745b6996..131be88c 100644 --- a/lib/dao.js +++ b/lib/dao.js @@ -434,6 +434,48 @@ DataAccessObject._normalize = function (filter) { filter.skip = offset; } + if (filter.order) { + var order = filter.order; + if (!Array.isArray(order)) { + order = [order]; + } + var fields = []; + for (var i = 0, m = order.length; i < m; i++) { + if (typeof order[i] === 'string') { + // Normalize 'f1 ASC, f2 DESC, f3' to ['f1 ASC', 'f2 DESC', 'f3'] + var tokens = order[i].split(/(?:\s*,\s*)+/); + for (var t = 0, n = tokens.length; t < n; t++) { + var token = tokens[t]; + if (token.length === 0) { + // Skip empty token + continue; + } + var parts = token.split(/\s+/); + if (parts.length >= 2) { + var dir = parts[1].toUpperCase(); + if (dir === 'ASC' || dir === 'DESC') { + token = parts[0] + ' ' + dir; + } else { + err = new Error(util.format('The order %j has invalid direction', token)); + err.statusCode = 400; + throw err; + } + } + fields.push(token); + } + } else { + err = new Error(util.format('The order %j is not valid', order[i])); + err.statusCode = 400; + throw err; + } + } + if (fields.length === 1 && typeof filter.order === 'string') { + filter.order = fields[0]; + } else { + filter.order = fields; + } + } + // normalize fields as array of included property names if (filter.fields) { filter.fields = fieldsToArray(filter.fields, @@ -445,6 +487,25 @@ DataAccessObject._normalize = function (filter) { return filter; }; +function DateType(arg) { + return new Date(arg); +} + +function BooleanType(val) { + if (val === 'true') { + return true; + } else if (val === 'false') { + return false; + } else { + return Boolean(val); + } +} + +function NumberType(val) { + var num = Number(val); + return !isNaN(num) ? num : val; +} + /* * Coerce values based the property types * @param {Object} where The where clause @@ -470,11 +531,11 @@ DataAccessObject._coerce = function (where) { if (p === 'and' || p === 'or' || p === 'nor') { var clauses = where[p]; if (Array.isArray(clauses)) { - for (var i = 0; i < clauses.length; i++) { - self._coerce(clauses[i]); + for (var k = 0; k < clauses.length; k++) { + self._coerce(clauses[k]); } } else { - err = new Error(util.format('The %p operator has invalid clauses %j', p, clauses)); + err = new Error(util.format('The %s operator has invalid clauses %j', p, clauses)); err.statusCode = 400; throw err; } @@ -488,30 +549,16 @@ DataAccessObject._coerce = function (where) { DataType = DataType[0]; } if (DataType === Date) { - var OrigDate = Date; - DataType = function Date(arg) { - return new OrigDate(arg); - }; + DataType = DateType; } else if (DataType === Boolean) { - DataType = function (val) { - if (val === 'true') { - return true; - } else if (val === 'false') { - return false; - } else { - return Boolean(val); - } - }; + DataType = BooleanType; } else if (DataType === Number) { // This fixes a regression in mongodb connector // For numbers, only convert it produces a valid number // LoopBack by default injects a number id. We should fix it based // on the connector's input, for example, MongoDB should use string // while RDBs typically use number - DataType = function (val) { - var num = Number(val); - return !isNaN(num) ? num : val; - }; + DataType = NumberType; } if (!DataType) { @@ -543,6 +590,31 @@ DataAccessObject._coerce = function (where) { if (op in val) { val = val[op]; operator = op; + switch(operator) { + case 'inq': + case 'nin': + if (!Array.isArray(val)) { + err = new Error(util.format('The %s property has invalid clause %j', p, where[p])); + err.statusCode = 400; + throw err; + } + break; + case 'between': + if (!Array.isArray(val) || val.length !== 2) { + err = new Error(util.format('The %s property has invalid clause %j', p, where[p])); + err.statusCode = 400; + throw err; + } + break; + case 'like': + case 'nlike': + if (!(typeof val === 'string' || val instanceof RegExp)) { + err = new Error(util.format('The %s property has invalid clause %j', p, where[p])); + err.statusCode = 400; + throw err; + } + break; + } break; } } diff --git a/test/memory.test.js b/test/memory.test.js index 4016209e..40283b82 100644 --- a/test/memory.test.js +++ b/test/memory.test.js @@ -141,6 +141,64 @@ describe('Memory connector', function () { }); }); + it('should throw if the like value is not string or regexp', function (done) { + User.find({where: {name: {like: 123}}}, function (err, posts) { + should.exist(err); + done(); + }); + }); + + it('should throw if the nlike value is not string or regexp', function (done) { + User.find({where: {name: {nlike: 123}}}, function (err, posts) { + should.exist(err); + done(); + }); + }); + + it('should throw if the inq value is not an array', function (done) { + User.find({where: {name: {inq: '12'}}}, function (err, posts) { + should.exist(err); + done(); + }); + }); + + it('should throw if the nin value is not an array', function (done) { + User.find({where: {name: {nin: '12'}}}, function (err, posts) { + should.exist(err); + done(); + }); + }); + + it('should throw if the between value is not an array', function (done) { + User.find({where: {name: {between: '12'}}}, function (err, posts) { + should.exist(err); + done(); + }); + }); + + it('should throw if the between value is not an array of length 2', function (done) { + User.find({where: {name: {between: ['12']}}}, function (err, posts) { + should.exist(err); + done(); + }); + }); + + it('support order with multiple fields', function (done) { + User.find({order: 'vip ASC, seq DESC'}, function (err, posts) { + should.not.exist(err); + posts[0].seq.should.be.eql(4); + posts[1].seq.should.be.eql(3); + done(); + }); + }); + + it('should throw if order has wrong direction', function (done) { + User.find({order: 'seq ABC'}, function (err, posts) { + should.exist(err); + done(); + }); + }); + function seed(done) { var beatles = [ { From e7a97da3bfea18449413bb6460c6dfbfd52aae87 Mon Sep 17 00:00:00 2001 From: Raymond Feng Date: Fri, 27 Jun 2014 10:12:10 -0700 Subject: [PATCH 05/23] Bump version and update deps --- package.json | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/package.json b/package.json index 8e633c6d..ace15d4e 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "loopback-datasource-juggler", - "version": "1.6.1", + "version": "1.6.2", "description": "LoopBack DataSoure Juggler", "keywords": [ "StrongLoop", @@ -24,14 +24,14 @@ ], "devDependencies": { "should": "~1.2.2", - "mocha": "~1.18.2" + "mocha": "~1.20.1" }, "dependencies": { "async": "~0.9.0", - "inflection": "~1.3.5", - "traverse": "~0.6.6", + "debug": "~1.0.2", + "inflection": "~1.3.7", "qs": "~0.6.6", - "debug": "~0.8.1" + "traverse": "~0.6.6" }, "license": { "name": "Dual MIT/StrongLoop", From f7afade22902fc251a251eb7aaaecd27e4cb3ba2 Mon Sep 17 00:00:00 2001 From: Raymond Feng Date: Wed, 2 Jul 2014 12:20:56 -0700 Subject: [PATCH 06/23] Make sure 'upsert' is used as the remote operation name See https://github.com/strongloop/loopback/issues/359 --- lib/dao.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/dao.js b/lib/dao.js index 131be88c..7e8194cb 100644 --- a/lib/dao.js +++ b/lib/dao.js @@ -222,7 +222,10 @@ function stillConnecting(dataSource, obj, args) { * @param {Object} data The model instance data * @param {Function} callback The callback function (optional). */ -DataAccessObject.upsert = DataAccessObject.updateOrCreate = function upsert(data, callback) { +// [FIXME] rfeng: This is a hack to set up 'upsert' first so that +// 'upsert' will be used as the name for strong-remoting to keep it backward +// compatible for angular SDK +DataAccessObject.updateOrCreate = DataAccessObject.upsert = function upsert(data, callback) { if (stillConnecting(this.getDataSource(), this, arguments)) { return; } From 0296ef113d1d57fbdd03e779d140ef2d31c55be9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miroslav=20Bajto=C5=A1?= Date: Thu, 3 Jul 2014 08:17:01 +0200 Subject: [PATCH 07/23] Make sure 'deleteById' is used as the remote operation name See strongloop/loopback#359 --- lib/dao.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/dao.js b/lib/dao.js index 7e8194cb..bc04d80b 100644 --- a/lib/dao.js +++ b/lib/dao.js @@ -897,7 +897,10 @@ DataAccessObject.remove = DataAccessObject.deleteAll = DataAccessObject.destroyA * @param {Function} cb Callback called with (err) */ -DataAccessObject.removeById = DataAccessObject.deleteById = DataAccessObject.destroyById = function deleteById(id, cb) { +// [FIXME] rfeng: This is a hack to set up 'deleteById' first so that +// 'deleteById' will be used as the name for strong-remoting to keep it backward +// compatible for angular SDK +DataAccessObject.removeById = DataAccessObject.destroyById = DataAccessObject.deleteById = function deleteById(id, cb) { if (stillConnecting(this.getDataSource(), this, arguments)) return; var Model = this; From 1e3be7f474b0c903fa0fcb2bf58c73e12c85c307 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miroslav=20Bajto=C5=A1?= Date: Thu, 3 Jul 2014 08:41:31 +0200 Subject: [PATCH 08/23] 1.6.3 --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index ace15d4e..6d46bf66 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "loopback-datasource-juggler", - "version": "1.6.2", + "version": "1.6.3", "description": "LoopBack DataSoure Juggler", "keywords": [ "StrongLoop", From b66183f7dc3ae415e6fb733945da5a0e39c17c07 Mon Sep 17 00:00:00 2001 From: Samuel Reed Date: Fri, 4 Jul 2014 16:40:08 -0500 Subject: [PATCH 09/23] DAO.prototype.exists should return 'boolean' type. Signed-off-by: Samuel Reed --- lib/dao.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/dao.js b/lib/dao.js index bc04d80b..50b9eef9 100644 --- a/lib/dao.js +++ b/lib/dao.js @@ -330,7 +330,7 @@ setRemoting(DataAccessObject.exists, { description: 'Check whether a model instance exists in the data source', accepts: {arg: 'id', type: 'any', description: 'Model id', required: true, http: {source: 'path'}}, - returns: {arg: 'exists', type: 'any'}, + returns: {arg: 'exists', type: 'boolean'}, http: {verb: 'get', path: '/:id/exists'} }); From a37129bdff9f6066e061f6ee3445819ef5503f5d Mon Sep 17 00:00:00 2001 From: Raymond Feng Date: Tue, 8 Jul 2014 14:04:20 -0700 Subject: [PATCH 10/23] Allows default model class to be configured --- lib/model-builder.js | 3 ++- test/loopback-dl.test.js | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 35 insertions(+), 1 deletion(-) diff --git a/lib/model-builder.js b/lib/model-builder.js index 89c98d1e..9398a34c 100644 --- a/lib/model-builder.js +++ b/lib/model-builder.js @@ -37,6 +37,7 @@ function ModelBuilder() { // create blank models pool this.models = {}; this.definitions = {}; + this.defaultModelBaseClass = DefaultModelBaseClass; } // Inherit from EventEmitter @@ -131,7 +132,7 @@ ModelBuilder.prototype.define = function defineClass(className, properties, sett } // Set up the base model class - var ModelBaseClass = parent || DefaultModelBaseClass; + var ModelBaseClass = parent || this.defaultModelBaseClass; var baseClass = settings.base || settings['super']; if (baseClass) { if (isModelClass(baseClass)) { diff --git a/test/loopback-dl.test.js b/test/loopback-dl.test.js index 3fa1ccd8..31eec3e7 100644 --- a/test/loopback-dl.test.js +++ b/test/loopback-dl.test.js @@ -1285,6 +1285,39 @@ describe('Load models from json', function () { } }); + it('should allow customization of default model base class', function () { + var modelBuilder = new ModelBuilder(); + + var User = modelBuilder.define('User', { + name: String, + bio: ModelBuilder.Text, + approved: Boolean, + joinedAt: Date, + age: Number + }); + + modelBuilder.defaultModelBaseClass = User; + + var Customer = modelBuilder.define('Customer', {customerId: {type: String, id: true}}); + assert(Customer.prototype instanceof User); + }); + + it('should allow model base class', function () { + var modelBuilder = new ModelBuilder(); + + var User = modelBuilder.define('User', { + name: String, + bio: ModelBuilder.Text, + approved: Boolean, + joinedAt: Date, + age: Number + }); + + var Customer = modelBuilder.define('Customer', + {customerId: {type: String, id: true}}, {}, User); + assert(Customer.prototype instanceof User); + }); + it('should be able to extend models', function (done) { var modelBuilder = new ModelBuilder(); From 6aaa3f216e12097261aec879d75ef7d83dd717c7 Mon Sep 17 00:00:00 2001 From: Fabien Franzen Date: Fri, 11 Jul 2014 15:29:47 +0200 Subject: [PATCH 11/23] RelationDefinition applyScope/applyMapping MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add the ability to set static/dynamic filtering (where, order, limit, skip, fields …) and property mapping/transfer (de-normalization) for hasMany/hasOne. --- lib/relation-definition.js | 128 +++++++++++++++++++++++++++---------- lib/scope.js | 1 + test/relations.test.js | 121 ++++++++++++++++++++++++++++++++++- 3 files changed, 213 insertions(+), 37 deletions(-) diff --git a/lib/relation-definition.js b/lib/relation-definition.js index c50cf0d6..18993ff8 100644 --- a/lib/relation-definition.js +++ b/lib/relation-definition.js @@ -5,6 +5,7 @@ var assert = require('assert'); var util = require('util'); var i8n = require('inflection'); var defineScope = require('./scope.js').defineScope; +var mergeQuery = require('./scope.js').mergeQuery; var ModelBaseClass = require('./model.js'); exports.Relation = Relation; @@ -68,6 +69,8 @@ function RelationDefinition(definition) { this.modelThrough = definition.modelThrough; this.keyThrough = definition.keyThrough; this.multiple = (this.type !== 'belongsTo' && this.type !== 'hasOne'); + this.mapping = definition.mapping || {}; + this.scope = definition.scope; } RelationDefinition.prototype.toJSON = function () { @@ -87,6 +90,41 @@ RelationDefinition.prototype.toJSON = function () { return json; }; +/** + * Apply the configured scope to the filter/query object. + * @param {Object} modelInstance + * @param {Object} filter (where, order, limit, fields, ...) + */ +RelationDefinition.prototype.applyScope = function(modelInstance, filter) { + if (typeof this.scope === 'function') { + var scope = this.scope.call(this, modelInstance, filter); + if (typeof scope === 'object') { + mergeQuery(filter, scope); + } + } else if (typeof this.scope === 'object') { + mergeQuery(filter, this.scope); + } +}; + +/** + * Apply the configured mapping to the target object. + * @param {Object} modelInstance + * @param {Object} target + */ +RelationDefinition.prototype.applyMapping = function(modelInstance, target) { + if (typeof this.mapping === 'function') { + var data = this.mapping.call(this, modelInstance); + for(var k in data) { + target[k] = data[k]; + } + } else if (typeof this.mapping === 'object') { + for(var k in this.mapping) { + var key = this.mapping[k]; + target[key] = modelInstance[k]; + } + } +}; + /** * A relation attaching to a given model instance * @param {RelationDefinition|Object} definition @@ -315,7 +353,7 @@ RelationDefinition.hasMany = function hasMany(modelFrom, modelTo, params) { var fk = params.foreignKey || i8n.camelize(thisClassName + '_id', true); var idName = modelFrom.dataSource.idName(modelFrom.modelName) || 'id'; - + var definition = new RelationDefinition({ name: relationName, type: RelationTypes.hasMany, @@ -323,9 +361,11 @@ RelationDefinition.hasMany = function hasMany(modelFrom, modelTo, params) { keyFrom: idName, keyTo: fk, modelTo: modelTo, - multiple: true + multiple: true, + mapping: params.mapping, + scope: params.scope }); - + if (params.through) { definition.modelThrough = params.through; var keyThrough = definition.throughKey || i8n.camelize(modelTo.modelName + '_id', true); @@ -352,12 +392,15 @@ RelationDefinition.hasMany = function hasMany(modelFrom, modelTo, params) { scopeMethods.create = scopeMethod(definition, 'create'); scopeMethods.build = scopeMethod(definition, 'build'); } - + // Mix the property and scoped methods into the prototype class defineScope(modelFrom.prototype, params.through || modelTo, relationName, function () { var filter = {}; filter.where = {}; filter.where[fk] = this[idName]; + + definition.applyScope(this, filter); + if (params.through) { filter.collect = i8n.camelize(modelTo.modelName, true); filter.include = filter.collect; @@ -384,42 +427,33 @@ HasMany.prototype.findById = function (id, cb) { var fk = this.definition.keyTo; var pk = this.definition.keyFrom; var modelInstance = this.modelInstance; - modelTo.findById(id, function (err, inst) { + var idName = this.definition.modelTo.definition.idName(); + var filter = {}; + filter.where = {}; + filter.where[idName] = id; + filter.where[fk] = modelInstance[pk]; + + this.definition.applyScope(modelInstance, filter); + + modelTo.findOne(filter, function (err, inst) { if (err) { return cb(err); } if (!inst) { return cb(new Error('Not found')); } - // Check if the foreign key matches the primary key - if (inst[fk] && inst[fk].toString() === modelInstance[pk].toString()) { - cb(null, inst); - } else { - cb(new Error('Permission denied: foreign key does not match the primary key')); - } + cb(null, inst); }); }; HasMany.prototype.destroyById = function (id, cb) { var self = this; - var modelTo = this.definition.modelTo; - var fk = this.definition.keyTo; - var pk = this.definition.keyFrom; - var modelInstance = this.modelInstance; - modelTo.findById(id, function (err, inst) { + this.findById(id, function(err, inst) { if (err) { return cb(err); } - if (!inst) { - return cb(new Error('Not found')); - } - // Check if the foreign key matches the primary key - if (inst[fk] && inst[fk].toString() === modelInstance[pk].toString()) { - self.removeFromCache(inst[fk]); - inst.destroy(cb); - } else { - cb(new Error('Permission denied: foreign key does not match the primary key')); - } + self.removeFromCache(inst[fk]); + inst.destroy(cb); }); }; @@ -451,6 +485,9 @@ HasManyThrough.prototype.create = function create(data, done) { var d = {}; d[fk1] = modelInstance[definition.keyFrom]; d[fk2] = to[pk2]; + + definition.applyMapping(modelInstance, d); + // Then create the through model modelThrough.create(d, function (e, through) { if (e) { @@ -486,15 +523,20 @@ HasManyThrough.prototype.add = function (acInst, done) { var pk2 = definition.modelTo.definition.idName(); var fk2 = findBelongsTo(modelThrough, definition.modelTo, pk2); - + query[fk1] = this.modelInstance[pk1]; query[fk2] = acInst[pk2] || acInst; + + var filter = { where: query }; + + definition.applyScope(this.modelInstance, filter); data[fk1] = this.modelInstance[pk1]; data[fk2] = acInst[pk2] || acInst; + definition.applyMapping(this.modelInstance, data); // Create an instance of the through model - modelThrough.findOrCreate({where: query}, data, function(err, ac) { + modelThrough.findOrCreate(filter, data, function(err, ac) { if(!err) { if (acInst instanceof definition.modelTo) { self.addToCache(acInst); @@ -526,8 +568,12 @@ HasManyThrough.prototype.remove = function (acInst, done) { query[fk1] = this.modelInstance[pk1]; query[fk2] = acInst[pk2] || acInst; + + var filter = { where: query }; + + definition.applyScope(this.modelInstance, filter); - modelThrough.deleteAll(query, function (err) { + modelThrough.deleteAll(filter.where, function (err) { if (!err) { self.removeFromCache(query[fk2]); } @@ -575,7 +621,7 @@ RelationDefinition.belongsTo = function (modelFrom, modelTo, params) { var idName = modelFrom.dataSource.idName(modelTo.modelName) || 'id'; var relationName = params.as || i8n.camelize(modelTo.modelName, true); var fk = params.foreignKey || relationName + 'Id'; - + var relationDef = modelFrom.relations[relationName] = new RelationDefinition({ name: relationName, type: RelationTypes.belongsTo, @@ -788,14 +834,15 @@ RelationDefinition.hasOne = function (modelFrom, modelTo, params) { var relationName = params.as || i8n.camelize(modelTo.modelName, true); var fk = params.foreignKey || i8n.camelize(modelFrom.modelName + '_id', true); - + var relationDef = modelFrom.relations[relationName] = new RelationDefinition({ name: relationName, type: RelationTypes.hasOne, modelFrom: modelFrom, keyFrom: pk, keyTo: fk, - modelTo: modelTo + modelTo: modelTo, + mapping: params.mapping }); modelFrom.dataSource.defineForeignKey(modelTo.modelName, fk, modelFrom.modelName); @@ -835,7 +882,11 @@ HasOne.prototype.create = function (targetModelData, cb) { targetModelData = targetModelData || {}; targetModelData[fk] = modelInstance[pk]; var query = {where: {}}; - query.where[fk] = targetModelData[fk] + query.where[fk] = targetModelData[fk]; + + this.definition.applyScope(modelInstance, query); + this.definition.applyMapping(modelInstance, targetModelData); + modelTo.findOne(query, function(err, result) { if(err) { cb(err); @@ -869,13 +920,16 @@ HasMany.prototype.create = function (targetModelData, cb) { var fk = this.definition.keyTo; var pk = this.definition.keyFrom; var modelInstance = this.modelInstance; - + if (typeof targetModelData === 'function' && !cb) { cb = targetModelData; targetModelData = {}; } targetModelData = targetModelData || {}; targetModelData[fk] = modelInstance[pk]; + + this.definition.applyMapping(modelInstance, targetModelData); + modelTo.create(targetModelData, function(err, targetModel) { if(!err) { // Refresh the cache @@ -895,8 +949,12 @@ HasMany.prototype.build = HasOne.prototype.build = function(targetModelData) { var modelTo = this.definition.modelTo; var pk = this.definition.keyFrom; var fk = this.definition.keyTo; + targetModelData = targetModelData || {}; targetModelData[fk] = this.modelInstance[pk]; + + this.definition.applyMapping(this.modelInstance, targetModelData); + return new modelTo(targetModelData); }; @@ -916,6 +974,7 @@ HasOne.prototype.related = function (refresh, params) { var modelTo = this.definition.modelTo; var fk = this.definition.keyTo; var pk = this.definition.keyFrom; + var definition = this.definition; var modelInstance = this.modelInstance; if (arguments.length === 1) { @@ -937,6 +996,7 @@ HasOne.prototype.related = function (refresh, params) { if (cachedValue === undefined) { var query = {where: {}}; query.where[fk] = modelInstance[pk]; + definition.applyScope(modelInstance, query); modelTo.findOne(query, function (err, inst) { if (err) { return cb(err); diff --git a/lib/scope.js b/lib/scope.js index 5267ce8d..9c94ffce 100644 --- a/lib/scope.js +++ b/lib/scope.js @@ -5,6 +5,7 @@ var defineCachedRelations = utils.defineCachedRelations; * Module exports */ exports.defineScope = defineScope; +exports.mergeQuery = mergeQuery; function ScopeDefinition(definition) { this.sourceModel = definition.sourceModel; diff --git a/test/relations.test.js b/test/relations.test.js index b8f34254..0ba08a3c 100644 --- a/test/relations.test.js +++ b/test/relations.test.js @@ -1,7 +1,8 @@ // This test written in mocha+should.js var should = require('./init.js'); -var db, Book, Chapter, Author, Reader, Publisher; +var db, Book, Chapter, Author, Reader; +var Category, Product; describe('relations', function () { before(function (done) { @@ -10,6 +11,8 @@ describe('relations', function () { Chapter = db.define('Chapter', {name: {type: String, index: true}}); Author = db.define('Author', {name: String}); Reader = db.define('Reader', {name: String}); + Category = db.define('Category', {name: String}); + Product = db.define('Product', {name: String, type: String}); db.automigrate(function () { Book.destroyAll(function () { @@ -25,7 +28,7 @@ describe('relations', function () { after(function () { // db.disconnect(); }); - + describe('hasMany', function () { it('can be declared in different ways', function (done) { Book.hasMany(Chapter); @@ -122,6 +125,117 @@ describe('relations', function () { should.equal(Book.prototype.chapters._targetClass, 'Chapter'); }); }); + + describe('hasMany with mapping', function () { + it('can be declared with mapping', function (done) { + Book.hasMany(Chapter, { mapping: { type: 'bookType' } }); + db.automigrate(done); + }); + + it('should create record on scope', function (done) { + Book.create({ type: 'fiction' }, function (err, book) { + book.chapters.create(function (err, c) { + should.not.exist(err); + should.exist(c); + c.bookId.should.equal(book.id); + c.bookType.should.equal('fiction'); + done(); + }); + }); + }); + }); + + describe('hasMany with scope', function () { + it('can be declared with mapping', function (done) { + Category.hasMany(Product, { + mapping: function(inst) { + if (!inst.productType) return; // skip + return { type: inst.productType }; + }, + scope: function(inst, filter) { + var m = this.mapping(inst); // re-use mapping + if (m) return { where: m }; + } + }); + db.automigrate(done); + }); + + it('should create record on scope', function (done) { + Category.create(function (err, c) { + c.products.create({ type: 'book' }, function(err, p) { + p.categoryId.should.equal(c.id); + p.type.should.equal('book'); + c.products.create({ type: 'widget' }, function(err, p) { + p.categoryId.should.equal(c.id); + p.type.should.equal('widget'); + done(); + }); + }); + }); + }); + + it('should find record on scope', function (done) { + Category.findOne(function (err, c) { + c.products(function(err, products) { + products.should.have.length(2); + done(); + }); + }); + }); + + it('should find record on scope - filtered', function (done) { + Category.findOne(function (err, c) { + c.products({ where: { type: 'book' } }, function(err, products) { + products.should.have.length(1); + products[0].type.should.equal('book'); + done(); + }); + }); + }); + + // So why not just do the above? In LoopBack, the context + // that gets passed into a beforeRemote handler contains + // a reference to the parent scope/instance: ctx.instance + // in order to enforce a (dynamic scope) at runtime + // a temporary property can be set in the beforeRemoting + // handler. Optionally, a dynamic mapping can be declared. + // + // The code below simulates this. + + it('should create record on scope - mapped', function (done) { + Category.findOne(function (err, c) { + c.productType = 'tool'; // temporary, for mapping + c.products.create(function(err, p) { + p.categoryId.should.equal(c.id); + p.type.should.equal('tool'); + done(); + }); + }); + }); + + it('should find record on scope - scoped', function (done) { + Category.findOne(function (err, c) { + c.productType = 'book'; // temporary, for scoping + c.products(function(err, products) { + products.should.have.length(1); + products[0].type.should.equal('book'); + done(); + }); + }); + }); + + it('should find record on scope - scoped', function (done) { + Category.findOne(function (err, c) { + c.productType = 'tool'; // temporary, for scoping + c.products(function(err, products) { + products.should.have.length(1); + products[0].type.should.equal('tool'); + done(); + }); + }); + }); + + }); describe('belongsTo', function () { var List, Item, Fear, Mind; @@ -190,7 +304,7 @@ describe('relations', function () { }); it('can be declared using hasOne method', function () { - Supplier.hasOne(Account); + Supplier.hasOne(Account, { mapping: { name: 'supplierName' } }); Object.keys((new Account()).toObject()).should.include('supplierId'); (new Supplier()).account.should.be.an.instanceOf(Function); }); @@ -207,6 +321,7 @@ describe('relations', function () { should.exist(act); act.should.be.an.instanceOf(Account); supplier.account().id.should.equal(act.id); + act.supplierName.should.equal(supplier.name); done(); }); }); From 5f1431aa0557553c4fa6d96b5b8f840b5fda8900 Mon Sep 17 00:00:00 2001 From: Fabien Franzen Date: Fri, 11 Jul 2014 22:07:57 +0200 Subject: [PATCH 12/23] Don't check uniqueness of blank values --- lib/validations.js | 1 + test/validations.test.js | 15 +++++++++++++++ 2 files changed, 16 insertions(+) diff --git a/lib/validations.js b/lib/validations.js index a12a8127..0b9388e7 100644 --- a/lib/validations.js +++ b/lib/validations.js @@ -305,6 +305,7 @@ function validateCustom(attr, conf, err, done) { * Uniqueness validator */ function validateUniqueness(attr, conf, err, done) { + if (blank(this[attr])) return done(); var cond = {where: {}}; cond.where[attr] = this[attr]; diff --git a/test/validations.test.js b/test/validations.test.js index c3b5c8d4..a5d52ae6 100644 --- a/test/validations.test.js +++ b/test/validations.test.js @@ -227,6 +227,21 @@ describe('validations', function () { done(err); }); }); + + it('should skip blank values', function (done) { + User.validatesUniquenessOf('email'); + var u = new User({email: ' '}); + Boolean(u.isValid(function (valid) { + valid.should.be.true; + u.save(function () { + var u2 = new User({email: null}); + u2.isValid(function (valid) { + valid.should.be.true; + done(); + }); + }); + })).should.be.false; + }); }); describe('format', function () { From a58dbe3a54896eb0ab2247e1612aaad05599321e Mon Sep 17 00:00:00 2001 From: Fabien Franzen Date: Fri, 11 Jul 2014 22:56:02 +0200 Subject: [PATCH 13/23] More validations and tests Added validatesAbsenceOf. Handle async with if/unless prevention of validators correctly. See: #170 #158 --- lib/validations.js | 34 ++++++++++++++++++++-- test/validations.test.js | 61 ++++++++++++++++++++++++++++++++++++++-- 2 files changed, 90 insertions(+), 5 deletions(-) diff --git a/lib/validations.js b/lib/validations.js index 0b9388e7..bcf6a805 100644 --- a/lib/validations.js +++ b/lib/validations.js @@ -46,6 +46,20 @@ function Validatable() { */ Validatable.validatesPresenceOf = getConfigurator('presence'); +/** + * Validate absence of one or more specified properties. + * A model should not include a property to be considered valid; fails when validated field not blank. + * + * For example, validate absence of reserved + * ``` + * Post.validatesAbsenceOf('reserved', { unless: 'special' }); + * + * @param {String} propertyName One or more property names. + * @options {Object} errMsg Optional custom error message. Default is "can't be set" + * @property {String} message Error message to use instead of default. + */ +Validatable.validatesAbsenceOf = getConfigurator('absence'); + /** * Validate length. Require a property length to be within a specified range. * Three kinds of validations: min, max, is. @@ -225,6 +239,15 @@ function validatePresence(attr, conf, err) { } } +/*! + * Absence validator + */ +function validateAbsence(attr, conf, err) { + if (!blank(this[attr])) { + err(); + } +} + /*! * Length validator */ @@ -332,6 +355,7 @@ function validateUniqueness(attr, conf, err, done) { var validators = { presence: validatePresence, + absence: validateAbsence, length: validateLength, numericality: validateNumericality, inclusion: validateInclusion, @@ -470,8 +494,11 @@ function validationFailed(inst, v, cb) { // here we should check skip validation conditions (if, unless) // that can be specified in conf - if (skipValidation(inst, conf, 'if')) return false; - if (skipValidation(inst, conf, 'unless')) return false; + if (skipValidation(inst, conf, 'if') + || skipValidation(inst, conf, 'unless')) { + if (cb) cb(true); + return false; + } var fail = false; var validator = validators[conf.validation]; @@ -500,7 +527,7 @@ function validationFailed(inst, v, cb) { message = 'is invalid'; } } - inst.errors.add(attr, message, code); + if (kind !== false) inst.errors.add(attr, message, code); fail = true; }); if (cb) { @@ -533,6 +560,7 @@ function skipValidation(inst, conf, kind) { var defaultMessages = { presence: 'can\'t be blank', + absence: 'can\'t be set', length: { min: 'too short', max: 'too long', diff --git a/test/validations.test.js b/test/validations.test.js index a5d52ae6..1a3faf55 100644 --- a/test/validations.test.js +++ b/test/validations.test.js @@ -159,6 +159,20 @@ describe('validations', function () { }); }); + + describe('absence', function () { + + it('should validate absence', function () { + User.validatesAbsenceOf('reserved', { if: 'locked' }); + var u = new User({reserved: 'foo', locked: true}); + u.isValid().should.not.be.true; + u.reserved = null; + u.isValid().should.be.true; + var u = new User({reserved: 'foo', locked: false}); + u.isValid().should.be.true; + }); + + }); describe('uniqueness', function () { it('should validate uniqueness', function (done) { @@ -242,6 +256,18 @@ describe('validations', function () { }); })).should.be.false; }); + + it('should work with if/unless', function (done) { + User.validatesUniquenessOf('email', { + if: function() { return true; }, + unless: function() { return false; } + }); + var u = new User({email: 'hello'}); + Boolean(u.isValid(function (valid) { + valid.should.be.true; + done(); + })).should.be.false; + }); }); describe('format', function () { @@ -266,7 +292,38 @@ describe('validations', function () { }); describe('custom', function () { - it('should validate using custom sync validation'); - it('should validate using custom async validation'); + it('should validate using custom sync validation', function() { + User.validate('email', function (err) { + if (this.email === 'hello') err(); + }); + var u = new User({email: 'hello'}); + Boolean(u.isValid()).should.be.false; + }); + + it('should validate and return detailed error messages', function() { + User.validate('global', function (err) { + if (this.email === 'hello' || this.email === 'hey') { + this.errors.add('hello', 'Cannot be `' + this.email + '`', 'invalid'); + err(false); // false: prevent global error message + } + }); + var u = new User({email: 'hello'}); + Boolean(u.isValid()).should.be.false; + u.errors.should.eql({ hello: [ 'Cannot be `hello`' ] }); + }); + + it('should validate using custom async validation', function(done) { + User.validateAsync('email', function (err, next) { + process.nextTick(next); + }, { + if: function() { return true; }, + unless: function() { return false; } + }); + var u = new User({email: 'hello'}); + Boolean(u.isValid(function (valid) { + valid.should.be.true; + done(); + })).should.be.false; + }); }); }); From 50656b82067ed5551ead93acc3790dc5d0919e39 Mon Sep 17 00:00:00 2001 From: Fabien Franzen Date: Fri, 11 Jul 2014 23:03:07 +0200 Subject: [PATCH 14/23] Handle custom error codes Fixes #151 --- lib/validations.js | 2 +- test/validations.test.js | 8 +++++--- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/lib/validations.js b/lib/validations.js index bcf6a805..9cd368f9 100644 --- a/lib/validations.js +++ b/lib/validations.js @@ -506,7 +506,7 @@ function validationFailed(inst, v, cb) { validatorArguments.push(attr); validatorArguments.push(conf); validatorArguments.push(function onerror(kind) { - var message, code = conf.validation; + var message, code = conf.code || conf.validation; if (conf.message) { message = conf.message; } diff --git a/test/validations.test.js b/test/validations.test.js index 1a3faf55..935a2669 100644 --- a/test/validations.test.js +++ b/test/validations.test.js @@ -295,21 +295,23 @@ describe('validations', function () { it('should validate using custom sync validation', function() { User.validate('email', function (err) { if (this.email === 'hello') err(); - }); + }, { code: 'invalid-email' }); var u = new User({email: 'hello'}); Boolean(u.isValid()).should.be.false; + u.errors.codes.should.eql({ email: ['invalid-email'] }); }); it('should validate and return detailed error messages', function() { User.validate('global', function (err) { if (this.email === 'hello' || this.email === 'hey') { - this.errors.add('hello', 'Cannot be `' + this.email + '`', 'invalid'); + this.errors.add('email', 'Cannot be `' + this.email + '`', 'invalid-email'); err(false); // false: prevent global error message } }); var u = new User({email: 'hello'}); Boolean(u.isValid()).should.be.false; - u.errors.should.eql({ hello: [ 'Cannot be `hello`' ] }); + u.errors.should.eql({ email: ['Cannot be `hello`'] }); + u.errors.codes.should.eql({ email: ['invalid-email'] }); }); it('should validate using custom async validation', function(done) { From 61f6d49518f81e17804a148a0895f234e00c37e1 Mon Sep 17 00:00:00 2001 From: Fabien Franzen Date: Fri, 11 Jul 2014 23:55:15 +0200 Subject: [PATCH 15/23] Fix validateUniqueness/nextTick --- lib/validations.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/validations.js b/lib/validations.js index 9cd368f9..6998f328 100644 --- a/lib/validations.js +++ b/lib/validations.js @@ -328,7 +328,9 @@ function validateCustom(attr, conf, err, done) { * Uniqueness validator */ function validateUniqueness(attr, conf, err, done) { - if (blank(this[attr])) return done(); + if (blank(this[attr])) { + return process.nextTick(done); + } var cond = {where: {}}; cond.where[attr] = this[attr]; From 48c4f25b09ba75c607a7d0dc5ee642d8e233be64 Mon Sep 17 00:00:00 2001 From: Fabien Franzen Date: Sat, 12 Jul 2014 00:02:16 +0200 Subject: [PATCH 16/23] Renamed mapping to properties --- lib/relation-definition.js | 33 ++++++++++++++++----------------- test/relations.test.js | 20 ++++++++++---------- 2 files changed, 26 insertions(+), 27 deletions(-) diff --git a/lib/relation-definition.js b/lib/relation-definition.js index 18993ff8..704bcfaf 100644 --- a/lib/relation-definition.js +++ b/lib/relation-definition.js @@ -69,7 +69,7 @@ function RelationDefinition(definition) { this.modelThrough = definition.modelThrough; this.keyThrough = definition.keyThrough; this.multiple = (this.type !== 'belongsTo' && this.type !== 'hasOne'); - this.mapping = definition.mapping || {}; + this.properties = definition.properties || {}; this.scope = definition.scope; } @@ -107,19 +107,19 @@ RelationDefinition.prototype.applyScope = function(modelInstance, filter) { }; /** - * Apply the configured mapping to the target object. + * Apply the configured properties to the target object. * @param {Object} modelInstance * @param {Object} target */ -RelationDefinition.prototype.applyMapping = function(modelInstance, target) { - if (typeof this.mapping === 'function') { - var data = this.mapping.call(this, modelInstance); +RelationDefinition.prototype.applyProperties = function(modelInstance, target) { + if (typeof this.properties === 'function') { + var data = this.properties.call(this, modelInstance); for(var k in data) { target[k] = data[k]; } - } else if (typeof this.mapping === 'object') { - for(var k in this.mapping) { - var key = this.mapping[k]; + } else if (typeof this.properties === 'object') { + for(var k in this.properties) { + var key = this.properties[k]; target[key] = modelInstance[k]; } } @@ -362,7 +362,7 @@ RelationDefinition.hasMany = function hasMany(modelFrom, modelTo, params) { keyTo: fk, modelTo: modelTo, multiple: true, - mapping: params.mapping, + properties: params.properties, scope: params.scope }); @@ -486,7 +486,7 @@ HasManyThrough.prototype.create = function create(data, done) { d[fk1] = modelInstance[definition.keyFrom]; d[fk2] = to[pk2]; - definition.applyMapping(modelInstance, d); + definition.applyProperties(modelInstance, d); // Then create the through model modelThrough.create(d, function (e, through) { @@ -533,7 +533,7 @@ HasManyThrough.prototype.add = function (acInst, done) { data[fk1] = this.modelInstance[pk1]; data[fk2] = acInst[pk2] || acInst; - definition.applyMapping(this.modelInstance, data); + definition.applyProperties(this.modelInstance, data); // Create an instance of the through model modelThrough.findOrCreate(filter, data, function(err, ac) { @@ -662,7 +662,6 @@ RelationDefinition.belongsTo = function (modelFrom, modelTo, params) { fn.returns = {arg: relationName, type: 'object', root: true}; modelFrom.prototype['__get__' + relationName] = fn; - }; BelongsTo.prototype.create = function(targetModelData, cb) { @@ -710,7 +709,7 @@ BelongsTo.prototype.related = function (refresh, params) { var pk = this.definition.keyTo; var fk = this.definition.keyFrom; var modelInstance = this.modelInstance; - + if (arguments.length === 1) { params = refresh; refresh = false; @@ -842,7 +841,7 @@ RelationDefinition.hasOne = function (modelFrom, modelTo, params) { keyFrom: pk, keyTo: fk, modelTo: modelTo, - mapping: params.mapping + properties: params.properties }); modelFrom.dataSource.defineForeignKey(modelTo.modelName, fk, modelFrom.modelName); @@ -885,7 +884,7 @@ HasOne.prototype.create = function (targetModelData, cb) { query.where[fk] = targetModelData[fk]; this.definition.applyScope(modelInstance, query); - this.definition.applyMapping(modelInstance, targetModelData); + this.definition.applyProperties(modelInstance, targetModelData); modelTo.findOne(query, function(err, result) { if(err) { @@ -928,7 +927,7 @@ HasMany.prototype.create = function (targetModelData, cb) { targetModelData = targetModelData || {}; targetModelData[fk] = modelInstance[pk]; - this.definition.applyMapping(modelInstance, targetModelData); + this.definition.applyProperties(modelInstance, targetModelData); modelTo.create(targetModelData, function(err, targetModel) { if(!err) { @@ -953,7 +952,7 @@ HasMany.prototype.build = HasOne.prototype.build = function(targetModelData) { targetModelData = targetModelData || {}; targetModelData[fk] = this.modelInstance[pk]; - this.definition.applyMapping(this.modelInstance, targetModelData); + this.definition.applyProperties(this.modelInstance, targetModelData); return new modelTo(targetModelData); }; diff --git a/test/relations.test.js b/test/relations.test.js index 0ba08a3c..2fae2b9b 100644 --- a/test/relations.test.js +++ b/test/relations.test.js @@ -126,9 +126,9 @@ describe('relations', function () { }); }); - describe('hasMany with mapping', function () { - it('can be declared with mapping', function (done) { - Book.hasMany(Chapter, { mapping: { type: 'bookType' } }); + describe('hasMany with properties', function () { + it('can be declared with properties', function (done) { + Book.hasMany(Chapter, { properties: { type: 'bookType' } }); db.automigrate(done); }); @@ -146,14 +146,14 @@ describe('relations', function () { }); describe('hasMany with scope', function () { - it('can be declared with mapping', function (done) { + it('can be declared with properties', function (done) { Category.hasMany(Product, { - mapping: function(inst) { + properties: function(inst) { if (!inst.productType) return; // skip return { type: inst.productType }; }, scope: function(inst, filter) { - var m = this.mapping(inst); // re-use mapping + var m = this.properties(inst); // re-use properties if (m) return { where: m }; } }); @@ -198,13 +198,13 @@ describe('relations', function () { // a reference to the parent scope/instance: ctx.instance // in order to enforce a (dynamic scope) at runtime // a temporary property can be set in the beforeRemoting - // handler. Optionally, a dynamic mapping can be declared. + // handler. Optionally,properties dynamic properties can be declared. // // The code below simulates this. - it('should create record on scope - mapped', function (done) { + it('should create record on scope - properties', function (done) { Category.findOne(function (err, c) { - c.productType = 'tool'; // temporary, for mapping + c.productType = 'tool'; // temporary c.products.create(function(err, p) { p.categoryId.should.equal(c.id); p.type.should.equal('tool'); @@ -304,7 +304,7 @@ describe('relations', function () { }); it('can be declared using hasOne method', function () { - Supplier.hasOne(Account, { mapping: { name: 'supplierName' } }); + Supplier.hasOne(Account, { properties: { name: 'supplierName' } }); Object.keys((new Account()).toObject()).should.include('supplierId'); (new Supplier()).account.should.be.an.instanceOf(Function); }); From 03617b58df812ad90c85732c4690d737e45dc669 Mon Sep 17 00:00:00 2001 From: Fabien Franzen Date: Sat, 12 Jul 2014 00:24:07 +0200 Subject: [PATCH 17/23] Sign-off Signed-off-by: Fabien Franzen --- test/relations.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/relations.test.js b/test/relations.test.js index 2fae2b9b..9d23ee3b 100644 --- a/test/relations.test.js +++ b/test/relations.test.js @@ -28,7 +28,7 @@ describe('relations', function () { after(function () { // db.disconnect(); }); - + describe('hasMany', function () { it('can be declared in different ways', function (done) { Book.hasMany(Chapter); From ae9c7f8cac44f81d6ba816d6ac2ba0ef7628bb80 Mon Sep 17 00:00:00 2001 From: Raymond Feng Date: Mon, 14 Jul 2014 08:56:33 -0700 Subject: [PATCH 18/23] Fix the error message --- lib/datasource.js | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/lib/datasource.js b/lib/datasource.js index 6640768e..8b5d51ab 100644 --- a/lib/datasource.js +++ b/lib/datasource.js @@ -221,10 +221,9 @@ DataSource._resolveConnector = function (name, loader) { var connector = tryModules(names, loader); var error = null; if (!connector) { - error = '\nWARNING: LoopBack connector "' + name - + '" is not installed at any of the locations ' + names - + '. To fix, run:\n\n npm install ' - + name + '\n'; + error = util.format('\nWARNING: LoopBack connector "%s" is not installed ' + + 'as any of the following modules:\n\n %s\n\nTo fix, run:\n\n npm install %s\n', + name, names.join('\n'), names[names.length -1]); } return { connector: connector, From f3dbc6ca5f71378a8f99d61306ef2e812349ee23 Mon Sep 17 00:00:00 2001 From: Raymond Feng Date: Tue, 15 Jul 2014 08:50:34 -0700 Subject: [PATCH 19/23] Remoting methods for hasMany through --- lib/relation-definition.js | 283 ++++++++++++++++++++++++++++++++++--- test/relations.test.js | 271 ++++++++++++++++++++++++++++++++++- 2 files changed, 534 insertions(+), 20 deletions(-) diff --git a/lib/relation-definition.js b/lib/relation-definition.js index 704bcfaf..397c41b2 100644 --- a/lib/relation-definition.js +++ b/lib/relation-definition.js @@ -381,13 +381,86 @@ RelationDefinition.hasMany = function hasMany(modelFrom, modelTo, params) { var scopeMethods = { findById: scopeMethod(definition, 'findById'), - destroy: scopeMethod(definition, 'destroyById') - } + destroy: scopeMethod(definition, 'destroyById'), + updateById: scopeMethod(definition, 'updateById'), + exists: scopeMethod(definition, 'exists') + }; + + var findByIdFunc = scopeMethods.findById; + findByIdFunc.shared = true; + findByIdFunc.http = {verb: 'get', path: '/' + relationName + '/:fk'}; + findByIdFunc.accepts = {arg: 'fk', type: 'any', + description: 'Foreign key for ' + relationName, required: true, + http: {source: 'path'}}; + findByIdFunc.description = 'Find a related item by id for ' + relationName; + findByIdFunc.returns = {arg: 'result', type: 'object', root: true}; + + modelFrom.prototype['__findById__' + relationName] = findByIdFunc; + + var destroyByIdFunc = scopeMethods.destroy; + destroyByIdFunc.shared = true; + destroyByIdFunc.http = {verb: 'delete', path: '/' + relationName + '/:fk'}; + destroyByIdFunc.accepts = {arg: 'fk', type: 'any', + description: 'Foreign key for ' + relationName, required: true, + http: {source: 'path'}}; + destroyByIdFunc.description = 'Delete a related item by id for ' + relationName; + destroyByIdFunc.returns = {}; + + modelFrom.prototype['__destroyById__' + relationName] = destroyByIdFunc; + + var updateByIdFunc = scopeMethods.updateById; + updateByIdFunc.shared = true; + updateByIdFunc.http = {verb: 'put', path: '/' + relationName + '/:fk'}; + updateByIdFunc.accepts = {arg: 'fk', type: 'any', + description: 'Foreign key for ' + relationName, required: true, + http: {source: 'path'}}; + updateByIdFunc.description = 'Update a related item by id for ' + relationName; + updateByIdFunc.returns = {arg: 'result', type: 'object', root: true}; + + modelFrom.prototype['__updateById__' + relationName] = updateByIdFunc; if(definition.modelThrough) { scopeMethods.create = scopeMethod(definition, 'create'); scopeMethods.add = scopeMethod(definition, 'add'); scopeMethods.remove = scopeMethod(definition, 'remove'); + + var addFunc = scopeMethods.add; + addFunc.shared = true; + addFunc.http = {verb: 'put', path: '/' + relationName + '/rel/:fk'}; + addFunc.accepts = {arg: 'fk', type: 'any', + description: 'Foreign key for ' + relationName, required: true, + http: {source: 'path'}}; + addFunc.description = 'Add a related item by id for ' + relationName; + addFunc.returns = {arg: relationName, type: 'object', root: true}; + + modelFrom.prototype['__link__' + relationName] = addFunc; + + var removeFunc = scopeMethods.remove; + removeFunc.shared = true; + removeFunc.http = {verb: 'delete', path: '/' + relationName + '/rel/:fk'}; + removeFunc.accepts = {arg: 'fk', type: 'any', + description: 'Foreign key for ' + relationName, required: true, + http: {source: 'path'}}; + removeFunc.description = 'Remove the ' + relationName + ' relation to an item by id'; + removeFunc.returns = {}; + + modelFrom.prototype['__unlink__' + relationName] = removeFunc; + + // FIXME: [rfeng] How to map a function with callback(err, true|false) to HEAD? + // true --> 200 and false --> 404? + /* + var existsFunc = scopeMethods.exists; + existsFunc.shared = true; + existsFunc.http = {verb: 'head', path: '/' + relationName + '/rel/:fk'}; + existsFunc.accepts = {arg: 'fk', type: 'any', + description: 'Foreign key for ' + relationName, required: true, + http: {source: 'path'}}; + existsFunc.description = 'Check the existence of ' + relationName + ' relation to an item by id'; + existsFunc.returns = {}; + + modelFrom.prototype['__exists__' + relationName] = existsFunc; + */ + } else { scopeMethods.create = scopeMethod(definition, 'create'); scopeMethods.build = scopeMethod(definition, 'build'); @@ -411,26 +484,41 @@ RelationDefinition.hasMany = function hasMany(modelFrom, modelTo, params) { }; function scopeMethod(definition, methodName) { + var relationClass = RelationClasses[definition.type]; + if (definition.type === RelationTypes.hasMany && definition.modelThrough) { + relationClass = RelationClasses.hasManyThrough; + } var method = function () { - var relationClass = RelationClasses[definition.type]; - if (definition.type === RelationTypes.hasMany && definition.modelThrough) { - relationClass = RelationClasses.hasManyThrough; - } var relation = new relationClass(definition, this); return relation[methodName].apply(relation, arguments); }; + + var relationMethod = relationClass.prototype[methodName]; + if (relationMethod.shared) { + method.shared = true; + method.accepts = relationMethod.accepts; + method.returns = relationMethod.returns; + method.http = relationMethod.http; + method.description = relationMethod.description; + } return method; } -HasMany.prototype.findById = function (id, cb) { +/** + * Find a related item by foreign key + * @param {*} fkId The foreign key + * @param {Function} cb The callback function + */ +HasMany.prototype.findById = function (fkId, cb) { var modelTo = this.definition.modelTo; var fk = this.definition.keyTo; var pk = this.definition.keyFrom; var modelInstance = this.modelInstance; + var idName = this.definition.modelTo.definition.idName(); var filter = {}; filter.where = {}; - filter.where[idName] = id; + filter.where[idName] = fkId; filter.where[fk] = modelInstance[pk]; this.definition.applyScope(modelInstance, filter); @@ -440,23 +528,147 @@ HasMany.prototype.findById = function (id, cb) { return cb(err); } if (!inst) { - return cb(new Error('Not found')); + err = new Error('No instance with id ' + fkId + ' found for ' + modelTo.modelName); + err.statusCode = 404; + return cb(err); + } + // Check if the foreign key matches the primary key + if (inst[fk] && inst[fk].toString() === modelInstance[pk].toString()) { + cb(null, inst); + } else { + err = new Error('Key mismatch: ' + this.definition.modelFrom.modelName + '.' + pk + + ': ' + modelInstance[pk] + + ', ' + modelTo.modelName + '.' + fk + ': ' + inst[fk]); + err.statusCode = 400; + cb(err); } - cb(null, inst); }); }; -HasMany.prototype.destroyById = function (id, cb) { - var self = this; - this.findById(id, function(err, inst) { +/** + * Find a related item by foreign key + * @param {*} fkId The foreign key + * @param {Function} cb The callback function + */ +HasMany.prototype.exists = function (fkId, cb) { + var modelTo = this.definition.modelTo; + var fk = this.definition.keyTo; + var pk = this.definition.keyFrom; + var modelInstance = this.modelInstance; + + modelTo.findById(fkId, function (err, inst) { if (err) { return cb(err); } - self.removeFromCache(inst[fk]); + if (!inst) { + return cb(null, false); + } + // Check if the foreign key matches the primary key + if (inst[fk] && inst[fk].toString() === modelInstance[pk].toString()) { + cb(null, true); + } else { + cb(null, false); + } + }); +}; + +/** + * Update a related item by foreign key + * @param {*} fkId The foreign key + * @param {Function} cb The callback function + */ +HasMany.prototype.updateById = function (fkId, data, cb) { + this.findById(fkId, function (err, inst) { + if (err) { + return cb && cb(err); + } + inst.updateAttributes(data, cb); + }); +}; + +/** + * Delete a related item by foreign key + * @param {*} fkId The foreign key + * @param {Function} cb The callback function + */ +HasMany.prototype.destroyById = function (fkId, cb) { + var self = this; + this.findById(fkId, function(err, inst) { + if (err) { + return cb(err); + } + self.removeFromCache(inst[fkId]); inst.destroy(cb); }); }; +/** + * Find a related item by foreign key + * @param {*} fkId The foreign key value + * @param {Function} cb The callback function + */ +HasManyThrough.prototype.findById = function (fkId, cb) { + var self = this; + var modelTo = this.definition.modelTo; + var pk = this.definition.keyFrom; + var modelInstance = this.modelInstance; + var modelThrough = this.definition.modelThrough; + + self.exists(fkId, function (err, exists) { + if (err || !exists) { + if (!err) { + err = new Error('No relation found in ' + modelThrough.modelName + + ' for (' + self.definition.modelFrom.modelName + '.' + modelInstance[pk] + + ',' + modelTo.modelName + '.' + fkId + ')'); + err.statusCode = 404; + } + return cb(err); + } + modelTo.findById(fkId, function (err, inst) { + if (err) { + return cb(err); + } + if (!inst) { + err = new Error('No instance with id ' + fkId + ' found for ' + modelTo.modelName); + err.statusCode = 404; + return cb(err); + } + cb(err, inst); + }); + }); +}; + +/** + * Delete a related item by foreign key + * @param {*} fkId The foreign key + * @param {Function} cb The callback function + */ +HasManyThrough.prototype.destroyById = function (fkId, cb) { + var self = this; + var modelTo = this.definition.modelTo; + var pk = this.definition.keyFrom; + var modelInstance = this.modelInstance; + var modelThrough = this.definition.modelThrough; + + self.exists(fkId, function (err, exists) { + if (err || !exists) { + if (!err) { + err = new Error('No record found in ' + modelThrough.modelName + + ' for (' + self.definition.modelFrom.modelName + '.' + modelInstance[pk] + + ' ,' + modelTo.modelName + '.' + fkId + ')'); + err.statusCode = 404; + } + return cb(err); + } + self.remove(fkId, function(err) { + if(err) { + return cb(err); + } + modelTo.deleteById(fkId, cb); + }); + }); +}; + // Create an instance of the target model and connect it to the instance of // the source model by creating an instance of the through model HasManyThrough.prototype.create = function create(data, done) { @@ -546,6 +758,37 @@ HasManyThrough.prototype.add = function (acInst, done) { }); }; +/** + * Check if the target model instance is related to the 'hasMany' relation + * @param {Object|ID} acInst The actual instance or id value + */ +HasManyThrough.prototype.exists = function (acInst, done) { + var definition = this.definition; + var modelThrough = definition.modelThrough; + var pk1 = definition.keyFrom; + + var data = {}; + var query = {}; + + var fk1 = findBelongsTo(modelThrough, definition.modelFrom, + definition.keyFrom); + + // The primary key for the target model + var pk2 = definition.modelTo.definition.idName(); + + var fk2 = findBelongsTo(modelThrough, definition.modelTo, pk2); + + query[fk1] = this.modelInstance[pk1]; + query[fk2] = acInst[pk2] || acInst; + + data[fk1] = this.modelInstance[pk1]; + data[fk2] = acInst[pk2] || acInst; + + modelThrough.count(query, function(err, ac) { + done(err, ac > 0); + }); +}; + /** * Remove the target model instance from the 'hasMany' relation * @param {Object|ID) acInst The actual instance or id value @@ -739,7 +982,11 @@ BelongsTo.prototype.related = function (refresh, params) { self.resetCache(inst); cb(null, inst); } else { - cb(new Error('Permission denied: foreign key does not match the primary key')); + err = new Error('Key mismatch: ' + self.definition.modelFrom.modelName + '.' + fk + + ': ' + modelInstance[fk] + + ', ' + modelTo.modelName + '.' + pk + ': ' + inst[pk]); + err.statusCode = 400; + cb(err); } }); return modelInstance[fk]; @@ -1008,7 +1255,11 @@ HasOne.prototype.related = function (refresh, params) { self.resetCache(inst); cb(null, inst); } else { - cb(new Error('Permission denied')); + err = new Error('Key mismatch: ' + self.definition.modelFrom.modelName + '.' + pk + + ': ' + modelInstance[pk] + + ', ' + modelTo.modelName + '.' + fk + ': ' + inst[fk]); + err.statusCode = 400; + cb(err); } }); return modelInstance[pk]; diff --git a/test/relations.test.js b/test/relations.test.js index 9d23ee3b..8c6580d0 100644 --- a/test/relations.test.js +++ b/test/relations.test.js @@ -76,12 +76,12 @@ describe('relations', function () { book.chapters.create({name: 'a'}, function () { book.chapters.create({name: 'z'}, function () { book.chapters.create({name: 'c'}, function () { - fetch(book); + verify(book); }); }); }); }); - function fetch(book) { + function verify(book) { book.chapters(function (err, ch) { should.not.exist(err); should.exist(ch); @@ -105,13 +105,13 @@ describe('relations', function () { id = ch.id; book.chapters.create({name: 'z'}, function () { book.chapters.create({name: 'c'}, function () { - fetch(book); + verify(book); }); }); }); }); - function fetch(book) { + function verify(book) { book.chapters.findById(id, function (err, ch) { should.not.exist(err); should.exist(ch); @@ -124,6 +124,269 @@ describe('relations', function () { it('should set targetClass on scope property', function() { should.equal(Book.prototype.chapters._targetClass, 'Chapter'); }); + + it('should update scoped record', function (done) { + var id; + Book.create(function (err, book) { + book.chapters.create({name: 'a'}, function (err, ch) { + id = ch.id; + book.chapters.updateById(id, {name: 'aa'}, function(err, ch) { + verify(book); + }); + }); + }); + + function verify(book) { + book.chapters.findById(id, function (err, ch) { + should.not.exist(err); + should.exist(ch); + ch.id.should.equal(id); + ch.name.should.equal('aa'); + done(); + }); + } + }); + + it('should destroy scoped record', function (done) { + var id; + Book.create(function (err, book) { + book.chapters.create({name: 'a'}, function (err, ch) { + id = ch.id; + book.chapters.destroy(id, function(err, ch) { + verify(book); + }); + }); + }); + + function verify(book) { + book.chapters.findById(id, function (err, ch) { + should.exist(err); + done(); + }); + } + }); + + it('should check existence of a scoped record', function (done) { + var id; + Book.create(function (err, book) { + book.chapters.create({name: 'a'}, function (err, ch) { + id = ch.id; + book.chapters.create({name: 'z'}, function () { + book.chapters.create({name: 'c'}, function () { + verify(book); + }); + }); + }); + }); + + function verify(book) { + book.chapters.exists(id, function (err, flag) { + should.not.exist(err); + flag.should.be.eql(true); + done(); + }); + } + }); + }); + + describe('hasMany through', function () { + var Physician, Patient, Appointment; + + before(function (done) { + db = getSchema(); + Physician = db.define('Physician', {name: String}); + Patient = db.define('Patient', {name: String}); + Appointment = db.define('Appointment', {date: {type: Date, + default: function () { + return new Date(); + }}}); + + Physician.hasMany(Patient, {through: Appointment}); + Patient.hasMany(Physician, {through: Appointment}); + Appointment.belongsTo(Patient); + Appointment.belongsTo(Physician); + + db.automigrate(['Physician', 'Patient', 'Appointment'], function (err) { + done(err); + }); + }); + + it('should build record on scope', function (done) { + Physician.create(function (err, physician) { + var patient = physician.patients.build(); + patient.physicianId.should.equal(physician.id); + patient.save(done); + }); + }); + + it('should create record on scope', function (done) { + Physician.create(function (err, physician) { + physician.patients.create(function (err, patient) { + should.not.exist(err); + should.exist(patient); + Appointment.find({where: {physicianId: physician.id, patientId: patient.id}}, + function(err, apps) { + should.not.exist(err); + apps.should.have.lengthOf(1); + done(); + }); + }); + }); + }); + + it('should fetch all scoped instances', function (done) { + Physician.create(function (err, physician) { + physician.patients.create({name: 'a'}, function () { + physician.patients.create({name: 'z'}, function () { + physician.patients.create({name: 'c'}, function () { + verify(physician); + }); + }); + }); + }); + function verify(physician) { + physician.patients(function (err, ch) { + should.not.exist(err); + should.exist(ch); + ch.should.have.lengthOf(3); + done(); + }); + } + }); + + it('should find scoped record', function (done) { + var id; + Physician.create(function (err, physician) { + physician.patients.create({name: 'a'}, function (err, ch) { + id = ch.id; + physician.patients.create({name: 'z'}, function () { + physician.patients.create({name: 'c'}, function () { + verify(physician); + }); + }); + }); + }); + + function verify(physician) { + physician.patients.findById(id, function (err, ch) { + should.not.exist(err); + should.exist(ch); + ch.id.should.equal(id); + done(); + }); + } + }); + + it('should set targetClass on scope property', function() { + should.equal(Physician.prototype.patients._targetClass, 'Patient'); + }); + + it('should update scoped record', function (done) { + var id; + Physician.create(function (err, physician) { + physician.patients.create({name: 'a'}, function (err, ch) { + id = ch.id; + physician.patients.updateById(id, {name: 'aa'}, function(err, ch) { + verify(physician); + }); + }); + }); + + function verify(physician) { + physician.patients.findById(id, function (err, ch) { + should.not.exist(err); + should.exist(ch); + ch.id.should.equal(id); + ch.name.should.equal('aa'); + done(); + }); + } + }); + + it('should destroy scoped record', function (done) { + var id; + Physician.create(function (err, physician) { + physician.patients.create({name: 'a'}, function (err, ch) { + id = ch.id; + physician.patients.destroy(id, function(err, ch) { + verify(physician); + }); + }); + }); + + function verify(physician) { + physician.patients.findById(id, function (err, ch) { + should.exist(err); + done(); + }); + } + }); + + it('should check existence of a scoped record', function (done) { + var id; + Physician.create(function (err, physician) { + physician.patients.create({name: 'a'}, function (err, ch) { + id = ch.id; + physician.patients.create({name: 'z'}, function () { + physician.patients.create({name: 'c'}, function () { + verify(physician); + }); + }); + }); + }); + + function verify(physician) { + physician.patients.exists(id, function (err, flag) { + should.not.exist(err); + flag.should.be.eql(true); + done(); + }); + } + }); + + it('should allow to add connection with instance', function (done) { + Physician.create({name: 'ph1'}, function (e, physician) { + Patient.create({name: 'pa1'}, function (e, patient) { + physician.patients.add(patient, function (e, app) { + should.not.exist(e); + should.exist(app); + app.should.be.an.instanceOf(Appointment); + app.physicianId.should.equal(physician.id); + app.patientId.should.equal(patient.id); + done(); + }); + }); + }); + }); + + it('should allow to remove connection with instance', function (done) { + var id; + Physician.create(function (err, physician) { + physician.patients.create({name: 'a'}, function (err, patient) { + id = patient.id; + physician.patients.remove(id, function (err, ch) { + verify(physician); + }); + }); + }); + + function verify(physician) { + physician.patients.exists(id, function (err, flag) { + should.not.exist(err); + flag.should.be.eql(false); + done(); + }); + } + }); + + beforeEach(function (done) { + Appointment.destroyAll(function (err) { + Physician.destroyAll(function (err) { + Patient.destroyAll(done); + }); + }); + }); + }); describe('hasMany with properties', function () { From 9325ce316bf757695f39d230d4dadb158e9b69a4 Mon Sep 17 00:00:00 2001 From: Raymond Feng Date: Tue, 8 Jul 2014 10:54:13 -0700 Subject: [PATCH 20/23] Allow before hooks to pass arguments to next() --- lib/dao.js | 17 +++++++++-------- lib/hooks.js | 9 +++++++-- lib/validations.js | 4 ++-- test/hooks.test.js | 36 ++++++++++++++++++++++++++++++++++-- 4 files changed, 52 insertions(+), 14 deletions(-) diff --git a/lib/dao.js b/lib/dao.js index bc04d80b..271008bb 100644 --- a/lib/dao.js +++ b/lib/dao.js @@ -178,8 +178,8 @@ DataAccessObject.create = function (data, callback) { }); }); }, obj); - }, obj); - }, obj); + }, obj, callback); + }, obj, callback); } // for chaining @@ -1032,8 +1032,8 @@ DataAccessObject.prototype.save = function (options, callback) { }); }); }); - }, data); - }, data); + }, data, callback); + }, data, callback); } }; @@ -1136,7 +1136,7 @@ DataAccessObject.prototype.remove = Model.emit('deleted', id); }); }.bind(this)); - }); + }, null, cb); }; /** @@ -1198,7 +1198,8 @@ DataAccessObject.prototype.updateAttributes = function updateAttributes(data, cb typedData[key] = inst[key]; } - inst._adapter().updateAttributes(model, getIdValue(inst.constructor, inst), inst.constructor._forDB(typedData), function (err) { + inst._adapter().updateAttributes(model, getIdValue(inst.constructor, inst), + inst.constructor._forDB(typedData), function (err) { done.call(inst, function () { saveDone.call(inst, function () { if(cb) cb(err, inst); @@ -1206,8 +1207,8 @@ DataAccessObject.prototype.updateAttributes = function updateAttributes(data, cb }); }); }); - }, data); - }, data); + }, data, cb); + }, data, cb); } }, data); }; diff --git a/lib/hooks.js b/lib/hooks.js index 9288f55b..acf69e4a 100644 --- a/lib/hooks.js +++ b/lib/hooks.js @@ -27,7 +27,7 @@ Hookable.beforeDestroy = null; Hookable.afterDestroy = null; // TODO: Evaluate https://github.com/bnoguchi/hooks-js/ -Hookable.prototype.trigger = function trigger(actionName, work, data) { +Hookable.prototype.trigger = function trigger(actionName, work, data, callback) { var capitalizedName = capitalize(actionName); var beforeHook = this.constructor["before" + capitalizedName] || this.constructor["pre" + capitalizedName]; @@ -42,8 +42,13 @@ Hookable.prototype.trigger = function trigger(actionName, work, data) { // we only call "before" hook when we have actual action (work) to perform if (work) { if (beforeHook) { - // before hook should be called on instance with one param: callback + // before hook should be called on instance with two parameters: next and data beforeHook.call(inst, function () { + // Check arguments to next(err, result) + if (arguments.length) { + return callback && callback.apply(null, arguments); + } + // No err & result is present, proceed with the real work // actual action also have one param: callback work.call(inst, next); }, data); diff --git a/lib/validations.js b/lib/validations.js index a12a8127..af774d62 100644 --- a/lib/validations.js +++ b/lib/validations.js @@ -389,7 +389,7 @@ Validatable.prototype.isValid = function (callback, data) { validationsDone.call(inst, function () { callback(valid); }); - }); + }, data, callback); } return valid; } @@ -440,7 +440,7 @@ Validatable.prototype.isValid = function (callback, data) { } } - }, data); + }, data, callback); if (async) { // in case of async validation we should return undefined here, diff --git a/test/hooks.test.js b/test/hooks.test.js index f613122b..b1502f99 100644 --- a/test/hooks.test.js +++ b/test/hooks.test.js @@ -81,7 +81,27 @@ describe('hooks', function () { } User.afterCreate = function () { - throw new Error('shouldn\'t be called') + throw new Error('shouldn\'t be called'); + }; + User.create(function (err, user) { + User.dataSource.connector.create = old; + done(); + }); + }); + + it('afterCreate should not be triggered on failed beforeCreate', function (done) { + User.beforeCreate = function (next, data) { + // Skip next() + next(new Error('fail in beforeCreate')); + }; + + var old = User.dataSource.connector.create; + User.dataSource.connector.create = function (modelName, id, cb) { + throw new Error('shouldn\'t be called'); + } + + User.afterCreate = function () { + throw new Error('shouldn\'t be called'); }; User.create(function (err, user) { User.dataSource.connector.create = old; @@ -173,6 +193,18 @@ describe('hooks', function () { }); }); + it('beforeSave should be able to skip next', function (done) { + User.create(function (err, user) { + User.beforeSave = function (next, data) { + next(null, 'XYZ'); + }; + user.save(function(err, result) { + result.should.be.eql('XYZ'); + done(); + }); + }); + }); + }); describe('update', function () { @@ -221,7 +253,7 @@ describe('hooks', function () { it('should not trigger after-hook on failed save', function (done) { User.afterUpdate = function () { - should.fail('afterUpdate shouldn\'t be called') + should.fail('afterUpdate shouldn\'t be called'); }; User.create(function (err, user) { var save = User.dataSource.connector.save; From f406c32b33e21fedff325d61f5d1e90e94c782bc Mon Sep 17 00:00:00 2001 From: Raymond Feng Date: Tue, 15 Jul 2014 12:55:40 -0700 Subject: [PATCH 21/23] Bump version --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 6d46bf66..14fabf9f 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "loopback-datasource-juggler", - "version": "1.6.3", + "version": "1.7.0", "description": "LoopBack DataSoure Juggler", "keywords": [ "StrongLoop", From ee6da650e9e1b082e18bb0c7a073fa5259ede068 Mon Sep 17 00:00:00 2001 From: Raymond Feng Date: Tue, 15 Jul 2014 16:09:54 -0700 Subject: [PATCH 22/23] Test instance or id by the model type --- lib/relation-definition.js | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/lib/relation-definition.js b/lib/relation-definition.js index 397c41b2..563e8cf8 100644 --- a/lib/relation-definition.js +++ b/lib/relation-definition.js @@ -737,14 +737,14 @@ HasManyThrough.prototype.add = function (acInst, done) { var fk2 = findBelongsTo(modelThrough, definition.modelTo, pk2); query[fk1] = this.modelInstance[pk1]; - query[fk2] = acInst[pk2] || acInst; + query[fk2] = (acInst instanceof definition.modelTo) ? acInst[pk2] : acInst; var filter = { where: query }; definition.applyScope(this.modelInstance, filter); data[fk1] = this.modelInstance[pk1]; - data[fk2] = acInst[pk2] || acInst; + data[fk2] = (acInst instanceof definition.modelTo) ? acInst[pk2] : acInst; definition.applyProperties(this.modelInstance, data); // Create an instance of the through model @@ -779,10 +779,11 @@ HasManyThrough.prototype.exists = function (acInst, done) { var fk2 = findBelongsTo(modelThrough, definition.modelTo, pk2); query[fk1] = this.modelInstance[pk1]; - query[fk2] = acInst[pk2] || acInst; + + query[fk2] = (acInst instanceof definition.modelTo) ? acInst[pk2] : acInst; data[fk1] = this.modelInstance[pk1]; - data[fk2] = acInst[pk2] || acInst; + data[fk2] = (acInst instanceof definition.modelTo) ? acInst[pk2] : acInst; modelThrough.count(query, function(err, ac) { done(err, ac > 0); @@ -810,7 +811,7 @@ HasManyThrough.prototype.remove = function (acInst, done) { var fk2 = findBelongsTo(modelThrough, definition.modelTo, pk2); query[fk1] = this.modelInstance[pk1]; - query[fk2] = acInst[pk2] || acInst; + query[fk2] = (acInst instanceof definition.modelTo) ? acInst[pk2] : acInst; var filter = { where: query }; From 82c6c8e8511327e287f1126009011be0a847392a Mon Sep 17 00:00:00 2001 From: Raymond Feng Date: Tue, 15 Jul 2014 16:10:37 -0700 Subject: [PATCH 23/23] Make sure related properties are defined for RDBMS --- test/relations.test.js | 46 +++++++++++++++++++++--------------------- 1 file changed, 23 insertions(+), 23 deletions(-) diff --git a/test/relations.test.js b/test/relations.test.js index 8c6580d0..0691ded8 100644 --- a/test/relations.test.js +++ b/test/relations.test.js @@ -5,31 +5,27 @@ var db, Book, Chapter, Author, Reader; var Category, Product; describe('relations', function () { - before(function (done) { - db = getSchema(); - Book = db.define('Book', {name: String}); - Chapter = db.define('Chapter', {name: {type: String, index: true}}); - Author = db.define('Author', {name: String}); - Reader = db.define('Reader', {name: String}); - Category = db.define('Category', {name: String}); - Product = db.define('Product', {name: String, type: String}); - db.automigrate(function () { - Book.destroyAll(function () { - Chapter.destroyAll(function () { - Author.destroyAll(function () { - Reader.destroyAll(done); + describe('hasMany', function () { + before(function (done) { + db = getSchema(); + Book = db.define('Book', {name: String, type: String}); + Chapter = db.define('Chapter', {name: {type: String, index: true}, + bookType: String}); + Author = db.define('Author', {name: String}); + Reader = db.define('Reader', {name: String}); + + db.automigrate(function () { + Book.destroyAll(function () { + Chapter.destroyAll(function () { + Author.destroyAll(function () { + Reader.destroyAll(done); + }); }); }); }); }); - }); - after(function () { - // db.disconnect(); - }); - - describe('hasMany', function () { it('can be declared in different ways', function (done) { Book.hasMany(Chapter); Book.hasMany(Reader, {as: 'users'}); @@ -115,7 +111,7 @@ describe('relations', function () { book.chapters.findById(id, function (err, ch) { should.not.exist(err); should.exist(ch); - ch.id.should.equal(id); + ch.id.should.eql(id); done(); }); } @@ -140,7 +136,7 @@ describe('relations', function () { book.chapters.findById(id, function (err, ch) { should.not.exist(err); should.exist(ch); - ch.id.should.equal(id); + ch.id.should.eql(id); ch.name.should.equal('aa'); done(); }); @@ -407,9 +403,13 @@ describe('relations', function () { }); }); }); - + describe('hasMany with scope', function () { it('can be declared with properties', function (done) { + db = getSchema(); + Category = db.define('Category', {name: String, productType: String}); + Product = db.define('Product', {name: String, type: String}); + Category.hasMany(Product, { properties: function(inst) { if (!inst.productType) return; // skip @@ -563,7 +563,7 @@ describe('relations', function () { before(function () { db = getSchema(); Supplier = db.define('Supplier', {name: String}); - Account = db.define('Account', {accountNo: String}); + Account = db.define('Account', {accountNo: String, supplierName: String}); }); it('can be declared using hasOne method', function () {