Improve pretty-print diffing of Datastore entities

This removes the countless lines of the form "[null, []]" in registry_tool diffs
that are an artifact of the way we handle nulls in Objectify.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=133409440

java/google/registry/tools/MutatingCommand.java

@@ -23,7 +23,7 @@ import static com.google.common.base.Preconditions.checkState;
 import static com.google.common.base.Strings.emptyToNull;
 import static google.registry.model.ofy.ObjectifyService.ofy;
 import static google.registry.util.DatastoreServiceUtils.getNameOrId;
-import static google.registry.util.DiffUtils.prettyPrintDeepDiff;
+import static google.registry.util.DiffUtils.prettyPrintEntityDeepDiff;
 
 import com.google.common.base.Joiner;
 import com.google.common.base.MoreObjects;
@@ -101,16 +101,17 @@ public abstract class MutatingCommand extends ConfirmingCommand implements Remot
     /** Returns a string representation of this entity change. */
     @Override
     public String toString() {
+      String changeText;
+      if (type == ChangeType.UPDATE) {
+        String diffText = prettyPrintEntityDeepDiff(
+            oldEntity.toDiffableFieldMap(), newEntity.toDiffableFieldMap());
+        changeText = Optional.fromNullable(emptyToNull(diffText)).or("[no changes]\n");
+      } else {
+        changeText = MoreObjects.firstNonNull(oldEntity, newEntity) + "\n";
+      }
       return String.format(
           "%s %s\n%s",
-          UPPER_UNDERSCORE.to(UPPER_CAMEL, type.toString()),
+          UPPER_UNDERSCORE.to(UPPER_CAMEL, type.toString()), getEntityId(), changeText);
-          getEntityId(),
-          type == ChangeType.UPDATE
-              ? Optional
-                  .fromNullable(emptyToNull(prettyPrintDeepDiff(
-                      oldEntity.toDiffableFieldMap(), newEntity.toDiffableFieldMap())))
-                  .or("[no changes]\n")
-              : (MoreObjects.firstNonNull(oldEntity, newEntity) + "\n"));
     }
   }
 
@@ -153,7 +154,7 @@ public abstract class MutatingCommand extends ConfirmingCommand implements Remot
             checkState(
                 Objects.equals(change.oldEntity, existingEntity),
                 "Entity changed since init() was called.\n%s",
-                prettyPrintDeepDiff(
+                prettyPrintEntityDeepDiff(
                     change.oldEntity == null ? ImmutableMap.of()
                         : change.oldEntity.toDiffableFieldMap(),
                     existingEntity == null ? ImmutableMap.of()

java/google/registry/ui/server/registrar/RegistrarServlet.java

@@ -139,7 +139,7 @@ public class RegistrarServlet extends ResourceServlet {
       String registrarName,
       Map<String, Object> existingRegistrar,
       Map<String, Object> updatedRegistrar) {
-    Map<?, ?> diffs = DiffUtils.deepDiff(existingRegistrar, updatedRegistrar);
+    Map<?, ?> diffs = DiffUtils.deepDiff(existingRegistrar, updatedRegistrar, true);
     @SuppressWarnings("unchecked")
     Set<String> changedKeys = (Set<String>) diffs.keySet();
     if (CollectionUtils.difference(changedKeys, "lastUpdateTime").isEmpty()) {

java/google/registry/util/DiffUtils.java

@@ -26,6 +26,7 @@ import com.google.common.collect.Iterables;
 import com.google.common.collect.Ordering;
 import com.google.common.collect.Sets;
 import com.google.common.primitives.Primitives;
+import java.util.Collection;
 import java.util.Map;
 import java.util.Objects;
 import java.util.Set;
@@ -57,25 +58,37 @@ public final class DiffUtils {
     }
   }
 
-  /** Pretty-prints a deep diff between two maps. */
+  /** Pretty-prints a deep diff between two maps that represent Datastore entities. */
-  public static String prettyPrintDeepDiff(Map<?, ?> a, Map<?, ?> b) {
+  public static String prettyPrintEntityDeepDiff(Map<?, ?> a, Map<?, ?> b) {
-    return prettyPrintDiffedMap(deepDiff(a, b), null);
+    return prettyPrintDiffedMap(deepDiff(a, b, true), null);
   }
 
   /**
-   * Pretty-prints a deep diff between two maps. Path is prefixed to each output line of the diff.
+   * Pretty-prints a deep diff between two maps that represent XML documents. Path is prefixed to
+   * each output line of the diff.
    */
-  public static String prettyPrintDeepDiff(Map<?, ?> a, Map<?, ?> b, @Nullable String path) {
+  public static String prettyPrintXmlDeepDiff(Map<?, ?> a, Map<?, ?> b, @Nullable String path) {
-    return prettyPrintDiffedMap(deepDiff(a, b), path);
+    return prettyPrintDiffedMap(deepDiff(a, b, false), path);
   }
 
   /** Compare two maps and return a map containing, at each key where they differed, both values. */
-  public static ImmutableMap<?, ?> deepDiff(Map<?, ?> a, Map<?, ?> b) {
+  public static ImmutableMap<?, ?> deepDiff(
+      Map<?, ?> a, Map<?, ?> b, boolean ignoreNullToCollection) {
     ImmutableMap.Builder<Object, Object> diff = new ImmutableMap.Builder<>();
     for (Object key : Sets.union(a.keySet(), b.keySet())) {
       Object aValue = a.get(key);
       Object bValue = b.get(key);
-      if (!Objects.equals(aValue, bValue)) {
+      if (Objects.equals(aValue, bValue)) {
+        // The objects are equal, so print nothing.
+      } else if (ignoreNullToCollection
+          && aValue == null
+          && bValue instanceof Collection
+          && ((Collection<?>) bValue).isEmpty()) {
+        // Ignore a mismatch between Objectify's use of null to store empty collections and our
+        // code's builder methods, which yield empty collections for the same fields.  This
+        // prevents useless lines of the form "[null, []]" from appearing in diffs.
+      } else {
+        // The objects aren't equal, so output a diff.
         if (aValue instanceof String && bValue instanceof String
             && a.toString().contains("\n") && b.toString().contains("\n")) {
           aValue = stringToMap((String) aValue);
@@ -87,7 +100,7 @@ public final class DiffUtils {
           bValue = iterableToSortedMap((Iterable<?>) bValue);
         }
         diff.put(key, (aValue instanceof Map && bValue instanceof Map)
-            ? deepDiff((Map<?, ?>) aValue, (Map<?, ?>) bValue)
+            ? deepDiff((Map<?, ?>) aValue, (Map<?, ?>) bValue, ignoreNullToCollection)
             : new DiffPair(aValue, bValue));
       }
     }

javatests/google/registry/testing/TaskQueueHelper.java

@@ -23,7 +23,7 @@ import static com.google.common.collect.Iterables.transform;
 import static com.google.common.collect.Multisets.containsOccurrences;
 import static com.google.common.truth.Truth.assertThat;
 import static com.google.common.truth.Truth.assert_;
-import static google.registry.util.DiffUtils.prettyPrintDeepDiff;
+import static google.registry.util.DiffUtils.prettyPrintEntityDeepDiff;
 import static java.nio.charset.StandardCharsets.UTF_8;
 import static java.util.Arrays.asList;
 
@@ -227,7 +227,7 @@ public class TaskQueueHelper {
                 .transform(new Function<TaskStateInfo, String>() {
                     @Override
                     public String apply(TaskStateInfo input) {
-                      return prettyPrintDeepDiff(
+                      return prettyPrintEntityDeepDiff(
                           taskMatcherMap,
                           Maps.filterKeys(
                               new MatchableTaskInfo(input).toMap(),

javatests/google/registry/util/DiffUtilsTest.java

@@ -15,9 +15,12 @@
 package google.registry.util;
 
 import static com.google.common.truth.Truth.assertThat;
+import static google.registry.util.DiffUtils.prettyPrintEntityDeepDiff;
 import static google.registry.util.DiffUtils.prettyPrintSetDiff;
 
 import com.google.common.collect.ImmutableSet;
+import java.util.HashMap;
+import java.util.Map;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
@@ -57,6 +60,18 @@ public class DiffUtilsTest {
         .isEqualTo("\n    ADDED: [y, z]\n    REMOVED: [b, c]\n    FINAL CONTENTS: [a, y, z]");
   }
 
+  @Test
+  public void test_emptyToNullCollection_doesntDisplay() {
+    Map<String, Object> mapA = new HashMap<String, Object>();
+    mapA.put("a", "jim");
+    mapA.put("b", null);
+    Map<String, Object> mapB = new HashMap<String, Object>();
+    mapB.put("a", "tim");
+    mapB.put("b", ImmutableSet.of());
+    // This ensures that it is not outputting a diff of [b -> [null, []].
+    assertThat(prettyPrintEntityDeepDiff(mapA, mapB)).isEqualTo("a -> [jim, tim]\n");
+  }
+
   @Test
   public void test_prettyPrintSetDiff_addedAndRemovedElements_objects() {
     DummyObject a = DummyObject.create("a");

javatests/google/registry/xml/XmlTestUtils.java

@@ -16,7 +16,7 @@ package google.registry.xml;
 
 import static com.google.common.base.Preconditions.checkNotNull;
 import static com.google.common.truth.Truth.assert_;
-import static google.registry.util.DiffUtils.prettyPrintDeepDiff;
+import static google.registry.util.DiffUtils.prettyPrintXmlDeepDiff;
 import static org.joda.time.DateTimeZone.UTC;
 
 import com.google.common.base.Splitter;
@@ -59,7 +59,7 @@ public class XmlTestUtils {
           message,
           expected,
           actual,
-          prettyPrintDeepDiff(expectedMap, actualMap, null)));
+          prettyPrintXmlDeepDiff(expectedMap, actualMap, null)));
     }
   }