Open
Conversation
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 introduces MSSQL connector support using the Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant MSSQLConnector
participant Tedious as Tedious Driver
participant MSSQL as MSSQL Server
App->>MSSQLConnector: getInstance() / getClient()
MSSQLConnector->>Tedious: new Connection(config)
Tedious->>MSSQL: TCP Connect
MSSQL-->>Tedious: Connected
Tedious-->>MSSQLConnector: Connection Ready
App->>MSSQLConnector: exec(sql, parameters?)
MSSQLConnector->>MSSQLConnector: prepareSqlParameters(sql, params)
Note over MSSQLConnector: Replace ? with `@1`, `@2`, ...
MSSQLConnector->>Tedious: Request + addParameter()
Tedious->>MSSQL: Execute Query
MSSQL-->>Tedious: Row Events + requestCompleted
MSSQLConnector->>MSSQLConnector: Accumulate Rows
Tedious-->>MSSQLConnector: requestCompleted / error
MSSQLConnector-->>App: Rows or Error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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
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
1 participant
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
New Features
Documentation
Tests
Chores