From 5f62f91cd5ad2fa080091fbd4042a0194e61d21d Mon Sep 17 00:00:00 2001 From: Ben McIlwain Date: Sun, 3 Nov 2019 14:08:31 -0500 Subject: [PATCH] Don't wrap exceptions thrown inside Cloud SQL transactions (#338) * Don't wrap exceptions thrown inside Cloud SQL transactions There's no reason to wrap them in PersistenceException, and it makes dealing with thrown exceptions harder as seen in the diffs on test classes in this commit. --- .../registry/model/registry/RegistryLockDao.java | 1 + .../model/transaction/JpaTransactionManagerImpl.java | 11 ++++------- .../registry/model/registry/RegistryLockDaoTest.java | 11 +++-------- .../google/registry/model/tmch/ClaimsListDaoTest.java | 5 +---- .../persistence/CurrencyUnitConverterTest.java | 6 +----- .../registry/schema/tld/PremiumListDaoTest.java | 10 +++------- 6 files changed, 13 insertions(+), 31 deletions(-) diff --git a/core/src/main/java/google/registry/model/registry/RegistryLockDao.java b/core/src/main/java/google/registry/model/registry/RegistryLockDao.java index 88e434587..1b88fff59 100644 --- a/core/src/main/java/google/registry/model/registry/RegistryLockDao.java +++ b/core/src/main/java/google/registry/model/registry/RegistryLockDao.java @@ -41,6 +41,7 @@ public final class RegistryLockDao { Long.class) .setParameter("verificationCode", verificationCode) .getSingleResult(); + // TODO(gbrodman): Don't throw NPE here. Maybe NoResultException fits better? checkNotNull(revisionId, "No registry lock with this code"); return em.find(RegistryLock.class, revisionId); }); diff --git a/core/src/main/java/google/registry/model/transaction/JpaTransactionManagerImpl.java b/core/src/main/java/google/registry/model/transaction/JpaTransactionManagerImpl.java index faab1e9e9..9d5e18042 100644 --- a/core/src/main/java/google/registry/model/transaction/JpaTransactionManagerImpl.java +++ b/core/src/main/java/google/registry/model/transaction/JpaTransactionManagerImpl.java @@ -79,17 +79,14 @@ public class JpaTransactionManagerImpl implements JpaTransactionManager { T result = work.run(); txn.commit(); return result; - } catch (Throwable transactionException) { - String rollbackMessage; + } catch (RuntimeException e) { try { txn.rollback(); - rollbackMessage = "transaction rolled back"; + logger.atWarning().log("Error during transaction; transaction rolled back"); } catch (Throwable rollbackException) { - logger.atSevere().withCause(rollbackException).log("Rollback failed, suppressing error"); - rollbackMessage = "transaction rollback failed"; + logger.atSevere().withCause(rollbackException).log("Rollback failed; suppressing error"); } - throw new PersistenceException( - "Error during transaction, " + rollbackMessage, transactionException); + throw e; } finally { txnInfo.clear(); } diff --git a/core/src/test/java/google/registry/model/registry/RegistryLockDaoTest.java b/core/src/test/java/google/registry/model/registry/RegistryLockDaoTest.java index c69203404..06d63fe90 100644 --- a/core/src/test/java/google/registry/model/registry/RegistryLockDaoTest.java +++ b/core/src/test/java/google/registry/model/registry/RegistryLockDaoTest.java @@ -25,7 +25,6 @@ import google.registry.schema.domain.RegistryLock.Action; import google.registry.testing.AppEngineRule; import java.util.Optional; import java.util.UUID; -import javax.persistence.PersistenceException; import org.junit.Rule; import org.junit.Test; import org.junit.runner.RunWith; @@ -54,15 +53,11 @@ public final class RegistryLockDaoTest { public void testSaveAndLoad_failure_differentCode() { RegistryLock lock = createLock(); RegistryLockDao.save(lock); - PersistenceException exception = + NullPointerException thrown = assertThrows( - PersistenceException.class, + NullPointerException.class, () -> RegistryLockDao.getByVerificationCode(UUID.randomUUID().toString())); - assertThat(exception) - .hasCauseThat() - .hasMessageThat() - .isEqualTo("No registry lock with this code"); - assertThat(exception).hasCauseThat().isInstanceOf(NullPointerException.class); + assertThat(thrown).hasMessageThat().isEqualTo("No registry lock with this code"); } @Test diff --git a/core/src/test/java/google/registry/model/tmch/ClaimsListDaoTest.java b/core/src/test/java/google/registry/model/tmch/ClaimsListDaoTest.java index c238f8efe..17beda3df 100644 --- a/core/src/test/java/google/registry/model/tmch/ClaimsListDaoTest.java +++ b/core/src/test/java/google/registry/model/tmch/ClaimsListDaoTest.java @@ -22,7 +22,6 @@ import google.registry.model.transaction.JpaTransactionManagerRule; import google.registry.schema.tmch.ClaimsList; import google.registry.testing.FakeClock; import javax.persistence.NoResultException; -import javax.persistence.PersistenceException; import org.junit.Rule; import org.junit.Test; import org.junit.runner.RunWith; @@ -71,9 +70,7 @@ public class ClaimsListDaoTest { @Test public void getCurrent_throwsNoResultExceptionIfTableIsEmpty() { - PersistenceException thrown = - assertThrows(PersistenceException.class, () -> ClaimsListDao.getCurrent()); - assertThat(thrown).hasCauseThat().isInstanceOf(NoResultException.class); + assertThrows(NoResultException.class, ClaimsListDao::getCurrent); } @Test diff --git a/core/src/test/java/google/registry/persistence/CurrencyUnitConverterTest.java b/core/src/test/java/google/registry/persistence/CurrencyUnitConverterTest.java index 6f1b5b97c..cca8315ed 100644 --- a/core/src/test/java/google/registry/persistence/CurrencyUnitConverterTest.java +++ b/core/src/test/java/google/registry/persistence/CurrencyUnitConverterTest.java @@ -75,11 +75,7 @@ public class CurrencyUnitConverterTest { jpaTm() .transact( () -> jpaTm().getEntityManager().find(TestEntity.class, "id").currency)); - assertThat(thrown) - .hasCauseThat() - .hasCauseThat() - .hasMessageThat() - .isEqualTo("Unknown currency 'XXXX'"); + assertThat(thrown).hasCauseThat().hasMessageThat().isEqualTo("Unknown currency 'XXXX'"); } @Entity(name = "TestEntity") // Override entity name to avoid the nested class reference. diff --git a/core/src/test/java/google/registry/schema/tld/PremiumListDaoTest.java b/core/src/test/java/google/registry/schema/tld/PremiumListDaoTest.java index 21e3d366e..1b37ad68e 100644 --- a/core/src/test/java/google/registry/schema/tld/PremiumListDaoTest.java +++ b/core/src/test/java/google/registry/schema/tld/PremiumListDaoTest.java @@ -21,7 +21,6 @@ import static google.registry.testing.JUnitBackports.assertThrows; import com.google.common.collect.ImmutableMap; import google.registry.model.transaction.JpaTransactionManagerRule; import java.math.BigDecimal; -import javax.persistence.PersistenceException; import org.joda.money.CurrencyUnit; import org.junit.Rule; import org.junit.Test; @@ -68,16 +67,13 @@ public class PremiumListDaoTest { @Test public void saveNew_throwsWhenPremiumListAlreadyExists() { PremiumListDao.saveNew(PremiumList.create("testlist", CurrencyUnit.USD, TEST_PRICES)); - PersistenceException thrown = + IllegalArgumentException thrown = assertThrows( - PersistenceException.class, + IllegalArgumentException.class, () -> PremiumListDao.saveNew( PremiumList.create("testlist", CurrencyUnit.USD, TEST_PRICES))); - assertThat(thrown) - .hasCauseThat() - .hasMessageThat() - .contains("A premium list of this name already exists"); + assertThat(thrown).hasMessageThat().contains("A premium list of this name already exists"); } @Test