PersistModule doesn't bind interceptor for class-level @Transactional annotation

[GoogleCodeExporter]

I was working on a JDBC implementation of guice-persist and noticed that my 
transactional interceptor wasn't being called if a class was annotated with 
@Transactional and the methods were not. Currently, the guice-persist tests 
don't seem to catch it, but if you change the method runOperationInTxn() in 
ClassLevelManagedLocalTransactionsTest.TransactionalObject to

    public void runOperationInTxn() {
      assertTrue(session.getTransaction().isActive());
      JpaTestEntity entity = new JpaTestEntity();
      entity.setText(UNIQUE_TEXT);
      session.persist(entity);
    }

then testSimpleTransaction() will fail.

PersistModule needs

    bindInterceptor(annotatedWith(Transactional.class), any(), getTransactionInterceptor());

added in addition to the single call to bindInterceptor that it currently has.

I think this needs to be fixed before 3.0 is released.

Original issue reported on code.google.com by cgdec...@gmail.com on 27 Jan 2011 at 11:43

[GoogleCodeExporter]

This is also the reason the two disabled tests in 
ClassLevelManagedLocalTransactionsTest would fail (no rollback occurring). 
Attached is a patch that re-enables those disabled tests and adds assertions 
that a transaction is active when the methods in the @Transactional objects are 
called. It also modifies PersistModule to fix the issue.

Original comment by cgdec...@gmail.com on 28 Jan 2011 at 12:33

Attachments:

• issue595.patch

[GoogleCodeExporter]

A couple other concerns about guice-persist:

1. The javadoc for UnitOfWork.begin() specifies that "If there is already [a 
session to the data layer] open, the invocation will do nothing. In this way, 
you can define arbitrary units-of-work that nest within one another safely." 
However, JpaPersistService.begin() throws IllegalStateException if it's called 
twice on the same thread without a call to end() in between. This seems to 
break the interface's contract.

However, if you have begin() do nothing if it a unit of work is already open 
(which seems necessary by the contract), you'd need to ensure that only the 
call to end() that matches the first call to begin actually closes the 
session... perhaps keep a ThreadLocal count of the number of times begin() is 
called and then count down again each time end() is called, closing the session 
when end() has been called as many times as begin() was?

2. JpaPersistService.get() starts a UnitOfWork if one has not already been 
started. It then does a checkState to ensure that a UnitOfWork has been 
started, with a message indicating that the user should ensure that they start 
a UnitOfWork before trying to get() an EntityManager. Since a UnitOfWork is 
always started before this check, it'll never throw an exception.

However, I think that the correct behavior here would be to throw the exception 
rather than starting a UnitOfWork for the user. The problem is that an 
EntityManager will be set in the ThreadLocal, but it probably will not be 
removed because if the user didn't call UnitOfWork.begin() before getting the 
EntityManager, they probably won't call UnitOfWork.end() after either. This 
seems like a problem to me. The user should be required to use UnitOfWork, or 
at least @Transactional (which ensures the UnitOfWork is started and ended 
properly if one isn't started already), to manage the lifetime of the session.

Original comment by cgdec...@gmail.com on 29 Jan 2011 at 7:51

[GoogleCodeExporter]

About #2 above: On second thought, is the point to allow users to choose to not 
manually start a UnitOfWork and have it only start when (and if) it's needed? 
And then to have them call end() at some point in case one was started? I can 
see the advantage in that, but I still somewhat worry that it makes it too easy 
to leave an unclosed EntityManager in the ThreadLocal. I also think in many 
cases, the EntityManager would likely be retrieved even if it weren't used, 
either due to a high level @Transactional method or due to a class that has the 
EntityManager injected into it being instantiated.

I think my preference would be to require that either a UnitOfWork be manually 
started before a class that has an EntityManager injected into it is 
instantiated (already the case for any webapp using PersistFilter) or that a 
Provider<EntityManager> be injected and called in each transactional method 
when a UnitOfWork is guaranteed to be active.

Original comment by cgdec...@gmail.com on 30 Jan 2011 at 3:01

[GoogleCodeExporter]

fixed in r1493, thanks for reporting it!  if the other problems aren't open as 
bug reports already, please file new bugs for those.

Original comment by sberlin on 14 Feb 2011 at 1:13

• Changed state: Fixed

[GoogleCodeExporter]

FYI - I've raised two issues which relate to the points made in comment 2 (730 
and 731). I've also attached a patch to issue 730 which addresses both of 
these. 

Point 1 - addressed using a ThreadLocal count of invocations, as suggested here
Point 2 - get() no longer starts a UnitOfWork. It will throw an 
IllegalStateException if no UnitOfWork is active

Original comment by spootsy....@gmail.com on 10 Oct 2012 at 11:50