From 6aaa3f216e12097261aec879d75ef7d83dd717c7 Mon Sep 17 00:00:00 2001 From: Fabien Franzen Date: Fri, 11 Jul 2014 15:29:47 +0200 Subject: [PATCH 1/3] 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 48c4f25b09ba75c607a7d0dc5ee642d8e233be64 Mon Sep 17 00:00:00 2001 From: Fabien Franzen Date: Sat, 12 Jul 2014 00:02:16 +0200 Subject: [PATCH 2/3] 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 3/3] 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);