feat: MSSQL connector - #197
Open
vkuttyp wants to merge 25 commits into
Open
Conversation
vkuttyp
marked this pull request as draft
November 28, 2025 14:45
vkuttyp
marked this pull request as ready for review
November 28, 2025 14:46
Copilot AI
added a commit
to vkuttyp/db0
that referenced
this pull request
Feb 1, 2026
Resolved conflicts between PR unjs#197 (MSSQL connector) and upstream main: - Updated docker-compose.yaml to use port mappings for all services (pg, mysql, mssql) - Merged formatting changes in src/types.ts - Merged package.json dependency updates - Regenerated pnpm-lock.yaml to resolve merge conflicts - Applied tsconfig.json updates from upstream - Included all MSSQL connector files and tests Build and lint verified successfully. Co-authored-by: vkuttyp <146238+vkuttyp@users.noreply.github.com>
📝 WalkthroughWalkthroughThis PR adds MSSQL connector support through the ChangesMSSQL Connector Support
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant App
participant MSSQLConnector
participant Tedious
participant MSSQLServer
App->>MSSQLConnector: Execute SQL with parameters
MSSQLConnector->>MSSQLConnector: Rewrite ? placeholders and infer types
MSSQLConnector->>Tedious: Create Request and bind parameters
Tedious->>MSSQLServer: Execute request
MSSQLServer-->>Tedious: Return rows or error
Tedious-->>MSSQLConnector: requestCompleted or error
MSSQLConnector-->>App: Return results or enriched error
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Biome (2.5.3)package.jsonFile contains syntax errors that prevent linting: Line 74: expected Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
src/connectors/mssql.ts (1)
1-7: Remove duplicate import.
Connectionis imported on line 2 and again aliased on line 4 asTediousConnection. Only one import is needed.♻️ Remove duplicate import
import { Connection, Request, - Connection as TediousConnection, type ConnectionConfiguration, TYPES, } from "tedious"; + +type TediousConnection = Connection;Or simply use
Connectionthroughout the code instead ofTediousConnection.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/connectors/mssql.ts` around lines 1 - 7, The import list in src/connectors/mssql.ts duplicates the same symbol—you're importing Connection twice (once as Connection and again aliased as TediousConnection); remove the redundant alias and use a single import (either keep Connection and replace all TediousConnection usages with Connection, or keep the alias and remove the plain Connection) so only one import of the tedious Connection remains and update any references to the chosen identifier (Connection or TediousConnection) accordingly.test/connectors/mssql.test.ts (3)
340-354: Commented-out cleanup code may leave test artifacts.The
afterAllblock has the database cleanup code commented out (lines 342-352). This could leave theTestDB_CreateTestdatabase on the server between test runs.♻️ Either uncomment the cleanup or add a comment explaining why it's disabled
afterAll(async () => { - // Clean up: drop the test database if it exists - // try { - // await db.exec(` - // IF EXISTS (SELECT * FROM sys.databases WHERE name = '${testDbName}') - // BEGIN - // ALTER DATABASE [${testDbName}] SET SINGLE_USER WITH ROLLBACK IMMEDIATE; - // DROP DATABASE [${testDbName}]; - // END - // `); - // } catch (error) { - // // Ignore errors if database doesn't exist - // } + // Clean up: drop the test database if it exists + try { + await db.exec(` + IF EXISTS (SELECT * FROM sys.databases WHERE name = '${testDbName}') + BEGIN + ALTER DATABASE [${testDbName}] SET SINGLE_USER WITH ROLLBACK IMMEDIATE; + DROP DATABASE [${testDbName}]; + END + `); + } catch { + // Ignore errors if database doesn't exist + } await db.dispose(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/connectors/mssql.test.ts` around lines 340 - 354, The afterAll cleanup currently has the db.exec block commented out, which risks leaving the TestDB_CreateTest database behind; either uncomment and restore the cleanup SQL that checks existence and drops testDbName using db.exec (ensuring ALTER DATABASE ... SET SINGLE_USER WITH ROLLBACK IMMEDIATE before DROP) and then call db.dispose(), or add a clear inline comment in the afterAll next to the db.exec block explaining why the cleanup is intentionally disabled (referencing afterAll, db.exec, testDbName, and db.dispose) so future maintainers know this is deliberate.
395-407: Skipped test should explain why it's disabled.
it.skip("should drop an existing database", ...)doesn't have an explanation for why it's skipped. Consider adding a comment or TODO explaining the reason (e.g., connection issues, race condition with other tests, etc.).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/connectors/mssql.test.ts` around lines 395 - 407, The skipped test it.skip("should drop an existing database", ...) lacks an explanation for why it's disabled; update that test by either replacing it with test.todo("should drop an existing database", "reason...") or leaving it skipped but adding a short comment/TODO directly above the it.skip describing the cause (e.g., connection flakiness, race condition, required environment, or reference to an issue/JIRA id) so future readers know when and how to re-enable the test; reference the test name ("should drop an existing database") when making the change.
543-551: Consider usingexpect(...).rejects.toThrow()pattern instead of try/catch withexpect.fail().The vitest/jest idiomatic way to test for thrown errors is using the
rejectsmatcher. Additionally,expect.fail()may not be available in all vitest configurations.♻️ Suggested refactor
- it("should provide error context with SQL and parameters", async () => { - try { - const stmt = db.prepare("SELECT * FROM invalid_table WHERE id = ?"); - await stmt.all(123); - expect.fail("Should have thrown an error"); - } catch (error: any) { - expect(error.sql).toBeDefined(); - expect(error.parameters).toBeDefined(); - } - }); + it("should provide error context with SQL and parameters", async () => { + const stmt = db.prepare("SELECT * FROM invalid_table WHERE id = ?"); + await expect(stmt.all(123)).rejects.toMatchObject({ + sql: expect.any(String), + parameters: expect.any(Array), + }); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/connectors/mssql.test.ts` around lines 543 - 551, Replace the try/catch pattern with the vitest async matcher: call db.prepare(...) to get stmt and then assert the promise rejection with await expect(stmt.all(123)).rejects.toMatchObject({ sql: expect.anything(), parameters: expect.anything() }) (or use rejects.toHaveProperty for each field) instead of using expect.fail(); this keeps the test idiomatic and lets you assert the error shape from the stmt.all call directly.docker-compose.yaml (1)
19-26: Consider using port mapping instead ofnetwork_mode: "host"for cross-platform compatibility.The
pgandmysqlservices use explicit port mappings, butmssqlusesnetwork_mode: "host". Host networking doesn't work the same way on macOS and Windows (Docker Desktop), which may cause connectivity issues for contributors on those platforms.♻️ Suggested change for consistency with other services
mssql: # https://hub.docker.com/_/microsoft-mssql-server image: mcr.microsoft.com/mssql/server:2022-latest - network_mode: "host" + ports: ["1433:1433"] environment: ACCEPT_EULA: "Y" MSSQL_SA_PASSWORD: "MyStrong!Passw0rd" MSSQL_PID: "Developer"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker-compose.yaml` around lines 19 - 26, The mssql service currently uses network_mode: "host" which breaks Docker Desktop on macOS/Windows; replace network_mode in the mssql service with explicit port mapping (add a ports section mapping host 1433 to container 1433, e.g., "1433:1433") and remove network_mode to match how pg and mysql are configured, ensuring the MSSQL_SA_PASSWORD and other environment variables remain unchanged so local containers remain reachable cross-platform.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.env.example:
- Around line 14-18: The MSSQL env block variables (MSSQL_HOST, MSSQL_DB_NAME,
MSSQL_PORT, MSSQL_USERNAME, MSSQL_PASSWORD) are out-of-order and the file lacks
a trailing blank line; reorder the block to satisfy the linter (e.g.,
alphabetically: MSSQL_DB_NAME, MSSQL_HOST, MSSQL_PASSWORD, MSSQL_PORT,
MSSQL_USERNAME or follow the project's env ordering convention) and add a single
trailing newline at EOF so the .env.example passes dotenv-linter checks.
In `@src/connectors/mssql.ts`:
- Around line 84-92: The code currently calls connection.close() inside
request.on("requestCompleted") and request.on("error"), which breaks connection
reuse via the cached _client returned by getClient(); remove the
connection.close() calls from both request handlers so requests resolve/reject
without closing the shared connection, keep the resolve({ rows, success }) and
reject(error) behavior, and add a dispose() method on the MSSQL connector class
that closes the underlying _client connection and clears _client when the
database instance is torn down so connections are cleaned up properly.
- Around line 23-31: The connect callback in the Promise created for new
Connection(opts) calls reject(error) but continues execution and still assigns
_client = client; update the client.connect callback (the anonymous function
passed to client.connect) to stop execution after a failure—either by adding a
return immediately after reject(error) or by using an if/else so _client =
client only runs when there is no error—so the cached _client is only set on
successful connections.
- Around line 112-155: Add a dispose() implementation on the returned Connector
object to close the underlying Tedious connection: call the client
shutdown/close method obtained via getClient() (or the instance returned by
getInstance()) and ensure it returns a Promise<void>; reference the connector
returned in the function, the getClient()/getInstance() helpers, and the
TediousConnection type so dispose() invokes the proper close/disconnect on that
connection and handles any errors asynchronously.
---
Nitpick comments:
In `@docker-compose.yaml`:
- Around line 19-26: The mssql service currently uses network_mode: "host" which
breaks Docker Desktop on macOS/Windows; replace network_mode in the mssql
service with explicit port mapping (add a ports section mapping host 1433 to
container 1433, e.g., "1433:1433") and remove network_mode to match how pg and
mysql are configured, ensuring the MSSQL_SA_PASSWORD and other environment
variables remain unchanged so local containers remain reachable cross-platform.
In `@src/connectors/mssql.ts`:
- Around line 1-7: The import list in src/connectors/mssql.ts duplicates the
same symbol—you're importing Connection twice (once as Connection and again
aliased as TediousConnection); remove the redundant alias and use a single
import (either keep Connection and replace all TediousConnection usages with
Connection, or keep the alias and remove the plain Connection) so only one
import of the tedious Connection remains and update any references to the chosen
identifier (Connection or TediousConnection) accordingly.
In `@test/connectors/mssql.test.ts`:
- Around line 340-354: The afterAll cleanup currently has the db.exec block
commented out, which risks leaving the TestDB_CreateTest database behind; either
uncomment and restore the cleanup SQL that checks existence and drops testDbName
using db.exec (ensuring ALTER DATABASE ... SET SINGLE_USER WITH ROLLBACK
IMMEDIATE before DROP) and then call db.dispose(), or add a clear inline comment
in the afterAll next to the db.exec block explaining why the cleanup is
intentionally disabled (referencing afterAll, db.exec, testDbName, and
db.dispose) so future maintainers know this is deliberate.
- Around line 395-407: The skipped test it.skip("should drop an existing
database", ...) lacks an explanation for why it's disabled; update that test by
either replacing it with test.todo("should drop an existing database",
"reason...") or leaving it skipped but adding a short comment/TODO directly
above the it.skip describing the cause (e.g., connection flakiness, race
condition, required environment, or reference to an issue/JIRA id) so future
readers know when and how to re-enable the test; reference the test name
("should drop an existing database") when making the change.
- Around line 543-551: Replace the try/catch pattern with the vitest async
matcher: call db.prepare(...) to get stmt and then assert the promise rejection
with await expect(stmt.all(123)).rejects.toMatchObject({ sql: expect.anything(),
parameters: expect.anything() }) (or use rejects.toHaveProperty for each field)
instead of using expect.fail(); this keeps the test idiomatic and lets you
assert the error shape from the stmt.all call directly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9c326f28-39c7-436f-9922-4d9f9572fc9b
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (13)
.env.example.github/workflows/ci.ymldocker-compose.yamldocs/2.connectors/1.index.mddocs/2.connectors/mssql.mdpackage.jsonscripts/gen-connectors.tssrc/_connectors.tssrc/connectors/mssql.tssrc/types.tstest/connectors/_tests.tstest/connectors/mssql.test.tsvitest.config.ts
|
Would love to see this merged! |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/ci.yml (1)
9-9: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDisable persisted checkout credentials for this test job.
The workflow runs repository-controlled install, build, and test commands after checkout. Since checkout’s
persist-credentialsdefaults totrue, malicious code in those dependencies is exposed to the persistedGITHUB_TOKEN; set it tofalsebecause no later step requires authenticated Git operations.🔒 Proposed fix
- uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7 + with: + persist-credentials: false🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml at line 9, Update the actions/checkout step in the CI test job to set persist-credentials to false. Keep the existing checkout action reference and ensure no later step relies on persisted authenticated Git credentials.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@package.json`:
- Around line 70-76: Clean up the dependency declarations in package.json by
removing the duplicate typescript, vitest, and wrangler entries, retaining their
intended updated versions. Restore valid JSON syntax by ensuring the preceding
dependency entry is properly comma-terminated.
---
Outside diff comments:
In @.github/workflows/ci.yml:
- Line 9: Update the actions/checkout step in the CI test job to set
persist-credentials to false. Keep the existing checkout action reference and
ensure no later step relies on persisted authenticated Git credentials.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 02214d62-987c-4c22-ab1d-4dc4c97c2fbc
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
.github/workflows/ci.ymldocker-compose.yamldocs/2.connectors/1.index.mdpackage.jsonsrc/types.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- docs/2.connectors/1.index.md
- docker-compose.yaml
- src/types.ts
Comment on lines
+70
to
76
| "tedious": "^19.1.3", | ||
| "typescript": "^5.9.3", | ||
| "vitest": "^4.1.0", | ||
| "wrangler": "^4.74.0" | ||
| "typescript": "^7.0.2", | ||
| "vitest": "^4.1.10", | ||
| "wrangler": "^4.110.0" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Fix the duplicate dependency declarations and invalid JSON.
Lines 71-73 duplicate the declarations on lines 74-76, and line 73 is missing the comma required before the next property. This makes package.json unparsable and blocks installation, scripts, and CI. Retain one declaration for each package, using the intended updated versions.
Proposed fix
"scule": "^1.3.0",
"tedious": "^19.1.3",
- "typescript": "^5.9.3",
- "vitest": "^4.1.0",
- "wrangler": "^4.74.0"
"typescript": "^7.0.2",
"vitest": "^4.1.10",
"wrangler": "^4.110.0"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "tedious": "^19.1.3", | |
| "typescript": "^5.9.3", | |
| "vitest": "^4.1.0", | |
| "wrangler": "^4.74.0" | |
| "typescript": "^7.0.2", | |
| "vitest": "^4.1.10", | |
| "wrangler": "^4.110.0" | |
| "tedious": "^19.1.3", | |
| "typescript": "^7.0.2", | |
| "vitest": "^4.1.10", | |
| "wrangler": "^4.110.0" |
🧰 Tools
🪛 Biome (2.5.3)
[error] 74-74: expected , but instead found "typescript"
(parse)
[error] 71-71: The key typescript was already declared.
(lint/suspicious/noDuplicateObjectKeys)
[error] 72-72: The key vitest was already declared.
(lint/suspicious/noDuplicateObjectKeys)
[error] 73-73: The key wrangler was already declared.
(lint/suspicious/noDuplicateObjectKeys)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@package.json` around lines 70 - 76, Clean up the dependency declarations in
package.json by removing the duplicate typescript, vitest, and wrangler entries,
retaining their intended updated versions. Restore valid JSON syntax by ensuring
the preceding dependency entry is properly comma-terminated.
Source: Linters/SAST tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Based on this PR #121
Resolves #120
Summary by CodeRabbit
docker-composewith an MSSQL service for local development..env.examplewith MSSQL connection environment variables.