Don't automatically create an EntityManager outside of a UnitOfWork, …

…because it leads to leaks. Fixes #739, fixes #997, fixes #730, fixes #985, fixes #959, fixes #731. This also adds an optional Options to the JpaPersistModule constructor, if users wish to use the legacy behavior. This is a breaking change, but to a better default value. Users who want to keep the dangerous form can opt back in with the options.

PiperOrigin-RevId: 525852009

extensions/persist/src/com/google/inject/persist/jpa/JpaPersistModule.java

@@ -44,11 +44,17 @@
  */
 public final class JpaPersistModule extends PersistModule {
   private final String jpaUnit;
+  private final JpaPersistOptions options;
 
   public JpaPersistModule(String jpaUnit) {
+    this(jpaUnit, JpaPersistOptions.builder().build());
+  }
+
+  public JpaPersistModule(String jpaUnit, JpaPersistOptions options) {
     Preconditions.checkArgument(
         null != jpaUnit && jpaUnit.length() > 0, "JPA unit name must be a non-empty string.");
     this.jpaUnit = jpaUnit;
+    this.options = options;
   }
 
   private Map<?, ?> properties;
@@ -57,6 +63,7 @@ public JpaPersistModule(String jpaUnit) {
   @Override
   protected void configurePersistence() {
     bindConstant().annotatedWith(Jpa.class).to(jpaUnit);
+    bind(JpaPersistOptions.class).annotatedWith(Jpa.class).toInstance(options);
 
     bind(JpaPersistService.class).in(Singleton.class);
 

extensions/persist/src/com/google/inject/persist/jpa/JpaPersistOptions.java

@@ -0,0 +1,61 @@
+/*
+ * Copyright (C) 2023 Google, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.google.inject.persist.jpa;
+
+/** Options that configure how the JPA persist service will work. */
+public final class JpaPersistOptions {
+
+  private final boolean autoBeginWorkOnEntityManagerCreation;
+
+  private JpaPersistOptions(JpaPersistOptions.Builder builder) {
+    this.autoBeginWorkOnEntityManagerCreation = builder.autoBeginWorkOnEntityManagerCreation;
+  }
+
+  /**
+   * Returns true if the work unit should automatically begin when the EntityManager is created, if
+   * it hasn't already begun.
+   *
+   * <p>This defaults to <b>false</b> because it's not safe, as careless usage can lead to leaking
+   * sessions.
+   */
+  public boolean getAutoBeginWorkOnEntityManagerCreation() {
+    return autoBeginWorkOnEntityManagerCreation;
+  }
+
+  /** Returns a builder to set options. */
+  public static Builder builder() {
+    return new Builder();
+  }
+
+  /** A builder to create the options. */
+  public static final class Builder {
+    private boolean autoBeginWorkOnEntityManagerCreation;
+
+    private Builder() {}
+
+    public JpaPersistOptions build() {
+      return new JpaPersistOptions(this);
+    }
+
+    /** Sets the {@link JpaPersistOptions#getAutoBeginWorkOnEntityManagerCreation} property. */
+    public Builder setAutoBeginWorkOnEntityManagerCreation(
+        boolean autoBeginWorkOnEntityManagerCreation) {
+      this.autoBeginWorkOnEntityManagerCreation = autoBeginWorkOnEntityManagerCreation;
+      return this;
+    }
+  }
+}

extensions/persist/src/com/google/inject/persist/jpa/JpaPersistService.java

@@ -33,24 +33,30 @@
 import javax.persistence.EntityManagerFactory;
 import javax.persistence.Persistence;
 
-/** @author Dhanji R. Prasanna (dhanji@gmail.com) */
+/**
+ * @author Dhanji R. Prasanna (dhanji@gmail.com)
+ */
 @Singleton
 class JpaPersistService implements Provider<EntityManager>, UnitOfWork, PersistService {
   private final ThreadLocal<EntityManager> entityManager = new ThreadLocal<>();
 
   private final String persistenceUnitName;
   private final Map<?, ?> persistenceProperties;
+  private final JpaPersistOptions options;
 
   @Inject
   public JpaPersistService(
-      @Jpa String persistenceUnitName, @Nullable @Jpa Map<?, ?> persistenceProperties) {
+      @Jpa JpaPersistOptions options,
+      @Jpa String persistenceUnitName,
+      @Nullable @Jpa Map<?, ?> persistenceProperties) {
+    this.options = options;
     this.persistenceUnitName = persistenceUnitName;
     this.persistenceProperties = persistenceProperties;
   }
 
   @Override
   public EntityManager get() {
-    if (!isWorking()) {
+    if (options.getAutoBeginWorkOnEntityManagerCreation() && !isWorking()) {
       begin();
     }
 

extensions/persist/test/com/google/inject/persist/BUILD

@@ -27,6 +27,7 @@ java_library(
         "//third_party/java/hibernate:hibernate5",
         "//third_party/java/hsqldb:hsqldb2",
         "//third_party/java/javax_persistence",
+        "//third_party/java/jsr330_inject",
         "//third_party/java/junit",
         "//third_party/java/mockito",
     ],

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

@@ -21,10 +21,12 @@
 import com.google.inject.Injector;
 import com.google.inject.persist.PersistService;
 import com.google.inject.persist.Transactional;
+import com.google.inject.persist.UnitOfWork;
 import java.io.FileNotFoundException;
 import java.io.IOException;
 import java.util.Date;
 import java.util.List;
+import javax.inject.Provider;
 import javax.persistence.EntityManager;
 import javax.persistence.EntityManagerFactory;
 import junit.framework.TestCase;
@@ -62,6 +64,7 @@ public void tearDown() {
   public void testSimpleTransaction() {
     injector.getInstance(TransactionalObject.class).runOperationInTxn();
 
+    injector.getInstance(UnitOfWork.class).begin();
     EntityManager session = injector.getInstance(EntityManager.class);
     assertFalse(
         "EntityManager was not closed by transactional service",
@@ -92,6 +95,7 @@ public void testSimpleTransactionRollbackOnChecked() {
       // ignore
     }
 
+    injector.getInstance(UnitOfWork.class).begin();
     EntityManager session = injector.getInstance(EntityManager.class);
     assertFalse(
         "EntityManager was not closed by transactional service (rollback didnt happen?)",
@@ -119,6 +123,7 @@ public void testSimpleTransactionRollbackOnCheckedExcepting() {
       // ignored
     }
 
+    injector.getInstance(UnitOfWork.class).begin();
     EntityManager session = injector.getInstance(EntityManager.class);
     assertFalse(
         "Txn was not closed by transactional service (commit didnt happen?)",
@@ -144,6 +149,7 @@ public void testSimpleTransactionRollbackOnUnchecked() {
       // ignore
     }
 
+    injector.getInstance(UnitOfWork.class).begin();
     EntityManager session = injector.getInstance(EntityManager.class);
     assertFalse(
         "EntityManager was not closed by transactional service (rollback didnt happen?)",
@@ -184,9 +190,10 @@ public void testTransactionalDoesntAffectObjectMethods() {
 
   @Transactional
   public static class TransactionalObject {
-    @Inject EntityManager session;
+    @Inject Provider<EntityManager> sessionProvider;
 
     public void runOperationInTxn() {
+      EntityManager session = sessionProvider.get();
       assertTrue(session.getTransaction().isActive());
       JpaTestEntity entity = new JpaTestEntity();
       entity.setText(UNIQUE_TEXT);
@@ -196,10 +203,11 @@ public void runOperationInTxn() {
 
   @Transactional
   public static class TransactionalObject4 {
-    @Inject EntityManager session;
+    @Inject Provider<EntityManager> sessionProvider;
 
     @Transactional
     public void runOperationInTxnThrowingUnchecked() {
+      EntityManager session = sessionProvider.get();
       assertTrue(session.getTransaction().isActive());
       JpaTestEntity entity = new JpaTestEntity();
       entity.setText(TRANSIENT_UNIQUE_TEXT);
@@ -211,9 +219,10 @@ public void runOperationInTxnThrowingUnchecked() {
 
   @Transactional(rollbackOn = IOException.class, ignore = FileNotFoundException.class)
   public static class TransactionalObject3 {
-    @Inject EntityManager session;
+    @Inject Provider<EntityManager> sessionProvider;
 
     public void runOperationInTxnThrowingCheckedExcepting() throws IOException {
+      EntityManager session = sessionProvider.get();
       assertTrue(session.getTransaction().isActive());
       JpaTestEntity entity = new JpaTestEntity();
       entity.setText(UNIQUE_TEXT_2);
@@ -225,9 +234,10 @@ public void runOperationInTxnThrowingCheckedExcepting() throws IOException {
 
   @Transactional(rollbackOn = IOException.class)
   public static class TransactionalObject2 {
-    @Inject EntityManager session;
+    @Inject Provider<EntityManager> sessionProvider;
 
     public void runOperationInTxnThrowingChecked() throws IOException {
+      EntityManager session = sessionProvider.get();
       assertTrue(session.getTransaction().isActive());
       JpaTestEntity entity = new JpaTestEntity();
       entity.setText(TRANSIENT_UNIQUE_TEXT);

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

@@ -54,6 +54,8 @@ public void testSessionCreateOnInjection() {
     //startup persistence
     injector.getInstance(PersistService.class).start();
 
+    injector.getInstance(UnitOfWork.class).begin();
+
     //obtain em
     assertTrue(injector.getInstance(EntityManager.class).isOpen());
   }

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

@@ -22,6 +22,7 @@
 import com.google.inject.Provider;
 import com.google.inject.persist.PersistService;
 import com.google.inject.persist.Transactional;
+import com.google.inject.persist.UnitOfWork;
 import com.google.inject.persist.finder.Finder;
 import java.util.ArrayList;
 import java.util.Date;
@@ -62,11 +63,7 @@ public void testDynamicFinderListAll() {
 
     dao.persist(te);
 
-    //im not sure this hack works...
-    assertFalse(
-        "Duplicate entity managers crossing-scope",
-        dao.lastEm.equals(injector.getInstance(EntityManager.class)));
-
+    injector.getInstance(UnitOfWork.class).begin();
     List<JpaTestEntity> list = injector.getInstance(JpaFinder.class).listAll();
     assertNotNull(list);
     assertFalse(list.isEmpty());

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

@@ -50,6 +50,8 @@ public void testSessionCreateOnInjection() {
     //startup persistence
     injector.getInstance(PersistService.class).start();
 
+    injector.getInstance(UnitOfWork.class).begin();
+
     //obtain em
     assertTrue(injector.getInstance(EntityManager.class).isOpen());
   }

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

@@ -57,11 +57,6 @@ public void testEntityManagerLifecyclePerTxn() {
 
     dao.persist(te);
 
-    //im not sure this hack works...
-    assertFalse(
-        "Duplicate entity managers crossing-scope",
-        dao.lastEm.equals(injector.getInstance(EntityManager.class)));
-
     //try to start a new em in a new txn
     dao = injector.getInstance(JpaDao.class);
 
@@ -80,11 +75,6 @@ public void testEntityManagerLifecyclePerTxn2() {
 
     dao.persist(te);
 
-    //im not sure this hack works...
-    assertFalse(
-        "Duplicate entity managers crossing-scope",
-        dao.lastEm.equals(injector.getInstance(EntityManager.class)));
-
     //try to start a new em in a new txn
     dao = injector.getInstance(JpaDao.class);
 

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

@@ -24,6 +24,7 @@
 import com.google.inject.persist.UnitOfWork;
 import java.io.IOException;
 import java.util.Date;
+import javax.inject.Provider;
 import javax.persistence.EntityManager;
 import javax.persistence.EntityManagerFactory;
 import javax.persistence.NoResultException;
@@ -58,6 +59,7 @@ public void testSimpleTransaction() {
         .getInstance(JoiningLocalTransactionsTest.TransactionalObject.class)
         .runOperationInTxn();
 
+    injector.getInstance(UnitOfWork.class).begin();
     EntityManager em = injector.getInstance(EntityManager.class);
     assertFalse("txn was not closed by transactional service", em.getTransaction().isActive());
 
@@ -86,6 +88,7 @@ public void testSimpleTransactionRollbackOnChecked() {
       injector.getInstance(UnitOfWork.class).end();
     }
 
+    injector.getInstance(UnitOfWork.class).begin();
     EntityManager em = injector.getInstance(EntityManager.class);
 
     assertFalse(
@@ -114,6 +117,7 @@ public void testSimpleTransactionRollbackOnUnchecked() {
       injector.getInstance(UnitOfWork.class).end();
     }
 
+    injector.getInstance(UnitOfWork.class).begin();
     EntityManager em = injector.getInstance(EntityManager.class);
     assertFalse(
         "Session was not closed by transactional service (rollback didnt happen?)",
@@ -131,11 +135,11 @@ public void testSimpleTransactionRollbackOnUnchecked() {
   }
 
   public static class TransactionalObject {
-    private final EntityManager em;
+    private final Provider<EntityManager> emProvider;
 
     @Inject
-    public TransactionalObject(EntityManager em) {
-      this.em = em;
+    public TransactionalObject(Provider<EntityManager> emProvider) {
+      this.emProvider = emProvider;
     }
 
     @Transactional
@@ -145,6 +149,7 @@ public void runOperationInTxn() {
 
     @Transactional(rollbackOn = IOException.class)
     public void runOperationInTxnInternal() {
+      EntityManager em = emProvider.get();
       JpaTestEntity entity = new JpaTestEntity();
       entity.setText(UNIQUE_TEXT);
       em.persist(entity);
@@ -159,7 +164,7 @@ public void runOperationInTxnThrowingChecked() throws IOException {
     private void runOperationInTxnThrowingCheckedInternal() throws IOException {
       JpaTestEntity entity = new JpaTestEntity();
       entity.setText(TRANSIENT_UNIQUE_TEXT);
-      em.persist(entity);
+      emProvider.get().persist(entity);
 
       throw new IOException();
     }
@@ -173,7 +178,7 @@ public void runOperationInTxnThrowingUnchecked() {
     public void runOperationInTxnThrowingUncheckedInternal() {
       JpaTestEntity entity = new JpaTestEntity();
       entity.setText(TRANSIENT_UNIQUE_TEXT);
-      em.persist(entity);
+      emProvider.get().persist(entity);
 
       throw new IllegalStateException();
     }

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

@@ -34,7 +34,8 @@ public class JpaPersistServiceTest extends TestCase {
   private static final Properties PERSISTENCE_PROPERTIES = new Properties();
 
   private final JpaPersistService sut =
-      new JpaPersistService(PERSISTENCE_UNIT_NAME, PERSISTENCE_PROPERTIES);
+      new JpaPersistService(
+          JpaPersistOptions.builder().build(), PERSISTENCE_UNIT_NAME, PERSISTENCE_PROPERTIES);
   private final PersistenceProvider provider = mock(PersistenceProvider.class);
   private final EntityManagerFactory factory = mock(EntityManagerFactory.class);
   private final EntityManager entityManager = mock(EntityManager.class);