PhilWaldmann / openrecord

Make ORMs great again!
https://openrecord.js.org
MIT License
486 stars 38 forks source link

Help with Transaction, please #80

Closed arthurfranca closed 5 years ago

arthurfranca commented 5 years ago

Hello, tried using transaction like:

await new Promise(resolve => store.startTransaction(resolve))
  .then(trx => Promise.all(
    [Model1.useTransaction(trx).create({ valid: 12 }),
    Model2.useTransaction(trx).create({ invalid: 'a validation error' })]
  ))

But the validation error on Model2 did not prevent Model1 record creation, so there was no rollback. Is transaction available or am i using it wrong?

PhilWaldmann commented 5 years ago
await store.startTransaction(trx => {
  return Promise.all([
    Model1.useTransaction(trx).create({ valid: 12 }),
    Model2.useTransaction(trx).create({ invalid: 'a validation error' })
  ])
})

should do the trick.

arthurfranca commented 5 years ago

Thx, it's better now. But it still seems to have a strange bug.

I've created a test and it fails

it.only('test transaction', async () => {
    expect(await Model1.totalCount()).to.be.equal(0)

    await (
      store
        .startTransaction(t =>
          Promise.all([
            Model1.useTransaction(t).create({ valid: 12 }),
            Model2.useTransaction(t).create({ invalid: 'a validation error' })
          ])
        ).catch(e => console.log(e) })
    )

    expect(await Model1.totalCount()).to.be.equal(0) // OK

    await (
      store
        .startTransaction(t =>
          Promise.all([
            Model1.useTransaction(t).create({ valid: 13 }),
            Model1.useTransaction(t).create({ valid: 14 }),
          ])
        ).catch(e => console.log(e) })
    )

    expect(await Model1.totalCount()).to.be.equal(2) // AssertionError: expected 3 to equal 2
  })

So, after a failing transaction (with successful rollback), if i save the Model1 again (using a transaction or not), openrecord/knex seems to remember and also commit the valid but previously uncommited operation from first transaction along with the new create operation.

PhilWaldmann commented 5 years ago

Hmm... that's strange! I'll test it later today!

PhilWaldmann commented 5 years ago

Hi @arthurfranca

I had a bug in my tests which was hiding a bug inside openrecord. Please install v2.8.1 which should fix the problem you are facing.

arthurfranca commented 5 years ago

Ive installed v2.8.1 and can confirm it's working now. Thx

arthurfranca commented 5 years ago

One more thing, it seems there's no useTransaction when record is already loaded.

I think it would be a nice addition being able to do something like this:

await store.startTransaction(tx => {
 return Promise.all([
    record.save(tx) // or record.save().useTransaction(tx) or record.useTransaction(tx).save()
    record.update({}, tx) // or record.update({}).useTransaction(tx) or record.useTransaction(tx).update({})
  ])
})
PhilWaldmann commented 5 years ago

save() takes an optional param options. So you could do the following:

await store.startTransaction(transaction => {
 return Promise.all([
    record.save({transaction})
  ])
})

It's not yet documented, but used internally. I'll add documentation and also enhance update() and create() to support the same options object.

PhilWaldmann commented 5 years ago

Documentation is online. Thanks for your support!