tediousjs / node-mssql

Microsoft SQL Server client for Node.js
https://tediousjs.github.io/node-mssql
MIT License
2.23k stars 466 forks source link

Support passing multiSubnetFailover option to tedious driver #1626

Closed isaru66 closed 6 months ago

isaru66 commented 6 months ago

What this does:

This PR allow option multiSubnetFailover to be passed from typeorm -> node-mssql -> tedious driver.

This option allow support for MSSQL alway on avalibility group that primary site and secondary site using IP in difference subnet. image ref: https://learn.microsoft.com/en-us/sql/sql-server/failover-clusters/windows/sql-server-multi-subnet-clustering-sql-server?view=sql-server-ver16#DNS in tediousjs this feature was implemented in https://github.com/tediousjs/tedious/pull/362

with current version of TypeORM (0.3.20), multiSubnetFailover option can be passed in extra section as below.

export const AppDataSource = new DataSource({
    type: "mssql",
    host: "127.0.0.1",
    username: "sa",
    password: "somepassword",
    database: "tempdb",
    synchronize: true,
    logging: false,
    entities: [User],
    migrations: [],
    subscribers: [],
    options: {
        trustServerCertificate: true,  
    },
    extra: {
        multiSubnetFailover: true,
    }
})

however, without this PR. TypeORM pass multiSubnetFailover config option to Node-MSSQL, however Node-MSSQL will never be passed this option to TediousJS.

Related issues:

https://github.com/tediousjs/node-mssql/issues/1619

Pre/Post merge checklist:

dhensby commented 6 months ago

If you see line 14-17 of the file you've updated. All the options passed are added to cfg.options, so this change is redundant and I do not believe is the cause of the problem.

As pointed out in the issue, there is no extras prop supported.

Can you confirm you've actually run this locally and it works Vs the current stable release, because I can't see how this makes a difference nor magically jumps a prop from extras to options.

dhensby commented 6 months ago

From your reply in the linked issue, I am further convinced this will not "work" and, as pointed out, this is a redundant change as the props are already present.

isaru66 commented 6 months ago

@dhensby

I have confirm that I have run this locally, and we need the proposed code change to pass multiSubnetFailover option to tediousjs driver.

From TypeORM, node-mssql was created via mssql.ConnectionPool(connectionOptions) image

I have debug the code locally, if the current code worked, At line 39 multiSubnetFailover should be already in cfg.options already, but it doesn't. image

Another things is, as trustServerCertificate and multiSubnetFailover is configured at the same level. We can see that trustServerCertificate also required to be explicitely defined in order to be passed down to tedious driver at line16 as well. image

I also test this code in our customers environment and confirm that once we added the code fix, the tedius driver now use parallel connection strategy on the actual Microsoft SQL server that configure multi-subnet Always on avalibility group.

Currently we need to fork node-mssql and applied patch in another repo, but I think it would be good if the fix get merged up to node-mssql directly. If the code styling is not good - I can change based on your comments.

dhensby commented 6 months ago

@isaru66 I deleted your comment because I don't think it's good to expect people to download zip files to get their hands on source code. Either just a gist or a dedicated repo if it's too complicated to paste into a comment directly.