From 4c6f35d23d44654fbafd8e6c407a0c99bf6d1ad4 Mon Sep 17 00:00:00 2001 From: Fabien Franzen Date: Sun, 7 Sep 2014 12:17:42 +0200 Subject: [PATCH 1/6] Only check id as part of embedsMany relation --- lib/relation-definition.js | 30 ++++++++++--- test/relations.test.js | 87 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 111 insertions(+), 6 deletions(-) diff --git a/lib/relation-definition.js b/lib/relation-definition.js index f6188a02..a36c4eac 100644 --- a/lib/relation-definition.js +++ b/lib/relation-definition.js @@ -1865,12 +1865,26 @@ RelationDefinition.embedsMany = function embedsMany(modelFrom, modelTo, params) embed: true }); - modelFrom.dataSource.defineProperty(modelFrom.modelName, propertyName, { + modelFrom.dataSource.defineProperty(modelFrom.modelName, propertyName, { type: [modelTo], default: function() { return []; } }); - if (typeof modelTo.dataSource.connector.generateId !== 'function') { - modelTo.validatesPresenceOf(idName); // unique id is required + if (typeof modelTo.dataSource.connector.generateId !== 'function' + || !modelFrom.definition.settings.idInjection) { + modelFrom.validate(propertyName, function(err) { + var self = this; + var embeddedList = this[propertyName] || []; + var hasErrors = false; + embeddedList.forEach(function(item, idx) { + if (item instanceof modelTo && item[idName] == undefined) { + hasErrors = true; + var msg = 'contains invalid item at index `' + idx + '`:'; + msg += ' `' + idName + '` is blank'; + self.errors.add(propertyName, msg, 'invalid'); + } + }); + if (hasErrors) err(false); + }); } if (!params.polymorphic) { @@ -1893,13 +1907,17 @@ RelationDefinition.embedsMany = function embedsMany(modelFrom, modelTo, params) var self = this; var embeddedList = this[propertyName] || []; var hasErrors = false; - embeddedList.forEach(function(item) { + embeddedList.forEach(function(item, idx) { if (item instanceof modelTo) { if (!item.isValid()) { hasErrors = true; - var id = item[idName] || '(blank)'; + var id = item[idName]; var first = Object.keys(item.errors)[0]; - var msg = 'contains invalid item: `' + id + '`'; + if (id) { + var msg = 'contains invalid item: `' + id + '`'; + } else { + var msg = 'contains invalid item at index `' + idx + '`'; + } msg += ' (`' + first + '` ' + item.errors[first] + ')'; self.errors.add(propertyName, msg, 'invalid'); } diff --git a/test/relations.test.js b/test/relations.test.js index b99f31ba..3b9f7436 100644 --- a/test/relations.test.js +++ b/test/relations.test.js @@ -2183,6 +2183,93 @@ describe('relations', function () { }); + describe('embedsMany - persisted model', function () { + + var address0, address1, address2; + + before(function (done) { + db = getSchema(); + Person = db.define('Person', {name: String}); + Address = db.define('Address', {street: String}); + Address.validatesPresenceOf('street'); + + db.automigrate(function () { + Person.destroyAll(done); + }); + }); + + it('can be declared', function (done) { + Person.embedsMany(Address); + db.automigrate(done); + }); + + it('should create individual items (0)', function(done) { + Address.create({ street: 'Street 0' }, function(err, inst) { + address0 = inst; + done(); + }); + }); + + it('should create individual items (1)', function(done) { + Address.create({ street: 'Street 1' }, function(err, inst) { + address1 = inst; + done(); + }); + }); + + it('should create individual items (2)', function(done) { + Address.create({ street: 'Street 2' }, function(err, inst) { + address2 = inst; + done(); + }); + }); + + it('should create embedded items on scope', function(done) { + Person.create({ name: 'Fred' }, function(err, p) { + p.addressList.create(address1.toObject(), function(err, address) { + should.not.exist(err); + address.id.should.eql(address1.id); + address1.street.should.equal('Street 1'); + p.addressList.create(address2.toObject(), function(err, address) { + should.not.exist(err); + address.id.should.eql(address2.id); + address2.street.should.equal('Street 2'); + done(); + }); + }); + }); + }); + + it('should validate embedded items on scope - id', function(done) { + Person.create({ name: 'Wilma' }, function(err, p) { + p.addressList.create({ id: null, street: 'Street 1' }, function(err, address) { + should.exist(err); + err.name.should.equal('ValidationError'); + err.details.codes.addresses.should.eql(['invalid']); + var expected = 'The `Person` instance is not valid. '; + expected += 'Details: `addresses` contains invalid item at index `0`: `id` is blank.'; + err.message.should.equal(expected); + done(); + }); + }); + }); + + it('should validate embedded items on scope - street', function(done) { + Person.create({ name: 'Wilma' }, function(err, p) { + p.addressList.create({ id: 1234 }, function(err, address) { + should.exist(err); + err.name.should.equal('ValidationError'); + err.details.codes.street.should.eql(['presence']); + var expected = 'The `Address` instance is not valid. '; + expected += 'Details: `street` can\'t be blank.'; + err.message.should.equal(expected); + done(); + }); + }); + }); + + }); + describe('embedsMany - relations, scope and properties', function () { var category, job1, job2, job3; From ef816d490ab35d5e0962b32d4599dc1074808069 Mon Sep 17 00:00:00 2001 From: Fabien Franzen Date: Sun, 7 Sep 2014 12:44:45 +0200 Subject: [PATCH 2/6] More tests for embedsMany with persistent model --- lib/relation-definition.js | 3 +-- test/relations.test.js | 34 ++++++++++++++++++++++++++++++---- 2 files changed, 31 insertions(+), 6 deletions(-) diff --git a/lib/relation-definition.js b/lib/relation-definition.js index a36c4eac..5c1681d0 100644 --- a/lib/relation-definition.js +++ b/lib/relation-definition.js @@ -1869,8 +1869,7 @@ RelationDefinition.embedsMany = function embedsMany(modelFrom, modelTo, params) type: [modelTo], default: function() { return []; } }); - if (typeof modelTo.dataSource.connector.generateId !== 'function' - || !modelFrom.definition.settings.idInjection) { + if (typeof modelTo.dataSource.connector.generateId !== 'function') { modelFrom.validate(propertyName, function(err) { var self = this; var embeddedList = this[propertyName] || []; diff --git a/test/relations.test.js b/test/relations.test.js index 3b9f7436..b145b662 100644 --- a/test/relations.test.js +++ b/test/relations.test.js @@ -2186,6 +2186,7 @@ describe('relations', function () { describe('embedsMany - persisted model', function () { var address0, address1, address2; + var person; before(function (done) { db = getSchema(); @@ -2199,7 +2200,7 @@ describe('relations', function () { }); it('can be declared', function (done) { - Person.embedsMany(Address); + Person.embedsMany(Address, {scope: {order: 'street'}}); db.automigrate(done); }); @@ -2224,22 +2225,47 @@ describe('relations', function () { }); }); - it('should create embedded items on scope', function(done) { + it('should add embedded items on scope', function(done) { Person.create({ name: 'Fred' }, function(err, p) { + person = p; p.addressList.create(address1.toObject(), function(err, address) { should.not.exist(err); address.id.should.eql(address1.id); - address1.street.should.equal('Street 1'); + address.street.should.equal('Street 1'); p.addressList.create(address2.toObject(), function(err, address) { should.not.exist(err); address.id.should.eql(address2.id); - address2.street.should.equal('Street 2'); + address.street.should.equal('Street 2'); done(); }); }); }); }); + it('should create embedded items on scope', function(done) { + Person.findById(person.id, function(err, p) { + p.addressList.create({ street: 'Street 3' }, function(err, address) { + should.not.exist(err); + address.should.have.property('id'); // not within Address seq! + address.street.should.equal('Street 3'); + done(); + }); + }); + }); + + it('should have embedded items on scope', function(done) { + Person.findById(person.id, function(err, p) { + p.addressList(function(err, addresses) { + should.not.exist(err); + addresses.should.have.length(3); + addresses[0].street.should.equal('Street 1'); + addresses[1].street.should.equal('Street 2'); + addresses[2].street.should.equal('Street 3'); + done(); + }); + }); + }); + it('should validate embedded items on scope - id', function(done) { Person.create({ name: 'Wilma' }, function(err, p) { p.addressList.create({ id: null, street: 'Street 1' }, function(err, address) { From 95764232b9e69da31ca71145013df883df1390b0 Mon Sep 17 00:00:00 2001 From: Fabien Franzen Date: Sun, 7 Sep 2014 12:59:47 +0200 Subject: [PATCH 3/6] Introduce embedsMany persistent: true option When set, instead of only embedding the model (on creation) it will be persisted first, and subsequently embedded. --- lib/relation-definition.js | 29 +++++++++++++++-------- test/relations.test.js | 47 ++++++++++++++++++++++++++------------ 2 files changed, 53 insertions(+), 23 deletions(-) diff --git a/lib/relation-definition.js b/lib/relation-definition.js index 5c1681d0..dbfc4af5 100644 --- a/lib/relation-definition.js +++ b/lib/relation-definition.js @@ -2199,24 +2199,34 @@ EmbedsMany.prototype.create = function (targetModelData, cb) { var inst = this.build(targetModelData); - var err = inst.isValid() ? null : new ValidationError(inst); - - if (err) { - return process.nextTick(function() { - cb(err); + var updateEmbeddedList = function() { + modelInstance.updateAttribute(propertyName, + embeddedList, function(err, modelInst) { + cb(err, err ? null : inst); }); } - modelInstance.updateAttribute(propertyName, - embeddedList, function(err, modelInst) { - cb(err, err ? null : inst); - }); + if (this.definition.options.persistent) { + inst.save(function(err) { // will validate + if (err) return cb(err, inst); + updateEmbeddedList(); + }); + } else { + var err = inst.isValid() ? null : new ValidationError(inst); + if (err) { + return process.nextTick(function() { + cb(err); + }); + } + updateEmbeddedList(); + } }; EmbedsMany.prototype.build = function(targetModelData) { var modelTo = this.definition.modelTo; var modelInstance = this.modelInstance; var forceId = this.definition.options.forceId; + var persistent = this.definition.options.persistent; var connector = modelTo.dataSource.connector; var pk = this.definition.keyTo; @@ -2228,6 +2238,7 @@ EmbedsMany.prototype.build = function(targetModelData) { targetModelData = targetModelData || {}; var assignId = (forceId || targetModelData[pk] === undefined); + assignId = assignId && !persistent; if (assignId && pkType === Number) { var ids = embeddedList.map(function(m) { diff --git a/test/relations.test.js b/test/relations.test.js index b145b662..cd131762 100644 --- a/test/relations.test.js +++ b/test/relations.test.js @@ -13,6 +13,10 @@ var getTransientDataSource = function(settings) { return new DataSource('transient', settings, db.modelBuilder); }; +var getMemoryDataSource = function(settings) { + return new DataSource('memory', settings, db.modelBuilder); +}; + describe('relations', function () { describe('hasMany', function () { @@ -2188,8 +2192,12 @@ describe('relations', function () { var address0, address1, address2; var person; + // This test spefically uses the Memory connector + // in order to test the use of the auto-generated + // id, in the sequence of the related model. + before(function (done) { - db = getSchema(); + db = getMemoryDataSource(); Person = db.define('Person', {name: String}); Address = db.define('Address', {street: String}); Address.validatesPresenceOf('street'); @@ -2200,12 +2208,18 @@ describe('relations', function () { }); it('can be declared', function (done) { - Person.embedsMany(Address, {scope: {order: 'street'}}); + // to save related model itself, set + // persistent: true + Person.embedsMany(Address, { + scope: {order: 'street'}, + options: {persistent: true} + }); db.automigrate(done); }); it('should create individual items (0)', function(done) { Address.create({ street: 'Street 0' }, function(err, inst) { + inst.id.should.equal(1); // offset sequence address0 = inst; done(); }); @@ -2213,6 +2227,7 @@ describe('relations', function () { it('should create individual items (1)', function(done) { Address.create({ street: 'Street 1' }, function(err, inst) { + inst.id.should.equal(2); address1 = inst; done(); }); @@ -2220,21 +2235,29 @@ describe('relations', function () { it('should create individual items (2)', function(done) { Address.create({ street: 'Street 2' }, function(err, inst) { + inst.id.should.equal(3); address2 = inst; done(); }); }); + it('should create individual items (3)', function(done) { + Address.create({ street: 'Street 3' }, function(err, inst) { + inst.id.should.equal(4); // offset sequence + done(); + }); + }); + it('should add embedded items on scope', function(done) { Person.create({ name: 'Fred' }, function(err, p) { person = p; p.addressList.create(address1.toObject(), function(err, address) { should.not.exist(err); - address.id.should.eql(address1.id); + address.id.should.eql(2); address.street.should.equal('Street 1'); p.addressList.create(address2.toObject(), function(err, address) { should.not.exist(err); - address.id.should.eql(address2.id); + address.id.should.eql(3); address.street.should.equal('Street 2'); done(); }); @@ -2244,10 +2267,10 @@ describe('relations', function () { it('should create embedded items on scope', function(done) { Person.findById(person.id, function(err, p) { - p.addressList.create({ street: 'Street 3' }, function(err, address) { + p.addressList.create({ street: 'Street 4' }, function(err, address) { should.not.exist(err); - address.should.have.property('id'); // not within Address seq! - address.street.should.equal('Street 3'); + address.id.should.equal(5); // in Address sequence, correct offset + address.street.should.equal('Street 4'); done(); }); }); @@ -2260,7 +2283,7 @@ describe('relations', function () { addresses.should.have.length(3); addresses[0].street.should.equal('Street 1'); addresses[1].street.should.equal('Street 2'); - addresses[2].street.should.equal('Street 3'); + addresses[2].street.should.equal('Street 4'); done(); }); }); @@ -2269,12 +2292,8 @@ describe('relations', function () { it('should validate embedded items on scope - id', function(done) { Person.create({ name: 'Wilma' }, function(err, p) { p.addressList.create({ id: null, street: 'Street 1' }, function(err, address) { - should.exist(err); - err.name.should.equal('ValidationError'); - err.details.codes.addresses.should.eql(['invalid']); - var expected = 'The `Person` instance is not valid. '; - expected += 'Details: `addresses` contains invalid item at index `0`: `id` is blank.'; - err.message.should.equal(expected); + should.not.exist(err); + address.street.should.equal('Street 1'); done(); }); }); From 21e1083e88fc53b2c4489d446c6ade783615d3ec Mon Sep 17 00:00:00 2001 From: Fabien Franzen Date: Sun, 7 Sep 2014 13:10:23 +0200 Subject: [PATCH 4/6] Implemented persistent: true option for embedsOne --- lib/relation-definition.js | 41 ++++++++++++++++++++++------------- test/relations.test.js | 44 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 70 insertions(+), 15 deletions(-) diff --git a/lib/relation-definition.js b/lib/relation-definition.js index dbfc4af5..54474635 100644 --- a/lib/relation-definition.js +++ b/lib/relation-definition.js @@ -1775,18 +1775,28 @@ EmbedsOne.prototype.create = function (targetModelData, cb) { var inst = this.build(targetModelData); - var err = inst.isValid() ? null : new ValidationError(inst); - - if (err) { - return process.nextTick(function() { - cb(err); + var updateEmbedded = function() { + modelInstance.updateAttribute(propertyName, + inst, function(err) { + cb(err, err ? null : inst); }); - } + }; - modelInstance.updateAttribute(propertyName, - inst, function(err) { - cb(err, err ? null : inst); - }); + if (this.definition.options.persistent) { + inst.save(function(err) { // will validate + if (err) return cb(err, inst); + updateEmbedded(); + }); + } else { + var err = inst.isValid() ? null : new ValidationError(inst); + if (err) { + process.nextTick(function() { + cb(err); + }); + } else { + updateEmbedded(); + } + } }; EmbedsOne.prototype.build = function (targetModelData) { @@ -2199,26 +2209,27 @@ EmbedsMany.prototype.create = function (targetModelData, cb) { var inst = this.build(targetModelData); - var updateEmbeddedList = function() { + var updateEmbedded = function() { modelInstance.updateAttribute(propertyName, embeddedList, function(err, modelInst) { cb(err, err ? null : inst); }); - } + }; if (this.definition.options.persistent) { inst.save(function(err) { // will validate if (err) return cb(err, inst); - updateEmbeddedList(); + updateEmbedded(); }); } else { var err = inst.isValid() ? null : new ValidationError(inst); if (err) { - return process.nextTick(function() { + process.nextTick(function() { cb(err); }); + } else { + updateEmbedded(); } - updateEmbeddedList(); } }; diff --git a/test/relations.test.js b/test/relations.test.js index cd131762..7e64cd7e 100644 --- a/test/relations.test.js +++ b/test/relations.test.js @@ -1833,6 +1833,50 @@ describe('relations', function () { }); + describe('embedsOne - persisted model', function () { + + // This test spefically uses the Memory connector + // in order to test the use of the auto-generated + // id, in the sequence of the related model. + + before(function () { + db = getMemoryDataSource(); + Person = db.define('Person', {name: String}); + Passport = db.define('Passport', + {name:{type:'string', required: true}} + ); + }); + + it('can be declared using embedsOne method', function (done) { + Person.embedsOne(Passport, { + options: {persistent: true} + }); + db.automigrate(done); + }); + + it('should create an item - to offset id', function(done) { + Passport.create({name:'Wilma'}, function(err, p) { + should.not.exist(err); + p.id.should.equal(1); + p.name.should.equal('Wilma'); + done(); + }); + }); + + it('should create an embedded item on scope', function(done) { + Person.create({name: 'Fred'}, function(err, p) { + should.not.exist(err); + p.passportItem.create({name: 'Fredric'}, function(err, passport) { + should.not.exist(err); + p.passport.id.should.eql(2); + p.passport.name.should.equal('Fredric'); + done(); + }); + }); + }); + + }); + describe('embedsMany', function () { var address1, address2; From e441924fa3bc74cc4d5c98c149a77818edb979de Mon Sep 17 00:00:00 2001 From: Fabien Franzen Date: Sun, 7 Sep 2014 13:24:05 +0200 Subject: [PATCH 5/6] Allow embedsOne to use auto-generated id (from connector) If the connector has a `generateId` method - like Transient - it can be used to set the id (when the property has been set to `generated: true`). --- lib/relation-definition.js | 16 +++++++++++++++- test/relations.test.js | 31 +++++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 1 deletion(-) diff --git a/lib/relation-definition.js b/lib/relation-definition.js index 54474635..0637ef9f 100644 --- a/lib/relation-definition.js +++ b/lib/relation-definition.js @@ -1803,11 +1803,25 @@ EmbedsOne.prototype.build = function (targetModelData) { var modelTo = this.definition.modelTo; var modelInstance = this.modelInstance; var propertyName = this.definition.keyFrom; + var forceId = this.definition.options.forceId; + var persistent = this.definition.options.persistent; + var connector = modelTo.dataSource.connector; targetModelData = targetModelData || {}; this.definition.applyProperties(modelInstance, targetModelData); + var pk = this.definition.keyTo; + var pkProp = modelTo.definition.properties[pk]; + + var assignId = (forceId || targetModelData[pk] === undefined); + assignId = assignId && !persistent && (pkProp && pkProp.generated); + + if (assignId && typeof connector.generateId === 'function') { + var id = connector.generateId(modelTo.modelName, targetModelData, pk); + targetModelData[pk] = id; + } + var embeddedInstance = new modelTo(targetModelData); modelInstance[propertyName] = embeddedInstance; @@ -2241,7 +2255,7 @@ EmbedsMany.prototype.build = function(targetModelData) { var connector = modelTo.dataSource.connector; var pk = this.definition.keyTo; - var pkProp = modelTo.definition.properties[pk] + var pkProp = modelTo.definition.properties[pk]; var pkType = pkProp && pkProp.type; var embeddedList = this.embeddedList(); diff --git a/test/relations.test.js b/test/relations.test.js index 7e64cd7e..57987a76 100644 --- a/test/relations.test.js +++ b/test/relations.test.js @@ -1877,6 +1877,37 @@ describe('relations', function () { }); + describe('embedsOne - generated id', function () { + + before(function () { + tmp = getTransientDataSource(); + db = getSchema(); + Person = db.define('Person', {name: String}); + Passport = tmp.define('Passport', + {id: {type:'string', id: true, generated:true}}, + {name: {type:'string', required: true}} + ); + }); + + it('can be declared using embedsOne method', function (done) { + Person.embedsOne(Passport); + db.automigrate(done); + }); + + it('should create an embedded item on scope', function(done) { + Person.create({name: 'Fred'}, function(err, p) { + should.not.exist(err); + p.passportItem.create({name: 'Fredric'}, function(err, passport) { + should.not.exist(err); + passport.id.should.match(/^[0-9a-fA-F]{24}$/); + p.passport.name.should.equal('Fredric'); + done(); + }); + }); + }); + + }); + describe('embedsMany', function () { var address1, address2; From d1a08ef6b31652b4b6b867d445dc11a703f7cd97 Mon Sep 17 00:00:00 2001 From: Fabien Franzen Date: Sun, 7 Sep 2014 20:54:11 +0200 Subject: [PATCH 6/6] Add test case for Numeric ids (with optional forceId) --- test/relations.test.js | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/test/relations.test.js b/test/relations.test.js index 57987a76..86884434 100644 --- a/test/relations.test.js +++ b/test/relations.test.js @@ -2091,6 +2091,45 @@ describe('relations', function () { }); + describe('embedsMany - numeric ids + forceId', function () { + + before(function (done) { + tmp = getTransientDataSource(); + db = getSchema(); + Person = db.define('Person', {name: String}); + Address = tmp.define('Address', { + id: {type: Number, id:true}, + street: String + }); + + db.automigrate(function () { + Person.destroyAll(done); + }); + }); + + it('can be declared', function (done) { + Person.embedsMany(Address, {options: {forceId: true}}); + db.automigrate(done); + }); + + it('should create embedded items on scope', function(done) { + Person.create({ name: 'Fred' }, function(err, p) { + p.addressList.create({ street: 'Street 1' }, function(err, address) { + should.not.exist(err); + address.id.should.equal(1); + p.addressList.create({ street: 'Street 2' }, function(err, address) { + address.id.should.equal(2); + p.addressList.create({ id: 12345, street: 'Street 3' }, function(err, address) { + address.id.should.equal(3); + done(); + }); + }); + }); + }); + }); + + }); + describe('embedsMany - explicit ids', function () { before(function (done) { tmp = getTransientDataSource();