Use composite primary key for RegistrarContact (#761)

* Use composite primary key for RegistrarPoc

* Increase the serial number for flyway file and resolve comments

* Rebase on HEAD

* Rebase on HEAD

core/src/main/java/google/registry/model/registrar/Registrar.java

@@ -73,6 +73,7 @@ import google.registry.model.annotations.ReportedOn;
 import google.registry.model.common.EntityGroupRoot;
 import google.registry.model.registrar.Registrar.BillingAccountEntry.CurrencyMapper;
 import google.registry.model.registry.Registry;
+import google.registry.persistence.VKey;
 import google.registry.schema.replay.DatastoreAndSqlEntity;
 import google.registry.util.CidrAddressBlock;
 import java.security.cert.CertificateParsingException;
@@ -703,6 +704,10 @@ public class Registrar extends ImmutableObject
     return new Builder(clone(this));
   }
 
+  public VKey<Registrar> createVKey() {
+    return VKey.create(Registrar.class, clientIdentifier, Key.create(this));
+  }
+
   /** A builder for constructing {@link Registrar}, since it is immutable. */
   public static class Builder extends Buildable.Builder<Registrar> {
     public Builder() {}

core/src/main/java/google/registry/model/registrar/RegistrarContact.java

@@ -20,6 +20,7 @@ import static com.google.common.base.Strings.isNullOrEmpty;
 import static com.google.common.collect.ImmutableSet.toImmutableSet;
 import static com.google.common.collect.Sets.difference;
 import static com.google.common.io.BaseEncoding.base64;
+import static google.registry.model.common.EntityGroupRoot.getCrossTldKey;
 import static google.registry.model.ofy.ObjectifyService.ofy;
 import static google.registry.model.registrar.Registrar.checkValidEmail;
 import static google.registry.persistence.transaction.TransactionManagerFactory.tm;
@@ -35,20 +36,26 @@ import com.google.common.collect.Streams;
 import com.googlecode.objectify.Key;
 import com.googlecode.objectify.annotation.Entity;
 import com.googlecode.objectify.annotation.Id;
+import com.googlecode.objectify.annotation.Ignore;
 import com.googlecode.objectify.annotation.Index;
+import com.googlecode.objectify.annotation.OnLoad;
 import com.googlecode.objectify.annotation.Parent;
 import google.registry.model.Buildable;
 import google.registry.model.ImmutableObject;
 import google.registry.model.JsonMapBuilder;
 import google.registry.model.Jsonifiable;
 import google.registry.model.annotations.ReportedOn;
+import google.registry.model.registrar.RegistrarContact.RegistrarPocId;
+import google.registry.persistence.VKey;
 import google.registry.schema.replay.DatastoreAndSqlEntity;
+import java.io.Serializable;
 import java.util.Arrays;
 import java.util.Map;
 import java.util.Optional;
 import java.util.Set;
 import javax.annotation.Nullable;
-import javax.persistence.Column;
+import javax.persistence.IdClass;
+import javax.persistence.PostLoad;
 import javax.persistence.Table;
 import javax.persistence.Transient;
 
@@ -59,16 +66,17 @@ import javax.persistence.Transient;
  * <p>IMPORTANT NOTE: Any time that you change, update, or delete RegistrarContact entities, you
  * *MUST* also modify the persisted Registrar entity with {@link Registrar#contactsRequireSyncing}
  * set to true.
+ *
+ * <p>TODO(b/163366543): Rename the class name to RegistrarPoc after database migration
  */
 @ReportedOn
 @Entity
-@javax.persistence.Entity
+@javax.persistence.Entity(name = "RegistrarPoc")
 @Table(
-    name = "RegistrarPoc",
     indexes = {
       @javax.persistence.Index(columnList = "gaeUserId", name = "registrarpoc_gae_user_id_idx")
     })
-// TODO(shicong): Rename the class name to RegistrarPoc after database migration
+@IdClass(RegistrarPocId.class)
 public class RegistrarContact extends ImmutableObject
     implements DatastoreAndSqlEntity, Jsonifiable {
 
@@ -113,9 +121,10 @@ public class RegistrarContact extends ImmutableObject
   /** The email address of the contact. */
   @Id
   @javax.persistence.Id
-  @Column(nullable = false)
   String emailAddress;
 
+  @Ignore @javax.persistence.Id String registrarId;
+
   /** External email address of this contact used for registry lock confirmations. */
   String registryLockEmailAddress;
 
@@ -342,6 +351,39 @@ public class RegistrarContact extends ImmutableObject
         .build();
   }
 
+  /** Sets Cloud SQL specific fields when the entity is loaded from Datastore. */
+  @OnLoad
+  void onLoad() {
+    registrarId = parent.getName();
+  }
+
+  /** Sets Datastore specific fields when the entity is loaded from Cloud SQL. */
+  @PostLoad
+  void postLoad() {
+    parent = Key.create(getCrossTldKey(), Registrar.class, registrarId);
+  }
+
+  public VKey<RegistrarContact> createVKey() {
+    return VKey.create(
+        RegistrarContact.class, new RegistrarPocId(emailAddress, registrarId), Key.create(this));
+  }
+
+  /** Class to represent the composite primary key for {@link RegistrarContact} entity. */
+  static class RegistrarPocId extends ImmutableObject implements Serializable {
+
+    String emailAddress;
+
+    String registrarId;
+
+    // Hibernate requires this default constructor.
+    private RegistrarPocId() {}
+
+    RegistrarPocId(String emailAddress, String registrarId) {
+      this.emailAddress = emailAddress;
+      this.registrarId = registrarId;
+    }
+  }
+
   /** A builder for constructing a {@link RegistrarContact}, since it is immutable. */
   public static class Builder extends Buildable.Builder<RegistrarContact> {
     public Builder() {}
@@ -372,6 +414,7 @@ public class RegistrarContact extends ImmutableObject
             !isNullOrEmpty(getInstance().registryLockEmailAddress),
             "Registry lock email must not be null if allowing registry lock access");
       }
+      getInstance().registrarId = getInstance().parent.getName();
       return cloneEmptyToNull(super.build());
     }
 

core/src/test/java/google/registry/schema/registrar/RegistrarContactTest.java

@@ -0,0 +1,73 @@
+// Copyright 2020 The Nomulus Authors. All Rights Reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package google.registry.schema.registrar;
+
+import static com.google.common.truth.Truth.assertThat;
+import static google.registry.model.registrar.RegistrarContact.Type.WHOIS;
+import static google.registry.persistence.transaction.TransactionManagerFactory.jpaTm;
+import static google.registry.testing.SqlHelper.saveRegistrar;
+
+import com.google.common.collect.ImmutableSet;
+import google.registry.model.registrar.Registrar;
+import google.registry.model.registrar.RegistrarContact;
+import google.registry.persistence.transaction.JpaTestRules;
+import google.registry.persistence.transaction.JpaTestRules.JpaIntegrationWithCoverageExtension;
+import google.registry.testing.DatastoreEntityExtension;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Order;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.RegisterExtension;
+
+/** Unit tests for persisting {@link RegistrarContact} entities. */
+class RegistrarContactTest {
+
+  @RegisterExtension
+  @Order(value = 1)
+  DatastoreEntityExtension datastoreEntityExtension = new DatastoreEntityExtension();
+
+  @RegisterExtension
+  JpaIntegrationWithCoverageExtension jpa =
+      new JpaTestRules.Builder().buildIntegrationWithCoverageExtension();
+
+  private Registrar testRegistrar;
+
+  private RegistrarContact testRegistrarPoc;
+
+  @BeforeEach
+  public void beforeEach() {
+    testRegistrar = saveRegistrar("registrarId");
+    testRegistrarPoc =
+        new RegistrarContact.Builder()
+            .setParent(testRegistrar)
+            .setName("Judith Registrar")
+            .setEmailAddress("judith.doe@example.com")
+            .setRegistryLockEmailAddress("judith.doe@external.com")
+            .setPhoneNumber("+1.2125650000")
+            .setFaxNumber("+1.2125650001")
+            .setTypes(ImmutableSet.of(WHOIS))
+            .setVisibleInWhoisAsAdmin(true)
+            .setVisibleInWhoisAsTech(false)
+            .setVisibleInDomainWhoisAsAbuse(false)
+            .build();
+  }
+
+  @Test
+  void testPersistence_succeeds() {
+    jpaTm().transact(() -> jpaTm().saveNew(testRegistrarPoc));
+    RegistrarContact persisted =
+        jpaTm().transact(() -> jpaTm().load(testRegistrarPoc.createVKey()));
+    assertThat(persisted).isEqualTo(testRegistrarPoc);
+  }
+}

core/src/test/java/google/registry/testing/SqlHelper.java

@@ -21,6 +21,7 @@ import static org.junit.jupiter.api.Assertions.assertThrows;
 
 import com.google.common.base.Throwables;
 import com.google.common.collect.ImmutableList;
+import google.registry.model.registrar.Registrar;
 import google.registry.model.registry.RegistryLockDao;
 import google.registry.schema.domain.RegistryLock;
 import java.sql.SQLException;
@@ -59,10 +60,10 @@ public class SqlHelper {
     return jpaTm().transact(() -> RegistryLockDao.getByRevisionId(revisionId));
   }
 
-  public static void saveRegistrar(String clientId) {
+  public static Registrar saveRegistrar(String clientId) {
-    jpaTm()
+    Registrar registrar = makeRegistrar1().asBuilder().setClientId(clientId).build();
-        .transact(
+    jpaTm().transact(() -> jpaTm().saveNew(registrar));
-            () -> jpaTm().saveNew(makeRegistrar1().asBuilder().setClientId(clientId).build()));
+    return jpaTm().transact(() -> jpaTm().load(registrar.createVKey()));
   }
 
   public static void assertThrowForeignKeyViolation(Executable executable) {

core/src/test/resources/google/registry/ui/server/registrar/update_registrar_email.txt

@@ -11,9 +11,9 @@ emailAddress: the.registrar@example.com -> thase@the.registrar
 url: http://my.fake.url -> http://my.new.url
 contacts:
     ADDED:
-        {parent=Key<?>(EntityGroupRoot("cross-tld")/Registrar("TheRegistrar")), name=Extra Terrestrial, emailAddress=etphonehome@example.com, registryLockEmailAddress=null, phoneNumber=+1.2345678901, faxNumber=null, types=[ADMIN, BILLING, TECH, WHOIS], gaeUserId=null, visibleInWhoisAsAdmin=true, visibleInWhoisAsTech=false, visibleInDomainWhoisAsAbuse=false, allowedToSetRegistryLockPassword=false}
+        {parent=Key<?>(EntityGroupRoot("cross-tld")/Registrar("TheRegistrar")), name=Extra Terrestrial, emailAddress=etphonehome@example.com, registrarId=TheRegistrar, registryLockEmailAddress=null, phoneNumber=+1.2345678901, faxNumber=null, types=[ADMIN, BILLING, TECH, WHOIS], gaeUserId=null, visibleInWhoisAsAdmin=true, visibleInWhoisAsTech=false, visibleInDomainWhoisAsAbuse=false, allowedToSetRegistryLockPassword=false}
     REMOVED:
-        {parent=Key<?>(EntityGroupRoot("cross-tld")/Registrar("TheRegistrar")), name=John Doe, emailAddress=johndoe@theregistrar.com, registryLockEmailAddress=null, phoneNumber=+1.1234567890, faxNumber=null, types=[ADMIN], gaeUserId=31337, visibleInWhoisAsAdmin=false, visibleInWhoisAsTech=false, visibleInDomainWhoisAsAbuse=false, allowedToSetRegistryLockPassword=false}
+        {parent=Key<?>(EntityGroupRoot("cross-tld")/Registrar("TheRegistrar")), name=John Doe, emailAddress=johndoe@theregistrar.com, registrarId=TheRegistrar, registryLockEmailAddress=null, phoneNumber=+1.1234567890, faxNumber=null, types=[ADMIN], gaeUserId=31337, visibleInWhoisAsAdmin=false, visibleInWhoisAsTech=false, visibleInDomainWhoisAsAbuse=false, allowedToSetRegistryLockPassword=false}
     FINAL CONTENTS:
-        {parent=Key<?>(EntityGroupRoot("cross-tld")/Registrar("TheRegistrar")), name=Extra Terrestrial, emailAddress=etphonehome@example.com, registryLockEmailAddress=null, phoneNumber=+1.2345678901, faxNumber=null, types=[ADMIN, BILLING, TECH, WHOIS], gaeUserId=null, visibleInWhoisAsAdmin=true, visibleInWhoisAsTech=false, visibleInDomainWhoisAsAbuse=false, allowedToSetRegistryLockPassword=false},
+        {parent=Key<?>(EntityGroupRoot("cross-tld")/Registrar("TheRegistrar")), name=Extra Terrestrial, emailAddress=etphonehome@example.com, registrarId=TheRegistrar, registryLockEmailAddress=null, phoneNumber=+1.2345678901, faxNumber=null, types=[ADMIN, BILLING, TECH, WHOIS], gaeUserId=null, visibleInWhoisAsAdmin=true, visibleInWhoisAsTech=false, visibleInDomainWhoisAsAbuse=false, allowedToSetRegistryLockPassword=false},
-        {parent=Key<?>(EntityGroupRoot("cross-tld")/Registrar("TheRegistrar")), name=Marla Singer, emailAddress=Marla.Singer@crr.com, registryLockEmailAddress=Marla.Singer.RegistryLock@crr.com, phoneNumber=+1.2128675309, faxNumber=null, types=[TECH], gaeUserId=12345, visibleInWhoisAsAdmin=false, visibleInWhoisAsTech=false, visibleInDomainWhoisAsAbuse=false, allowedToSetRegistryLockPassword=false}
+        {parent=Key<?>(EntityGroupRoot("cross-tld")/Registrar("TheRegistrar")), name=Marla Singer, emailAddress=Marla.Singer@crr.com, registrarId=TheRegistrar, registryLockEmailAddress=Marla.Singer.RegistryLock@crr.com, phoneNumber=+1.2128675309, faxNumber=null, types=[TECH], gaeUserId=12345, visibleInWhoisAsAdmin=false, visibleInWhoisAsTech=false, visibleInDomainWhoisAsAbuse=false, allowedToSetRegistryLockPassword=false}

db/src/main/resources/sql/flyway/V50__use_composite_key_for_registrar_poc.sql

@@ -0,0 +1,20 @@
+-- Copyright 2020 The Nomulus Authors. All Rights Reserved.
+--
+-- Licensed under the Apache License, Version 2.0 (the "License");
+-- you may not use this file except in compliance with the License.
+-- You may obtain a copy of the License at
+--
+--     http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+
+alter table "RegistrarPoc" add column registrar_id text;
+
+alter table "RegistrarPoc" drop constraint "RegistrarPoc_pkey";
+
+alter table "RegistrarPoc"
+    add constraint "RegistrarPoc_pkey" primary key (registrar_id, email_address);

db/src/main/resources/sql/schema/db-schema.sql.generated

@@ -518,6 +518,7 @@ create sequence history_id_sequence start 1 increment 1;
 
     create table "RegistrarPoc" (
        email_address text not null,
+        registrar_id text not null,
         allowed_to_set_registry_lock_password boolean not null,
         fax_number text,
         gae_user_id text,
@@ -530,7 +531,7 @@ create sequence history_id_sequence start 1 increment 1;
         visible_in_domain_whois_as_abuse boolean not null,
         visible_in_whois_as_admin boolean not null,
         visible_in_whois_as_tech boolean not null,
-        primary key (email_address)
+        primary key (email_address, registrar_id)
     );
 
     create table "RegistryLock" (

db/src/main/resources/sql/schema/nomulus.golden.sql

@@ -777,7 +777,8 @@ CREATE TABLE public."RegistrarPoc" (
     visible_in_domain_whois_as_abuse boolean NOT NULL,
     visible_in_whois_as_admin boolean NOT NULL,
     visible_in_whois_as_tech boolean NOT NULL,
-    registry_lock_email_address text
+    registry_lock_email_address text,
+    registrar_id text NOT NULL
 );
 
 
@@ -1154,7 +1155,7 @@ ALTER TABLE ONLY public."PremiumList"
 --
 
 ALTER TABLE ONLY public."RegistrarPoc"
-    ADD CONSTRAINT "RegistrarPoc_pkey" PRIMARY KEY (email_address);
+    ADD CONSTRAINT "RegistrarPoc_pkey" PRIMARY KEY (registrar_id, email_address);
 
 
 --