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
+  @Id @Transient String repoId;
-  // 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;
 
-  /** 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
+   * The time when this resource was created.
-  // 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>Map the method to XML, not the field, because if we map the field (with an adaptor class) it
-  @Column(nullable = false)
+   * will never be omitted from the xml even if the timestamp inside creationTime is null and we
-  @Index
+   * return null from the adaptor (instead it gets written as an empty tag).
-  CreateAutoTimestamp creationTime = CreateAutoTimestamp.create(null);
+   *
+   * <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
+  @Index DateTime deletionTime;
-  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() {
+   * The values of all the fields on the {@link ContactBase} object after the action represented by
-    return contactBase;
+   * 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() {
+   * The values of all the fields on the {@link DomainContent} object after the action represented
-    return domainContent;
+   * 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() {
+   * The values of all the fields on the {@link HostBase} object after the action represented by
-    return hostBase;
+   * 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());
   }

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

@@ -32,17 +32,25 @@ import javax.annotation.Nullable;
 /** Truth subject for asserting things about ImmutableObjects that are not built in. */
 public final class ImmutableObjectSubject extends Subject {
 
-  private final ImmutableObject actual;
+  @Nullable private final ImmutableObject actual;
 
-  protected ImmutableObjectSubject(FailureMetadata failureMetadata, ImmutableObject actual) {
+  protected ImmutableObjectSubject(
+      FailureMetadata failureMetadata, @Nullable ImmutableObject actual) {
     super(failureMetadata, actual);
     this.actual = actual;
   }
 
-  public void isEqualExceptFields(ImmutableObject expected, String... ignoredFields) {
+  public void isEqualExceptFields(@Nullable ImmutableObject expected, String... ignoredFields) {
-    Map<Field, Object> actualFields = filterFields(actual, ignoredFields);
+    if (actual == null) {
-    Map<Field, Object> expectedFields = filterFields(expected, ignoredFields);
+      assertThat(expected).isNull();
-    assertThat(actualFields).containsExactlyEntriesIn(expectedFields);
+    } else {
+      assertThat(expected).isNotNull();
+    }
+    if (actual != null) {
+      Map<Field, Object> actualFields = filterFields(actual, ignoredFields);
+      Map<Field, Object> expectedFields = filterFields(expected, ignoredFields);
+      assertThat(actualFields).containsExactlyEntriesIn(expectedFields);
+    }
   }
 
   public static Correspondence<ImmutableObject, ImmutableObject> immutableObjectCorrespondence(

core/src/test/java/google/registry/model/history/ContactHistoryTest.java

@@ -53,7 +53,32 @@ public class ContactHistoryTest extends EntityTestCase {
     jpaTm()
         .transact(
             () -> {
-              ContactHistory fromDatabase = jpaTm().load(VKey.createSql(ContactHistory.class, 1L));
+              ContactHistory fromDatabase =
+                  jpaTm().load(VKey.createSql(ContactHistory.class, contactHistory.getId()));
+              assertContactHistoriesEqual(fromDatabase, contactHistory);
+              assertThat(fromDatabase.getContactRepoId().getSqlKey())
+                  .isEqualTo(contactHistory.getContactRepoId().getSqlKey());
+            });
+  }
+
+  @Test
+  void testLegacyPersistence_nullContactBase() {
+    saveRegistrar("TheRegistrar");
+
+    ContactResource contact = newContactResourceWithRoid("contactId", "contact1");
+    jpaTm().transact(() -> jpaTm().insert(contact));
+    VKey<ContactResource> contactVKey = contact.createVKey();
+    ContactResource contactFromDb = jpaTm().transact(() -> jpaTm().load(contactVKey));
+    ContactHistory contactHistory =
+        createContactHistory(contactFromDb, contactVKey).asBuilder().setContactBase(null).build();
+    contactHistory.id = null;
+    jpaTm().transact(() -> jpaTm().insert(contactHistory));
+
+    jpaTm()
+        .transact(
+            () -> {
+              ContactHistory fromDatabase =
+                  jpaTm().load(VKey.createSql(ContactHistory.class, contactHistory.getId()));
               assertContactHistoriesEqual(fromDatabase, contactHistory);
               assertThat(fromDatabase.getContactRepoId().getSqlKey())
                   .isEqualTo(contactHistory.getContactRepoId().getSqlKey());
@@ -106,7 +131,7 @@ public class ContactHistoryTest extends EntityTestCase {
         .that(one)
         .isEqualExceptFields(two, "contactBase", "contactRepoId", "parent");
     assertAboutImmutableObjects()
-        .that(one.getContactBase())
+        .that(one.getContactBase().orElse(null))
-        .isEqualExceptFields(two.getContactBase(), "repoId");
+        .isEqualExceptFields(two.getContactBase().orElse(null), "repoId");
   }
 }

core/src/test/java/google/registry/model/history/DomainHistoryTest.java

@@ -40,6 +40,7 @@ import google.registry.model.reporting.DomainTransactionRecord;
 import google.registry.model.reporting.DomainTransactionRecord.TransactionReportField;
 import google.registry.model.reporting.HistoryEntry;
 import google.registry.persistence.VKey;
+import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 
 /** Tests for {@link DomainHistory}. */
@@ -49,28 +50,33 @@ public class DomainHistoryTest extends EntityTestCase {
     super(JpaEntityCoverageCheck.ENABLED);
   }
 
+  @BeforeEach
+  void beforeEach() {
+    saveRegistrar("TheRegistrar");
+  }
+
   @Test
   void testPersistence() {
-    saveRegistrar("TheRegistrar");
+    DomainBase domain = createDomainWithContactsAndHosts();
-
+    DomainHistory domainHistory = createDomainHistory(domain);
-    HostResource host = newHostResourceWithRoid("ns1.example.com", "host1");
+    domainHistory.id = null;
-    ContactResource contact = newContactResourceWithRoid("contactId", "contact1");
+    jpaTm().transact(() -> jpaTm().insert(domainHistory));
 
     jpaTm()
         .transact(
             () -> {
-              jpaTm().insert(host);
+              DomainHistory fromDatabase = jpaTm().load(domainHistory.createVKey());
-              jpaTm().insert(contact);
+              assertDomainHistoriesEqual(fromDatabase, domainHistory);
+              assertThat(fromDatabase.getDomainRepoId().getSqlKey())
+                  .isEqualTo(domainHistory.getDomainRepoId().getSqlKey());
             });
+  }
 
-    DomainBase domain =
+  @Test
-        newDomainBase("example.tld", "domainRepoId", contact)
+  void testLegacyPersistence_nullResource() {
-            .asBuilder()
+    DomainBase domain = createDomainWithContactsAndHosts();
-            .setNameservers(host.createVKey())
+    DomainHistory domainHistory =
-            .build();
+        createDomainHistory(domain).asBuilder().setDomainContent(null).build();
-    jpaTm().transact(() -> jpaTm().insert(domain));
-
-    DomainHistory domainHistory = createDomainHistory(domain);
     domainHistory.id = null;
     jpaTm().transact(() -> jpaTm().insert(domainHistory));
 
@@ -91,8 +97,6 @@ public class DomainHistoryTest extends EntityTestCase {
 
   @Test
   void testOfyPersistence() {
-    saveRegistrar("TheRegistrar");
-
     HostResource host = newHostResourceWithRoid("ns1.example.com", "host1");
     ContactResource contact = newContactResourceWithRoid("contactId", "contact1");
 
@@ -115,7 +119,7 @@ public class DomainHistoryTest extends EntityTestCase {
     tm().transact(() -> tm().insert(domainHistory));
 
     // retrieving a HistoryEntry or a DomainHistory with the same key should return the same object
-    // note: due to the @EntitySubclass annotation. all Keys for ContactHistory objects will have
+    // note: due to the @EntitySubclass annotation. all Keys for DomainHistory objects will have
     // type HistoryEntry
     VKey<DomainHistory> domainHistoryVKey =
         VKey.createOfy(DomainHistory.class, Key.create(domainHistory));
@@ -127,11 +131,33 @@ public class DomainHistoryTest extends EntityTestCase {
     assertThat(domainHistoryFromDb).isEqualTo(historyEntryFromDb);
   }
 
+  static DomainBase createDomainWithContactsAndHosts() {
+    HostResource host = newHostResourceWithRoid("ns1.example.com", "host1");
+    ContactResource contact = newContactResourceWithRoid("contactId", "contact1");
+
+    jpaTm()
+        .transact(
+            () -> {
+              jpaTm().insert(host);
+              jpaTm().insert(contact);
+            });
+
+    DomainBase domain =
+        newDomainBase("example.tld", "domainRepoId", contact)
+            .asBuilder()
+            .setNameservers(host.createVKey())
+            .build();
+    jpaTm().transact(() -> jpaTm().insert(domain));
+    return domain;
+  }
+
   static void assertDomainHistoriesEqual(DomainHistory one, DomainHistory two) {
     assertAboutImmutableObjects()
         .that(one)
         .isEqualExceptFields(
             two, "domainContent", "domainRepoId", "parent", "nsHosts", "domainTransactionRecords");
+    assertThat(one.getDomainContent().map(DomainContent::getDomainName))
+        .isEqualTo(two.getDomainContent().map(DomainContent::getDomainName));
     // NB: the record's ID gets reset by Hibernate, causing the hash code to differ so we have to
     // compare it separately
     assertThat(one.getDomainTransactionRecords())

core/src/test/java/google/registry/model/history/HostHistoryTest.java

@@ -1,4 +1,4 @@
-// Copyright 2017 The Nomulus Authors. All Rights Reserved.
+// Copyright 2020 The Nomulus Authors. All Rights Reserved.
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
@@ -45,7 +45,8 @@ public class HostHistoryTest extends EntityTestCase {
 
     HostResource host = newHostResourceWithRoid("ns1.example.com", "host1");
     jpaTm().transact(() -> jpaTm().insert(host));
-    VKey<HostResource> hostVKey = VKey.createSql(HostResource.class, "host1");
+    VKey<HostResource> hostVKey =
+        VKey.create(HostResource.class, "host1", Key.create(HostResource.class, "host1"));
     HostResource hostFromDb = jpaTm().transact(() -> jpaTm().load(hostVKey));
     HostHistory hostHistory = createHostHistory(hostFromDb, hostVKey);
     hostHistory.id = null;
@@ -62,19 +63,46 @@ public class HostHistoryTest extends EntityTestCase {
   }
 
   @Test
-  public void testOfySave() {
+  void testLegacyPersistence_nullHostBase() {
+    saveRegistrar("TheRegistrar");
+    HostResource host = newHostResourceWithRoid("ns1.example.com", "host1");
+    jpaTm().transact(() -> jpaTm().insert(host));
+
+    VKey<HostResource> hostVKey =
+        VKey.create(HostResource.class, "host1", Key.create(HostResource.class, "host1"));
+    HostResource hostFromDb = jpaTm().transact(() -> jpaTm().load(hostVKey));
+
+    HostHistory hostHistory =
+        createHostHistory(hostFromDb, hostVKey).asBuilder().setHostBase(null).build();
+    hostHistory.id = null;
+    jpaTm().transact(() -> jpaTm().insert(hostHistory));
+
+    jpaTm()
+        .transact(
+            () -> {
+              HostHistory fromDatabase =
+                  jpaTm().load(VKey.createSql(HostHistory.class, hostHistory.getId()));
+              assertHostHistoriesEqual(fromDatabase, hostHistory);
+              assertThat(fromDatabase.getHostRepoId().getSqlKey())
+                  .isEqualTo(hostHistory.getHostRepoId().getSqlKey());
+            });
+  }
+
+  @Test
+  void testOfySave() {
     saveRegistrar("registrar1");
 
     HostResource host = newHostResourceWithRoid("ns1.example.com", "host1");
     tm().transact(() -> tm().insert(host));
-    VKey<HostResource> hostVKey = VKey.create(HostResource.class, "host1", Key.create(host));
+    VKey<HostResource> hostVKey =
+        VKey.create(HostResource.class, "host1", Key.create(HostResource.class, "host1"));
     HostResource hostFromDb = tm().transact(() -> tm().load(hostVKey));
     HostHistory hostHistory = createHostHistory(hostFromDb, hostVKey);
     fakeClock.advanceOneMilli();
     tm().transact(() -> tm().insert(hostHistory));
 
     // retrieving a HistoryEntry or a HostHistory with the same key should return the same object
-    // note: due to the @EntitySubclass annotation. all Keys for ContactHistory objects will have
+    // note: due to the @EntitySubclass annotation. all Keys for HostHistory objects will have
     // type HistoryEntry
     VKey<HostHistory> hostHistoryVKey = VKey.createOfy(HostHistory.class, Key.create(hostHistory));
     VKey<HistoryEntry> historyEntryVKey =
@@ -88,8 +116,8 @@ public class HostHistoryTest extends EntityTestCase {
   private void assertHostHistoriesEqual(HostHistory one, HostHistory two) {
     assertAboutImmutableObjects().that(one).isEqualExceptFields(two, "hostBase");
     assertAboutImmutableObjects()
-        .that(one.getHostBase())
+        .that(one.getHostBase().orElse(null))
-        .isEqualExceptFields(two.getHostBase(), "repoId");
+        .isEqualExceptFields(two.getHostBase().orElse(null), "repoId");
   }
 
   private HostHistory createHostHistory(HostBase hostBase, VKey<HostResource> hostVKey) {

core/src/test/java/google/registry/model/history/LegacyHistoryObjectTest.java

@@ -0,0 +1,211 @@
+// Copyright 2020 The Nomulus Authors. All Rights Reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package google.registry.model.history;
+
+import static com.google.common.truth.Truth.assertThat;
+import static google.registry.model.ImmutableObjectSubject.assertAboutImmutableObjects;
+import static google.registry.persistence.transaction.TransactionManagerFactory.jpaTm;
+import static google.registry.persistence.transaction.TransactionManagerFactory.tm;
+import static google.registry.testing.DatastoreHelper.newContactResourceWithRoid;
+import static google.registry.testing.DatastoreHelper.newHostResourceWithRoid;
+import static google.registry.testing.SqlHelper.saveRegistrar;
+import static google.registry.util.CollectionUtils.nullToEmpty;
+import static java.nio.charset.StandardCharsets.UTF_8;
+
+import com.google.common.collect.ImmutableSet;
+import com.googlecode.objectify.Key;
+import google.registry.model.EntityTestCase;
+import google.registry.model.EppResource;
+import google.registry.model.contact.ContactHistory;
+import google.registry.model.contact.ContactResource;
+import google.registry.model.domain.DomainBase;
+import google.registry.model.domain.DomainHistory;
+import google.registry.model.domain.Period;
+import google.registry.model.eppcommon.Trid;
+import google.registry.model.host.HostHistory;
+import google.registry.model.host.HostResource;
+import google.registry.model.reporting.DomainTransactionRecord;
+import google.registry.model.reporting.DomainTransactionRecord.TransactionReportField;
+import google.registry.model.reporting.HistoryEntry;
+import google.registry.persistence.VKey;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+/** Tests to check {@link HistoryEntry} + its subclasses' transitions to/from Datastore/SQL. */
+public class LegacyHistoryObjectTest extends EntityTestCase {
+
+  public LegacyHistoryObjectTest() {
+    super(JpaEntityCoverageCheck.ENABLED);
+  }
+
+  @BeforeEach
+  void beforeEach() {
+    saveRegistrar("TheRegistrar");
+  }
+
+  @Test
+  void testFullConversion_contact() {
+    // Create+save an old contact HistoryEntry, reload it, and verify it's a proper ContactHistory
+    ContactResource contact = newContactResourceWithRoid("contactId", "contact1");
+    HistoryEntry legacyHistoryEntry = historyEntryBuilderFor(contact).build();
+    tm().transact(() -> tm().insert(legacyHistoryEntry));
+
+    // In Datastore, we will save it as HistoryEntry but retrieve it as ContactHistory
+    long historyEntryId = legacyHistoryEntry.getId();
+    HistoryEntry fromObjectify =
+        tm().transact(
+                () ->
+                    tm().load(
+                            VKey.create(
+                                HistoryEntry.class,
+                                historyEntryId,
+                                Key.create(legacyHistoryEntry))));
+    // The objects will be mostly the same, but the ContactHistory object has a couple extra fields
+    assertAboutImmutableObjects()
+        .that(legacyHistoryEntry)
+        .isEqualExceptFields(fromObjectify, "contactBase", "contactRepoId");
+    assertThat(fromObjectify instanceof ContactHistory).isTrue();
+    ContactHistory legacyContactHistory = (ContactHistory) fromObjectify;
+
+    // Next, save that from-Datastore object in SQL and verify we can load it back in
+    legacyContactHistory.id = null;
+    jpaTm()
+        .transact(
+            () -> {
+              jpaTm().insert(contact);
+              jpaTm().insert(legacyContactHistory);
+            });
+    ContactHistory legacyHistoryFromSql =
+        jpaTm()
+            .transact(
+                () ->
+                    jpaTm()
+                        .load(VKey.createSql(ContactHistory.class, legacyContactHistory.getId())));
+    assertAboutImmutableObjects()
+        .that(legacyContactHistory)
+        .isEqualExceptFields(legacyHistoryFromSql);
+    // can't compare contactRepoId directly since it doesn't save the ofy key
+    assertThat(legacyContactHistory.getContactRepoId().getSqlKey())
+        .isEqualTo(legacyHistoryFromSql.getContactRepoId().getSqlKey());
+  }
+
+  @Test
+  void testFullConversion_domain() {
+    // Create+save an old domain HistoryEntry, reload it, and verify it's a proper DomainHistory
+    DomainBase domain = DomainHistoryTest.createDomainWithContactsAndHosts();
+    HistoryEntry legacyHistoryEntry = historyEntryForDomain(domain);
+    tm().transact(() -> tm().insert(legacyHistoryEntry));
+
+    // In Datastore, we will save it as HistoryEntry but retrieve it as DomainHistory
+    long historyEntryId = legacyHistoryEntry.getId();
+    HistoryEntry fromObjectify =
+        tm().transact(
+                () ->
+                    tm().load(
+                            VKey.create(
+                                HistoryEntry.class,
+                                historyEntryId,
+                                Key.create(legacyHistoryEntry))));
+    // The objects will be mostly the same, but the DomainHistory object has a couple extra fields
+    assertAboutImmutableObjects()
+        .that(legacyHistoryEntry)
+        .isEqualExceptFields(fromObjectify, "domainContent", "domainRepoId", "nsHosts");
+    assertThat(fromObjectify instanceof DomainHistory).isTrue();
+    DomainHistory legacyDomainHistory = (DomainHistory) fromObjectify;
+
+    // Next, save that from-Datastore object in SQL and verify we can load it back in
+    jpaTm().transact(() -> jpaTm().insert(legacyDomainHistory));
+    DomainHistory legacyHistoryFromSql =
+        jpaTm().transact(() -> jpaTm().load(legacyDomainHistory.createVKey()));
+    // Don't compare nsHosts directly because one is null and the other is empty
+    assertAboutImmutableObjects()
+        .that(legacyDomainHistory)
+        .isEqualExceptFields(
+            // NB: period, transaction records, and other client ID are added in #794
+            legacyHistoryFromSql, "period", "domainTransactionRecords", "otherClientId", "nsHosts");
+    assertThat(nullToEmpty(legacyDomainHistory.getNsHosts()))
+        .isEqualTo(nullToEmpty(legacyHistoryFromSql.getNsHosts()));
+  }
+
+  @Test
+  void testFullConversion_host() {
+    // Create+save an old host HistoryEntry, reload it, and verify it's a proper HostHistory
+    HostResource host = newHostResourceWithRoid("hs1.example.com", "host1");
+    HistoryEntry legacyHistoryEntry = historyEntryBuilderFor(host).build();
+    tm().transact(() -> tm().insert(legacyHistoryEntry));
+
+    // In Datastore, we will save it as HistoryEntry but retrieve it as HostHistory
+    long historyEntryId = legacyHistoryEntry.getId();
+    HistoryEntry fromObjectify =
+        tm().transact(
+                () ->
+                    tm().load(
+                            VKey.create(
+                                HistoryEntry.class,
+                                historyEntryId,
+                                Key.create(legacyHistoryEntry))));
+    // The objects will be mostly the same, but the HostHistory object has a couple extra fields
+    assertAboutImmutableObjects()
+        .that(legacyHistoryEntry)
+        .isEqualExceptFields(fromObjectify, "hostBase", "hostRepoId");
+    assertThat(fromObjectify instanceof HostHistory).isTrue();
+    HostHistory legacyHostHistory = (HostHistory) fromObjectify;
+
+    // Next, save that from-Datastore object in SQL and verify we can load it back in
+    legacyHostHistory.id = null;
+    jpaTm()
+        .transact(
+            () -> {
+              jpaTm().insert(host);
+              jpaTm().insert(legacyHostHistory);
+            });
+    HostHistory legacyHistoryFromSql =
+        jpaTm()
+            .transact(
+                () -> jpaTm().load(VKey.createSql(HostHistory.class, legacyHostHistory.getId())));
+    assertAboutImmutableObjects().that(legacyHostHistory).isEqualExceptFields(legacyHistoryFromSql);
+    // can't compare hostRepoId directly since it doesn't save the ofy key in SQL
+    assertThat(legacyHostHistory.getHostRepoId().getSqlKey())
+        .isEqualTo(legacyHistoryFromSql.getHostRepoId().getSqlKey());
+  }
+
+  private HistoryEntry historyEntryForDomain(DomainBase domain) {
+    DomainTransactionRecord transactionRecord =
+        new DomainTransactionRecord.Builder()
+            .setTld("foobar")
+            .setReportingTime(fakeClock.nowUtc())
+            .setReportField(TransactionReportField.NET_ADDS_1_YR)
+            .setReportAmount(1)
+            .build();
+    return historyEntryBuilderFor(domain)
+        .setPeriod(Period.create(1, Period.Unit.YEARS))
+        .setDomainTransactionRecords(ImmutableSet.of(transactionRecord))
+        .setOtherClientId("TheRegistrar")
+        .build();
+  }
+
+  private HistoryEntry.Builder historyEntryBuilderFor(EppResource parent) {
+    return new HistoryEntry.Builder()
+        .setParent(parent)
+        .setType(HistoryEntry.Type.DOMAIN_CREATE)
+        .setXmlBytes("<xml></xml>".getBytes(UTF_8))
+        .setModificationTime(fakeClock.nowUtc())
+        .setClientId("TheRegistrar")
+        .setTrid(Trid.create("ABC-123", "server-trid"))
+        .setBySuperuser(false)
+        .setReason("reason")
+        .setRequestedByRegistrar(false);
+  }
+}

db/src/main/resources/sql/flyway/V57__history_null_content.sql

@@ -0,0 +1,31 @@
+-- Copyright 2020 The Nomulus Authors. All Rights Reserved.
+--
+-- Licensed under the Apache License, Version 2.0 (the "License");
+-- you may not use this file except in compliance with the License.
+-- You may obtain a copy of the License at
+--
+--     http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+
+-- Note: we drop the not-null constraints from the history tables but we keep them in the
+-- EPP resource tables since nothing inserted there should be null
+
+ALTER TABLE "ContactHistory" ALTER COLUMN creation_registrar_id DROP NOT NULL;
+ALTER TABLE "ContactHistory" ALTER COLUMN creation_time DROP NOT NULL;
+ALTER TABLE "ContactHistory" ALTER COLUMN current_sponsor_registrar_id DROP NOT NULL;
+ALTER TABLE "ContactHistory" ALTER COLUMN history_reason DROP NOT NULL;
+
+ALTER TABLE "DomainHistory" ALTER COLUMN creation_registrar_id DROP NOT NULL;
+ALTER TABLE "DomainHistory" ALTER COLUMN creation_time DROP NOT NULL;
+ALTER TABLE "DomainHistory" ALTER COLUMN current_sponsor_registrar_id DROP NOT NULL;
+ALTER TABLE "DomainHistory" ALTER COLUMN history_reason DROP NOT NULL;
+
+ALTER TABLE "HostHistory" ALTER COLUMN creation_registrar_id DROP NOT NULL;
+ALTER TABLE "HostHistory" ALTER COLUMN creation_time DROP NOT NULL;
+ALTER TABLE "HostHistory" ALTER COLUMN current_sponsor_registrar_id DROP NOT NULL;
+ALTER TABLE "HostHistory" ALTER COLUMN history_reason DROP NOT NULL;

db/src/main/resources/sql/schema/db-schema.sql.generated

@@ -94,9 +94,9 @@ create sequence temp_history_id_sequence start 1 increment 50;
     create table "Contact" (
        repo_id text not null,
         update_timestamp timestamptz,
-        creation_registrar_id text not null,
+        creation_registrar_id text,
-        creation_time timestamptz not null,
+        creation_time timestamptz,
-        current_sponsor_registrar_id text not null,
+        current_sponsor_registrar_id text,
         deletion_time timestamptz,
         last_epp_update_registrar_id text,
         last_epp_update_time timestamptz,
@@ -155,7 +155,7 @@ create sequence temp_history_id_sequence start 1 increment 50;
         history_by_superuser boolean not null,
         history_registrar_id text,
         history_modification_time timestamptz not null,
-        history_reason text not null,
+        history_reason text,
         history_requested_by_registrar boolean not null,
         history_client_transaction_id text,
         history_server_transaction_id text,
@@ -207,9 +207,9 @@ create sequence temp_history_id_sequence start 1 increment 50;
         transfer_status text,
         voice_phone_extension text,
         voice_phone_number text,
-        creation_registrar_id text not null,
+        creation_registrar_id text,
-        creation_time timestamptz not null,
+        creation_time timestamptz,
-        current_sponsor_registrar_id text not null,
+        current_sponsor_registrar_id text,
         deletion_time timestamptz,
         last_epp_update_registrar_id text,
         last_epp_update_time timestamptz,
@@ -238,9 +238,9 @@ create sequence temp_history_id_sequence start 1 increment 50;
     create table "Domain" (
        repo_id text not null,
         update_timestamp timestamptz,
-        creation_registrar_id text not null,
+        creation_registrar_id text,
-        creation_time timestamptz not null,
+        creation_time timestamptz,
-        current_sponsor_registrar_id text not null,
+        current_sponsor_registrar_id text,
         deletion_time timestamptz,
         last_epp_update_registrar_id text,
         last_epp_update_time timestamptz,
@@ -291,7 +291,7 @@ create sequence temp_history_id_sequence start 1 increment 50;
         history_by_superuser boolean not null,
         history_registrar_id text,
         history_modification_time timestamptz not null,
-        history_reason text not null,
+        history_reason text,
         history_requested_by_registrar boolean not null,
         history_client_transaction_id text,
         history_server_transaction_id text,
@@ -334,9 +334,9 @@ create sequence temp_history_id_sequence start 1 increment 50;
         transfer_pending_expiration_time timestamptz,
         transfer_request_time timestamptz,
         transfer_status text,
-        creation_registrar_id text not null,
+        creation_registrar_id text,
-        creation_time timestamptz not null,
+        creation_time timestamptz,
-        current_sponsor_registrar_id text not null,
+        current_sponsor_registrar_id text,
         deletion_time timestamptz,
         last_epp_update_registrar_id text,
         last_epp_update_time timestamptz,
@@ -384,9 +384,9 @@ create sequence temp_history_id_sequence start 1 increment 50;
     create table "Host" (
        repo_id text not null,
         update_timestamp timestamptz,
-        creation_registrar_id text not null,
+        creation_registrar_id text,
-        creation_time timestamptz not null,
+        creation_time timestamptz,
-        current_sponsor_registrar_id text not null,
+        current_sponsor_registrar_id text,
         deletion_time timestamptz,
         last_epp_update_registrar_id text,
         last_epp_update_time timestamptz,
@@ -404,7 +404,7 @@ create sequence temp_history_id_sequence start 1 increment 50;
         history_by_superuser boolean not null,
         history_registrar_id text,
         history_modification_time timestamptz not null,
-        history_reason text not null,
+        history_reason text,
         history_requested_by_registrar boolean not null,
         history_client_transaction_id text,
         history_server_transaction_id text,
@@ -415,9 +415,9 @@ create sequence temp_history_id_sequence start 1 increment 50;
         last_superordinate_change timestamptz,
         last_transfer_time timestamptz,
         superordinate_domain text,
-        creation_registrar_id text not null,
+        creation_registrar_id text,
-        creation_time timestamptz not null,
+        creation_time timestamptz,
-        current_sponsor_registrar_id text not null,
+        current_sponsor_registrar_id text,
         deletion_time timestamptz,
         last_epp_update_registrar_id text,
         last_epp_update_time timestamptz,

db/src/main/resources/sql/schema/nomulus.golden.sql

@@ -284,7 +284,7 @@ CREATE TABLE public."ContactHistory" (
     history_by_superuser boolean NOT NULL,
     history_registrar_id text,
     history_modification_time timestamp with time zone NOT NULL,
-    history_reason text NOT NULL,
+    history_reason text,
     history_requested_by_registrar boolean NOT NULL,
     history_client_transaction_id text,
     history_server_transaction_id text,
@@ -336,9 +336,9 @@ CREATE TABLE public."ContactHistory" (
     transfer_status text,
     voice_phone_extension text,
     voice_phone_number text,
-    creation_registrar_id text NOT NULL,
+    creation_registrar_id text,
-    creation_time timestamp with time zone NOT NULL,
+    creation_time timestamp with time zone,
-    current_sponsor_registrar_id text NOT NULL,
+    current_sponsor_registrar_id text,
     deletion_time timestamp with time zone,
     last_epp_update_registrar_id text,
     last_epp_update_time timestamp with time zone,
@@ -423,7 +423,7 @@ CREATE TABLE public."DomainHistory" (
     history_by_superuser boolean NOT NULL,
     history_registrar_id text,
     history_modification_time timestamp with time zone NOT NULL,
-    history_reason text NOT NULL,
+    history_reason text,
     history_requested_by_registrar boolean NOT NULL,
     history_client_transaction_id text,
     history_server_transaction_id text,
@@ -465,9 +465,9 @@ CREATE TABLE public."DomainHistory" (
     transfer_pending_expiration_time timestamp with time zone,
     transfer_request_time timestamp with time zone,
     transfer_status text,
-    creation_registrar_id text NOT NULL,
+    creation_registrar_id text,
-    creation_time timestamp with time zone NOT NULL,
+    creation_time timestamp with time zone,
-    current_sponsor_registrar_id text NOT NULL,
+    current_sponsor_registrar_id text,
     deletion_time timestamp with time zone,
     last_epp_update_registrar_id text,
     last_epp_update_time timestamp with time zone,
@@ -601,7 +601,7 @@ CREATE TABLE public."HostHistory" (
     history_by_superuser boolean NOT NULL,
     history_registrar_id text NOT NULL,
     history_modification_time timestamp with time zone NOT NULL,
-    history_reason text NOT NULL,
+    history_reason text,
     history_requested_by_registrar boolean NOT NULL,
     history_client_transaction_id text,
     history_server_transaction_id text,
@@ -612,9 +612,9 @@ CREATE TABLE public."HostHistory" (
     last_superordinate_change timestamp with time zone,
     last_transfer_time timestamp with time zone,
     superordinate_domain text,
-    creation_registrar_id text NOT NULL,
+    creation_registrar_id text,
-    creation_time timestamp with time zone NOT NULL,
+    creation_time timestamp with time zone,
-    current_sponsor_registrar_id text NOT NULL,
+    current_sponsor_registrar_id text,
     deletion_time timestamp with time zone,
     last_epp_update_registrar_id text,
     last_epp_update_time timestamp with time zone,