From 8193f402bbdab896b5d464c070a8df6f3e3fe33f Mon Sep 17 00:00:00 2001 From: Fabien Franzen Date: Fri, 15 Aug 2014 12:55:10 +0200 Subject: [PATCH] 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(); + }); + }); + }); });