Display changes when updating reserved list (#1093)

* add stageEntityChange to show diff

* add test cases

core/src/main/java/google/registry/tools/MutatingCommand.java

@@ -55,7 +55,9 @@ public abstract class MutatingCommand extends ConfirmingCommand implements Comma
 
     /** The possible types of mutation that can be performed on an entity. */
     public enum ChangeType {
-      CREATE, DELETE, UPDATE;
+      CREATE,
+      DELETE,
+      UPDATE;
 
       /** Return the ChangeType corresponding to the given combination of version existences. */
       public static ChangeType get(boolean hasOldVersion, boolean hasNewVersion) {
@@ -78,7 +80,7 @@ public abstract class MutatingCommand extends ConfirmingCommand implements Comma
     /** The key that points to the entity being changed. */
     final VKey<?> key;
 
-    public EntityChange(ImmutableObject oldEntity, ImmutableObject newEntity) {
+    private EntityChange(ImmutableObject oldEntity, ImmutableObject newEntity) {
       type = ChangeType.get(oldEntity != null, newEntity != null);
       checkArgument(
           type != ChangeType.UPDATE || Key.create(oldEntity).equals(Key.create(newEntity)),
@@ -96,6 +98,34 @@ public abstract class MutatingCommand extends ConfirmingCommand implements Comma
               : VKey.createOfy(entity.getClass(), Key.create(entity));
     }
 
+    /**
+     * EntityChange constructor that supports Vkey override. A Vkey is a key of an entity. This is a
+     * workaround to handle cases when a SqlEntity instance does not have a primary key before being
+     * persisted.
+     */
+    private EntityChange(ImmutableObject oldEntity, ImmutableObject newEntity, VKey<?> vkey) {
+      type = ChangeType.get(oldEntity != null, newEntity != null);
+      Key<?> oldKey = Key.create(oldEntity), newKey = Key.create(newEntity);
+      if (type == ChangeType.UPDATE) {
+        checkArgument(
+            oldKey.equals(newKey), "Both entity versions in an update must have the same Key.");
+        checkArgument(
+            oldKey.equals(vkey.getOfyKey()),
+            "The Key of the entity must be the same as the OfyKey of the vkey");
+      } else if (type == ChangeType.CREATE) {
+        checkArgument(
+            newKey.equals(vkey.getOfyKey()),
+            "Both entity versions in an update must have the same Key.");
+      } else if (type == ChangeType.DELETE) {
+        checkArgument(
+            oldKey.equals(vkey.getOfyKey()),
+            "The Key of the entity must be the same as the OfyKey of the vkey");
+      }
+      this.oldEntity = oldEntity;
+      this.newEntity = newEntity;
+      key = vkey;
+    }
+
     /** Returns a human-readable ID string for the entity being changed. */
     public String getEntityId() {
       return String.format(
@@ -110,7 +140,8 @@ public abstract class MutatingCommand extends ConfirmingCommand implements Comma
     public String toString() {
       String changeText;
       if (type == ChangeType.UPDATE) {
-        String diffText = prettyPrintEntityDeepDiff(
+        String diffText =
+            prettyPrintEntityDeepDiff(
                 oldEntity.toDiffableFieldMap(), newEntity.toDiffableFieldMap());
         changeText = Optional.ofNullable(emptyToNull(diffText)).orElse("[no changes]\n");
       } else {
@@ -205,8 +236,8 @@ public abstract class MutatingCommand extends ConfirmingCommand implements Comma
   }
 
   /**
-   * Subclasses can call this to stage a mutation to an entity that will be applied by execute().
+   * Stages an entity change that will be applied by execute(). Both ImmutableObject instances must
-   * Note that both objects passed must correspond to versions of the same entity with the same key.
+   * be some version of the same entity with the same key.
    *
    * @param oldEntity the existing version of the entity, or null to create a new entity
    * @param newEntity the new version of the entity to save, or null to delete the entity
@@ -222,6 +253,25 @@ public abstract class MutatingCommand extends ConfirmingCommand implements Comma
     lastAddedKey = change.key;
   }
 
+  /**
+   * Stages an entity change which will be applied by execute(), with the support of Vkey override.
+   * It supports cases of SqlEntity instances that do not have primary keys before being persisted.
+   *
+   * @param oldEntity the existing version of the entity, or null to create a new entity
+   * @param newEntity the new version of the entity to save, or null to delete the entity
+   * @param vkey the key of the entity
+   */
+  protected void stageEntityChange(
+      @Nullable ImmutableObject oldEntity, @Nullable ImmutableObject newEntity, VKey vkey) {
+    EntityChange change = new EntityChange(oldEntity, newEntity, vkey);
+    checkArgument(
+        !changedEntitiesMap.containsKey(change.key),
+        "Cannot apply multiple changes for the same entity: %s",
+        change.getEntityId());
+    changedEntitiesMap.put(change.key, change);
+    lastAddedKey = change.key;
+  }
+
   /**
    * Subclasses can call this to write out all previously requested entity changes since the last
    * transaction flush in a transaction.

core/src/main/java/google/registry/tools/UpdateReservedListCommand.java

@@ -14,17 +14,17 @@
 
 package google.registry.tools;
 
-import static com.google.common.base.Preconditions.checkArgument;
 import static google.registry.util.ListNamingUtils.convertFilePathToName;
 import static java.nio.charset.StandardCharsets.UTF_8;
 
 import com.beust.jcommander.Parameters;
 import com.google.common.base.Strings;
+import com.googlecode.objectify.Key;
 import google.registry.model.registry.label.ReservedList;
+import google.registry.persistence.VKey;
 import google.registry.util.SystemClock;
 import java.nio.file.Files;
 import java.util.List;
-import java.util.Optional;
 import org.joda.time.DateTime;
 
 /** Command to safely update {@link ReservedList} on Datastore. */
@@ -34,20 +34,38 @@ final class UpdateReservedListCommand extends CreateOrUpdateReservedListCommand
   @Override
   protected void init() throws Exception {
     name = Strings.isNullOrEmpty(name) ? convertFilePathToName(input) : name;
-    Optional<ReservedList> existing = ReservedList.get(name);
+    ReservedList existingReservedList =
-    checkArgument(
+        ReservedList.get(name)
-        existing.isPresent(), "Could not update reserved list %s because it doesn't exist.", name);
+            .orElseThrow(
+                () ->
+                    new IllegalArgumentException(
+                        String.format(
+                            "Could not update reserved list %s because it doesn't exist.", name)));
     boolean shouldPublish =
-        this.shouldPublish == null ? existing.get().getShouldPublish() : this.shouldPublish;
+        this.shouldPublish == null ? existingReservedList.getShouldPublish() : this.shouldPublish;
     List<String> allLines = Files.readAllLines(input, UTF_8);
     DateTime now = new SystemClock().nowUtc();
     ReservedList.Builder updated =
-        existing
+        existingReservedList
-            .get()
             .asBuilder()
             .setReservedListMapFromLines(allLines)
             .setLastUpdateTime(now)
             .setShouldPublish(shouldPublish);
     reservedList = updated.build();
+    // only call stageEntityChange if there are changes in entries
+
+    if (!existingReservedList
+        .getReservedListEntries()
+        .equals(reservedList.getReservedListEntries())) {
+      // calls the stageEntityChange method that takes old entity, new entity and a new vkey;
+      // a vkey has to be created here explicitly for ReservedList instances.
+      // ReservedList is a sqlEntity; it triggers the static method Vkey.create(Key<?> ofyCall),
+      // which invokes a static ReservedList.createVkey(Key ofyKey) method that does not exist.
+      // the sql primary key field (revisionId) is only set when it's being persisted;
+      stageEntityChange(
+          existingReservedList,
+          reservedList,
+          VKey.createOfy(ReservedList.class, Key.create(existingReservedList)));
+    }
   }
 }

core/src/test/java/google/registry/tools/UpdateReservedListCommandTest.java

@@ -18,14 +18,19 @@ 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.persistReservedList;
+import static google.registry.testing.TestDataHelper.loadFile;
 import static google.registry.util.DateTimeUtils.START_OF_TIME;
+import static java.nio.charset.StandardCharsets.UTF_8;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
+import com.google.common.io.Files;
 import google.registry.model.registry.label.ReservedList;
 import google.registry.model.registry.label.ReservedList.ReservedListEntry;
 import google.registry.model.registry.label.ReservedListSqlDao;
+import java.io.File;
+import java.nio.file.Paths;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 
@@ -135,4 +140,38 @@ class UpdateReservedListCommandTest
     verifyXnq9jyb4cInDatastore();
     assertThat(ReservedListSqlDao.checkExists("xn--q9jyb4c_common-reserved")).isTrue();
   }
+
+  @Test
+  void testSuccess_noChanges() throws Exception {
+    File reservedTermsFile = tmpDir.resolve("xn--q9jyb4c_common-reserved.txt").toFile();
+    // after running runCommandForced, the file now contains "helicopter,FULLY_BLOCKED" which is
+    // populated in the @BeforeEach method of this class and the rest of terms from
+    // example_reserved_terms.csv, which are populated in the @BeforeEach of
+    // CreateOrUpdateReservedListCommandTestCases.java.
+    runCommandForced("--name=xn--q9jyb4c_common-reserved", "--input=" + reservedTermsPath);
+
+    // set up to write content already in file
+    String reservedTermsCsv =
+        loadFile(CreateOrUpdateReservedListCommandTestCase.class, "example_reserved_terms.csv");
+    Files.asCharSink(reservedTermsFile, UTF_8).write(reservedTermsCsv);
+    reservedTermsPath = reservedTermsFile.getPath();
+    // create a command instance and assign its input
+    UpdateReservedListCommand command = new UpdateReservedListCommand();
+    command.input = Paths.get(reservedTermsPath);
+    // run again with terms from example_reserved_terms.csv
+    command.init();
+
+    assertThat(command.prompt()).isEqualTo("No entity changes to apply.");
+  }
+
+  @Test
+  void testSuccess_withChanges() throws Exception {
+    // changes come from example_reserved_terms.csv, which are populated in @BeforeEach of
+    // CreateOrUpdateReservedListCommandTestCases.java
+    UpdateReservedListCommand command = new UpdateReservedListCommand();
+    command.input = Paths.get(reservedTermsPath);
+    command.init();
+
+    assertThat(command.prompt()).contains("Update ReservedList@xn--q9jyb4c_common-reserved");
+  }
 }