Adding support for driver choice and col encrypt

[ctgbarcalow]

What this does:

This modification would allow Windows users to choose their own driver instead of being forced to use the Native Client 11.0. It also makes a slight modification to the default connection strings to allow for the use of encrypted columns.

Related issues:

Pre/Post merge checklist:

• Update change log

[dhensby]

Thanks for this @ctgbarcalow. Please fix the linting issues. Please can you also add some test coverage?

[dhensby]

I'm not a huge fan of adding this config helper to the object that is meant to expose only tests - it feels a bit out of place and confusing that we have one function here that's not actually a test itself. Given it's only used in the test setup once, I'm not sure it needs to be pulled out to a separate function rather than just used inline.

What is the need to verify this anyway? If it's a test assertion it needs to go in a test case (it function) and if it's just a test of the config composition that can go in the unit tests where other config tests are.

[ctgbarcalow]

I agree with your assessment, testing what was modified in the function wasn't straightforward at all to me. I couldn't see a way to call _poolCreate() without a valid config that connects to a server because it in turn calls msnodesql.open(...). I thought about extracting the whole thing to its own _parseConnectionString(...) but that seemed a little overkill and I didn't want to mess with your code that much. All I could think to test at that point was to open an ODBC connection. And that's definitely not a unit test.

[dhensby]

If this rejects does the test fail? I think the assertion needs to be wrapped in a try catch and the error passed to done().

Using promises is a little easier:

sql.query('select serverproperty(N\'servername\') as servername; ').then(() => {
  assert.ok(result.recordset[0].servername)
  done();
}).catch(done);

vs

sql.query('select serverproperty(N\'servername\') as servername; ', (err, result) => {
  if (err) {
    done(err);
  } else {
    try {
      assert.ok(result.recordset[0].servername)
      done()
    } catch (e) {
      done(e)
    }
  }
});

[ctgbarcalow]

Technically it would fail in the before statement when it tried (and failed) to connect.

Maybe all I need is a test of the forced connection and leave it at that.

    ...
    'connection healthy works force ODBC' (config, done) {
      config.options.driver = 'ODBC Driver 17 for SQL Server'
      this['connection healthy works'](config, done)
    },
    ...

[dhensby]

So we are seeing failures in the Visual Studio 2015 builds with the error:

[Microsoft][ODBC Driver Manager] Data source name not found and no default driver specified

Perhaps this is because the ODBC Driver 17 isn't installed on that image? Seems a bit odd that it's only happening on those images.

[dhensby]

I'm just reviving this, but wondering if it's needed now we have the ability for consumers just to pass the connection string directly to the driver: https://github.com/tediousjs/node-mssql/blob/v9.3.0/lib/msnodesqlv8/connection-pool.js#L19-L34

If a connection string is provided, then it is used as-is, allowing consumers to just construct the connection string they need themselves. This has actually been the case for some time.

[dhensby]

We are seeing the same problem as we were on AppVeyor where it appears that ODBC Driver 17 isn't installed - we can install it in our runs and see if that fixes the tests.

[dhensby]

OK - I can't get this passing in CI and I'm not entirely sure why. I'm also not sure this feature is even needed anymore as consumers can provide their own connection string themselves now.

@ctgbarcalow I'm really sorry this has taken a year to get to this point; do you have any interest in trying to get it to pass in CI or do you agree that the feature is now not required given my point above?

[ctgbarcalow]

I'm not able to connect to an SQL Server using the connectionString parameters of config and the ODBC driver. I could be doing it wrong through. I would be happy to help with trying to get it to pass CI but I couldn't find a way to edit the steps. It did look like some of the steps might be OOO.

[dhensby]

So you're able to get this working locally? But if you provide your own connection string it doesn't work?

To get it working in CI we need the right drivers installed, I think, the question is what are they?