From 5f6ea2cbf26b634f8ca129f77cfffb6af46d0eda Mon Sep 17 00:00:00 2001 From: Shicong Huang Date: Wed, 2 Sep 2020 20:05:53 -0400 Subject: [PATCH] Fix cascade issue for GracePeriod (#775) * Fix cascade issue for GracePeriod * Rebase on HEAD * Make GracePeriod immutable * Add javadoc and use nullToEmptyImmutableCopy --- .../registry/model/domain/DomainBase.java | 17 +- .../registry/model/domain/DomainContent.java | 18 +- .../model/domain/DomainBaseSqlTest.java | 294 ++++++++++++++---- 3 files changed, 263 insertions(+), 66 deletions(-) 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 0565f3161..285c311f9 100644 --- a/core/src/main/java/google/registry/model/domain/DomainBase.java +++ b/core/src/main/java/google/registry/model/domain/DomainBase.java @@ -14,7 +14,6 @@ package google.registry.model.domain; - import com.googlecode.objectify.Key; import google.registry.model.EppResource; import google.registry.model.EppResource.ForeignKeyedEppResource; @@ -82,12 +81,26 @@ public class DomainBase extends DomainContent return super.nsHosts; } + /** + * Returns the set of {@link GracePeriod} associated with the domain. + * + *

This is the getter method specific for Hibernate to access the field so it is set to + * private. The caller can use the public {@link #getGracePeriods()} to get the grace periods. + * + *

Note that we need to set `insertable = false, updatable = false` for @JoinColumn, otherwise + * Hibernate would try to set the foreign key to null(through an UPDATE TABLE sql) instead of + * deleting the whole entry from the table when the {@link GracePeriod} is removed from the set. + */ @Access(AccessType.PROPERTY) @OneToMany( cascade = {CascadeType.ALL}, fetch = FetchType.EAGER, orphanRemoval = true) - @JoinColumn(name = "domainRepoId", referencedColumnName = "repoId") + @JoinColumn( + name = "domainRepoId", + referencedColumnName = "repoId", + insertable = false, + updatable = false) @SuppressWarnings("UnusedMethod") private Set getInternalGracePeriods() { return gracePeriods; 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 3c8143d22..32ecd0071 100644 --- a/core/src/main/java/google/registry/model/domain/DomainContent.java +++ b/core/src/main/java/google/registry/model/domain/DomainContent.java @@ -280,12 +280,10 @@ public class DomainContent extends EppResource // object will have a null hashcode so that it can get a recalculated hashcode // when its hashCode() is invoked. // TODO(b/162739503): Remove this after fully migrating to Cloud SQL. - if (gracePeriods != null) { - gracePeriods = - gracePeriods.stream() - .map(gracePeriod -> gracePeriod.cloneWithDomainRepoId(getRepoId())) - .collect(toImmutableSet()); - } + gracePeriods = + nullToEmptyImmutableCopy(gracePeriods).stream() + .map(gracePeriod -> gracePeriod.cloneWithDomainRepoId(getRepoId())) + .collect(toImmutableSet()); } @PostLoad @@ -698,7 +696,13 @@ public class DomainContent extends EppResource } checkArgumentNotNull(instance.getRegistrant(), "Missing registrant"); instance.tld = getTldFromDomainName(instance.fullyQualifiedDomainName); - return super.build(); + + T newDomain = super.build(); + // Hibernate throws exception if gracePeriods is null because we enabled all cascadable + // operations and orphan removal. + newDomain.gracePeriods = + newDomain.gracePeriods == null ? ImmutableSet.of() : newDomain.gracePeriods; + return newDomain; } public B setDomainName(String domainName) { diff --git a/core/src/test/java/google/registry/model/domain/DomainBaseSqlTest.java b/core/src/test/java/google/registry/model/domain/DomainBaseSqlTest.java index a6ecd6fab..1957534a0 100644 --- a/core/src/test/java/google/registry/model/domain/DomainBaseSqlTest.java +++ b/core/src/test/java/google/registry/model/domain/DomainBaseSqlTest.java @@ -14,6 +14,7 @@ package google.registry.model.domain; +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.testing.SqlHelper.assertThrowForeignKeyViolation; @@ -21,6 +22,7 @@ import static google.registry.testing.SqlHelper.saveRegistrar; import static google.registry.util.DateTimeUtils.END_OF_TIME; import static google.registry.util.DateTimeUtils.START_OF_TIME; import static org.joda.time.DateTimeZone.UTC; +import static org.junit.jupiter.api.Assertions.fail; import com.google.common.collect.ImmutableSet; import google.registry.model.contact.ContactResource; @@ -37,7 +39,7 @@ import google.registry.persistence.transaction.JpaTestRules; import google.registry.persistence.transaction.JpaTestRules.JpaIntegrationWithCoverageExtension; import google.registry.testing.DatastoreEntityExtension; import google.registry.testing.FakeClock; -import javax.persistence.EntityManager; +import java.util.Arrays; import org.joda.time.DateTime; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Order; @@ -119,77 +121,217 @@ public class DomainBaseSqlTest { @Test void testDomainBasePersistence() { - jpaTm() - .transact( - () -> { - // Persist the contacts. Note that these need to be persisted before the domain - // otherwise we get a foreign key constraint error. If we ever decide to defer the - // relevant foreign key checks to commit time, then the order would not matter. - jpaTm().saveNew(contact); - jpaTm().saveNew(contact2); - - // Persist the domain. - jpaTm().saveNew(domain); - - // Persist the host. This does _not_ need to be persisted before the domain, - // because only the row in the join table (DomainHost) is subject to foreign key - // constraints, and Hibernate knows to insert it after domain and host. - jpaTm().saveNew(host); - }); + persistDomain(); jpaTm() .transact( () -> { - // Load the domain in its entirety. - EntityManager em = jpaTm().getEntityManager(); - DomainBase result = em.find(DomainBase.class, "4-COM"); - - // Fix DS data, since we can't persist it yet. - result = - result - .asBuilder() - .setDsData( - ImmutableSet.of( - DelegationSignerData.create(1, 2, 3, new byte[] {0, 1, 2}))) - .build(); - - // Fix the original creation timestamp (this gets initialized on first write) - DomainBase org = domain.asBuilder().setCreationTime(result.getCreationTime()).build(); - - // Note that the equality comparison forces a lazy load of all fields. - assertAboutImmutableObjects() - .that(result) - .isEqualExceptFields(org, "updateTimestamp"); + DomainBase result = jpaTm().load(domain.createVKey()); + assertEqualDomainExcept(result); }); } @Test void testHostForeignKeyConstraints() { assertThrowForeignKeyViolation( - () -> { - jpaTm() - .transact( - () -> { - // Persist the domain without the associated host object. - jpaTm().saveNew(contact); - jpaTm().saveNew(contact2); - jpaTm().saveNew(domain); - }); - }); + () -> + jpaTm() + .transact( + () -> { + // Persist the domain without the associated host object. + jpaTm().saveNew(contact); + jpaTm().saveNew(contact2); + jpaTm().saveNew(domain); + })); } @Test void testContactForeignKeyConstraints() { assertThrowForeignKeyViolation( - () -> { - jpaTm() - .transact( - () -> { - // Persist the domain without the associated contact objects. - jpaTm().saveNew(domain); - jpaTm().saveNew(host); - }); - }); + () -> + jpaTm() + .transact( + () -> { + // Persist the domain without the associated contact objects. + jpaTm().saveNew(domain); + jpaTm().saveNew(host); + })); + } + + @Test + void testResaveDomain_succeeds() { + persistDomain(); + jpaTm() + .transact( + () -> { + DomainBase persisted = jpaTm().load(domain.createVKey()); + jpaTm().saveNewOrUpdate(persisted.asBuilder().build()); + }); + jpaTm() + .transact( + () -> { + // Load the domain in its entirety. + DomainBase result = jpaTm().load(domain.createVKey()); + assertEqualDomainExcept(result); + }); + } + + @Test + void testModifyGracePeriod_setEmptyCollectionSuccessfully() { + persistDomain(); + jpaTm() + .transact( + () -> { + DomainBase persisted = jpaTm().load(domain.createVKey()); + DomainBase modified = + persisted.asBuilder().setGracePeriods(ImmutableSet.of()).build(); + jpaTm().saveNewOrUpdate(modified); + }); + + jpaTm() + .transact( + () -> { + DomainBase persisted = jpaTm().load(domain.createVKey()); + assertThat(persisted.getGracePeriods()).isEmpty(); + }); + } + + @Test + void testModifyGracePeriod_setNullCollectionSuccessfully() { + persistDomain(); + jpaTm() + .transact( + () -> { + DomainBase persisted = jpaTm().load(domain.createVKey()); + DomainBase modified = persisted.asBuilder().setGracePeriods(null).build(); + jpaTm().saveNewOrUpdate(modified); + }); + + jpaTm() + .transact( + () -> { + DomainBase persisted = jpaTm().load(domain.createVKey()); + assertThat(persisted.getGracePeriods()).isEmpty(); + }); + } + + @Test + void testModifyGracePeriod_addThenRemoveSuccessfully() { + persistDomain(); + jpaTm() + .transact( + () -> { + DomainBase persisted = jpaTm().load(domain.createVKey()); + DomainBase modified = + persisted + .asBuilder() + .addGracePeriod( + GracePeriod.create( + GracePeriodStatus.RENEW, "4-COM", END_OF_TIME, "registrar1", null)) + .build(); + jpaTm().saveNewOrUpdate(modified); + }); + + jpaTm() + .transact( + () -> { + DomainBase persisted = jpaTm().load(domain.createVKey()); + assertThat(persisted.getGracePeriods().size()).isEqualTo(2); + persisted + .getGracePeriods() + .forEach( + gracePeriod -> { + assertThat(gracePeriod.id).isNotNull(); + if (gracePeriod.getType() == GracePeriodStatus.ADD) { + assertAboutImmutableObjects() + .that(gracePeriod) + .isEqualExceptFields( + GracePeriod.create( + GracePeriodStatus.ADD, + "4-COM", + END_OF_TIME, + "registrar1", + null), + "id"); + } else if (gracePeriod.getType() == GracePeriodStatus.RENEW) { + assertAboutImmutableObjects() + .that(gracePeriod) + .isEqualExceptFields( + GracePeriod.create( + GracePeriodStatus.RENEW, + "4-COM", + END_OF_TIME, + "registrar1", + null), + "id"); + } else { + fail("Unexpected GracePeriod: " + gracePeriod); + } + }); + assertEqualDomainExcept(persisted, "gracePeriods"); + }); + + jpaTm() + .transact( + () -> { + DomainBase persisted = jpaTm().load(domain.createVKey()); + DomainBase.Builder builder = persisted.asBuilder(); + for (GracePeriod gracePeriod : persisted.getGracePeriods()) { + if (gracePeriod.getType() == GracePeriodStatus.RENEW) { + builder.removeGracePeriod(gracePeriod); + } + } + jpaTm().saveNewOrUpdate(builder.build()); + }); + + jpaTm() + .transact( + () -> { + DomainBase persisted = jpaTm().load(domain.createVKey()); + assertEqualDomainExcept(persisted); + }); + } + + @Test + void testModifyGracePeriod_removeThenAddSuccessfully() { + persistDomain(); + jpaTm() + .transact( + () -> { + DomainBase persisted = jpaTm().load(domain.createVKey()); + DomainBase modified = + persisted.asBuilder().setGracePeriods(ImmutableSet.of()).build(); + jpaTm().saveNewOrUpdate(modified); + }); + + jpaTm() + .transact( + () -> { + DomainBase persisted = jpaTm().load(domain.createVKey()); + assertThat(persisted.getGracePeriods()).isEmpty(); + DomainBase modified = + persisted + .asBuilder() + .addGracePeriod( + GracePeriod.create( + GracePeriodStatus.ADD, "4-COM", END_OF_TIME, "registrar1", null)) + .build(); + jpaTm().saveNewOrUpdate(modified); + }); + + jpaTm() + .transact( + () -> { + DomainBase persisted = jpaTm().load(domain.createVKey()); + assertThat(persisted.getGracePeriods().size()).isEqualTo(1); + assertAboutImmutableObjects() + .that(persisted.getGracePeriods().iterator().next()) + .isEqualExceptFields( + GracePeriod.create( + GracePeriodStatus.ADD, "4-COM", END_OF_TIME, "registrar1", null), + "id"); + assertEqualDomainExcept(persisted, "gracePeriods"); + }); } @Test @@ -232,4 +374,42 @@ public class DomainBaseSqlTest { .setPersistedCurrentSponsorClientId("registrar1") .build(); } + + private void persistDomain() { + jpaTm() + .transact( + () -> { + // Persist the contacts. Note that these need to be persisted before the domain + // otherwise we get a foreign key constraint error. If we ever decide to defer the + // relevant foreign key checks to commit time, then the order would not matter. + jpaTm().saveNew(contact); + jpaTm().saveNew(contact2); + + // Persist the domain. + jpaTm().saveNew(domain); + + // Persist the host. This does _not_ need to be persisted before the domain, + // because only the row in the join table (DomainHost) is subject to foreign key + // constraints, and Hibernate knows to insert it after domain and host. + jpaTm().saveNew(host); + }); + } + + private void assertEqualDomainExcept(DomainBase thatDomain, String... excepts) { + // Fix DS data, since we can't persist it yet. + thatDomain = + thatDomain + .asBuilder() + .setDsData(ImmutableSet.of(DelegationSignerData.create(1, 2, 3, new byte[] {0, 1, 2}))) + .build(); + + // Fix the original creation timestamp (this gets initialized on first write) + DomainBase org = domain.asBuilder().setCreationTime(thatDomain.getCreationTime()).build(); + + String[] moreExcepts = Arrays.copyOf(excepts, excepts.length + 1); + moreExcepts[moreExcepts.length - 1] = "updateTimestamp"; + + // Note that the equality comparison forces a lazy load of all fields. + assertAboutImmutableObjects().that(thatDomain).isEqualExceptFields(org, moreExcepts); + } }