jakartaee / persistence

https://jakartaee.github.io/persistence/
Other
191 stars 55 forks source link

add methods for managing Session/Transaction lifecycle to SessionFactory #410

Closed gavinking closed 11 months ago

gavinking commented 1 year ago

The transaction management idiom in JPA is not so trivial:

var entityManager = factory.createEntityManager();
var transaction = entityManager.getTransaction();
try {
    transaction.begin();
    // do some work...
    transaction.commit();
}
catch (Exception e) {
    if (transaction.isActive()) {
        try {
            transaction.rollback();
        }
        catch (Exception x) {
            e.addSuppressed(x);
        }
   }
   throw e; 
}
finally {
    entityManager.close();
} 

I think we should add a withTransaction() method to EntityManagerFactory declared as follows:

<R> R withTransaction(Function<EntityManager,R> work);

The work would be performed within the scope of a resource-local transaction.

rbygrave commented 1 year ago

I presume the alternative of using try-with-resources blocks is not workable? Seems it could be done for EntityManager but problematic to get a transaction.close() in place?

gavinking commented 1 year ago

@rbygrave I don't think we would have a good way to detect that an exception had been thrown from code inside AutoCloseable.close(). So no, I don't see a way to do it, though I have not thought deeply about it.

gavinking commented 1 year ago

Folks, any feedback on https://github.com/jakartaee/persistence/pull/414?

One issue here is whether we need two flavors of withTransaction(), one void, and the other returning a value.

I think we do, but I don't know what to call them.

gavinking commented 11 months ago

@hantsy @Tomas-Kraus @lukasj and everyone else:

  1. Are there any votes for renaming to executeInTransaction() + computeInTransaction()?
  2. Alternatively, would anyone be in favor of just calling these methods run() and call() or execute() and compute(), and letting you pass in a TxType.REQUIRED, TxType.REQUIRES_NEW, etc.

So according to 2, you would write:

entityManagerFactory.execute(TxType.REQUIRED, entityManager -> {
    entityManager.persist(book);
});

which is obviously quite a lot harder to type than:

entityManagerFactory.executeInTransaction(entityManager -> {
    entityManager.persist(book);
});

But it's not actually that much worse to look at, and it's quite a lot more flexible.

Another argument agains 2 is that the way jakarta.transaction actually defines this enum is IMO pretty nasty:

We could of course define our own TransactionPropagationType enum, perhaps justifiable on the basis that it's not just for JTA but also for resource-local transactions, but I would really prefer not to go there.

lukasj commented 11 months ago

For now, let's go without direct dependency on JTA API which would bring in CDI API (transitively)