From b66183f7dc3ae415e6fb733945da5a0e39c17c07 Mon Sep 17 00:00:00 2001 From: Samuel Reed Date: Fri, 4 Jul 2014 16:40:08 -0500 Subject: [PATCH 01/15] 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 02/15] 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 03/15] 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 04/15] 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 05/15] 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 06/15] 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 07/15] 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 08/15] 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 09/15] 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 10/15] 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 11/15] 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 12/15] 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 13/15] 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 14/15] 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 15/15] 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 () {