From b9cfa655465598467820f639df7f9424998d9210 Mon Sep 17 00:00:00 2001 From: Lai Jiang Date: Thu, 14 Mar 2024 10:37:44 -0400 Subject: [PATCH] Refactor some code related to the transaction manager (#2366) Removed the deprecation mark as it is natural to expose methods related to a transaction like getting the entity manager or checking if one is in a transaction through the transaction manager interface. --- .../google/registry/bsa/BsaTransactions.java | 7 ++--- .../registry/bsa/persistence/Queries.java | 24 ++++++++------ .../transaction/JpaTransactionManager.java | 8 ++--- .../JpaTransactionManagerImpl.java | 31 ++----------------- .../transaction/TransactionManager.java | 12 ++----- 5 files changed, 26 insertions(+), 56 deletions(-) diff --git a/core/src/main/java/google/registry/bsa/BsaTransactions.java b/core/src/main/java/google/registry/bsa/BsaTransactions.java index ebeb9ea342d..609c49fa185 100644 --- a/core/src/main/java/google/registry/bsa/BsaTransactions.java +++ b/core/src/main/java/google/registry/bsa/BsaTransactions.java @@ -16,7 +16,6 @@ import static com.google.common.base.Verify.verify; import static google.registry.persistence.PersistenceModule.TransactionIsolationLevel.TRANSACTION_REPEATABLE_READ; -import static google.registry.persistence.transaction.JpaTransactionManagerImpl.isInTransaction; import static google.registry.persistence.transaction.TransactionManagerFactory.replicaTm; import static google.registry.persistence.transaction.TransactionManagerFactory.tm; @@ -36,18 +35,18 @@ public final class BsaTransactions { @CanIgnoreReturnValue public static T bsaTransact(Callable work) { - verify(!isInTransaction(), "May only be used for top-level transactions."); + verify(!tm().inTransaction(), "May only be used for top-level transactions."); return tm().transact(TRANSACTION_REPEATABLE_READ, work); } public static void bsaTransact(ThrowingRunnable work) { - verify(!isInTransaction(), "May only be used for top-level transactions."); + verify(!tm().inTransaction(), "May only be used for top-level transactions."); tm().transact(TRANSACTION_REPEATABLE_READ, work); } @CanIgnoreReturnValue public static T bsaQuery(Callable work) { - verify(!isInTransaction(), "May only be used for top-level transactions."); + verify(!tm().inTransaction(), "May only be used for top-level transactions."); // TRANSACTION_REPEATABLE_READ is default on replica. return replicaTm().transact(work); } diff --git a/core/src/main/java/google/registry/bsa/persistence/Queries.java b/core/src/main/java/google/registry/bsa/persistence/Queries.java index 0ee13691786..c8558548386 100644 --- a/core/src/main/java/google/registry/bsa/persistence/Queries.java +++ b/core/src/main/java/google/registry/bsa/persistence/Queries.java @@ -17,7 +17,7 @@ import static com.google.common.base.Verify.verify; import static google.registry.bsa.BsaStringUtils.DOMAIN_SPLITTER; import static google.registry.bsa.BsaTransactions.bsaQuery; -import static google.registry.persistence.transaction.JpaTransactionManagerImpl.em; +import static google.registry.persistence.transaction.TransactionManagerFactory.tm; import com.google.common.collect.ImmutableCollection; import com.google.common.collect.ImmutableList; @@ -39,7 +39,7 @@ private Queries() {} * JpaTransactionManagerImpl}. */ private static Object detach(Object obj) { - em().detach(obj); + tm().getEntityManager().detach(obj); return obj; } @@ -49,7 +49,8 @@ public static ImmutableList batchReadBsaLabelText( return ImmutableList.copyOf( bsaQuery( () -> - em().createQuery( + tm().getEntityManager() + .createQuery( "SELECT b.label FROM BsaLabel b WHERE b.label > :lastRead ORDER BY b.label", String.class) .setParameter("lastRead", lastRead.orElse("")) @@ -60,7 +61,8 @@ public static ImmutableList batchReadBsaLabelText( static Stream queryBsaUnblockableDomainByLabels( ImmutableCollection labels) { return ((Stream) - em().createQuery("FROM BsaUnblockableDomain WHERE label in (:labels)") + tm().getEntityManager() + .createQuery("FROM BsaUnblockableDomain WHERE label in (:labels)") .setParameter("labels", labels) .getResultStream()) .map(Queries::detach) @@ -69,7 +71,8 @@ static Stream queryBsaUnblockableDomainByLabels( static Stream queryBsaLabelByLabels(ImmutableCollection labels) { return ((Stream) - em().createQuery("FROM BsaLabel where label in (:labels)") + tm().getEntityManager() + .createQuery("FROM BsaLabel where label in (:labels)") .setParameter("labels", labels) .getResultStream()) .map(Queries::detach) @@ -77,7 +80,8 @@ static Stream queryBsaLabelByLabels(ImmutableCollection labels } static int deleteBsaLabelByLabels(ImmutableCollection labels) { - return em().createQuery("DELETE FROM BsaLabel where label IN (:deleted_labels)") + return tm().getEntityManager() + .createQuery("DELETE FROM BsaLabel where label IN (:deleted_labels)") .setParameter("deleted_labels", labels) .executeUpdate(); } @@ -87,7 +91,8 @@ static ImmutableList batchReadUnblockables( return ImmutableList.copyOf( bsaQuery( () -> - em().createQuery( + tm().getEntityManager() + .createQuery( "FROM BsaUnblockableDomain d WHERE d.label > :label OR (d.label = :label" + " AND d.tld > :tld) ORDER BY d.tld, d.label ") .setParameter("label", lastRead.map(d -> d.label).orElse("")) @@ -111,13 +116,14 @@ static ImmutableSet queryUnblockablesByNames(ImmutableSet domain "SELECT CONCAT(d.label, '.', d.tld) FROM \"BsaUnblockableDomain\" d " + "WHERE (d.label, d.tld) IN (%s)", labelTldParis); - return ImmutableSet.copyOf(em().createNativeQuery(sql).getResultList()); + return ImmutableSet.copyOf(tm().getEntityManager().createNativeQuery(sql).getResultList()); } static ImmutableSet queryNewlyCreatedDomains( ImmutableCollection tlds, DateTime minCreationTime, DateTime now) { return ImmutableSet.copyOf( - em().createQuery( + tm().getEntityManager() + .createQuery( "SELECT domainName FROM Domain WHERE creationTime >= :minCreationTime " + "AND deletionTime > :now " + "AND tld in (:tlds)", diff --git a/core/src/main/java/google/registry/persistence/transaction/JpaTransactionManager.java b/core/src/main/java/google/registry/persistence/transaction/JpaTransactionManager.java index d393f3b79a8..6cac899ae15 100644 --- a/core/src/main/java/google/registry/persistence/transaction/JpaTransactionManager.java +++ b/core/src/main/java/google/registry/persistence/transaction/JpaTransactionManager.java @@ -36,12 +36,10 @@ public interface JpaTransactionManager extends TransactionManager { * *

The returned instance is closed when the current transaction completes. * - * @deprecated Use the static {@link JpaTransactionManagerImpl#em} method for now. In current - * implementation the entity manager is obtained from a static {@code ThreadLocal} object that - * is set up by the outermost {@link #transact} call. As an instance method, this method gives - * the illusion that the call site has control over which database instance to use. + *

Note that in the current implementation the entity manager is obtained from a static {@code + * ThreadLocal} object that is set up by the outermost {@link #transact} call. Nested call sites + * have no control over which database instance to use. */ - @Deprecated // See Javadoc above. EntityManager getEntityManager(); /** diff --git a/core/src/main/java/google/registry/persistence/transaction/JpaTransactionManagerImpl.java b/core/src/main/java/google/registry/persistence/transaction/JpaTransactionManagerImpl.java index 49ad62d6b8c..59a06ce9d12 100644 --- a/core/src/main/java/google/registry/persistence/transaction/JpaTransactionManagerImpl.java +++ b/core/src/main/java/google/registry/persistence/transaction/JpaTransactionManagerImpl.java @@ -91,26 +91,6 @@ public class JpaTransactionManagerImpl implements JpaTransactionManager { private static final ThreadLocal transactionInfo = ThreadLocal.withInitial(TransactionInfo::new); - /** Returns true if inside a transaction; returns false otherwise. */ - public static boolean isInTransaction() { - return transactionInfo.get().inTransaction; - } - - /** - * Returns the {@link EntityManager} for the current database transaction. - * - *

This method must be call from inside a transaction. - */ - public static EntityManager em() { - EntityManager entityManager = transactionInfo.get().entityManager; - if (entityManager == null) { - throw new PersistenceException( - "No EntityManager has been initialized. getEntityManager() must be invoked in the scope" - + " of a transaction"); - } - return entityManager; - } - public JpaTransactionManagerImpl(EntityManagerFactory emf, Clock clock, boolean readOnly) { this.emf = emf; this.clock = clock; @@ -131,16 +111,10 @@ public EntityManager getStandaloneEntityManager() { return emf.createEntityManager(); } - @Deprecated // See Javadoc of interface method. @Override public EntityManager getEntityManager() { - EntityManager entityManager = transactionInfo.get().entityManager; - if (entityManager == null) { - throw new PersistenceException( - "No EntityManager has been initialized. getEntityManager() must be invoked in the scope" - + " of a transaction"); - } - return entityManager; + assertInTransaction(); + return transactionInfo.get().entityManager; } @Override @@ -158,7 +132,6 @@ public Query query(String sqlString) { return getEntityManager().createQuery(sqlString); } - @Deprecated // See Javadoc of instance method. @Override public boolean inTransaction() { return transactionInfo.get().inTransaction; diff --git a/core/src/main/java/google/registry/persistence/transaction/TransactionManager.java b/core/src/main/java/google/registry/persistence/transaction/TransactionManager.java index adae5c7fe35..d8cccaa53e0 100644 --- a/core/src/main/java/google/registry/persistence/transaction/TransactionManager.java +++ b/core/src/main/java/google/registry/persistence/transaction/TransactionManager.java @@ -34,16 +34,10 @@ public interface TransactionManager { /** * Returns {@code true} if the caller is in a transaction. * - *

Note that this function is kept for backward compatibility. We will review the use case - * later when adding the cloud sql implementation. - * - * @deprecated Use the static {@link JpaTransactionManagerImpl#isInTransaction()} method for now. - * In current implementation the entity manager is obtained from a static {@code ThreadLocal} - * object that is set up by the outermost {@link #transact} call. As an instance method, this - * method gives the illusion that the call site has control over which database instance to - * use. + *

Note that in the current implementation the entity manager is obtained from a static {@code + * ThreadLocal} object that is set up by the outermost {@link #transact} call. Nested call sites + * have no control over which database instance to use. */ - @Deprecated // See Javadoc above. boolean inTransaction(); /**