diff --git a/core/src/main/java/google/registry/model/EppResource.java b/core/src/main/java/google/registry/model/EppResource.java index 2c10ad714..1b680c84f 100644 --- a/core/src/main/java/google/registry/model/EppResource.java +++ b/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. * + *

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 + * *

This is in the (\w|_){1,80}-\w{1,8} format specified by RFC 5730 for roidType. * * @see RFC 5730 */ - @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. + * + *

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. + * + *

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. + * + *

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). + * + *

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; 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 64875c0e1..7a103e7fe 100644 --- a/core/src/main/java/google/registry/model/contact/ContactHistory.java +++ b/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 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. + * + *

Will be absent for objects created prior to the Registry 3.0 SQL migration. + */ + public Optional 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 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)); 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 fd52ab5eb..ac926f408 100644 --- a/core/src/main/java/google/registry/model/domain/DomainHistory.java +++ b/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. + * + *

Will be absent for objects created prior to the Registry 3.0 SQL migration. + */ + public Optional 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. */ 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 24dfd13e9..65e32d730 100644 --- a/core/src/main/java/google/registry/model/host/HostHistory.java +++ b/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 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. + * + *

Will be absent for objects created prior to the Registry 3.0 SQL migration. + */ + public Optional 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 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)); diff --git a/core/src/main/java/google/registry/model/reporting/HistoryEntry.java b/core/src/main/java/google/registry/model/reporting/HistoryEntry.java index 0d3c4c25f..b5670f903 100644 --- a/core/src/main/java/google/registry/model/reporting/HistoryEntry.java +++ b/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(); } diff --git a/core/src/main/java/google/registry/persistence/converter/CreateAutoTimestampConverter.java b/core/src/main/java/google/registry/persistence/converter/CreateAutoTimestampConverter.java index 938528368..b70605c27 100644 --- a/core/src/main/java/google/registry/persistence/converter/CreateAutoTimestampConverter.java +++ b/core/src/main/java/google/registry/persistence/converter/CreateAutoTimestampConverter.java @@ -32,7 +32,11 @@ public class CreateAutoTimestampConverter implements AttributeConverter { @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()); } diff --git a/core/src/test/java/google/registry/model/ImmutableObjectSubject.java b/core/src/test/java/google/registry/model/ImmutableObjectSubject.java index c3fc5bd03..fa8c74e83 100644 --- a/core/src/test/java/google/registry/model/ImmutableObjectSubject.java +++ b/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) { - Map actualFields = filterFields(actual, ignoredFields); - Map expectedFields = filterFields(expected, ignoredFields); - assertThat(actualFields).containsExactlyEntriesIn(expectedFields); + public void isEqualExceptFields(@Nullable ImmutableObject expected, String... ignoredFields) { + if (actual == null) { + assertThat(expected).isNull(); + } else { + assertThat(expected).isNotNull(); + } + if (actual != null) { + Map actualFields = filterFields(actual, ignoredFields); + Map expectedFields = filterFields(expected, ignoredFields); + assertThat(actualFields).containsExactlyEntriesIn(expectedFields); + } } public static Correspondence immutableObjectCorrespondence( diff --git a/core/src/test/java/google/registry/model/history/ContactHistoryTest.java b/core/src/test/java/google/registry/model/history/ContactHistoryTest.java index 88e6f02eb..198c60d45 100644 --- a/core/src/test/java/google/registry/model/history/ContactHistoryTest.java +++ b/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 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()) - .isEqualExceptFields(two.getContactBase(), "repoId"); + .that(one.getContactBase().orElse(null)) + .isEqualExceptFields(two.getContactBase().orElse(null), "repoId"); } } diff --git a/core/src/test/java/google/registry/model/history/DomainHistoryTest.java b/core/src/test/java/google/registry/model/history/DomainHistoryTest.java index f46347ba7..c5a39c843 100644 --- a/core/src/test/java/google/registry/model/history/DomainHistoryTest.java +++ b/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"); - - HostResource host = newHostResourceWithRoid("ns1.example.com", "host1"); - ContactResource contact = newContactResourceWithRoid("contactId", "contact1"); + DomainBase domain = createDomainWithContactsAndHosts(); + DomainHistory domainHistory = createDomainHistory(domain); + domainHistory.id = null; + jpaTm().transact(() -> jpaTm().insert(domainHistory)); jpaTm() .transact( () -> { - jpaTm().insert(host); - jpaTm().insert(contact); + DomainHistory fromDatabase = jpaTm().load(domainHistory.createVKey()); + assertDomainHistoriesEqual(fromDatabase, domainHistory); + assertThat(fromDatabase.getDomainRepoId().getSqlKey()) + .isEqualTo(domainHistory.getDomainRepoId().getSqlKey()); }); + } - DomainBase domain = - newDomainBase("example.tld", "domainRepoId", contact) - .asBuilder() - .setNameservers(host.createVKey()) - .build(); - jpaTm().transact(() -> jpaTm().insert(domain)); - - DomainHistory domainHistory = createDomainHistory(domain); + @Test + void testLegacyPersistence_nullResource() { + DomainBase domain = createDomainWithContactsAndHosts(); + DomainHistory domainHistory = + createDomainHistory(domain).asBuilder().setDomainContent(null).build(); 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 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()) diff --git a/core/src/test/java/google/registry/model/history/HostHistoryTest.java b/core/src/test/java/google/registry/model/history/HostHistoryTest.java index 660779115..ed0d8b88f 100644 --- a/core/src/test/java/google/registry/model/history/HostHistoryTest.java +++ b/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 hostVKey = VKey.createSql(HostResource.class, "host1"); + VKey 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 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 hostVKey = VKey.create(HostResource.class, "host1", Key.create(host)); + VKey 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 hostHistoryVKey = VKey.createOfy(HostHistory.class, Key.create(hostHistory)); VKey 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()) - .isEqualExceptFields(two.getHostBase(), "repoId"); + .that(one.getHostBase().orElse(null)) + .isEqualExceptFields(two.getHostBase().orElse(null), "repoId"); } private HostHistory createHostHistory(HostBase hostBase, VKey hostVKey) { diff --git a/core/src/test/java/google/registry/model/history/LegacyHistoryObjectTest.java b/core/src/test/java/google/registry/model/history/LegacyHistoryObjectTest.java new file mode 100644 index 000000000..ac9b3fc0a --- /dev/null +++ b/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("".getBytes(UTF_8)) + .setModificationTime(fakeClock.nowUtc()) + .setClientId("TheRegistrar") + .setTrid(Trid.create("ABC-123", "server-trid")) + .setBySuperuser(false) + .setReason("reason") + .setRequestedByRegistrar(false); + } +} diff --git a/db/src/main/resources/sql/flyway/V57__history_null_content.sql b/db/src/main/resources/sql/flyway/V57__history_null_content.sql new file mode 100644 index 000000000..2bcc50de6 --- /dev/null +++ b/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; diff --git a/db/src/main/resources/sql/schema/db-schema.sql.generated b/db/src/main/resources/sql/schema/db-schema.sql.generated index 22740dc3f..e529dd81b 100644 --- a/db/src/main/resources/sql/schema/db-schema.sql.generated +++ b/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_time timestamptz not null, - current_sponsor_registrar_id text not null, + creation_registrar_id text, + creation_time timestamptz, + 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_time timestamptz not null, - current_sponsor_registrar_id text not null, + creation_registrar_id text, + creation_time timestamptz, + 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_time timestamptz not null, - current_sponsor_registrar_id text not null, + creation_registrar_id text, + creation_time timestamptz, + 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_time timestamptz not null, - current_sponsor_registrar_id text not null, + creation_registrar_id text, + creation_time timestamptz, + 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_time timestamptz not null, - current_sponsor_registrar_id text not null, + creation_registrar_id text, + creation_time timestamptz, + 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_time timestamptz not null, - current_sponsor_registrar_id text not null, + creation_registrar_id text, + creation_time timestamptz, + current_sponsor_registrar_id text, deletion_time timestamptz, last_epp_update_registrar_id text, last_epp_update_time timestamptz, diff --git a/db/src/main/resources/sql/schema/nomulus.golden.sql b/db/src/main/resources/sql/schema/nomulus.golden.sql index 14bad5f15..d39e1d99c 100644 --- a/db/src/main/resources/sql/schema/nomulus.golden.sql +++ b/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_time timestamp with time zone NOT NULL, - current_sponsor_registrar_id text NOT NULL, + creation_registrar_id text, + creation_time timestamp with time zone, + 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_time timestamp with time zone NOT NULL, - current_sponsor_registrar_id text NOT NULL, + creation_registrar_id text, + creation_time timestamp with time zone, + 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_time timestamp with time zone NOT NULL, - current_sponsor_registrar_id text NOT NULL, + creation_registrar_id text, + creation_time timestamp with time zone, + 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,