Allow explicitly for null EPP resources in History objects (#790)

* Allow explicitly for null EPP resources in History objects

* Repo IDs should always be nonnull

* Add a test to verify loading / comparison of legacy HistoryEntry objects

* Format javadoc + annotations

* More javadoc changes

* V52 -> V56

* V56 -> V57

* saveNew -> insert in new tests

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

@@ -62,23 +62,32 @@ public abstract class EppResource extends BackupGroupRoot implements Buildable {
   /**
    * Unique identifier in the registry for this resource.
    *
+   * <p>Not persisted so that we can store these in references to other objects. Subclasses that
+   * wish to use this as the primary key should create a getter method annotated with @Id
+   *
    * <p>This is in the (\w|_){1,80}-\w{1,8} format specified by RFC 5730 for roidType.
    *
    * @see <a href="https://tools.ietf.org/html/rfc5730">RFC 5730</a>
    */
-  @Id
-  // not persisted so that we can store these in references to other objects. Subclasses that wish
-  // to use this as the primary key should create a getter method annotated with @Id
-  @Transient
-  String repoId;
+  @Id @Transient String repoId;
 
-  /** The ID of the registrar that is currently sponsoring this resource. */
+  /**
+   * The ID of the registrar that is currently sponsoring this resource.
+   *
+   * <p>This can be null in the case of pre-Registry-3.0-migration history objects with null
+   * resource fields.
+   */
   @Index
-  @Column(name = "currentSponsorRegistrarId", nullable = false)
+  @Column(name = "currentSponsorRegistrarId")
   String currentSponsorClientId;
 
-  /** The ID of the registrar that created this resource. */
-  @Column(name = "creationRegistrarId", nullable = false)
+  /**
+   * The ID of the registrar that created this resource.
+   *
+   * <p>This can be null in the case of pre-Registry-3.0-migration history objects with null
+   * resource fields.
+   */
+  @Column(name = "creationRegistrarId")
   String creationClientId;
 
   /**
@@ -91,13 +100,17 @@ public abstract class EppResource extends BackupGroupRoot implements Buildable {
   @Column(name = "lastEppUpdateRegistrarId")
   String lastEppUpdateClientId;
 
-  /** The time when this resource was created. */
-  // Map the method to XML, not the field, because if we map the field (with an adaptor class) it
-  // will never be omitted from the xml even if the timestamp inside creationTime is null and we
-  // return null from the adaptor. (Instead it gets written as an empty tag.)
-  @Column(nullable = false)
-  @Index
-  CreateAutoTimestamp creationTime = CreateAutoTimestamp.create(null);
+  /**
+   * The time when this resource was created.
+   *
+   * <p>Map the method to XML, not the field, because if we map the field (with an adaptor class) it
+   * will never be omitted from the xml even if the timestamp inside creationTime is null and we
+   * return null from the adaptor (instead it gets written as an empty tag).
+   *
+   * <p>This can be null in the case of pre-Registry-3.0-migration history objects with null
+   * resource fields.
+   */
+  @Index CreateAutoTimestamp creationTime = CreateAutoTimestamp.create(null);
 
   /**
    * The time when this resource was or will be deleted.
@@ -112,8 +125,7 @@ public abstract class EppResource extends BackupGroupRoot implements Buildable {
    * out of the index at that time, as long as we query for resources whose deletion time is before
    * now.
    */
-  @Index
-  DateTime deletionTime;
+  @Index DateTime deletionTime;
 
   /**
    * The time that this resource was last updated.
@@ -144,7 +156,7 @@ public abstract class EppResource extends BackupGroupRoot implements Buildable {
     return repoId;
   }
 
-  // Hibernate needs this to populate the repo ID, but no one else should ever use it
+  /** This method exists solely to satisfy Hibernate. Use {@link Builder} instead. */
   @SuppressWarnings("UnusedMethod")
   private void setRepoId(String repoId) {
     this.repoId = repoId;

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

@@ -19,6 +19,8 @@ import com.googlecode.objectify.annotation.EntitySubclass;
 import google.registry.model.EppResource;
 import google.registry.model.reporting.HistoryEntry;
 import google.registry.persistence.VKey;
+import java.util.Optional;
+import javax.annotation.Nullable;
 import javax.persistence.Access;
 import javax.persistence.AccessType;
 import javax.persistence.Column;
@@ -26,6 +28,7 @@ import javax.persistence.Entity;
 import javax.persistence.GeneratedValue;
 import javax.persistence.GenerationType;
 import javax.persistence.Id;
+import javax.persistence.PostLoad;
 
 /**
  * A persisted history entry representing an EPP modification to a contact.
@@ -47,7 +50,7 @@ import javax.persistence.Id;
 public class ContactHistory extends HistoryEntry {
 
   // Store ContactBase instead of ContactResource so we don't pick up its @Id
-  ContactBase contactBase;
+  @Nullable ContactBase contactBase;
 
   @Column(nullable = false)
   VKey<ContactResource> contactRepoId;
@@ -61,9 +64,14 @@ public class ContactHistory extends HistoryEntry {
     return super.getId();
   }
 
-  /** The state of the {@link ContactBase} object at this point in time. */
-  public ContactBase getContactBase() {
-    return contactBase;
+  /**
+   * The values of all the fields on the {@link ContactBase} object after the action represented by
+   * this history object was executed.
+   *
+   * <p>Will be absent for objects created prior to the Registry 3.0 SQL migration.
+   */
+  public Optional<ContactBase> getContactBase() {
+    return Optional.ofNullable(contactBase);
   }
 
   /** The key to the {@link ContactResource} this is based off of. */
@@ -71,6 +79,20 @@ public class ContactHistory extends HistoryEntry {
     return contactRepoId;
   }
 
+  @PostLoad
+  void postLoad() {
+    // Normally Hibernate would see that the contact fields are all null and would fill contactBase
+    // with a null object. Unfortunately, the updateTimestamp is never null in SQL.
+    if (contactBase != null && contactBase.getContactId() == null) {
+      contactBase = null;
+    }
+    // Fill in the full, symmetric, parent repo ID key
+    Key<ContactResource> parentKey =
+        Key.create(ContactResource.class, (String) contactRepoId.getSqlKey());
+    parent = parentKey;
+    contactRepoId = VKey.create(ContactResource.class, contactRepoId.getSqlKey(), parentKey);
+  }
+
   @Override
   public Builder asBuilder() {
     return new Builder(clone(this));

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

@@ -27,6 +27,7 @@ import google.registry.model.reporting.DomainTransactionRecord;
 import google.registry.model.reporting.HistoryEntry;
 import google.registry.persistence.VKey;
 import java.io.Serializable;
+import java.util.Optional;
 import java.util.Set;
 import javax.annotation.Nullable;
 import javax.persistence.Access;
@@ -69,7 +70,7 @@ import javax.persistence.Table;
 public class DomainHistory extends HistoryEntry {
 
   // Store DomainContent instead of DomainBase so we don't pick up its @Id
-  DomainContent domainContent;
+  @Nullable DomainContent domainContent;
 
   @Id String domainRepoId;
 
@@ -140,9 +141,14 @@ public class DomainHistory extends HistoryEntry {
     return nsHosts;
   }
 
-  /** The state of the {@link DomainContent} object at this point in time. */
-  public DomainContent getDomainContent() {
-    return domainContent;
+  /**
+   * The values of all the fields on the {@link DomainContent} object after the action represented
+   * by this history object was executed.
+   *
+   * <p>Will be absent for objects created prior to the Registry 3.0 SQL migration.
+   */
+  public Optional<DomainContent> getDomainContent() {
+    return Optional.ofNullable(domainContent);
   }
 
   /** The key to the {@link DomainBase} this is based off of. */
@@ -158,7 +164,13 @@ public class DomainHistory extends HistoryEntry {
   void postLoad() {
     if (domainContent != null) {
       domainContent.nsHosts = nullToEmptyImmutableCopy(nsHosts);
+      // Normally Hibernate would see that the domain fields are all null and would fill
+      // domainContent with a null object. Unfortunately, the updateTimestamp is never null in SQL.
+      if (domainContent.getDomainName() == null) {
+        domainContent = null;
+      }
     }
+    parent = Key.create(DomainBase.class, domainRepoId);
   }
 
   /** Class to represent the composite primary key of {@link DomainHistory} entity. */

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

@@ -19,6 +19,8 @@ import com.googlecode.objectify.annotation.EntitySubclass;
 import google.registry.model.EppResource;
 import google.registry.model.reporting.HistoryEntry;
 import google.registry.persistence.VKey;
+import java.util.Optional;
+import javax.annotation.Nullable;
 import javax.persistence.Access;
 import javax.persistence.AccessType;
 import javax.persistence.Column;
@@ -26,6 +28,7 @@ import javax.persistence.Entity;
 import javax.persistence.GeneratedValue;
 import javax.persistence.GenerationType;
 import javax.persistence.Id;
+import javax.persistence.PostLoad;
 
 /**
  * A persisted history entry representing an EPP modification to a host.
@@ -48,7 +51,7 @@ import javax.persistence.Id;
 public class HostHistory extends HistoryEntry {
 
   // Store HostBase instead of HostResource so we don't pick up its @Id
-  HostBase hostBase;
+  @Nullable HostBase hostBase;
 
   @Column(nullable = false)
   VKey<HostResource> hostRepoId;
@@ -62,9 +65,14 @@ public class HostHistory extends HistoryEntry {
     return super.getId();
   }
 
-  /** The state of the {@link HostBase} object at this point in time. */
-  public HostBase getHostBase() {
-    return hostBase;
+  /**
+   * The values of all the fields on the {@link HostBase} object after the action represented by
+   * this history object was executed.
+   *
+   * <p>Will be absent for objects created prior to the Registry 3.0 SQL migration.
+   */
+  public Optional<HostBase> getHostBase() {
+    return Optional.ofNullable(hostBase);
   }
 
   /** The key to the {@link google.registry.model.host.HostResource} this is based off of. */
@@ -72,6 +80,19 @@ public class HostHistory extends HistoryEntry {
     return hostRepoId;
   }
 
+  @PostLoad
+  void postLoad() {
+    // Normally Hibernate would see that the host fields are all null and would fill hostBase
+    // with a null object. Unfortunately, the updateTimestamp is never null in SQL.
+    if (hostBase != null && hostBase.getHostName() == null) {
+      hostBase = null;
+    }
+    // Fill in the full, symmetric, parent repo ID key
+    Key<HostResource> parentKey = Key.create(HostResource.class, (String) hostRepoId.getSqlKey());
+    parent = parentKey;
+    hostRepoId = VKey.create(HostResource.class, hostRepoId.getSqlKey(), parentKey);
+  }
+
   @Override
   public Builder asBuilder() {
     return new Builder(clone(this));

core/src/main/java/google/registry/model/reporting/HistoryEntry.java

@@ -178,7 +178,7 @@ public class HistoryEntry extends ImmutableObject implements Buildable, Datastor
   boolean bySuperuser;
 
   /** Reason for the change. */
-  @Column(nullable = false, name = "historyReason")
+  @Column(name = "historyReason")
   String reason;
 
   /** Whether this change was requested by a registrar. */
@@ -364,7 +364,10 @@ public class HistoryEntry extends ImmutableObject implements Buildable, Datastor
       setBySuperuser(historyEntry.bySuperuser);
       setReason(historyEntry.reason);
       setRequestedByRegistrar(historyEntry.requestedByRegistrar);
-      setDomainTransactionRecords(nullToEmptyImmutableCopy(historyEntry.domainTransactionRecords));
+      setDomainTransactionRecords(
+          historyEntry.domainTransactionRecords == null
+              ? null
+              : ImmutableSet.copyOf(historyEntry.domainTransactionRecords));
       return thisCastToDerived();
     }
 

core/src/main/java/google/registry/persistence/converter/CreateAutoTimestampConverter.java

@@ -32,7 +32,11 @@ public class CreateAutoTimestampConverter
     implements AttributeConverter<CreateAutoTimestamp, Timestamp> {
 
   @Override
-  public Timestamp convertToDatabaseColumn(CreateAutoTimestamp entity) {
+  @Nullable
+  public Timestamp convertToDatabaseColumn(@Nullable CreateAutoTimestamp entity) {
+    if (entity == null) {
+      return null;
+    }
     DateTime dateTime = firstNonNull(entity.getTimestamp(), jpaTm().getTransactionTime());
     return Timestamp.from(DateTimeUtils.toZonedDateTime(dateTime).toInstant());
   }