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

add MS SQL Server dialect. #595

Merged
merged 80 commits into from
Sep 9, 2023

Conversation

igalklebanov
Copy link
Member

@igalklebanov igalklebanov commented Jul 17, 2023

might close #38.

This PR adds a core dialect for MS SQL Server. It uses the tedious library as the underlying driver, and the tarn library for connection pool management. All external code is provided through the dialect's config upon instantiation.
It also introduces MS SQL Server to our testing suites.
It also makes necessary small refactors in our testing suites, being more explicit about what's being tested, now that we have more than 3 core dialects.


Gaps in our querying or schema APIs relevant to mssql that should be addressed in future PRs:

  1. implement the output clause so insert id can be retrieved. add OUTPUT clause support. #828
  2. implement merge queries to support on conflict like queries. add MERGE query support. #700
  3. implement identity method @ column builders. add IDENTITY column support. #823
  4. add bit, uniqueidentifier, nvarchar data types.
  5. add persisted @ column builders.
  6. ability to create things if they don't exist - if possible.
  7. docs site - getting started, other places that mention dialects. Mssql docs #724
  8. computed columns.
  9. or alter view.
  10. from at create type.
  11. support setting data type and more things (e.g. not null) in the same alter column clause.
  12. json_value(ref, <json_path>). add eb.jsonPath<$>. #791
  13. json helpers, if possible. add mssql json helpers. #818
  14. top N support. add TOP clause support. #821
  15. limit offset support. add FETCH clause support. #822

@igalklebanov igalklebanov added blocked Blocked waiting for another feature built-in dialect Related to a built-in dialect enhancement New feature or request tests labels Jul 17, 2023
@vercel
Copy link

vercel bot commented Jul 17, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
kysely ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 9, 2023 0:11am

@igalklebanov igalklebanov mentioned this pull request Aug 13, 2023
@sofanati-nour
Copy link

Igal doing the lord's work right here, thank you so much.

@MartinEide
Copy link

MartinEide commented Aug 16, 2023

Often these sql servers are hosted in azure. Put any thought into authentication using azure active directory?
Or is the connection with mssql purely handled by mssql?

Maybe a dumb question.
https://learn.microsoft.com/en-us/azure/azure-sql/database/azure-sql-javascript-mssql-quickstart

import * as dotenv from 'dotenv';
import sql from 'mssql';
dotenv.config({ path: `.env.${process.env.NODE_ENV}`, debug: true });

const server = process.env.AZURE_SQL_SERVER;
const database = process.env.AZURE_SQL_DATABASE;
const port = parseInt(process.env.AZURE_SQL_PORT);
const type = process.env.AZURE_SQL_AUTHENTICATIONTYPE;


export const config = {
    server,
    port,
    database,
    authentication: {
        type
    },
    options: {
        encrypt: true
    }
};

sql.connect(config);

Update:
Looked further into the PR. Understand that you used tedious:
https://tediousjs.github.io/tedious/api-connection.html#event_connect

And in the docs of tedious there are different authentication types. Including azure-active-directory-default.
So i'd guess you just configure your tedious configuration into the mssqlDialect?

@igalklebanov
Copy link
Member Author

@MartinEide we're unopinionated by design. always refer to the underlying driver's (tedious in this case) docs for connection options.

I have no idea, never used ms sql server in production.

@dylel
Copy link

dylel commented Aug 24, 2023

It appears that limit(1) isn't converted correctly, currently it adds it to the end of the select statement, however in sql server its called TOP instead of limit.

SQL SERVER: SELECT TOP(1) * FROM dbo.MyTable
MySQL: SELECT * FROM dbo.MyTable LIMIT 1

I don't think it can be used anywhere else apart from directly after the select keyword and might need to be hoisted there somehow. Maybe it would be better to add a completely different method like .top(1) but I realise that would stray from the current api. It's likely that offset will cause a similar issue as I think mysql can specify offset, limit in one line where as sql server separates them completely but I'm not that familiar with mysql.

Is there another way to call limit in a query, raw sql? i'm currently using this in a nested sub query and would be happy to not have to add a method or touch the current api to get this to work if there was some other way.

I've been putting this through its paces and seems to be pretty solid overall as this was the only thing I found so far, its my first time really diving into kysely but I'll keep reporting in as I explore the feasibility of bringing my c# codebase to typescript.

TLDR: looks great so far

@igalklebanov
Copy link
Member Author

igalklebanov commented Aug 29, 2023

@dylel This PR only handles the mssql dialect and introducing mssql to our existing test suites (which is a lot). Anything related to querying APIs will or will not be dealt with in other PRs if and when this PR is merged.

Check the tests to see how I managed to pull things off, usually using modifyX methods that allow raw SQL before/after certain keywords. These are temporary workarounds.

@igalklebanov igalklebanov changed the title exploring mssql support. MS SQL Server dialect. Aug 29, 2023
@igalklebanov igalklebanov changed the title MS SQL Server dialect. add MS SQL Server dialect. Aug 29, 2023
@vicary
Copy link

vicary commented Aug 31, 2023

@dylel Do you think it's a good idea to normalize it into OFFSET X ROWS FETCH NEXT Y ROWS ONLY instead of TOP X?

@dylel
Copy link

dylel commented Sep 2, 2023

@dylel Do you think it's a good idea to normalize it into OFFSET X ROWS FETCH NEXT Y ROWS ONLY instead of TOP X?

I think the verbose solution would be better, and have them be completely separate methods. I don't like the idea of introducing 'TOP X' and would be in favor of just polyfiling .limit() to have a case for mssql so that users can just use the same api and not have to carry the mental model of "i'm using sql server therefore I have to call a different method" but i'm sure @igalklebanov has some thoughts for future PR's. I've been successfully using the modifyStart/End functions for the time being and they work well for now

@vicary
Copy link

vicary commented Sep 2, 2023

I don't think TOP X is a good idea, because .offset() never works. Unless we want to conditionally appends the SQL, depends on whether user has called .offset(). This sounds not quite consistent with the current simple approach.

If we are going the offset ... fetch route, we may start with visitSelectQuery, visiLimit and visiOffset in mssql-query-compiler.ts. Here is a patch file.

It leaves one question, order by and offset is always required for this clause. Are users responsible for calling .orderBy() and .offset() themselves?

@dylel @igalklebanov wdyt?

Copy link
Member

@koskimas koskimas left a comment

Choose a reason for hiding this comment

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

Finally got to reviewing this giant! Amazing work! I have nothing to nitpick here. Let's merge this and improve as we go 💯

src/util/object-utils.ts Show resolved Hide resolved
@igalklebanov igalklebanov merged commit a5a36c0 into kysely-org:master Sep 9, 2023
@igalklebanov igalklebanov deleted the mssql-dialect branch September 9, 2023 15:07
@igalklebanov igalklebanov added the mssql Related to MS SQL Server (MSSQL) label Sep 9, 2023
@movahhedi
Copy link
Contributor

When will this be released? Need it soo much

@movahhedi
Copy link
Contributor

Guys? Any updates?

Gaspero pushed a commit to Gaspero/kysely that referenced this pull request Oct 2, 2023
* add mssql dev dependency.

* add mssql to docker-compose.yml.

* config? @ pg driver.

* initial dialect implementation.

* fix browser tests by getting tedious stuff in config.

* introduce mssql to test-setup.

* get dialect to a working state.

* introduce mssql to insert test suite.

* introduce mssql to select test suite.

* add mssql to test scripts.

* make tests pass type-checks.

* fix new tests after update branch.

* introduce mssql to update test suite.

* fix mysql test expectation.

* introduce mssql to delete test suite.

* introduce mssql to with test suite.

* introduce mssql to with schema test suite.

* introduce mssql to where test suite.

* test mssql vs. selectNoFrom.

* introduce mssql to set operation test suite.

* fix error rejections.

* introduce mssql to schema module test suite.

* remove mssql, add tedious and tarn.

* initial pool implementation using tarn.

* fix failing where tests.

* fix compilation errors at select tests.

* introduce mssql to schema module test suite, pt. 2.

* compile mssql alter table alter column properly.

* introduce mssql to schema module test suite, pt. 3.

* fix failing where test.

* introduce mssql to sanitize identifiers test suite.

* introduce mssql to raw sql test suite.

* introduce mssql to raw query test suite.

* introduce mssql to order by test suite.

* introduce mssql to join test suite.

* introduce mssql to having test suite.

* introduce mssql into group by test suite.

* empty and/or works everywhere.

* introduce mssql to expression test suite.

* introduce mssql to execute test suite.

* introduce mssql to deduplicate joins test suite.

* introduce mssql to coalesce test suite.

* introduce mssql to clear test suite.

* introduce mssql case test suite.

* introduce mssql to camel case test suite.

* introduce mssql to aggregate function test suite.

* lift only.

* connection validate shouldn't trigger kysely stuff.

* introduce mssql to transaction test suite, pt. 1.

* fix introspect tests.

* remove mssql types.

* add @types/tedious dev dep.

* introduce mssql to transaction test suite, pt. 2.

* introduce mssql to introspect test suite, pt. 1.

* introduce mssql to introspect test suite, pt. 2.

* fix default to column introspection @ mssql.

* autoincrementing columns are technically columns with default values.

* dont use ifNotExists in mssql migrations.

* introduce mssql to migration test suite.

* implement streaming.

* fix schema tests.

* rename DIALECTS_WITH_MSSQL back to DIALECTS.

* misc adjustments and js docs.

* fix wording of `supportsCreateIfNotExists`.

* add mssql to key value set variant test case.
@koskimas
Copy link
Member

This is now released with 0.27.0

@movahhedi
Copy link
Contributor

movahhedi commented Dec 30, 2023

Thanks for making our lives easier! 🎉🥳

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
built-in dialect Related to a built-in dialect enhancement New feature or request mssql Related to MS SQL Server (MSSQL) tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SQL Server support
8 participants