From ef1a1f9e11ed5ff24bc6c6fba1b4f1e1e803eaba Mon Sep 17 00:00:00 2001
From: gbrodman <gbrodman@google.com>
Date: Sat, 14 Aug 2021 12:34:23 -0600
Subject: [PATCH] Remove recursive load in DBMSS cache (#1286)

* Remove recursive load in DBMSS cache

This occurs because if we do a standard transaction, the JpaTxnManager
checks to see if we should be doing backups, which involves loading the
migration state schedule (causing the recursion). When starting the
transaction to load the schedule, we should explicitly
transactWithoutBackup so there's no need to check.

This wasn't hit in tests because we previously manually set the
replication to not occur in the JpaTransactionManagerExtension -- we
remove that and related setters.
---
 .../registry/model/common/DatabaseMigrationStateSchedule.java   | 2 +-
 .../registry/model/replay/ReplicateToDatastoreActionTest.java   | 2 --
 .../persistence/transaction/JpaTransactionManagerExtension.java | 2 +-
 .../registry/persistence/transaction/TransactionTest.java       | 1 -
 core/src/test/java/google/registry/testing/ReplayExtension.java | 2 +-
 5 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/core/src/main/java/google/registry/model/common/DatabaseMigrationStateSchedule.java b/core/src/main/java/google/registry/model/common/DatabaseMigrationStateSchedule.java
index 513808691..3cb240657 100644
--- a/core/src/main/java/google/registry/model/common/DatabaseMigrationStateSchedule.java
+++ b/core/src/main/java/google/registry/model/common/DatabaseMigrationStateSchedule.java
@@ -222,7 +222,7 @@ public class DatabaseMigrationStateSchedule extends CrossTldSingleton implements
   @VisibleForTesting
   static TimedTransitionProperty<MigrationState, MigrationStateTransition> getUncached() {
     return jpaTm()
-        .transactNew(
+        .transactWithoutBackup(
             () -> {
               try {
                 return jpaTm()
diff --git a/core/src/test/java/google/registry/model/replay/ReplicateToDatastoreActionTest.java b/core/src/test/java/google/registry/model/replay/ReplicateToDatastoreActionTest.java
index 566e86af9..f73239fc1 100644
--- a/core/src/test/java/google/registry/model/replay/ReplicateToDatastoreActionTest.java
+++ b/core/src/test/java/google/registry/model/replay/ReplicateToDatastoreActionTest.java
@@ -32,7 +32,6 @@ import google.registry.model.common.DatabaseMigrationStateSchedule.MigrationStat
 import google.registry.model.ofy.CommitLogBucket;
 import google.registry.model.ofy.Ofy;
 import google.registry.persistence.VKey;
-import google.registry.persistence.transaction.JpaTransactionManagerImpl;
 import google.registry.persistence.transaction.TransactionEntity;
 import google.registry.testing.AppEngineExtension;
 import google.registry.testing.DatabaseHelper;
@@ -69,7 +68,6 @@ public class ReplicateToDatastoreActionTest {
 
   @BeforeEach
   public void setUp() {
-    JpaTransactionManagerImpl.removeReplaySqlToDsOverrideForTest();
     injectExtension.setStaticField(Ofy.class, "clock", fakeClock);
     // Use a single bucket to expose timestamp inversion problems.
     injectExtension.setStaticField(
diff --git a/core/src/test/java/google/registry/persistence/transaction/JpaTransactionManagerExtension.java b/core/src/test/java/google/registry/persistence/transaction/JpaTransactionManagerExtension.java
index 5c621a5ae..2c58964a8 100644
--- a/core/src/test/java/google/registry/persistence/transaction/JpaTransactionManagerExtension.java
+++ b/core/src/test/java/google/registry/persistence/transaction/JpaTransactionManagerExtension.java
@@ -203,12 +203,12 @@ abstract class JpaTransactionManagerExtension implements BeforeEachCallback, Aft
     JpaTransactionManagerImpl txnManager = new JpaTransactionManagerImpl(emf, clock);
     cachedTm = TransactionManagerFactory.jpaTm();
     TransactionManagerFactory.setJpaTm(Suppliers.ofInstance(txnManager));
-    JpaTransactionManagerImpl.setReplaySqlToDatastoreOverrideForTest(false);
   }
 
   @Override
   public void afterEach(ExtensionContext context) {
     TransactionManagerFactory.setJpaTm(Suppliers.ofInstance(cachedTm));
+    // Even though we didn't set this, reset it to make sure no other tests are affected
     JpaTransactionManagerImpl.removeReplaySqlToDsOverrideForTest();
     cachedTm = null;
   }
diff --git a/core/src/test/java/google/registry/persistence/transaction/TransactionTest.java b/core/src/test/java/google/registry/persistence/transaction/TransactionTest.java
index c1215c7a2..d87594cba 100644
--- a/core/src/test/java/google/registry/persistence/transaction/TransactionTest.java
+++ b/core/src/test/java/google/registry/persistence/transaction/TransactionTest.java
@@ -58,7 +58,6 @@ class TransactionTest {
 
   @BeforeEach
   void beforeEach() {
-    JpaTransactionManagerImpl.removeReplaySqlToDsOverrideForTest();
     inject.setStaticField(Ofy.class, "clock", fakeClock);
     fooEntity = new TestEntity("foo");
     barEntity = new TestEntity("bar");
diff --git a/core/src/test/java/google/registry/testing/ReplayExtension.java b/core/src/test/java/google/registry/testing/ReplayExtension.java
index 3aab237d0..3ae42ca4c 100644
--- a/core/src/test/java/google/registry/testing/ReplayExtension.java
+++ b/core/src/test/java/google/registry/testing/ReplayExtension.java
@@ -105,7 +105,7 @@ public class ReplayExtension implements BeforeEachCallback, AfterEachCallback {
     replay();
     injectExtension.afterEach(context);
     if (sqlToDsReplicator != null) {
-      JpaTransactionManagerImpl.setReplaySqlToDatastoreOverrideForTest(false);
+      JpaTransactionManagerImpl.removeReplaySqlToDsOverrideForTest();
     }
   }