feat: skip config validation when using connector #1542

ruyadorno · 2023-05-30T20:38:46Z

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: #1540
Fixes: #1541

codecov · 2023-05-30T21:33:03Z

Codecov Report

Merging #1542 (20bf8f1) into master (4f3e210) will decrease coverage by 0.50%.
The diff coverage is 93.10%.

❗ Current head 20bf8f1 differs from pull request most recent head faff247. Consider uploading reports for the commit faff247 to get more accurate results

@@            Coverage Diff             @@
##           master    #1542      +/-   ##
==========================================
- Coverage   80.45%   79.96%   -0.50%     
==========================================
  Files          92       92              
  Lines        4692     4682      -10     
  Branches      871      866       -5     
==========================================
- Hits         3775     3744      -31     
- Misses        644      672      +28     
+ Partials      273      266       -7

Files Changed	Coverage Δ
src/connection.ts	`62.74% <93.10%> (-2.00%)`	⬇️

... and 8 files with indirect coverage changes

src/connection.ts

MichaelSun90 · 2023-05-31T17:22:32Z

src/connection.ts

@@ -1902,6 +1902,9 @@ class Connection extends EventEmitter {

    if (this.config.options.port) {
      return this.connectOnPort(this.config.options.port, this.config.options.multiSubnetFailover, signal, this.config.options.connector);
+    } else if (this.config.options.connector) {
+      // port and multiSubnetFailover are not used when using a custom connector
+      return this.connectOnPort(0, false, signal, this.config.options.connector);


There is a possibility that user proved both port and connector from config, the logic will go into the if statement instead of the else if and still work properly since the connector is passed into connectOnPort. Another case is, if use only pass in connector and logic will got into the else if and still call the connectOnPort.
Under both cases, with or without port passed into connectOnPort function, the connect socket here will be provided by the passed in customConnector, and the passed in port number should be ignored within connectOnPort.

thanks @MichaelSun90! these are also great examples of the kind of tests I can add to make codecov happy 😊

I added the connector check to take precedence just to err on the safe side but at the same time I made sure to set port to undefined during the validation phase of the constructor along with adding tests for the combinations of providing port/server/connector. Let me know if it looks better now 😊

arthurschreiber · 2023-05-31T19:41:22Z

@ruyadorno I'm wondering how this is going to interact with all the code that expects config.server (and also config.port) to be set? There's the type checking that's supposed to make sure that config.server is set to an actual value, and here's a whole bunch of logging code that expects config.server to be set to a string (and config.port to be set as well as).

It would be nice if that all could be cleaned up to either be consistent, or at least not to lead to log messages containing undefined as the server name and a bogus port. 🤔

ruyadorno · 2023-05-31T20:25:42Z

@arthurschreiber that's a good point! let me take a look at these config.server logging LOC 👀

I might just try to clean them up as part of this PR then in case it looks actionable 😊

ruyadorno · 2023-06-01T20:52:20Z

@arthurschreiber I updated the PR with a pass at updating occurrences of many logging messages that were using the server:port combo!

ruyadorno · 2023-06-05T20:34:42Z

Went through a few more iterations to improve code coverage and cleaned a few things up, should be good to go now 😊 lmk!

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 <ruyadorno@google.com>

ruyadorno · 2023-07-24T21:06:52Z

hi @MichaelSun90! Added your suggestion to update the type definition for the server config (from c72786c) along with a few type conversions for TS lint and rebased on top of the latest commits from master.

Let me know if you think this is in a good shape to land now 😊 I'm looking forward to finally have the connector config usage cleaned up!

…-factory-method

arthurschreiber · 2023-09-06T19:22:56Z

src/instance-lookup.ts

@@ -14,7 +14,7 @@ const MYSTERY_HEADER_LENGTH = 3;
 type LookupFunction = (hostname: string, options: dns.LookupAllOptions, callback: (err: NodeJS.ErrnoException | null, addresses: dns.LookupAddress[]) => void) => void;

 // Most of the functionality has been determined from from jTDS's MSSqlServerInfo class.
-export async function instanceLookup(options: { server: string, instanceName: string, timeout?: number, retries?: number, port?: number, lookup?: LookupFunction, signal: AbortSignal }) {
+export async function instanceLookup(options: { server: undefined | string, instanceName: string, timeout?: number, retries?: number, port?: number, lookup?: LookupFunction, signal: AbortSignal }) {


@ruyadorno I think this change is not correct. Logically, the instanceLookup can not work without a hostname, so changing the type to allow undefined is not correct. Also, casting options.server to a string using String further down is also not right - if someone would call this method with undefined, it would try to send udp messages to the hostname "undefined". 😅

I think it's better to change the location where this method is called instead (will leave a separate comment for that).

arthurschreiber · 2023-09-06T19:24:52Z

src/connection.ts

+    if (this.config.options.connector) {
+      // port and multiSubnetFailover are not used when using a custom connector
+      return this.connectOnPort(0, false, signal, this.config.options.connector);
+    } else if (this.config.options.port) {
      return this.connectOnPort(this.config.options.port, this.config.options.multiSubnetFailover, signal, this.config.options.connector);
    } else {
      return instanceLookup({


I think this is the place you need to change. We know here that this.config.server is not undefined in this else block (even if the type is declared as string | undefined), so we can tell TypeScript that this is the case by changing this.config.server to this.config.server!.

ruyadorno force-pushed the fix-config-validation-when-using-connector-factory-method branch from e86efcc to 8c62e1d Compare May 30, 2023 20:49

ruyadorno mentioned this pull request May 30, 2023

Tedious sample config contains extraneous properties GoogleCloudPlatform/cloud-sql-nodejs-connector#132

Open

MichaelSun90 reviewed May 31, 2023

View reviewed changes

src/connection.ts Show resolved Hide resolved

MichaelSun90 reviewed May 31, 2023

View reviewed changes

ruyadorno force-pushed the fix-config-validation-when-using-connector-factory-method branch 2 times, most recently from 2a7fbe5 to 8e92a0b Compare June 1, 2023 20:51

ruyadorno force-pushed the fix-config-validation-when-using-connector-factory-method branch 3 times, most recently from 3221920 to f5960e4 Compare June 5, 2023 18:20

ruyadorno force-pushed the fix-config-validation-when-using-connector-factory-method branch from 20bf8f1 to 634c947 Compare July 24, 2023 21:02

arthurschreiber changed the title ~~fix: skip config validation when using connector~~ feat: skip config validation when using connector Aug 21, 2023

Merge branch 'master' into fix-config-validation-when-using-connector…

faff247

…-factory-method

arthurschreiber reviewed Sep 6, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: skip config validation when using connector #1542

feat: skip config validation when using connector #1542

ruyadorno commented May 30, 2023

codecov bot commented May 30, 2023 •

edited

MichaelSun90 May 31, 2023

ruyadorno May 31, 2023

ruyadorno Jun 1, 2023

arthurschreiber commented May 31, 2023

ruyadorno commented May 31, 2023

ruyadorno commented Jun 1, 2023 •

edited

ruyadorno commented Jun 5, 2023

ruyadorno commented Jul 24, 2023

arthurschreiber Sep 6, 2023

arthurschreiber Sep 6, 2023

feat: skip config validation when using connector #1542

Are you sure you want to change the base?

feat: skip config validation when using connector #1542

Conversation

ruyadorno commented May 30, 2023

codecov bot commented May 30, 2023 • edited

Codecov Report

MichaelSun90 May 31, 2023

Choose a reason for hiding this comment

ruyadorno May 31, 2023

Choose a reason for hiding this comment

ruyadorno Jun 1, 2023

Choose a reason for hiding this comment

arthurschreiber commented May 31, 2023

ruyadorno commented May 31, 2023

ruyadorno commented Jun 1, 2023 • edited

ruyadorno commented Jun 5, 2023

ruyadorno commented Jul 24, 2023

arthurschreiber Sep 6, 2023

Choose a reason for hiding this comment

arthurschreiber Sep 6, 2023

Choose a reason for hiding this comment

codecov bot commented May 30, 2023 •

edited

ruyadorno commented Jun 1, 2023 •

edited