Various fixes to mssql dialect (#2653)

* Fixed float type of mssql to be float

* Many tests where postgres test was not actually ran at all

* Migrations to be mssql compatible

Mssql driver doesn't handle if multiple queries are sent to same transaction concurrently.

* Prevented mssql failing when invalid schema builder was executed by accident

Instead of trying to generate sql from broken schema calls, just make exception to leak before query compiling is started

* Fixed mssql trx rollback to always throw an error

Also modified some connection test query to be mssql compatible

* Fixed various bugs from MSSQL driver to make tests run

* Fixed mssql unique index to be compatible with other dialect implementations

* Enable running mssql tests on CI

* Test for #2588

* Updated tests to not be dependend on tables left from previous test rans

* Trying to make mssql server work on travis

.travis.yml

@@ -10,13 +10,13 @@ cache:
 matrix:
   include:
   - node_js: "10"
-    env: TEST_ORACLEDB=true DB="maria mysql mysql2 postgres sqlite3 oracledb" CXX=g++-4.8 KNEX_TEST_TIMEOUT=60000 ORACLE_HOME=/u01/app/oracle/product/11.2.0/xe ORACLE_SID=XE OCI_LIB_DIR=/u01/app/oracle/product/11.2.0/xe/lib LD_LIBRARY_PATH=/u01/app/oracle/product/11.2.0/xe/lib
+    env: TEST_ORACLEDB=true DB="mssql mysql mysql2 postgres sqlite3 oracledb" CXX=g++-4.8 KNEX_TEST_TIMEOUT=60000 ORACLE_HOME=/u01/app/oracle/product/11.2.0/xe ORACLE_SID=XE OCI_LIB_DIR=/u01/app/oracle/product/11.2.0/xe/lib LD_LIBRARY_PATH=/u01/app/oracle/product/11.2.0/xe/lib
   - node_js: "8"
-    env: TEST_ORACLEDB=true DB="maria mysql mysql2 postgres sqlite3 oracledb" CXX=g++-4.8 KNEX_TEST_TIMEOUT=60000 ORACLE_HOME=/u01/app/oracle/product/11.2.0/xe ORACLE_SID=XE OCI_LIB_DIR=/u01/app/oracle/product/11.2.0/xe/lib LD_LIBRARY_PATH=/u01/app/oracle/product/11.2.0/xe/lib
+    env: TEST_ORACLEDB=true DB="mssql mysql mysql2 postgres sqlite3 oracledb" CXX=g++-4.8 KNEX_TEST_TIMEOUT=60000 ORACLE_HOME=/u01/app/oracle/product/11.2.0/xe ORACLE_SID=XE OCI_LIB_DIR=/u01/app/oracle/product/11.2.0/xe/lib LD_LIBRARY_PATH=/u01/app/oracle/product/11.2.0/xe/lib
   - node_js: "6"
-    env: TEST_ORACLEDB=true DB="maria mysql mysql2 postgres sqlite3 oracledb" CXX=g++-4.8 KNEX_TEST_TIMEOUT=60000 ORACLE_HOME=/u01/app/oracle/product/11.2.0/xe ORACLE_SID=XE OCI_LIB_DIR=/u01/app/oracle/product/11.2.0/xe/lib LD_LIBRARY_PATH=/u01/app/oracle/product/11.2.0/xe/lib
+    env: TEST_ORACLEDB=true DB="mssql mysql mysql2 postgres sqlite3 oracledb" CXX=g++-4.8 KNEX_TEST_TIMEOUT=60000 ORACLE_HOME=/u01/app/oracle/product/11.2.0/xe ORACLE_SID=XE OCI_LIB_DIR=/u01/app/oracle/product/11.2.0/xe/lib LD_LIBRARY_PATH=/u01/app/oracle/product/11.2.0/xe/lib
   - node_js: "7"
-    env: DB="maria mysql mysql2 postgres sqlite3" CXX=g++-4.8 KNEX_TEST_TIMEOUT=60000
+    env: DB="mssql mysql mysql2 postgres sqlite3" CXX=g++-4.8 KNEX_TEST_TIMEOUT=60000
 
 install: npm i
 
@@ -28,6 +28,9 @@ before_install:
 before_script:
   - psql -c 'create database knex_test;' -U postgres
   - mysql -e 'create database knex_test;'
+  - npm run mssql:init
+  - docker ps -a
+  - netstat -tulpn
 
 after_script:
   - npm run-script coveralls

package.json

@@ -81,6 +81,7 @@
     "test": "npm run pre_test && istanbul --config=test/.istanbul.yml cover node_modules/mocha/bin/_mocha -- --check-leaks -t 10000 -b -R spec test/index.js && npm run tape",
     "oracledb:test": "docker rmi -f --no-prune knex-test-oracledb && docker build -f scripts/oracle-tests-Dockerfile --tag knex-test-oracledb . && docker run -i -t knex-test-oracledb",
     "mssql:init": "docker-compose -f scripts/mssql-docker-compose.yml up --no-start && docker-compose -f scripts/mssql-docker-compose.yml start",
+    "postmssql:init": "node scripts/wait-for-mssql-server.js",
     "mssql:test": "DB=mssql npm test",
     "mssql:destroy": "docker-compose -f scripts/mssql-docker-compose.yml stop",
     "stress:init": "docker-compose -f scripts/stress-test/docker-compose.yml up --no-start && docker-compose -f scripts/stress-test/docker-compose.yml start",

scripts/stress-test/knex-stress-test.js

@@ -37,7 +37,7 @@ const mssql = Knex({
 });
 
 /* TODO: figure out how to nicely install oracledb node driver on osx
-const mysql = Knex({
+const oracledb = Knex({
   client: 'oracledb',
   connection: {
     user          : "travis",

scripts/wait-for-mssql-server.js

@@ -0,0 +1,51 @@
+var Connection = require('tedious').Connection;
+
+var config = {
+  userName: "sa",
+  password: "S0meVeryHardPassword",
+  server: "localhost",
+  options: {
+    database: "knex_test",
+  }
+};
+
+let didConnect = false;
+let tryCount = 0;
+
+function tryToConnect() {
+  tryCount++;
+  if (tryCount > 50) {
+    console.log("Giving up... it fails if it fails");
+    process.exit(0);
+  }
+
+  console.log("Connecting... to mssql");
+
+  var connection = new Connection(config);
+
+  connection.on('end', () => {
+    if (!didConnect) {
+      console.log("Couldnt connnect yet... try again in two secs...");
+      setTimeout(tryToConnect, 2000);
+    }
+  });
+
+  connection.on('error', () => {
+    // prevent leaking errors.. driver seems to sometimes emit error event, 
+    // sometimes connect event with error
+    // and some times just closes connection without error / connect events
+    // (debug event says that socket was ended and thats it...)
+  });
+
+  connection.on('connect', (err) => {
+    if (!err) {
+      console.log("Connecting mssql server was a great success!");
+      didConnect = true;
+    } else {
+      console.log("Error was passed to connect event.");
+    }
+    connection.close();
+  });
+}
+
+tryToConnect();

src/dialects/mssql/index.js

@@ -29,10 +29,10 @@ function Client_MSSQL(config = {}) {
   }
 
   // mssql always creates pool :( lets try to unpool it as much as possible
-  config.pool = {
+  this.mssqlPoolSettings = {
     min: 1,
     max: 1,
-    idleTimeoutMillis: Number.MAX_SAFE_INTEGER,
+    idleTimeoutMillis: Number.MAX_SAFE_INTEGER,  
     evictionRunIntervalMillis: 0
   };
 
@@ -199,7 +199,10 @@ assign(Client_MSSQL.prototype, {
   // connection needs to be added to the pool.
   acquireRawConnection() {
     return new Promise((resolver, rejecter) => {
-      const connection = new this.driver.ConnectionPool(this.connectionSettings);
+      const settings = Object.assign({}, this.connectionSettings);
+      settings.pool = this.mssqlPoolSettings;
+
+      const connection = new this.driver.ConnectionPool(settings);
       connection.connect((err) => {
         if (err) {
           return rejecter(err)

src/dialects/mssql/query/compiler.js

@@ -165,11 +165,14 @@ assign(QueryCompiler_MSSQL.prototype, {
   },
 
   forUpdate() {
-    return 'with (READCOMMITTEDLOCK)';
+    // this doesn't work exacltly as it should, one should also mention index while locking
+    // https://stackoverflow.com/a/9818448/360060
+    return 'with (UPDLOCK)';
   },
 
   forShare() {
-    return 'with (NOLOCK)';
+    // http://www.sqlteam.com/article/introduction-to-locking-in-sql-server
+    return 'with (HOLDLOCK)'; 
   },
 
   // Compiles a `columnInfo` query.

src/dialects/mssql/schema/columncompiler.js

@@ -24,13 +24,12 @@ assign(ColumnCompiler_MSSQL.prototype, {
   bigint: 'bigint',
 
   double(precision, scale) {
-    if (!precision) return 'decimal'
-    return `decimal(${this._num(precision, 8)}, ${this._num(scale, 2)})`
+    return 'float'; 
   },
 
   floating(precision, scale) {
-    if (!precision) return 'decimal'
-    return `decimal(${this._num(precision, 8)}, ${this._num(scale, 2)})`
+    // ignore precicion / scale which is mysql specific stuff
+    return `float`;
   },
 
   integer(length) {

src/dialects/mssql/schema/tablecompiler.js

@@ -19,7 +19,7 @@ inherits(TableCompiler_MSSQL, TableCompiler);
 
 assign(TableCompiler_MSSQL.prototype, {
 
-  createAlterTableMethods: ['foreign', 'primary', 'unique'],
+  createAlterTableMethods: ['foreign', 'primary'],
   createQuery (columns, ifNot) {
     const createStatement = ifNot ? `if object_id('${this.tableName()}', 'U') is null CREATE TABLE ` : 'CREATE TABLE ';
     const sql = createStatement + this.tableName() + (this._formatting ? ' (\n    ' : ' (') + columns.sql.join(this._formatting ? ',\n    ' : ', ') + ')';
@@ -118,11 +118,16 @@ assign(TableCompiler_MSSQL.prototype, {
 
   unique (columns, indexName) {
     indexName = indexName ? this.formatter.wrap(indexName) : this._indexCommand('unique', this.tableNameRaw, columns);
-    if (!this.forCreate) {
-      this.pushQuery(`CREATE UNIQUE INDEX ${indexName} ON ${this.tableName()} (${this.formatter.columnize(columns)})`);
-    } else {
-      this.pushQuery(`CONSTRAINT ${indexName} UNIQUE (${this.formatter.columnize(columns)})`);
+
+    if (!Array.isArray(columns)) {
+      columns = [columns];
     }
+
+    const whereAllTheColumnsAreNotNull = columns.map(column => this.formatter.columnize(column) + ' IS NOT NULL').join(' AND ');
+
+    // make unique constraint that allows null https://stackoverflow.com/a/767702/360060 
+    // to be more or less compatible with other DBs (if any of the columns is NULL then "duplicates" are allowed)
+    this.pushQuery(`CREATE UNIQUE INDEX ${indexName} ON ${this.tableName()} (${this.formatter.columnize(columns)}) WHERE ${whereAllTheColumnsAreNotNull}`);
   },
 
   // Compile a drop index command.
@@ -146,7 +151,7 @@ assign(TableCompiler_MSSQL.prototype, {
   // Compile a drop unique key command.
   dropUnique (column, indexName) {
     indexName = indexName ? this.formatter.wrap(indexName) : this._indexCommand('unique', this.tableNameRaw, column);
-    this.pushQuery(`ALTER TABLE ${this.tableName()} DROP CONSTRAINT ${indexName}`);
+    this.pushQuery(`DROP INDEX ${indexName} ON ${this.tableName()}`);
   }
 
 })

src/dialects/mssql/transaction.js

@@ -1,5 +1,6 @@
 import Promise from 'bluebird';
 import Transaction from '../../transaction';
+import { isUndefined } from 'lodash'
 const debug = require('debug')('knex:tx')
 
 export default class Transaction_MSSQL extends Transaction {
@@ -32,7 +33,13 @@ export default class Transaction_MSSQL extends Transaction {
     debug('%s: rolling back', this.txid)
     return conn.tx_.rollback()
       .then(
-        () => this._rejecter(error),
+        () => {
+          let err = error;
+          if(isUndefined(error)) {
+            err = new Error(`Transaction rejected with non-error: ${error}`)
+          }
+          this._rejecter(err)
+        },
         err => {
           if (error) err.originalError = error;
           return this._rejecter(err);
@@ -58,6 +65,7 @@ export default class Transaction_MSSQL extends Transaction {
         configConnection ||
         t.client.acquireConnection();
     }).tap(function(conn) {
+      conn.__knexTxId = t.txid;
       if (!t.outerTx) {
         t.conn = conn
         conn.tx_ = conn.transaction()

src/migrate/index.js

@@ -198,7 +198,6 @@ export default class Migrator {
 
   _getLock(trx) {
     const transact = trx ? fn => fn(trx) : fn => this.knex.transaction(fn);
-
     return transact(trx => {
       return this._isLocked(trx)
         .then(isLocked => {

test/integration/builder/additional.js

@@ -213,7 +213,7 @@ module.exports = function(knex) {
         .truncate()
         .testSql(function(tester) {
           tester('mysql', 'truncate `test_table_two`');
-          tester('postgresql', 'truncate "test_table_two" restart identity');
+          tester('pg', 'truncate "test_table_two" restart identity');
           tester('pg-redshift', 'truncate "test_table_two"');
           tester('sqlite3', "delete from `test_table_two`");
           tester('oracle', "truncate table \"test_table_two\"");
@@ -289,7 +289,7 @@ module.exports = function(knex) {
               "type": "char"
             }
           });
-        tester('postgresql', 'select * from information_schema.columns where table_name = ? and table_catalog = ? and table_schema = current_schema',
+        tester('pg', 'select * from information_schema.columns where table_name = ? and table_catalog = ? and table_schema = current_schema()',
         null, {
           "enum_value": {
             "defaultValue": null,
@@ -369,7 +369,7 @@ module.exports = function(knex) {
       });
     });
 
-    it('gets the columnInfo', function() {
+    it('gets the columnInfo with columntype', function() {
       return knex('datatype_test').columnInfo('uuid').testSql(function(tester) {
         tester('mysql',
           'select * from information_schema.columns where table_name = ? and table_schema = ?',
@@ -379,7 +379,7 @@ module.exports = function(knex) {
             "nullable": false,
             "type": "char"
           });
-        tester('postgresql', 'select * from information_schema.columns where table_name = ? and table_catalog = ? and table_schema = current_schema',
+        tester('pg', 'select * from information_schema.columns where table_name = ? and table_catalog = ? and table_schema = current_schema()',
         null, {
           "defaultValue": null,
           "maxLength": null,
@@ -465,7 +465,7 @@ module.exports = function(knex) {
           t.renameColumn('about', 'about_col');
         }).testSql(function(tester) {
           tester('mysql', ["show fields from `accounts` where field = ?"]);
-          tester('postgresql', ["alter table \"accounts\" rename \"about\" to \"about_col\""]);
+          tester('pg', ["alter table \"accounts\" rename \"about\" to \"about_col\""]);
           tester('pg-redshift', ["alter table \"accounts\" rename \"about\" to \"about_col\""]);
           tester('sqlite3', ["PRAGMA table_info(`accounts`)"]);
           tester('oracle', ["alter table \"accounts\" rename column \"about\" to \"about_col\""]);
@@ -503,7 +503,7 @@ module.exports = function(knex) {
           t.dropColumn('first_name');
         }).testSql(function(tester) {
           tester('mysql', ["alter table `accounts` drop `first_name`"]);
-          tester('postgresql', ['alter table "accounts" drop column "first_name"']);
+          tester('pg', ['alter table "accounts" drop column "first_name"']);
           tester('pg-redshift', ['alter table "accounts" drop column "first_name"']);
           tester('sqlite3', ["PRAGMA table_info(`accounts`)"]);
           tester('oracle', ['alter table "accounts" drop ("first_name")']);
@@ -720,27 +720,24 @@ module.exports = function(knex) {
     });
 
     it('Event: start', function() {
-      // On redshift, cannot set an identity column to a value
-      if (/redshift/i.test(knex.client.dialect)) { return; }
       return knex('accounts')
-        .insert({id: '999', last_name: 'Start'})
+        .insert({last_name: 'Start event test'})
         .then(function() {
           var queryBuilder = knex('accounts').select();
 
           queryBuilder.on('start', function(builder) {
             //Alter builder prior to compilation
             //Select only one row
             builder
-              .where('id', '999')
+              .where('last_name', 'Start event test')
               .first();
           });
 
           return queryBuilder
         })
         .then(function(row) {
           expect(row).to.exist;
-          expect(String(row.id)).to.equal('999');
-          expect(row.last_name).to.equal('Start');
+          expect(row.last_name).to.equal('Start event test');
         });
     });