kysely-org / kysely

A type-safe typescript SQL query builder
https://kysely.dev
MIT License
10.47k stars 267 forks source link

add `ControlledTransaction`. #962

Closed igalklebanov closed 21 hours ago

igalklebanov commented 5 months ago

closes #257.

This PR adds some optional savepoint-related methods to drivers - need to make sure we don't introduce a breaking change here! A new ControlledTransaction that allows manual commit, rollback and savepoint-related commands.

const trx = await db.startTransaction().execute()

try {
  const moshe = await trx
    .insertInto('person')
    .values({ name: 'Moshe' })
    .returning('id')
    .executeTakeFirstOrThrow()

  const trxAfterFoo = await trx.savepoint('foo').execute()

  try {
    const bone = await trxAfterFoo
      .insertInto('toy')
      .values({ name: 'Bone', price: 1.99 })
      .returning('id')
      .executeTakeFirstOrThrow()

    await trxAfterFoo
      .insertInto('pet')
      .values({ 
        owner_id: moshe.id, 
        name: 'Lucky', 
        favorite_toy_id: bone.id 
      })
      .execute()
  } catch (error) {
    await trxAfterFoo.rollbackToSavepoint('foo').execute()
  }

  await trxAfterFoo.releaseSavepoint('foo').execute()

  await trx.insertInto('audit').values({ action: 'added Moshe' }).execute()

  await trx.commit().execute()
} catch (error) {
  await trx.rollback().execute()
}

Some design goals (that hopefully will be met once this is done) with ControlledTransaction:

vercel[bot] commented 5 months 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 Oct 5, 2024 4:53pm
seivan commented 5 months ago

I was wondering, since you already have a reference to the transaction trxAfterFoo and it has been named at creation with trx.savepoint('foo').execute() why not default to it

instead of

trxAfterFoo.rollbackToSavepoint('foo')

It should technically be enough with trxAfterFoo.rollbackToSavepoint() or am I missing something?

The reason I ask is because I'd like something simple like db.startTransaction() and db.endTransaction() to be in beforeEach and afterEach test, so each unit test runs with empty tables.

igalklebanov commented 4 months ago

I was wondering, since you already have a reference to the transaction trxAfterFoo and it has been named at creation with trx.savepoint('foo').execute() why not default to it

instead of

trxAfterFoo.rollbackToSavepoint('foo')

It should technically be enough with trxAfterFoo.rollbackToSavepoint() or am I missing something?

The reason I ask is because I'd like something simple like db.startTransaction() and db.endTransaction() to be in beforeEach and afterEach test, so each unit test runs with empty tables.

I'll investigate, but off the top:

  1. trxAfterFoo provides autocompletion and type-checking of the savepoint argument you pass to rollbackToSavepoint. What are we saving here? a few characters? what's so wrong about:
let trx: ControlledTransaction<DB, ['post_setup']>;

before(async () => {
  const initialTrx = await db.startTransaction().execute();
  await setupTest(initialTrx);
  trx = await trx.savepoint('post_setup').execute();
});

afterEach(async () => {
  await trx.rollbackToSavepoint('post_setup').execute();
});

after(async () => {
  await trx.rollback().execute();
});
  1. This would introduce the need for either runtime errors when the instance doesn't have an "immediate" savepoint to default to OR to subclass ControlledTransaction and move some savepoint methods to the child. Having to pass the argument and having its type bound to a list of savepoints allows us to keep things less complicated.

  2. It is less WYSIWYG.

  3. Tests are not the only use case for this, tho I'd love for all use cases to have good DX here.

  4. trxAfterFoo allows rolling back and releasing savepoints up to "foo" AND savepoints that were created before it.

  5. You'd still have to hold onto a new instance of the same transaction, but with foo in its state. We don't mutate existing instances as a design principle.

aq1018 commented 2 months ago

Just want to say this is a great feature, and I eagerly await for this to be released.