From 83c6740223187ceb68d8fe942d1bff7ba2411327 Mon Sep 17 00:00:00 2001 From: Weimin Yu Date: Tue, 14 Dec 2021 15:46:05 -0500 Subject: [PATCH] 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. --- .../registry/model/ImmutableObject.java | 12 +++-------- .../google/registry/model/common/Cursor.java | 19 ------------------ .../model/contact/ContactHistory.java | 20 ------------------- .../registry/model/domain/DomainHistory.java | 20 ------------------- .../registry/model/host/HostHistory.java | 20 ------------------- .../model/registrar/RegistrarContact.java | 20 ------------------- .../registry/model/ImmutableObjectTest.java | 11 ++++------ 7 files changed, 7 insertions(+), 115 deletions(-) diff --git a/core/src/main/java/google/registry/model/ImmutableObject.java b/core/src/main/java/google/registry/model/ImmutableObject.java index fb615c714..3db9e4f8e 100644 --- a/core/src/main/java/google/registry/model/ImmutableObject.java +++ b/core/src/main/java/google/registry/model/ImmutableObject.java @@ -156,10 +156,6 @@ public abstract class ImmutableObject implements Cloneable { * } * } * - * - *

This method makes use of {@link #toStringHelper}, which embeds {@link - * System#identityHashCode} in the output string. Subclasses that require deterministic string - * representations across JVM instances should override this method. */ @Override public String toString() { @@ -185,11 +181,9 @@ public abstract class ImmutableObject implements Cloneable { public String toStringHelper(SortedMap 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. */ diff --git a/core/src/main/java/google/registry/model/common/Cursor.java b/core/src/main/java/google/registry/model/common/Cursor.java index c3fb5b85d..2aca2ad80 100644 --- a/core/src/main/java/google/registry/model/common/Cursor.java +++ b/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.of("scope", scope, "type", type) - .entrySet())) - .replaceAll("\n", "\n ") - + "\n}"; - } } } diff --git a/core/src/main/java/google/registry/model/contact/ContactHistory.java b/core/src/main/java/google/registry/model/contact/ContactHistory.java index 7a8f2061c..ab2f6de18 100644 --- a/core/src/main/java/google/registry/model/contact/ContactHistory.java +++ b/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.of( - "contactRepoId", getContactRepoId(), "id", getId()) - .entrySet())) - .replaceAll("\n", "\n ") - + "\n}"; - } } @Override diff --git a/core/src/main/java/google/registry/model/domain/DomainHistory.java b/core/src/main/java/google/registry/model/domain/DomainHistory.java index f26d028c2..e804fc42f 100644 --- a/core/src/main/java/google/registry/model/domain/DomainHistory.java +++ b/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.of( - "domainRepoId", getDomainRepoId(), "id", getId()) - .entrySet())) - .replaceAll("\n", "\n ") - + "\n}"; - } } @Override diff --git a/core/src/main/java/google/registry/model/host/HostHistory.java b/core/src/main/java/google/registry/model/host/HostHistory.java index 247f9e700..0831a65e6 100644 --- a/core/src/main/java/google/registry/model/host/HostHistory.java +++ b/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.of( - "hostRepoId", getHostRepoId(), "id", getId()) - .entrySet())) - .replaceAll("\n", "\n ") - + "\n}"; - } } @Override diff --git a/core/src/main/java/google/registry/model/registrar/RegistrarContact.java b/core/src/main/java/google/registry/model/registrar/RegistrarContact.java index 647cf4214..d4aefc041 100644 --- a/core/src/main/java/google/registry/model/registrar/RegistrarContact.java +++ b/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.of( - "emailAddress", emailAddress, "registrarId", registrarId) - .entrySet())) - .replaceAll("\n", "\n ") - + "\n}"; - } } /** A builder for constructing a {@link RegistrarContact}, since it is immutable. */ diff --git a/core/src/test/java/google/registry/model/ImmutableObjectTest.java b/core/src/test/java/google/registry/model/ImmutableObjectTest.java index a67bfc794..a84905391 100644 --- a/core/src/test/java/google/registry/model/ImmutableObjectTest.java +++ b/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 {