Don't wrap methods declared by Object in transactions. Fixes #958, fixes #788.

PiperOrigin-RevId: 526028062

Author: sameb

extensions/persist/pom.xml

@@ -15,9 +15,9 @@
 
   <dependencies>
     <dependency>
-      <groupId>org.hibernate.javax.persistence</groupId>
-      <artifactId>hibernate-jpa-2.0-api</artifactId>
-      <version>1.0.0.Final</version>
+      <groupId>javax.persistence</groupId>
+      <artifactId>javax.persistence-api</artifactId>
+      <version>2.2</version>
       <scope>provided</scope>
     </dependency>
     <dependency>

extensions/persist/src/com/google/inject/persist/PersistModule.java

@@ -21,6 +21,9 @@
 
 import com.google.inject.AbstractModule;
 import com.google.inject.internal.InternalFlags;
+import com.google.inject.matcher.AbstractMatcher;
+import com.google.inject.matcher.Matcher;
+import java.lang.reflect.Method;
 import org.aopalliance.intercept.MethodInterceptor;
 
 /**
@@ -38,7 +41,8 @@ protected final void configure() {
     requireBinding(UnitOfWork.class);
     if (InternalFlags.isBytecodeGenEnabled()) {
       // class-level @Transacational
-      bindInterceptor(annotatedWith(Transactional.class), any(), getTransactionInterceptor());
+      bindInterceptor(
+          annotatedWith(Transactional.class), NOT_OBJECT_METHOD, getTransactionInterceptor());
       // method-level @Transacational
       bindInterceptor(any(), annotatedWith(Transactional.class), getTransactionInterceptor());
     }
@@ -47,4 +51,12 @@ protected final void configure() {
   protected abstract void configurePersistence();
 
   protected abstract MethodInterceptor getTransactionInterceptor();
+
+  private static final Matcher<Method> NOT_OBJECT_METHOD =
+      new AbstractMatcher<Method>() {
+        @Override
+        public boolean matches(Method m) {
+          return !Object.class.equals(m.getDeclaringClass());
+        }
+      };
 }

extensions/persist/test/com/google/inject/persist/jpa/ClassLevelManagedLocalTransactionsTest.java

@@ -26,6 +26,7 @@
 import java.util.Date;
 import java.util.List;
 import javax.persistence.EntityManager;
+import javax.persistence.EntityManagerFactory;
 import junit.framework.TestCase;
 
 /**
@@ -48,7 +49,7 @@ public class ClassLevelManagedLocalTransactionsTest extends TestCase {
   public void setUp() {
     injector = Guice.createInjector(new JpaPersistModule("testUnit"));
 
-    //startup persistence
+    // startup persistence
     injector.getInstance(PersistService.class).start();
   }
 
@@ -66,7 +67,7 @@ public void testSimpleTransaction() {
         "EntityManager was not closed by transactional service",
         session.getTransaction().isActive());
 
-    //test that the data has been stored
+    // test that the data has been stored
     session.getTransaction().begin();
     Object result =
         session
@@ -88,15 +89,15 @@ public void testSimpleTransactionRollbackOnChecked() {
     try {
       injector.getInstance(TransactionalObject2.class).runOperationInTxnThrowingChecked();
     } catch (IOException e) {
-      //ignore
+      // ignore
     }
 
     EntityManager session = injector.getInstance(EntityManager.class);
     assertFalse(
         "EntityManager was not closed by transactional service (rollback didnt happen?)",
         session.getTransaction().isActive());
 
-    //test that the data has been stored
+    // test that the data has been stored
     session.getTransaction().begin();
     List<?> result =
         session
@@ -115,15 +116,15 @@ public void testSimpleTransactionRollbackOnCheckedExcepting() {
       injector.getInstance(TransactionalObject3.class).runOperationInTxnThrowingCheckedExcepting();
       fail("Exception was not thrown by test txn-al method!");
     } catch (IOException e) {
-      //ignored
+      // ignored
     }
 
     EntityManager session = injector.getInstance(EntityManager.class);
     assertFalse(
         "Txn was not closed by transactional service (commit didnt happen?)",
         session.getTransaction().isActive());
 
-    //test that the data has been stored
+    // test that the data has been stored
     session.getTransaction().begin();
     Object result =
         session
@@ -140,15 +141,15 @@ public void testSimpleTransactionRollbackOnUnchecked() {
     try {
       injector.getInstance(TransactionalObject4.class).runOperationInTxnThrowingUnchecked();
     } catch (RuntimeException re) {
-      //ignore
+      // ignore
     }
 
     EntityManager session = injector.getInstance(EntityManager.class);
     assertFalse(
         "EntityManager was not closed by transactional service (rollback didnt happen?)",
         session.getTransaction().isActive());
 
-    //test that the data has been stored
+    // test that the data has been stored
     session.getTransaction().begin();
     List<?> result =
         session
@@ -161,6 +162,26 @@ public void testSimpleTransactionRollbackOnUnchecked() {
     assertTrue("a result was returned! rollback sure didnt happen!!!", result.isEmpty());
   }
 
+  public void testTransactionalDoesntAffectObjectMethods() {
+    // Given a persist service that tracks when it's called
+    JpaPersistService persistService = injector.getInstance(JpaPersistService.class);
+    EntityManagerFactory originalEMF = injector.getInstance(EntityManagerFactory.class);
+    TrackedEntityManagerFactory trackingEMF = new TrackedEntityManagerFactory(originalEMF);
+    persistService.start(trackingEMF);
+
+    FakeTransactionalObject txnObj = injector.getInstance(FakeTransactionalObject.class);
+
+    String unused = txnObj.toString();
+    assertFalse(
+        "Should not have created a transaction for toString method",
+        trackingEMF.hasCreatedSomeEntityManager());
+
+    txnObj.fakeTransactionalMethod();
+    assertTrue(
+        "Transaction should have been created for normal method",
+        trackingEMF.hasCreatedSomeEntityManager());
+  }
+
   @Transactional
   public static class TransactionalObject {
     @Inject EntityManager session;
@@ -215,4 +236,9 @@ public void runOperationInTxnThrowingChecked() throws IOException {
       throw new IOException();
     }
   }
+
+  @Transactional
+  public static class FakeTransactionalObject {
+    public void fakeTransactionalMethod() {}
+  }
 }

extensions/persist/test/com/google/inject/persist/jpa/TrackedEntityManagerFactory.java

@@ -0,0 +1,107 @@
+package com.google.inject.persist.jpa;
+
+import java.util.Map;
+import javax.persistence.Cache;
+import javax.persistence.EntityGraph;
+import javax.persistence.EntityManager;
+import javax.persistence.EntityManagerFactory;
+import javax.persistence.PersistenceUnitUtil;
+import javax.persistence.Query;
+import javax.persistence.SynchronizationType;
+import javax.persistence.criteria.CriteriaBuilder;
+import javax.persistence.metamodel.Metamodel;
+
+/** Proxy EntityManager that adds tracking capabilities, keeping a count of created objects. */
+class TrackedEntityManagerFactory implements EntityManagerFactory {
+
+  private final EntityManagerFactory delegate;
+  private int entityManagerCreatedCount = 0;
+
+  public TrackedEntityManagerFactory(EntityManagerFactory delegate) {
+    this.delegate = delegate;
+  }
+
+  public boolean hasCreatedSomeEntityManager() {
+    return (entityManagerCreatedCount > 0);
+  }
+
+  public int getEntityManagerCreatedCount() {
+    return entityManagerCreatedCount;
+  }
+
+  @Override
+  public boolean isOpen() {
+    return delegate.isOpen();
+  }
+
+  @Override
+  public Map<String, Object> getProperties() {
+    return delegate.getProperties();
+  }
+
+  @Override
+  public PersistenceUnitUtil getPersistenceUnitUtil() {
+    return delegate.getPersistenceUnitUtil();
+  }
+
+  @Override
+  public Metamodel getMetamodel() {
+    return delegate.getMetamodel();
+  }
+
+  @Override
+  public CriteriaBuilder getCriteriaBuilder() {
+    return delegate.getCriteriaBuilder();
+  }
+
+  @Override
+  public Cache getCache() {
+    return delegate.getCache();
+  }
+
+  @SuppressWarnings("rawtypes")
+  @Override
+  public EntityManager createEntityManager(Map arg0) {
+    entityManagerCreatedCount++;
+    return delegate.createEntityManager(arg0);
+  }
+
+  @Override
+  public EntityManager createEntityManager() {
+    entityManagerCreatedCount++;
+    return delegate.createEntityManager();
+  }
+
+  @Override
+  public EntityManager createEntityManager(SynchronizationType synchronizationType) {
+    entityManagerCreatedCount++;
+    return delegate.createEntityManager(synchronizationType);
+  }
+
+  @SuppressWarnings("rawtypes")
+  @Override
+  public EntityManager createEntityManager(SynchronizationType synchronizationType, Map map) {
+    entityManagerCreatedCount++;
+    return delegate.createEntityManager(synchronizationType, map);
+  }
+
+  @Override
+  public void close() {
+    delegate.close();
+  }
+
+  @Override
+  public void addNamedQuery(String name, Query query) {
+    delegate.addNamedQuery(name, query);
+  }
+
+  @Override
+  public <T> T unwrap(Class<T> cls) {
+    return delegate.unwrap(cls);
+  }
+
+  @Override
+  public <T> void addNamedEntityGraph(String graphName, EntityGraph<T> entityGraph) {
+    delegate.addNamedEntityGraph(graphName, entityGraph);
+  }
+}