Add SQL schema for DelegationSignerData (#713)

* Add SQL schema for DelegationSignerData

* Remove join table

* Rebased on HEAD

* Rebase on head

core/src/main/java/google/registry/flows/domain/DomainUpdateFlow.java

@@ -67,6 +67,7 @@ import google.registry.model.domain.DomainCommand.Update.AddRemove;
 import google.registry.model.domain.DomainCommand.Update.Change;
 import google.registry.model.domain.fee.FeeUpdateCommandExtension;
 import google.registry.model.domain.metadata.MetadataExtension;
+import google.registry.model.domain.secdns.DelegationSignerData;
 import google.registry.model.domain.secdns.SecDnsUpdateExtension;
 import google.registry.model.domain.superuser.DomainUpdateSuperuserExtension;
 import google.registry.model.eppcommon.AuthInfo;
@@ -238,10 +239,16 @@ public final class DomainUpdateFlow implements TransactionalFlow {
     DomainBase.Builder domainBuilder =
         domain
             .asBuilder()
-            // Handle the secDNS extension.
+            // Handle the secDNS extension. As dsData in secDnsUpdate is read from EPP input and
+            // does not have domainRepoId set, we create a copy of the existing dsData without
+            // domainRepoId for comparison.
             .setDsData(
                 secDnsUpdate.isPresent()
-                    ? updateDsData(domain.getDsData(), secDnsUpdate.get())
+                    ? updateDsData(
+                        domain.getDsData().stream()
+                            .map(DelegationSignerData::cloneWithoutDomainRepoId)
+                            .collect(toImmutableSet()),
+                        secDnsUpdate.get())
                     : domain.getDsData())
             .setLastEppUpdateTime(now)
             .setLastEppUpdateClientId(clientId)

core/src/main/java/google/registry/model/domain/DomainBase.java

@@ -19,6 +19,7 @@ import google.registry.model.EppResource;
 import google.registry.model.EppResource.ForeignKeyedEppResource;
 import google.registry.model.annotations.ExternalMessagingName;
 import google.registry.model.annotations.ReportedOn;
+import google.registry.model.domain.secdns.DelegationSignerData;
 import google.registry.model.host.HostResource;
 import google.registry.persistence.VKey;
 import google.registry.persistence.WithStringVKey;
@@ -113,6 +114,32 @@ public class DomainBase extends DomainContent
     restoreOfyKeys(getRepoId());
   }
 
+  /**
+   * Returns the set of {@link DelegationSignerData} 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 #getDsData()} to get the DS data.
+   *
+   * <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 DelegationSignerData} is removed from
+   * the set.
+   */
+  @Access(AccessType.PROPERTY)
+  @OneToMany(
+      cascade = {CascadeType.ALL},
+      fetch = FetchType.EAGER,
+      orphanRemoval = true)
+  @JoinColumn(
+      name = "domainRepoId",
+      referencedColumnName = "repoId",
+      insertable = false,
+      updatable = false)
+  @SuppressWarnings("UnusedMethod")
+  private Set<DelegationSignerData> getInternalDelegationSignerData() {
+    return dsData;
+  }
+
   @Override
   public VKey<DomainBase> createVKey() {
     return VKey.create(DomainBase.class, getRepoId(), Key.create(this));

core/src/main/java/google/registry/model/domain/DomainContent.java

@@ -317,6 +317,10 @@ public class DomainContent extends EppResource
     autorenewPollMessageHistoryId = getHistoryId(autorenewPollMessage);
     autorenewBillingEventHistoryId = getHistoryId(autorenewBillingEvent);
     deletePollMessageHistoryId = getHistoryId(deletePollMessage);
+    dsData =
+        nullToEmptyImmutableCopy(dsData).stream()
+            .map(dsData -> dsData.cloneWithDomainRepoId(getRepoId()))
+            .collect(toImmutableSet());
   }
 
   /**
@@ -469,6 +473,12 @@ public class DomainContent extends EppResource
     this.gracePeriods = gracePeriods;
   }
 
+  // Hibernate needs this in order to populate dsData but no one else should ever use it
+  @SuppressWarnings("UnusedMethod")
+  private void setInternalDelegationSignerData(Set<DelegationSignerData> dsData) {
+    this.dsData = dsData;
+  }
+
   public final String getCurrentSponsorClientId() {
     return getPersistedCurrentSponsorClientId();
   }
@@ -791,10 +801,16 @@ public class DomainContent extends EppResource
       instance.tld = getTldFromDomainName(instance.fullyQualifiedDomainName);
 
       T newDomain = super.build();
-      // Hibernate throws exception if gracePeriods is null because we enabled all cascadable
-      // operations and orphan removal.
+      // Hibernate throws exception if gracePeriods or dsData is null because we enabled all
+      // cascadable operations and orphan removal.
       newDomain.gracePeriods =
           newDomain.gracePeriods == null ? ImmutableSet.of() : newDomain.gracePeriods;
+      newDomain.dsData =
+          newDomain.dsData == null
+              ? ImmutableSet.of()
+              : newDomain.dsData.stream()
+                  .map(ds -> ds.cloneWithDomainRepoId(instance.getRepoId()))
+                  .collect(toImmutableSet());
       return newDomain;
     }
 

core/src/main/java/google/registry/model/domain/secdns/DelegationSignerData.java

@@ -14,11 +14,22 @@
 
 package google.registry.model.domain.secdns;
 
+import static google.registry.util.PreconditionsUtils.checkArgumentNotNull;
+
 import com.googlecode.objectify.annotation.Embed;
+import com.googlecode.objectify.annotation.Ignore;
 import google.registry.model.ImmutableObject;
+import google.registry.model.domain.secdns.DelegationSignerData.DelegationSignerDataId;
 import google.registry.schema.replay.DatastoreAndSqlEntity;
+import java.io.Serializable;
+import javax.persistence.Column;
+import javax.persistence.Entity;
+import javax.persistence.IdClass;
+import javax.persistence.Index;
+import javax.persistence.Table;
 import javax.xml.bind.DatatypeConverter;
 import javax.xml.bind.annotation.XmlElement;
+import javax.xml.bind.annotation.XmlTransient;
 import javax.xml.bind.annotation.XmlType;
 import javax.xml.bind.annotation.adapters.HexBinaryAdapter;
 import javax.xml.bind.annotation.adapters.XmlJavaTypeAdapter;
@@ -31,19 +42,26 @@ import javax.xml.bind.annotation.adapters.XmlJavaTypeAdapter;
  */
 @Embed
 @XmlType(name = "dsData")
-@javax.persistence.Entity
+@Entity
+@Table(indexes = @Index(columnList = "domainRepoId"))
+@IdClass(DelegationSignerDataId.class)
 public class DelegationSignerData extends ImmutableObject implements DatastoreAndSqlEntity {
 
   private DelegationSignerData() {}
 
+  @Ignore @XmlTransient @javax.persistence.Id String domainRepoId;
+
   /** The identifier for this particular key in the domain. */
-  @javax.persistence.Id int keyTag;
+  @javax.persistence.Id
+  @Column(nullable = false)
+  int keyTag;
 
   /**
    * The algorithm used by this key.
    *
    * @see <a href="http://tools.ietf.org/html/rfc4034#appendix-A.1">RFC 4034 Appendix A.1</a>
    */
+  @Column(nullable = false)
   @XmlElement(name = "alg")
   int algorithm;
 
@@ -52,6 +70,7 @@ public class DelegationSignerData extends ImmutableObject implements DatastoreAn
    *
    * @see <a href="http://tools.ietf.org/html/rfc4034#appendix-A.2">RFC 4034 Appendix A.2</a>
    */
+  @Column(nullable = false)
   int digestType;
 
   /**
@@ -59,6 +78,7 @@ public class DelegationSignerData extends ImmutableObject implements DatastoreAn
    *
    * @see <a href="http://tools.ietf.org/html/rfc4034#section-5.1.4">RFC 4034 Section 5.1.4</a>
    */
+  @Column(nullable = false)
   @XmlJavaTypeAdapter(HexBinaryAdapter.class)
   byte[] digest;
 
@@ -82,16 +102,34 @@ public class DelegationSignerData extends ImmutableObject implements DatastoreAn
     return digest == null ? "" : DatatypeConverter.printHexBinary(digest);
   }
 
+  public DelegationSignerData cloneWithDomainRepoId(String domainRepoId) {
+    DelegationSignerData clone = clone(this);
+    clone.domainRepoId = checkArgumentNotNull(domainRepoId);
+    return clone;
+  }
+
+  public DelegationSignerData cloneWithoutDomainRepoId() {
+    DelegationSignerData clone = clone(this);
+    clone.domainRepoId = null;
+    return clone;
+  }
+
   public static DelegationSignerData create(
-      int keyTag, int algorithm, int digestType, byte[] digest) {
+      int keyTag, int algorithm, int digestType, byte[] digest, String domainRepoId) {
     DelegationSignerData instance = new DelegationSignerData();
     instance.keyTag = keyTag;
     instance.algorithm = algorithm;
     instance.digestType = digestType;
     instance.digest = digest;
+    instance.domainRepoId = domainRepoId;
     return instance;
   }
 
+  public static DelegationSignerData create(
+      int keyTag, int algorithm, int digestType, byte[] digest) {
+    return create(keyTag, algorithm, digestType, digest, null);
+  }
+
   public static DelegationSignerData create(
       int keyTag, int algorithm, int digestType, String digestAsHex) {
     return create(keyTag, algorithm, digestType, DatatypeConverter.parseHexBinary(digestAsHex));
@@ -107,4 +145,20 @@ public class DelegationSignerData extends ImmutableObject implements DatastoreAn
         "%d %d %d %s",
         this.keyTag, this.algorithm, this.digestType, DatatypeConverter.printHexBinary(digest));
   }
+
+  static class DelegationSignerDataId extends ImmutableObject implements Serializable {
+    String domainRepoId;
+    int keyTag;
+
+    private DelegationSignerDataId() {}
+
+    private DelegationSignerDataId(String domainRepoId, int keyTag) {
+      this.domainRepoId = domainRepoId;
+      this.keyTag = keyTag;
+    }
+
+    public static DelegationSignerDataId create(String domainRepoId, int keyTag) {
+      return new DelegationSignerDataId(checkArgumentNotNull(domainRepoId), keyTag);
+    }
+  }
 }

core/src/test/java/google/registry/flows/domain/DomainCreateFlowTest.java

@@ -821,10 +821,12 @@ class DomainCreateFlowTest extends ResourceFlowTestCase<DomainCreateFlow, Domain
     setEppInput("domain_create_dsdata_no_maxsiglife.xml");
     persistContactsAndHosts("tld"); // For some reason this sample uses "tld".
     doSuccessfulTest("tld");
+    DomainBase domain = reloadResourceByForeignKey();
     assertAboutDomains()
-        .that(reloadResourceByForeignKey())
+        .that(domain)
         .hasExactlyDsData(
-            DelegationSignerData.create(12345, 3, 1, base16().decode("49FD46E6C4B45C55D4AC")));
+            DelegationSignerData.create(12345, 3, 1, base16().decode("49FD46E6C4B45C55D4AC"))
+                .cloneWithDomainRepoId(domain.getRepoId()));
   }
 
   @Test

core/src/test/java/google/registry/flows/domain/DomainUpdateFlowTest.java

@@ -14,6 +14,7 @@
 
 package google.registry.flows.domain;
 
+import static com.google.common.collect.ImmutableSet.toImmutableSet;
 import static com.google.common.collect.Sets.union;
 import static com.google.common.io.BaseEncoding.base16;
 import static com.google.common.truth.Truth.assertThat;
@@ -470,7 +471,11 @@ class DomainUpdateFlowTest extends ResourceFlowTestCase<DomainUpdateFlow, Domain
         .that(resource)
         .hasOnlyOneHistoryEntryWhich()
         .hasType(HistoryEntry.Type.DOMAIN_UPDATE);
-    assertThat(resource.getDsData()).isEqualTo(expectedDsData);
+    assertThat(resource.getDsData())
+        .isEqualTo(
+            expectedDsData.stream()
+                .map(ds -> ds.cloneWithDomainRepoId(resource.getRepoId()))
+                .collect(toImmutableSet()));
     assertDnsTasksEnqueued("example.tld");
   }
 

core/src/test/java/google/registry/model/domain/DomainBaseSqlTest.java

@@ -25,6 +25,7 @@ import static org.joda.time.DateTimeZone.UTC;
 import static org.junit.jupiter.api.Assertions.fail;
 
 import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Sets;
 import com.googlecode.objectify.Key;
 import google.registry.model.billing.BillingEvent;
 import google.registry.model.billing.BillingEvent.Flag;
@@ -342,6 +343,50 @@ public class DomainBaseSqlTest {
             });
   }
 
+  @Test
+  void testModifyDsData_addThenRemoveSuccessfully() {
+    persistDomain();
+    DelegationSignerData extraDsData =
+        DelegationSignerData.create(2, 2, 3, new byte[] {0, 1, 2}, "4-COM");
+    ImmutableSet<DelegationSignerData> unionDsData =
+        Sets.union(domain.getDsData(), ImmutableSet.of(extraDsData)).immutableCopy();
+
+    // Add an extra DelegationSignerData to dsData set.
+    jpaTm()
+        .transact(
+            () -> {
+              DomainBase persisted = jpaTm().load(domain.createVKey());
+              assertThat(persisted.getDsData()).containsExactlyElementsIn(domain.getDsData());
+              DomainBase modified = persisted.asBuilder().setDsData(unionDsData).build();
+              jpaTm().put(modified);
+            });
+
+    // Verify that the persisted domain entity contains both DelegationSignerData records.
+    jpaTm()
+        .transact(
+            () -> {
+              DomainBase persisted = jpaTm().load(domain.createVKey());
+              assertThat(persisted.getDsData()).containsExactlyElementsIn(unionDsData);
+              assertEqualDomainExcept(persisted, "dsData");
+            });
+
+    // Remove the extra DelegationSignerData record from dsData set.
+    jpaTm()
+        .transact(
+            () -> {
+              DomainBase persisted = jpaTm().load(domain.createVKey());
+              jpaTm().put(persisted.asBuilder().setDsData(domain.getDsData()).build());
+            });
+
+    // Verify that the persisted domain is equal to the original domain.
+    jpaTm()
+        .transact(
+            () -> {
+              DomainBase persisted = jpaTm().load(domain.createVKey());
+              assertEqualDomainExcept(persisted);
+            });
+  }
+
   @Test
   void testUpdates() {
     jpaTm()
@@ -358,16 +403,6 @@ public class DomainBaseSqlTest {
         .transact(
             () -> {
               DomainBase result = jpaTm().load(domain.createVKey());
-
-              // Fix DS data, since we can't persist that yet.
-              result =
-                  result
-                      .asBuilder()
-                      .setDsData(
-                          ImmutableSet.of(
-                              DelegationSignerData.create(1, 2, 3, new byte[] {0, 1, 2})))
-                      .build();
-
               assertAboutImmutableObjects()
                   .that(result)
                   .isEqualExceptFields(domain, "updateTimestamp", "creationTime");
@@ -572,13 +607,6 @@ public class DomainBaseSqlTest {
   }
 
   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();