mirrors/nomulus
1
0
Fork 0
mirror of https://github.com/google/nomulus synced 2026-09-02 14:17:12 +00:00
Code Issues Releases Wiki Activity

Refactor transact() related methods. (#2195)

Browse Source 
This PR makes a few changes to make it possible to turn on
per-transaction isolation level with minimal disruption:

1) Changed the signatures of transact() and reTransact() methods to allow
passing in lambdas that throw checked exceptions. Previously one has
always to wrap such lambdas in try-and-retrow blocks, which wasn't a
big issue when one can liberally open nested transactions around small
lambdas and keeps the "throwing" part outside the lambda. This becomes a
much bigger hassle when the goal is to eliminate nested transactions and
put as much code as possible within the top-level lambda. As a result,
the transactNoRetry() method now handles checked exceptions by re-throwing
them as runtime exceptions.

2) Changed the name and meaning of the config file field that used to
indicate if per-transaction isolation level is enabled or not. Now it
decides if transact() is called within a transaction, whether to
throw or to log, regardless whether the transaction could have
succeeded based on the isolation override level (if provided). The
flag will initially be set to false and would help us identify all
instances of nested calls and either refactor them or use reTransact()
instead. Once we are fairly certain that no nested calls to transact()
exists, we flip the flag to true and start enforcing this logic.
Eventually the flag will go away and nested calls to transact() will
always throw.

3) Per-transaction isolation level will now always be applied, if an
override is provided. Because currently there should be no actual
use of such feature (except for places where we explicitly use an
override and have ensured no nested transactions exist, like in
RefreshDnsForAllDomainsAction), we do not expect any issues with
conflicting isolation levels, which would resulted in failure.

3) transactNoRetry() is made package private and removed from the
exposed API of JpaTransactionManager. This saves a lot of redundant
methods that do not have a practical use. The only instances where this
method was called outside the package was in the reader of
RegistryJpaIO, which should have no problem with retrying.
This commit is contained in:
Lai Jiang
2023-11-03 17:43:27 -04:00
committed by GitHub
parent cd23fea698
commit 08471242df
13 changed files with 270 additions and 329 deletions
Download Patch File Download Diff File Expand all files Collapse all files
core/src/test/java/google/registry/flows/FlowRunnerTest.java
+4 -7
View File
@@ -31,7 +31,6 @@ import com.google.common.base.Splitter;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.ImmutableSortedMap;
import com.google.common.testing.TestLogHandler;
import google.registry.config.RegistryConfig;
import google.registry.flows.certs.CertificateChecker;
import google.registry.model.eppcommon.Trid;
import google.registry.model.eppoutput.EppOutput.ResponseOrGreeting;
@@ -89,8 +88,8 @@ class FlowRunnerTest {
@Override
public ResponseOrGreeting run() {
tm().assertTransactionIsolationLevel(
isolationLevel.orElse(tm().getDefaultTransactionIsolationLevel()));
assertThat(tm().getCurrentTransactionIsolationLevel())
.isEqualTo(isolationLevel.orElse(tm().getDefaultTransactionIsolationLevel()));
return mock(EppResponse.class);
}
}
@@ -136,10 +135,8 @@ class FlowRunnerTest {
Optional.of(TransactionIsolationLevel.TRANSACTION_READ_UNCOMMITTED);
flowRunner.flowClass = TestTransactionalFlow.class;
flowRunner.flowProvider = () -> new TestTransactionalFlow(flowRunner.isolationLevelOverride);
if (RegistryConfig.getHibernatePerTransactionIsolationEnabled()) {
flowRunner.run(eppMetricBuilder);
assertThat(eppMetricBuilder.build().getCommandName()).hasValue("TestTransactional");
}
flowRunner.run(eppMetricBuilder);
assertThat(eppMetricBuilder.build().getCommandName()).hasValue("TestTransactional");
}
@Test
core/src/test/java/google/registry/persistence/transaction/DatabaseExceptionTest.java
+3 -3
View File
@@ -18,7 +18,7 @@ import static com.google.common.truth.Truth.assertThat;
import static com.google.common.truth.Truth8.assertThat;
import static google.registry.persistence.transaction.DatabaseException.getSqlError;
import static google.registry.persistence.transaction.DatabaseException.getSqlExceptionDetails;
import static google.registry.persistence.transaction.DatabaseException.tryWrapAndThrow;
import static google.registry.persistence.transaction.DatabaseException.throwIfSqlException;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.times;
@@ -99,13 +99,13 @@ public class DatabaseExceptionTest {
@Test
void tryWrapAndThrow_notSQLException() {
RuntimeException orig = new RuntimeException(new Exception());
tryWrapAndThrow(orig);
throwIfSqlException(orig);
}
@Test
void tryWrapAndThrow_hasSQLException() {
Throwable orig = new Throwable(new SQLException());
assertThrows(DatabaseException.class, () -> tryWrapAndThrow(orig));
assertThrows(DatabaseException.class, () -> throwIfSqlException(orig));
}
@Test
core/src/test/java/google/registry/persistence/transaction/JpaTransactionManagerImplTest.java
+142 -164
View File
@@ -14,6 +14,7 @@
package google.registry.persistence.transaction;
import static com.google.common.base.Preconditions.checkState;
import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.common.truth.Truth.assertThat;
import static google.registry.persistence.PersistenceModule.TransactionIsolationLevel.TRANSACTION_READ_COMMITTED;
@@ -28,6 +29,7 @@ import static google.registry.testing.TestDataHelper.fileClassPath;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.doThrow;
import static org.mockito.Mockito.mockStatic;
import static org.mockito.Mockito.spy;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
@@ -36,6 +38,7 @@ import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import google.registry.config.RegistryConfig;
import google.registry.model.ImmutableObject;
import google.registry.persistence.PersistenceModule.TransactionIsolationLevel;
import google.registry.persistence.VKey;
import google.registry.persistence.transaction.JpaTestExtensions.JpaUnitTestExtension;
import google.registry.testing.DatabaseHelper;
@@ -43,7 +46,6 @@ import google.registry.testing.FakeClock;
import java.io.Serializable;
import java.math.BigInteger;
import java.util.NoSuchElementException;
import java.util.function.Supplier;
import javax.persistence.Entity;
import javax.persistence.EntityManager;
import javax.persistence.Id;
@@ -53,6 +55,8 @@ import javax.persistence.PersistenceException;
import javax.persistence.RollbackException;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.RegisterExtension;
import org.junit.jupiter.api.function.Executable;
import org.mockito.MockedStatic;
/**
* Unit tests for SQL only APIs defined in {@link JpaTransactionManagerImpl}. Note that the tests
@@ -94,7 +98,7 @@ class JpaTransactionManagerImplTest {
insertPerson(10);
insertCompany("Foo");
insertCompany("Bar");
tm().assertTransactionIsolationLevel(tm().getDefaultTransactionIsolationLevel());
assertTransactionIsolationLevel(tm().getDefaultTransactionIsolationLevel());
});
assertPersonCount(1);
assertPersonExist(10);
@@ -105,145 +109,98 @@ class JpaTransactionManagerImplTest {
@Test
void transact_setIsolationLevel() {
// If not specified, run at the default isolation level.
tm().transact(
() -> {
tm().assertTransactionIsolationLevel(
RegistryConfig.getHibernatePerTransactionIsolationEnabled()
? TRANSACTION_READ_UNCOMMITTED
: tm().getDefaultTransactionIsolationLevel());
return null;
},
() -> assertTransactionIsolationLevel(tm().getDefaultTransactionIsolationLevel()),
null);
tm().transact(
() -> assertTransactionIsolationLevel(TRANSACTION_READ_UNCOMMITTED),
TRANSACTION_READ_UNCOMMITTED);
// Make sure that we can start a new transaction on the same thread with a different isolation
// level.
// Make sure that we can start a new transaction on the same thread at a different level.
tm().transact(
() -> {
tm().assertTransactionIsolationLevel(
RegistryConfig.getHibernatePerTransactionIsolationEnabled()
? TRANSACTION_REPEATABLE_READ
: tm().getDefaultTransactionIsolationLevel());
return null;
},
() -> assertTransactionIsolationLevel(TRANSACTION_REPEATABLE_READ),
TRANSACTION_REPEATABLE_READ);
}
@Test
void transact_nestedTransactions_perTransactionIsolationLevelEnabled() {
if (!RegistryConfig.getHibernatePerTransactionIsolationEnabled()) {
return;
}
// Nested transactions allowed (both at the default isolation level).
tm().transact(
() -> {
tm().assertTransactionIsolationLevel(tm().getDefaultTransactionIsolationLevel());
tm().transact(
() -> {
tm().assertTransactionIsolationLevel(
void transact_nestedTransactions_disabled() {
try (MockedStatic<RegistryConfig> config = mockStatic(RegistryConfig.class)) {
config.when(RegistryConfig::getHibernateAllowNestedTransactions).thenReturn(false);
// transact() not allowed in nested transactions.
IllegalStateException thrown =
assertThrows(
IllegalStateException.class,
() ->
tm().transact(
() -> {
assertTransactionIsolationLevel(
tm().getDefaultTransactionIsolationLevel());
});
});
// Nested transactions allowed (enclosed transaction does not have an override, using the
// enclosing transaction's level).
tm().transact(
() -> {
tm().assertTransactionIsolationLevel(TRANSACTION_READ_UNCOMMITTED);
tm().transact(
() -> {
tm().assertTransactionIsolationLevel(TRANSACTION_READ_UNCOMMITTED);
});
},
TRANSACTION_READ_UNCOMMITTED);
// Nested transactions allowed (Both have the same override).
tm().transact(
() -> {
tm().assertTransactionIsolationLevel(TRANSACTION_REPEATABLE_READ);
tm().transact(
() -> {
tm().assertTransactionIsolationLevel(TRANSACTION_REPEATABLE_READ);
},
TRANSACTION_REPEATABLE_READ);
},
TRANSACTION_REPEATABLE_READ);
// Nested transactions disallowed (enclosed transaction has an override that conflicts from the
// default).
IllegalStateException e =
assertThrows(
IllegalStateException.class,
() ->
tm().transact(
() -> {
tm().transact(() -> {}, TRANSACTION_READ_COMMITTED);
}));
assertThat(e).hasMessageThat().contains("conflict detected");
// Nested transactions disallowed (conflicting overrides).
e =
assertThrows(
IllegalStateException.class,
() ->
tm().transact(
() -> {
tm().transact(() -> {}, TRANSACTION_READ_COMMITTED);
},
TRANSACTION_REPEATABLE_READ));
assertThat(e).hasMessageThat().contains("conflict detected");
tm().transact(() -> null);
}));
assertThat(thrown).hasMessageThat().contains("Nested transaction detected");
// reTransact() allowed in nested transactions.
tm().transact(
() -> {
assertTransactionIsolationLevel(tm().getDefaultTransactionIsolationLevel());
tm().reTransact(
() ->
assertTransactionIsolationLevel(
tm().getDefaultTransactionIsolationLevel()));
});
// reTransact() respects enclosing transaction's isolation level.
tm().transact(
() -> {
assertTransactionIsolationLevel(TRANSACTION_READ_UNCOMMITTED);
tm().reTransact(
() -> assertTransactionIsolationLevel(TRANSACTION_READ_UNCOMMITTED));
},
TRANSACTION_READ_UNCOMMITTED);
}
}
@Test
void transact_nestedTransactions_perTransactionIsolationLevelDisabled() {
if (RegistryConfig.getHibernatePerTransactionIsolationEnabled()) {
return;
void transact_nestedTransactions_enabled() {
try (MockedStatic<RegistryConfig> config = mockStatic(RegistryConfig.class)) {
config.when(RegistryConfig::getHibernateAllowNestedTransactions).thenReturn(true);
// transact() allowed in nested transactions.
tm().transact(
() -> {
assertTransactionIsolationLevel(tm().getDefaultTransactionIsolationLevel());
tm().reTransact(
() ->
assertTransactionIsolationLevel(
tm().getDefaultTransactionIsolationLevel()));
});
// transact() not allowed in nested transactions if isolation level is specified.
IllegalStateException thrown =
assertThrows(
IllegalStateException.class,
() ->
tm().transact(
() -> {
assertTransactionIsolationLevel(
tm().getDefaultTransactionIsolationLevel());
tm().transact(() -> null, TRANSACTION_READ_COMMITTED);
}));
assertThat(thrown).hasMessageThat().contains("cannot be specified");
// reTransact() allowed in nested transactions.
tm().transact(
() -> {
assertTransactionIsolationLevel(tm().getDefaultTransactionIsolationLevel());
tm().reTransact(
() ->
assertTransactionIsolationLevel(
tm().getDefaultTransactionIsolationLevel()));
});
// reTransact() respects enclosing transaction's isolation level.
tm().transact(
() -> {
assertTransactionIsolationLevel(TRANSACTION_READ_UNCOMMITTED);
tm().reTransact(
() -> assertTransactionIsolationLevel(TRANSACTION_READ_UNCOMMITTED));
},
TRANSACTION_READ_UNCOMMITTED);
}
tm().transact(
() -> {
tm().assertTransactionIsolationLevel(tm().getDefaultTransactionIsolationLevel());
tm().transact(
() -> {
tm().assertTransactionIsolationLevel(
tm().getDefaultTransactionIsolationLevel());
});
});
tm().transact(
() -> {
tm().assertTransactionIsolationLevel(tm().getDefaultTransactionIsolationLevel());
tm().transact(
() -> {
tm().assertTransactionIsolationLevel(
tm().getDefaultTransactionIsolationLevel());
});
},
TRANSACTION_READ_UNCOMMITTED);
tm().transact(
() -> {
tm().assertTransactionIsolationLevel(tm().getDefaultTransactionIsolationLevel());
tm().transact(
() -> {
tm().assertTransactionIsolationLevel(
tm().getDefaultTransactionIsolationLevel());
},
TRANSACTION_READ_UNCOMMITTED);
});
tm().transact(
() -> {
tm().assertTransactionIsolationLevel(tm().getDefaultTransactionIsolationLevel());
tm().transact(
() -> {
tm().assertTransactionIsolationLevel(
tm().getDefaultTransactionIsolationLevel());
},
TRANSACTION_READ_UNCOMMITTED);
},
TRANSACTION_READ_UNCOMMITTED);
tm().transact(
() -> {
tm().assertTransactionIsolationLevel(tm().getDefaultTransactionIsolationLevel());
tm().transact(
() -> {
tm().assertTransactionIsolationLevel(
tm().getDefaultTransactionIsolationLevel());
},
TRANSACTION_READ_COMMITTED);
},
TRANSACTION_READ_UNCOMMITTED);
}
@Test
@@ -299,32 +256,55 @@ class JpaTransactionManagerImplTest {
OptimisticLockException.class,
() -> spyJpaTm.transact(() -> spyJpaTm.delete(theEntityKey)));
verify(spyJpaTm, times(3)).delete(theEntityKey);
Supplier<Runnable> supplier =
() -> {
Runnable work = () -> spyJpaTm.delete(theEntityKey);
work.run();
return null;
};
assertThrows(OptimisticLockException.class, () -> spyJpaTm.transact(supplier));
assertThrows(
OptimisticLockException.class,
() -> spyJpaTm.transact(() -> spyJpaTm.delete(theEntityKey)));
verify(spyJpaTm, times(6)).delete(theEntityKey);
}
@Test
void transactNoRetry_doesNotRetryOptimisticLockException() {
JpaTransactionManager spyJpaTm = spy(tm());
doThrow(OptimisticLockException.class).when(spyJpaTm).delete(any(VKey.class));
spyJpaTm.transactNoRetry(() -> spyJpaTm.insert(theEntity));
assertThrows(
OptimisticLockException.class,
() -> spyJpaTm.transactNoRetry(() -> spyJpaTm.delete(theEntityKey)));
verify(spyJpaTm, times(1)).delete(theEntityKey);
Supplier<Runnable> supplier =
void transactNoRetry_nested() {
JpaTransactionManagerImpl tm = (JpaTransactionManagerImpl) tm();
// Calling transactNoRetry() without an isolation level override inside a transaction is fine.
tm.transact(
() -> {
Runnable work = () -> spyJpaTm.delete(theEntityKey);
work.run();
tm.transactNoRetry(
() -> {
assertTransactionIsolationLevel(tm.getDefaultTransactionIsolationLevel());
return null;
},
null);
});
// Calling transactNoRetry() with an isolation level override inside a transaction is not
// allowed.
IllegalStateException thrown =
assertThrows(
IllegalStateException.class,
() -> tm.transact(() -> tm.transactNoRetry(() -> null, TRANSACTION_READ_UNCOMMITTED)));
assertThat(thrown).hasMessageThat().contains("cannot be specified");
}
@Test
void transactNoRetry_doesNotRetryOptimisticLockException() {
JpaTransactionManagerImpl spyJpaTm = spy((JpaTransactionManagerImpl) tm());
doThrow(OptimisticLockException.class).when(spyJpaTm).delete(any(VKey.class));
spyJpaTm.transactNoRetry(
() -> {
spyJpaTm.insert(theEntity);
return null;
};
assertThrows(OptimisticLockException.class, () -> spyJpaTm.transactNoRetry(supplier));
},
null);
Executable transaction =
() ->
spyJpaTm.transactNoRetry(
() -> {
spyJpaTm.delete(theEntityKey);
return null;
},
null);
assertThrows(OptimisticLockException.class, transaction);
verify(spyJpaTm, times(1)).delete(theEntityKey);
assertThrows(OptimisticLockException.class, transaction);
verify(spyJpaTm, times(2)).delete(theEntityKey);
}
@@ -338,13 +318,8 @@ class JpaTransactionManagerImplTest {
assertThrows(
RuntimeException.class, () -> spyJpaTm.transact(() -> spyJpaTm.delete(theEntityKey)));
verify(spyJpaTm, times(3)).delete(theEntityKey);
Supplier<Runnable> supplier =
() -> {
Runnable work = () -> spyJpaTm.delete(theEntityKey);
work.run();
return null;
};
assertThrows(RuntimeException.class, () -> spyJpaTm.transact(supplier));
assertThrows(
RuntimeException.class, () -> spyJpaTm.transact(() -> spyJpaTm.delete(theEntityKey)));
verify(spyJpaTm, times(6)).delete(theEntityKey);
}
@@ -740,20 +715,13 @@ class JpaTransactionManagerImplTest {
doThrow(OptimisticLockException.class).when(spyJpaTm).delete(any(VKey.class));
spyJpaTm.transact(() -> spyJpaTm.insert(theEntity));
Supplier<Runnable> supplier =
() -> {
Runnable work = () -> spyJpaTm.delete(theEntityKey);
work.run();
return null;
};
assertThrows(
OptimisticLockException.class,
() ->
spyJpaTm.transact(
() -> {
spyJpaTm.exists(theEntity);
spyJpaTm.transact(supplier);
spyJpaTm.transact(() -> spyJpaTm.delete(theEntityKey));
}));
verify(spyJpaTm, times(3)).exists(theEntity);
@@ -814,6 +782,16 @@ class JpaTransactionManagerImplTest {
assertCompanyCount(0);
}
private static void assertTransactionIsolationLevel(TransactionIsolationLevel expectedLevel) {
tm().assertInTransaction();
TransactionIsolationLevel currentLevel = tm().getCurrentTransactionIsolationLevel();
checkState(
currentLevel == expectedLevel,
"Current transaction isolation level (%s) is not as expected (%s)",
currentLevel,
expectedLevel);
}
private static int countTable(String tableName) {
return tm().transact(
() -> {
core/src/test/java/google/registry/persistence/transaction/ReplicaSimulatingJpaTransactionManager.java
+19 -35
View File
@@ -14,6 +14,9 @@
package google.registry.persistence.transaction;
import static com.google.common.base.Throwables.throwIfUnchecked;
import static google.registry.persistence.transaction.DatabaseException.throwIfSqlException;
import com.google.common.collect.ImmutableCollection;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
@@ -21,7 +24,7 @@ import google.registry.model.ImmutableObject;
import google.registry.persistence.PersistenceModule.TransactionIsolationLevel;
import google.registry.persistence.VKey;
import java.util.Optional;
import java.util.function.Supplier;
import java.util.concurrent.Callable;
import java.util.stream.Stream;
import javax.persistence.EntityManager;
import javax.persistence.Query;
@@ -60,11 +63,6 @@ public class ReplicaSimulatingJpaTransactionManager implements JpaTransactionMan
return delegate.getCurrentTransactionIsolationLevel();
}
@Override
public void assertTransactionIsolationLevel(TransactionIsolationLevel expectedLevel) {
delegate.assertTransactionIsolationLevel(expectedLevel);
}
@Override
public EntityManager getStandaloneEntityManager() {
return delegate.getStandaloneEntityManager();
@@ -101,9 +99,15 @@ public class ReplicaSimulatingJpaTransactionManager implements JpaTransactionMan
}
@Override
public <T> T transact(Supplier<T> work, TransactionIsolationLevel isolationLevel) {
if (delegate.inTransaction()) {
return work.get();
public <T> T transact(Callable<T> work, TransactionIsolationLevel isolationLevel) {
if (inTransaction()) {
try {
return work.call();
} catch (Exception e) {
throwIfSqlException(e);
throwIfUnchecked(e);
throw new RuntimeException(e);
}
}
return delegate.transact(
() -> {
@@ -111,33 +115,23 @@ public class ReplicaSimulatingJpaTransactionManager implements JpaTransactionMan
.getEntityManager()
.createNativeQuery("SET TRANSACTION READ ONLY")
.executeUpdate();
return work.get();
return work.call();
},
isolationLevel);
}
@Override
public <T> T reTransact(Supplier<T> work) {
public <T> T reTransact(Callable<T> work) {
return transact(work);
}
@Override
public <T> T transact(Supplier<T> work) {
public <T> T transact(Callable<T> work) {
return transact(work, null);
}
@Override
public <T> T transactNoRetry(Supplier<T> work, TransactionIsolationLevel isolationLevel) {
return transact(work, isolationLevel);
}
@Override
public <T> T transactNoRetry(Supplier<T> work) {
return transactNoRetry(work, null);
}
@Override
public void transact(Runnable work, TransactionIsolationLevel isolationLevel) {
public void transact(ThrowingRunnable work, TransactionIsolationLevel isolationLevel) {
transact(
() -> {
work.run();
@@ -147,25 +141,15 @@ public class ReplicaSimulatingJpaTransactionManager implements JpaTransactionMan
}
@Override
public void reTransact(Runnable work) {
public void reTransact(ThrowingRunnable work) {
transact(work);
}
@Override
public void transact(Runnable work) {
public void transact(ThrowingRunnable work) {
transact(work, null);
}
@Override
public void transactNoRetry(Runnable work, TransactionIsolationLevel isolationLevel) {
transact(work, isolationLevel);
}
@Override
public void transactNoRetry(Runnable work) {
transactNoRetry(work, null);
}
@Override
public DateTime getTransactionTime() {
return delegate.getTransactionTime();