From 5b7514096ff201d87d1bcd8649612fd049def1a6 Mon Sep 17 00:00:00 2001 From: gbrodman Date: Wed, 23 Feb 2022 11:07:10 -0500 Subject: [PATCH] Fix DTR creation in one location and clean up replay comparison (#1529) * Fix DTR creation in one location and clean up replay comparison --- .../registry/flows/domain/DomainFlowUtils.java | 11 ++++++++++- .../registry/model/contact/ContactHistory.java | 2 +- .../google/registry/model/domain/DomainHistory.java | 5 ++++- .../google/registry/model/host/HostHistory.java | 2 +- .../registry/model/reporting/HistoryEntry.java | 4 ++-- .../google/registry/testing/ReplayExtension.java | 13 ++++--------- 6 files changed, 22 insertions(+), 15 deletions(-) diff --git a/core/src/main/java/google/registry/flows/domain/DomainFlowUtils.java b/core/src/main/java/google/registry/flows/domain/DomainFlowUtils.java index 4a7042bda..1efab8370 100644 --- a/core/src/main/java/google/registry/flows/domain/DomainFlowUtils.java +++ b/core/src/main/java/google/registry/flows/domain/DomainFlowUtils.java @@ -1112,7 +1112,16 @@ public class DomainFlowUtils { // Only cancel fields which are cancelable if (cancelableFields.contains(record.getReportField())) { int cancelledAmount = -1 * record.getReportAmount(); - recordsBuilder.add(record.asBuilder().setReportAmount(cancelledAmount).build()); + // NB: It's necessary to create a new DomainTransactionRecord from scratch so that we + // don't retain the ID of the previous record to cancel. If we keep the ID, Hibernate + // will remove that record from the DB entirely as the record will be re-parented on + // this DomainHistory being created now. + recordsBuilder.add( + DomainTransactionRecord.create( + record.getTld(), + record.getReportingTime(), + record.getReportField(), + cancelledAmount)); } } } 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 ab2f6de18..82a44119e 100644 --- a/core/src/main/java/google/registry/model/contact/ContactHistory.java +++ b/core/src/main/java/google/registry/model/contact/ContactHistory.java @@ -64,7 +64,7 @@ public class ContactHistory extends HistoryEntry implements SqlEntity, UnsafeSer // Store ContactBase instead of ContactResource so we don't pick up its @Id // Nullable for the sake of pre-Registry-3.0 history objects - @Nullable ContactBase contactBase; + @DoNotCompare @Nullable ContactBase contactBase; @Id @Access(AccessType.PROPERTY) 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 e804fc42f..ad7b2e02f 100644 --- a/core/src/main/java/google/registry/model/domain/DomainHistory.java +++ b/core/src/main/java/google/registry/model/domain/DomainHistory.java @@ -86,7 +86,7 @@ public class DomainHistory extends HistoryEntry implements SqlEntity { // Store DomainContent instead of DomainBase so we don't pick up its @Id // Nullable for the sake of pre-Registry-3.0 history objects - @Nullable DomainContent domainContent; + @DoNotCompare @Nullable DomainContent domainContent; @Id @Access(AccessType.PROPERTY) @@ -105,6 +105,7 @@ public class DomainHistory extends HistoryEntry implements SqlEntity { // We could have reused domainContent.nsHosts here, but Hibernate throws a weird exception after // we change to use a composite primary key. // TODO(b/166776754): Investigate if we can reuse domainContent.nsHosts for storing host keys. + @DoNotCompare @ElementCollection @JoinTable( name = "DomainHistoryHost", @@ -118,6 +119,7 @@ public class DomainHistory extends HistoryEntry implements SqlEntity { @Column(name = "host_repo_id") Set> nsHosts; + @DoNotCompare @OneToMany( cascade = {CascadeType.ALL}, fetch = FetchType.EAGER, @@ -137,6 +139,7 @@ public class DomainHistory extends HistoryEntry implements SqlEntity { // HashSet rather than ImmutableSet so that Hibernate can fill them out lazily on request Set dsDataHistories = new HashSet<>(); + @DoNotCompare @OneToMany( cascade = {CascadeType.ALL}, fetch = FetchType.EAGER, 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 0831a65e6..2f54dc24a 100644 --- a/core/src/main/java/google/registry/model/host/HostHistory.java +++ b/core/src/main/java/google/registry/model/host/HostHistory.java @@ -66,7 +66,7 @@ public class HostHistory extends HistoryEntry implements SqlEntity, UnsafeSerial // Store HostBase instead of HostResource so we don't pick up its @Id // Nullable for the sake of pre-Registry-3.0 history objects - @Nullable HostBase hostBase; + @DoNotCompare @Nullable HostBase hostBase; @Id @Access(AccessType.PROPERTY) 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 b9d9e7d13..9cd6cef53 100644 --- a/core/src/main/java/google/registry/model/reporting/HistoryEntry.java +++ b/core/src/main/java/google/registry/model/reporting/HistoryEntry.java @@ -324,7 +324,7 @@ public class HistoryEntry extends ImmutableObject // Note: how we wish to treat this Hibernate setter depends on the current state of the object // and what's passed in. The key principle is that we wish to maintain the link between parent // and child objects, meaning that we should keep around whichever of the two sets (the - // parameter vs the class variable and clear/populate that as appropriate. + // parameter vs the class variable) and clear/populate that as appropriate. // // If the class variable is a PersistentSet and we overwrite it here, Hibernate will throw // an exception "A collection with cascade=”all-delete-orphan” was no longer referenced by the @@ -539,7 +539,7 @@ public class HistoryEntry extends ImmutableObject public B setDomainTransactionRecords( ImmutableSet domainTransactionRecords) { - getInstance().domainTransactionRecords = domainTransactionRecords; + getInstance().setDomainTransactionRecords(domainTransactionRecords); return thisCastToDerived(); } } diff --git a/core/src/test/java/google/registry/testing/ReplayExtension.java b/core/src/test/java/google/registry/testing/ReplayExtension.java index 53aeb8cbb..9499fd78f 100644 --- a/core/src/test/java/google/registry/testing/ReplayExtension.java +++ b/core/src/test/java/google/registry/testing/ReplayExtension.java @@ -100,17 +100,12 @@ public class ReplayExtension implements BeforeEachCallback, AfterEachCallback { private static ImmutableSet IGNORED_ENTITIES = Streams.concat( ImmutableSet.of( - // These entities *should* be comparable, but this isn't working yet so exclude - // them so we can tackle them independently. + // These entities are @Embed-ded in Datastore + "DelegationSignerData", + "DomainDsDataHistory", + "DomainTransactionRecord", "GracePeriod", "GracePeriodHistory", - "HistoryEntry", - "DomainHistory", - "ContactHistory", - "HostHistory", - "DomainDsDataHistory", - "DelegationSignerData", - "DomainTransactionRecord", // These entities are legitimately not comparable. "ClaimsEntry",