Merge pull request #295 from fabien/fix/embeds-many

Various improvements to embedded relations

lib/relation-definition.js

@@ -1783,29 +1783,53 @@ EmbedsOne.prototype.create = function (targetModelData, cb) {
   
   var inst = this.build(targetModelData);
   
-  var err = inst.isValid() ? null : new ValidationError(inst);
+  var updateEmbedded = function() {
-  
-  if (err) {
-    return process.nextTick(function() {
-      cb(err); 
-    });
-  }
-  
     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) {
   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;
   
@@ -1878,7 +1902,20 @@ RelationDefinition.embedsMany = function embedsMany(modelFrom, modelTo, params)
   });
   
   if (typeof modelTo.dataSource.connector.generateId !== 'function') {
-    modelTo.validatesPresenceOf(idName); // unique id is required
+    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) {
@@ -1901,13 +1938,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];
+            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');
           }
@@ -2193,28 +2234,39 @@ EmbedsMany.prototype.create = function (targetModelData, cb) {
   
   var inst = this.build(targetModelData);
   
-  var err = inst.isValid() ? null : new ValidationError(inst);
+  var updateEmbedded = function() {
-  
-  if (err) {
-    return process.nextTick(function() {
-      cb(err); 
-    });
-  }
-  
     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);
+      updateEmbedded();
+    });
+  } else {
+    var err = inst.isValid() ? null : new ValidationError(inst);
+    if (err) {
+      process.nextTick(function() {
+        cb(err); 
+      });
+    } else {
+      updateEmbedded();
+    }
+  }
 };
 
 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;
-  var pkProp = modelTo.definition.properties[pk]
+  var pkProp = modelTo.definition.properties[pk];
   var pkType = pkProp && pkProp.type;
   
   var embeddedList = this.embeddedList();
@@ -2222,6 +2274,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) { 

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 () {
@@ -1849,6 +1853,81 @@ 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('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;
@@ -2039,6 +2118,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();
@@ -2210,6 +2328,134 @@ describe('relations', function () {
     
   });
   
+  describe('embedsMany - persisted model', 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 = getMemoryDataSource();
+      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) {
+      // 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();
+      });
+    });
+    
+    it('should create individual items (1)', function(done) {
+      Address.create({ street: 'Street 1' }, function(err, inst) {
+        inst.id.should.equal(2);
+        address1 = inst;
+        done();
+      });
+    });
+    
+    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(2);
+          address.street.should.equal('Street 1');
+          p.addressList.create(address2.toObject(), function(err, address) {
+            should.not.exist(err);
+            address.id.should.eql(3);
+            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 4' }, function(err, address) {
+          should.not.exist(err);
+          address.id.should.equal(5); // in Address sequence, correct offset
+          address.street.should.equal('Street 4');
+          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 4');
+          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.not.exist(err);
+          address.street.should.equal('Street 1');
+          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;