Open dcousineau opened 1 month ago
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 | Jun 18, 2024 5:36pm |
Hey @igalklebanov I'm sure you're busy, wanted to see if there's anything you're waiting on from me regarding this PR or if it's still sitting in the queue. I didn't see any connection or dialect tests for me to add test-cases to.
Hey π
Thank you! πͺ
LGTM. Does it make sense to add something similar to MssqlDialect
?
@igalklebanov I'm a huge fan of consistency, when I made the change I added it to MySQL because I saw an onCreateConnection
there too. I could easily swing through MSSQL and get that one going too...
@igalklebanov looking at the MSSQL driver it doesn't currently use a WeakMap connection cache pattern that the MySQL and Postgres drivers use.
My gut says refactoring the MSSQL driver to use a connection cache and adding the onCreate...
and onReserve...
connection lifecycle functions is better off in a separate PR.
I could 'shoehorn' both without adding a cache and saving the cache behaviors for later, e.g.:
async acquireConnection(): Promise<DatabaseConnection> {
const connection = await this.#pool.acquire().promise
// Begin Added Code...
if (this.#config?.onCreateConnection) {
await this.#config.onCreateConnection(connection)
}
if (this.#config?.onReserveConnection) {
await this.#config.onReserveConnection(connection)
}
// End Added Code...
return connection;
}
Of course this would make onCreateConnection
functionally identical to onReserveConnection
until the WeakMap connection cache behaviors are added...
What would be your preference (do the whole refactor of adding connection cache, doing the temporary shim, or leaving alone for a separate PR)?
let's go with separate PR, after this one gets merged.
Sounds good to me. I started poking at it so I'll open a separate PR later. It's been over 10 years since I've dealt in mssql and even then that was back in the PHP5 / Doctrine 1.0 days.
onReserveConnection
callback to the Postgres and MySql dialect driversonCreateConnection
Problem
My projects use row-level security in Postgres to ensure multi-tenancy. As a result I have a need to ensure Postgres session variables are set on a given connection to ensure context is carried through. The pre-existing
onCreateConnection
callback for the Postgres dialect only executes on new connections, meaning if a connection doesn't expire and is re-used by the pool the callback will not fire and context setting will not happen.For example, assume we run
SET app.org_id = ${org_id}
for every connection:onCreateConnection
which runsSET app.org_id = OrgA
on Con1onCreateConnection
Adding an additional callback that runs every time a connection is pulled from Kysely mitigates the issue by ensuring the session variables can be set every time at the expense of possibly running it more often than is required.