Fix cascade issue for GracePeriod (#775)

* Fix cascade issue for GracePeriod

* Rebase on HEAD

* Make GracePeriod immutable

* Add javadoc and use nullToEmptyImmutableCopy

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.
+   *
+   * <p>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.
+   *
+   * <p>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<GracePeriod> getInternalGracePeriods() {
     return gracePeriods;

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 =
+        nullToEmptyImmutableCopy(gracePeriods).stream()
-          gracePeriods.stream()
+            .map(gracePeriod -> gracePeriod.cloneWithDomainRepoId(getRepoId()))
-              .map(gracePeriod -> gracePeriod.cloneWithDomainRepoId(getRepoId()))
+            .collect(toImmutableSet());
-              .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) {

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()
+    persistDomain();
-        .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);
-            });
 
     jpaTm()
         .transact(
             () -> {
-              // Load the domain in its entirety.
+              DomainBase result = jpaTm().load(domain.createVKey());
-              EntityManager em = jpaTm().getEntityManager();
+              assertEqualDomainExcept(result);
-              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");
             });
   }
 
   @Test
   void testHostForeignKeyConstraints() {
     assertThrowForeignKeyViolation(
-        () -> {
+        () ->
-          jpaTm()
+            jpaTm()
-              .transact(
+                .transact(
-                  () -> {
+                    () -> {
-                    // Persist the domain without the associated host object.
+                      // Persist the domain without the associated host object.
-                    jpaTm().saveNew(contact);
+                      jpaTm().saveNew(contact);
-                    jpaTm().saveNew(contact2);
+                      jpaTm().saveNew(contact2);
-                    jpaTm().saveNew(domain);
+                      jpaTm().saveNew(domain);
-                  });
+                    }));
-        });
   }
 
   @Test
   void testContactForeignKeyConstraints() {
     assertThrowForeignKeyViolation(
-        () -> {
+        () ->
-          jpaTm()
+            jpaTm()
-              .transact(
+                .transact(
-                  () -> {
+                    () -> {
-                    // Persist the domain without the associated contact objects.
+                      // Persist the domain without the associated contact objects.
-                    jpaTm().saveNew(domain);
+                      jpaTm().saveNew(domain);
-                    jpaTm().saveNew(host);
+                      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);
+  }
 }