From 35850f6632684765759721f192026baf5a659907 Mon Sep 17 00:00:00 2001 From: Fabien Franzen Date: Fri, 15 Aug 2014 11:28:25 +0200 Subject: [PATCH 1/8] Clarified tests, fixed BelongsTo.prototype.create Added clarified test-case based on previous documentation example. Fixed BelongsTo.prototype.create - although the foreignKey was set on the model instance, it was never actually persisted, unless you'd issue a separate call to save the 'parent' model. --- lib/relation-definition.js | 25 +++++++++------- test/relations.test.js | 61 ++++++++++++++++++++++++++++---------- 2 files changed, 60 insertions(+), 26 deletions(-) diff --git a/lib/relation-definition.js b/lib/relation-definition.js index 011b545b..f7af8d2a 100644 --- a/lib/relation-definition.js +++ b/lib/relation-definition.js @@ -155,16 +155,20 @@ RelationDefinition.prototype.applyScope = function(modelInstance, filter) { * @param {Object} modelInstance * @param {Object} target */ -RelationDefinition.prototype.applyProperties = function(modelInstance, target) { +RelationDefinition.prototype.applyProperties = function(modelInstance, obj) { + var source = modelInstance, target = obj; + if (this.options.invertProperties) { + source = obj, target = modelInstance; + } if (typeof this.properties === 'function') { - var data = this.properties.call(this, modelInstance); + var data = this.properties.call(this, source); for(var k in data) { target[k] = data[k]; } } else if (typeof this.properties === 'object') { for(var k in this.properties) { var key = this.properties[k]; - target[key] = modelInstance[k]; + target[key] = source[k]; } } if ((this.type !== 'belongsTo' || this.type === 'hasOne') @@ -1044,14 +1048,17 @@ BelongsTo.prototype.create = function(targetModelData, cb) { cb = targetModelData; targetModelData = {}; } - + this.definition.applyProperties(modelInstance, targetModelData || {}); modelTo.create(targetModelData, function(err, targetModel) { if(!err) { modelInstance[fk] = targetModel[pk]; - self.resetCache(targetModel); - cb && cb(err, targetModel); + modelInstance.save(function(err, inst) { + if (cb && err) return cb && cb(err); + self.resetCache(targetModel); + cb && cb(err, targetModel); + }); } else { cb && cb(err); } @@ -1100,9 +1107,7 @@ BelongsTo.prototype.related = function (refresh, params) { modelInstance[fk] = params[pk]; if (discriminator) modelInstance[discriminator] = params.constructor.modelName; - var data = {}; - this.definition.applyProperties(params, data); - modelInstance.setAttributes(data); + this.definition.applyProperties(modelInstance, params); self.resetCache(params); } else if (typeof params === 'function') { // acts as async getter @@ -1607,7 +1612,7 @@ EmbedsMany.prototype.related = function(receiver, scopeParams, condOrRefresh, cb var params = mergeQuery(actualCond, scopeParams); - if (params.where) { + if (params.where) { // TODO [fabien] Support order/sorting embeddedList = embeddedList ? embeddedList.filter(applyFilter(params)) : embeddedList; } diff --git a/test/relations.test.js b/test/relations.test.js index d1a2003a..da672b1a 100644 --- a/test/relations.test.js +++ b/test/relations.test.js @@ -1109,11 +1109,9 @@ describe('relations', function () { p.person.create({name: 'Fred', age: 36 }, function(err, person) { personCreated = person; p.personId.should.equal(person.id); - p.save(function (err, p) { - person.name.should.equal('Fred'); - person.passportNotes.should.equal('Some notes...'); - done(); - }); + person.name.should.equal('Fred'); + person.passportNotes.should.equal('Some notes...'); + done(); }); }); @@ -1191,7 +1189,6 @@ describe('relations', function () { Article.create(function (e, article) { article.tags.create({name: 'popular'}, function (e, t) { t.should.be.an.instanceOf(Tag); - // console.log(t); ArticleTag.findOne(function (e, at) { should.exist(at); at.tagId.toString().should.equal(t.id.toString()); @@ -1566,19 +1563,15 @@ describe('relations', function () { describe('embedsMany - relations, scope and properties', function () { - var product1, product2, product3; + var category, product1, product2, product3; - before(function (done) { + before(function () { db = getSchema(); Category = db.define('Category', {name: String}); Product = db.define('Product', {name: String}); Link = db.define('Link', {name: String}); - - db.automigrate(function () { - Person.destroyAll(done); - }); }); - + it('can be declared', function (done) { Category.embedsMany(Link, { as: 'items', // rename @@ -1588,6 +1581,7 @@ describe('relations', function () { Link.belongsTo(Product, { foreignKey: 'id', // re-use the actual product id properties: { id: 'id', name: 'name' }, // denormalize, transfer id + options: { invertProperties: true } }); db.automigrate(function() { Product.create({ name: 'Product 0' }, done); // offset ids for tests @@ -1607,7 +1601,7 @@ describe('relations', function () { }); }); - it('should create items on scope', function(done) { + it('should associate items on scope', function(done) { Category.create({ name: 'Category A' }, function(err, cat) { var link = cat.items.build(); link.product(product1); @@ -1718,13 +1712,47 @@ describe('relations', function () { }); }); - it('should remove embedded items by reference id', function(done) { + it('should have removed embedded items by reference id', function(done) { Category.findOne(function(err, cat) { cat.links.should.have.length(1); done(); }); }); + it('should create items on scope', function(done) { + Category.create({ name: 'Category B' }, function(err, cat) { + category = cat; + var link = cat.items.build({ notes: 'Some notes...' }); + link.product.create({ name: 'Product 1' }, function(err, p) { + cat.save(function(err, cat) { // save parent object! + cat.links[0].id.should.eql(p.id); + cat.links[0].name.should.equal('Product 1'); // denormalized + cat.links[0].notes.should.equal('Some notes...'); + cat.items.at(0).should.equal(cat.links[0]); + done(); + }); + }) + }); + }); + + it('should find items on scope', function(done) { + Category.findById(category.id, function(err, cat) { + cat.name.should.equal('Category B'); + cat.links.toObject().should.eql([ + {id: 5, name: 'Product 1', notes: 'Some notes...'} + ]); + cat.items.at(0).should.equal(cat.links[0]); + cat.items(function(err, items) { // alternative access + items.should.be.an.array; + items.should.have.length(1); + items[0].product(function(err, p) { + p.name.should.equal('Product 1'); // actual value + done(); + }); + }); + }); + }); + }); describe('embedsMany - polymorphic relations', function () { @@ -1757,7 +1785,8 @@ describe('relations', function () { }); Link.belongsTo('linked', { polymorphic: true, // needs unique auto-id - properties: { name: 'name' } // denormalized + properties: { name: 'name' }, // denormalized + options: { invertProperties: true } }); db.automigrate(done); }); From c3c2c85ce4e81854e56c398decc5ac6c6aec4b43 Mon Sep 17 00:00:00 2001 From: Fabien Franzen Date: Fri, 15 Aug 2014 12:55:10 +0200 Subject: [PATCH 2/8] Refactor polymorphic relations, fix inverse #215 See #215 - when creating a related item through a the inverse of a polymorphic HABTM relation, the through-model was not created correctly. By refactoring the specifics into the `polymorphic` property of a RelationDefinition, it's now possible to handle this correctly. --- lib/relation-definition.js | 62 ++++++++++++++++++++++++++------------ test/relations.test.js | 34 +++++++++++++++++++++ 2 files changed, 76 insertions(+), 20 deletions(-) diff --git a/lib/relation-definition.js b/lib/relation-definition.js index f7af8d2a..38aee0ea 100644 --- a/lib/relation-definition.js +++ b/lib/relation-definition.js @@ -98,8 +98,8 @@ function RelationDefinition(definition) { this.keyFrom = definition.keyFrom; this.modelTo = definition.modelTo; this.keyTo = definition.keyTo; - this.discriminator = definition.discriminator; - if (!this.discriminator) { + this.polymorphic = definition.polymorphic; + if (typeof this.polymorphic !== 'object') { assert(this.modelTo, 'Target model is required'); } this.modelThrough = definition.modelThrough; @@ -137,8 +137,13 @@ RelationDefinition.prototype.applyScope = function(modelInstance, filter) { filter = filter || {}; filter.where = filter.where || {}; if ((this.type !== 'belongsTo' || this.type === 'hasOne') - && typeof this.discriminator === 'string') { // polymorphic - filter.where[this.discriminator] = this.modelFrom.modelName; + && typeof this.polymorphic === 'object') { // polymorphic + var discriminator = this.polymorphic.discriminator; + if (this.polymorphic.invert) { + filter.where[discriminator] = this.modelTo.modelName; + } else { + filter.where[discriminator] = this.modelFrom.modelName; + } } if (typeof this.scope === 'function') { var scope = this.scope.call(this, modelInstance, filter); @@ -172,8 +177,13 @@ RelationDefinition.prototype.applyProperties = function(modelInstance, obj) { } } if ((this.type !== 'belongsTo' || this.type === 'hasOne') - && typeof this.discriminator === 'string') { // polymorphic - target[this.discriminator] = this.modelFrom.modelName; + && typeof this.polymorphic === 'object') { // polymorphic + var discriminator = this.polymorphic.discriminator; + if (this.polymorphic.invert) { + target[discriminator] = this.modelTo.modelName; + } else { + target[discriminator] = this.modelFrom.modelName; + } } }; @@ -461,10 +471,11 @@ 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 discriminator; + var discriminator, polymorphic; if (params.polymorphic) { - var polymorphic = polymorphicParams(params.polymorphic); + polymorphic = polymorphicParams(params.polymorphic); + polymorphic.invert = !!params.invert; discriminator = polymorphic.discriminator; if (!params.invert) { fk = polymorphic.foreignKey; @@ -480,12 +491,12 @@ RelationDefinition.hasMany = function hasMany(modelFrom, modelTo, params) { modelFrom: modelFrom, keyFrom: idName, keyTo: fk, - discriminator: discriminator, modelTo: modelTo, multiple: true, properties: params.properties, scope: params.scope, - options: params.options + options: params.options, + polymorphic: polymorphic }); definition.modelThrough = params.through; @@ -700,9 +711,13 @@ var throughKeys = function(definition) { var modelThrough = definition.modelThrough; var pk2 = definition.modelTo.definition.idName(); - if (definition.discriminator) { // polymorphic + if (typeof definition.polymorphic === 'object') { // polymorphic var fk1 = definition.keyTo; - var fk2 = definition.keyThrough; + if (definition.polymorphic.invert) { + var fk2 = definition.polymorphic.foreignKey; + } else { + var fk2 = definition.keyThrough; + } } else { var fk1 = findBelongsTo(modelThrough, definition.modelFrom, definition.keyFrom); @@ -1004,11 +1019,11 @@ RelationDefinition.belongsTo = function (modelFrom, modelTo, params) { modelFrom: modelFrom, keyFrom: fk, keyTo: idName, - discriminator: discriminator, modelTo: modelTo, properties: params.properties, scope: params.scope, - options: params.options + options: params.options, + polymorphic: polymorphic }); // Define a property for the scope so that we have 'this' for the scoped methods @@ -1086,10 +1101,10 @@ BelongsTo.prototype.related = function (refresh, params) { var self = this; var modelFrom = this.definition.modelFrom; var modelTo = this.definition.modelTo; - var discriminator = this.definition.discriminator; var pk = this.definition.keyTo; var fk = this.definition.keyFrom; var modelInstance = this.modelInstance; + var discriminator; if (arguments.length === 1) { params = refresh; @@ -1098,6 +1113,10 @@ BelongsTo.prototype.related = function (refresh, params) { throw new Error('Method can\'t be called with more than two arguments'); } + if (typeof this.definition.polymorphic === 'object') { + discriminator = this.definition.polymorphic.discriminator; + } + var cachedValue; if (!refresh) { cachedValue = self.getCache(); @@ -1105,7 +1124,10 @@ BelongsTo.prototype.related = function (refresh, params) { if (params instanceof ModelBaseClass) { // acts as setter modelTo = params.constructor; modelInstance[fk] = params[pk]; - if (discriminator) modelInstance[discriminator] = params.constructor.modelName; + + if (discriminator) { + modelInstance[discriminator] = params.constructor.modelName; + } this.definition.applyProperties(modelInstance, params); @@ -1245,10 +1267,10 @@ 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 discriminator; + var discriminator, polymorphic; if (params.polymorphic) { - var polymorphic = polymorphicParams(params.polymorphic); + polymorphic = polymorphicParams(params.polymorphic); fk = polymorphic.foreignKey; discriminator = polymorphic.discriminator; if (!params.through) { @@ -1262,10 +1284,10 @@ RelationDefinition.hasOne = function (modelFrom, modelTo, params) { modelFrom: modelFrom, keyFrom: pk, keyTo: fk, - discriminator: discriminator, modelTo: modelTo, properties: params.properties, - options: params.options + options: params.options, + polymorphic: polymorphic }); modelFrom.dataSource.defineForeignKey(modelTo.modelName, fk, modelFrom.modelName); diff --git a/test/relations.test.js b/test/relations.test.js index da672b1a..5887adb0 100644 --- a/test/relations.test.js +++ b/test/relations.test.js @@ -1030,6 +1030,40 @@ describe('relations', function () { }); }); }); + + it('should create polymorphic item through relation scope', function (done) { + Picture.findById(anotherPicture.id, function(err, p) { + p.authors.create({ name: 'Author 3' }, function(err, a) { + should.not.exist(err); + author = a; + author.name.should.equal('Author 3'); + done(); + }); + }); + }); + + it('should create polymorphic through model - new author', function (done) { + PictureLink.findOne({ where: { + pictureId: anotherPicture.id, imageableId: author.id, imageableType: 'Author' + } }, function(err, link) { + should.not.exist(err); + link.pictureId.should.eql(anotherPicture.id); + link.imageableId.should.eql(author.id); + link.imageableType.should.equal('Author'); + done(); + }); + }); + + it('should find polymorphic items - new author', function (done) { + Author.findById(author.id, function(err, author) { + author.pictures(function(err, pics) { + pics.should.have.length(1); + pics[0].id.should.eql(anotherPicture.id); + pics[0].name.should.equal('Example'); + done(); + }); + }); + }); }); From 21801058c9aa75f67a86924416a59bc377b835c1 Mon Sep 17 00:00:00 2001 From: Fabien Franzen Date: Fri, 15 Aug 2014 15:12:02 +0200 Subject: [PATCH 3/8] Refactor embedsMany - auto-save parent With this change, saving an embedded model now correctly updates the parent model. Before, a separate `save()` call on the parent was required, contrary to other relation types. --- lib/relation-definition.js | 62 ++++++++++++++++++++++++++++++-------- test/relations.test.js | 34 ++++++++++++++++----- 2 files changed, 75 insertions(+), 21 deletions(-) diff --git a/lib/relation-definition.js b/lib/relation-definition.js index 38aee0ea..2e5f3766 100644 --- a/lib/relation-definition.js +++ b/lib/relation-definition.js @@ -1607,11 +1607,48 @@ RelationDefinition.embedsMany = function embedsMany(modelFrom, modelTo, params) scopeDefinition.related = scopeMethods.related; }; +EmbedsMany.prototype.prepareEmbeddedInstance = function(inst) { + if (inst && inst.triggerParent !== 'function') { + var relationName = this.definition.name; + var modelInstance = this.modelInstance; + inst.triggerParent = function(actionName, callback) { + if (actionName === 'save' || actionName === 'destroy') { + var embeddedList = modelInstance[relationName] || []; + modelInstance.updateAttribute(relationName, + embeddedList, function(err, modelInst) { + callback(err, err ? null : modelInst); + }); + } else { + process.nextTick(callback); + } + }; + var originalTrigger = inst.trigger; + inst.trigger = function(actionName, work, data, callback) { + if (typeof work === 'function') { + var originalWork = work; + work = function(next) { + originalWork.call(this, function(done) { + inst.triggerParent(actionName, function(err, inst) { + next(done); // TODO [fabien] - error handling? + }); + }); + }; + } + originalTrigger.call(this, actionName, work, data, callback); + }; + } +}; + +EmbedsMany.prototype.embeddedList = function(modelInstance) { + modelInstance = modelInstance || this.modelInstance; + var embeddedList = modelInstance[this.definition.name] || []; + embeddedList.forEach(this.prepareEmbeddedInstance.bind(this)); + return embeddedList; +}; + EmbedsMany.prototype.related = function(receiver, scopeParams, condOrRefresh, cb) { var modelTo = this.definition.modelTo; - var relationName = this.definition.name; var modelInstance = this.modelInstance; - var self = receiver; var actualCond = {}; var actualRefresh = false; @@ -1628,9 +1665,9 @@ EmbedsMany.prototype.related = function(receiver, scopeParams, condOrRefresh, cb throw new Error('Method can be only called with one or two arguments'); } - var embeddedList = self[relationName] || []; + var embeddedList = this.embeddedList(receiver); - this.definition.applyScope(modelInstance, actualCond); + this.definition.applyScope(receiver, actualCond); var params = mergeQuery(actualCond, scopeParams); @@ -1652,10 +1689,9 @@ EmbedsMany.prototype.related = function(receiver, scopeParams, condOrRefresh, cb EmbedsMany.prototype.findById = function (fkId, cb) { var pk = this.definition.keyFrom; var modelTo = this.definition.modelTo; - var relationName = this.definition.name; var modelInstance = this.modelInstance; - var embeddedList = modelInstance[relationName] || []; + var embeddedList = this.embeddedList(); var find = function(id) { for (var i = 0; i < embeddedList.length; i++) { @@ -1695,7 +1731,7 @@ EmbedsMany.prototype.updateById = function (fkId, data, cb) { var relationName = this.definition.name; var modelInstance = this.modelInstance; - var embeddedList = modelInstance[relationName] || []; + var embeddedList = this.embeddedList(); var inst = this.findById(fkId); @@ -1729,7 +1765,7 @@ EmbedsMany.prototype.destroyById = function (fkId, cb) { var relationName = this.definition.name; var modelInstance = this.modelInstance; - var embeddedList = modelInstance[relationName] || []; + var embeddedList = this.embeddedList(); var inst = (fkId instanceof modelTo) ? fkId : this.findById(fkId); @@ -1754,10 +1790,9 @@ EmbedsMany.prototype.unset = EmbedsMany.prototype.destroyById; EmbedsMany.prototype.at = function (index, cb) { var modelTo = this.definition.modelTo; - var relationName = this.definition.name; var modelInstance = this.modelInstance; - var embeddedList = modelInstance[relationName] || []; + var embeddedList = this.embeddedList(); var item = embeddedList[parseInt(index)]; item = (item instanceof modelTo) ? item : null; @@ -1784,7 +1819,7 @@ EmbedsMany.prototype.create = function (targetModelData, cb) { } targetModelData = targetModelData || {}; - var embeddedList = modelInstance[relationName] || []; + var embeddedList = this.embeddedList(); var inst = this.build(targetModelData); @@ -1805,11 +1840,10 @@ EmbedsMany.prototype.create = function (targetModelData, cb) { EmbedsMany.prototype.build = function(targetModelData) { var pk = this.definition.keyFrom; var modelTo = this.definition.modelTo; - var relationName = this.definition.name; var modelInstance = this.modelInstance; var autoId = this.definition.options.autoId !== false; - var embeddedList = modelInstance[relationName] || []; + var embeddedList = this.embeddedList(); targetModelData = targetModelData || {}; @@ -1834,6 +1868,8 @@ EmbedsMany.prototype.build = function(targetModelData) { embeddedList.push(inst); } + this.prepareEmbeddedInstance(inst); + return inst; }; diff --git a/test/relations.test.js b/test/relations.test.js index 5887adb0..7f239f33 100644 --- a/test/relations.test.js +++ b/test/relations.test.js @@ -1758,14 +1758,12 @@ describe('relations', function () { category = cat; var link = cat.items.build({ notes: 'Some notes...' }); link.product.create({ name: 'Product 1' }, function(err, p) { - cat.save(function(err, cat) { // save parent object! - cat.links[0].id.should.eql(p.id); - cat.links[0].name.should.equal('Product 1'); // denormalized - cat.links[0].notes.should.equal('Some notes...'); - cat.items.at(0).should.equal(cat.links[0]); - done(); - }); - }) + cat.links[0].id.should.eql(p.id); + cat.links[0].name.should.equal('Product 1'); // denormalized + cat.links[0].notes.should.equal('Some notes...'); + cat.items.at(0).should.equal(cat.links[0]); + done(); + }); }); }); @@ -1787,6 +1785,26 @@ describe('relations', function () { }); }); + it('should update items on scope - and save parent', function(done) { + Category.findById(category.id, function(err, cat) { + var link = cat.items.at(0); + link.updateAttributes({notes: 'Updated notes...'}, function(err, link) { + link.notes.should.equal('Updated notes...'); + done(); + }); + }); + }); + + it('should find items on scope - verify update', function(done) { + Category.findById(category.id, function(err, cat) { + cat.name.should.equal('Category B'); + cat.links.toObject().should.eql([ + {id: 5, name: 'Product 1', notes: 'Updated notes...'} + ]); + done(); + }); + }); + }); describe('embedsMany - polymorphic relations', function () { From c2f9ee381cc7b470e2cf230a4e00523f11d638b3 Mon Sep 17 00:00:00 2001 From: Fabien Franzen Date: Fri, 15 Aug 2014 15:24:00 +0200 Subject: [PATCH 4/8] Implement embedded.destroy() integration --- lib/relation-definition.js | 7 ++++++- test/relations.test.js | 17 +++++++++++++++++ 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/lib/relation-definition.js b/lib/relation-definition.js index 2e5f3766..65843c29 100644 --- a/lib/relation-definition.js +++ b/lib/relation-definition.js @@ -1609,11 +1609,16 @@ RelationDefinition.embedsMany = function embedsMany(modelFrom, modelTo, params) EmbedsMany.prototype.prepareEmbeddedInstance = function(inst) { if (inst && inst.triggerParent !== 'function') { + var self = this; var relationName = this.definition.name; var modelInstance = this.modelInstance; inst.triggerParent = function(actionName, callback) { if (actionName === 'save' || actionName === 'destroy') { - var embeddedList = modelInstance[relationName] || []; + var embeddedList = self.embeddedList(); + if (actionName === 'destroy') { + var index = embeddedList.indexOf(inst); + if (index > -1) embeddedList.splice(index, 1); + } modelInstance.updateAttribute(relationName, embeddedList, function(err, modelInst) { callback(err, err ? null : modelInst); diff --git a/test/relations.test.js b/test/relations.test.js index 7f239f33..53f04229 100644 --- a/test/relations.test.js +++ b/test/relations.test.js @@ -1805,6 +1805,23 @@ describe('relations', function () { }); }); + it('should remove items from scope - and save parent', function(done) { + Category.findById(category.id, function(err, cat) { + cat.items.at(0).destroy(function(err, link) { + cat.links.should.eql([]); + done(); + }); + }); + }); + + it('should find items on scope - verify destroy', function(done) { + Category.findById(category.id, function(err, cat) { + cat.name.should.equal('Category B'); + cat.links.should.eql([]); + done(); + }); + }); + }); describe('embedsMany - polymorphic relations', function () { From 78e2c9c9d47f987ef56cdb4f043461a406a2e324 Mon Sep 17 00:00:00 2001 From: Fabien Franzen Date: Fri, 15 Aug 2014 11:28:25 +0200 Subject: [PATCH 5/8] Clarified tests, fixed BelongsTo.prototype.create Added clarified test-case based on previous documentation example. Fixed BelongsTo.prototype.create - although the foreignKey was set on the model instance, it was never actually persisted, unless you'd issue a separate call to save the 'parent' model. --- lib/relation-definition.js | 25 +++++++++------- test/relations.test.js | 61 ++++++++++++++++++++++++++++---------- 2 files changed, 60 insertions(+), 26 deletions(-) diff --git a/lib/relation-definition.js b/lib/relation-definition.js index 93d1b6ca..edc833d2 100644 --- a/lib/relation-definition.js +++ b/lib/relation-definition.js @@ -181,16 +181,20 @@ RelationDefinition.prototype.applyScope = function(modelInstance, filter) { * @param {Object} modelInstance * @param {Object} target */ -RelationDefinition.prototype.applyProperties = function(modelInstance, target) { +RelationDefinition.prototype.applyProperties = function(modelInstance, obj) { + var source = modelInstance, target = obj; + if (this.options.invertProperties) { + source = obj, target = modelInstance; + } if (typeof this.properties === 'function') { - var data = this.properties.call(this, modelInstance); + var data = this.properties.call(this, source); for(var k in data) { target[k] = data[k]; } } else if (typeof this.properties === 'object') { for(var k in this.properties) { var key = this.properties[k]; - target[key] = modelInstance[k]; + target[key] = source[k]; } } if ((this.type !== 'belongsTo' || this.type === 'hasOne') @@ -1082,14 +1086,17 @@ BelongsTo.prototype.create = function(targetModelData, cb) { cb = targetModelData; targetModelData = {}; } - + this.definition.applyProperties(modelInstance, targetModelData || {}); modelTo.create(targetModelData, function(err, targetModel) { if(!err) { modelInstance[fk] = targetModel[pk]; - self.resetCache(targetModel); - cb && cb(err, targetModel); + modelInstance.save(function(err, inst) { + if (cb && err) return cb && cb(err); + self.resetCache(targetModel); + cb && cb(err, targetModel); + }); } else { cb && cb(err); } @@ -1138,9 +1145,7 @@ BelongsTo.prototype.related = function (refresh, params) { modelInstance[fk] = params[pk]; if (discriminator) modelInstance[discriminator] = params.constructor.modelName; - var data = {}; - this.definition.applyProperties(params, data); - modelInstance.setAttributes(data); + this.definition.applyProperties(modelInstance, params); self.resetCache(params); } else if (typeof params === 'function') { // acts as async getter @@ -1652,7 +1657,7 @@ EmbedsMany.prototype.related = function(receiver, scopeParams, condOrRefresh, cb var params = mergeQuery(actualCond, scopeParams); - if (params.where) { + if (params.where) { // TODO [fabien] Support order/sorting embeddedList = embeddedList ? embeddedList.filter(applyFilter(params)) : embeddedList; } diff --git a/test/relations.test.js b/test/relations.test.js index ce0e62f3..458382b4 100644 --- a/test/relations.test.js +++ b/test/relations.test.js @@ -1109,11 +1109,9 @@ describe('relations', function () { p.person.create({name: 'Fred', age: 36 }, function(err, person) { personCreated = person; p.personId.should.equal(person.id); - p.save(function (err, p) { - person.name.should.equal('Fred'); - person.passportNotes.should.equal('Some notes...'); - done(); - }); + person.name.should.equal('Fred'); + person.passportNotes.should.equal('Some notes...'); + done(); }); }); @@ -1191,7 +1189,6 @@ describe('relations', function () { Article.create(function (e, article) { article.tags.create({name: 'popular'}, function (e, t) { t.should.be.an.instanceOf(Tag); - // console.log(t); ArticleTag.findOne(function (e, at) { should.exist(at); at.tagId.toString().should.equal(t.id.toString()); @@ -1566,19 +1563,15 @@ describe('relations', function () { describe('embedsMany - relations, scope and properties', function () { - var product1, product2, product3; + var category, product1, product2, product3; - before(function (done) { + before(function () { db = getSchema(); Category = db.define('Category', {name: String}); Product = db.define('Product', {name: String}); Link = db.define('Link', {name: String}); - - db.automigrate(function () { - Person.destroyAll(done); - }); }); - + it('can be declared', function (done) { Category.embedsMany(Link, { as: 'items', // rename @@ -1588,6 +1581,7 @@ describe('relations', function () { Link.belongsTo(Product, { foreignKey: 'id', // re-use the actual product id properties: { id: 'id', name: 'name' }, // denormalize, transfer id + options: { invertProperties: true } }); db.automigrate(function() { Product.create({ name: 'Product 0' }, done); // offset ids for tests @@ -1607,7 +1601,7 @@ describe('relations', function () { }); }); - it('should create items on scope', function(done) { + it('should associate items on scope', function(done) { Category.create({ name: 'Category A' }, function(err, cat) { var link = cat.items.build(); link.product(product1); @@ -1718,13 +1712,47 @@ describe('relations', function () { }); }); - it('should remove embedded items by reference id', function(done) { + it('should have removed embedded items by reference id', function(done) { Category.findOne(function(err, cat) { cat.links.should.have.length(1); done(); }); }); + it('should create items on scope', function(done) { + Category.create({ name: 'Category B' }, function(err, cat) { + category = cat; + var link = cat.items.build({ notes: 'Some notes...' }); + link.product.create({ name: 'Product 1' }, function(err, p) { + cat.save(function(err, cat) { // save parent object! + cat.links[0].id.should.eql(p.id); + cat.links[0].name.should.equal('Product 1'); // denormalized + cat.links[0].notes.should.equal('Some notes...'); + cat.items.at(0).should.equal(cat.links[0]); + done(); + }); + }) + }); + }); + + it('should find items on scope', function(done) { + Category.findById(category.id, function(err, cat) { + cat.name.should.equal('Category B'); + cat.links.toObject().should.eql([ + {id: 5, name: 'Product 1', notes: 'Some notes...'} + ]); + cat.items.at(0).should.equal(cat.links[0]); + cat.items(function(err, items) { // alternative access + items.should.be.an.array; + items.should.have.length(1); + items[0].product(function(err, p) { + p.name.should.equal('Product 1'); // actual value + done(); + }); + }); + }); + }); + }); describe('embedsMany - polymorphic relations', function () { @@ -1757,7 +1785,8 @@ describe('relations', function () { }); Link.belongsTo('linked', { polymorphic: true, // needs unique auto-id - properties: { name: 'name' } // denormalized + properties: { name: 'name' }, // denormalized + options: { invertProperties: true } }); db.automigrate(done); }); From 8193f402bbdab896b5d464c070a8df6f3e3fe33f Mon Sep 17 00:00:00 2001 From: Fabien Franzen Date: Fri, 15 Aug 2014 12:55:10 +0200 Subject: [PATCH 6/8] Refactor polymorphic relations, fix inverse #215 See #215 - when creating a related item through a the inverse of a polymorphic HABTM relation, the through-model was not created correctly. By refactoring the specifics into the `polymorphic` property of a RelationDefinition, it's now possible to handle this correctly. --- lib/relation-definition.js | 62 ++++++++++++++++++++++++++------------ test/relations.test.js | 34 +++++++++++++++++++++ 2 files changed, 76 insertions(+), 20 deletions(-) diff --git a/lib/relation-definition.js b/lib/relation-definition.js index edc833d2..f824a8a5 100644 --- a/lib/relation-definition.js +++ b/lib/relation-definition.js @@ -101,8 +101,8 @@ function RelationDefinition(definition) { this.keyFrom = definition.keyFrom; this.modelTo = definition.modelTo; this.keyTo = definition.keyTo; - this.discriminator = definition.discriminator; - if (!this.discriminator) { + this.polymorphic = definition.polymorphic; + if (typeof this.polymorphic !== 'object') { assert(this.modelTo, 'Target model is required'); } this.modelThrough = definition.modelThrough; @@ -163,8 +163,13 @@ RelationDefinition.prototype.applyScope = function(modelInstance, filter) { filter = filter || {}; filter.where = filter.where || {}; if ((this.type !== 'belongsTo' || this.type === 'hasOne') - && typeof this.discriminator === 'string') { // polymorphic - filter.where[this.discriminator] = this.modelFrom.modelName; + && typeof this.polymorphic === 'object') { // polymorphic + var discriminator = this.polymorphic.discriminator; + if (this.polymorphic.invert) { + filter.where[discriminator] = this.modelTo.modelName; + } else { + filter.where[discriminator] = this.modelFrom.modelName; + } } if (typeof this.scope === 'function') { var scope = this.scope.call(this, modelInstance, filter); @@ -198,8 +203,13 @@ RelationDefinition.prototype.applyProperties = function(modelInstance, obj) { } } if ((this.type !== 'belongsTo' || this.type === 'hasOne') - && typeof this.discriminator === 'string') { // polymorphic - target[this.discriminator] = this.modelFrom.modelName; + && typeof this.polymorphic === 'object') { // polymorphic + var discriminator = this.polymorphic.discriminator; + if (this.polymorphic.invert) { + target[discriminator] = this.modelTo.modelName; + } else { + target[discriminator] = this.modelFrom.modelName; + } } }; @@ -496,10 +506,11 @@ 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 discriminator; + var discriminator, polymorphic; if (params.polymorphic) { - var polymorphic = polymorphicParams(params.polymorphic); + polymorphic = polymorphicParams(params.polymorphic); + polymorphic.invert = !!params.invert; discriminator = polymorphic.discriminator; if (!params.invert) { fk = polymorphic.foreignKey; @@ -515,12 +526,12 @@ RelationDefinition.hasMany = function hasMany(modelFrom, modelTo, params) { modelFrom: modelFrom, keyFrom: idName, keyTo: fk, - discriminator: discriminator, modelTo: modelTo, multiple: true, properties: params.properties, scope: params.scope, - options: params.options + options: params.options, + polymorphic: polymorphic }); definition.modelThrough = params.through; @@ -736,9 +747,13 @@ var throughKeys = function(definition) { var modelThrough = definition.modelThrough; var pk2 = definition.modelTo.definition.idName(); - if (definition.discriminator) { // polymorphic + if (typeof definition.polymorphic === 'object') { // polymorphic var fk1 = definition.keyTo; - var fk2 = definition.keyThrough; + if (definition.polymorphic.invert) { + var fk2 = definition.polymorphic.foreignKey; + } else { + var fk2 = definition.keyThrough; + } } else { var fk1 = findBelongsTo(modelThrough, definition.modelFrom, definition.keyFrom); @@ -1040,11 +1055,11 @@ RelationDefinition.belongsTo = function (modelFrom, modelTo, params) { modelFrom: modelFrom, keyFrom: fk, keyTo: idName, - discriminator: discriminator, modelTo: modelTo, properties: params.properties, scope: params.scope, - options: params.options + options: params.options, + polymorphic: polymorphic }); // Define a property for the scope so that we have 'this' for the scoped methods @@ -1124,10 +1139,10 @@ BelongsTo.prototype.related = function (refresh, params) { var self = this; var modelFrom = this.definition.modelFrom; var modelTo = this.definition.modelTo; - var discriminator = this.definition.discriminator; var pk = this.definition.keyTo; var fk = this.definition.keyFrom; var modelInstance = this.modelInstance; + var discriminator; if (arguments.length === 1) { params = refresh; @@ -1136,6 +1151,10 @@ BelongsTo.prototype.related = function (refresh, params) { throw new Error('Method can\'t be called with more than two arguments'); } + if (typeof this.definition.polymorphic === 'object') { + discriminator = this.definition.polymorphic.discriminator; + } + var cachedValue; if (!refresh) { cachedValue = self.getCache(); @@ -1143,7 +1162,10 @@ BelongsTo.prototype.related = function (refresh, params) { if (params instanceof ModelBaseClass) { // acts as setter modelTo = params.constructor; modelInstance[fk] = params[pk]; - if (discriminator) modelInstance[discriminator] = params.constructor.modelName; + + if (discriminator) { + modelInstance[discriminator] = params.constructor.modelName; + } this.definition.applyProperties(modelInstance, params); @@ -1282,10 +1304,10 @@ 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 discriminator; + var discriminator, polymorphic; if (params.polymorphic) { - var polymorphic = polymorphicParams(params.polymorphic); + polymorphic = polymorphicParams(params.polymorphic); fk = polymorphic.foreignKey; discriminator = polymorphic.discriminator; if (!params.through) { @@ -1299,10 +1321,10 @@ RelationDefinition.hasOne = function (modelFrom, modelTo, params) { modelFrom: modelFrom, keyFrom: pk, keyTo: fk, - discriminator: discriminator, modelTo: modelTo, properties: params.properties, - options: params.options + options: params.options, + polymorphic: polymorphic }); modelFrom.dataSource.defineForeignKey(modelTo.modelName, fk, modelFrom.modelName); diff --git a/test/relations.test.js b/test/relations.test.js index 458382b4..7db9957f 100644 --- a/test/relations.test.js +++ b/test/relations.test.js @@ -1030,6 +1030,40 @@ describe('relations', function () { }); }); }); + + it('should create polymorphic item through relation scope', function (done) { + Picture.findById(anotherPicture.id, function(err, p) { + p.authors.create({ name: 'Author 3' }, function(err, a) { + should.not.exist(err); + author = a; + author.name.should.equal('Author 3'); + done(); + }); + }); + }); + + it('should create polymorphic through model - new author', function (done) { + PictureLink.findOne({ where: { + pictureId: anotherPicture.id, imageableId: author.id, imageableType: 'Author' + } }, function(err, link) { + should.not.exist(err); + link.pictureId.should.eql(anotherPicture.id); + link.imageableId.should.eql(author.id); + link.imageableType.should.equal('Author'); + done(); + }); + }); + + it('should find polymorphic items - new author', function (done) { + Author.findById(author.id, function(err, author) { + author.pictures(function(err, pics) { + pics.should.have.length(1); + pics[0].id.should.eql(anotherPicture.id); + pics[0].name.should.equal('Example'); + done(); + }); + }); + }); }); From 96a276a12b4a75a85b1092ccbd4ad0b31c5fd056 Mon Sep 17 00:00:00 2001 From: Fabien Franzen Date: Fri, 15 Aug 2014 15:12:02 +0200 Subject: [PATCH 7/8] Refactor embedsMany - auto-save parent With this change, saving an embedded model now correctly updates the parent model. Before, a separate `save()` call on the parent was required, contrary to other relation types. --- lib/relation-definition.js | 67 ++++++++++++++++++++++++++++++-------- test/relations.test.js | 34 ++++++++++++++----- 2 files changed, 80 insertions(+), 21 deletions(-) diff --git a/lib/relation-definition.js b/lib/relation-definition.js index f824a8a5..e11b28bc 100644 --- a/lib/relation-definition.js +++ b/lib/relation-definition.js @@ -1652,11 +1652,53 @@ RelationDefinition.embedsMany = function embedsMany(modelFrom, modelTo, params) return definition; }; +EmbedsMany.prototype.prepareEmbeddedInstance = function(inst) { + if (inst && inst.triggerParent !== 'function') { + var self = this; + var propertyName = this.definition.keyFrom; + var modelInstance = this.modelInstance; + inst.triggerParent = function(actionName, callback) { + if (actionName === 'save' || actionName === 'destroy') { + var embeddedList = self.embeddedList(); + if (actionName === 'destroy') { + var index = embeddedList.indexOf(inst); + if (index > -1) embeddedList.splice(index, 1); + } + modelInstance.updateAttribute(propertyName, + embeddedList, function(err, modelInst) { + callback(err, err ? null : modelInst); + }); + } else { + process.nextTick(callback); + } + }; + var originalTrigger = inst.trigger; + inst.trigger = function(actionName, work, data, callback) { + if (typeof work === 'function') { + var originalWork = work; + work = function(next) { + originalWork.call(this, function(done) { + inst.triggerParent(actionName, function(err, inst) { + next(done); // TODO [fabien] - error handling? + }); + }); + }; + } + originalTrigger.call(this, actionName, work, data, callback); + }; + } +}; + +EmbedsMany.prototype.embeddedList = function(modelInstance) { + modelInstance = modelInstance || this.modelInstance; + var embeddedList = modelInstance[this.definition.keyFrom] || []; + embeddedList.forEach(this.prepareEmbeddedInstance.bind(this)); + return embeddedList; +}; + EmbedsMany.prototype.related = function(receiver, scopeParams, condOrRefresh, cb) { var modelTo = this.definition.modelTo; - var propertyName = this.definition.keyFrom; var modelInstance = this.modelInstance; - var self = receiver; var actualCond = {}; var actualRefresh = false; @@ -1673,9 +1715,9 @@ EmbedsMany.prototype.related = function(receiver, scopeParams, condOrRefresh, cb throw new Error('Method can be only called with one or two arguments'); } - var embeddedList = self[propertyName] || []; + var embeddedList = this.embeddedList(receiver); - this.definition.applyScope(modelInstance, actualCond); + this.definition.applyScope(receiver, actualCond); var params = mergeQuery(actualCond, scopeParams); @@ -1697,10 +1739,9 @@ EmbedsMany.prototype.related = function(receiver, scopeParams, condOrRefresh, cb EmbedsMany.prototype.findById = function (fkId, cb) { var pk = this.definition.keyTo; var modelTo = this.definition.modelTo; - var propertyName = this.definition.keyFrom; var modelInstance = this.modelInstance; - var embeddedList = modelInstance[propertyName] || []; + var embeddedList = this.embeddedList(); var find = function(id) { for (var i = 0; i < embeddedList.length; i++) { @@ -1740,7 +1781,7 @@ EmbedsMany.prototype.updateById = function (fkId, data, cb) { var propertyName = this.definition.keyFrom; var modelInstance = this.modelInstance; - var embeddedList = modelInstance[propertyName] || []; + var embeddedList = this.embeddedList(); var inst = this.findById(fkId); @@ -1774,7 +1815,7 @@ EmbedsMany.prototype.destroyById = function (fkId, cb) { var propertyName = this.definition.keyFrom; var modelInstance = this.modelInstance; - var embeddedList = modelInstance[propertyName] || []; + var embeddedList = this.embeddedList(); var inst = (fkId instanceof modelTo) ? fkId : this.findById(fkId); @@ -1799,10 +1840,9 @@ EmbedsMany.prototype.unset = EmbedsMany.prototype.destroyById; EmbedsMany.prototype.at = function (index, cb) { var modelTo = this.definition.modelTo; - var propertyName = this.definition.keyFrom; var modelInstance = this.modelInstance; - var embeddedList = modelInstance[propertyName] || []; + var embeddedList = this.embeddedList(); var item = embeddedList[parseInt(index)]; item = (item instanceof modelTo) ? item : null; @@ -1829,7 +1869,7 @@ EmbedsMany.prototype.create = function (targetModelData, cb) { } targetModelData = targetModelData || {}; - var embeddedList = modelInstance[propertyName] || []; + var embeddedList = this.embeddedList(); var inst = this.build(targetModelData); @@ -1850,11 +1890,10 @@ EmbedsMany.prototype.create = function (targetModelData, cb) { EmbedsMany.prototype.build = function(targetModelData) { var pk = this.definition.keyTo; var modelTo = this.definition.modelTo; - var propertyName = this.definition.keyFrom; var modelInstance = this.modelInstance; var autoId = this.definition.options.autoId !== false; - var embeddedList = modelInstance[propertyName] || []; + var embeddedList = this.embeddedList(); targetModelData = targetModelData || {}; @@ -1879,6 +1918,8 @@ EmbedsMany.prototype.build = function(targetModelData) { embeddedList.push(inst); } + this.prepareEmbeddedInstance(inst); + return inst; }; diff --git a/test/relations.test.js b/test/relations.test.js index 7db9957f..55a8e4cf 100644 --- a/test/relations.test.js +++ b/test/relations.test.js @@ -1758,14 +1758,12 @@ describe('relations', function () { category = cat; var link = cat.items.build({ notes: 'Some notes...' }); link.product.create({ name: 'Product 1' }, function(err, p) { - cat.save(function(err, cat) { // save parent object! - cat.links[0].id.should.eql(p.id); - cat.links[0].name.should.equal('Product 1'); // denormalized - cat.links[0].notes.should.equal('Some notes...'); - cat.items.at(0).should.equal(cat.links[0]); - done(); - }); - }) + cat.links[0].id.should.eql(p.id); + cat.links[0].name.should.equal('Product 1'); // denormalized + cat.links[0].notes.should.equal('Some notes...'); + cat.items.at(0).should.equal(cat.links[0]); + done(); + }); }); }); @@ -1787,6 +1785,26 @@ describe('relations', function () { }); }); + it('should update items on scope - and save parent', function(done) { + Category.findById(category.id, function(err, cat) { + var link = cat.items.at(0); + link.updateAttributes({notes: 'Updated notes...'}, function(err, link) { + link.notes.should.equal('Updated notes...'); + done(); + }); + }); + }); + + it('should find items on scope - verify update', function(done) { + Category.findById(category.id, function(err, cat) { + cat.name.should.equal('Category B'); + cat.links.toObject().should.eql([ + {id: 5, name: 'Product 1', notes: 'Updated notes...'} + ]); + done(); + }); + }); + }); describe('embedsMany - polymorphic relations', function () { From 94310549cf08c8b2458160ce8df852b3ad2eb447 Mon Sep 17 00:00:00 2001 From: Fabien Franzen Date: Fri, 15 Aug 2014 15:24:00 +0200 Subject: [PATCH 8/8] Implement embedded.destroy() integration --- test/relations.test.js | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/test/relations.test.js b/test/relations.test.js index 55a8e4cf..bc0a09d4 100644 --- a/test/relations.test.js +++ b/test/relations.test.js @@ -1805,6 +1805,23 @@ describe('relations', function () { }); }); + it('should remove items from scope - and save parent', function(done) { + Category.findById(category.id, function(err, cat) { + cat.items.at(0).destroy(function(err, link) { + cat.links.should.eql([]); + done(); + }); + }); + }); + + it('should find items on scope - verify destroy', function(done) { + Category.findById(category.id, function(err, cat) { + cat.name.should.equal('Category B'); + cat.links.should.eql([]); + done(); + }); + }); + }); describe('embedsMany - polymorphic relations', function () {