`DSLContext` allows mixed use of `TransactionRunnable` and `ContextTransactionalRunnable`

facboy commented 1 year ago

Expected behavior

From discussion at https://github.com/jOOQ/jOOQ/discussions/14455

The transaction nesting breaks if (in the same call chain) you mix usage of TransactionRunnable and ContextTransactionRunnable. It would probably be best to detect mixed usage and throw an error instead.

Actual behavior

If you do something like the below, you will find that the transaction nesting levels will become incorrect and eg the top-level transaction in MyApplication might actually be committed when the nested call in MyUpdater completes.

class MyApplication {
  void run() {
    configuration.dsl().transactionResult(() -> MyUpdater.useTransaction(configuration.dsl()));
  }
}

class MyUpdater {
  static MyResult useTransaction(DSLContext ctx) {
    return ctx.transactionResult(txCtx -> { /* use txCtx */ });
  }
}

Steps to reproduce the problem

Something along the lines of the code above.

jOOQ Version

Jooq 3.17.7

Database product and version

PostgreSQL 10.6 on x86_64-pc-linux-gnu, compiled by gcc (GCC) 4.8.5 20150623 (Red Hat 4.8.5-28), 64-bit

Java Version

openjdk version "17.0.1" 2021-10-19

OS Version

No response

JDBC driver name and version (include name if unofficial driver)

No response

lukaseder commented 1 year ago

Thanks for the report. I'll try to reproduce this.

lukaseder commented 1 year ago

I can confirm, what looks like a nested transaction (with a savepoint) is indeed just a top level transaction, so everything is committed. Here's a test that currently fails:

try {
    create.transaction(() -> {
        create.insertInto(TAuthor())
              .columns(TAuthor_ID(), TAuthor_LAST_NAME())
              .values(3, "A")
              .execute();

        assertEquals(3, create.fetchCount(TAuthor()));
        create.transaction(ctx -> {
            ctx.dsl()
               .insertInto(TAuthor())
               .columns(TAuthor_ID(), TAuthor_LAST_NAME())
               .values(4, "B")
               .execute();

            assertEquals(4, ctx.dsl().fetchCount(TAuthor()));
        });

        throw new MyRuntimeException("test");
    });
}
catch (MyRuntimeException e) {}

assertEquals(2, create.fetchCount(TAuthor()));

There are 2 ways to fix this:

Reject the combination of transaction modes, as you suggest
Make it work correctly, allowing to nest a TransactionRunnable in a ContextTransactionRunnable

I'm not sure if this should be fixed without further thought, though. Certainly a lot of applications out there are now relying on the current behaviour, even if it is wrong. A fix would produce a ton of subtle problems.

I'll think about this, and perhaps, wait for community feedback. You can probably implement your validation logic in a TransactionListener as a workaround?

facboy commented 1 year ago

thanks for the response. in my case i have a few places i can block the thread-local usage if i need to, so it's not an immediate problem.

jOOQ / jOOQ