Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #187 +/- ##
=======================================
Coverage ? 55.92%
=======================================
Files ? 26
Lines ? 1105
Branches ? 139
=======================================
Hits ? 618
Misses ? 485
Partials ? 2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThis pull request adds complete MariaDB connector support to the library, including the connector implementation with lazy connection initialization and prepared statements, type definitions, npm dependencies, Docker configuration, tests, and documentation. Changes
Sequence DiagramsequenceDiagram
participant App as Application
participant Conn as MariaDB Connector
participant Cache as Connection Cache
participant MDB as MariaDB Database
App->>Conn: mariadbConnector(opts)
activate Conn
Conn->>Conn: initialize (lazy mode)
deactivate Conn
App->>Conn: query(sql, params)
activate Conn
Conn->>Cache: getConnection()
alt First use
Cache->>MDB: connect(opts)
MDB-->>Cache: connection
Cache-->>Conn: connection
else Already cached
Cache-->>Conn: connection
end
Conn->>MDB: execute(sql, params)
MDB-->>Conn: results
Conn-->>App: rows/affected
deactivate Conn
App->>Conn: dispose()
activate Conn
Conn->>MDB: end()
MDB-->>Conn: ok
Conn->>Cache: clear()
deactivate Conn
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
📝 Coding Plan
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 |
|
@pi0 I rebassed and polished the PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package.json`:
- Around line 96-98: Add a "mariadb" entry to the package.json peerDependencies
so the optional flag in peerDependenciesMeta applies; update the
peerDependencies object to declare the mariadb package (matching the versioning
style used for other DB connectors) ensuring peerDependencies and
peerDependenciesMeta both reference "mariadb" to mark it as an optional peer
dependency.
In `@src/connectors/mariadb.ts`:
- Around line 15-40: The bug is that getConnection awaits
mariadb.createConnection before assigning _connection, allowing concurrent
callers to create multiple connections; fix by memoizing the in-flight creation:
introduce an internal promise (e.g. _connectionPromise) and in getConnection
assign _connectionPromise = mariadb.createConnection(...) immediately, then
await it, set _connection from the resolved connection, and clear
_connectionPromise on dispose; update getInstance to await/return the resolved
connection (via getConnection/_connectionPromise) and change dispose to await
the promise (if any) then call .end() on the resolved _connection and clear both
_connection and _connectionPromise.
In `@test/connectors/mariadb.test.ts`:
- Around line 5-14: The test currently gates with
describe.runIf(process.env.MARIADB_URL) but then uses hardcoded
host/port/credentials in testConnector and connector, so CI/local DSNs are
ignored; update the test to derive the connection config from
process.env.MARIADB_URL instead of the literals: inside the test setup (the call
to testConnector and connector) parse process.env.MARIADB_URL (or pass it into
connector if it accepts a DSN) to populate host/port/user/password/database,
falling back to the existing literals only if the env var is absent; reference
the describe.runIf, testConnector and connector call sites when making the
change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 14282cdc-d319-4bf4-9a98-9dc0c24d8e93
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
docker-compose.yamldocs/2.connectors/mariadb.mdpackage.jsonsrc/_connectors.tssrc/connectors/mariadb.tssrc/types.tstest/connectors/_tests.tstest/connectors/mariadb.test.ts
| let _connection: mariadb.Connection | undefined; | ||
| const getConnection = async () => { | ||
| if (_connection) { | ||
| return _connection; | ||
| } | ||
|
|
||
| _connection = await mariadb.createConnection({ | ||
| ...opts, | ||
| }); | ||
|
|
||
| return _connection; | ||
| }; | ||
|
|
||
| const query: InternalQuery = (sql, params) => | ||
| getConnection().then((c) => c.query(sql, params)); | ||
|
|
||
| return { | ||
| name: "mariadb", | ||
| dialect: "mariadb", | ||
| getInstance: () => getConnection(), | ||
| exec: (sql) => query(sql), | ||
| prepare: (sql) => new StatementWrapper(sql, query), | ||
| dispose: async () => { | ||
| await _connection?.end?.(); | ||
| _connection = undefined; | ||
| }, |
There was a problem hiding this comment.
Memoize the first createConnection() call.
Line 21 assigns _connection only after the await, so two concurrent first callers can each open a separate MariaDB connection. One then falls out of the cache, getInstance() can observe different instances, and dispose() never closes the loser.
🔒 Proposed fix for connection initialization
export default function mariadbConnector(
opts: ConnectorOptions,
): Connector<mariadb.Connection> {
let _connection: mariadb.Connection | undefined;
+ let _connectionPromise: Promise<mariadb.Connection> | undefined;
+
const getConnection = async () => {
if (_connection) {
return _connection;
}
+ if (_connectionPromise) {
+ return _connectionPromise;
+ }
- _connection = await mariadb.createConnection({
- ...opts,
- });
+ _connectionPromise = mariadb.createConnection({ ...opts }).then((connection) => {
+ _connection = connection;
+ return connection;
+ }).catch((error) => {
+ _connectionPromise = undefined;
+ throw error;
+ });
- return _connection;
+ return _connectionPromise;
};
const query: InternalQuery = (sql, params) =>
getConnection().then((c) => c.query(sql, params));
@@
exec: (sql) => query(sql),
prepare: (sql) => new StatementWrapper(sql, query),
dispose: async () => {
- await _connection?.end?.();
+ const connection = _connection ?? (await _connectionPromise);
+ await connection?.end?.();
_connection = undefined;
+ _connectionPromise = undefined;
},
};
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/connectors/mariadb.ts` around lines 15 - 40, The bug is that
getConnection awaits mariadb.createConnection before assigning _connection,
allowing concurrent callers to create multiple connections; fix by memoizing the
in-flight creation: introduce an internal promise (e.g. _connectionPromise) and
in getConnection assign _connectionPromise = mariadb.createConnection(...)
immediately, then await it, set _connection from the resolved connection, and
clear _connectionPromise on dispose; update getInstance to await/return the
resolved connection (via getConnection/_connectionPromise) and change dispose to
await the promise (if any) then call .end() on the resolved _connection and
clear both _connection and _connectionPromise.
There was a problem hiding this comment.
Same lazy-init pattern used across all connectors here (mysql2, postgresql, etc). Fixing the race would be a separate change touching every connector, not scoped to this PR.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check lazy-init patterns in other connectors
rg '_connection' src/connectors/ -n -A2 -B2 --type=ts
Length of output: 3883
@oritwoen, confirmed — mysql2.ts and cloudflare-hyperdrive-mysql.ts use the exact same unguarded _connection pattern, so the new connector is consistent with the existing codebase. Keeping it scoped to a separate cross-cutting fix makes sense.
Would you like me to open a follow-up GitHub issue to track fixing the promise-memoization race across all affected connectors (mysql2, mariadb, cloudflare-hyperdrive-mysql, etc.)?
🐇
✏️ Learnings added
Learnt from: oritwoen
Repo: unjs/db0 PR: 187
File: src/connectors/mariadb.ts:15-40
Timestamp: 2026-03-16T20:03:01.114Z
Learning: In the db0 repository (unjs/db0), all connection-based connectors (mysql2, mariadb, cloudflare-hyperdrive-mysql) intentionally share the same lazy-init `_connection` pattern without promise memoization. The concurrent-call race condition is a known cross-cutting concern to be addressed in a separate PR touching all connectors, not within individual connector PRs.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| describe.runIf(process.env.MARIADB_URL)("connectors: mariadb.test", () => { | ||
| testConnector({ | ||
| dialect: "mariadb", | ||
| connector: connector({ | ||
| host: "localhost", | ||
| port: 3307, | ||
| user: "test", | ||
| password: "test", | ||
| database: "db0", | ||
| }), |
There was a problem hiding this comment.
Use MARIADB_URL for the actual connection config.
Line 5 gates the suite on MARIADB_URL, but Lines 8-13 ignore its value and always connect to localhost:3307 with fixed credentials. Any non-default CI or local DSN will still enable the suite and then hit the wrong server.
🔧 Proposed test setup
import { describe } from "vitest";
import connector from "../../src/connectors/mariadb";
import { testConnector } from "./_tests";
-describe.runIf(process.env.MARIADB_URL)("connectors: mariadb.test", () => {
+const mariadbUrl = process.env.MARIADB_URL;
+
+describe.runIf(Boolean(mariadbUrl))("connectors: mariadb.test", () => {
+ const url = new URL(mariadbUrl!);
+
testConnector({
dialect: "mariadb",
connector: connector({
- host: "localhost",
- port: 3307,
- user: "test",
- password: "test",
- database: "db0",
+ host: url.hostname,
+ port: Number(url.port || 3306),
+ user: decodeURIComponent(url.username),
+ password: decodeURIComponent(url.password),
+ database: url.pathname.replace(/^\//, ""),
}),
});
});📝 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.
| describe.runIf(process.env.MARIADB_URL)("connectors: mariadb.test", () => { | |
| testConnector({ | |
| dialect: "mariadb", | |
| connector: connector({ | |
| host: "localhost", | |
| port: 3307, | |
| user: "test", | |
| password: "test", | |
| database: "db0", | |
| }), | |
| import { describe } from "vitest"; | |
| import connector from "../../src/connectors/mariadb"; | |
| import { testConnector } from "./_tests"; | |
| const mariadbUrl = process.env.MARIADB_URL; | |
| describe.runIf(Boolean(mariadbUrl))("connectors: mariadb.test", () => { | |
| const url = new URL(mariadbUrl!); | |
| testConnector({ | |
| dialect: "mariadb", | |
| connector: connector({ | |
| host: url.hostname, | |
| port: Number(url.port || 3306), | |
| user: decodeURIComponent(url.username), | |
| password: decodeURIComponent(url.password), | |
| database: url.pathname.replace(/^\//, ""), | |
| }), | |
| }); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/connectors/mariadb.test.ts` around lines 5 - 14, The test currently
gates with describe.runIf(process.env.MARIADB_URL) but then uses hardcoded
host/port/credentials in testConnector and connector, so CI/local DSNs are
ignored; update the test to derive the connection config from
process.env.MARIADB_URL instead of the literals: inside the test setup (the call
to testConnector and connector) parse process.env.MARIADB_URL (or pass it into
connector if it accepts a DSN) to populate host/port/user/password/database,
falling back to the existing literals only if the env var is absent; reference
the describe.runIf, testConnector and connector call sites when making the
change.
There was a problem hiding this comment.
Matches the existing pattern - mysql2.test.ts does the same thing. The env var is just a gate, not the actual DSN.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check mysql2.test.ts to verify the existing pattern
cat test/connectors/mysql2.test.ts 2>/dev/null || fd mysql2.test -e ts --exec cat {}
Length of output: 380
@oritwoen, you're right — mysql2.test.ts uses exactly the same pattern. My suggestion was incorrect given the established convention in this codebase. The env var is intentionally just a gate to skip the suite when the DB isn't available, not a DSN to configure the connection. I'll note this for future reviews.
🧠 Learnings used
Learnt from: oritwoen
Repo: unjs/db0 PR: 187
File: src/connectors/mariadb.ts:15-40
Timestamp: 2026-03-16T20:03:01.114Z
Learning: In the db0 repository (unjs/db0), all connection-based connectors (mysql2, mariadb, cloudflare-hyperdrive-mysql) intentionally share the same lazy-init `_connection` pattern without promise memoization. The concurrent-call race condition is a known cross-cutting concern to be addressed in a separate PR touching all connectors, not within individual connector PRs.
1 participant
Resolves #186
Summary by CodeRabbit
New Features
Documentation
Tests