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.

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

feat: skip config validation when using connector #1542

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ruyadorno
Copy link
Contributor

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

codecov bot commented May 30, 2023

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

@@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Collaborator

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

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

ruyadorno commented Jun 1, 2023

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

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

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 <[email protected]>
@ruyadorno ruyadorno force-pushed the fix-config-validation-when-using-connector-factory-method branch from 20bf8f1 to 634c947 Compare July 24, 2023 21:02
@ruyadorno
Copy link
Contributor Author

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!

@arthurschreiber arthurschreiber changed the title fix: skip config validation when using connector feat: skip config validation when using connector Aug 21, 2023
@@ -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 }) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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).

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({
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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