fix(mssql): duplicate order in FETCH/NEXT queries (#12257)

lib/dialects/mssql/query-generator.js

@@ -917,8 +917,18 @@ class MSSQLQueryGenerator extends AbstractQueryGenerator {
 
     if (options.limit || options.offset) {
       if (!options.order || options.include && !orders.subQueryOrder.length) {
-        fragment += options.order && !isSubQuery ? ', ' : ' ORDER BY ';
-        fragment += `${this.quoteTable(options.tableAs || model.name)}.${this.quoteIdentifier(model.primaryKeyField)}`;
+        const tablePkFragment = `${this.quoteTable(options.tableAs || model.name)}.${this.quoteIdentifier(model.primaryKeyField)}`;
+        if (!options.order) {
+          fragment += ` ORDER BY ${tablePkFragment}`;
+        } else {
+          const orderFieldNames = _.map(options.order, order => order[0]);
+          const primaryKeyFieldAlreadyPresent = _.includes(orderFieldNames, model.primaryKeyField);
+
+          if (!primaryKeyFieldAlreadyPresent) {
+            fragment += options.order && !isSubQuery ? ', ' : ' ORDER BY ';
+            fragment += tablePkFragment;
+          }
+        }
       }
 
       if (options.offset || options.limit) {

test/integration/dialects/mssql/regressions.test.js

@@ -8,7 +8,7 @@ const chai = require('chai'),
   dialect = Support.getTestDialect();
 
 if (dialect.match(/^mssql/)) {
-  describe('[MSSQL Specific] Regressions', () => {
+  describe(Support.getTestDialectTeaser('Regressions'), () => {
     it('does not duplicate columns in ORDER BY statement, #9008', async function() {
       const LoginLog = this.sequelize.define('LoginLog', {
         ID: {
@@ -80,72 +80,120 @@ if (dialect.match(/^mssql/)) {
       expect(logs).to.have.length(2);
       expect(logs[0].User.get('UserName')).to.equal('Shaktimaan');
       expect(logs[1].User.get('UserName')).to.equal('Aryamaan');
-    });
-  });
-
-  it('sets the varchar(max) length correctly on describeTable', async function() {
-    const Users = this.sequelize.define('_Users', {
-      username: Sequelize.STRING('MAX')
-    }, { freezeTableName: true });
 
-    await Users.sync({ force: true });
-    const metadata = await this.sequelize.getQueryInterface().describeTable('_Users');
-    const username = metadata.username;
-    expect(username.type).to.include('(MAX)');
-  });
+      // #11258 and similar
+      const otherLogs = await LoginLog.findAll({
+        include: [
+          {
+            model: User,
+            where: {
+              UserName: {
+                [Op.like]: '%maan%'
+              }
+            }
+          }
+        ],
+        order: [['id', 'DESC']],
+        offset: 0,
+        limit: 10
+      });
 
-  it('sets the char(10) length correctly on describeTable', async function() {
-    const Users = this.sequelize.define('_Users', {
-      username: Sequelize.CHAR(10)
-    }, { freezeTableName: true });
+      expect(otherLogs).to.have.length(2);
+      expect(otherLogs[0].User.get('UserName')).to.equal('Aryamaan');
+      expect(otherLogs[1].User.get('UserName')).to.equal('Shaktimaan');
 
-    await Users.sync({ force: true });
-    const metadata = await this.sequelize.getQueryInterface().describeTable('_Users');
-    const username = metadata.username;
-    expect(username.type).to.include('(10)');
-  });
+      // Separate queries can apply order freely
+      const separateUsers = await User.findAll({
+        include: [
+          {
+            model: LoginLog,
+            separate: true,
+            order: [
+              'id'
+            ]
+          }
+        ],
+        where: {
+          UserName: {
+            [Op.like]: '%maan%'
+          }
+        },
+        order: ['UserName', ['UserID', 'DESC']],
+        offset: 0,
+        limit: 10
+      });
 
-  it('saves value bigger than 2147483647, #11245', async function() {
-    const BigIntTable =  this.sequelize.define('BigIntTable', {
-      business_id: {
-        type: Sequelize.BIGINT,
-        allowNull: false
-      }
-    }, {
-      freezeTableName: true
+      expect(separateUsers).to.have.length(2);
+      expect(separateUsers[0].get('UserName')).to.equal('Aryamaan');
+      expect(separateUsers[0].get('LoginLogs')).to.have.length(1);
+      expect(separateUsers[1].get('UserName')).to.equal('Shaktimaan');
+      expect(separateUsers[1].get('LoginLogs')).to.have.length(1);
     });
 
-    const bigIntValue = 2147483648;
-
-    await BigIntTable.sync({ force: true });
+    it('sets the varchar(max) length correctly on describeTable', async function() {
+      const Users = this.sequelize.define('_Users', {
+        username: Sequelize.STRING('MAX')
+      }, { freezeTableName: true });
 
-    await BigIntTable.create({
-      business_id: bigIntValue
+      await Users.sync({ force: true });
+      const metadata = await this.sequelize.getQueryInterface().describeTable('_Users');
+      const username = metadata.username;
+      expect(username.type).to.include('(MAX)');
     });
 
-    const record = await BigIntTable.findOne();
-    expect(Number(record.business_id)).to.equals(bigIntValue);
-  });
+    it('sets the char(10) length correctly on describeTable', async function() {
+      const Users = this.sequelize.define('_Users', {
+        username: Sequelize.CHAR(10)
+      }, { freezeTableName: true });
 
-  it('saves boolean is true, #12090', async function() {
-    const BooleanTable =  this.sequelize.define('BooleanTable', {
-      status: {
-        type: Sequelize.BOOLEAN,
-        allowNull: false
-      }
-    }, {
-      freezeTableName: true
+      await Users.sync({ force: true });
+      const metadata = await this.sequelize.getQueryInterface().describeTable('_Users');
+      const username = metadata.username;
+      expect(username.type).to.include('(10)');
     });
 
-    const value = true;
+    it('saves value bigger than 2147483647, #11245', async function() {
+      const BigIntTable =  this.sequelize.define('BigIntTable', {
+        business_id: {
+          type: Sequelize.BIGINT,
+          allowNull: false
+        }
+      }, {
+        freezeTableName: true
+      });
+
+      const bigIntValue = 2147483648;
 
-    await BooleanTable.sync({ force: true });
+      await BigIntTable.sync({ force: true });
 
-    await BooleanTable.create({
-      status: value
+      await BigIntTable.create({
+        business_id: bigIntValue
+      });
+
+      const record = await BigIntTable.findOne();
+      expect(Number(record.business_id)).to.equals(bigIntValue);
     });
 
-    const record = await BooleanTable.findOne();
-    expect(record.status).to.equals(value);
+    it('saves boolean is true, #12090', async function() {
+      const BooleanTable =  this.sequelize.define('BooleanTable', {
+        status: {
+          type: Sequelize.BOOLEAN,
+          allowNull: false
+        }
+      }, {
+        freezeTableName: true
+      });
+
+      const value = true;
+
+      await BooleanTable.sync({ force: true });
+
+      await BooleanTable.create({
+        status: value
+      });
+
+      const record = await BooleanTable.findOne();
+      expect(record.status).to.equals(value);
+    });
   });
 }