zydronium/google-nomulus
1
0
Fork 0
mirror of https://github.com/google/nomulus.git synced 2025-05-30 01:10:14 +02:00
Code Issues Projects Releases Packages Wiki Activity

Use the parent to store the history repo ID and fill in the base object (#830)

Browse source 
* 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
This commit is contained in:
gbrodman 2020-10-09 16:01:51 -04:00 • committed by GitHub
parent 7a68b1b6f0
commit 4ec7f23e84
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
12 changed files with 73 additions and 116 deletions
Download patch file Download diff file Expand all files Collapse all files

31
core/src/main/java/google/registry/model/contact/ContactHistory.java
View file

@ -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<ContactResource> getContactRepoId() {
return VKey.create(
ContactResource.class, contactRepoId, Key.create(ContactResource.class, contactRepoId));
public VKey<ContactResource> getParentVKey() {
return VKey.create(ContactResource.class, getContactRepoId());
}
/** Creates a {@link VKey} instance for this entity. */
public VKey<ContactHistory> 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<? extends EppResource> parent) {
super.setParent(parent);
getInstance().contactRepoId = parent.getName();
return this;
}
}
}

7
core/src/main/java/google/registry/model/domain/DomainBase.java
View file

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

41
core/src/main/java/google/registry/model/domain/DomainContent.java
View file

@ -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}.
*
* <p>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<DesignatedContact> contactsBuilder = new ImmutableSet.Builder<>();
@ -350,34 +344,9 @@ public class DomainContent extends EppResource
}
allContacts = contactsBuilder.build();
}
/**
* Restores the composite ofy keys from SQL data.
*
* <p>MUST ONLY BE CALLED FROM A PostLoad method. This is a package-visible method that
* effectively mutates an immutable object.
*
* <p>We have to do this because:
*
* <ul>
* <li>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.
* <li>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.
* <li>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}.
* </ul>
*/
void restoreOfyKeys(String repoId) {
// Reconstitute the ofy keys.
Key<DomainBase> myKey = Key.create(DomainBase.class, repoId);
// Reconstitute the composite ofy keys from the SQL data.
Key<DomainBase> 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<? extends DomainContent, ?> asBuilder() {
return new Builder<>(clone(this));
}

33
core/src/main/java/google/registry/model/domain/DomainHistory.java
View file

@ -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<DomainBase> getDomainRepoId() {
return VKey.create(DomainBase.class, domainRepoId, Key.create(DomainBase.class, domainRepoId));
public VKey<DomainBase> getParentVKey() {
return VKey.create(DomainBase.class, getDomainRepoId());
}
/** Creates a {@link VKey} instance for this entity. */
public VKey<DomainHistory> 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<? extends EppResource> parent) {
super.setParent(parent);
getInstance().domainRepoId = parent.getName();
return this;
}
}
}

12
core/src/main/java/google/registry/model/eppcommon/StatusValue.java
View file

@ -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<Class<? extends EppResource>> classes;

32
core/src/main/java/google/registry/model/host/HostHistory.java
View file

@ -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<HostResource> getHostRepoId() {
return VKey.create(HostResource.class, hostRepoId, Key.create(HostResource.class, hostRepoId));
public VKey<HostResource> getParentVKey() {
return VKey.create(HostResource.class, getHostRepoId());
}
/** Creates a {@link VKey} instance for this entity. */
public VKey<HostHistory> 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<? extends EppResource> parent) {
super.setParent(parent);
getInstance().hostRepoId = parent.getName();
return this;
}
}
}

4
core/src/main/java/google/registry/persistence/VKey.java
View file

@ -88,7 +88,7 @@ public class VKey<T> extends ImmutableObject implements Serializable {
*/
public static <T> VKey<T> create(Class<T> 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<T> extends ImmutableObject implements Serializable {
*/
public static <T> VKey<T> create(Class<T> 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());

6
core/src/test/java/google/registry/model/history/ContactHistoryTest.java
View file

@ -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());
});
}

6
core/src/test/java/google/registry/model/history/DomainHistoryTest.java
View file

@ -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()

6
core/src/test/java/google/registry/model/history/HostHistoryTest.java
View file

@ -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());
});
}

8
core/src/test/java/google/registry/model/history/LegacyHistoryObjectTest.java
View file

@ -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) {

3
core/src/test/resources/google/registry/model/schema.txt
View file

@ -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<google.registry.model.reporting.DomainTransactionRecord> 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<google.registry.model.reporting.DomainTransactionRecord> 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<google.registry.model.reporting.DomainTransactionRecord> domainTransactionRecords;