From 4ec7f23e84baff62340b793a09db3f5233d8cf55 Mon Sep 17 00:00:00 2001 From: gbrodman Date: Fri, 9 Oct 2020 16:01:51 -0400 Subject: [PATCH] Use the parent to store the history repo ID and fill in the base object (#830) * Use the parent to store the history repo ID and fill in the base object Storing the repo ID in the parent and in the base object has two primary benefits. First, it unifies the parent information in the HistoryEntry's `parent` object. This simplifies the builders and the data flow. Second, when possible (which should be always, post-migration) we fill out the DomainContent's repo ID (similarly for the other EPP resources) which means that when reconstituting the ofy keys we don't need to pass the repo ID in from a separate object. This way, all the data are encapsulated where they should be. The primary downside here is that it further reduces the "immutability" of the history objects (since we're using the Hibernate setter for the parent repo ID) but we weren't immutable anyway. * Respond to CR - compare the entire vkeys in tests - always return the parent for repo ID * Simplify creation of parent VKeys * Fix flipped isAssignableFrom check in VKey * Merge remote-tracking branch 'origin/master' into historyRepoId --- .../model/contact/ContactHistory.java | 31 +++++++------- .../registry/model/domain/DomainBase.java | 7 ---- .../registry/model/domain/DomainContent.java | 41 +++---------------- .../registry/model/domain/DomainHistory.java | 33 +++++++-------- .../registry/model/eppcommon/StatusValue.java | 12 +++++- .../registry/model/host/HostHistory.java | 32 +++++++-------- .../google/registry/persistence/VKey.java | 4 +- .../model/history/ContactHistoryTest.java | 6 +-- .../model/history/DomainHistoryTest.java | 6 +-- .../model/history/HostHistoryTest.java | 6 +-- .../history/LegacyHistoryObjectTest.java | 8 ++-- .../google/registry/model/schema.txt | 3 -- 12 files changed, 73 insertions(+), 116 deletions(-) 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 0796d93d9..56e0e830e 100644 --- a/core/src/main/java/google/registry/model/contact/ContactHistory.java +++ b/core/src/main/java/google/registry/model/contact/ContactHistory.java @@ -17,7 +17,6 @@ package google.registry.model.contact; import com.google.common.collect.ImmutableList; import com.googlecode.objectify.Key; import com.googlecode.objectify.annotation.EntitySubclass; -import google.registry.model.EppResource; import google.registry.model.ImmutableObject; import google.registry.model.contact.ContactHistory.ContactHistoryId; import google.registry.model.reporting.HistoryEntry; @@ -58,7 +57,17 @@ public class ContactHistory extends HistoryEntry implements SqlEntity { // Store ContactBase instead of ContactResource so we don't pick up its @Id @Nullable ContactBase contactBase; - @Id String contactRepoId; + @Id + @Access(AccessType.PROPERTY) + public String getContactRepoId() { + return parent.getName(); + } + + /** This method is private because it is only used by Hibernate. */ + @SuppressWarnings("unused") + private void setContactRepoId(String contactRepoId) { + parent = Key.create(ContactResource.class, contactRepoId); + } @Id @Column(name = "historyRevisionId") @@ -79,15 +88,14 @@ public class ContactHistory extends HistoryEntry implements SqlEntity { } /** The key to the {@link ContactResource} this is based off of. */ - public VKey getContactRepoId() { - return VKey.create( - ContactResource.class, contactRepoId, Key.create(ContactResource.class, contactRepoId)); + public VKey getParentVKey() { + return VKey.create(ContactResource.class, getContactRepoId()); } /** Creates a {@link VKey} instance for this entity. */ public VKey createVKey() { return VKey.create( - ContactHistory.class, new ContactHistoryId(contactRepoId, getId()), Key.create(this)); + ContactHistory.class, new ContactHistoryId(getContactRepoId(), getId()), Key.create(this)); } @PostLoad @@ -97,8 +105,6 @@ public class ContactHistory extends HistoryEntry implements SqlEntity { if (contactBase != null && contactBase.getContactId() == null) { contactBase = null; } - // Fill in the full, symmetric, parent repo ID key - parent = Key.create(ContactResource.class, contactRepoId); } // In Datastore, save as a HistoryEntry object regardless of this object's type @@ -184,17 +190,8 @@ public class ContactHistory extends HistoryEntry implements SqlEntity { } public Builder setContactRepoId(String contactRepoId) { - getInstance().contactRepoId = contactRepoId; getInstance().parent = Key.create(ContactResource.class, contactRepoId); return this; } - - // We can remove this once all HistoryEntries are converted to History objects - @Override - public Builder setParent(Key parent) { - super.setParent(parent); - getInstance().contactRepoId = parent.getName(); - return this; - } } } diff --git a/core/src/main/java/google/registry/model/domain/DomainBase.java b/core/src/main/java/google/registry/model/domain/DomainBase.java index 67778758b..aa1d77a70 100644 --- a/core/src/main/java/google/registry/model/domain/DomainBase.java +++ b/core/src/main/java/google/registry/model/domain/DomainBase.java @@ -36,7 +36,6 @@ import javax.persistence.Index; import javax.persistence.JoinColumn; import javax.persistence.JoinTable; import javax.persistence.OneToMany; -import javax.persistence.PostLoad; import javax.persistence.Table; import org.joda.time.DateTime; @@ -108,12 +107,6 @@ public class DomainBase extends DomainContent return gracePeriods; } - @PostLoad - @SuppressWarnings("UnusedMethod") - private final void postLoad() { - restoreOfyKeys(getRepoId()); - } - /** * Returns the set of {@link DelegationSignerData} associated with the domain. * diff --git a/core/src/main/java/google/registry/model/domain/DomainContent.java b/core/src/main/java/google/registry/model/domain/DomainContent.java index a726e1222..ab39f073f 100644 --- a/core/src/main/java/google/registry/model/domain/DomainContent.java +++ b/core/src/main/java/google/registry/model/domain/DomainContent.java @@ -35,6 +35,7 @@ import static google.registry.util.DomainNameUtils.canonicalizeDomainName; import static google.registry.util.DomainNameUtils.getTldFromDomainName; import static google.registry.util.PreconditionsUtils.checkArgumentNotNull; +import com.google.common.base.Predicate; import com.google.common.collect.ImmutableSet; import com.google.common.collect.ImmutableSortedSet; import com.google.common.collect.Ordering; @@ -68,7 +69,6 @@ import java.util.HashSet; import java.util.Objects; import java.util.Optional; import java.util.Set; -import java.util.function.Predicate; import javax.annotation.Nullable; import javax.persistence.Access; import javax.persistence.AccessType; @@ -323,15 +323,9 @@ public class DomainContent extends EppResource .collect(toImmutableSet()); } - /** - * The {@link javax.persistence.PostLoad} method for {@link DomainContent}. - * - *

We name this domainContentPostLoad to distinguish it from the {@link PostLoad} method in - * DomainBase (if they share the same name, this one is never called). - */ @PostLoad @SuppressWarnings("UnusedMethod") - private final void domainContentPostLoad() { + private final void postLoad() { // Reconstitute the contact list. ImmutableSet.Builder contactsBuilder = new ImmutableSet.Builder<>(); @@ -350,34 +344,9 @@ public class DomainContent extends EppResource } allContacts = contactsBuilder.build(); - } - /** - * Restores the composite ofy keys from SQL data. - * - *

MUST ONLY BE CALLED FROM A PostLoad method. This is a package-visible method that - * effectively mutates an immutable object. - * - *

We have to do this because: - * - *

    - *
  • We've changed the {@link PostLoad} method behavior to make all {@link PostLoad} calls in - * the class hierarchy (and not merely the most specific one) be called after an object is - * loaded. - *
  • When restoring a {@link DomainBase} object (which is a subclass) the repo id is not - * populated until after our {@link PostLoad} method is called. Therefore, we need to - * restore these ofy keys (which depend on the repo id) from {@link DomainBase}'s {@link - * PostLoad} method. - *
  • When restoring a {@link DomainHistory} object, hibernate restores a {@link DomainContent} - * instance, therefore we need our own {@link PostLoad} method to restore the other fields. - * In order to restore the ofy keys, we need to invoke this method separately from {@link - * DomainHistory}'s {@link PostLoad} method and pass in the repo id, which is stored in a - * different field in {@link DomainHistory}. - *
- */ - void restoreOfyKeys(String repoId) { - // Reconstitute the ofy keys. - Key myKey = Key.create(DomainBase.class, repoId); + // Reconstitute the composite ofy keys from the SQL data. + Key myKey = Key.create(DomainBase.class, getRepoId()); deletePollMessage = restoreOfyFrom(myKey, deletePollMessage, deletePollMessageHistoryId); autorenewBillingEvent = restoreOfyFrom(myKey, autorenewBillingEvent, autorenewBillingEventHistoryId); @@ -764,7 +733,7 @@ public class DomainContent extends EppResource /** An override of {@link EppResource#asBuilder} with tighter typing. */ @Override - public Builder asBuilder() { + 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 0c1cfc1fd..228937834 100644 --- a/core/src/main/java/google/registry/model/domain/DomainHistory.java +++ b/core/src/main/java/google/registry/model/domain/DomainHistory.java @@ -20,7 +20,6 @@ import com.google.common.collect.ImmutableList; import com.googlecode.objectify.Key; import com.googlecode.objectify.annotation.EntitySubclass; import com.googlecode.objectify.annotation.Ignore; -import google.registry.model.EppResource; import google.registry.model.ImmutableObject; import google.registry.model.domain.DomainHistory.DomainHistoryId; import google.registry.model.host.HostResource; @@ -73,7 +72,17 @@ public class DomainHistory extends HistoryEntry implements SqlEntity { // Store DomainContent instead of DomainBase so we don't pick up its @Id @Nullable DomainContent domainContent; - @Id String domainRepoId; + @Id + @Access(AccessType.PROPERTY) + public String getDomainRepoId() { + return parent.getName(); + } + + /** This method is private because it is only used by Hibernate. */ + @SuppressWarnings("unused") + private void setDomainRepoId(String domainRepoId) { + parent = Key.create(DomainBase.class, domainRepoId); + } // We could have reused domainContent.nsHosts here, but Hibernate throws a weird exception after // we change to use a composite primary key. @@ -152,14 +161,14 @@ public class DomainHistory extends HistoryEntry implements SqlEntity { } /** The key to the {@link DomainBase} this is based off of. */ - public VKey getDomainRepoId() { - return VKey.create(DomainBase.class, domainRepoId, Key.create(DomainBase.class, domainRepoId)); + public VKey getParentVKey() { + return VKey.create(DomainBase.class, getDomainRepoId()); } /** Creates a {@link VKey} instance for this entity. */ public VKey createVKey() { return VKey.create( - DomainHistory.class, new DomainHistoryId(domainRepoId, getId()), Key.create(this)); + DomainHistory.class, new DomainHistoryId(getDomainRepoId(), getId()), Key.create(this)); } @PostLoad @@ -171,10 +180,11 @@ public class DomainHistory extends HistoryEntry implements SqlEntity { if (domainContent.getDomainName() == null) { domainContent = null; } else { - domainContent.restoreOfyKeys(domainRepoId); + if (domainContent.getRepoId() == null) { + domainContent = domainContent.asBuilder().setRepoId(parent.getName()).build(); + } } } - parent = Key.create(DomainBase.class, domainRepoId); } // In Datastore, save as a HistoryEntry object regardless of this object's type @@ -263,17 +273,8 @@ public class DomainHistory extends HistoryEntry implements SqlEntity { } public Builder setDomainRepoId(String domainRepoId) { - getInstance().domainRepoId = domainRepoId; getInstance().parent = Key.create(DomainBase.class, domainRepoId); return this; } - - // We can remove this once all HistoryEntries are converted to History objects - @Override - public Builder setParent(Key parent) { - super.setParent(parent); - getInstance().domainRepoId = parent.getName(); - return this; - } } } diff --git a/core/src/main/java/google/registry/model/eppcommon/StatusValue.java b/core/src/main/java/google/registry/model/eppcommon/StatusValue.java index 9911b065e..5aa27deee 100644 --- a/core/src/main/java/google/registry/model/eppcommon/StatusValue.java +++ b/core/src/main/java/google/registry/model/eppcommon/StatusValue.java @@ -20,8 +20,10 @@ import static com.google.common.base.Strings.nullToEmpty; import com.google.common.collect.ImmutableSet; import google.registry.model.EppResource; +import google.registry.model.contact.ContactBase; import google.registry.model.contact.ContactResource; import google.registry.model.domain.DomainBase; +import google.registry.model.domain.DomainContent; import google.registry.model.host.HostBase; import google.registry.model.host.HostResource; import google.registry.model.translators.EnumToAttributeAdapter.EppEnum; @@ -128,9 +130,15 @@ public enum StatusValue implements EppEnum { /** Enum to help clearly list which resource types a status value is allowed to be present on. */ private enum AllowedOn { - ALL(ContactResource.class, DomainBase.class, HostBase.class, HostResource.class), + ALL( + ContactBase.class, + ContactResource.class, + DomainContent.class, + DomainBase.class, + HostBase.class, + HostResource.class), NONE, - DOMAINS(DomainBase.class); + DOMAINS(DomainContent.class, DomainBase.class); private final ImmutableSet> classes; 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 af9e2f34e..078020037 100644 --- a/core/src/main/java/google/registry/model/host/HostHistory.java +++ b/core/src/main/java/google/registry/model/host/HostHistory.java @@ -14,11 +14,9 @@ package google.registry.model.host; - import com.google.common.collect.ImmutableList; import com.googlecode.objectify.Key; import com.googlecode.objectify.annotation.EntitySubclass; -import google.registry.model.EppResource; import google.registry.model.ImmutableObject; import google.registry.model.host.HostHistory.HostHistoryId; import google.registry.model.reporting.HistoryEntry; @@ -60,7 +58,17 @@ public class HostHistory extends HistoryEntry implements SqlEntity { // Store HostBase instead of HostResource so we don't pick up its @Id @Nullable HostBase hostBase; - @Id String hostRepoId; + @Id + @Access(AccessType.PROPERTY) + public String getHostRepoId() { + return parent.getName(); + } + + /** This method is private because it is only used by Hibernate. */ + @SuppressWarnings("unused") + private void setHostRepoId(String hostRepoId) { + parent = Key.create(HostResource.class, hostRepoId); + } @Id @Column(name = "historyRevisionId") @@ -81,13 +89,14 @@ public class HostHistory extends HistoryEntry implements SqlEntity { } /** The key to the {@link google.registry.model.host.HostResource} this is based off of. */ - public VKey getHostRepoId() { - return VKey.create(HostResource.class, hostRepoId, Key.create(HostResource.class, hostRepoId)); + public VKey getParentVKey() { + return VKey.create(HostResource.class, getHostRepoId()); } /** Creates a {@link VKey} instance for this entity. */ public VKey createVKey() { - return VKey.create(HostHistory.class, new HostHistoryId(hostRepoId, getId()), Key.create(this)); + return VKey.create( + HostHistory.class, new HostHistoryId(getHostRepoId(), getId()), Key.create(this)); } @PostLoad @@ -97,8 +106,6 @@ public class HostHistory extends HistoryEntry implements SqlEntity { if (hostBase != null && hostBase.getHostName() == null) { hostBase = null; } - // Fill in the full, symmetric, parent repo ID key - parent = Key.create(HostResource.class, hostRepoId); } // In Datastore, save as a HistoryEntry object regardless of this object's type @@ -184,17 +191,8 @@ public class HostHistory extends HistoryEntry implements SqlEntity { } public Builder setHostRepoId(String hostRepoId) { - getInstance().hostRepoId = hostRepoId; getInstance().parent = Key.create(HostResource.class, hostRepoId); return this; } - - // We can remove this once all HistoryEntries are converted to History objects - @Override - public Builder setParent(Key parent) { - super.setParent(parent); - getInstance().hostRepoId = parent.getName(); - return this; - } } } diff --git a/core/src/main/java/google/registry/persistence/VKey.java b/core/src/main/java/google/registry/persistence/VKey.java index b8fda844f..edce5a70b 100644 --- a/core/src/main/java/google/registry/persistence/VKey.java +++ b/core/src/main/java/google/registry/persistence/VKey.java @@ -88,7 +88,7 @@ public class VKey extends ImmutableObject implements Serializable { */ public static VKey create(Class kind, long id) { checkArgument( - kind.isAssignableFrom(BackupGroupRoot.class), + BackupGroupRoot.class.isAssignableFrom(kind), "The kind %s is not a BackupGroupRoot and thus needs its entire entity group chain" + " specified in a parent", kind.getCanonicalName()); @@ -106,7 +106,7 @@ public class VKey extends ImmutableObject implements Serializable { */ public static VKey create(Class kind, String name) { checkArgument( - kind.isAssignableFrom(BackupGroupRoot.class), + BackupGroupRoot.class.isAssignableFrom(kind), "The kind %s is not a BackupGroupRoot and thus needs its entire entity group chain" + " specified in a parent", kind.getCanonicalName()); 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 62428abe6..879276623 100644 --- a/core/src/test/java/google/registry/model/history/ContactHistoryTest.java +++ b/core/src/test/java/google/registry/model/history/ContactHistoryTest.java @@ -54,8 +54,7 @@ public class ContactHistoryTest extends EntityTestCase { () -> { ContactHistory fromDatabase = jpaTm().load(contactHistory.createVKey()); assertContactHistoriesEqual(fromDatabase, contactHistory); - assertThat(fromDatabase.getContactRepoId().getSqlKey()) - .isEqualTo(contactHistory.getContactRepoId().getSqlKey()); + assertThat(fromDatabase.getParentVKey()).isEqualTo(contactHistory.getParentVKey()); }); } @@ -79,8 +78,7 @@ public class ContactHistoryTest extends EntityTestCase { () -> { ContactHistory fromDatabase = jpaTm().load(contactHistory.createVKey()); assertContactHistoriesEqual(fromDatabase, contactHistory); - assertThat(fromDatabase.getContactRepoId().getSqlKey()) - .isEqualTo(contactHistory.getContactRepoId().getSqlKey()); + assertThat(fromDatabase.getParentVKey()).isEqualTo(contactHistory.getParentVKey()); }); } 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 0d2f930a4..cbeb24876 100644 --- a/core/src/test/java/google/registry/model/history/DomainHistoryTest.java +++ b/core/src/test/java/google/registry/model/history/DomainHistoryTest.java @@ -66,8 +66,7 @@ public class DomainHistoryTest extends EntityTestCase { () -> { DomainHistory fromDatabase = jpaTm().load(domainHistory.createVKey()); assertDomainHistoriesEqual(fromDatabase, domainHistory); - assertThat(fromDatabase.getDomainRepoId().getSqlKey()) - .isEqualTo(domainHistory.getDomainRepoId().getSqlKey()); + assertThat(fromDatabase.getParentVKey()).isEqualTo(domainHistory.getParentVKey()); }); } @@ -83,8 +82,7 @@ public class DomainHistoryTest extends EntityTestCase { () -> { DomainHistory fromDatabase = jpaTm().load(domainHistory.createVKey()); assertDomainHistoriesEqual(fromDatabase, domainHistory); - assertThat(fromDatabase.getDomainRepoId().getSqlKey()) - .isEqualTo(domainHistory.getDomainRepoId().getSqlKey()); + assertThat(fromDatabase.getParentVKey()).isEqualTo(domainHistory.getParentVKey()); assertThat(fromDatabase.getNsHosts()) .containsExactlyElementsIn( domainHistory.getNsHosts().stream() 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 1604ef98d..11e037f30 100644 --- a/core/src/test/java/google/registry/model/history/HostHistoryTest.java +++ b/core/src/test/java/google/registry/model/history/HostHistoryTest.java @@ -55,8 +55,7 @@ public class HostHistoryTest extends EntityTestCase { () -> { HostHistory fromDatabase = jpaTm().load(hostHistory.createVKey()); assertHostHistoriesEqual(fromDatabase, hostHistory); - assertThat(fromDatabase.getHostRepoId().getSqlKey()) - .isEqualTo(hostHistory.getHostRepoId().getSqlKey()); + assertThat(fromDatabase.getParentVKey()).isEqualTo(hostHistory.getParentVKey()); }); } @@ -76,8 +75,7 @@ public class HostHistoryTest extends EntityTestCase { () -> { HostHistory fromDatabase = jpaTm().load(hostHistory.createVKey()); assertHostHistoriesEqual(fromDatabase, hostHistory); - assertThat(fromDatabase.getHostRepoId().getSqlKey()) - .isEqualTo(hostHistory.getHostRepoId().getSqlKey()); + assertThat(fromDatabase.getParentVKey()).isEqualTo(hostHistory.getParentVKey()); }); } diff --git a/core/src/test/java/google/registry/model/history/LegacyHistoryObjectTest.java b/core/src/test/java/google/registry/model/history/LegacyHistoryObjectTest.java index ce454c00b..5169c0d6e 100644 --- a/core/src/test/java/google/registry/model/history/LegacyHistoryObjectTest.java +++ b/core/src/test/java/google/registry/model/history/LegacyHistoryObjectTest.java @@ -92,8 +92,8 @@ public class LegacyHistoryObjectTest extends EntityTestCase { .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()); + assertThat(legacyContactHistory.getParentVKey().getSqlKey()) + .isEqualTo(legacyHistoryFromSql.getParentVKey().getSqlKey()); } @Test @@ -169,8 +169,8 @@ public class LegacyHistoryObjectTest extends EntityTestCase { jpaTm().transact(() -> jpaTm().load(legacyHostHistory.createVKey())); 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()); + assertThat(legacyHostHistory.getParentVKey().getSqlKey()) + .isEqualTo(legacyHistoryFromSql.getParentVKey().getSqlKey()); } private HistoryEntry historyEntryForDomain(DomainBase domain) { diff --git a/core/src/test/resources/google/registry/model/schema.txt b/core/src/test/resources/google/registry/model/schema.txt index 076502d99..eaae1ad5c 100644 --- a/core/src/test/resources/google/registry/model/schema.txt +++ b/core/src/test/resources/google/registry/model/schema.txt @@ -133,7 +133,6 @@ class google.registry.model.contact.ContactHistory { google.registry.model.reporting.HistoryEntry$Type type; java.lang.Boolean requestedByRegistrar; java.lang.String clientId; - java.lang.String contactRepoId; java.lang.String otherClientId; java.lang.String reason; java.util.Set domainTransactionRecords; @@ -272,7 +271,6 @@ class google.registry.model.domain.DomainHistory { google.registry.model.reporting.HistoryEntry$Type type; java.lang.Boolean requestedByRegistrar; java.lang.String clientId; - java.lang.String domainRepoId; java.lang.String otherClientId; java.lang.String reason; java.util.Set domainTransactionRecords; @@ -403,7 +401,6 @@ class google.registry.model.host.HostHistory { google.registry.model.reporting.HistoryEntry$Type type; java.lang.Boolean requestedByRegistrar; java.lang.String clientId; - java.lang.String hostRepoId; java.lang.String otherClientId; java.lang.String reason; java.util.Set domainTransactionRecords;