From 79b4cb0d826124a84dcd2410ef30ea5af5626b95 Mon Sep 17 00:00:00 2001 From: sarahcaseybot Date: Wed, 10 Mar 2021 14:16:39 -0500 Subject: [PATCH] Add ReservedList to CloudSQL in unit tests (#992) * Add ReservedList to CloudSQL in unit tests * Remove unnecessary changes * Change databasehelper to use DualDatabaseDao --- .../model/registry/label/ReservedList.java | 2 +- ....java => ReservedListDualDatabaseDao.java} | 29 ++++----- .../CreateOrUpdateReservedListCommand.java | 4 +- .../registry/model/registry/RegistryTest.java | 59 ++++++++++++++++--- .../registry/testing/DatabaseHelper.java | 16 ++--- .../tools/UpdateReservedListCommandTest.java | 10 ++-- 6 files changed, 81 insertions(+), 39 deletions(-) rename core/src/main/java/google/registry/model/registry/label/{ReservedListDualWriteDao.java => ReservedListDualDatabaseDao.java} (85%) diff --git a/core/src/main/java/google/registry/model/registry/label/ReservedList.java b/core/src/main/java/google/registry/model/registry/label/ReservedList.java index 27931fbae..eb97b9471 100644 --- a/core/src/main/java/google/registry/model/registry/label/ReservedList.java +++ b/core/src/main/java/google/registry/model/registry/label/ReservedList.java @@ -248,7 +248,7 @@ public final class ReservedList @Override public ReservedList load(String listName) { return tm().isOfy() - ? ReservedListDualWriteDao.getLatestRevision(listName).orElse(null) + ? ReservedListDualDatabaseDao.getLatestRevision(listName).orElse(null) : ReservedListSqlDao.getLatestRevision(listName).orElse(null); } }); diff --git a/core/src/main/java/google/registry/model/registry/label/ReservedListDualWriteDao.java b/core/src/main/java/google/registry/model/registry/label/ReservedListDualDatabaseDao.java similarity index 85% rename from core/src/main/java/google/registry/model/registry/label/ReservedListDualWriteDao.java rename to core/src/main/java/google/registry/model/registry/label/ReservedListDualDatabaseDao.java index 14e771c10..ed5282521 100644 --- a/core/src/main/java/google/registry/model/registry/label/ReservedListDualWriteDao.java +++ b/core/src/main/java/google/registry/model/registry/label/ReservedListDualDatabaseDao.java @@ -23,6 +23,7 @@ import com.google.common.collect.MapDifference.ValueDifference; import com.google.common.collect.Maps; import com.google.common.flogger.FluentLogger; import com.googlecode.objectify.Key; +import google.registry.model.DatabaseMigrationUtils; import google.registry.model.registry.label.ReservedList.ReservedListEntry; import google.registry.persistence.VKey; import java.util.Map; @@ -35,24 +36,22 @@ import java.util.Optional; *

TODO(b/160993806): Delete this DAO and switch to use the SQL only DAO after migrating to Cloud * SQL. */ -public class ReservedListDualWriteDao { +public class ReservedListDualDatabaseDao { private static final FluentLogger logger = FluentLogger.forEnclosingClass(); - private ReservedListDualWriteDao() {} + private ReservedListDualDatabaseDao() {} /** Persist a new reserved list to Cloud SQL. */ public static void save(ReservedList reservedList) { ofyTm().transact(() -> ofyTm().put(reservedList)); - try { logger.atInfo().log("Saving reserved list %s to Cloud SQL", reservedList.getName()); - ReservedListSqlDao.save(reservedList); + DatabaseMigrationUtils.suppressExceptionUnlessInTest( + () -> ReservedListSqlDao.save(reservedList), + "Error saving the reserved list to Cloud SQL."); logger.atInfo().log( "Saved reserved list %s with %d entries to Cloud SQL", reservedList.getName(), reservedList.getReservedListEntries().size()); - } catch (Throwable t) { - logger.atSevere().withCause(t).log("Error saving the reserved list to Cloud SQL."); - } } /** @@ -66,12 +65,10 @@ public class ReservedListDualWriteDao { VKey.createOfy( ReservedList.class, Key.create(getCrossTldKey(), ReservedList.class, reservedListName))); - try { - // Also load the list from Cloud SQL, compare the two lists, and log if different. - maybeDatastoreList.ifPresent(ReservedListDualWriteDao::loadAndCompareCloudSqlList); - } catch (Throwable t) { - logger.atSevere().withCause(t).log("Error comparing reserved lists."); - } + // Also load the list from Cloud SQL, compare the two lists, and log if different. + DatabaseMigrationUtils.suppressExceptionUnlessInTest( + () -> maybeDatastoreList.ifPresent(ReservedListDualDatabaseDao::loadAndCompareCloudSqlList), + "Error comparing reserved lists."); return maybeDatastoreList; } @@ -95,7 +92,7 @@ public class ReservedListDualWriteDao { Maps.difference(datastoreLabelsToReservations, cloudSqlList.reservedListMap); if (!diff.areEqual()) { if (diff.entriesDiffering().size() > 10) { - logger.atWarning().log( + throw new IllegalStateException( String.format( "Unequal reserved lists detected, Cloud SQL list with revision" + " id %d has %d different records than the current" @@ -114,11 +111,11 @@ public class ReservedListDualWriteDao { + " %s in Cloud SQL.\n", label, valueDiff.leftValue(), valueDiff.rightValue())); }); - logger.atWarning().log(diffMessage.toString()); + throw new IllegalStateException(diffMessage.toString()); } } } else { - logger.atWarning().log("Reserved list in Cloud SQL is empty."); + throw new IllegalStateException("Reserved list in Cloud SQL is empty."); } } } diff --git a/core/src/main/java/google/registry/tools/CreateOrUpdateReservedListCommand.java b/core/src/main/java/google/registry/tools/CreateOrUpdateReservedListCommand.java index b77a87e94..40051c420 100644 --- a/core/src/main/java/google/registry/tools/CreateOrUpdateReservedListCommand.java +++ b/core/src/main/java/google/registry/tools/CreateOrUpdateReservedListCommand.java @@ -17,7 +17,7 @@ package google.registry.tools; import com.beust.jcommander.Parameter; import com.google.common.flogger.FluentLogger; import google.registry.model.registry.label.ReservedList; -import google.registry.model.registry.label.ReservedListDualWriteDao; +import google.registry.model.registry.label.ReservedListDualDatabaseDao; import google.registry.tools.params.PathParameter; import java.nio.file.Path; import javax.annotation.Nullable; @@ -61,7 +61,7 @@ public abstract class CreateOrUpdateReservedListCommand extends MutatingCommand name, reservedList.getReservedListEntries().size()); try { logger.atInfo().log("Saving reserved list for TLD %s", name); - ReservedListDualWriteDao.save(reservedList); + ReservedListDualDatabaseDao.save(reservedList); logger.atInfo().log(message); } catch (Throwable e) { message = "Unexpected error saving reserved list from nomulus tool command"; diff --git a/core/src/test/java/google/registry/model/registry/RegistryTest.java b/core/src/test/java/google/registry/model/registry/RegistryTest.java index ff03d677d..04331b4a2 100644 --- a/core/src/test/java/google/registry/model/registry/RegistryTest.java +++ b/core/src/test/java/google/registry/model/registry/RegistryTest.java @@ -33,6 +33,7 @@ import static org.joda.money.CurrencyUnit.EUR; import static org.joda.money.CurrencyUnit.USD; import static org.junit.jupiter.api.Assertions.assertThrows; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import com.google.common.collect.ImmutableSortedMap; import com.googlecode.objectify.Key; @@ -135,9 +136,23 @@ public class RegistryTest extends EntityTestCase { @TestOfyAndSql void testSetReservedList_doesntMutateExistingRegistry() { ReservedList rl15 = - persistReservedList("tld-reserved15", "potato,FULLY_BLOCKED", "phone,FULLY_BLOCKED"); + persistReservedList( + new ReservedList.Builder() + .setName("tld-reserved15") + .setReservedListMapFromLines( + ImmutableList.of("potato,FULLY_BLOCKED", "phone,FULLY_BLOCKED")) + .setShouldPublish(true) + .setLastUpdateTime(fakeClock.nowUtc()) + .build()); ReservedList rl16 = - persistReservedList("tld-reserved16", "port,FULLY_BLOCKED", "manteau,FULLY_BLOCKED"); + persistReservedList( + new ReservedList.Builder() + .setName("tld-reserved16") + .setReservedListMapFromLines( + ImmutableList.of("port,FULLY_BLOCKED", "manteau,FULLY_BLOCKED")) + .setShouldPublish(true) + .setLastUpdateTime(fakeClock.nowUtc()) + .build()); Registry registry1 = newRegistry("propter", "PROPTER") .asBuilder() @@ -173,9 +188,23 @@ public class RegistryTest extends EntityTestCase { @TestOfyAndSql void testSetReservedLists() { ReservedList rl5 = - persistReservedList("tld-reserved5", "lol,FULLY_BLOCKED", "cat,FULLY_BLOCKED"); + persistReservedList( + new ReservedList.Builder() + .setName("tld-reserved5") + .setReservedListMapFromLines( + ImmutableList.of("potato,FULLY_BLOCKED", "phone,FULLY_BLOCKED")) + .setShouldPublish(true) + .setLastUpdateTime(fakeClock.nowUtc()) + .build()); ReservedList rl6 = - persistReservedList("tld-reserved6", "hammock,FULLY_BLOCKED", "mouse,FULLY_BLOCKED"); + persistReservedList( + new ReservedList.Builder() + .setName("tld-reserved6") + .setReservedListMapFromLines( + ImmutableList.of("port,FULLY_BLOCKED", "manteau,FULLY_BLOCKED")) + .setShouldPublish(true) + .setLastUpdateTime(fakeClock.nowUtc()) + .build()); Registry r = Registry.get("tld").asBuilder().setReservedLists(ImmutableSet.of(rl5, rl6)).build(); assertThat(r.getReservedLists().stream().map(Key::getName)) @@ -186,15 +215,29 @@ public class RegistryTest extends EntityTestCase { @TestOfyAndSql void testSetReservedListsByName() { - persistReservedList("tld-reserved24", "lol,FULLY_BLOCKED", "cat,FULLY_BLOCKED"); - persistReservedList("tld-reserved25", "mit,FULLY_BLOCKED", "tim,FULLY_BLOCKED"); + persistReservedList( + new ReservedList.Builder() + .setName("tld-reserved15") + .setReservedListMapFromLines( + ImmutableList.of("potato,FULLY_BLOCKED", "phone,FULLY_BLOCKED")) + .setShouldPublish(true) + .setLastUpdateTime(fakeClock.nowUtc()) + .build()); + persistReservedList( + new ReservedList.Builder() + .setName("tld-reserved16") + .setReservedListMapFromLines( + ImmutableList.of("port,FULLY_BLOCKED", "manteau,FULLY_BLOCKED")) + .setShouldPublish(true) + .setLastUpdateTime(fakeClock.nowUtc()) + .build()); Registry r = Registry.get("tld") .asBuilder() - .setReservedListsByName(ImmutableSet.of("tld-reserved24", "tld-reserved25")) + .setReservedListsByName(ImmutableSet.of("tld-reserved15", "tld-reserved16")) .build(); assertThat(r.getReservedLists().stream().map(Key::getName)) - .containsExactly("tld-reserved24", "tld-reserved25"); + .containsExactly("tld-reserved15", "tld-reserved16"); r = Registry.get("tld").asBuilder().setReservedListsByName(ImmutableSet.of()).build(); assertThat(r.getReservedLists()).isEmpty(); } diff --git a/core/src/test/java/google/registry/testing/DatabaseHelper.java b/core/src/test/java/google/registry/testing/DatabaseHelper.java index 4975814ea..bc8c6958e 100644 --- a/core/src/test/java/google/registry/testing/DatabaseHelper.java +++ b/core/src/test/java/google/registry/testing/DatabaseHelper.java @@ -105,6 +105,7 @@ import google.registry.model.registry.label.PremiumList.PremiumListEntry; import google.registry.model.registry.label.PremiumList.PremiumListRevision; import google.registry.model.registry.label.PremiumListDualDao; import google.registry.model.registry.label.ReservedList; +import google.registry.model.registry.label.ReservedListDualDatabaseDao; import google.registry.model.reporting.HistoryEntry; import google.registry.model.transfer.ContactTransferData; import google.registry.model.transfer.DomainTransferData; @@ -345,6 +346,13 @@ public class DatabaseHelper { return persistReservedList(listName, true, lines); } + public static ReservedList persistReservedList(ReservedList reservedList) { + ReservedListDualDatabaseDao.save(reservedList); + maybeAdvanceClock(); + tm().clearSessionCache(); + return reservedList; + } + public static ReservedList persistReservedList( String listName, boolean shouldPublish, String... lines) { ReservedList reservedList = @@ -354,13 +362,7 @@ public class DatabaseHelper { .setShouldPublish(shouldPublish) .setLastUpdateTime(DateTime.now(DateTimeZone.UTC)) .build(); - return tm().isOfy() - ? persistResource(reservedList) - : tm().transact( - () -> { - tm().insert(reservedList); - return reservedList; - }); + return persistReservedList(reservedList); } /** diff --git a/core/src/test/java/google/registry/tools/UpdateReservedListCommandTest.java b/core/src/test/java/google/registry/tools/UpdateReservedListCommandTest.java index da754ff59..c661851ab 100644 --- a/core/src/test/java/google/registry/tools/UpdateReservedListCommandTest.java +++ b/core/src/test/java/google/registry/tools/UpdateReservedListCommandTest.java @@ -17,7 +17,7 @@ package google.registry.tools; import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.Truth8.assertThat; import static google.registry.model.registry.label.ReservationType.FULLY_BLOCKED; -import static google.registry.testing.DatabaseHelper.persistResource; +import static google.registry.testing.DatabaseHelper.persistReservedList; import static google.registry.util.DateTimeUtils.START_OF_TIME; import static org.junit.jupiter.api.Assertions.assertThrows; @@ -35,11 +35,11 @@ class UpdateReservedListCommandTest @BeforeEach void beforeEach() { - populateInitialReservedListInDatastore(true); + populateInitialReservedListInDatabase(true); } - private void populateInitialReservedListInDatastore(boolean shouldPublish) { - persistResource( + private void populateInitialReservedListInDatabase(boolean shouldPublish) { + persistReservedList( new ReservedList.Builder() .setName("xn--q9jyb4c_common-reserved") .setReservedListMapFromLines(ImmutableList.of("helicopter,FULLY_BLOCKED")) @@ -89,7 +89,7 @@ class UpdateReservedListCommandTest @Test void testSuccess_shouldPublish_doesntOverrideFalseIfNotSpecified() throws Exception { - populateInitialReservedListInDatastore(false); + populateInitialReservedListInDatabase(false); runCommandForced("--input=" + reservedTermsPath); assertThat(ReservedList.get("xn--q9jyb4c_common-reserved")).isPresent(); ReservedList reservedList = ReservedList.get("xn--q9jyb4c_common-reserved").get();