Fix when dropping a unique index in MSSQL

[wilsonge]

If an MSSQL unique index is created as an index https://github.com/wilsonge/knex/blob/156d1128de1f692dd6de2841198884deaa8276d8/src/dialects/mssql/schema/tablecompiler.js#L121-L125 it needs to be dropped as an index not as a constraint. You can also see in the tests that we are creating index's and dropping constraints.

https://docs.microsoft.com/en-us/sql/t-sql/statements/drop-index-transact-sql?view=sql-server-2017 isn't very clear - it says The DROP INDEX statement does not apply to indexes created by defining PRIMARY KEY or UNIQUE constraints isn't as clear as it should be - it basically says that you must drop a unique constraint as a constraint. But that means you should still drop unique index's as an index.

This is tested on a local mssql docker container

[kibertoad]

@smorey2 Ping?..

[elhigu]

This is at least missing an integration test, which shows that query works as intented

[wilsonge]

The existing tests cover this already - I'm not exactly sure what extra tests you want added?

[kibertoad]

@wilsonge Integration test for dropping unique index that is failing without changes in this PR.

[elhigu]

@wilsonge existing test checks that query is built as intended, which is no an integration test since it doesn't write anything to DB. In integration test that query is actually ran against real DB and then it should verify that the query indeed actually did drop the index correctly.

[wilsonge]

Yes I understand what an integration test is. But you don't have any mssql integration tests - all the integration tests run in sqlite - so for a simple bug fix - getting me to create a full set of mssql integration tests on travis - seems like a slightly harsh requirement?

[kibertoad]

@wilsonge Are you sure this is accurate? If you check test/knexfile.js, there is configuration for test mssql database, and it is my understanding that existing integration tests are also run on MSSQL already.

[kibertoad]

@wilsonge MSSQL-specific tests do not get executed in Travis, I think (set of databases for CI execution is "maria mysql mysql2 postgres sqlite3 oracledb"), but I don't think that there is an expectation of you building entire solution for running MSSQL tests in CI; just writing a test that would be passing for you locally and in CI after someone eventually adds dockerized mssql test execution would probably suffice.

[wilsonge]

OK where should this test live then? Because I read this https://github.com/tgriesser/knex/blob/master/test/integration/index.js as only having sqllite code in. And I don't see any place for tests of different dialects at a quick pass through? More than happy to write something if I'm pointed in the right direction tho :)

[kibertoad]

@wilsonge Check out test/integration/schema/index.js to see how testing for different databases is done, and it's probably appropriate place for your case as well.

There are also cases of db-specific tests there, e. g.

    describe('addColumn', function() {
      describe('mysql only', function() {
        if (!knex ||
            !knex.client ||
            (!(/mysql/i.test(knex.client.dialect)) && !(/maria/i.test(knex.client.dialect)))
        ) {
          return Promise.resolve();
        }

        before(function() {
          return knex.schema.createTable('add_column_test_mysql', function (tbl) {
            tbl.integer('field_foo');
            tbl.integer('field_bar');
          }).then(function() {
            return knex.schema.alterTable('add_column_test_mysql', function (tbl) {
              tbl.integer('field_foo').comment('foo').alter();
              tbl.integer('field_bar').comment('bar').alter();
              tbl.integer('field_first').first().comment('First');
              tbl.integer('field_after_foo').after('field_foo').comment('After');
              tbl.increments('field_nondefault_increments').comment('Comment on increment col');
            });
          });
        });

(not sure if you'd need something like that, presumably creating and dropping unique index is a universal thing with similar outcome, and how exactly it is done is beyond an integration test)

[elhigu]

This PR is now irrelevant. In #2653 I had to change the way how unique constraint is created to be always just an unique index instead of creating it as unique constraint (unique constraint didn't allow to create partial index which was necessary for constraint to work the same way with other dialects). I'll add integration test for create / drop / create unique constraint there too... that way at least if some query doesn't work some of the queries should throw an error.