From 2b38ad8a2524e068a6d8fa5ac3b8ba3a9554c2e0 Mon Sep 17 00:00:00 2001 From: Ben McIlwain Date: Thu, 30 Dec 2021 17:22:43 -0500 Subject: [PATCH] Don't throw errors when existing premium list is empty (#1479) * Don't throw errors when existing premium list is empty This state is possible to get into when things go wrong and it shouldn't prevent saving new revisions of the list. Note that it will continue to throw errors if you attempt to save a new revision that is blank (which is usually a mistake). See http://b/211774375 --- .../model/tld/label/PremiumListDao.java | 2 ++ .../model/tld/label/PremiumListUtils.java | 2 -- .../tools/UpdatePremiumListCommand.java | 8 +++---- .../model/tld/label/PremiumListDaoTest.java | 10 ++++++++ .../tools/UpdatePremiumListCommandTest.java | 23 ++++++++++++++++++- 5 files changed, 38 insertions(+), 7 deletions(-) diff --git a/core/src/main/java/google/registry/model/tld/label/PremiumListDao.java b/core/src/main/java/google/registry/model/tld/label/PremiumListDao.java index cfbecbf88..1953ce609 100644 --- a/core/src/main/java/google/registry/model/tld/label/PremiumListDao.java +++ b/core/src/main/java/google/registry/model/tld/label/PremiumListDao.java @@ -14,6 +14,7 @@ package google.registry.model.tld.label; +import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.collect.ImmutableList.toImmutableList; import static google.registry.config.RegistryConfig.getDomainLabelListCacheDuration; import static google.registry.config.RegistryConfig.getSingletonCachePersistDuration; @@ -154,6 +155,7 @@ public class PremiumListDao { } public static PremiumList save(String name, CurrencyUnit currencyUnit, List inputData) { + checkArgument(!inputData.isEmpty(), "New premium list data cannot be empty"); return save(PremiumListUtils.parseToPremiumList(name, currencyUnit, inputData)); } diff --git a/core/src/main/java/google/registry/model/tld/label/PremiumListUtils.java b/core/src/main/java/google/registry/model/tld/label/PremiumListUtils.java index 6fa8b26d1..d25f9d61d 100644 --- a/core/src/main/java/google/registry/model/tld/label/PremiumListUtils.java +++ b/core/src/main/java/google/registry/model/tld/label/PremiumListUtils.java @@ -14,7 +14,6 @@ package google.registry.model.tld.label; -import static com.google.common.base.Preconditions.checkArgument; import static org.joda.time.DateTimeZone.UTC; import com.google.common.collect.ImmutableMap; @@ -38,7 +37,6 @@ public class PremiumListUtils { .setCreationTimestamp(DateTime.now(UTC)) .build(); ImmutableMap prices = partialPremiumList.parse(inputData); - checkArgument(inputData.size() > 0, "Input cannot be empty"); Map priceAmounts = Maps.transformValues(prices, PremiumEntry::getValue); return partialPremiumList.asBuilder().setLabelsToPrices(priceAmounts).build(); } diff --git a/core/src/main/java/google/registry/tools/UpdatePremiumListCommand.java b/core/src/main/java/google/registry/tools/UpdatePremiumListCommand.java index 61b2c2a47..096156602 100644 --- a/core/src/main/java/google/registry/tools/UpdatePremiumListCommand.java +++ b/core/src/main/java/google/registry/tools/UpdatePremiumListCommand.java @@ -16,6 +16,7 @@ package google.registry.tools; import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.collect.ImmutableSet.toImmutableSet; +import static google.registry.model.tld.label.PremiumListUtils.parseToPremiumList; import static google.registry.persistence.transaction.TransactionManagerFactory.jpaTm; import static google.registry.util.ListNamingUtils.convertFilePathToName; import static java.nio.charset.StandardCharsets.UTF_8; @@ -27,7 +28,6 @@ import com.google.common.collect.Streams; import google.registry.model.tld.label.PremiumList; import google.registry.model.tld.label.PremiumList.PremiumEntry; import google.registry.model.tld.label.PremiumListDao; -import google.registry.model.tld.label.PremiumListUtils; import java.nio.file.Files; import java.util.List; import java.util.Optional; @@ -45,11 +45,11 @@ class UpdatePremiumListCommand extends CreateOrUpdatePremiumListCommand { String.format("Could not update premium list %s because it doesn't exist.", name)); List existingEntry = getExistingPremiumEntry(list.get()).asList(); inputData = Files.readAllLines(inputFile, UTF_8); + checkArgument(!inputData.isEmpty(), "New premium list data cannot be empty"); currency = list.get().getCurrency(); // reconstructing existing premium list to bypass Hibernate lazy initialization exception - PremiumList existingPremiumList = - PremiumListUtils.parseToPremiumList(name, currency, existingEntry); - PremiumList updatedPremiumList = PremiumListUtils.parseToPremiumList(name, currency, inputData); + PremiumList existingPremiumList = parseToPremiumList(name, currency, existingEntry); + PremiumList updatedPremiumList = parseToPremiumList(name, currency, inputData); return String.format( "Update premium list for %s?\n Old List: %s\n New List: %s", diff --git a/core/src/test/java/google/registry/model/tld/label/PremiumListDaoTest.java b/core/src/test/java/google/registry/model/tld/label/PremiumListDaoTest.java index d8b4787e5..45aab6d06 100644 --- a/core/src/test/java/google/registry/model/tld/label/PremiumListDaoTest.java +++ b/core/src/test/java/google/registry/model/tld/label/PremiumListDaoTest.java @@ -22,6 +22,7 @@ import static google.registry.testing.DatabaseHelper.persistResource; import static org.joda.money.CurrencyUnit.JPY; import static org.joda.money.CurrencyUnit.USD; import static org.joda.time.Duration.standardDays; +import static org.junit.jupiter.api.Assertions.assertThrows; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; @@ -239,6 +240,15 @@ public class PremiumListDaoTest { .hasValue(moneyOf(JPY, 15000)); } + @Test + void testSave_throwsOnEmptyInputData() { + IllegalArgumentException thrown = + assertThrows( + IllegalArgumentException.class, + () -> PremiumListDao.save("test-list", CurrencyUnit.GBP, ImmutableList.of())); + assertThat(thrown).hasMessageThat().isEqualTo("New premium list data cannot be empty"); + } + @Test void test_savePremiumList_clearsCache() { assertThat(PremiumListDao.premiumListCache.getIfPresent("testname")).isNull(); diff --git a/core/src/test/java/google/registry/tools/UpdatePremiumListCommandTest.java b/core/src/test/java/google/registry/tools/UpdatePremiumListCommandTest.java index a7d5c3661..a0b9c663d 100644 --- a/core/src/test/java/google/registry/tools/UpdatePremiumListCommandTest.java +++ b/core/src/test/java/google/registry/tools/UpdatePremiumListCommandTest.java @@ -84,6 +84,27 @@ class UpdatePremiumListCommandTest assertThat(entries.contains("eth,USD 9999.00")).isTrue(); } + @Test + void commandRun_successUpdateList_whenExistingListIsEmpty() throws Exception { + File existingPremiumFile = tmpDir.resolve(TLD_TEST + ".txt").toFile(); + Files.asCharSink(existingPremiumFile, UTF_8).write(""); + + File newPremiumFile = tmpDir.resolve(String.format("%s.txt", TLD_TEST)).toFile(); + String newPremiumListData = "eth,USD 9999"; + Files.asCharSink(newPremiumFile, UTF_8).write(newPremiumListData); + + UpdatePremiumListCommand command = new UpdatePremiumListCommand(); + // data come from @beforeEach of CreateOrUpdatePremiumListCommandTestCase.java + command.inputFile = Paths.get(newPremiumFile.getPath()); + runCommandForced("--name=" + TLD_TEST, "--input=" + command.inputFile); + + ImmutableSet entries = + command.getExistingPremiumEntry(PremiumListDao.getLatestRevision(TLD_TEST).get()); + assertThat(entries.size()).isEqualTo(1); + // verify that list is updated; cannot use only string since price is formatted; + assertThat(entries.contains("eth,USD 9999.00")).isTrue(); + } + @Test void commandRun_successUpdateMultiLineList() throws Exception { File tmpFile = tmpDir.resolve(TLD_TEST + ".txt").toFile(); @@ -112,7 +133,7 @@ class UpdatePremiumListCommandTest command.inputFile = tmpPath; command.name = TLD_TEST; IllegalArgumentException thrown = assertThrows(IllegalArgumentException.class, command::prompt); - assertThat(thrown).hasMessageThat().contains("Input cannot be empty"); + assertThat(thrown).hasMessageThat().isEqualTo("New premium list data cannot be empty"); } @Test