From d7f7568761f38f174967b1d49c88fcd85554610c Mon Sep 17 00:00:00 2001
From: Michael Muller <mmuller@google.com>
Date: Fri, 4 Jun 2021 11:38:42 -0400
Subject: [PATCH] Fix copy causing premature hash calculation (#1189)

* Fix copy causing premature hash calculation

The creation of a builder to set the DomainContent repo id in DomainHistory
triggers an equality check which causes the hash code of an associated
transfer data object to be calculated prematurely, before the Ofy keys are
reconstituted.  Replace this with a simple setter, which is acceptible in this
case because the object is being loaded and is considered to be not fully
constructed yet.

* Do setRepoId() in Contact and Host history

Not essential for these as far as we know, but it's safer and more consistent.

* Fixed typos
---
 .../java/google/registry/model/EppResource.java     | 13 ++++++++++---
 .../registry/model/contact/ContactHistory.java      |  5 ++++-
 .../google/registry/model/domain/DomainHistory.java |  5 ++++-
 .../google/registry/model/host/HostHistory.java     |  5 ++++-
 .../registry/model/transfer/DomainTransferData.java |  1 -
 5 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/core/src/main/java/google/registry/model/EppResource.java b/core/src/main/java/google/registry/model/EppResource.java
index 753d319d1..fda0c6879 100644
--- a/core/src/main/java/google/registry/model/EppResource.java
+++ b/core/src/main/java/google/registry/model/EppResource.java
@@ -155,9 +155,16 @@ public abstract class EppResource extends BackupGroupRoot implements Buildable {
     return repoId;
   }
 
-  /** This method exists solely to satisfy Hibernate. Use {@link Builder} instead. */
-  @SuppressWarnings("UnusedMethod")
-  private void setRepoId(String repoId) {
+  /**
+   * Sets the repository ID.
+   *
+   * <p>This should only be used for restoring the repo id of an object being loaded in a PostLoad
+   * method (effectively, when it is still under construction by Hibernate). In all other cases, the
+   * object should be regarded as immutable and changes should go through a Builder.
+   *
+   * <p>In addition to this special case use, this method must exist to satisfy Hibernate.
+   */
+  public void setRepoId(String repoId) {
     this.repoId = repoId;
   }
 
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 033a51329..61bc07119 100644
--- a/core/src/main/java/google/registry/model/contact/ContactHistory.java
+++ b/core/src/main/java/google/registry/model/contact/ContactHistory.java
@@ -115,7 +115,10 @@ public class ContactHistory extends HistoryEntry implements SqlEntity {
       contactBase = null;
     }
     if (contactBase != null && contactBase.getRepoId() == null) {
-      contactBase = contactBase.asBuilder().setRepoId(parent.getName()).build();
+      // contactBase hasn't been fully constructed yet, so it's ok to go in and mutate it.  Though
+      // the use of the Builder is not necessarily problematic in this case, this is still safer as
+      // the Builder can do things like comparisons that compute the hash code.
+      contactBase.setRepoId(parent.getName());
     }
   }
 
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 0eb6af37f..d96388f10 100644
--- a/core/src/main/java/google/registry/model/domain/DomainHistory.java
+++ b/core/src/main/java/google/registry/model/domain/DomainHistory.java
@@ -258,7 +258,10 @@ public class DomainHistory extends HistoryEntry implements SqlEntity {
         domainContent = null;
       } else {
         if (domainContent.getRepoId() == null) {
-          domainContent = domainContent.asBuilder().setRepoId(parent.getName()).build();
+          // domainContent still hasn't been fully constructed yet, so it's ok to go in and mutate
+          // it.  In fact, we have to because going through the builder causes the hash codes of
+          // contained objects to be calculated prematurely.
+          domainContent.setRepoId(parent.getName());
         }
       }
     }
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 2526073b8..0930f03c2 100644
--- a/core/src/main/java/google/registry/model/host/HostHistory.java
+++ b/core/src/main/java/google/registry/model/host/HostHistory.java
@@ -116,7 +116,10 @@ public class HostHistory extends HistoryEntry implements SqlEntity {
       hostBase = null;
     }
     if (hostBase != null && hostBase.getRepoId() == null) {
-      hostBase = hostBase.asBuilder().setRepoId(parent.getName()).build();
+      // hostBase hasn't been fully constructed yet, so it's ok to go in and mutate it.  Though the
+      // use of the Builder is not necessarily problematic in this case, this is still safer as the
+      // Builder can do things like comparisons that compute the hash code.
+      hostBase.setRepoId(parent.getName());
     }
   }
 
diff --git a/core/src/main/java/google/registry/model/transfer/DomainTransferData.java b/core/src/main/java/google/registry/model/transfer/DomainTransferData.java
index 397746243..6696bddd8 100644
--- a/core/src/main/java/google/registry/model/transfer/DomainTransferData.java
+++ b/core/src/main/java/google/registry/model/transfer/DomainTransferData.java
@@ -164,7 +164,6 @@ public class DomainTransferData extends TransferData<DomainTransferData.Builder>
       serverApproveEntities = null;
       postLoad();
     }
-    hashCode = null; // reset the hash code since we may have changed the entities
   }
 
   /**