Merge pull request #1176 from mickhansen/has-many-bug

Fixes some bugs related to create/setters/adders and timestamps: false

lib/associations/has-many-double-linked.js

@@ -121,15 +121,23 @@ module.exports = (function() {
       if (!newObj) {
         obsoleteAssociations.push(old)
       } else if (Object(targetAssociation.through) === targetAssociation.through) {
+        var throughAttributes = newObj[self.association.through.name];
+        // Quick-fix for subtle bug when using existing objects that might have the through model attached (not as an attribute object)
+        if (throughAttributes instanceof self.association.through.DAO) {
+          throughAttributes = {};
+        }
+
         var changedAssociation = {
           where: {},
-          attributes: Utils._.defaults({}, newObj[self.association.through.name], defaultAttributes)
+          attributes: Utils._.defaults({}, throughAttributes, defaultAttributes)
         }
 
         changedAssociation.where[self.association.identifier] = self.instance[self.association.identifier] || self.instance.id
         changedAssociation.where[foreignIdentifier] = newObj[foreignIdentifier] || newObj.id
 
-        changedAssociations.push(changedAssociation)
+        if (Object.keys(changedAssociation.attributes).length) {
+          changedAssociations.push(changedAssociation)
+        }
       }
     })
 
@@ -187,11 +195,15 @@ module.exports = (function() {
     attributes[this.association.identifier] = ((sourceKeys.length === 1) ? this.instance[sourceKeys[0]] : this.instance.id)
     attributes[foreignIdentifier] = ((targetKeys.length === 1) ? newAssociation[targetKeys[0]] : newAssociation.id)
 
-    if (exists) { // implies hasJoinTableModel === true
+    if (exists) {
       var where = attributes
-      attributes = Utils._.defaults({}, newAssociation[association.through.name], additionalAttributes)
+      attributes = Utils._.defaults({}, newAssociation[targetAssociation.through.name], additionalAttributes)
 
-      targetAssociation.through.update(attributes, where).proxy(emitterProxy)
+      if (Object.keys(attributes).length) {
+        targetAssociation.through.update(attributes, where).proxy(emitterProxy)
+      } else {
+        emitterProxy.emit('success')
+      }
     } else {
       attributes = Utils._.defaults(attributes, newAssociation[targetAssociation.through.name], additionalAttributes)
 

lib/associations/has-many.js

@@ -241,12 +241,7 @@ module.exports = (function() {
             var Class = self.doubleLinked ? HasManyMultiLinked : HasManySingleLinked
             new Class(self, instance).injectSetter(emitter, oldAssociatedObjects, newAssociatedObjects, defaultAttributes)
           })
-          .error(function(err) {
-            emitter.emit('error', err)
-          })
-          .on('sql', function(sql) {
-            emitter.emit('sql', sql)
-          })
+          .proxy(emitter, {events: ['error', 'sql']})
       }).run()
     }
 
@@ -260,9 +255,9 @@ module.exports = (function() {
       where[newAssociatedObject.daoFactory.tableName+'.'+primaryKey] = newAssociatedObject[primaryKey]
       return new Utils.CustomEventEmitter(function(emitter) {
         instance[self.accessors.get]({ where: where })
-          .error(function(err){ emitter.emit('error', err)})
+          .proxy(emitter, {events: ['error', 'sql']})
           .success(function(currentAssociatedObjects) {
-            if (currentAssociatedObjects.length === 0 || self.hasJoinTableModel === true) {
+            if (currentAssociatedObjects.length === 0 || Object(self.through) === self.through) {
               var Class = self.doubleLinked ? HasManyMultiLinked : HasManySingleLinked
               new Class(self, instance).injectAdder(emitter, newAssociatedObject, additionalAttributes, !!currentAssociatedObjects.length)
             } else {
@@ -274,7 +269,7 @@ module.exports = (function() {
 
     obj[this.accessors.remove] = function(oldAssociatedObject) {
       var instance = this
-      var customEventEmitter = new Utils.CustomEventEmitter(function() {
+      return new Utils.CustomEventEmitter(function(emitter) {
         instance[self.accessors.get]().success(function(currentAssociatedObjects) {
           var newAssociations = []
             , oldAssociations = []
@@ -302,12 +297,10 @@ module.exports = (function() {
 
           var run = function(err) {
             if (!!err) {
-              return customEventEmitter.emit('error', err)
+              return emitter.emit('error', err)
             }
 
-            instance[self.accessors.set](newAssociations)
-              .success(function() { customEventEmitter.emit('success', null) })
-              .error(function(err) { customEventEmitter.emit('error', err) })
+            instance[self.accessors.set](newAssociations).proxy(emitter)
           }
 
           if (oldAssociations.length > 0) {
@@ -316,8 +309,7 @@ module.exports = (function() {
             run()
           }
         })
-      })
-      return customEventEmitter.run()
+      }).run()
     }
 
     return this

lib/dialects/mariadb/query.js

@@ -88,6 +88,7 @@ module.exports = (function() {
           .on('error', function(err) {
             errorDetected = true
             self.emit('sql', self.sql)
+            err.sql = sql
             self.emit('error', err, self.callee)
           })
           .on('end', function(info) {

lib/dialects/mysql/query.js

@@ -27,6 +27,7 @@ module.exports = (function() {
       this.emit('sql', this.sql)
 
       if (err) {
+        err.sql = sql
         this.emit('error', err, this.callee)
       } else {
         this.emit('success', this.formatResults(results))

lib/dialects/postgres/query-generator.js

@@ -256,9 +256,12 @@ module.exports = (function() {
     },
 
     insertQuery: function(tableName, attrValueHash, attributes) {
+      var query
+        , valueQuery = "INSERT INTO <%= table %> (<%= attributes %>) VALUES (<%= values %>) RETURNING *;"
+        , emptyQuery = "INSERT INTO <%= table %> DEFAULT VALUES RETURNING *;"
+
       attrValueHash = Utils.removeNullValuesFromHash(attrValueHash, this.options.omitNull)
 
-      var query     = "INSERT INTO <%= table %> (<%= attributes %>) VALUES (<%= values %>) RETURNING *;"
       // Remove serials that are null or undefined, which causes an error in PG
       Utils._.forEach(attrValueHash, function(value, key, hash) {
         if (tables[tableName]) {
@@ -284,17 +287,32 @@ module.exports = (function() {
       , values:     rowValues.join(",")
       }
 
+      query = replacements.attributes.length ? valueQuery : emptyQuery
+
       return Utils._.template(query)(replacements)
     },
 
     bulkInsertQuery: function(tableName, attrValueHashes) {
       var query     = "INSERT INTO <%= table %> (<%= attributes %>) VALUES <%= tuples %> RETURNING *;"
         , tuples    = []
+        , serials   = []
+
+      Utils._.forEach(attrValueHashes, function(attrValueHash, i) {
+        if (i === 0) {
+          Utils._.forEach(attrValueHash, function(value, key, hash) {
+            if (tables[tableName] && tables[tableName][key]) {
+              if (['bigserial', 'serial'].indexOf(tables[tableName][key]) !== -1) {
+                serials.push(key)
+              }
+            }
+          })
+        }
 
-      Utils._.forEach(attrValueHashes, function(attrValueHash) {
-        removeSerialsFromHash(tableName, attrValueHash)
         tuples.push("(" +
-          Utils._.values(attrValueHash).map(function(value){
+          Utils._.map(attrValueHash, function(value, key){
+            if (serials.indexOf(key) !== -1) {
+              return 'DEFAULT';
+            }
             return this.escape(value)
           }.bind(this)).join(",") +
         ")")

lib/dialects/postgres/query.js

@@ -36,6 +36,7 @@ module.exports = (function() {
 
     query.on('error', function(err) {
       receivedError = true
+      err.sql = sql
       self.emit('error', err, self.callee)
     })
 

lib/dialects/sqlite/query-generator.js

@@ -156,7 +156,9 @@ module.exports = (function() {
     insertQuery: function(tableName, attrValueHash) {
       attrValueHash = Utils.removeNullValuesFromHash(attrValueHash, this.options.omitNull)
 
-      var query = "INSERT INTO <%= table %> (<%= attributes %>) VALUES (<%= values %>);";
+      var query
+        , valueQuery = "INSERT INTO <%= table %> (<%= attributes %>) VALUES (<%= values %>);"
+        , emptyQuery = "INSERT INTO <%= table %> DEFAULT VALUES;"
 
       var replacements  = {
         table: this.quoteIdentifier(tableName),
@@ -166,6 +168,7 @@ module.exports = (function() {
         }.bind(this)).join(",")
       }
 
+      query = replacements.attributes.length ? valueQuery : emptyQuery
       return Utils._.template(query)(replacements)
     },
 

lib/dialects/sqlite/query.js

@@ -44,6 +44,7 @@ module.exports = (function() {
             self.emit('sql', self.sql)
 
             if (err) {
+              err.sql = self.sql
               onFailure.call(self, err)
             } else {
               this.columnTypes = columnTypes

test/associations/has-many.test.js

@@ -766,8 +766,6 @@ describe(Support.getTestDialectTeaser("HasMany"), function() {
       })
 
       describe('inserting in join table', function () {
-
-
         describe('add', function () {
           it('should insert data provided on the object into the join table', function (done) {
             var self = this
@@ -800,6 +798,32 @@ describe(Support.getTestDialectTeaser("HasMany"), function() {
               })
             })
           })
+
+          it('should be able to add twice (second call result in UPDATE call) without any attributes (and timestamps off) on the through model', function (done) {
+            var Worker = this.sequelize.define('Worker', {}, {timestamps: false})
+              , Task = this.sequelize.define('Task', {}, {timestamps: false})
+              , WorkerTasks = this.sequelize.define('WorkerTasks', {}, {timestamps: false})
+
+            Worker.hasMany(Task, { through: WorkerTasks })
+            Task.hasMany(Worker, { through: WorkerTasks })
+
+            this.sequelize.sync().done(function(err) { 
+              expect(err).not.to.be.ok
+              Worker.create().done(function (err, worker) {
+                expect(err).not.to.be.ok
+                Task.create().done(function (err, task) {
+                  expect(err).not.to.be.ok
+                  worker.addTask(task).done(function (err) {
+                    expect(err).not.to.be.ok
+                    worker.addTask(task).done(function (err) {
+                      expect(err).not.to.be.ok
+                      done()
+                    })
+                  })
+                })
+              })
+            })
+          })
         })
 
         describe('set', function () {
@@ -832,6 +856,68 @@ describe(Support.getTestDialectTeaser("HasMany"), function() {
               })
             })
           })
+
+          it('should be able to set twice (second call result in UPDATE calls) without any attributes (and timestamps off) on the through model', function (done) {
+            var Worker = this.sequelize.define('Worker', {}, {timestamps: false})
+              , Task = this.sequelize.define('Task', {}, {timestamps: false})
+              , WorkerTasks = this.sequelize.define('WorkerTasks', {}, {timestamps: false})
+
+            Worker.hasMany(Task, { through: WorkerTasks })
+            Task.hasMany(Worker, { through: WorkerTasks })
+
+            this.sequelize.sync().done(function(err) { 
+              expect(err).not.to.be.ok
+              Worker.create().done(function (err, worker) {
+                expect(err).not.to.be.ok
+                Task.bulkCreate([{}, {}]).done(function (err) {
+                  expect(err).not.to.be.ok
+                  Task.findAll().done(function (err, tasks) {
+                    expect(err).not.to.be.ok
+                    worker.setTasks(tasks).done(function (err) {
+                      worker.setTasks(tasks).done(function (err) {
+                        expect(err).not.to.be.ok
+                        done()
+                      })
+                    })
+                  })
+                })
+              })
+            })
+          })
+        })
+      })
+
+      describe('removing from the join table', function () {
+        it('should remove a single entry without any attributes (and timestamps off) on the through model', function (done) {
+          var Worker = this.sequelize.define('Worker', {}, {timestamps: false})
+            , Task = this.sequelize.define('Task', {}, {timestamps: false})
+            , WorkerTasks = this.sequelize.define('WorkerTasks', {}, {timestamps: false})
+
+          Worker.hasMany(Task, { through: WorkerTasks })
+          Task.hasMany(Worker, { through: WorkerTasks })
+
+          this.sequelize.sync().done(function(err) { 
+            expect(err).not.to.be.ok
+            Worker.create({}).done(function (err, worker) {
+              expect(err).not.to.be.ok
+              Task.bulkCreate([{}, {}]).done(function (err) {
+                expect(err).not.to.be.ok
+                Task.findAll().done(function (err, tasks) {
+                  expect(err).not.to.be.ok
+                  worker.setTasks(tasks).done(function (err) {
+                    worker.removeTask(tasks[0]).done(function (err) {
+                      expect(err).not.to.be.ok
+
+                      worker.getTasks().done(function (err, tasks) {
+                        expect(tasks.length).to.equal(1)
+                        done()
+                      })  
+                    })
+                  })
+                })
+              })
+            })
+          })
         })
       })
     })

test/dao-factory/create.test.js

@@ -595,6 +595,17 @@ describe(Support.getTestDialectTeaser("DAOFactory"), function () {
       })
     })
 
+    it('should allow blank creates (with timestamps: false)', function (done) {
+      var Worker = this.sequelize.define('Worker', {}, {timestamps: false})
+      Worker.sync().done(function(err) { 
+        Worker.create().done(function (err, worker) {
+          expect(err).not.to.be.ok
+          expect(worker).to.be.ok
+          done()
+        })
+      })
+    })
+
     describe('enums', function() {
       it('correctly restores enum values', function(done) {
         var self = this
@@ -872,6 +883,17 @@ describe(Support.getTestDialectTeaser("DAOFactory"), function () {
       })
     })
 
+    it('should allow blank creates (with timestamps: false)', function (done) {
+      var Worker = this.sequelize.define('Worker', {}, {timestamps: false})
+      Worker.sync().done(function(err) { 
+        Worker.bulkCreate([{}, {}]).done(function (err, workers) {
+          expect(err).not.to.be.ok
+          expect(workers).to.be.ok
+          done()
+        })
+      })
+    })
+
     describe('enums', function() {
       it('correctly restores enum values', function(done) {
         var self = this

test/postgres/query-generator.test.js

@@ -415,6 +415,10 @@ if (dialect.match(/^postgres/)) {
 
       insertQuery: [
         {
+          arguments: ['myTable', {}],
+          expectation: "INSERT INTO \"myTable\" DEFAULT VALUES RETURNING *;"
+        },
+        {
           arguments: ['myTable', {name: 'foo'}],
           expectation: "INSERT INTO \"myTable\" (\"name\") VALUES ('foo') RETURNING *;"
         }, {