From 0b6a5aec9623f100be0ddb6ade7942f71022716a Mon Sep 17 00:00:00 2001 From: sarahcaseybot Date: Fri, 19 Mar 2021 18:49:15 -0400 Subject: [PATCH] Modify DeleteReservedListCommand to delete from both databases (#1025) * Modify DeleteReservedListCommand to use both databases * switch to confirming command * fix typo --- .../label/ReservedListDatastoreDao.java | 5 ++++ .../label/ReservedListDualDatabaseDao.java | 15 ++++++++++++ .../registry/label/ReservedListSqlDao.java | 5 ++++ .../tools/DeleteReservedListCommand.java | 17 ++++++++++---- .../label/ReservedListDatastoreDaoTest.java | 15 ++++++++++++ .../ReservedListDualDatabaseDaoTest.java | 23 ++++++++++++++++++- .../label/ReservedListSqlDaoTest.java | 15 ++++++++++++ .../tools/DeleteReservedListCommandTest.java | 5 ++-- 8 files changed, 92 insertions(+), 8 deletions(-) diff --git a/core/src/main/java/google/registry/model/registry/label/ReservedListDatastoreDao.java b/core/src/main/java/google/registry/model/registry/label/ReservedListDatastoreDao.java index a0315f4ec..b76b503d7 100644 --- a/core/src/main/java/google/registry/model/registry/label/ReservedListDatastoreDao.java +++ b/core/src/main/java/google/registry/model/registry/label/ReservedListDatastoreDao.java @@ -31,6 +31,11 @@ public class ReservedListDatastoreDao { ofyTm().transact(() -> ofyTm().put(reservedList)); } + /** Delete a reserved list from Datastore. */ + public static void delete(ReservedList reservedList) { + ofyTm().transact(() -> ofyTm().delete(reservedList)); + } + /** * Returns the most recent revision of the {@link ReservedList} with the specified name, if it * exists. diff --git a/core/src/main/java/google/registry/model/registry/label/ReservedListDualDatabaseDao.java b/core/src/main/java/google/registry/model/registry/label/ReservedListDualDatabaseDao.java index c8899440a..f24c83c80 100644 --- a/core/src/main/java/google/registry/model/registry/label/ReservedListDualDatabaseDao.java +++ b/core/src/main/java/google/registry/model/registry/label/ReservedListDualDatabaseDao.java @@ -51,6 +51,21 @@ public class ReservedListDualDatabaseDao { } } + /** Delete a reserved list from both databases. */ + public static void delete(ReservedList reservedList) { + if (isDatastore(TransitionId.DOMAIN_LABEL_LISTS)) { + ReservedListDatastoreDao.delete(reservedList); + DatabaseMigrationUtils.suppressExceptionUnlessInTest( + () -> ReservedListSqlDao.delete(reservedList), + "Error deleting the reserved list from Cloud SQL."); + } else { + ReservedListSqlDao.delete(reservedList); + DatabaseMigrationUtils.suppressExceptionUnlessInTest( + () -> ReservedListDatastoreDao.delete(reservedList), + "Error deleting the reserved list from Datastore."); + } + } + /** * Returns the most recent revision of the {@link ReservedList} with the specified name, if it * exists. diff --git a/core/src/main/java/google/registry/model/registry/label/ReservedListSqlDao.java b/core/src/main/java/google/registry/model/registry/label/ReservedListSqlDao.java index 90f33920a..a41403f86 100644 --- a/core/src/main/java/google/registry/model/registry/label/ReservedListSqlDao.java +++ b/core/src/main/java/google/registry/model/registry/label/ReservedListSqlDao.java @@ -41,6 +41,11 @@ public class ReservedListSqlDao { reservedList.getName(), reservedList.getReservedListEntries().size()); } + /** Deletes a reserved list from Cloud SQL. */ + public static void delete(ReservedList reservedList) { + jpaTm().transact(() -> jpaTm().delete(reservedList)); + } + /** * Returns the most recent revision of the {@link ReservedList} with the specified name, if it * exists. diff --git a/core/src/main/java/google/registry/tools/DeleteReservedListCommand.java b/core/src/main/java/google/registry/tools/DeleteReservedListCommand.java index bf4cd30c7..e6c84f0ad 100644 --- a/core/src/main/java/google/registry/tools/DeleteReservedListCommand.java +++ b/core/src/main/java/google/registry/tools/DeleteReservedListCommand.java @@ -21,13 +21,14 @@ import com.beust.jcommander.Parameters; import com.google.common.base.Joiner; import com.google.common.collect.ImmutableSet; import google.registry.model.registry.label.ReservedList; +import google.registry.model.registry.label.ReservedListDualDatabaseDao; /** - * Command to delete a {@link ReservedList} in Datastore. This command will fail if the reserved - * list is currently in use on a tld. + * Command to delete a {@link ReservedList} from the database. This command will fail if the + * reserved list is currently in use on a tld. */ -@Parameters(separators = " =", commandDescription = "Deletes a ReservedList in Datastore.") -final class DeleteReservedListCommand extends MutatingCommand { +@Parameters(separators = " =", commandDescription = "Deletes a ReservedList from the database.") +final class DeleteReservedListCommand extends ConfirmingCommand { @Parameter( names = {"-n", "--name"}, @@ -47,6 +48,12 @@ final class DeleteReservedListCommand extends MutatingCommand { tldsUsedOn.isEmpty(), "Cannot delete reserved list because it is used on these tld(s): %s", Joiner.on(", ").join(tldsUsedOn)); - stageEntityChange(existing, null); + } + + @Override + protected String execute() { + ReservedList existing = ReservedList.get(name).get(); + ReservedListDualDatabaseDao.delete(existing); + return String.format("Deleted reserved list: %s", name); } } diff --git a/core/src/test/java/google/registry/model/registry/label/ReservedListDatastoreDaoTest.java b/core/src/test/java/google/registry/model/registry/label/ReservedListDatastoreDaoTest.java index 739b3f1a2..bb5dbaf97 100644 --- a/core/src/test/java/google/registry/model/registry/label/ReservedListDatastoreDaoTest.java +++ b/core/src/test/java/google/registry/model/registry/label/ReservedListDatastoreDaoTest.java @@ -72,6 +72,21 @@ public class ReservedListDatastoreDaoTest { assertThat(savedList.get()).isEqualTo(reservedList); } + @Test + void delete_worksSuccessfully() { + ReservedListDatastoreDao.save(reservedList); + assertThat(ReservedListDatastoreDao.getLatestRevision("testlist").isPresent()).isTrue(); + ReservedListDatastoreDao.delete(reservedList); + assertThat(ReservedListDatastoreDao.getLatestRevision("testlist").isPresent()).isFalse(); + } + + @Test + void delete_listNotInDatastore() { + assertThat(ReservedListDatastoreDao.getLatestRevision("testlist").isPresent()).isFalse(); + ReservedListDatastoreDao.delete(reservedList); + assertThat(ReservedListDatastoreDao.getLatestRevision("testlist").isPresent()).isFalse(); + } + @Test void getLatestRevision_worksSuccessfully() { assertThat(ReservedListDatastoreDao.getLatestRevision("testlist").isPresent()).isFalse(); diff --git a/core/src/test/java/google/registry/model/registry/label/ReservedListDualDatabaseDaoTest.java b/core/src/test/java/google/registry/model/registry/label/ReservedListDualDatabaseDaoTest.java index 710b44963..6d89b0008 100644 --- a/core/src/test/java/google/registry/model/registry/label/ReservedListDualDatabaseDaoTest.java +++ b/core/src/test/java/google/registry/model/registry/label/ReservedListDualDatabaseDaoTest.java @@ -90,7 +90,7 @@ public class ReservedListDualDatabaseDaoTest extends EntityTestCase { } @TestOfyAndSql - void testSave_CloudSqlPrimary_success() { + void testSave_cloudSqlPrimary_success() { fakeClock.advanceBy(Duration.standardDays(5)); ReservedListDualDatabaseDao.save(reservedList); Optional savedList = @@ -98,6 +98,27 @@ public class ReservedListDualDatabaseDaoTest extends EntityTestCase { assertThat(savedList.get()).isEqualTo(reservedList); } + @TestOfyAndSql + void testDelete_datastorePrimary_success() { + ReservedListDualDatabaseDao.save(reservedList); + assertThat(ReservedListDualDatabaseDao.getLatestRevision(reservedList.getName()).isPresent()) + .isTrue(); + ReservedListDualDatabaseDao.delete(reservedList); + assertThat(ReservedListDualDatabaseDao.getLatestRevision(reservedList.getName()).isPresent()) + .isFalse(); + } + + @TestOfyAndSql + void testDelete_cloudSqlPrimary_success() { + fakeClock.advanceBy(Duration.standardDays(5)); + ReservedListDualDatabaseDao.save(reservedList); + assertThat(ReservedListDualDatabaseDao.getLatestRevision(reservedList.getName()).isPresent()) + .isTrue(); + ReservedListDualDatabaseDao.delete(reservedList); + assertThat(ReservedListDualDatabaseDao.getLatestRevision(reservedList.getName()).isPresent()) + .isFalse(); + } + @TestOfyAndSql void testSaveAndLoad_datastorePrimary_emptyList() { ReservedList list = diff --git a/core/src/test/java/google/registry/model/registry/label/ReservedListSqlDaoTest.java b/core/src/test/java/google/registry/model/registry/label/ReservedListSqlDaoTest.java index 92c58236a..d6de2c971 100644 --- a/core/src/test/java/google/registry/model/registry/label/ReservedListSqlDaoTest.java +++ b/core/src/test/java/google/registry/model/registry/label/ReservedListSqlDaoTest.java @@ -80,6 +80,21 @@ public class ReservedListSqlDaoTest { }); } + @Test + void delete_worksSuccessfully() { + ReservedListSqlDao.save(testReservedList); + assertThat(ReservedListSqlDao.checkExists("testlist")).isTrue(); + ReservedListSqlDao.delete(testReservedList); + assertThat(ReservedListSqlDao.checkExists("testlist")).isFalse(); + } + + @Test + void delete_listNotInDatabase() { + assertThat(ReservedListSqlDao.checkExists("testlist")).isFalse(); + ReservedListSqlDao.delete(testReservedList); + assertThat(ReservedListSqlDao.checkExists("testlist")).isFalse(); + } + @Test void checkExists_worksSuccessfully() { assertThat(ReservedListSqlDao.checkExists("testlist")).isFalse(); diff --git a/core/src/test/java/google/registry/tools/DeleteReservedListCommandTest.java b/core/src/test/java/google/registry/tools/DeleteReservedListCommandTest.java index 148a5873b..f07c7451f 100644 --- a/core/src/test/java/google/registry/tools/DeleteReservedListCommandTest.java +++ b/core/src/test/java/google/registry/tools/DeleteReservedListCommandTest.java @@ -23,6 +23,7 @@ import static org.junit.jupiter.api.Assertions.assertThrows; import google.registry.model.registry.Registry; import google.registry.model.registry.label.ReservedList; +import google.registry.model.registry.label.ReservedListDualDatabaseDao; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -40,7 +41,7 @@ class DeleteReservedListCommandTest extends CommandTestCase runCommandForced("--name=" + reservedList.getName())); - assertThat(ReservedList.get(reservedList.getName())).isPresent(); + assertThat(ReservedListDualDatabaseDao.getLatestRevision(reservedList.getName())).isPresent(); assertThat(thrown) .hasMessageThat() .isEqualTo("Cannot delete reserved list because it is used on these tld(s): xn--q9jyb4c");