Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Open
ruyadorno opened this issue May 25, 2023 · 2 comments · May be fixed by #1542
Open

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

ruyadorno opened this issue May 25, 2023 · 2 comments · May be fixed by #1542

Comments

@ruyadorno
Copy link
Contributor

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 😊
@ruyadorno ruyadorno changed the title Server and port properties are required when using the custom connector factory method server and port properties are required when using the custom connector factory method May 25, 2023
@ruyadorno ruyadorno mentioned this issue May 25, 2023
Open
@ruyadorno ruyadorno changed the title server and port properties are required when using the custom connector factory method server and port properties are required when using the custom connector factory method May 26, 2023
@MichaelSun90
Copy link
Contributor

MichaelSun90 commented May 26, 2023 •
edited
Loading

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
Copy link
Contributor Author

ruyadorno commented May 26, 2023 •
edited
Loading

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 😊

ruyadorno added a commit to ruyadorno/tedious that referenced this issue May 30, 2023
@ruyadorno
fix: skip config validation when using connector
e86efcc 
This changeset fixes a problem in the original custom `connector`
factory method implementation that required the `server` config property
to be defined even though its value is ignored.

Removing the validation when `config.options.connector` is defined fixes
the problem.

Ref: tediousjs#1540
Fixes: tediousjs#1541
Signed-off-by: Ruy Adorno <[email protected]>
@ruyadorno ruyadorno linked a pull request May 30, 2023 that will close this issue
Open
ruyadorno added a commit to ruyadorno/tedious that referenced this issue May 30, 2023
@ruyadorno
fix: skip config validation when using connector
8c62e1d 
This changeset fixes a problem in the original custom `connector`
factory method implementation that required the `server` config property
to be defined even though its value is ignored.

Removing the validation when `config.options.connector` is defined fixes
the problem.

Ref: tediousjs#1540
Fixes: tediousjs#1541
Signed-off-by: Ruy Adorno <[email protected]>
ruyadorno added a commit to ruyadorno/tedious that referenced this issue Jun 1, 2023
@ruyadorno
fix: skip config validation when using connector
2a7fbe5 
This changeset fixes a problem in the original custom `connector`
factory method implementation that required the `server` config property
to be defined even though its value is ignored.

Removing the validation when `config.options.connector` is defined fixes
the problem.

Ref: tediousjs#1540
Fixes: tediousjs#1541
Signed-off-by: Ruy Adorno <[email protected]>
ruyadorno added a commit to ruyadorno/tedious that referenced this issue Jun 1, 2023
@ruyadorno
fix: skip config validation when using connector
8e92a0b 
This changeset fixes a problem in the original custom `connector`
factory method implementation that required the `server` config property
to be defined even though its value is ignored.

Removing the validation when `config.options.connector` is defined fixes
the problem.

Ref: tediousjs#1540
Fixes: tediousjs#1541
Signed-off-by: Ruy Adorno <[email protected]>
ruyadorno added a commit to ruyadorno/tedious that referenced this issue Jun 2, 2023
@ruyadorno
fix: skip config validation when using connector
02576a6 
This changeset fixes a problem in the original custom `connector`
factory method implementation that required the `server` config property
to be defined even though its value is ignored.

Removing the validation when `config.options.connector` is defined fixes
the problem.

Ref: tediousjs#1540
Fixes: tediousjs#1541
Signed-off-by: Ruy Adorno <[email protected]>
ruyadorno added a commit to ruyadorno/tedious that referenced this issue Jun 5, 2023
@ruyadorno
fix: skip config validation when using connector
3221920 
This changeset fixes a problem in the original custom `connector`
factory method implementation that required the `server` config property
to be defined even though its value is ignored.

Removing the validation when `config.options.connector` is defined fixes
the problem.

Ref: tediousjs#1540
Fixes: tediousjs#1541
Signed-off-by: Ruy Adorno <[email protected]>
ruyadorno added a commit to ruyadorno/tedious that referenced this issue Jun 5, 2023
@ruyadorno
fix: skip config validation when using connector
f5960e4 
This changeset fixes a problem in the original custom `connector`
factory method implementation that required the `server` config property
to be defined even though its value is ignored.

Removing the validation when `config.options.connector` is defined fixes
the problem.

Ref: tediousjs#1540
Fixes: tediousjs#1541
Signed-off-by: Ruy Adorno <[email protected]>
ruyadorno added a commit to ruyadorno/tedious that referenced this issue Jul 24, 2023
@ruyadorno
fix: skip config validation when using connector
634c947 
This changeset fixes a problem in the original custom `connector`
factory method implementation that required the `server` config property
to be defined even though its value is ignored.

Removing the validation when `config.options.connector` is defined fixes
the problem.

Ref: tediousjs#1540
Fixes: tediousjs#1541
Signed-off-by: Ruy Adorno <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feat: skip config validation when using connector ruyadorno/tedious
2 participants
@ruyadorno @MichaelSun90