Always use Cloud SQL as primary in SignedMarkRevocationListDao (#1061)

* Modify ClaimsList DAO to always use Cloud SQL as primary

* Revert ClaimsList add changes to SignedMarkRevocationList

* Fix flow tests

* Use start of time for empty list

* replace lambda with method reference

core/src/main/java/google/registry/model/smd/SignedMarkRevocationListDao.java

@@ -19,7 +19,6 @@ import static com.google.common.base.Preconditions.checkState;
 import static com.google.common.collect.ImmutableList.toImmutableList;
 import static com.google.common.collect.Iterables.isEmpty;
 import static google.registry.model.DatabaseMigrationUtils.suppressExceptionUnlessInTest;
-import static google.registry.model.common.DatabaseTransitionSchedule.PrimaryDatabase.DATASTORE;
 import static google.registry.model.common.EntityGroupRoot.getCrossTldKey;
 import static google.registry.model.ofy.ObjectifyService.allocateId;
 import static google.registry.model.ofy.ObjectifyService.ofy;
@@ -35,9 +34,6 @@ import com.google.common.collect.Iterables;
 import com.google.common.collect.MapDifference;
 import com.google.common.collect.Maps;
 import com.google.common.flogger.FluentLogger;
-import google.registry.model.DatabaseMigrationUtils;
-import google.registry.model.common.DatabaseTransitionSchedule.PrimaryDatabase;
-import google.registry.model.common.DatabaseTransitionSchedule.TransitionId;
 import google.registry.util.CollectionUtils;
 import java.util.Map;
 import java.util.Optional;
@@ -50,42 +46,24 @@ public class SignedMarkRevocationListDao {
   /**
    * Loads the {@link SignedMarkRevocationList}.
    *
-   * <p>Loads the list from the specified primary database, and attempts to load from the secondary
+   * <p>Loads the list from Cloud SQL, and attempts to load from Datastore. If the load from
-   * database. If the load the secondary database fails, or the list from the secondary database
+   * Datastore fails, or the list from Datastore does not match the list from Cloud SQL, the error
-   * does not match the list from the primary database, the error will be logged but no exception
+   * will be logged but no exception will be thrown.
-   * will be thrown.
    */
   static SignedMarkRevocationList load() {
-    PrimaryDatabase primaryDatabase =
+    Optional<SignedMarkRevocationList> primaryList = loadFromCloudSql();
-        tm().transactNew(
-                () ->
-                    DatabaseMigrationUtils.getPrimaryDatabase(
-                        TransitionId.SIGNED_MARK_REVOCATION_LIST));
-    Optional<SignedMarkRevocationList> primaryList =
-        primaryDatabase.equals(DATASTORE) ? loadFromDatastore() : loadFromCloudSql();
     if (!primaryList.isPresent()) {
-      throw new IllegalStateException(
+      return SignedMarkRevocationList.create(START_OF_TIME, ImmutableMap.of());
-          String.format(
-              "SignedMarkRevocationList not found in the primary database (%s).",
-              primaryDatabase.name()));
     }
     suppressExceptionUnlessInTest(
-        () -> loadAndCompare(primaryDatabase, primaryList.get()),
+        () -> loadAndCompare(primaryList.get()),
-        String.format(
+        "Error loading and comparing the SignedMarkRevocationList from Datastore");
-            "Error loading and comparing the SignedMarkRevocationList from the secondary database"
-                + " (%s).",
-            primaryDatabase.equals(DATASTORE) ? "Cloud SQL" : "Datastore"));
     return primaryList.get();
   }
 
-  /**
+  /** Loads the list from Datastore and compares it to the list from Cloud SQL. */
-   * Loads the list from the secondary database and compares it to the list from the primary
+  private static void loadAndCompare(SignedMarkRevocationList primaryList) {
-   * database.
+    Optional<SignedMarkRevocationList> secondaryList = loadFromDatastore();
-   */
-  private static void loadAndCompare(
-      PrimaryDatabase primaryDatabase, SignedMarkRevocationList primaryList) {
-    Optional<SignedMarkRevocationList> secondaryList =
-        primaryDatabase.equals(DATASTORE) ? loadFromCloudSql() : loadFromDatastore();
     if (secondaryList.isPresent() && !isNullOrEmpty(secondaryList.get().revokes)) {
       MapDifference<String, DateTime> diff =
           Maps.difference(primaryList.revokes, secondaryList.get().revokes);
@@ -93,11 +71,9 @@ public class SignedMarkRevocationListDao {
         if (diff.entriesDiffering().size() > 10) {
           String message =
               String.format(
-                  "Unequal SignedMarkRevocationList detected, %s list with revision id"
+                  "Unequal SignedMarkRevocationList detected, Datastore list with revision id"
-                      + " %d has %d different records than the current primary database list.",
+                      + " %d has %d different records than the current Cloud SQL list.",
-                  primaryDatabase.equals(DATASTORE) ? "Cloud SQL" : "Datastore",
+                  secondaryList.get().revisionId, diff.entriesDiffering().size());
-                  secondaryList.get().revisionId,
-                  diff.entriesDiffering().size());
           throw new IllegalStateException(message);
         } else {
           StringBuilder diffMessage =
@@ -107,21 +83,15 @@ public class SignedMarkRevocationListDao {
                   (label, valueDiff) ->
                       diffMessage.append(
                           String.format(
-                              "SMD %s has key %s in %s and key %s in secondary database.\n",
+                              "SMD %s has key %s in Cloud SQL and key %s in Datastore.\n",
-                              label,
+                              label, valueDiff.leftValue(), valueDiff.rightValue())));
-                              valueDiff.leftValue(),
-                              primaryDatabase.name(),
-                              valueDiff.rightValue())));
           throw new IllegalStateException(diffMessage.toString());
         }
       }
     } else {
       if (primaryList.size() != 0) {
         throw new IllegalStateException(
-            String.format(
+            "SignedMarkRevocationList in Datastore is empty while it is not empty in Cloud SQL.");
-                "SignedMarkRevocationList in %s is empty while it is not empty in the primary"
-                    + " database.",
-                primaryDatabase.equals(DATASTORE) ? "Cloud SQL" : "Datastore"));
       }
     }
   }
@@ -172,29 +142,16 @@ public class SignedMarkRevocationListDao {
   /**
    * Save the given {@link SignedMarkRevocationList}
    *
-   * <p>Saves the list to the specified primary database, and attempts to save to the secondary
+   * <p>Saves the list to Cloud SQL, and attempts to save to Datastore. If the save to Datastore
-   * database. If the save to the secondary database fails, the error will be logged but no
+   * fails, the error will be logged but no exception will be thrown.
-   * exception will be thrown.
    */
   static void save(SignedMarkRevocationList signedMarkRevocationList) {
-    PrimaryDatabase primaryDatabase =
-        tm().transactNew(
-                () ->
-                    DatabaseMigrationUtils.getPrimaryDatabase(
-                        TransitionId.SIGNED_MARK_REVOCATION_LIST));
-    if (primaryDatabase.equals(DATASTORE)) {
-      saveToDatastore(signedMarkRevocationList.revokes, signedMarkRevocationList.creationTime);
-      suppressExceptionUnlessInTest(
-          () -> SignedMarkRevocationListDao.saveToCloudSql(signedMarkRevocationList),
-          "Error inserting signed mark revocations into secondary database (Cloud SQL).");
-    } else {
       SignedMarkRevocationListDao.saveToCloudSql(signedMarkRevocationList);
       suppressExceptionUnlessInTest(
           () ->
               saveToDatastore(
                   signedMarkRevocationList.revokes, signedMarkRevocationList.creationTime),
           "Error inserting signed mark revocations into secondary database (Datastore).");
-    }
   }
 
   private static void saveToCloudSql(SignedMarkRevocationList signedMarkRevocationList) {

core/src/test/java/google/registry/model/smd/SignedMarkRevocationListDaoTest.java

@@ -16,21 +16,12 @@ package google.registry.model.smd;
 
 import static com.google.common.truth.Truth.assertThat;
 import static google.registry.model.ImmutableObjectSubject.assertAboutImmutableObjects;
-import static google.registry.model.ofy.ObjectifyService.ofy;
 import static google.registry.persistence.transaction.TransactionManagerFactory.jpaTm;
-import static google.registry.persistence.transaction.TransactionManagerFactory.tm;
-import static google.registry.util.DateTimeUtils.START_OF_TIME;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 
 import com.google.common.collect.ImmutableMap;
-import com.google.common.collect.ImmutableSortedMap;
 import google.registry.config.RegistryEnvironment;
 import google.registry.model.EntityTestCase;
-import google.registry.model.common.DatabaseTransitionSchedule;
-import google.registry.model.common.DatabaseTransitionSchedule.PrimaryDatabase;
-import google.registry.model.common.DatabaseTransitionSchedule.PrimaryDatabaseTransition;
-import google.registry.model.common.DatabaseTransitionSchedule.TransitionId;
-import google.registry.model.common.TimedTransitionProperty;
 import google.registry.persistence.transaction.JpaTestRules;
 import google.registry.persistence.transaction.JpaTestRules.JpaIntegrationWithCoverageExtension;
 import google.registry.testing.DatastoreEntityExtension;
@@ -38,8 +29,6 @@ import google.registry.testing.DualDatabaseTest;
 import google.registry.testing.SystemPropertyExtension;
 import google.registry.testing.TestOfyAndSql;
 import org.joda.time.DateTime;
-import org.joda.time.Duration;
-import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Order;
 import org.junit.jupiter.api.extension.RegisterExtension;
 
@@ -58,36 +47,8 @@ public class SignedMarkRevocationListDaoTest extends EntityTestCase {
   @Order(value = Integer.MAX_VALUE)
   final SystemPropertyExtension systemPropertyExtension = new SystemPropertyExtension();
 
-  @BeforeEach
-  void setup() {
-    fakeClock.setTo(DateTime.parse("1984-12-21T00:00:00.000Z"));
-    DatabaseTransitionSchedule schedule =
-        DatabaseTransitionSchedule.create(
-            TransitionId.SIGNED_MARK_REVOCATION_LIST,
-            TimedTransitionProperty.fromValueMap(
-                ImmutableSortedMap.of(
-                    START_OF_TIME,
-                    PrimaryDatabase.DATASTORE,
-                    fakeClock.nowUtc().plusDays(1),
-                    PrimaryDatabase.CLOUD_SQL),
-                PrimaryDatabaseTransition.class));
-
-    tm().transactNew(() -> ofy().saveWithoutBackup().entity(schedule).now());
-  }
-
-  @TestOfyAndSql
-  void testSave_datastorePrimary_success() {
-    SignedMarkRevocationList list =
-        SignedMarkRevocationList.create(
-            fakeClock.nowUtc(), ImmutableMap.of("mark", fakeClock.nowUtc().minusHours(1)));
-    SignedMarkRevocationListDao.save(list);
-    SignedMarkRevocationList fromDb = SignedMarkRevocationListDao.load();
-    assertAboutImmutableObjects().that(fromDb).isEqualExceptFields(list, "revisionId");
-  }
-
   @TestOfyAndSql
   void testSave_cloudSqlPrimary_success() {
-    fakeClock.advanceBy(Duration.standardDays(5));
     SignedMarkRevocationList list =
         SignedMarkRevocationList.create(
             fakeClock.nowUtc(), ImmutableMap.of("mark", fakeClock.nowUtc().minusHours(1)));
@@ -96,18 +57,8 @@ public class SignedMarkRevocationListDaoTest extends EntityTestCase {
     assertAboutImmutableObjects().that(fromDb).isEqualExceptFields(list);
   }
 
-  @TestOfyAndSql
-  void testSaveAndLoad_datastorePrimary_emptyList() {
-    SignedMarkRevocationList list =
-        SignedMarkRevocationList.create(START_OF_TIME, ImmutableMap.of());
-    SignedMarkRevocationListDao.save(list);
-    SignedMarkRevocationList fromDb = SignedMarkRevocationListDao.load();
-    assertAboutImmutableObjects().that(fromDb).isEqualExceptFields(list, "revisionId");
-  }
-
   @TestOfyAndSql
   void testSaveAndLoad_cloudSqlPrimary_emptyList() {
-    fakeClock.advanceBy(Duration.standardDays(5));
     SignedMarkRevocationList list =
         SignedMarkRevocationList.create(fakeClock.nowUtc(), ImmutableMap.of());
     SignedMarkRevocationListDao.save(list);
@@ -115,26 +66,8 @@ public class SignedMarkRevocationListDaoTest extends EntityTestCase {
     assertAboutImmutableObjects().that(fromDb).isEqualExceptFields(list, "revisionId");
   }
 
-  @TestOfyAndSql
-  void testSave_datastorePrimary_multipleVersions() {
-    SignedMarkRevocationList list =
-        SignedMarkRevocationList.create(
-            fakeClock.nowUtc(), ImmutableMap.of("mark", fakeClock.nowUtc().minusHours(1)));
-    SignedMarkRevocationListDao.save(list);
-    assertThat(SignedMarkRevocationListDao.load().isSmdRevoked("mark", fakeClock.nowUtc()))
-        .isTrue();
-
-    // Now remove the revocation
-    SignedMarkRevocationList secondList =
-        SignedMarkRevocationList.create(fakeClock.nowUtc(), ImmutableMap.of());
-    SignedMarkRevocationListDao.save(secondList);
-    assertThat(SignedMarkRevocationListDao.load().isSmdRevoked("mark", fakeClock.nowUtc()))
-        .isFalse();
-  }
-
   @TestOfyAndSql
   void testSave_cloudSqlPrimary_multipleVersions() {
-    fakeClock.advanceBy(Duration.standardDays(5));
     SignedMarkRevocationList list =
         SignedMarkRevocationList.create(
             fakeClock.nowUtc(), ImmutableMap.of("mark", fakeClock.nowUtc().minusHours(1)));
@@ -150,28 +83,9 @@ public class SignedMarkRevocationListDaoTest extends EntityTestCase {
         .isFalse();
   }
 
-  @TestOfyAndSql
-  void testLoad_datastorePrimary_unequalLists() {
-    SignedMarkRevocationList list =
-        SignedMarkRevocationList.create(
-            fakeClock.nowUtc(), ImmutableMap.of("mark", fakeClock.nowUtc().minusHours(1)));
-    SignedMarkRevocationListDao.save(list);
-    SignedMarkRevocationList list2 =
-        SignedMarkRevocationList.create(
-            fakeClock.nowUtc(), ImmutableMap.of("mark", fakeClock.nowUtc().minusHours(3)));
-    jpaTm().transact(() -> jpaTm().put(list2));
-    RuntimeException thrown =
-        assertThrows(RuntimeException.class, SignedMarkRevocationListDao::load);
-    assertThat(thrown)
-        .hasMessageThat()
-        .contains(
-            "SMD mark has key 1984-12-20T23:00:00.000Z in DATASTORE and key"
-                + " 1984-12-20T21:00:00.000Z in secondary database.");
-  }
-
   @TestOfyAndSql
   void testLoad_cloudSqlPrimary_unequalLists() {
-    fakeClock.advanceBy(Duration.standardDays(5));
+    fakeClock.setTo(DateTime.parse("1984-12-26T00:00:00.000Z"));
     SignedMarkRevocationList list =
         SignedMarkRevocationList.create(
             fakeClock.nowUtc(), ImmutableMap.of("mark", fakeClock.nowUtc().minusHours(1)));
@@ -185,14 +99,13 @@ public class SignedMarkRevocationListDaoTest extends EntityTestCase {
     assertThat(thrown)
         .hasMessageThat()
         .contains(
-            "SMD mark has key 1984-12-25T21:00:00.000Z in CLOUD_SQL and key"
+            "SMD mark has key 1984-12-25T21:00:00.000Z in Cloud SQL and key"
-                + " 1984-12-25T23:00:00.000Z in secondary database.");
+                + " 1984-12-25T23:00:00.000Z in Datastore.");
   }
 
   @TestOfyAndSql
   void testLoad_cloudSqlPrimary_unequalLists_succeedsInProduction() {
     RegistryEnvironment.PRODUCTION.setup(systemPropertyExtension);
-    fakeClock.advanceBy(Duration.standardDays(5));
     SignedMarkRevocationList list =
         SignedMarkRevocationList.create(
             fakeClock.nowUtc(), ImmutableMap.of("mark", fakeClock.nowUtc().minusHours(1)));
@@ -205,25 +118,8 @@ public class SignedMarkRevocationListDaoTest extends EntityTestCase {
     assertAboutImmutableObjects().that(fromDb).isEqualExceptFields(list2, "revisionId");
   }
 
-  @TestOfyAndSql
-  void testLoad_datastorePrimary_noListInCloudSql() {
-    SignedMarkRevocationList list =
-        SignedMarkRevocationList.create(
-            fakeClock.nowUtc(), ImmutableMap.of("mark", fakeClock.nowUtc().minusHours(1)));
-    SignedMarkRevocationListDao.save(list);
-    jpaTm().transact(() -> jpaTm().delete(list));
-    RuntimeException thrown =
-        assertThrows(RuntimeException.class, SignedMarkRevocationListDao::load);
-    assertThat(thrown)
-        .hasMessageThat()
-        .contains(
-            "SignedMarkRevocationList in Cloud SQL is empty while it is not empty in the"
-                + " primary database.");
-  }
-
   @TestOfyAndSql
   void testLoad_cloudSqlPrimary_noListInDatastore() {
-    fakeClock.advanceBy(Duration.standardDays(5));
     SignedMarkRevocationList list =
         SignedMarkRevocationList.create(
             fakeClock.nowUtc(), ImmutableMap.of("mark", fakeClock.nowUtc().minusHours(1)));
@@ -233,7 +129,6 @@ public class SignedMarkRevocationListDaoTest extends EntityTestCase {
     assertThat(thrown)
         .hasMessageThat()
         .contains(
-            "SignedMarkRevocationList in Datastore is empty while it is not empty in the"
+            "SignedMarkRevocationList in Datastore is empty while it is not empty in Cloud SQL.");
-                + " primary database.");
   }
 }