Adds onReserveConnection to Postgres and MySQL dialect configs

[dcousineau]

• Adds the onReserveConnection callback to the Postgres and MySql dialect drivers
• This method is called for every connection checkout, regardless if the connection has been seen before or not.
• Companion to onCreateConnection

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:

1. Pool is empty (no connections)
2. Request handler asks for a connection for OrgA.
3. A new connection, Con1, is created and returned
4. Kysely executes onCreateConnection which runs SET app.org_id = OrgA on Con1
5. Request handler completes, Con1 is returned to the Pool
6. Request handler asks for connection for OrgB
7. Con1 is reused and returned by the Pool (as it has not expired and closed)
8. Kysely does NOT execute onCreateConnection
9. Request handler fails to complete as sql queries are blocked by RLS policies (context is set to OrgA, but app attempts to make OrgB queries)

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.

[vercel]

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	Jul 14, 2024 9:40pm

[dcousineau]

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.

[igalklebanov]

Hey 👋

Thank you! 💪

LGTM. Does it make sense to add something similar to MssqlDialect?

[dcousineau]

@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...

[dcousineau]

@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)?

[igalklebanov]

let's go with separate PR, after this one gets merged.

[dcousineau]

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.