fix(mssql): improper parameter binding in sql generation (#8782)

lib/dialects/mssql/query.js

@@ -144,16 +144,9 @@ class Query extends AbstractQuery {
 
   static formatBindParameters(sql, values, dialect) {
     const bindParam = {};
-    let i = 0;
-    const seen = {};
     const replacementFunc = (match, key, values) => {
-      if (seen[key] !== undefined) {
-        return seen[key];
-      }
       if (values[key] !== undefined) {
-        i = i + 1;
         bindParam[key] = values[key];
-        seen[key] = '$' + i;
         return '@' + key;
       }
       return undefined;

test/unit/dialects/mssql/connection-manager.js → test/unit/dialects/mssql/connection-manager.test.js

@@ -3,38 +3,42 @@
 const chai = require('chai'),
   expect = chai.expect,
   Sequelize = require(__dirname + '/../../../../index'),
+  Support = require(__dirname + '/../../support'),
+  dialect = Support.getTestDialect(),
   tedious = require('tedious'),
   sinon = require('sinon'),
   connectionStub = sinon.stub(tedious, 'Connection');
 
 connectionStub.returns({on() {}});
 
-describe('[MSSQL] Connection Manager', () => {
-  let instance,
-    config;
-  beforeEach(() => {
-    config = {
-      dialect: 'mssql',
-      database: 'none',
-      username: 'none',
-      password: 'none',
-      host: 'localhost',
-      port: 2433,
-      pool: {},
-      dialectOptions: {
-        domain: 'TEST.COM'
-      }
-    };
-    instance = new Sequelize(config.database
-      , config.username
-      , config.password
-      , config);
-  });
-
-  it('connectionManager._connect() Does not delete `domain` from config.dialectOptions',
-    () => {
-      expect(config.dialectOptions.domain).to.equal('TEST.COM');
-      instance.dialect.connectionManager._connect(config);
-      expect(config.dialectOptions.domain).to.equal('TEST.COM');
+if (dialect === 'mssql') {
+  describe('[MSSQL Specific] Connection Manager', () => {
+    let instance,
+      config;
+    beforeEach(() => {
+      config = {
+        dialect: 'mssql',
+        database: 'none',
+        username: 'none',
+        password: 'none',
+        host: 'localhost',
+        port: 2433,
+        pool: {},
+        dialectOptions: {
+          domain: 'TEST.COM'
+        }
+      };
+      instance = new Sequelize(config.database
+        , config.username
+        , config.password
+        , config);
     });
-});
+
+    it('connectionManager._connect() Does not delete `domain` from config.dialectOptions',
+      () => {
+        expect(config.dialectOptions.domain).to.equal('TEST.COM');
+        instance.dialect.connectionManager._connect(config);
+        expect(config.dialectOptions.domain).to.equal('TEST.COM');
+      });
+  });
+}

test/unit/dialects/mssql/query.js → test/unit/dialects/mssql/query.test.js

@@ -2,7 +2,8 @@
 
 const path = require('path');
 const Query = require(path.resolve('./lib/dialects/mssql/query.js'));
-const Support = require(path.resolve('./test/support'));
+const Support = require(__dirname + '/../../support');
+const dialect = Support.getTestDialect();
 const sequelize = Support.sequelize;
 const sinon = require('sinon');
 const expect = require('chai').expect;
@@ -12,32 +13,58 @@ const connectionStub = { beginTransaction: () => {}, lib: tedious };
 
 let sandbox, query;
 
-describe('[MSSQL]', () => {
-  describe('beginTransaction', () => {
-    beforeEach(() => {
-      sandbox = sinon.sandbox.create();
-      const options = {
-        transaction: { name: 'transactionName' },
-        isolationLevel: 'REPEATABLE_READ',
-        logging: false
-      };
-      sandbox.stub(connectionStub, 'beginTransaction').callsFake(cb => {
-        cb();
+if (dialect === 'mssql') {
+  describe('[MSSQL Specific] Query', () => {
+    describe('beginTransaction', () => {
+      beforeEach(() => {
+        sandbox = sinon.sandbox.create();
+        const options = {
+          transaction: { name: 'transactionName' },
+          isolationLevel: 'REPEATABLE_READ',
+          logging: false
+        };
+        sandbox.stub(connectionStub, 'beginTransaction').callsFake(cb => {
+          cb();
+        });
+        query = new Query(connectionStub, sequelize, options);
       });
-      query = new Query(connectionStub, sequelize, options);
-    });
 
-    it('should call beginTransaction with correct arguments', () => {
-      return query._run(connectionStub, 'BEGIN TRANSACTION')
-        .then(() => {
-          expect(connectionStub.beginTransaction.called).to.equal(true);
-          expect(connectionStub.beginTransaction.args[0][1]).to.equal('transactionName');
-          expect(connectionStub.beginTransaction.args[0][2]).to.equal(tediousIsolationLevel.REPEATABLE_READ);
-        });
+      it('should call beginTransaction with correct arguments', () => {
+        return query._run(connectionStub, 'BEGIN TRANSACTION')
+          .then(() => {
+            expect(connectionStub.beginTransaction.called).to.equal(true);
+            expect(connectionStub.beginTransaction.args[0][1]).to.equal('transactionName');
+            expect(connectionStub.beginTransaction.args[0][2]).to.equal(tediousIsolationLevel.REPEATABLE_READ);
+          });
+      });
+
+      afterEach(() => {
+        sandbox.restore();
+      });
     });
 
-    afterEach(() => {
-      sandbox.restore();
+    describe('formatBindParameters', () => {
+      it('should convert Sequelize named binding format to MSSQL format', () => {
+        const sql = 'select $one as a, $two as b, $one as c, $three as d, $one as e';
+        const values = { one: 1, two: 2, three: 3 };
+
+        const expected = 'select @one as a, @two as b, @one as c, @three as d, @one as e';
+
+        const result = Query.formatBindParameters(sql, values, dialect);
+        expect(result[0]).to.be.a('string');
+        expect(result[0]).to.equal(expected);
+      });
+
+      it('should convert Sequelize numbered binding format to MSSQL format', () => {
+        const sql = 'select $1 as a, $2 as b, $1 as c, $3 as d, $1 as e';
+        const values = [1, 2, 3];
+
+        const expected = 'select @0 as a, @1 as b, @0 as c, @2 as d, @0 as e';
+
+        const result = Query.formatBindParameters(sql, values, dialect);
+        expect(result[0]).to.be.a('string');
+        expect(result[0]).to.equal(expected);
+      });
     });
   });
-});
+}

test/unit/dialects/mssql/resource-lock.test.js

@@ -2,63 +2,67 @@
 
 const ResourceLock = require('../../../../lib/dialects/mssql/resource-lock'),
   Promise = require('../../../../lib/promise'),
-  assert = require('assert');
+  assert = require('assert'),
+  Support = require(__dirname + '/../../support'),
+  dialect = Support.getTestDialect();
 
-describe('[MSSQL Specific] ResourceLock', () => {
-  it('should process requests serially', () => {
-    const expected = {};
-    const lock = new ResourceLock(expected);
-    let last = 0;
+if (dialect === 'mssql') {
+  describe('[MSSQL Specific] ResourceLock', () => {
+    it('should process requests serially', () => {
+      const expected = {};
+      const lock = new ResourceLock(expected);
+      let last = 0;
 
-    function validateResource(actual) {
-      assert.equal(actual, expected);
-    }
+      function validateResource(actual) {
+        assert.equal(actual, expected);
+      }
 
-    return Promise.all([
-      Promise.using(lock.lock(), resource => {
-        validateResource(resource);
-        assert.equal(last, 0);
-        last = 1;
+      return Promise.all([
+        Promise.using(lock.lock(), resource => {
+          validateResource(resource);
+          assert.equal(last, 0);
+          last = 1;
 
-        return Promise.delay(15);
-      }),
-      Promise.using(lock.lock(), resource => {
-        validateResource(resource);
-        assert.equal(last, 1);
-        last = 2;
-      }),
-      Promise.using(lock.lock(), resource => {
-        validateResource(resource);
-        assert.equal(last, 2);
-        last = 3;
+          return Promise.delay(15);
+        }),
+        Promise.using(lock.lock(), resource => {
+          validateResource(resource);
+          assert.equal(last, 1);
+          last = 2;
+        }),
+        Promise.using(lock.lock(), resource => {
+          validateResource(resource);
+          assert.equal(last, 2);
+          last = 3;
 
-        return Promise.delay(5);
-      })
-    ]);
-  });
+          return Promise.delay(5);
+        })
+      ]);
+    });
 
-  it('should still return resource after failure', () => {
-    const expected = {};
-    const lock = new ResourceLock(expected);
+    it('should still return resource after failure', () => {
+      const expected = {};
+      const lock = new ResourceLock(expected);
 
-    function validateResource(actual) {
-      assert.equal(actual, expected);
-    }
+      function validateResource(actual) {
+        assert.equal(actual, expected);
+      }
 
-    return Promise.all([
-      Promise.using(lock.lock(), resource => {
-        validateResource(resource);
+      return Promise.all([
+        Promise.using(lock.lock(), resource => {
+          validateResource(resource);
 
-        throw new Error('unexpected error');
-      }).catch(() => {}),
-      Promise.using(lock.lock(), validateResource)
-    ]);
-  });
+          throw new Error('unexpected error');
+        }).catch(() => {}),
+        Promise.using(lock.lock(), validateResource)
+      ]);
+    });
 
-  it('should be able to.lock resource without waiting on lock', () => {
-    const expected = {};
-    const lock = new ResourceLock(expected);
+    it('should be able to.lock resource without waiting on lock', () => {
+      const expected = {};
+      const lock = new ResourceLock(expected);
 
-    assert.equal(lock.unwrap(), expected);
+      assert.equal(lock.unwrap(), expected);
+    });
   });
-});
+}