[merge]

Mick Hansen
Commit 8fd97883 authored Feb 26, 2014 by Mick Hansen
Showing with 334 additions and 40 deletions
changelog.md
lib/associations/belongs-to.js
lib/dao-factory.js
lib/dialects/abstract/query-generator.js
lib/dialects/postgres/connector-manager.js
lib/transaction-manager.js
lib/transaction.js
test/dao-factory/findAll.test.js
test/include/findAll.test.js
test/sequelize.transaction.test.js
test/transaction-manager.test.js
--- a/changelog.md
+++ b/changelog.md
 Notice: All 1.7.x changes are present in 2.0.x aswell
-# v1.7.0-rc9 (next)
+# v1.7.0 (next)
+- [FEATURE] covers more advanced include cases with limiting and filtering (specifically cases where a include would be in the subquery but its child include wouldnt be, for cases where a 1:1 association had a 1:M association as a nested include)
+- [BUG] fixes issue where connection would timeout before calling COMMIT resulting in data never reaching the database [#1429](https://github.com/sequelize/sequelize/pull/1429)
+# v1.7.0-rc9
 - [PERFORMANCE] fixes performance regression introduced in rc7
 - [FEATURE] include all relations for a model [#1421](https://github.com/sequelize/sequelize/pull/1421)
- [FEATURE] covers more advanced include cases with limiting and filtering
+- [BUG] N:M adder/getter with through model and custom primary keys now work
 # v1.7.0-rc8
 - [BUG] fixes bug with required includes without wheres with subqueries

--- a/lib/associations/belongs-to.js
+++ b/lib/associations/belongs-to.js
@@ -21,6 +21,10 @@ module.exports = (function() {
      this.isAliased = true
    } else {
      this.as = Utils.singularize(this.target.tableName, this.target.options.language)
+      // Hotfix
+      if (this.as === this.target.tableName) {
+        this.as = Utils.singularize(this.target.name, this.target.options.language)
+      }
    }
    this.associationAccessor = this.isSelfAssociation

--- a/lib/dao-factory.js
+++ b/lib/dao-factory.js
@@ -1386,6 +1386,12 @@ module.exports = (function() {
      include = includes[index] = validateIncludedElement.call(this, include, tableNames)
+      include.parent = options
+      // associations that are required or have a required child as is not a ?:M association are candidates for the subquery
+      include.subQuery = !include.association.isMultiAssociation && (include.hasIncludeRequired || include.required)
+      include.hasParentWhere = options.hasParentWhere || !!options.where
+      include.hasParentRequired = options.hasParentRequired || !!options.required
      options.includeMap[include.as] = include
      options.includeNames.push(include.as)
      options.includeNames.push(include.as.substr(0,1).toLowerCase() + include.as.substr(1))

--- a/lib/dialects/abstract/query-generator.js
+++ b/lib/dialects/abstract/query-generator.js
@@ -587,10 +587,14 @@ module.exports = (function() {
      }
      if (options.include) {
-        var generateJoinQuery = function(include, parentTable) {
+        var generateJoinQueries = function(include, parentTable) {
          var table         = include.daoFactory.tableName
            , as            = include.as
            , joinQueryItem = ""
+            , joinQueries = {
+              mainQuery: [],
+              subQuery: []
+            }
            , attributes
            , association   = include.association
            , through       = include.through
@@ -610,8 +614,7 @@ module.exports = (function() {
              return self.quoteIdentifier(as) + "." + self.quoteIdentifier(attr) + " AS " + self.quoteIdentifier(as + "." + attr)
            })
-            // If not many to many, and we're doing a subquery, add attributes to the subquery
+            if (include.subQuery && subQuery) {
-            if (!include.association.isMultiAssociation && subQuery && (include.hasIncludeRequired || include.required)) {
              subQueryAttributes = subQueryAttributes.concat(attributes)
            } else {
              mainAttributes = mainAttributes.concat(attributes)
@@ -690,7 +693,12 @@ module.exports = (function() {
            // Filter statement
            // Used by both join and subquery where
-            where = self.quoteIdentifier(tableLeft) + "." + self.quoteIdentifier(attrLeft) + " = "
+            if (subQuery && !include.subQuery && include.parent.subQuery) {
+              where = self.quoteIdentifier(tableLeft + "." + attrLeft) + " = "
+            } else {
+              where = self.quoteIdentifier(tableLeft) + "." + self.quoteIdentifier(attrLeft) + " = "
+            }
            where += self.quoteIdentifier(tableRight) + "." + self.quoteIdentifier(attrRight)
            // Generate join SQL
@@ -709,26 +717,33 @@ module.exports = (function() {
            }
          }
+          if (include.subQuery && subQuery) {
+            joinQueries.subQuery.push(joinQueryItem);
+          } else {
+            joinQueries.mainQuery.push(joinQueryItem);
+          }
          if (include.include) {
            include.include.forEach(function(childInclude) {
              if (childInclude._pseudo) return
-              joinQueryItem += generateJoinQuery(childInclude, as)
+              var childJoinQueries = generateJoinQueries(childInclude, as)
+              if (childInclude.subQuery && subQuery) {
+                joinQueries.subQuery = joinQueries.subQuery.concat(childJoinQueries.subQuery)
+              } else {
+                joinQueries.mainQuery = joinQueries.mainQuery.concat(childJoinQueries.mainQuery)
+              }
            }.bind(this))
          }
+          return joinQueries
-          return joinQueryItem
        }
        // Loop through includes and generate subqueries
        options.include.forEach(function(include) {
-          var joinQueryItem = generateJoinQuery(include, tableName)
+          var joinQueries = generateJoinQueries(include, tableName)
-          // If not many to many, and we're doing a subquery, add joinQuery to subQueries
+          subJoinQueries = subJoinQueries.concat(joinQueries.subQuery)
-          if (!include.association.isMultiAssociation && (subQuery && (include.hasIncludeRequired || include.required))) {
+          mainJoinQueries = mainJoinQueries.concat(joinQueries.mainQuery)
-            subJoinQueries.push(joinQueryItem)
-          } else {
-            mainJoinQueries.push(joinQueryItem)
-          }
        }.bind(this))
      }
@@ -780,7 +795,7 @@ module.exports = (function() {
        if (Array.isArray(options.order)) {
          options.order.forEach(function (t) {
-            if (subQuery && !(t[0] instanceof daoFactory)) {
+            if (subQuery && !(t[0] instanceof daoFactory) && !(t[0].model instanceof daoFactory)) {
              subQueryOrder.push(this.quote(t, Model))
            }
            mainQueryOrder.push(this.quote(t, Model))

--- a/lib/dialects/postgres/connector-manager.js
+++ b/lib/dialects/postgres/connector-manager.js
+var Query                = require("./query")
-var Query   = require("./query")
+  , Utils                = require("../../utils")
-  , Utils   = require("../../utils")
+  , ConnectionParameters = require('pg/lib/connection-parameters')
 module.exports = (function() {
  var ConnectorManager = function(sequelize, config) {
@@ -16,13 +16,6 @@ module.exports = (function() {
    // https://github.com/brianc/node-postgres/issues/166#issuecomment-9514935
    this.pg.types.setTypeParser(20, String);
-    // set pooling parameters if specified
-    if (this.pooling) {
-      this.pg.defaults.poolSize           = this.config.pool.maxConnections || 10
-      this.pg.defaults.poolIdleTimeout    = this.config.pool.maxIdleTime    || 30000
-      this.pg.defaults.reapIntervalMillis = this.config.pool.reapInterval   || 1000
-    }
    this.disconnectTimeoutId = null
    this.pendingQueries = 0
    this.clientDrained = true
@@ -31,7 +24,7 @@ module.exports = (function() {
    this.onProcessExit = function () {
      this.disconnect()
    }.bind(this);
    process.on('exit', this.onProcessExit)
  }
  Utils._.extend(ConnectorManager.prototype, require("../connector-manager").prototype)
@@ -89,7 +82,16 @@ module.exports = (function() {
    this.isConnecting = true
    this.isConnected  = false
-    var uri = this.sequelize.getQueryInterface().QueryGenerator.databaseConnectionUri(this.config)
+    var uri    = this.sequelize.getQueryInterface().QueryGenerator.databaseConnectionUri(this.config)
+      , config = new ConnectionParameters(uri)
+    // set pooling parameters if specified
+    if (this.pooling) {
+      config.poolSize           = this.config.pool.maxConnections || 10
+      config.poolIdleTimeout    = this.config.pool.maxIdleTime    || 30000
+      config.reapIntervalMillis = this.config.pool.reapInterval   || 1000
+      config.uuid               = this.config.uuid
+    }
    var connectCallback = function(err, client, done) {
      self.isConnecting = false
@@ -142,7 +144,7 @@ module.exports = (function() {
    if (this.pooling) {
      // acquire client from pool
-      this.pg.connect(uri, connectCallback)
+      this.pg.connect(config, connectCallback)
    } else {
      if (!!this.client) {
        connectCallback(null, this.client)
@@ -151,13 +153,13 @@ module.exports = (function() {
        var responded = false
-        this.client = new this.pg.Client(uri)
+        this.client = new this.pg.Client(config)
        this.client.connect(function(err, client, done) {
          responded = true
          connectCallback(err, client || self.client, done)
        })
-        // If we didn't ever hear from the client.connect() callback the connection timedout, node-postgres does not treat this as an error since no active query was ever emitted
+        // If we didn't ever hear from the client.connect() callback the connection timeout, node-postgres does not treat this as an error since no active query was ever emitted
        this.client.on('end', function () {
          if (!responded) {
            connectCallback(new Error('Connection timed out'))

--- a/lib/transaction-manager.js
+++ b/lib/transaction-manager.js
@@ -22,8 +22,9 @@ TransactionManager.prototype.getConnectorManager = function(uuid) {
        {},
        Utils._.clone(config.pool || {}),
        {
-          maxConnections: 0,
+          minConnections: 1,
-          useReplicaton: false
+          maxConnections: 1,
+          useReplicaton:  false
        }
      )
      config.keepDefaultTimezone = true

--- a/lib/transaction.js
+++ b/lib/transaction.js
@@ -86,4 +86,4 @@ Transaction.prototype.cleanup = function() {
 var onError = function(err) {
  this.emit('error', err)
 }
\ No newline at end of file
--- a/test/dao-factory/findAll.test.js
+++ b/test/dao-factory/findAll.test.js
@@ -1039,7 +1039,7 @@ describe(Support.getTestDialectTeaser("DAOFactory"), function () {
              callback()
            })
          }, function() {done()})
-        }),
+        })
        it('sorts by 2nd degree association', function(done) {
          var self = this
@@ -1084,6 +1084,29 @@ describe(Support.getTestDialectTeaser("DAOFactory"), function () {
            })
          }, function() {done()})
        })
+        it('sorts by 2nd degree association with alias while using limit', function(done) {
+          var self = this
+          async.forEach([ [ 'ASC', 'Europe', 'France', 'Fred' ], [ 'DESC', 'Europe', 'England', 'Kim' ] ], function(params, callback) {
+            self.Continent.findAll({
+              include: [ { model: self.Country, include: [ self.Person, {model: self.Person, as: 'Residents' } ] } ],
+              order: [ [ { model: self.Country }, {model: self.Person, as: 'Residents' }, 'lastName', params[0] ] ],
+              limit: 3
+            }).done(function(err, continents) {
+              expect(err).not.to.be.ok
+              expect(continents).to.exist
+              expect(continents[0]).to.exist
+              expect(continents[0].name).to.equal(params[1])
+              expect(continents[0].countries).to.exist
+              expect(continents[0].countries[0]).to.exist
+              expect(continents[0].countries[0].name).to.equal(params[2])
+              expect(continents[0].countries[0].residents).to.exist
+              expect(continents[0].countries[0].residents[0]).to.exist
+              expect(continents[0].countries[0].residents[0].name).to.equal(params[3])
+              callback()
+            })
+          }, function() {done()})
+        })
      }),
      describe('ManyToMany', function() {

--- a/test/include/findAll.test.js
+++ b/test/include/findAll.test.js
@@ -780,7 +780,7 @@ describe(Support.getTestDialectTeaser("Include"), function () {
      })
    })
-    it('should be possible to define a belongsTo include as required with limit', function (done) {
+    it('should be possible to extend the on clause with a where option on a belongsTo include', function (done) {
      var User = this.sequelize.define('User', {})
        , Group = this.sequelize.define('Group', {
            name: DataTypes.STRING
@@ -815,6 +815,213 @@ describe(Support.getTestDialectTeaser("Include"), function () {
          User.findAll({
            include: [
              {model: Group, required: true}
+            ]
+          }).done(function (err, users) {
+            expect(err).not.to.be.ok
+            users.forEach(function (user) {
+              expect(user.group).to.be.ok
+            })
+            done()
+          })
+        })
+      })
+    })
+    it('should be possible to define a belongsTo include as required with child hasMany with limit', function (done) {
+      var User = this.sequelize.define('User', {})
+        , Group = this.sequelize.define('Group', {
+            name: DataTypes.STRING
+          })
+        , Category = this.sequelize.define('Category', {
+            category: DataTypes.STRING
+          })
+      User.belongsTo(Group)
+      Group.hasMany(Category)
+      this.sequelize.sync({force: true}).done(function () {
+        async.auto({
+          groups: function (callback) {
+            Group.bulkCreate([
+              {name: 'A'},
+              {name: 'B'}
+            ]).done(function () {
+              Group.findAll().done(callback)
+            })
+          },
+          users: function (callback) {
+            User.bulkCreate([{}, {}]).done(function () {
+              User.findAll().done(callback)
+            })
+          },
+          categories: function (callback) {
+            Category.bulkCreate([{}, {}]).done(function () {
+              Category.findAll().done(callback)
+            })
+          },
+          userGroups: ['users', 'groups', function (callback, results) {
+            var chainer = new Sequelize.Utils.QueryChainer()
+            chainer.add(results.users[0].setGroup(results.groups[1]))
+            chainer.add(results.users[1].setGroup(results.groups[0]))
+            chainer.run().done(callback)
+          }],
+          groupCategories: ['groups', 'categories', function (callback, results) {
+            var chainer = new Sequelize.Utils.QueryChainer()
+            results.groups.forEach(function (group) {
+              chainer.add(group.setCategories(results.categories))
+            })
+            chainer.run().done(callback)
+          }]
+        }, function (err, results) {
+          expect(err).not.to.be.ok
+          User.findAll({
+            include: [
+              {model: Group, required: true, include: [
+                {model: Category}
+              ]}
+            ],
+            limit: 1
+          }).done(function (err, users) {
+            expect(err).not.to.be.ok
+            expect(users.length).to.equal(1)
+            users.forEach(function (user) {
+              expect(user.group).to.be.ok
+              expect(user.group.categories).to.be.ok
+            })
+            done()
+          })
+        })
+      })
+    })
+    it('should be possible to define a belongsTo include as required with child hasMany with limit and aliases', function (done) {
+      var User = this.sequelize.define('User', {})
+        , Group = this.sequelize.define('Group', {
+            name: DataTypes.STRING
+          })
+        , Category = this.sequelize.define('Category', {
+            category: DataTypes.STRING
+          })
+      User.belongsTo(Group, {as: 'Team'})
+      Group.hasMany(Category, {as: 'Tags'})
+      this.sequelize.sync({force: true}).done(function () {
+        async.auto({
+          groups: function (callback) {
+            Group.bulkCreate([
+              {name: 'A'},
+              {name: 'B'}
+            ]).done(function () {
+              Group.findAll().done(callback)
+            })
+          },
+          users: function (callback) {
+            User.bulkCreate([{}, {}]).done(function () {
+              User.findAll().done(callback)
+            })
+          },
+          categories: function (callback) {
+            Category.bulkCreate([{}, {}]).done(function () {
+              Category.findAll().done(callback)
+            })
+          },
+          userGroups: ['users', 'groups', function (callback, results) {
+            var chainer = new Sequelize.Utils.QueryChainer()
+            chainer.add(results.users[0].setTeam(results.groups[1]))
+            chainer.add(results.users[1].setTeam(results.groups[0]))
+            chainer.run().done(callback)
+          }],
+          groupCategories: ['groups', 'categories', function (callback, results) {
+            var chainer = new Sequelize.Utils.QueryChainer()
+            results.groups.forEach(function (group) {
+              chainer.add(group.setTags(results.categories))
+            })
+            chainer.run().done(callback)
+          }]
+        }, function (err, results) {
+          expect(err).not.to.be.ok
+          User.findAll({
+            include: [
+              {model: Group, required: true, as: 'Team', include: [
+                {model: Category, as: 'Tags'}
+              ]}
+            ],
+            limit: 1
+          }).done(function (err, users) {
+            expect(err).not.to.be.ok
+            expect(users.length).to.equal(1)
+            users.forEach(function (user) {
+              expect(user.team).to.be.ok
+              expect(user.team.tags).to.be.ok
+            })
+            done()
+          })
+        })
+      })
+    })
+    it('should be possible to define a belongsTo include as required with child hasMany which is not required with limit', function (done) {
+      var User = this.sequelize.define('User', {})
+        , Group = this.sequelize.define('Group', {
+            name: DataTypes.STRING
+          })
+        , Category = this.sequelize.define('Category', {
+            category: DataTypes.STRING
+          })
+      User.belongsTo(Group)
+      Group.hasMany(Category)
+      this.sequelize.sync({force: true}).done(function () {
+        async.auto({
+          groups: function (callback) {
+            Group.bulkCreate([
+              {name: 'A'},
+              {name: 'B'}
+            ]).done(function () {
+              Group.findAll().done(callback)
+            })
+          },
+          users: function (callback) {
+            User.bulkCreate([{}, {}]).done(function () {
+              User.findAll().done(callback)
+            })
+          },
+          categories: function (callback) {
+            Category.bulkCreate([{}, {}]).done(function () {
+              Category.findAll().done(callback)
+            })
+          },
+          userGroups: ['users', 'groups', function (callback, results) {
+            var chainer = new Sequelize.Utils.QueryChainer()
+            chainer.add(results.users[0].setGroup(results.groups[1]))
+            chainer.add(results.users[1].setGroup(results.groups[0]))
+            chainer.run().done(callback)
+          }],
+          groupCategories: ['groups', 'categories', function (callback, results) {
+            var chainer = new Sequelize.Utils.QueryChainer()
+            results.groups.forEach(function (group) {
+              chainer.add(group.setCategories(results.categories))
+            })
+            chainer.run().done(callback)
+          }]
+        }, function (err, results) {
+          expect(err).not.to.be.ok
+          User.findAll({
+            include: [
+              {model: Group, required: true, include: [
+                {model: Category, required: false}
+              ]}
            ],
            limit: 1
          }).done(function (err, users) {
@@ -822,6 +1029,7 @@ describe(Support.getTestDialectTeaser("Include"), function () {
            expect(users.length).to.equal(1)
            users.forEach(function (user) {
              expect(user.group).to.be.ok
+              expect(user.group.categories).to.be.ok
            })
            done()
          })

--- a/test/sequelize.transaction.test.js
+++ b/test/sequelize.transaction.test.js
@@ -116,4 +116,35 @@ describe(Support.getTestDialectTeaser("Sequelize#transaction"), function () {
      }).done(done)
    })
  })
+  describe('complex long running example', function() {
+    it("works with promise syntax", function(done) {
+      Support.prepareTransactionTest(this.sequelize, function(sequelize) {
+        var Test = sequelize.define('Test', {
+          id:   { type: Support.Sequelize.INTEGER, primaryKey: true, autoIncrement: true},
+          name: { type: Support.Sequelize.STRING }
+        })
+        sequelize
+          .sync({ force: true })
+          .then(function() {
+            sequelize.transaction(function(transaction) {
+              Test
+                .create({ name: 'Peter' }, { transaction: transaction })
+                .then(function() {
+                  setTimeout(function() {
+                    transaction
+                      .commit()
+                      .then(function() { return Test.count() })
+                      .then(function(count) {
+                        expect(count).to.equal(1)
+                        done()
+                      })
+                  }, 1000)
+                })
+            })
+          })
+      })
+    })
+  })
 })
--- a/test/transaction-manager.test.js
+++ b/test/transaction-manager.test.js
@@ -60,12 +60,13 @@ describe(Support.getTestDialectTeaser("TransactionManager"), function () {
        }
        Object.keys(this.sequelize.config.pool || {}).forEach(function(key) {
-          if (key !== 'maxConnections') {
+          if (['minConnections', 'maxConnections'].indexOf(key) === -1) {
            expect(connectorManager.config.pool[key]).to.equal(self.sequelize.config.pool[key])
          }
        })
-        expect(connectorManager.config.pool.maxConnections).to.equal(0)
+        expect(connectorManager.config.pool.minConnections).to.equal(1)
+        expect(connectorManager.config.pool.maxConnections).to.equal(1)
      })
    })
  })