Merge pull request #1299 from overlookmotel/order-by-nested-associations-v2

Order by nested associations

lib/dialects/abstract/query-generator.js

 var Utils = require("../../utils")
   , SqlString = require("../../sql-string")
+  , daoFactory = require("../../dao-factory")
 
 module.exports = (function() {
   var QueryGenerator = {
@@ -331,7 +332,14 @@ module.exports = (function() {
     /*
       Quote an object based on its type. This is a more general version of quoteIdentifiers
       Strings: should proxy to quoteIdentifiers
-      Arrays: First argument should be qouted, second argument should be append without quoting
+      Arrays:
+        * Expects array in the form: [<model> (optional), <model> (optional),... String, String (optional)]
+          Each <model> can be a daoFactory or an object {model: DaoFactory, as: String}, matching include
+        * Zero or more models can be included in the array and are used to trace a path through the tree of
+          included nested associations. This produces the correct table name for the ORDER BY/GROUP BY SQL
+          and quotes it.
+        * If a single string is appended to end of array, it is quoted.
+          If two strings appended, the 1st string is quoted, the 2nd string unquoted.
       Objects:
         * If raw is set, that value should be returned verbatim, without quoting
         * If fn is set, the string should start with the value of fn, starting paren, followed by
@@ -339,14 +347,71 @@ module.exports = (function() {
           unless they are themselves objects
         * If direction is set, should be prepended
 
-      Currently this function is only used for ordering / grouping columns, but it could
+      Currently this function is only used for ordering / grouping columns and Sequelize.col(), but it could
       potentially also be used for other places where we want to be able to call SQL functions (e.g. as default values)
     */
-    quote: function(obj, force) {
+    quote: function(obj, parent, force) {
       if (Utils._.isString(obj)) {
         return this.quoteIdentifiers(obj, force)
       } else if (Array.isArray(obj)) {
-        return this.quote(obj[0], force) + ' ' + obj[1]
+        // loop through array, adding table names of models to quoted
+        // (checking associations to see if names should be singularised or not)
+        var quoted = []
+          , i
+          , len = obj.length
+        for (i = 0; i < len - 1; i++) {
+          var item = obj[i]
+          if (Utils._.isString(item) || item instanceof Utils.fn || item instanceof Utils.col || item instanceof Utils.literal || item instanceof Utils.cast || 'raw' in item) {
+            break
+          }
+
+          if (item instanceof daoFactory) {
+            item = {model: item}
+          }
+
+          // find applicable association for linking parent to this model
+          var model = item.model
+            , as
+            , associations = parent.associations
+            , association
+          if (item.hasOwnProperty('as')) {
+            as = item.as
+            association = Utils._.find(associations, function(association, associationName) {
+              return association.target === model && associationName === as
+            })
+          } else {
+            association = Utils._.find(associations, function(association, associationName) {
+              return association.target === model ?
+                associationName === (
+                  association.doubleLinked ?
+                  association.combinedName:
+                  (
+                    association.isSingleAssociation ?
+                    Utils.singularize(model.tableName, model.options.language) :
+                    parent.tableName + model.tableName
+                  )
+                ) :
+                association.targetAssociation && association.targetAssociation.through === model
+            })
+            // NB association.target !== model clause below is to singularize names of through tables in hasMany-hasMany joins
+            as = (association && (association.isSingleAssociation || association.target !== model)) ? Utils.singularize(model.tableName, model.options.language) : model.tableName
+          }
+
+          quoted[i] = as
+
+          if (!association) {
+            throw new Error('\'' + quoted.join('.') + '\' in order / group clause is not valid association')
+          }
+
+          parent = model
+        }
+
+        // add 1st string as quoted, 2nd as unquoted raw
+        var sql = (i > 0 ? this.quoteIdentifier(quoted.join('.')) + '.' : '') + this.quote(obj[i], parent, force)
+        if (i < len - 1) {
+          sql += ' ' + obj[i + 1]
+        }
+        return sql
       } else if (obj instanceof Utils.fn || obj instanceof Utils.col || obj instanceof Utils.literal || obj instanceof Utils.cast) {
         return obj.toString(this)
       } else if (Utils._.isObject(obj) && 'raw' in obj) {
@@ -497,12 +562,12 @@ module.exports = (function() {
         }
 
         if (attr instanceof Utils.fn || attr instanceof Utils.col) {
-          return self.quote(attr)
+          return attr.toString(self)
         }
 
         if(Array.isArray(attr) && attr.length == 2) {
           if (attr[0] instanceof Utils.fn || attr[0] instanceof Utils.col) {
-            attr[0] = self.quote(attr[0])
+            attr[0] = attr[0].toString(self)
             addTable = false
           }
           attr = [attr[0], this.quoteIdentifier(attr[1])].join(' as ')
@@ -703,7 +768,7 @@ module.exports = (function() {
 
       // Add GROUP BY to sub or main query
       if (options.group) {
-        options.group = Array.isArray(options.group) ? options.group.map(function (t) { return this.quote(t) }.bind(this)).join(', ') : options.group
+        options.group = Array.isArray(options.group) ? options.group.map(function (t) { return this.quote(t, factory) }.bind(this)).join(', ') : options.group
         if (subQuery) {
           subQueryItems.push(" GROUP BY " + options.group)
         } else {
@@ -723,7 +788,7 @@ module.exports = (function() {
 
       // Add ORDER to sub or main query
       if (options.order) {
-        options.order = Array.isArray(options.order) ? options.order.map(function (t) { return this.quote(t) }.bind(this)).join(', ') : options.order
+        options.order = Array.isArray(options.order) ? options.order.map(function (t) { return this.quote(t, factory) }.bind(this)).join(', ') : options.order
 
         if (subQuery) {
           subQueryItems.push(" ORDER BY " + options.order)
@@ -950,9 +1015,9 @@ module.exports = (function() {
       })
 
       if (options.include) {
-        return this.quoteIdentifier(keyParts.join('.')) + '.' +  this.quote(attributePart);
+        return this.quoteIdentifier(keyParts.join('.')) + '.' + this.quoteIdentifiers(attributePart)
       }
-      return this.quoteIdentifiers(dao.tableName + '.' +  attributePart);
+      return this.quoteIdentifiers(dao.tableName + '.' + attributePart)
     },
 
     getConditionalJoins: function(options, originalDao){

lib/utils.js

@@ -534,6 +534,9 @@ var Utils = module.exports = {
   },
 
   col: function (col) {
+    if (arguments.length > 1) {
+      col = Array.prototype.slice.call(arguments);
+    }
     this.col = col
   },
 
@@ -588,23 +591,27 @@ Utils.cast.prototype.toString = function(queryGenerator) {
   return 'CAST(' + this.val + ' AS ' + this.type.toUpperCase() + ')'
 }
 
-Utils.fn.prototype.toString = function(queryGenerator) {
+Utils.fn.prototype.toString = function(queryGenerator, parentModel) {
   return this.fn + '(' + this.args.map(function (arg) {
     if (arg instanceof Utils.fn || arg instanceof Utils.col) {
-      return arg.toString(queryGenerator)
+      return arg.toString(queryGenerator, parentModel)
     } else {
       return queryGenerator.escape(arg)
     }
   }).join(', ') + ')'
 }
 
-Utils.col.prototype.toString = function (queryGenerator) {
-  if (this.col.indexOf('*') === 0) {
+Utils.col.prototype.toString = function (queryGenerator, parentModel) {
+  if (Array.isArray(this.col)) {
+    if (!parent) {
+      throw new Error('Cannot call Sequelize.col() with array outside of order / group clause')
+    }
+  } else if (this.col.indexOf('*') === 0) {
     return '*'
   }
-  return queryGenerator.quote(this.col)
+  return queryGenerator.quote(this.col, parentModel)
 }
 
 Utils.CustomEventEmitter = require(__dirname + "/emitters/custom-event-emitter")
 Utils.QueryChainer = require(__dirname + "/query-chainer")
-Utils.Lingo = require("lingo")
\ No newline at end of file
+Utils.Lingo = require("lingo")

test/dao-factory/findAll.test.js

@@ -761,6 +761,224 @@ describe(Support.getTestDialectTeaser("DAOFactory"), function () {
       })
     })
 
+    describe('order by eager loaded tables', function() {
+      describe('HasMany', function() {
+        beforeEach(function(done) {
+          var self = this
+
+          self.Continent = this.sequelize.define('Continent', { name: Sequelize.STRING })
+          self.Country = this.sequelize.define('Country', { name: Sequelize.STRING })
+          self.Person = this.sequelize.define('Person', { name: Sequelize.STRING, lastName: Sequelize.STRING })
+
+          self.Continent.hasMany(self.Country)
+          self.Country.belongsTo(self.Continent)
+          self.Country.hasMany(self.Person)
+          self.Person.belongsTo(self.Country)
+          self.Country.hasMany(self.Person, { as: 'Residents', foreignKey: 'CountryResidentId' })
+          self.Person.belongsTo(self.Country, { as: 'CountryResident', foreignKey: 'CountryResidentId' })
+
+          async.forEach([ self.Continent, self.Country, self.Person ], function(model, callback) {
+            model.sync({ force: true }).done(callback)
+          }, function () {
+            async.parallel({
+              europe: function(callback) {self.Continent.create({ name: 'Europe' }).done(callback)},
+              asia: function(callback) {self.Continent.create({ name: 'Asia' }).done(callback)},
+              england: function(callback) {self.Country.create({ name: 'England' }).done(callback)},
+              france: function(callback) {self.Country.create({ name: 'France' }).done(callback)},
+              korea: function(callback) {self.Country.create({ name: 'Korea' }).done(callback)},
+              bob: function(callback) {self.Person.create({ name: 'Bob', lastName: 'Becket' }).done(callback)},
+              fred: function(callback) {self.Person.create({ name: 'Fred', lastName: 'Able' }).done(callback)},
+              pierre: function(callback) {self.Person.create({ name: 'Pierre', lastName: 'Paris' }).done(callback)},
+              kim: function(callback) {self.Person.create({ name: 'Kim', lastName: 'Z' }).done(callback)}
+            }, function(err, r) {
+              if (err) throw err
+
+              _.forEach(r, function(item, itemName) {
+                self[itemName] = item
+              })
+
+              async.parallel([
+                function(callback) {self.england.setContinent(self.europe).done(callback)},
+                function(callback) {self.france.setContinent(self.europe).done(callback)},
+                function(callback) {self.korea.setContinent(self.asia).done(callback)},
+
+                function(callback) {self.bob.setCountry(self.england).done(callback)},
+                function(callback) {self.fred.setCountry(self.england).done(callback)},
+                function(callback) {self.pierre.setCountry(self.france).done(callback)},
+                function(callback) {self.kim.setCountry(self.korea).done(callback)},
+
+                function(callback) {self.bob.setCountryResident(self.england).done(callback)},
+                function(callback) {self.fred.setCountryResident(self.france).done(callback)},
+                function(callback) {self.pierre.setCountryResident(self.korea).done(callback)},
+                function(callback) {self.kim.setCountryResident(self.england).done(callback)}
+              ], function(err) {
+                if (err) throw err
+                done()
+              })
+            })
+          })
+        })
+
+        it('sorts simply', function(done) {
+          var self = this
+          async.eachSeries([ [ 'ASC', 'Asia' ], [ 'DESC', 'Europe' ] ], function(params, callback) {
+            self.Continent.findAll({
+              order: [ [ 'name', params[0] ] ]
+            }).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])
+              callback()
+            })
+          }, function() {done()})
+        })
+
+        it('sorts by 1st degree association', function(done) {
+          var self = this
+          async.forEach([ [ 'ASC', 'Europe', 'England' ], [ 'DESC', 'Asia', 'Korea' ] ], function(params, callback) {
+            self.Continent.findAll({
+              include: [ self.Country ],
+              order: [ [ self.Country, 'name', params[0] ] ]
+            }).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])
+              callback()
+            })
+          }, function() {done()})
+        }),
+
+        it('sorts by 2nd degree association', function(done) {
+          var self = this
+          async.forEach([ [ 'ASC', 'Europe', 'England', 'Fred' ], [ 'DESC', 'Asia', 'Korea', 'Kim' ] ], function(params, callback) {
+            self.Continent.findAll({
+              include: [ { model: self.Country, include: [ self.Person ] } ],
+              order: [ [ self.Country, self.Person, 'lastName', params[0] ] ]
+            }).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].persons).to.exist
+              expect(continents[0].countries[0].persons[0]).to.exist
+              expect(continents[0].countries[0].persons[0].name).to.equal(params[3])
+              callback()
+            })
+          }, function() {done()})
+        }),
+
+        it('sorts by 2nd degree association with alias', 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: [ [ self.Country, {model: self.Person, as: 'Residents' }, 'lastName', params[0] ] ]
+            }).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() {
+        beforeEach(function(done) {
+          var self = this
+
+          self.Country = this.sequelize.define('Country', { name: Sequelize.STRING })
+          self.Industry = this.sequelize.define('Industry', { name: Sequelize.STRING })
+          self.IndustryCountry = this.sequelize.define('IndustryCountry', { numYears: Sequelize.INTEGER })
+
+          self.Country.hasMany(self.Industry, {through: self.IndustryCountry})
+          self.Industry.hasMany(self.Country, {through: self.IndustryCountry})
+
+          async.forEach([ self.Country, self.Industry ], function(model, callback) {
+            model.sync({ force: true }).done(callback)
+          }, function () {
+            async.parallel({
+              england: function(callback) {self.Country.create({ name: 'England' }).done(callback)},
+              france: function(callback) {self.Country.create({ name: 'France' }).done(callback)},
+              korea: function(callback) {self.Country.create({ name: 'Korea' }).done(callback)},
+              energy: function(callback) {self.Industry.create({ name: 'Energy' }).done(callback)},
+              media: function(callback) {self.Industry.create({ name: 'Media' }).done(callback)},
+              tech: function(callback) {self.Industry.create({ name: 'Tech' }).done(callback)}
+            }, function(err, r) {
+              if (err) throw err
+
+              _.forEach(r, function(item, itemName) {
+                self[itemName] = item
+              })
+
+              async.parallel([
+                function(callback) {self.england.addIndustry(self.energy, {numYears: 20}).done(callback)},
+                function(callback) {self.england.addIndustry(self.media, {numYears: 40}).done(callback)},
+                function(callback) {self.france.addIndustry(self.media, {numYears: 80}).done(callback)},
+                function(callback) {self.korea.addIndustry(self.tech, {numYears: 30}).done(callback)}
+              ], function(err) {
+                if (err) throw err
+                done()
+              })
+            })
+          })
+        })
+
+        it('sorts by 1st degree association', function(done) {
+          var self = this
+          async.forEach([ [ 'ASC', 'England', 'Energy' ], [ 'DESC', 'Korea', 'Tech' ] ], function(params, callback) {
+            self.Country.findAll({
+              include: [ self.Industry ],
+              order: [ [ self.Industry, 'name', params[0] ] ]
+            }).done(function(err, countries) {
+              expect(err).not.to.be.ok
+              expect(countries).to.exist
+              expect(countries[0]).to.exist
+              expect(countries[0].name).to.equal(params[1])
+              expect(countries[0].industries).to.exist
+              expect(countries[0].industries[0]).to.exist
+              expect(countries[0].industries[0].name).to.equal(params[2])
+              callback()
+            })
+          }, function() {done()})
+        })
+
+        it('sorts by through table attribute', function(done) {
+          var self = this
+          async.forEach([ [ 'ASC', 'England', 'Energy' ], [ 'DESC', 'France', 'Media' ] ], function(params, callback) {
+            self.Country.findAll({
+              include: [ self.Industry ],
+              order: [ [ self.Industry, self.IndustryCountry, 'numYears', params[0] ] ]
+            }).done(function(err, countries) {
+              expect(err).not.to.be.ok
+              expect(countries).to.exist
+              expect(countries[0]).to.exist
+              expect(countries[0].name).to.equal(params[1])
+              expect(countries[0].industries).to.exist
+              expect(countries[0].industries[0]).to.exist
+              expect(countries[0].industries[0].name).to.equal(params[2])
+              callback()
+            })
+          }, function() {done()})
+        })
+      })
+    })
+
     describe('normal findAll', function() {
       beforeEach(function(done) {
         var self = this