Merge pull request #2496 from overlookmotel/table-name-in-quote

Fix for ambiguous column names in ORDER BY

changelog.md

@@ -4,6 +4,7 @@
 
 
 #### Backwards compatability changes
 #### Backwards compatability changes
 - When eager-loading a many-to-many association, the attributes of the through table are now accessible through an attribute named after the through model rather than the through table name singularized. i.e. `Task.find({include: Worker})` where the table name for through model `TaskWorker` is `TableTaskWorkers` used to produce `{ Worker: { ..., TableTaskWorker: {...} } }`. It now produces `{ Worker: { ..., TaskWorker: {...} } }`. Does not affect models where table name is auto-defined by Sequelize, or where table name is model name pluralized.
 - When eager-loading a many-to-many association, the attributes of the through table are now accessible through an attribute named after the through model rather than the through table name singularized. i.e. `Task.find({include: Worker})` where the table name for through model `TaskWorker` is `TableTaskWorkers` used to produce `{ Worker: { ..., TableTaskWorker: {...} } }`. It now produces `{ Worker: { ..., TaskWorker: {...} } }`. Does not affect models where table name is auto-defined by Sequelize, or where table name is model name pluralized.
+- When using `Model#find()` with an `order` clause, the table name is prepended to the `ORDER BY` SQL. e.g. `ORDER BY Task.id` rather than `ORDER BY id`. The change is to avoid ambiguous column names where there are eager-loaded associations with the same column names. A side effect is that code like `Task.findAll( { include: [ User ], order: [ [ 'Users.id', 'ASC' ] ] } )` will now throw an error. This should be achieved with `Task.findAll( { include: [ User ], order: [ [ User, 'id', 'ASC' ] ] } )` instead.
 
 
 # 2.0.0-rc2
 # 2.0.0-rc2
 - [FEATURE] Added to posibility of using a sequelize object as key in `sequelize.where`. Also added the option of specifying a comparator
 - [FEATURE] Added to posibility of using a sequelize object as key in `sequelize.where`. Also added the option of specifying a comparator

lib/dialects/abstract/query-generator.js

@@ -574,7 +574,7 @@ module.exports = (function() {
         }
         }
 
 
         // add 1st string as quoted, 2nd as unquoted raw
         // add 1st string as quoted, 2nd as unquoted raw
-        var sql = (i > 0 ? this.quoteIdentifier(tableNames.join('.')) + '.' : '') + this.quote(obj[i], parent, force);
+        var sql = (i > 0 ? this.quoteIdentifier(tableNames.join('.')) + '.' : (Utils._.isString(obj[0]) ? this.quoteIdentifier(parent.name) + '.' : '')) + this.quote(obj[i], parent, force);
         if (i < len - 1) {
         if (i < len - 1) {
           sql += ' ' + obj[i + 1];
           sql += ' ' + obj[i + 1];
         }
         }

test/include/findAll.test.js

@@ -389,7 +389,7 @@ describe(Support.getTestDialectTeaser("Include"), function () {
                     ]}
                     ]}
                   ],
                   ],
                   order: [
                   order: [
-                    ['User.id', 'ASC']
+                    ['id', 'ASC']
                   ]
                   ]
                 }).done(function (err, users) {
                 }).done(function (err, users) {
                   expect(err).not.to.be.ok
                   expect(err).not.to.be.ok
@@ -780,8 +780,8 @@ describe(Support.getTestDialectTeaser("Include"), function () {
               {model: Tag}
               {model: Tag}
             ],
             ],
             order: [
             order: [
-              ['Product.id', 'ASC'],
-              ['Tags.id', 'ASC']
+              ['id', 'ASC'],
+              [Tag, 'id', 'ASC']
             ]
             ]
           }).done(function (err, products) {
           }).done(function (err, products) {
             expect(err).not.to.be.ok
             expect(err).not.to.be.ok
@@ -1460,7 +1460,7 @@ describe(Support.getTestDialectTeaser("Include"), function () {
                     ]}
                     ]}
                   ],
                   ],
                   order: [
                   order: [
-                    ['User.id', 'ASC']
+                    ['id', 'ASC']
                   ]
                   ]
                 }).done(function (err, users) {
                 }).done(function (err, users) {
                   expect(err).not.to.be.ok
                   expect(err).not.to.be.ok
@@ -1709,7 +1709,7 @@ describe(Support.getTestDialectTeaser("Include"), function () {
           ],
           ],
           limit: 6,
           limit: 6,
           order: [
           order: [
-            ['Product.id', 'ASC']
+            ['id', 'ASC']
           ]
           ]
         }).done(function (err, products) {
         }).done(function (err, products) {
           expect(err).not.to.be.ok
           expect(err).not.to.be.ok
@@ -1739,7 +1739,7 @@ describe(Support.getTestDialectTeaser("Include"), function () {
           ],
           ],
           limit: 10,
           limit: 10,
           order: [
           order: [
-            ['Product.id', 'ASC']
+            ['id', 'ASC']
           ]
           ]
         }).done(function (err, products) {
         }).done(function (err, products) {
           expect(err).not.to.be.ok
           expect(err).not.to.be.ok

test/include/schema.test.js

@@ -680,8 +680,8 @@ describe(Support.getTestDialectTeaser("Includes with schemas"), function () {
               {model: Tag}
               {model: Tag}
             ],
             ],
             order: [
             order: [
-              ['Product.id', 'ASC'],
-              ['Tags.id', 'ASC']
+              ['id', 'ASC'],
+              [Tag, 'id', 'ASC']
             ]
             ]
           }).done(function (err, products) {
           }).done(function (err, products) {
             expect(err).not.to.be.ok
             expect(err).not.to.be.ok
@@ -1316,7 +1316,7 @@ describe(Support.getTestDialectTeaser("Includes with schemas"), function () {
                     ]}
                     ]}
                   ],
                   ],
                   order: [
                   order: [
-                    ['User.id', 'ASC']
+                    ['id', 'ASC']
                   ]
                   ]
                 }).done(function (err, users) {
                 }).done(function (err, users) {
                   expect(err).not.to.be.ok
                   expect(err).not.to.be.ok
@@ -1401,7 +1401,7 @@ describe(Support.getTestDialectTeaser("Includes with schemas"), function () {
           ],
           ],
           limit: 3,
           limit: 3,
           order: [
           order: [
-            [self.models.Product.name+'.id', 'ASC']
+            ['id', 'ASC']
           ]
           ]
         }).done(function (err, products) {
         }).done(function (err, products) {
           expect(err).not.to.be.ok
           expect(err).not.to.be.ok
@@ -1430,7 +1430,7 @@ describe(Support.getTestDialectTeaser("Includes with schemas"), function () {
           ],
           ],
           limit: 6,
           limit: 6,
           order: [
           order: [
-            ['Product.id', 'ASC']
+            ['id', 'ASC']
           ]
           ]
         }).done(function (err, products) {
         }).done(function (err, products) {
           expect(err).not.to.be.ok
           expect(err).not.to.be.ok
@@ -1460,7 +1460,7 @@ describe(Support.getTestDialectTeaser("Includes with schemas"), function () {
           ],
           ],
           limit: 10,
           limit: 10,
           order: [
           order: [
-            ['Product.id', 'ASC']
+            ['id', 'ASC']
           ]
           ]
         }).done(function (err, products) {
         }).done(function (err, products) {
           expect(err).not.to.be.ok
           expect(err).not.to.be.ok

test/mysql/query-generator.test.js

@@ -163,9 +163,10 @@ if (Support.dialectIsMySQL()) {
           expectation: "SELECT * FROM `myTable` ORDER BY `myTable`.`id`;",
           expectation: "SELECT * FROM `myTable` ORDER BY `myTable`.`id`;",
           context: QueryGenerator
           context: QueryGenerator
         }, {
         }, {
-          arguments: ['myTable', {order: [["id", 'DESC']]}],
-          expectation: "SELECT * FROM `myTable` ORDER BY `id` DESC;",
-          context: QueryGenerator
+          arguments: ['myTable', {order: [["id", 'DESC']]}, function(sequelize) {return sequelize.define('myTable', {});}],
+          expectation: "SELECT * FROM `myTable` AS `myTable` ORDER BY `myTable`.`id` DESC;",
+          context: QueryGenerator,
+          needsSequelize: true
         }, {
         }, {
           title: 'raw arguments are neither quoted nor escaped',
           title: 'raw arguments are neither quoted nor escaped',
           arguments: ['myTable', {order: [[{raw: 'f1(f2(id))'}, 'DESC']]}],
           arguments: ['myTable', {order: [[{raw: 'f1(f2(id))'}, 'DESC']]}],
@@ -588,7 +589,8 @@ if (Support.dialectIsMySQL()) {
             // Options would normally be set by the query interface that instantiates the query-generator, but here we specify it explicitly
             // 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: {}};
             var context = test.context || {options: {}};
             if (test.needsSequelize) {
             if (test.needsSequelize) {
-              test.arguments[1] = test.arguments[1](this.sequelize);
+              if (_.isFunction(test.arguments[1])) test.arguments[1] = test.arguments[1](this.sequelize);
+              if (_.isFunction(test.arguments[2])) test.arguments[2] = test.arguments[2](this.sequelize);
             }
             }
             QueryGenerator.options = context.options;
             QueryGenerator.options = context.options;
             QueryGenerator._dialect = this.sequelize.dialect;
             QueryGenerator._dialect = this.sequelize.dialect;

test/postgres/query-generator.test.js

@@ -249,9 +249,10 @@ if (dialect.match(/^postgres/)) {
           expectation: 'SELECT * FROM "myTable" ORDER BY "myTable"."id";',
           expectation: 'SELECT * FROM "myTable" ORDER BY "myTable"."id";',
           context: QueryGenerator
           context: QueryGenerator
         }, {
         }, {
-          arguments: ['myTable', {order: [["id", 'DESC']]}],
-          expectation: 'SELECT * FROM "myTable" ORDER BY "id" DESC;',
-          context: QueryGenerator
+          arguments: ['myTable', {order: [["id", 'DESC']]}, function(sequelize) {return sequelize.define('myTable', {});}],
+          expectation: 'SELECT * FROM "myTable" AS "myTable" ORDER BY "myTable"."id" DESC;',
+          context: QueryGenerator,
+          needsSequelize: true
         }, {
         }, {
          title: 'raw arguments are neither quoted nor escaped',
          title: 'raw arguments are neither quoted nor escaped',
           arguments: ['myTable', {order: [[{raw: 'f1(f2(id))'},'DESC']]}],
           arguments: ['myTable', {order: [[{raw: 'f1(f2(id))'},'DESC']]}],
@@ -984,7 +985,8 @@ if (dialect.match(/^postgres/)) {
             // Options would normally be set by the query interface that instantiates the query-generator, but here we specify it explicitly
             // 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: {}};
             var context = test.context || {options: {}};
             if (test.needsSequelize) {
             if (test.needsSequelize) {
-              test.arguments[1] = test.arguments[1](this.sequelize);
+              if (_.isFunction(test.arguments[1])) test.arguments[1] = test.arguments[1](this.sequelize);
+              if (_.isFunction(test.arguments[2])) test.arguments[2] = test.arguments[2](this.sequelize);
             }
             }
             QueryGenerator.options = context.options;
             QueryGenerator.options = context.options;
             QueryGenerator._dialect = this.sequelize.dialect;
             QueryGenerator._dialect = this.sequelize.dialect;

test/sqlite/query-generator.test.js

@@ -154,9 +154,10 @@ if (dialect === 'sqlite') {
           expectation: "SELECT * FROM `myTable` ORDER BY `myTable`.`id`;",
           expectation: "SELECT * FROM `myTable` ORDER BY `myTable`.`id`;",
           context: QueryGenerator
           context: QueryGenerator
         }, {
         }, {
-          arguments: ['myTable', {order: [["id", 'DESC']]}],
-          expectation: "SELECT * FROM `myTable` ORDER BY `id` DESC;",
-          context: QueryGenerator
+          arguments: ['myTable', {order: [["id", 'DESC']]}, function(sequelize) {return sequelize.define('myTable', {});}],
+          expectation: "SELECT * FROM `myTable` AS `myTable` ORDER BY `myTable`.`id` DESC;",
+          context: QueryGenerator,
+          needsSequelize: true
         }, {
         }, {
           title: 'raw arguments are neither quoted nor escaped',
           title: 'raw arguments are neither quoted nor escaped',
           arguments: ['myTable', {order: [[{raw: 'f1(f2(id))'}, 'DESC']]}],
           arguments: ['myTable', {order: [[{raw: 'f1(f2(id))'}, 'DESC']]}],
@@ -536,7 +537,8 @@ if (dialect === 'sqlite') {
             // Options would normally be set by the query interface that instantiates the query-generator, but here we specify it explicitly
             // 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: {}};
             var context = test.context || {options: {}};
             if (test.needsSequelize) {
             if (test.needsSequelize) {
-              test.arguments[1] = test.arguments[1](this.sequelize);
+              if (_.isFunction(test.arguments[1])) test.arguments[1] = test.arguments[1](this.sequelize);
+              if (_.isFunction(test.arguments[2])) test.arguments[2] = test.arguments[2](this.sequelize);
             }
             }
             QueryGenerator.options = context.options;
             QueryGenerator.options = context.options;
             QueryGenerator._dialect = this.sequelize.dialect;
             QueryGenerator._dialect = this.sequelize.dialect;