Support premium list updating in Cloud SQL (#422)

* Support premium list updating in Cloud SQL This also removes the requirement to specify --also_cloud_sql in nomulus premium list tooling, instead always persisting to Cloud SQL. It removes a non-USD premium label in the global test premium list (the Cloud SQL schema doesn't support a mix of currency units in a single premium list). And it adds a method to PremiumListDao to grab the most recent version of a given list. * Merge branch 'master' into premium-lists-always-cloud-sql * Revert test change * Create new PremiumListUtils class and refactor out existing method * Fix tests and update an existing premium price
google · Jan 2, 2020 · 8f67c4b · 8f67c4b
1 parent c17a5c4
commit 8f67c4b
Show file tree

Hide file tree

Showing 18 changed files with 199 additions and 165 deletions.
diff --git a/core/src/main/java/google/registry/schema/tld/PremiumListDao.java b/core/src/main/java/google/registry/schema/tld/PremiumListDao.java
@@ -80,7 +80,7 @@ public static void update(PremiumList premiumList) {
    * <p>Note that this does not load <code>PremiumList.labelsToPrices</code>! If you need to check
    * prices, use {@link #getPremiumPrice}.
    */
-  static Optional<PremiumList> getLatestRevision(String premiumListName) {
+  public static Optional<PremiumList> getLatestRevision(String premiumListName) {
     return jpaTm()
         .transact(
             () ->

diff --git a/core/src/main/java/google/registry/schema/tld/PremiumListUtils.java b/core/src/main/java/google/registry/schema/tld/PremiumListUtils.java
@@ -0,0 +1,61 @@
+// Copyright 2019 The Nomulus Authors. All Rights Reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package google.registry.schema.tld;
+
+import static com.google.common.base.Preconditions.checkArgument;
+import static com.google.common.collect.ImmutableSet.toImmutableSet;
+
+import com.google.common.base.Splitter;
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.ImmutableSortedSet;
+import com.google.common.collect.Iterables;
+import com.google.common.collect.Maps;
+import google.registry.model.registry.label.PremiumList.PremiumListEntry;
+import java.math.BigDecimal;
+import java.util.List;
+import java.util.Map;
+import org.joda.money.CurrencyUnit;
+
+/** Static utility methods for {@link PremiumList}. */
+public class PremiumListUtils {
+
+  public static PremiumList parseToPremiumList(String name, String inputData) {
+    List<String> inputDataPreProcessed =
+        Splitter.on('\n').omitEmptyStrings().splitToList(inputData);
+
+    ImmutableMap<String, PremiumListEntry> prices =
+        new google.registry.model.registry.label.PremiumList.Builder()
+            .setName(name)
+            .build()
+            .parse(inputDataPreProcessed);
+    ImmutableSet<CurrencyUnit> currencies =
+        prices.values().stream()
+            .map(e -> e.getValue().getCurrencyUnit())
+            .distinct()
+            .collect(toImmutableSet());
+    checkArgument(
+        currencies.size() == 1,
+        "The Cloud SQL schema requires exactly one currency, but got: %s",
+        ImmutableSortedSet.copyOf(currencies));
+    CurrencyUnit currency = Iterables.getOnlyElement(currencies);
+
+    Map<String, BigDecimal> priceAmounts =
+        Maps.transformValues(prices, ple -> ple.getValue().getAmount());
+    return google.registry.schema.tld.PremiumList.create(name, currency, priceAmounts);
+  }
+
+  private PremiumListUtils() {}
+}
diff --git a/core/src/main/java/google/registry/tools/CreateOrUpdatePremiumListCommand.java b/core/src/main/java/google/registry/tools/CreateOrUpdatePremiumListCommand.java
@@ -16,7 +16,6 @@
 
 import static com.google.common.base.Strings.isNullOrEmpty;
 import static google.registry.security.JsonHttp.JSON_SAFETY_PREFIX;
-import static google.registry.tools.server.CreateOrUpdatePremiumListAction.ALSO_CLOUD_SQL_PARAM;
 import static google.registry.tools.server.CreateOrUpdatePremiumListAction.INPUT_PARAM;
 import static google.registry.tools.server.CreateOrUpdatePremiumListAction.NAME_PARAM;
 import static google.registry.util.ListNamingUtils.convertFilePathToName;
@@ -58,12 +57,6 @@ abstract class CreateOrUpdatePremiumListCommand extends ConfirmingCommand
       required = true)
   Path inputFile;
 
-  @Parameter(
-      names = {"--also_cloud_sql"},
-      description =
-          "Persist premium list to Cloud SQL in addition to Datastore; defaults to false.")
-  boolean alsoCloudSql;
-
   protected AppEngineConnection connection;
   protected int inputLineCount;
 
@@ -97,7 +90,6 @@ protected String prompt() {
   public String execute() throws Exception {
     ImmutableMap.Builder<String, String> params = new ImmutableMap.Builder<>();
     params.put(NAME_PARAM, name);
-    params.put(ALSO_CLOUD_SQL_PARAM, Boolean.toString(alsoCloudSql));
     String inputFileContents = new String(Files.readAllBytes(inputFile), UTF_8);
     String requestBody =
         Joiner.on('&').withKeyValueSeparator("=").join(

diff --git a/core/src/main/java/google/registry/tools/server/CreateOrUpdatePremiumListAction.java b/core/src/main/java/google/registry/tools/server/CreateOrUpdatePremiumListAction.java
@@ -14,26 +14,13 @@
 
 package google.registry.tools.server;
 
-import static com.google.common.base.Preconditions.checkArgument;
-import static com.google.common.collect.ImmutableSet.toImmutableSet;
 import static com.google.common.flogger.LazyArgs.lazy;
 
-import com.google.common.base.Splitter;
 import com.google.common.collect.ImmutableMap;
-import com.google.common.collect.ImmutableSet;
-import com.google.common.collect.ImmutableSortedSet;
-import com.google.common.collect.Iterables;
-import com.google.common.collect.Maps;
 import com.google.common.flogger.FluentLogger;
-import google.registry.model.registry.label.PremiumList;
-import google.registry.model.registry.label.PremiumList.PremiumListEntry;
 import google.registry.request.JsonResponse;
 import google.registry.request.Parameter;
-import java.math.BigDecimal;
-import java.util.List;
-import java.util.Map;
 import javax.inject.Inject;
-import org.joda.money.CurrencyUnit;
 
 /** Abstract base class for actions that update premium lists. */
 public abstract class CreateOrUpdatePremiumListAction implements Runnable {
@@ -44,7 +31,6 @@ public abstract class CreateOrUpdatePremiumListAction implements Runnable {
 
   public static final String NAME_PARAM = "name";
   public static final String INPUT_PARAM = "inputData";
-  public static final String ALSO_CLOUD_SQL_PARAM = "alsoCloudSql";
 
   @Inject JsonResponse response;
 
@@ -56,10 +42,6 @@ public abstract class CreateOrUpdatePremiumListAction implements Runnable {
   @Parameter(INPUT_PARAM)
   String inputData;
 
-  @Inject
-  @Parameter(ALSO_CLOUD_SQL_PARAM)
-  boolean alsoCloudSql;
-
   @Override
   public void run() {
     try {
@@ -76,40 +58,15 @@ public void run() {
       return;
     }
 
-    if (alsoCloudSql) {
-      try {
-        saveToCloudSql();
-      } catch (Throwable e) {
-        logger.atSevere().withCause(e).log(
-            "Unexpected error saving premium list to Cloud SQL from nomulus tool command");
-        response.setPayload(ImmutableMap.of("error", e.toString(), "status", "error"));
-        return;
-      }
+    try {
+      saveToCloudSql();
+    } catch (Throwable e) {
+      logger.atSevere().withCause(e).log(
+          "Unexpected error saving premium list to Cloud SQL from nomulus tool command");
+      response.setPayload(ImmutableMap.of("error", e.toString(), "status", "error"));
     }
   }
 
-  google.registry.schema.tld.PremiumList parseInputToPremiumList() {
-    List<String> inputDataPreProcessed =
-        Splitter.on('\n').omitEmptyStrings().splitToList(inputData);
-
-    ImmutableMap<String, PremiumListEntry> prices =
-        new PremiumList.Builder().setName(name).build().parse(inputDataPreProcessed);
-    ImmutableSet<CurrencyUnit> currencies =
-        prices.values().stream()
-            .map(e -> e.getValue().getCurrencyUnit())
-            .distinct()
-            .collect(toImmutableSet());
-    checkArgument(
-        currencies.size() == 1,
-        "The Cloud SQL schema requires exactly one currency, but got: %s",
-        ImmutableSortedSet.copyOf(currencies));
-    CurrencyUnit currency = Iterables.getOnlyElement(currencies);
-
-    Map<String, BigDecimal> priceAmounts =
-        Maps.transformValues(prices, ple -> ple.getValue().getAmount());
-    return google.registry.schema.tld.PremiumList.create(name, currency, priceAmounts);
-  }
-
   /** Logs the premium list data at INFO, truncated if too long. */
   void logInputData() {
     logger.atInfo().log(

diff --git a/core/src/main/java/google/registry/tools/server/CreatePremiumListAction.java b/core/src/main/java/google/registry/tools/server/CreatePremiumListAction.java
@@ -19,6 +19,7 @@
 import static google.registry.model.registry.label.PremiumListUtils.doesPremiumListExist;
 import static google.registry.model.registry.label.PremiumListUtils.savePremiumListAndEntries;
 import static google.registry.request.Action.Method.POST;
+import static google.registry.schema.tld.PremiumListUtils.parseToPremiumList;
 
 import com.google.common.base.Splitter;
 import com.google.common.collect.ImmutableMap;
@@ -81,7 +82,7 @@ protected void saveToCloudSql() {
     logger.atInfo().log("Saving premium list to Cloud SQL for TLD %s", name);
     // TODO(mcilwain): Call logInputData() here once Datastore persistence is removed.
 
-    google.registry.schema.tld.PremiumList premiumList = parseInputToPremiumList();
+    google.registry.schema.tld.PremiumList premiumList = parseToPremiumList(name, inputData);
     PremiumListDao.saveNew(premiumList);
 
     String message =

diff --git a/core/src/main/java/google/registry/tools/server/ToolsServerModule.java b/core/src/main/java/google/registry/tools/server/ToolsServerModule.java
@@ -60,12 +60,6 @@ static String provideInput(HttpServletRequest req) {
     return extractRequiredParameter(req, CreatePremiumListAction.INPUT_PARAM);
   }
 
-  @Provides
-  @Parameter("alsoCloudSql")
-  static boolean provideAlsoCloudSql(HttpServletRequest req) {
-    return extractBooleanParameter(req, CreatePremiumListAction.ALSO_CLOUD_SQL_PARAM);
-  }
-
   @Provides
   @Parameter("premiumListName")
   static String provideName(HttpServletRequest req) {

diff --git a/core/src/main/java/google/registry/tools/server/UpdatePremiumListAction.java b/core/src/main/java/google/registry/tools/server/UpdatePremiumListAction.java
@@ -17,13 +17,15 @@
 import static com.google.common.base.Preconditions.checkArgument;
 import static google.registry.model.registry.label.PremiumListUtils.savePremiumListAndEntries;
 import static google.registry.request.Action.Method.POST;
+import static google.registry.schema.tld.PremiumListUtils.parseToPremiumList;
 
 import com.google.common.base.Splitter;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.flogger.FluentLogger;
 import google.registry.model.registry.label.PremiumList;
 import google.registry.request.Action;
 import google.registry.request.auth.Auth;
+import google.registry.schema.tld.PremiumListDao;
 import java.util.List;
 import java.util.Optional;
 import javax.inject.Inject;
@@ -68,10 +70,17 @@ protected void saveToDatastore() {
     response.setPayload(ImmutableMap.of("status", "success", "message", message));
   }
 
-  // TODO(mcilwain): Implement this in a subsequent PR.
   @Override
   protected void saveToCloudSql() {
-    throw new UnsupportedOperationException(
-        "Updating of premium lists in Cloud SQL is not supported yet");
+    logger.atInfo().log("Updating premium list '%s' in Cloud SQL.", name);
+    // TODO(mcilwain): Add logInputData() call here once DB migration is complete.
+    google.registry.schema.tld.PremiumList premiumList = parseToPremiumList(name, inputData);
+    PremiumListDao.update(premiumList);
+    String message =
+        String.format(
+            "Updated premium list '%s' with %d entries.",
+            premiumList.getName(), premiumList.getLabelsToPrices().size());
+    logger.atInfo().log(message);
+    // TODO(mcilwain): Call response.setPayload() here once DB migration is complete.
   }
 }
diff --git a/core/src/test/java/google/registry/schema/integration/SqlIntegrationTestSuite.java b/core/src/test/java/google/registry/schema/integration/SqlIntegrationTestSuite.java
@@ -18,10 +18,13 @@
 import google.registry.model.transaction.JpaTestRules.JpaIntegrationTestRule;
 import google.registry.schema.cursor.CursorDaoTest;
 import google.registry.schema.tld.PremiumListDaoTest;
+import google.registry.schema.tld.PremiumListUtilsTest;
 import google.registry.schema.tld.ReservedListDaoTest;
 import google.registry.schema.tmch.ClaimsListDaoTest;
 import google.registry.tools.CreateReservedListCommandTest;
 import google.registry.tools.UpdateReservedListCommandTest;
+import google.registry.tools.server.CreatePremiumListActionTest;
+import google.registry.tools.server.UpdatePremiumListActionTest;
 import google.registry.ui.server.registrar.RegistryLockGetActionTest;
 import org.junit.runner.RunWith;
 import org.junit.runners.Suite;
@@ -41,10 +44,13 @@
   ClaimsListDaoTest.class,
   CreateReservedListCommandTest.class,
   CursorDaoTest.class,
+  CreatePremiumListActionTest.class,
   PremiumListDaoTest.class,
+  PremiumListUtilsTest.class,
   RegistryLockDaoTest.class,
   RegistryLockGetActionTest.class,
   ReservedListDaoTest.class,
+  UpdatePremiumListActionTest.class,
   UpdateReservedListCommandTest.class
 })
 public class SqlIntegrationTestSuite {}
diff --git a/core/src/test/java/google/registry/schema/tld/PremiumListDaoTest.java b/core/src/test/java/google/registry/schema/tld/PremiumListDaoTest.java
@@ -32,8 +32,8 @@
 import google.registry.model.transaction.JpaTestRules.JpaIntegrationTestRule;
 import google.registry.testing.AppEngineRule;
 import java.math.BigDecimal;
-import java.util.List;
 import java.util.Optional;
+import org.joda.money.CurrencyUnit;
 import org.joda.money.Money;
 import org.junit.Rule;
 import org.junit.Test;
@@ -66,13 +66,9 @@ public void saveNew_worksSuccessfully() {
     jpaTm()
         .transact(
             () -> {
-              PremiumList persistedList =
-                  jpaTm()
-                      .getEntityManager()
-                      .createQuery(
-                          "SELECT pl FROM PremiumList pl WHERE pl.name = :name", PremiumList.class)
-                      .setParameter("name", "testname")
-                      .getSingleResult();
+              Optional<PremiumList> persistedListOpt = PremiumListDao.getLatestRevision("testname");
+              assertThat(persistedListOpt).isPresent();
+              PremiumList persistedList = persistedListOpt.get();
               assertThat(persistedList.getLabelsToPrices()).containsExactlyEntriesIn(TEST_PRICES);
               assertThat(persistedList.getCreationTimestamp())
                   .isEqualTo(jpaRule.getTxnClock().nowUtc());
@@ -81,26 +77,40 @@ public void saveNew_worksSuccessfully() {
 
   @Test
   public void update_worksSuccessfully() {
-    PremiumListDao.saveNew(
+    PremiumListDao.saveNew(PremiumList.create("testname", CurrencyUnit.USD, TEST_PRICES));
+    Optional<PremiumList> persistedList = PremiumListDao.getLatestRevision("testname");
+    assertThat(persistedList).isPresent();
+    long firstRevisionId = persistedList.get().getRevisionId();
+    PremiumListDao.update(
         PremiumList.create(
-            "testname", USD, ImmutableMap.of("firstversion", BigDecimal.valueOf(123.45))));
-    PremiumListDao.update(PremiumList.create("testname", USD, TEST_PRICES));
+            "testname",
+            CurrencyUnit.USD,
+            ImmutableMap.of(
+                "update",
+                BigDecimal.valueOf(55343.12),
+                "new",
+                BigDecimal.valueOf(0.01),
+                "silver",
+                BigDecimal.valueOf(30.03))));
     jpaTm()
         .transact(
             () -> {
-              List<PremiumList> persistedLists =
-                  jpaTm()
-                      .getEntityManager()
-                      .createQuery(
-                          "SELECT pl FROM PremiumList pl WHERE pl.name = :name ORDER BY"
-                              + " pl.revisionId",
-                          PremiumList.class)
-                      .setParameter("name", "testname")
-                      .getResultList();
-              assertThat(persistedLists).hasSize(2);
-              assertThat(persistedLists.get(1).getLabelsToPrices())
-                  .containsExactlyEntriesIn(TEST_PRICES);
-              assertThat(persistedLists.get(1).getCreationTimestamp())
+              Optional<PremiumList> updatedListOpt = PremiumListDao.getLatestRevision("testname");
+              assertThat(updatedListOpt).isPresent();
+              PremiumList updatedList = updatedListOpt.get();
+              assertThat(updatedList.getLabelsToPrices())
+                  .containsExactlyEntriesIn(
+                      ImmutableMap.of(
+                          "update",
+                          BigDecimal.valueOf(55343.12),
+                          "new",
+                          BigDecimal.valueOf(0.01),
+                          "silver",
+                          BigDecimal.valueOf(30.03)));
+              assertThat(updatedList.getCreationTimestamp())
+                  .isEqualTo(jpaRule.getTxnClock().nowUtc());
+              assertThat(updatedList.getRevisionId()).isGreaterThan(firstRevisionId);
+              assertThat(updatedList.getCreationTimestamp())
                   .isEqualTo(jpaRule.getTxnClock().nowUtc());
             });
   }
@@ -116,7 +126,7 @@ public void saveNew_throwsWhenPremiumListAlreadyExists() {
   }
 
   @Test
-  public void update_throwsWhenPremiumListDoesntExist() {
+  public void update_throwsWhenListDoesntExist() {
     IllegalArgumentException thrown =
         assertThrows(
             IllegalArgumentException.class,