Make ImmutableObject.toString deterministic (#1469)

* Make ImmutableObject.toString deterministic

Remove the identity hash from the output. There is no use case
(including debugging) for it.

Removing it allows us to also remove some overriding implementations in
subclasses, and may also simplify tests.

core/src/main/java/google/registry/model/ImmutableObject.java

@@ -156,10 +156,6 @@ public abstract class ImmutableObject implements Cloneable {
    *   }
    * }
    * </pre>
-   *
-   * <p>This method makes use of {@link #toStringHelper}, which embeds {@link
-   * System#identityHashCode} in the output string. Subclasses that require deterministic string
-   * representations <em>across</em> JVM instances should override this method.
    */
   @Override
   public String toString() {
@@ -185,11 +181,9 @@ public abstract class ImmutableObject implements Cloneable {
 
   public String toStringHelper(SortedMap<String, Object> fields) {
     return String.format(
-        "%s (@%s): {\n%s",
-        getClass().getSimpleName(),
-        System.identityHashCode(this),
-        Joiner.on('\n').join(fields.entrySet()))
-            .replaceAll("\n", "\n    ") + "\n}";
+                "%s: {\n%s", getClass().getSimpleName(), Joiner.on('\n').join(fields.entrySet()))
+            .replaceAll("\n", "\n    ")
+        + "\n}";
   }
 
   /** Helper function to recursively hydrate an ImmutableObject. */

core/src/main/java/google/registry/model/common/Cursor.java

@@ -19,9 +19,7 @@ import static com.google.common.base.Preconditions.checkNotNull;
 import static google.registry.model.common.EntityGroupRoot.getCrossTldKey;
 import static google.registry.util.DateTimeUtils.START_OF_TIME;
 
-import com.google.common.base.Joiner;
 import com.google.common.base.Splitter;
-import com.google.common.collect.ImmutableSortedMap;
 import com.googlecode.objectify.Key;
 import com.googlecode.objectify.annotation.Entity;
 import com.googlecode.objectify.annotation.Id;
@@ -296,22 +294,5 @@ public class Cursor extends ImmutableObject implements DatastoreAndSqlEntity, Un
       this.type = type;
       this.scope = scope;
     }
-
-    /**
-     * A deterministic string representation of a {@link CursorId}. See {@link
-     * ImmutableObject#toString} for more information.
-     */
-    @Override
-    public String toString() {
-      return String.format(
-                  "%s: {\n%s",
-                  getClass().getSimpleName(),
-                  Joiner.on('\n')
-                      .join(
-                          ImmutableSortedMap.<String, Object>of("scope", scope, "type", type)
-                              .entrySet()))
-              .replaceAll("\n", "\n    ")
-          + "\n}";
-    }
   }
 }

core/src/main/java/google/registry/model/contact/ContactHistory.java

@@ -16,8 +16,6 @@ package google.registry.model.contact;
 
 import static google.registry.persistence.transaction.TransactionManagerFactory.jpaTm;
 
-import com.google.common.base.Joiner;
-import com.google.common.collect.ImmutableSortedMap;
 import com.googlecode.objectify.Key;
 import com.googlecode.objectify.annotation.EntitySubclass;
 import google.registry.model.EppResource;
@@ -202,24 +200,6 @@ public class ContactHistory extends HistoryEntry implements SqlEntity, UnsafeSer
     private void setId(long id) {
       this.id = id;
     }
-
-    /**
-     * A deterministic string representation of a {@link ContactHistoryId}. See {@link
-     * ImmutableObject#toString} for more information.
-     */
-    @Override
-    public String toString() {
-      return String.format(
-                  "%s: {\n%s",
-                  getClass().getSimpleName(),
-                  Joiner.on('\n')
-                      .join(
-                          ImmutableSortedMap.<String, Object>of(
-                                  "contactRepoId", getContactRepoId(), "id", getId())
-                              .entrySet()))
-              .replaceAll("\n", "\n    ")
-          + "\n}";
-    }
   }
 
   @Override

core/src/main/java/google/registry/model/domain/DomainHistory.java

@@ -18,9 +18,7 @@ import static com.google.common.collect.ImmutableSet.toImmutableSet;
 import static google.registry.persistence.transaction.TransactionManagerFactory.jpaTm;
 import static google.registry.util.CollectionUtils.nullToEmptyImmutableCopy;
 
-import com.google.common.base.Joiner;
 import com.google.common.collect.ImmutableSet;
-import com.google.common.collect.ImmutableSortedMap;
 import com.googlecode.objectify.Key;
 import com.googlecode.objectify.annotation.EntitySubclass;
 import google.registry.model.EppResource;
@@ -386,24 +384,6 @@ public class DomainHistory extends HistoryEntry implements SqlEntity {
     private void setId(long id) {
       this.id = id;
     }
-
-    /**
-     * A deterministic string representation of a {@link DomainHistoryId}. See {@link
-     * ImmutableObject#toString} for more information.
-     */
-    @Override
-    public String toString() {
-      return String.format(
-                  "%s: {\n%s",
-                  getClass().getSimpleName(),
-                  Joiner.on('\n')
-                      .join(
-                          ImmutableSortedMap.<String, Object>of(
-                                  "domainRepoId", getDomainRepoId(), "id", getId())
-                              .entrySet()))
-              .replaceAll("\n", "\n    ")
-          + "\n}";
-    }
   }
 
   @Override

core/src/main/java/google/registry/model/host/HostHistory.java

@@ -16,8 +16,6 @@ package google.registry.model.host;
 
 import static google.registry.persistence.transaction.TransactionManagerFactory.jpaTm;
 
-import com.google.common.base.Joiner;
-import com.google.common.collect.ImmutableSortedMap;
 import com.googlecode.objectify.Key;
 import com.googlecode.objectify.annotation.EntitySubclass;
 import google.registry.model.EppResource;
@@ -205,24 +203,6 @@ public class HostHistory extends HistoryEntry implements SqlEntity, UnsafeSerial
     private void setId(long id) {
       this.id = id;
     }
-
-    /**
-     * A deterministic string representation of a {@link HostHistoryId}. See {@link
-     * ImmutableObject#toString} for more information.
-     */
-    @Override
-    public String toString() {
-      return String.format(
-                  "%s: {\n%s",
-                  getClass().getSimpleName(),
-                  Joiner.on('\n')
-                      .join(
-                          ImmutableSortedMap.<String, Object>of(
-                                  "hostRepoId", getHostRepoId(), "id", getId())
-                              .entrySet()))
-              .replaceAll("\n", "\n    ")
-          + "\n}";
-    }
   }
 
   @Override

core/src/main/java/google/registry/model/registrar/RegistrarContact.java

@@ -32,9 +32,7 @@ import static java.util.stream.Collectors.joining;
 
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Enums;
-import com.google.common.base.Joiner;
 import com.google.common.collect.ImmutableSet;
-import com.google.common.collect.ImmutableSortedMap;
 import com.google.common.collect.ImmutableSortedSet;
 import com.google.common.collect.Streams;
 import com.googlecode.objectify.Key;
@@ -415,24 +413,6 @@ public class RegistrarContact extends ImmutableObject
       this.emailAddress = emailAddress;
       this.registrarId = registrarId;
     }
-
-    /**
-     * A deterministic string representation of a {@link RegistrarPocId}. See {@link
-     * ImmutableObject#toString} for more information.
-     */
-    @Override
-    public String toString() {
-      return String.format(
-                  "%s: {\n%s",
-                  getClass().getSimpleName(),
-                  Joiner.on('\n')
-                      .join(
-                          ImmutableSortedMap.<String, Object>of(
-                                  "emailAddress", emailAddress, "registrarId", registrarId)
-                              .entrySet()))
-              .replaceAll("\n", "\n    ")
-          + "\n}";
-    }
   }
 
   /** A builder for constructing a {@link RegistrarContact}, since it is immutable. */

core/src/test/java/google/registry/model/ImmutableObjectTest.java

@@ -68,11 +68,8 @@ public class ImmutableObjectTest {
   @Test
   void testToString_simpleClass() {
     SimpleObject object = new SimpleObject("foo", null);
-    assertThat(object.toString()).isEqualTo(""
-        + "SimpleObject (@" + System.identityHashCode(object) + "): {\n"
-        + "    a=foo\n"
-        + "    b=null\n"
-        + "}");
+    assertThat(object.toString())
+        .isEqualTo("" + "SimpleObject: {\n" + "    a=foo\n" + "    b=null\n" + "}");
   }
 
   @Test
@@ -332,8 +329,8 @@ public class ImmutableObjectTest {
 
     // The hash code test test is implicit in "equals", it is added here just for clarity.
     assertThat(instance1.hashCode()).isEqualTo(instance2.hashCode());
-    assertThat(instance1.toString()).matches(
-        "(?s)HasInsignificantFields (.*): \\{\\s*significant=significant\\s*\\}\\s*");
+    assertThat(instance1.toString())
+        .matches("(?s)HasInsignificantFields: \\{\\s*significant=significant\\s*\\}\\s*");
   }
 
   static class HasInsignificantFields extends ImmutableObject {