Make ordering consistent across all dialects

lib/dialects/query-generator.js → lib/dialects/abstract/query-generator.js

@@ -1,3 +1,5 @@
+var Utils = require("../../utils")
+
 module.exports = (function() {
   var QueryGenerator = {
     addSchema: function(opts) {
@@ -268,7 +270,35 @@ module.exports = (function() {
       Globally disable foreign key constraints
     */
     disableForeignKeyConstraintsQuery: function() {
-       throwMethodUndefined('disableForeignKeyConstraintsQuery')
+      throwMethodUndefined('disableForeignKeyConstraintsQuery')
+    },
+
+    /*
+      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
+      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
+          the values of cols (which is assumed to be an array), quoted and joined with ', ', 
+          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
+      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) {
+      if (Utils._.isString(obj)) {
+        return this.quoteIdentifiers(obj, force)
+      } else if (Array.isArray(obj)) {
+        return this.quote(obj[0], force) + ' ' + obj[1]
+      } else if (obj instanceof Utils.fn || obj instanceof Utils.col) {
+        return obj.toString(this)
+      } else if (Utils._.isObject(obj) && 'raw' in obj) {
+        return obj.raw
+      } else {
+        throw new Error('Unknown structure passed to order / group: ' + JSON.stringify(ojb))
+      }
     },
 
     /*

lib/dialects/mysql/query-generator.js

@@ -228,11 +228,12 @@ module.exports = (function() {
       }
 
       if (options.group) {
-        options.group = Array.isArray(options.group) ? options.group.map(function(t) { return this.quoteIdentifiers(t)}.bind(this)).join(', ') : this.quoteIdentifiers(options.group)
+        options.group = Array.isArray(options.group) ? options.group.map(function (t) { return this.quote(t) }.bind(this)).join(', ') : options.group
         query += " GROUP BY " + options.group
       }
 
       if (options.order) {
+        options.order = Array.isArray(options.order) ? options.order.map(function (t) { return this.quote(t) }.bind(this)).join(', ') : options.order
         query += " ORDER BY " + options.order
       }
 
@@ -612,5 +613,5 @@ module.exports = (function() {
 
   }
 
-  return Utils._.extend(Utils._.clone(require("../query-generator")), QueryGenerator)
+  return Utils._.extend(Utils._.clone(require("../abstract/query-generator")), QueryGenerator)
 })()

lib/dialects/postgres/query-generator.js

@@ -300,14 +300,12 @@ module.exports = (function() {
       }
 
       if(options.group) {
-        options.group = Array.isArray(options.group) ? options.group.map(function(t) { return this.quoteIdentifiers(t) }.bind(this)).join(', ') : this.quoteIdentifiers(options.group)
+        options.group = Array.isArray(options.group) ? options.group.map(function (t) { return this.quote(t) }.bind(this)).join(', ') : options.group
         query += " GROUP BY <%= group %>"
       }
 
       if(options.order) {
-        options.order = options.order.replace(/([^ ]+)(.*)/, function(m, g1, g2) {
-          return this.quoteIdentifiers(g1) + g2
-        }.bind(this))
+        options.order = Array.isArray(options.order) ? options.order.map(function (t) { return this.quote(t) }.bind(this)).join(', ') : options.order
         query += " ORDER BY <%= order %>"
       }
 
@@ -868,5 +866,5 @@ module.exports = (function() {
     return returning;
   }
 
-  return Utils._.extend(Utils._.clone(require("../query-generator")), QueryGenerator)
+  return Utils._.extend(Utils._.clone(require("../abstract/query-generator")), QueryGenerator)
 })()

lib/dialects/sqlite/query-generator.js

@@ -3,7 +3,7 @@ var Utils = require("../../utils")
   , SqlString = require("../../sql-string")
 
 var MySqlQueryGenerator = Utils._.extend(
-  Utils._.clone(require("../query-generator")),
+  Utils._.clone(require("../abstract/query-generator")),
   Utils._.clone(require("../mysql/query-generator"))
 )
 
@@ -211,11 +211,12 @@ module.exports = (function() {
       }
 
       if (options.group) {
-        options.group = Array.isArray(options.group) ? options.group.map(function(t) { return this.quoteIdentifiers(t)}.bind(this)).join(', ') : qa(options.group)
+        options.group = Array.isArray(options.group) ? options.group.map(function (t) { return this.quote(t) }.bind(this)).join(', ') : options.group
         query += " GROUP BY " + options.group
       }
 
       if (options.order) {
+        options.order = Array.isArray(options.order) ? options.order.map(function (t) { return this.quote(t) }.bind(this)).join(', ') : options.order
         query += " ORDER BY " + options.order
       }
 

lib/sequelize.js

@@ -318,5 +318,13 @@ module.exports = (function() {
     }).run()
   }
 
+  Sequelize.prototype.fn = function (fn) {
+    return new Utils.fn(fn, Array.prototype.slice.call(arguments, 1))
+  }
+
+  Sequelize.prototype.col = function (col) {
+    return new Utils.col(col)
+  }
+
   return Sequelize
 })()

lib/utils.js

@@ -468,9 +468,33 @@ var Utils = module.exports = {
   removeTicks: function(s, tickChar) {
     tickChar = tickChar || Utils.TICK_CHAR
     return s.replace(new RegExp(tickChar, 'g'), "")
+  },
+  /*
+   * Utility functions for representing SQL functions, and columns that should be escaped.
+   * Please do not use these functions directly, use Sequelize.fn and Sequelize.col instead.
+   */
+  fn: function (fn, args) {
+    this.fn = fn
+    this.args = args
+  },
+  col: function (col) {
+    this.col = col
   }
 }
 
+Utils.fn.prototype.toString = function(queryGenerator) {
+  return this.fn + '(' + this.args.map(function (arg) {
+    if (arg instanceof Utils.fn || arg instanceof Utils.col) {
+      return arg.toString(queryGenerator)
+    } else {
+      return queryGenerator.escape(arg)
+    }    
+  }).join(', ') + ')'
+}
+Utils.col.prototype.toString = function (queryGenerator) {
+  return queryGenerator.quote(this.col)
+}
+
 Utils.CustomEventEmitter = require(__dirname + "/emitters/custom-event-emitter")
 Utils.QueryChainer = require(__dirname + "/query-chainer")
-Utils.Lingo = require("lingo")
+Utils.Lingo = require("lingo")

test/dao-factory.test.js

@@ -2492,7 +2492,7 @@ describe(Support.getTestDialectTeaser("DAOFactory"), function () {
       it("sorts the results via a date column", function(done) {
         var self = this
         self.User.create({username: 'user3', data: 'bar', theDate: moment().add('hours', 2).toDate()}).success(function(){
-          self.User.findAll({ order: 'theDate DESC' }).success(function(users) {
+          self.User.findAll({ order: [['theDate', 'DESC']] }).success(function(users) {
             expect(users[0].id).to.be.above(users[2].id)
             done()
           })

test/mysql/query-generator.test.js

@@ -163,20 +163,77 @@ if (dialect.match(/^mysql/)) {
           expectation: "SELECT * FROM `myTable` ORDER BY id DESC;",
           context: QueryGenerator
         }, {
+          arguments: ['myTable', {order: ["id"]}],
+          expectation: "SELECT * FROM `myTable` ORDER BY `id`;",
+          context: QueryGenerator
+        }, {
+          arguments: ['myTable', {order: ["myTable.id"]}],
+          expectation: "SELECT * FROM `myTable` ORDER BY `myTable`.`id`;",
+          context: QueryGenerator
+        }, {
+          arguments: ['myTable', {order: [["id", 'DESC']]}],
+          expectation: "SELECT * FROM `myTable` ORDER BY `id` DESC;",
+          context: QueryGenerator
+        }, {
+          title: 'raw arguments are neither quoted nor escaped',
+          arguments: ['myTable', {order: [[{raw: 'f1(f2(id))'}, 'DESC']]}],
+          expectation: "SELECT * FROM `myTable` ORDER BY f1(f2(id)) DESC;",
+          context: QueryGenerator
+        }, {
+          title: 'functions can take functions as arguments',
+          arguments: ['myTable', function (sequelize) {
+            return { 
+              order: [[sequelize.fn('f1', sequelize.fn('f2', sequelize.col('id'))), 'DESC']] 
+            }
+          }],
+          expectation: "SELECT * FROM `myTable` ORDER BY f1(f2(`id`)) DESC;",
+          context: QueryGenerator,
+          needsSequelize: true
+        }, {
+          title: 'functions can take all types as arguments',
+          arguments: ['myTable', function (sequelize) {
+            return {
+              order: [
+                [sequelize.fn('f1', sequelize.col('myTable.id')), 'DESC'], 
+                [sequelize.fn('f2', 12, 'lalala', new Date(Date.UTC(2011, 2, 27, 10, 1, 55))), 'ASC']
+              ]
+            }
+          }],
+          expectation: "SELECT * FROM `myTable` ORDER BY f1(`myTable`.`id`) DESC, f2(12, 'lalala', '2011-03-27 10:01:55') ASC;",
+          context: QueryGenerator,
+          needsSequelize: true
+        }, {
+          title: 'single string argument is not quoted',
           arguments: ['myTable', {group: "name"}],
-          expectation: "SELECT * FROM `myTable` GROUP BY `name`;",
+          expectation: "SELECT * FROM `myTable` GROUP BY name;",
           context: QueryGenerator
         }, {
-          arguments: ['myTable', {group: ["name"]}],
+          arguments: ['myTable',  { group: ["name"] }],
           expectation: "SELECT * FROM `myTable` GROUP BY `name`;",
           context: QueryGenerator
         }, {
-          arguments: ['myTable', {group: ["name", "title"]}],
-          expectation: "SELECT * FROM `myTable` GROUP BY `name`, `title`;",
-          context: QueryGenerator
+          title: 'functions work for group by',
+          arguments: ['myTable', function (sequelize) {
+            return {
+              group: [sequelize.fn('YEAR', sequelize.col('createdAt'))]
+            }
+          }],
+          expectation: "SELECT * FROM `myTable` GROUP BY YEAR(`createdAt`);",
+          context: QueryGenerator,
+          needsSequelize: true
+        }, {
+          title: 'It is possible to mix sequelize.fn and string arguments to group by',
+          arguments: ['myTable', function (sequelize) {
+            return {
+              group: [sequelize.fn('YEAR', sequelize.col('createdAt')), 'title']
+            }
+          }],
+          expectation: "SELECT * FROM `myTable` GROUP BY YEAR(`createdAt`), `title`;",
+          context: QueryGenerator,
+          needsSequelize: true
         }, {
           arguments: ['myTable', {group: "name", order: "id DESC"}],
-          expectation: "SELECT * FROM `myTable` GROUP BY `name` ORDER BY id DESC;",
+          expectation: "SELECT * FROM `myTable` GROUP BY name ORDER BY id DESC;",
           context: QueryGenerator
         }, {
           arguments: ['myTable', {limit: 10}],
@@ -423,9 +480,12 @@ if (dialect.match(/^mysql/)) {
       describe(suiteTitle, function() {
         tests.forEach(function(test) {
           var title = test.title || 'MySQL correctly returns ' + test.expectation + ' for ' + util.inspect(test.arguments)
-          it(title, function(done) {
+          it(title, function(done) {            
             // Options would normally be set by the query interface that instantiates the query-generator, but here we specify it explicitly
             var context = test.context || {options: {}};
+            if (test.needsSequelize) {
+              test.arguments[1] = test.arguments[1](this.sequelize)
+            }
             QueryGenerator.options = context.options
             var conditions = QueryGenerator[suiteTitle].apply(QueryGenerator, test.arguments)
             expect(conditions).to.deep.equal(test.expectation)

test/postgres/query-generator.test.js

@@ -246,13 +246,73 @@ if (dialect.match(/^postgres/)) {
           expectation: "SELECT * FROM \"myTable\" WHERE foo='bar';"
         }, {
           arguments: ['myTable', {order: "id DESC"}],
-          expectation: "SELECT * FROM \"myTable\" ORDER BY \"id\" DESC;"
-        }, {
+          expectation: "SELECT * FROM \"myTable\" ORDER BY id DESC;"
+        }, {
+          arguments: ['myTable', {order: ["id"]}],
+          expectation: 'SELECT * FROM "myTable" ORDER BY "id";',
+          context: QueryGenerator
+        }, {
+          arguments: ['myTable', {order: ["myTable.id"]}],
+          expectation: 'SELECT * FROM "myTable" ORDER BY "myTable"."id";',
+          context: QueryGenerator
+        }, {
+          arguments: ['myTable', {order: [["id", 'DESC']]}],
+          expectation: 'SELECT * FROM "myTable" ORDER BY "id" DESC;',
+          context: QueryGenerator
+        }, {
+         title: 'raw arguments are neither quoted nor escaped',
+          arguments: ['myTable', {order: [[{raw: 'f1(f2(id))'},'DESC']]}],
+          expectation: 'SELECT * FROM "myTable" ORDER BY f1(f2(id)) DESC;',
+          context: QueryGenerator
+        }, {
+          title: 'functions can take functions as arguments',
+          arguments: ['myTable', function (sequelize) {
+            return { 
+              order: [[sequelize.fn('f1', sequelize.fn('f2', sequelize.col('id'))), 'DESC']] 
+            }
+          }],
+          expectation: 'SELECT * FROM "myTable" ORDER BY f1(f2("id")) DESC;',
+          context: QueryGenerator,
+          needsSequelize: true
+        }, {
+          title: 'functions can take all types as arguments',
+          arguments: ['myTable', function (sequelize) {
+            return {
+              order: [
+                [sequelize.fn('f1', sequelize.col('myTable.id')), 'DESC'], 
+                [sequelize.fn('f2', 12, 'lalala', new Date(Date.UTC(2011, 2, 27, 10, 1, 55))), 'ASC']
+              ]
+            }
+          }],
+          expectation: 'SELECT * FROM "myTable" ORDER BY f1("myTable"."id") DESC, f2(12, \'lalala\', \'2011-03-27 10:01:55.000 +00:00\') ASC;',
+          context: QueryGenerator,
+          needsSequelize: true
+        }, {
+          title: 'single string argument is not quoted',
           arguments: ['myTable', {group: "name"}],
-          expectation: "SELECT * FROM \"myTable\" GROUP BY \"name\";"
+          expectation: "SELECT * FROM \"myTable\" GROUP BY name;"
         }, {
           arguments: ['myTable', {group: ["name"]}],
           expectation: "SELECT * FROM \"myTable\" GROUP BY \"name\";"
+        },  {
+          title: 'functions work for group by',
+         arguments: ['myTable', function (sequelize) {
+            return {
+              group: [sequelize.fn('YEAR', sequelize.col('createdAt'))]
+            }
+          }],
+          expectation: "SELECT * FROM \"myTable\" GROUP BY YEAR(\"createdAt\");",
+          needsSequelize: true
+        },{
+          title: 'It is possible to mix sequelize.fn and string arguments to group by',
+          arguments: ['myTable', function (sequelize) {
+            return {
+              group: [sequelize.fn('YEAR', sequelize.col('createdAt')), 'title']
+            }
+          }],
+          expectation: "SELECT * FROM \"myTable\" GROUP BY YEAR(\"createdAt\"), \"title\";",
+          context: QueryGenerator,
+          needsSequelize: true
         }, {
           arguments: ['myTable', {group: ["name","title"]}],
           expectation: "SELECT * FROM \"myTable\" GROUP BY \"name\", \"title\";"
@@ -792,6 +852,9 @@ if (dialect.match(/^postgres/)) {
           it(title, function(done) {
             // Options would normally be set by the query interface that instantiates the query-generator, but here we specify it explicitly
             var context = test.context || {options: {}};
+            if (test.needsSequelize) {
+              test.arguments[1] = test.arguments[1](this.sequelize)
+            }
             QueryGenerator.options = context.options
             var conditions = QueryGenerator[suiteTitle].apply(QueryGenerator, test.arguments)
             expect(conditions).to.deep.equal(test.expectation)