Always use Cloud SQL as primary for ClaimsList (#1127)

* Always use Cloud SQL as primary for ClaimsList

* Add a test back

core/src/main/java/google/registry/model/tmch/ClaimsListDualDatabaseDao.java

@@ -16,9 +16,7 @@ package google.registry.model.tmch;
 
 import static google.registry.config.RegistryConfig.getDomainLabelListCacheDuration;
 import static google.registry.model.CacheUtils.tryMemoizeWithExpiration;
-import static google.registry.model.DatabaseMigrationUtils.isDatastore;
 import static google.registry.model.DatabaseMigrationUtils.suppressExceptionUnlessInTest;
-import static google.registry.model.common.DatabaseTransitionSchedule.TransitionId.CLAIMS_LIST;
 import static google.registry.util.DateTimeUtils.START_OF_TIME;
 
 import com.google.common.base.Supplier;
@@ -31,12 +29,11 @@ import java.util.Optional;
 /**
  * DAO for {@link ClaimsListShard} objects that handles the branching paths for SQL and Datastore.
  *
- * <p>For write actions, this class will perform the action against the primary database then, after
+ * <p>For write actions, this class will perform the action against Cloud SQL then, after that
- * * that success or failure, against the secondary database. If the secondary database fails, an
+ * success or failure, against Datastore. If Datastore fails, an error is logged (but not thrown).
- * error is logged (but not thrown).
  *
- * <p>For read actions, we will log if the primary and secondary databases * have different values
+ * <p>For read actions, we will log if the two databases have different values (or if the retrieval
- * (or if the retrieval from the second database fails).
+ * from Datastore fails).
  */
 public class ClaimsListDualDatabaseDao {
 
@@ -48,19 +45,13 @@ public class ClaimsListDualDatabaseDao {
 
   /**
    * Saves the given {@link ClaimsListShard} to both the primary and secondary databases, logging
-   * and skipping errors in the secondary DB.
+   * and skipping errors in Datastore.
    */
   public static void save(ClaimsListShard claimsList) {
-    if (isDatastore(CLAIMS_LIST)) {
-      claimsList.saveToDatastore();
-      suppressExceptionUnlessInTest(
-          () -> ClaimsListSqlDao.save(claimsList), "Error saving ClaimsList to SQL.");
-    } else {
       ClaimsListSqlDao.save(claimsList);
       suppressExceptionUnlessInTest(
           claimsList::saveToDatastore, "Error saving ClaimsListShard to Datastore.");
   }
-  }
 
   /** Returns the most recent revision of the {@link ClaimsListShard}, from cache. */
   public static ClaimsListShard get() {
@@ -69,42 +60,31 @@ public class ClaimsListDualDatabaseDao {
 
   /** Retrieves and compares the latest revision from the databases. */
   private static ClaimsListShard getUncached() {
-    Optional<ClaimsListShard> primaryResult;
+    Optional<ClaimsListShard> cloudSqlResult = ClaimsListSqlDao.get();
-    if (isDatastore(CLAIMS_LIST)) {
-      primaryResult = ClaimsListShard.getFromDatastore();
     suppressExceptionUnlessInTest(
         () -> {
-            Optional<ClaimsListShard> secondaryResult = ClaimsListSqlDao.get();
+          Optional<ClaimsListShard> datastoreResult = ClaimsListShard.getFromDatastore();
-            compareClaimsLists(primaryResult, secondaryResult);
+          compareClaimsLists(cloudSqlResult, datastoreResult);
-          },
-          "Error loading ClaimsList from SQL.");
-    } else {
-      primaryResult = ClaimsListSqlDao.get();
-      suppressExceptionUnlessInTest(
-          () -> {
-            Optional<ClaimsListShard> secondaryResult = ClaimsListShard.getFromDatastore();
-            compareClaimsLists(primaryResult, secondaryResult);
         },
         "Error loading ClaimsListShard from Datastore.");
-    }
+    return cloudSqlResult.orElse(ClaimsListShard.create(START_OF_TIME, ImmutableMap.of()));
-    return primaryResult.orElse(ClaimsListShard.create(START_OF_TIME, ImmutableMap.of()));
   }
 
   private static void compareClaimsLists(
-      Optional<ClaimsListShard> maybePrimary, Optional<ClaimsListShard> maybeSecondary) {
+      Optional<ClaimsListShard> maybeCloudSql, Optional<ClaimsListShard> maybeDatastore) {
-    if (maybePrimary.isPresent() && !maybeSecondary.isPresent()) {
+    if (maybeCloudSql.isPresent() && !maybeDatastore.isPresent()) {
-      throw new IllegalStateException("Claims list found in primary DB but not in secondary DB.");
+      throw new IllegalStateException("Claims list found in Cloud SQL but not in Datastore.");
     }
-    if (!maybePrimary.isPresent() && maybeSecondary.isPresent()) {
+    if (!maybeCloudSql.isPresent() && maybeDatastore.isPresent()) {
-      throw new IllegalStateException("Claims list found in secondary DB but not in primary DB.");
+      throw new IllegalStateException("Claims list found in Datastore but not in Cloud SQL.");
     }
-    if (!maybePrimary.isPresent()) {
+    if (!maybeCloudSql.isPresent()) {
       return;
     }
-    ClaimsListShard primary = maybePrimary.get();
+    ClaimsListShard sqlList = maybeCloudSql.get();
-    ClaimsListShard secondary = maybeSecondary.get();
+    ClaimsListShard datastoreList = maybeDatastore.get();
     MapDifference<String, String> diff =
-        Maps.difference(primary.labelsToKeys, secondary.getLabelsToKeys());
+        Maps.difference(sqlList.labelsToKeys, datastoreList.getLabelsToKeys());
     if (!diff.areEqual()) {
       if (diff.entriesDiffering().size()
               + diff.entriesOnlyOnRight().size()
@@ -112,9 +92,9 @@ public class ClaimsListDualDatabaseDao {
           > 10) {
         throw new IllegalStateException(
             String.format(
-                "Unequal claims lists detected, secondary list with revision id %d has %d"
+                "Unequal claims lists detected, Datastore list with revision id %d has %d"
-                    + " different records than the current primary list.",
+                    + " different records than the current Cloud SQL list.",
-                secondary.getRevisionId(), diff.entriesDiffering().size()));
+                datastoreList.getRevisionId(), diff.entriesDiffering().size()));
       } else {
         StringBuilder diffMessage = new StringBuilder("Unequal claims lists detected:\n");
         diff.entriesDiffering()
@@ -122,22 +102,22 @@ public class ClaimsListDualDatabaseDao {
                 (label, valueDiff) ->
                     diffMessage.append(
                         String.format(
-                            "Domain label %s has key %s in the primary DB and key %s "
+                            "Domain label %s has key %s in Cloud SQL and key %s "
-                                + "in the secondary DB.\n",
+                                + "in Datastore.\n",
                             label, valueDiff.leftValue(), valueDiff.rightValue())));
         diff.entriesOnlyOnLeft()
             .forEach(
                 (label, valueDiff) ->
                     diffMessage.append(
                         String.format(
-                            "Domain label %s with key %s only appears in the primary DB.\n",
+                            "Domain label %s with key %s only appears in Cloud SQL.\n",
                             label, valueDiff)));
         diff.entriesOnlyOnRight()
             .forEach(
                 (label, valueDiff) ->
                     diffMessage.append(
                         String.format(
-                            "Domain label %s with key %s only appears in the secondary DB.\n",
+                            "Domain label %s with key %s only appears in Datastore.\n",
                             label, valueDiff)));
         throw new IllegalStateException(diffMessage.toString());
       }

core/src/test/java/google/registry/model/tmch/ClaimsListDualDatabaseDaoTest.java

@@ -16,23 +16,14 @@ package google.registry.model.tmch;
 
 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.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.testing.SystemPropertyExtension;
 import org.joda.time.Duration;
-import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Order;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.RegisterExtension;
@@ -44,64 +35,25 @@ public class ClaimsListDualDatabaseDaoTest extends EntityTestCase {
   @Order(value = Integer.MAX_VALUE)
   final SystemPropertyExtension systemPropertyExtension = new SystemPropertyExtension();
 
-  @BeforeEach
-  void beforeEach() {
-    DatabaseTransitionSchedule schedule =
-        DatabaseTransitionSchedule.create(
-            TransitionId.CLAIMS_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());
-  }
-
-  @Test
-  void testGetList_missingSql() {
-    createClaimsList().saveToDatastore();
-    assertThat(assertThrows(IllegalStateException.class, ClaimsListDualDatabaseDao::get))
-        .hasMessageThat()
-        .isEqualTo("Claims list found in primary DB but not in secondary DB.");
-  }
-
   @Test
   void testGetList_missingOfy() {
-    fakeClock.advanceBy(Duration.standardDays(5));
     ClaimsListSqlDao.save(createClaimsList());
     assertThat(assertThrows(IllegalStateException.class, ClaimsListDualDatabaseDao::get))
         .hasMessageThat()
-        .isEqualTo("Claims list found in primary DB but not in secondary DB.");
+        .isEqualTo("Claims list found in Cloud SQL but not in Datastore.");
-  }
-
-  @Test
-  void testGetList_fromOfy_different() {
-    createClaimsList().saveToDatastore();
-    ClaimsListSqlDao.save(
-        ClaimsListShard.create(fakeClock.nowUtc(), ImmutableMap.of("foo", "bar")));
-    assertThat(assertThrows(IllegalStateException.class, ClaimsListDualDatabaseDao::get))
-        .hasMessageThat()
-        .isEqualTo(
-            "Unequal claims lists detected:\n"
-                + "Domain label label1 with key key1 only appears in the primary DB.\n"
-                + "Domain label label2 with key key2 only appears in the primary DB.\n"
-                + "Domain label foo with key bar only appears in the secondary DB.\n");
   }
 
   @Test
   void testGetList_fromSql_different() {
-    fakeClock.advanceBy(Duration.standardDays(5));
     ClaimsListShard.create(fakeClock.nowUtc(), ImmutableMap.of("foo", "bar")).saveToDatastore();
     ClaimsListSqlDao.save(createClaimsList());
     assertThat(assertThrows(IllegalStateException.class, ClaimsListDualDatabaseDao::get))
         .hasMessageThat()
         .isEqualTo(
             "Unequal claims lists detected:\n"
-                + "Domain label label1 with key key1 only appears in the primary DB.\n"
+                + "Domain label label1 with key key1 only appears in Cloud SQL.\n"
-                + "Domain label label2 with key key2 only appears in the primary DB.\n"
+                + "Domain label label2 with key key2 only appears in Cloud SQL.\n"
-                + "Domain label foo with key bar only appears in the secondary DB.\n");
+                + "Domain label foo with key bar only appears in Datastore.\n");
   }
 
   @Test
@@ -117,15 +69,6 @@ public class ClaimsListDualDatabaseDaoTest extends EntityTestCase {
     assertThat(tm().transact(ClaimsListDualDatabaseDao::get).getLabelsToKeys()).isEmpty();
   }
 
-  @Test
-  void testGetList_missingSql_notInTest() {
-    RegistryEnvironment.PRODUCTION.setup(systemPropertyExtension);
-    createClaimsList().saveToDatastore();
-    // Shouldn't fail in production
-    assertThat(ClaimsListDualDatabaseDao.get().getLabelsToKeys())
-        .isEqualTo(createClaimsList().getLabelsToKeys());
-  }
-
   @Test
   void testGetList_missingOfy_notInTest() {
     RegistryEnvironment.PRODUCTION.setup(systemPropertyExtension);