Improve TxManager API: Pass transaction as parameter instead of global state

[firov]

Current YOJ TxManager API uses global state via Tx.Current.get() to access active transaction. This creates several issues:

• Implicit contract: Methods accept Runnable/Supplier but don't explicitly pass transaction object
• Hidden dependency: Code accesses tables through global state
• Poor discoverability: Not obvious from method signature that transaction is available
• Testing complexity: Global state makes unit testing harder

Current Implementation

// Usage - implicit transaction access
var result = txManager.readOnly(() -> {
    return dao.table(SomeTable.class).find(id); // Uses Tx.Current.get() internally
});

Proposed Solution

Add overloads accepting [Function<RepositoryTransaction, T>]

// In TxManager
default <T> T tx(Function<RepositoryTransaction, T> function) {
    return tx(() -> function.apply(Tx.Current.get().getRepositoryTransaction()));
}

default void tx(Consumer<RepositoryTransaction> consumer) {
    tx(tx -> {
        consumer.accept(tx);
        return null;
    });
}

default <T> T readOnly(Function<RepositoryTransaction, T> function) {
    return readOnly().run(() -> function.apply(Tx.Current.get().getRepositoryTransaction()));
}

Usage After Changes

var result = txManager.readOnly(tx -> {
    return tx.someTable().find(id);
});

Benefits

• Explicit contract: Clear from signature that transaction is provided
• No magic: No need to know about Tx.Current internals
• Better IDE support: Type inference and navigation work properly
• Testability: Can pass mock transaction for testing
• Backward compatible: Old methods remain, new ones are optional

Migration

Change is backward compatible. Old code continues working:

[nvamelichev]

@firov I've upgraded the solution to using lambdas taking BaseDb instead of RepositoryTransaction, which allows for nicer syntax: tx.run(db -> db.projects().insert(...)); instead of tx.run(repositoryTransaction -> repositoryTransaction.table(Project.class).insert(...));.

But the results are... very very mixed, and I'm now not convinced that this is a positive change 👎👎👎

• 🥉 The usage syntax is not especially nice to use, compared to just @Inject'ing your Db implementation (that is just BaseDb.current(Db.class)) or even using BaseDb.current(Db.class) directly. You get added readability if you needed to use Tx.Current.get(), but that's about all the improvement you get.
• 🚨🚨🚨 Function<Db, R> and Consumer<Db> are lambda signature-compatible, this leads to ambiguities!
  • https://stackoverflow.com/a/30586429 => We cannot have the same tx() name for both methods, so we have to have ScopedTxManager.call() and ScopedTxManager.run() and choose between them. This is VERY sub-optimal, porting existing code becomes SUPER annoying SUPER quickly. Even if you're not porting existing tx() calls to call()/run(), you'd get annoyed during refactorings, because you'll sometimes need to change call() to run() and vice versa, when you upgrade from a one-liner lambda to a block lambda, for example.
  • Even if we only had have one Function<Db,R>-taking method in ScopedManager, we'd still get generics hell with AssertJ's assertThat(). It turns out that results of Function<Db,R>-taking method (which returns generic R) are considered by javac to be compatible with almost every interface argument that assertThat() takes, making manual disambiguation necessary! UBER UBER annoying. See:
    • Assertions.assertThat ambiguous reference error when passing a bounded generic instance assertj/assertj#839
    • https://joel-costigliola.github.io/assertj/assertj-core.html#ambiguous-compilation-error
• ReadOnlyBuilder/ScanBuilder are not supported (yet). This is the easiest to fix if we choose to go this way. But I'd rather we didn't go this way at all...

You can look at the PR yourself:
#199
and see if migration to the new API is worth the effort. It IMHO isn't. But just adding @ExperimentalApi ScopedTxManager for those who'd like to use it - without migrating every TxManager usage to it - is... acceptable to me.

[firov]

Yes, I also encountered the problem of ambiguities, but I solved it with different names, in my wrapper
`public interface Storage {
T read(Function<StorageReadTransaction, T> function);

void modify(Consumer<StorageModifyTransaction> consumer);

<T> T modifyAndRead(Function<StorageModifyTransaction, T> consumer);

}
`
At first look it is a downgrade, but after using it i found, that it is not the problem, and clear name if we need to get return value is not a bad thing.