server and port properties are required when using the custom connector factory method

[ruyadorno]

Software versions

• Tedious: 16.1.0
• Node.js: 16+

Additional Libraries Used and Versions

Particularly noticeable when using alongside the @google-cloud/cloud-sql-connector library that uses the connector factory method.

Connection configuration

Notice the highlighted properties that should not be required, since a custom connector function is being used:

const connection = new Connection({
-  server: '0.0.0.0',
  authentication: {
    type: 'default',
    options: {
      userName: process.env.SQLSERVER_USER,
      password: process.env.SQLSERVER_PASS,
    },
  },
  options: {
    connector: async () => net.connect({
      host: '192.168.1.212',
      port: 1433,
    }),
-    port: 9999,
    database: process.env.SQLSERVER_DB,
  },
})

Problem description

When using the new custom connector factory method, the server and port properties should not be required.

Expected behavior

Config options defining only a connector factory method should be valid.

Actual behavior

Error message/stack trace

The "config.server" property is required and must be of type string.

  test: open connection and run basic sqlserver commands
  stack: |
    new Connection (node_modules/tedious/src/connection.ts:1056:13)
    Test.<anonymous> (system-test/tedious-connect.cjs:30:24)
  at:
    line: 1056
    column: 13
    file: node_modules/tedious/src/connection.ts
    function: Connection

Any other details that can be helpful

Please note that I'm the original contributor of that feature and will be happy to follow up with a fix for this problem 😊

[MichaelSun90]

Hi @ruyadorno, thanks for catching this. The current validation logic need to be altered to accommodate the custom connector. if you do not have the band width, I can definitely make the fix later. These are my ideas for fixing this:

Under connection.ts, there is a type check to ensure user provide a server value before which throws the error message that you described:

if (typeof config.server !== 'string') {
      throw new TypeError('The "config.server" property is required and must be of type string.');
    }

it need to be latered to also check whether custom connector is used.

if (typeof config.server !== 'string' &&  typeof config.connector !== 'function') {
        throw new TypeError('The "config.server" property is required and must be of type string if "config.connector" is not used');
}

@arthurschreiber , Do you think this message looks OK?

For port, it should be ok to leave it empty since it used to be a optional entry. Also, from the code, the connect function seems directly use the socket that returned by custom connector, so it should use whatever port number passed in from the custom connector side and ignore the default value for connection configuration options.port.

[ruyadorno]

hi @MichaelSun90 thank you for the pointers! That sounds about what I had in mind initially!

For port, it should be ok to leave it empty since it used to be a optional entry. Also, from the code, the connect function seems directly use the socket that returned by custom connector, so it should use whatever port number passed in from the custom connector side and ignore the default value for connection configuration options.port.

I believe that port is also required because of this condition logic here:

tedious/src/connection.ts

Lines 1903 to 1904 in e4eadf8

	if (this.config.options.port) {
	return this.connectOnPort(this.config.options.port, this.config.options.multiSubnetFailover, signal, this.config.options.connector);

Something like if (this.config.options.port || this.config.options.connector) { can hopefully be enough to make it work here 😊