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(mssql): add MS SQL Server driver #1375

Merged
merged 72 commits into from Apr 5, 2024
Merged

feat(mssql): add MS SQL Server driver #1375

merged 72 commits into from Apr 5, 2024

Conversation

B4nan
Copy link
Member

@B4nan B4nan commented Feb 1, 2021

Adds a new @mikro-orm/mssql package.

Closes #771

@UTGuy
Copy link
Contributor

UTGuy commented Jul 15, 2021

@B4nan Would love to see more work done on this

@B4nan
Copy link
Member Author

B4nan commented Jul 19, 2021

@B4nan Would love to see more work done on this

Well, then do the work on this :] For me this have very low priority. I did gave it a try and rebased to latest v5 but it did not really help, there are more blockers than just the schema generator, and for me there are too much unknowns as I never used mssql.

@lgtm-com
Copy link

lgtm-com bot commented Apr 5, 2022

This pull request introduces 1 alert when merging d749e70 into bb467f7 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@lgtm-com
Copy link

lgtm-com bot commented Apr 5, 2022

This pull request introduces 1 alert when merging 23b2dd8 into bb467f7 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@UTGuy
Copy link
Contributor

UTGuy commented Apr 17, 2022

You asked... I've delivered.
#3023

@lgtm-com
Copy link

lgtm-com bot commented May 19, 2022

This pull request introduces 1 alert when merging 6b58377 into 71b77af - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@lgtm-com
Copy link

lgtm-com bot commented May 19, 2022

This pull request introduces 1 alert when merging 179ae6f into 71b77af - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@B4nan B4nan force-pushed the mssql branch 4 times, most recently from 1fbe609 to 445dcc2 Compare May 19, 2022 23:19
@lgtm-com
Copy link

lgtm-com bot commented May 19, 2022

This pull request introduces 1 alert when merging 445dcc2 into 71b77af - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@B4nan B4nan force-pushed the mssql branch 2 times, most recently from 7ca6218 to 5cb6cea Compare May 20, 2022 07:32
@lgtm-com
Copy link

lgtm-com bot commented May 20, 2022

This pull request introduces 2 alerts when merging 5cb6cea into 60ec052 - view on LGTM.com

new alerts:

  • 2 for Unused variable, import, function or class

@lgtm-com
Copy link

lgtm-com bot commented May 20, 2022

This pull request introduces 1 alert when merging b7bb78e into 60ec052 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@codecov-commenter
Copy link

codecov-commenter commented May 21, 2022

Codecov Report

Attention: Patch coverage is 99.83137% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 99.71%. Comparing base (c0edb1d) to head (e55065d).
Report is 1 commits behind head on master.

Files Patch % Lines
...ages/knex/src/dialects/mssql/MsSqlTableCompiler.ts 95.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1375      +/-   ##
==========================================
+ Coverage   99.70%   99.71%   +0.01%     
==========================================
  Files         238      252      +14     
  Lines       17248    17754     +506     
  Branches     4224     4087     -137     
==========================================
+ Hits        17197    17704     +507     
+ Misses         51       49       -2     
- Partials        0        1       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@B4nan
Copy link
Member Author

B4nan commented May 21, 2022

The clearDatabase calls are taking a lot of time (>500ms). Its pretty much all from the truncate.sql script. Will try the other suggestions from https://stackoverflow.com/questions/253849/cannot-truncate-table-because-it-is-being-referenced-by-a-foreign-key-constraint

edit: ok, much better now, tests went from 30-40s down to ~5s

@B4nan
Copy link
Member Author

B4nan commented May 21, 2022

All the mapped types you added seems to be not needed, I guess that was for some initial implementation, right? Will remove them, all the tests are passing just fine without them. If they are needed, we need a test case that shows why we need them.

Will try to get the schema generator working, for now with disabled FKs, given we can't even define all of them in the mssql-schema.sql file.

edit: looking into datetime, is there any reason not to use datetime2 by default? I will probably do that, as I want to have the length option supported as well as to use milliseconds for date version columns.

@UTGuy
Copy link
Contributor

UTGuy commented May 22, 2022

@B4nan

All the mapped types you added seems to be not needed, I guess that was for some initial implementation, right?

They are absolutely needed!

Unicode

MSSql uses N-types (Unicode types) which is different from regular strings (Non-unicode types). If you have no ability to apply Unicode types then languages like Chinese will fail. This is incredibly important.

TimeStamp

If the Sql database is not set to UTC then all timestamped Date's will not be correctly stamped to UTC. The TimeStamp type overrides this. MSSql has multiple "Datetime" types it can use... one doesn't append the 'Z' for example and another does. This is important if your coming from a DB that is older and not generated from mikro-orm.

@B4nan
Copy link
Member Author

B4nan commented May 22, 2022

They are absolutely needed!

I guess you are missing my point. Those types should be applied to all strings/dates, right? We dont need them if things work out of box the same way for all strings/dates.

If you think they are still needed (with my latest changes), please provide test case that proves it. I already removed them, everything is passing just fine. I dont want to bend internals via custom types - that is an extension for users, internally we can find more performant ways to do things.

@UTGuy
Copy link
Contributor

UTGuy commented May 22, 2022

Yes, I will add some tests that will cause the default assumption of Unicode to fail.
You either need to have a Unicode type or a NonUnicode type as MSSQL commonly uses both.
Unicode types use a different syntax as well

N'value'

You can't just set the database type to NVarchar and insert data into it with 'value', you have to use N'value'

I will create a test that will have an NVarchar property and insert Chinese characters using '<chinese_values>' (w/o the N) and you can see they will turn into ?????????

…ches

When you run a batch of queries via `schema.execute()`, they are now grouped based on blank lines - a blank line between groups of statements means they will be executed separately. This is important for some queries that cannot be ran in a single batch, like when you create a database and switch to it.
Previously, we only cared about the column type, now we also check if the column is/was a FK or not.
B4nan added a commit that referenced this pull request Apr 3, 2024
B4nan added a commit that referenced this pull request Apr 3, 2024
@B4nan B4nan marked this pull request as ready for review April 3, 2024 16:38
@B4nan B4nan changed the title feat(mssql): add MS SQL Server driver [WIP] feat(mssql): add MS SQL Server driver Apr 3, 2024
@B4nan B4nan merged commit eeaad45 into master Apr 5, 2024
11 checks passed
@B4nan B4nan deleted the mssql branch April 5, 2024 22:33
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.

Mssql support
4 participants