Use msnodesqlv8 driver with typeorm

xiaoweiliu945 commented 3 years ago

Issue Description

Tests break when using msnodesqlv8 driver

Expected Behavior

Expect all test pass

Actual Behavior

Steps to Reproduce

First issue is I have to replace this.mssql = PlatformTools.load("mssql") with this.mssql = PlatformTools.load("mssql/msnodesqlv8") in order to use msnodesqlv8 driver. Seesrc/driver/sqlserver/SqlServerDriver.ts

wonder if there is a fix for this so I don't need this workaround.

    protected loadDependencies(): void {
        try {
            this.mssql = PlatformTools.load("mssql/msnodesqlv8");

        } catch (e) { // todo: better error for browser env
            throw new DriverPackageNotInstalledError("SQL Server", "mssql");

I use msnodesqlv8 and I have the following tests failure:

test/functional/query-builder/count/query-builder-count.ts 
test/github-issues/134/issue-134.ts
test/github-issues/1716/issue-1716.ts
test/github-issues/2199/issue-2199.ts
test/github-issues/2518/issue-2518.ts
test/github-issues/4220/issue-4220.ts
test/other-issues/mssql-add-column-with-default-value/mssql-add-column-with-default-value.ts

My Environment

Dependency	Version
Operating System	MacOS 10.13.6
Node.js version	14.17.3
Typescript version	3.7.2
TypeORM version	0.2.34

Additional Context

Relevant Database Driver(s)

DB Type	Reproducible
`aurora-data-api`	no
`aurora-data-api-pg`	no
`better-sqlite3`	no
`cockroachdb`	no
`cordova`	no
`expo`	no
`mongodb`	no
`mysql`	no
`nativescript`	no
`oracle`	no
`postgres`	no
`react-native`	no
`sap`	no
`sqlite`	no
`sqlite-abstract`	no
`sqljs`	no
`sqlserver`	Yes

Are you willing to resolve this issue by submitting a Pull Request?

✖️ Yes, I have the time, and I know how to start.
✅Yes, I have the time, but I don't know how to start. I would need guidance.
✖️ No, I don’t have the time, but I can support (using donations) development.
✖️ No, I don’t have the time and I’m okay to wait for the community / maintainers to resolve this issue.

imnotjames commented 3 years ago

Indulge me in this because I truly don't know.

Why?

xiaoweiliu945 commented 3 years ago

@imnotjames wonder typeorm support msnodesqlv8 Because typeorm use mssql and mssql uses tediousjs as default driver, while claiming msnodesqlv8 is an alternative driver here: https://github.com/tediousjs/node-mssql

But when I want to use typeorm with msnodesqlv8, I have the above issues.

I basically refer to this issue: https://github.com/typeorm/typeorm/issues/5830, force typeorm to use msnodesqlv8 driver by using this.mssql = PlatformTools.load("mssql/msnodesqlv8")

Then I got 7 failed test cases, here I only ran mssql tests.

imnotjames commented 3 years ago

We don't officially support msnodesqlv8 as of now.

As far as I can tell this is similar to writing a new driver because of some differences in where the parameter types need to come from, etc.

imnotjames commented 3 years ago

If you're requesting a new driver I can update this issue as such. I don't see how else we'd handle this.

Feel free to include information on how the tests are failing, though.

xiaoweiliu945 commented 3 years ago

@imnotjames

oh I think this is more like a bug fix.

Because typeorm is using driver:mssql and mssql support both tedious and msnodesqlv8, so technically msnodesqlv8 should work with typeorm, and I also saw you provided example code here using msnodesqlv8: https://github.com/typeorm/typeorm/issues/5830#issuecomment-706973658.

I am thinking those failed 7 test scenarios are something need to be addressed or fixed. Not sure if I should call it a new feature, it might just be extra bug fix for driver:mssql

imnotjames commented 3 years ago

I did not provide any sample code there.

Still need more info on what the tests are doing that are failing.

I have a good feeling that they're not interoperable and there's issues that are not trivial to make work between the two underlying drivers even if the compatibility layer exists via the mssql package.

dsbert commented 2 years ago

One reason to use the other driver - it supports Trusted_Connection, where the default driver does not.

TBG-FR commented 2 years ago

Is someone still working on that ? Are there many people interested ? I had to drop TypeORM on a specific project because I needed that driver, but making the right changes to implement it could be interesting 👀

dsbert commented 2 years ago

Is someone still working on that ? Are there many people interested ? I had to drop TypeORM on a specific project because I needed that driver, but making the right changes to implement it could be interesting 👀

It's possible to make it work with a few minor changes in versions < 0.3.0. I think the latest release of TypeORM does include some of these changes and may support it out of the box. However, there are some issues with the driver itself. For example - https://github.com/tediousjs/node-mssql/issues/1385

typeorm / typeorm