diff --git a/java/google/registry/tools/MutatingCommand.java b/java/google/registry/tools/MutatingCommand.java index f42754751..67d07331d 100644 --- a/java/google/registry/tools/MutatingCommand.java +++ b/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()), - getEntityId(), - type == ChangeType.UPDATE - ? Optional - .fromNullable(emptyToNull(prettyPrintDeepDiff( - oldEntity.toDiffableFieldMap(), newEntity.toDiffableFieldMap()))) - .or("[no changes]\n") - : (MoreObjects.firstNonNull(oldEntity, newEntity) + "\n")); + UPPER_UNDERSCORE.to(UPPER_CAMEL, type.toString()), getEntityId(), changeText); } } @@ -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() diff --git a/java/google/registry/ui/server/registrar/RegistrarServlet.java b/java/google/registry/ui/server/registrar/RegistrarServlet.java index f268ad5de..bce21d16a 100644 --- a/java/google/registry/ui/server/registrar/RegistrarServlet.java +++ b/java/google/registry/ui/server/registrar/RegistrarServlet.java @@ -139,7 +139,7 @@ public class RegistrarServlet extends ResourceServlet { String registrarName, Map existingRegistrar, Map updatedRegistrar) { - Map diffs = DiffUtils.deepDiff(existingRegistrar, updatedRegistrar); + Map diffs = DiffUtils.deepDiff(existingRegistrar, updatedRegistrar, true); @SuppressWarnings("unchecked") Set changedKeys = (Set) diffs.keySet(); if (CollectionUtils.difference(changedKeys, "lastUpdateTime").isEmpty()) { diff --git a/java/google/registry/util/DiffUtils.java b/java/google/registry/util/DiffUtils.java index 7ce313e76..e7efcdb4c 100644 --- a/java/google/registry/util/DiffUtils.java +++ b/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. */ - public static String prettyPrintDeepDiff(Map a, Map b) { - return prettyPrintDiffedMap(deepDiff(a, b), null); + /** Pretty-prints a deep diff between two maps that represent Datastore entities. */ + public static String prettyPrintEntityDeepDiff(Map a, Map b) { + 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) { - return prettyPrintDiffedMap(deepDiff(a, b), path); + public static String prettyPrintXmlDeepDiff(Map a, Map b, @Nullable String 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 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)); } } diff --git a/javatests/google/registry/testing/TaskQueueHelper.java b/javatests/google/registry/testing/TaskQueueHelper.java index 020e2987b..1dc126c1b 100644 --- a/javatests/google/registry/testing/TaskQueueHelper.java +++ b/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() { @Override public String apply(TaskStateInfo input) { - return prettyPrintDeepDiff( + return prettyPrintEntityDeepDiff( taskMatcherMap, Maps.filterKeys( new MatchableTaskInfo(input).toMap(), diff --git a/javatests/google/registry/util/DiffUtilsTest.java b/javatests/google/registry/util/DiffUtilsTest.java index 71362836b..0f0320603 100644 --- a/javatests/google/registry/util/DiffUtilsTest.java +++ b/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 mapA = new HashMap(); + mapA.put("a", "jim"); + mapA.put("b", null); + Map mapB = new HashMap(); + 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"); diff --git a/javatests/google/registry/xml/XmlTestUtils.java b/javatests/google/registry/xml/XmlTestUtils.java index 0b9aa3b23..c0e566a35 100644 --- a/javatests/google/registry/xml/XmlTestUtils.java +++ b/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))); } }