Add annotation processor to generate converter for VKey (#566 )

Remove email-editing footgun (#503 )
* Remove email-editing footgun Email address is used as the primary key so we should be very careful about changing it. This will have even more importance when this is the location to which we will be sending registry lock confirmation emails. Note: we allow addition or removal of contacts through the UI (and don't want to disable that) and because all edits are performed by saving the entire list of contacts, we can't explicitly prevent all possible edits of email address in the backend. So this doesn't technically prevent anything security-wise, but it makes it much more difficult to accidentally edit an email when you shouldn't. * Enforce non-deletion of registry-lock-enabled contacts * Fix tests * Specify contact
2026-08-08 16:26:07 +00:00 · 2020-04-29 17:29:05 -04:00 · 2020-04-29 11:44:51 -04:00 · 2020-04-28 15:32:42 -04:00 · 2020-04-27 17:30:51 -04:00
68 changed files with 950 additions and 234 deletions
@@ -99,6 +99,15 @@ PRESUBMITS = {
        "System.(out|err).println is only allowed in tools/ packages. Please "
        "use a logger instead.",

+    # ObjectifyService.register is restricted to main/ or AppEngineRule.
+    PresubmitCheck(
+        r".*\bObjectifyService\.register", "java", {
+            "/build/", "/generated/", "node_modules/", "src/main/",
+            "AppEngineRule.java"
+        }):
+      "ObjectifyService.register is not allowed in tests. Please use "
+      "AppengineRule.register instead.",
+
    # PostgreSQLContainer instantiation must specify docker tag
    PresubmitCheck(
        r"[\s\S]*new\s+PostgreSQLContainer(<[\s\S]*>)?\(\s*\)[\s\S]*",
@@ -295,6 +295,8 @@ dependencies {
  testAnnotationProcessor deps['com.google.auto.value:auto-value']
  annotationProcessor deps['com.google.dagger:dagger-compiler']
  testAnnotationProcessor deps['com.google.dagger:dagger-compiler']
+  annotationProcessor project(':processor')
+  testAnnotationProcessor project(':processor')

  testCompile deps['com.google.appengine:appengine-testing']
  testCompile deps['com.google.guava:guava-testlib']
@@ -12,24 +12,24 @@ com.google.dagger:dagger-producers:2.21
 com.google.dagger:dagger-spi:2.21
 com.google.dagger:dagger:2.21
 com.google.errorprone:error_prone_annotation:2.3.3
-com.google.errorprone:error_prone_annotations:2.3.3
+com.google.errorprone:error_prone_annotations:2.3.4
 com.google.errorprone:error_prone_check_api:2.3.3
 com.google.errorprone:error_prone_core:2.3.3
 com.google.errorprone:error_prone_type_annotations:2.3.3
 com.google.errorprone:javac-shaded:9-dev-r4023-3
 com.google.googlejavaformat:google-java-format:1.5
 com.google.guava:failureaccess:1.0.1
-com.google.guava:guava:27.0.1-jre
+com.google.guava:guava:28.2-jre
 com.google.guava:listenablefuture:9999.0-empty-to-avoid-conflict-with-guava
-com.google.j2objc:j2objc-annotations:1.1
+com.google.j2objc:j2objc-annotations:1.3
 com.google.protobuf:protobuf-java:3.4.0
 com.googlecode.java-diff-utils:diffutils:1.3.0
-com.squareup:javapoet:1.11.1
+com.squareup:javapoet:1.12.1
 javax.annotation:jsr250-api:1.0
 javax.inject:javax.inject:1
+javax.persistence:javax.persistence-api:2.2
 org.checkerframework:checker-compat-qual:2.5.3
-org.checkerframework:checker-qual:2.5.3
+org.checkerframework:checker-qual:2.10.0
 org.checkerframework:dataflow:2.5.3
 org.checkerframework:javacutil:2.5.3
-org.codehaus.mojo:animal-sniffer-annotations:1.17
 org.pcollections:pcollections:2.1.2
@@ -12,24 +12,24 @@ com.google.dagger:dagger-producers:2.21
 com.google.dagger:dagger-spi:2.21
 com.google.dagger:dagger:2.21
 com.google.errorprone:error_prone_annotation:2.3.3
-com.google.errorprone:error_prone_annotations:2.3.3
+com.google.errorprone:error_prone_annotations:2.3.4
 com.google.errorprone:error_prone_check_api:2.3.3
 com.google.errorprone:error_prone_core:2.3.3
 com.google.errorprone:error_prone_type_annotations:2.3.3
 com.google.errorprone:javac-shaded:9-dev-r4023-3
 com.google.googlejavaformat:google-java-format:1.5
 com.google.guava:failureaccess:1.0.1
-com.google.guava:guava:27.0.1-jre
+com.google.guava:guava:28.2-jre
 com.google.guava:listenablefuture:9999.0-empty-to-avoid-conflict-with-guava
-com.google.j2objc:j2objc-annotations:1.1
+com.google.j2objc:j2objc-annotations:1.3
 com.google.protobuf:protobuf-java:3.4.0
 com.googlecode.java-diff-utils:diffutils:1.3.0
-com.squareup:javapoet:1.11.1
+com.squareup:javapoet:1.12.1
 javax.annotation:jsr250-api:1.0
 javax.inject:javax.inject:1
+javax.persistence:javax.persistence-api:2.2
 org.checkerframework:checker-compat-qual:2.5.3
-org.checkerframework:checker-qual:2.5.3
+org.checkerframework:checker-qual:2.10.0
 org.checkerframework:dataflow:2.5.3
 org.checkerframework:javacutil:2.5.3
-org.codehaus.mojo:animal-sniffer-annotations:1.17
 org.pcollections:pcollections:2.1.2
@@ -135,7 +135,7 @@ public abstract class EppResource extends BackupGroupRoot implements Buildable {
  @Transient
  ImmutableSortedMap<DateTime, Key<CommitLogManifest>> revisions = ImmutableSortedMap.of();

-  public final String getRepoId() {
+  public String getRepoId() {
    return repoId;
  }

@@ -77,7 +77,6 @@ import javax.annotation.Nullable;
 import javax.persistence.AttributeOverride;
 import javax.persistence.AttributeOverrides;
 import javax.persistence.Column;
-import javax.persistence.Convert;
 import javax.persistence.ElementCollection;
 import javax.persistence.Embedded;
 import javax.persistence.JoinTable;
@@ -147,7 +146,6 @@ public class DomainBase extends EppResource
  @Ignore
  @ElementCollection
  @JoinTable(name = "DomainHost")
-  @Convert(converter = HostResource.VKeyHostResourceConverter.class)
  Set<VKey<HostResource>> nsHostVKeys;

  /**
@@ -34,7 +34,7 @@ import google.registry.model.annotations.ReportedOn;
 import google.registry.model.domain.DomainBase;
 import google.registry.model.transfer.TransferData;
 import google.registry.persistence.VKey;
-import google.registry.persistence.converter.VKeyConverter;
+import google.registry.persistence.WithStringVKey;
 import java.net.InetAddress;
 import java.util.Optional;
 import java.util.Set;
@@ -54,6 +54,7 @@ import org.joda.time.DateTime;
@Entity
@javax.persistence.Entity
@ExternalMessagingName("host")
+@WithStringVKey
 public class HostResource extends EppResource implements ForeignKeyedEppResource {

  /**
@@ -212,11 +213,4 @@ public class HostResource extends EppResource implements ForeignKeyedEppResource
      return this;
    }
  }
-
-  public static class VKeyHostResourceConverter extends VKeyConverter<HostResource> {
-    @Override
-    protected Class<HostResource> getAttributeClass() {
-      return HostResource.class;
-    }
-  }
 }
@@ -151,11 +151,14 @@ public class ObjectifyService {
      String kind = Key.getKind(clazz);
      boolean registered = factory().getMetadata(kind) != null;
      if (clazz.isAnnotationPresent(Entity.class)) {
-        // Objectify silently ignores re-registrations for a given kind string, even if the classes
-        // being registered are distinct. Throw an exception if that would happen here.
-        checkState(!registered,
+        // Objectify silently replaces current registration for a given kind string when a different
+        // class is registered again for this kind. For simplicity's sake, throw an exception on any
+        // re-registration.
+        checkState(
+            !registered,
            "Kind '%s' already registered, cannot register new @Entity %s",
-            kind, clazz.getCanonicalName());
+            kind,
+            clazz.getCanonicalName());
      } else if (clazz.isAnnotationPresent(EntitySubclass.class)) {
        // Ensure that any @EntitySubclass classes have also had their parent @Entity registered,
        // which Objectify nominally requires but doesn't enforce in 4.x (though it may in 5.x).
@@ -16,7 +16,6 @@ package google.registry.model.server;

 import static com.google.common.base.Preconditions.checkArgument;
 import static google.registry.model.ofy.ObjectifyService.ofy;
-import static google.registry.persistence.transaction.TransactionManagerFactory.jpaTm;
 import static google.registry.persistence.transaction.TransactionManagerFactory.tm;
 import static google.registry.util.DateTimeUtils.isAtOrAfter;

@@ -29,7 +28,6 @@ import com.googlecode.objectify.annotation.Id;
 import google.registry.model.ImmutableObject;
 import google.registry.model.annotations.NotBackedUp;
 import google.registry.model.annotations.NotBackedUp.Reason;
-import google.registry.schema.server.LockDao;
 import google.registry.util.RequestStatusChecker;
 import google.registry.util.RequestStatusCheckerImpl;
 import java.io.Serializable;
@@ -197,22 +195,6 @@ public class Lock extends ImmutableObject implements Serializable {

                  // Checking if an unexpired lock still exists - if so, the lock can't be acquired.
                  Lock lock = ofy().load().type(Lock.class).id(lockId).now();
-                  try {
-                    jpaTm()
-                        .transact(
-                            () -> {
-                              Optional<google.registry.schema.server.Lock> cloudSqlLockOptional;
-                              if (tld == null) {
-                                cloudSqlLockOptional = LockDao.load(resourceName);
-                              } else {
-                                cloudSqlLockOptional = LockDao.load(resourceName, tld);
-                              }
-                              LockDao.compare(Optional.ofNullable(lock), cloudSqlLockOptional);
-                            });
-                  } catch (Exception e) {
-                    logger.atSevere().withCause(e).log(
-                        "Issue loading and comparing lock from Cloud SQL");
-                  }
                  if (lock != null) {
                    logger.atInfo().log(
                        "Loaded existing lock: %s for request: %s", lock.lockId, lock.requestLogId);
@@ -237,35 +219,6 @@ public class Lock extends ImmutableObject implements Serializable {
                  // don't need to be backed up.
                  ofy().saveWithoutBackup().entity(newLock);

-                  // create and save the lock to Cloud SQL
-                  try {
-                    jpaTm()
-                        .transact(
-                            () -> {
-                              google.registry.schema.server.Lock cloudSqlLock;
-                              if (tld == null) {
-                                cloudSqlLock =
-                                    google.registry.schema.server.Lock.createGlobal(
-                                        resourceName,
-                                        requestStatusChecker.getLogId(),
-                                        now,
-                                        leaseLength);
-                              } else {
-                                cloudSqlLock =
-                                    google.registry.schema.server.Lock.create(
-                                        resourceName,
-                                        tld,
-                                        requestStatusChecker.getLogId(),
-                                        now,
-                                        leaseLength);
-                              }
-                              LockDao.save(cloudSqlLock);
-                            });
-                  } catch (Exception e) {
-                    logger.atSevere().withCause(e).log(
-                        "Error saving lock to Cloud SQL: %s", newLock);
-                  }
-
                  return AcquireResult.create(now, lock, newLock, lockState);
                });

@@ -284,44 +237,12 @@ public class Lock extends ImmutableObject implements Serializable {
              // this can happen if release() is called around the expiration time and the lock
              // expires underneath us.
              Lock loadedLock = ofy().load().type(Lock.class).id(lockId).now();
-              try {
-                jpaTm()
-                    .transact(
-                        () -> {
-                          Optional<google.registry.schema.server.Lock> cloudSqlLockOptional;
-                          if (tld == null) {
-                            cloudSqlLockOptional = LockDao.load(resourceName);
-                          } else {
-                            cloudSqlLockOptional = LockDao.load(resourceName, tld);
-                          }
-                          LockDao.compare(Optional.ofNullable(loadedLock), cloudSqlLockOptional);
-                        });
-              } catch (Exception e) {
-                logger.atSevere().withCause(e).log(
-                    "Issue loading and comparing lock from Cloud SQL");
-              }
              if (Lock.this.equals(loadedLock)) {
                // Use noBackupOfy() so that we don't create a commit log entry for deleting the
                // lock.
                logger.atInfo().log("Deleting lock: %s", lockId);
                ofy().deleteWithoutBackup().entity(Lock.this);

-                // Remove the lock from Cloud SQL
-                try {
-                  jpaTm()
-                      .transact(
-                          () -> {
-                            if (tld == null) {
-                              LockDao.delete(resourceName);
-                            } else {
-                              LockDao.delete(resourceName, tld);
-                            }
-                          });
-                } catch (Exception e) {
-                  logger.atSevere().withCause(e).log(
-                      "Error deleting lock from Cloud SQL: %s", loadedLock);
-                }
-
                lockMetrics.recordRelease(
                    resourceName, tld, new Duration(acquiredTime, tm().getTransactionTime()));
              } else {
@@ -15,6 +15,7 @@
 package google.registry.persistence;

 import static com.google.common.base.Preconditions.checkState;
+import static google.registry.util.PreconditionsUtils.checkArgumentNotNull;

 import google.registry.model.ImmutableObject;
 import java.util.Optional;
@@ -41,20 +42,31 @@ public class VKey<T> extends ImmutableObject {
    this.primaryKey = primaryKey;
  }

-  public static <T> VKey<T> createSql(Class<? extends T> kind, Object primaryKey) {
-    return new VKey(kind, null, primaryKey);
+  /** Creates a {@link VKey} which only contains the sql primary key. */
+  public static <T> VKey<T> createSql(Class<? extends T> kind, Object sqlKey) {
+    checkArgumentNotNull(kind, "kind must not be null");
+    checkArgumentNotNull(sqlKey, "sqlKey must not be null");
+    return new VKey(kind, null, sqlKey);
  }

+  /** Creates a {@link VKey} which only contains the ofy primary key. */
  public static <T> VKey<T> createOfy(
-      Class<? extends T> kind, com.googlecode.objectify.Key<T> ofyKey) {
+      Class<? extends T> kind, com.googlecode.objectify.Key<? extends T> ofyKey) {
+    checkArgumentNotNull(kind, "kind must not be null");
+    checkArgumentNotNull(ofyKey, "ofyKey must not be null");
    return new VKey(kind, ofyKey, null);
  }

+  /** Creates a {@link VKey} which only contains both sql and ofy primary key. */
  public static <T> VKey<T> create(
-      Class<? extends T> kind, Object primaryKey, com.googlecode.objectify.Key ofyKey) {
-    return new VKey(kind, ofyKey, primaryKey);
+      Class<? extends T> kind, Object sqlKey, com.googlecode.objectify.Key ofyKey) {
+    checkArgumentNotNull(kind, "kind must not be null");
+    checkArgumentNotNull(sqlKey, "sqlKey must not be null");
+    checkArgumentNotNull(ofyKey, "ofyKey must not be null");
+    return new VKey(kind, ofyKey, sqlKey);
  }

+  /** Returns the type of the entity. */
  public Class<? extends T> getKind() {
    return this.kind;
  }
@@ -0,0 +1,34 @@
+// 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.persistence;
+
+import java.lang.annotation.ElementType;
+import java.lang.annotation.Target;
+import javax.persistence.AttributeConverter;
+import javax.persistence.Entity;
+
+/**
+ * Annotation for {@link Entity} which id is long type and needs an {@link AttributeConverter} for
+ * its VKey.
+ */
+@Target({ElementType.TYPE})
+public @interface WithLongVKey {
+  /**
+   * Sets the suffix of the class name for the {@link AttributeConverter} generated by
+   * LongVKeyProcessor. If not set, the suffix will be the type name of the VKey. Note that the
+   * class name will be "VKeyConverter_" concatenated with the suffix.
+   */
+  String classNameSuffix() default "";
+}
@@ -0,0 +1,34 @@
+// 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.persistence;
+
+import java.lang.annotation.ElementType;
+import java.lang.annotation.Target;
+import javax.persistence.AttributeConverter;
+import javax.persistence.Entity;
+
+/**
+ * Annotation for {@link Entity} which id is string type and needs an {@link AttributeConverter} for
+ * its VKey.
+ */
+@Target({ElementType.TYPE})
+public @interface WithStringVKey {
+  /**
+   * Sets the suffix of the class name for the {@link AttributeConverter} generated by
+   * StringVKeyProcessor. If not set, the suffix will be the type name of the VKey. Note that the
+   * class name will be "VKeyConverter_" concatenated with the suffix.
+   */
+  String classNameSuffix() default "";
+}
@@ -19,16 +19,16 @@ import javax.annotation.Nullable;
 import javax.persistence.AttributeConverter;

 /** Converts VKey to a string column. */
-public abstract class VKeyConverter<T> implements AttributeConverter<VKey<T>, String> {
+public abstract class VKeyConverter<T, C> implements AttributeConverter<VKey<? extends T>, C> {
  @Override
  @Nullable
-  public String convertToDatabaseColumn(@Nullable VKey<T> attribute) {
-    return attribute == null ? null : (String) attribute.getSqlKey();
+  public C convertToDatabaseColumn(@Nullable VKey<? extends T> attribute) {
+    return attribute == null ? null : (C) attribute.getSqlKey();
  }

  @Override
  @Nullable
-  public VKey<T> convertToEntityAttribute(@Nullable String dbData) {
+  public VKey<? extends T> convertToEntityAttribute(@Nullable C dbData) {
    return dbData == null ? null : VKey.createSql(getAttributeClass(), dbData);
  }

@@ -391,7 +391,8 @@ public class RegistrarSettingsAction implements Runnable, JsonActionRunner.JsonA
   * @throws FormException if the checks fail.
   */
  void checkContactRequirements(
-      Set<RegistrarContact> existingContacts, Set<RegistrarContact> updatedContacts) {
+      ImmutableSet<RegistrarContact> existingContacts,
+      ImmutableSet<RegistrarContact> updatedContacts) {
    // Check that no two contacts use the same email address.
    Set<String> emails = new HashSet<>();
    for (RegistrarContact contact : updatedContacts) {
@@ -435,7 +436,12 @@ public class RegistrarSettingsAction implements Runnable, JsonActionRunner.JsonA
      throw new ContactRequirementException(
          "An abuse contact visible in domain WHOIS query must be designated");
    }
+    checkContactRegistryLockRequirements(existingContacts, updatedContacts);
+  }

+  private static void checkContactRegistryLockRequirements(
+      ImmutableSet<RegistrarContact> existingContacts,
+      ImmutableSet<RegistrarContact> updatedContacts) {
    // Any contact(s) with new passwords must be allowed to set them
    for (RegistrarContact updatedContact : updatedContacts) {
      if (updatedContact.isRegistryLockAllowed()
@@ -463,6 +469,31 @@ public class RegistrarSettingsAction implements Runnable, JsonActionRunner.JsonA
        }
      }
    }
+
+    // Any previously-existing contacts with registry lock enabled cannot be deleted
+    existingContacts.stream()
+        .filter(RegistrarContact::isRegistryLockAllowed)
+        .forEach(
+            contact -> {
+              Optional<RegistrarContact> updatedContactOptional =
+                  updatedContacts.stream()
+                      .filter(
+                          updatedContact ->
+                              updatedContact.getEmailAddress().equals(contact.getEmailAddress()))
+                      .findFirst();
+              if (!updatedContactOptional.isPresent()) {
+                throw new FormException(
+                    String.format(
+                        "Cannot delete the contact %s that has registry lock enabled",
+                        contact.getEmailAddress()));
+              }
+              if (!updatedContactOptional.get().isRegistryLockAllowed()) {
+                throw new FormException(
+                    String.format(
+                        "Cannot remove the ability to use registry lock on the contact %s",
+                        contact.getEmailAddress()));
+              }
+            });
  }

  /**
@@ -48,9 +48,11 @@
    <class>google.registry.persistence.converter.StringListConverter</class>
    <class>google.registry.persistence.converter.StringSetConverter</class>
    <class>google.registry.persistence.converter.UpdateAutoTimestampConverter</class>
-    <class>google.registry.persistence.converter.VKeyConverter</class>
    <class>google.registry.persistence.converter.ZonedDateTimeConverter</class>

+    <!-- Generated converters for VKey -->
+    <class>google.registry.model.host.VKeyConverter_HostResource</class>
+
    <!-- TODO(weiminyu): check out application-layer validation. -->
    <validation-mode>NONE</validation-mode>
  </persistence-unit>
@@ -145,6 +145,7 @@
        {param label: 'Email' /}
        {param namePrefix: $namePrefix /}
        {param name: 'emailAddress' /}
+        {param disabled: not $readonly and $item['emailAddress'] != null /}
      {/call}
      {call registry.soy.forms.inputFieldRow data="all"}
        {param label: 'Phone' /}
@@ -50,7 +50,11 @@ import org.junit.runners.JUnit4;
 public class ExportCommitLogDiffActionTest {

  @Rule
-  public final AppEngineRule appEngine = AppEngineRule.builder().withDatastoreAndCloudSql().build();
+  public final AppEngineRule appEngine =
+      AppEngineRule.builder()
+          .withDatastoreAndCloudSql()
+          .withOfyTestEntities(TestObject.class)
+          .build();

  /** Local GCS service available for testing. */
  private final GcsService gcsService = GcsServiceFactory.createGcsService();
@@ -71,7 +71,11 @@ public class RestoreCommitLogsActionTest {
  final GcsService gcsService = createGcsService();

  @Rule
-  public final AppEngineRule appEngine = AppEngineRule.builder().withDatastoreAndCloudSql().build();
+  public final AppEngineRule appEngine =
+      AppEngineRule.builder()
+          .withDatastoreAndCloudSql()
+          .withOfyTestEntities(TestObject.class)
+          .build();

  @Before
  public void init() {
@@ -19,12 +19,10 @@ import static google.registry.model.ofy.ObjectifyService.ofy;
 import static google.registry.persistence.transaction.TransactionManagerFactory.tm;
 import static org.joda.time.DateTimeZone.UTC;

-import com.googlecode.objectify.ObjectifyService;
 import com.googlecode.objectify.annotation.Entity;
 import google.registry.model.common.CrossTldSingleton;
 import google.registry.testing.AppEngineRule;
 import org.joda.time.DateTime;
-import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -35,19 +33,18 @@ import org.junit.runners.JUnit4;
 public class CreateAutoTimestampTest {

  @Rule
-  public final AppEngineRule appEngine = AppEngineRule.builder().withDatastoreAndCloudSql().build();
+  public final AppEngineRule appEngine =
+      AppEngineRule.builder()
+          .withDatastoreAndCloudSql()
+          .withOfyTestEntities(TestObject.class)
+          .build();

  /** Timestamped class. */
-  @Entity
+  @Entity(name = "CatTestEntity")
  public static class TestObject extends CrossTldSingleton {
    CreateAutoTimestamp createTime = CreateAutoTimestamp.create(null);
  }

-  @Before
-  public void before() {
-    ObjectifyService.register(TestObject.class);
-  }
-
  private TestObject reload() {
    return ofy().load().entity(new TestObject()).now();
  }
@@ -27,7 +27,6 @@ import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
 import com.googlecode.objectify.Key;
-import com.googlecode.objectify.ObjectifyService;
 import com.googlecode.objectify.annotation.Entity;
 import com.googlecode.objectify.annotation.Id;
 import google.registry.testing.AppEngineRule;
@@ -39,7 +38,6 @@ import java.util.List;
 import java.util.Map;
 import java.util.Set;
 import org.joda.time.DateTime;
-import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -50,12 +48,11 @@ import org.junit.runners.JUnit4;
 public class ImmutableObjectTest {

  @Rule
-  public final AppEngineRule appEngine = AppEngineRule.builder().withDatastoreAndCloudSql().build();
-
-  @Before
-  public void register() {
-    ObjectifyService.register(ValueObject.class);
-  }
+  public final AppEngineRule appEngine =
+      AppEngineRule.builder()
+          .withDatastoreAndCloudSql()
+          .withOfyTestEntities(ValueObject.class)
+          .build();

  /** Simple subclass of ImmutableObject. */
  public static class SimpleObject extends ImmutableObject {
@@ -19,12 +19,10 @@ import static google.registry.model.ofy.ObjectifyService.ofy;
 import static google.registry.persistence.transaction.TransactionManagerFactory.tm;
 import static org.joda.time.DateTimeZone.UTC;

-import com.googlecode.objectify.ObjectifyService;
 import com.googlecode.objectify.annotation.Entity;
 import google.registry.model.common.CrossTldSingleton;
 import google.registry.testing.AppEngineRule;
 import org.joda.time.DateTime;
-import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -35,19 +33,18 @@ import org.junit.runners.JUnit4;
 public class UpdateAutoTimestampTest {

  @Rule
-  public final AppEngineRule appEngine = AppEngineRule.builder().withDatastoreAndCloudSql().build();
+  public final AppEngineRule appEngine =
+      AppEngineRule.builder()
+          .withDatastoreAndCloudSql()
+          .withOfyTestEntities(TestObject.class)
+          .build();

  /** Timestamped class. */
-  @Entity
+  @Entity(name = "UatTestEntity")
  public static class TestObject extends CrossTldSingleton {
    UpdateAutoTimestamp updateTime = UpdateAutoTimestamp.create(null);
  }

-  @Before
-  public void before() {
-    ObjectifyService.register(TestObject.class);
-  }
-
  private TestObject reload() {
    return ofy().load().entity(new TestObject()).now();
  }
@@ -16,7 +16,6 @@ package google.registry.model.ofy;

 import static com.google.appengine.api.datastore.EntityTranslator.convertToPb;
 import static com.google.common.truth.Truth.assertThat;
-import static com.googlecode.objectify.ObjectifyService.register;
 import static google.registry.model.common.EntityGroupRoot.getCrossTldKey;
 import static google.registry.model.ofy.CommitLogBucket.getBucketKey;
 import static google.registry.model.ofy.ObjectifyService.ofy;
@@ -47,7 +46,11 @@ import org.junit.runners.JUnit4;
 public class OfyCommitLogTest {

  @Rule
-  public final AppEngineRule appEngine = AppEngineRule.builder().withDatastoreAndCloudSql().build();
+  public final AppEngineRule appEngine =
+      AppEngineRule.builder()
+          .withDatastoreAndCloudSql()
+          .withOfyTestEntities(TestVirtualObject.class, Root.class, Child.class)
+          .build();

  @Rule
  public final InjectRule inject = new InjectRule();
@@ -56,8 +59,6 @@ public class OfyCommitLogTest {

  @Before
  public void before() {
-    register(Root.class);
-    register(Child.class);
    inject.setStaticField(Ofy.class, "clock", clock);
  }

@@ -28,7 +28,6 @@ import static org.mockito.Mockito.when;

 import google.registry.model.ofy.Ofy;
 import google.registry.model.server.Lock.LockState;
-import google.registry.schema.server.LockDao;
 import google.registry.testing.AppEngineRule;
 import google.registry.testing.FakeClock;
 import google.registry.testing.InjectRule;
@@ -65,19 +64,16 @@ public class LockTest {
    Optional<Lock> lock = Lock.acquire(RESOURCE_NAME, tld, leaseLength, requestStatusChecker, true);
    verify(Lock.lockMetrics).recordAcquire(RESOURCE_NAME, tld, expectedLockState);
    verifyNoMoreInteractions(Lock.lockMetrics);
-    assertThat(LockDao.load(RESOURCE_NAME, tld)).isPresent();
    Lock.lockMetrics = null;
    return lock;
  }

  private void release(Lock lock, String expectedTld, long expectedMillis) {
-    assertThat(LockDao.load(RESOURCE_NAME, expectedTld)).isPresent();
    Lock.lockMetrics = mock(LockMetrics.class);
    lock.release();
    verify(Lock.lockMetrics)
        .recordRelease(RESOURCE_NAME, expectedTld, Duration.millis(expectedMillis));
    verifyNoMoreInteractions(Lock.lockMetrics);
-    assertThat(LockDao.load(RESOURCE_NAME, expectedTld)).isEmpty();
    Lock.lockMetrics = null;
  }

@@ -23,7 +23,6 @@ import static org.joda.time.Duration.standardHours;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSortedMap;
 import com.googlecode.objectify.Key;
-import com.googlecode.objectify.ObjectifyService;
 import com.googlecode.objectify.annotation.Entity;
 import google.registry.model.common.CrossTldSingleton;
 import google.registry.model.ofy.CommitLogManifest;
@@ -45,13 +44,17 @@ public class CommitLogRevisionsTranslatorFactoryTest {

  private static final DateTime START_TIME = DateTime.parse("2000-01-01TZ");

-  @Entity
+  @Entity(name = "ClrtfTestEntity")
  public static class TestObject extends CrossTldSingleton {
    ImmutableSortedMap<DateTime, Key<CommitLogManifest>> revisions = ImmutableSortedMap.of();
  }

  @Rule
-  public final AppEngineRule appEngine = AppEngineRule.builder().withDatastoreAndCloudSql().build();
+  public final AppEngineRule appEngine =
+      AppEngineRule.builder()
+          .withDatastoreAndCloudSql()
+          .withOfyTestEntities(TestObject.class)
+          .build();

  @Rule
  public final InjectRule inject = new InjectRule();
@@ -60,7 +63,6 @@ public class CommitLogRevisionsTranslatorFactoryTest {

  @Before
  public void before() {
-    ObjectifyService.register(TestObject.class);
    inject.setStaticField(Ofy.class, "clock", clock);
  }

@@ -28,15 +28,19 @@ public class VKeyTest {

  @Rule
  public final AppEngineRule appEngineRule =
-      AppEngineRule.builder().withDatastoreAndCloudSql().build();
+      AppEngineRule.builder()
+          .withDatastoreAndCloudSql()
+          .withOfyTestEntities(TestObject.class)
+          .build();

  public VKeyTest() {}

  @Test
  public void testOptionalAccessors() {
-    VKey<TestObject> key = VKey.create(TestObject.class, null, null);
-    assertThat(key.maybeGetSqlKey().isPresent()).isFalse();
-    assertThat(key.maybeGetOfyKey().isPresent()).isFalse();
+    VKey<TestObject> key =
+        VKey.create(TestObject.class, "foo", Key.create(TestObject.create("foo")));
+    assertThat(key.maybeGetSqlKey().isPresent()).isTrue();
+    assertThat(key.maybeGetOfyKey().isPresent()).isTrue();

    Key<TestObject> ofyKey = Key.create(TestObject.create("foo"));
    assertThat(VKey.createOfy(TestObject.class, ofyKey).maybeGetOfyKey().get()).isEqualTo(ofyKey);
@@ -0,0 +1,65 @@
+// 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.persistence.converter;
+
+import static com.google.common.truth.Truth.assertThat;
+import static google.registry.persistence.transaction.TransactionManagerFactory.jpaTm;
+
+import google.registry.persistence.VKey;
+import google.registry.persistence.WithLongVKey;
+import google.registry.persistence.transaction.JpaTestRules;
+import google.registry.persistence.transaction.JpaTestRules.JpaUnitTestRule;
+import javax.persistence.Entity;
+import javax.persistence.Id;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
+
+/** Test SQL persistence of VKey. */
+@RunWith(JUnit4.class)
+public class LongVKeyConverterTest {
+
+  @Rule
+  public final JpaUnitTestRule jpaRule =
+      new JpaTestRules.Builder()
+          .withEntityClass(TestEntity.class, VKeyConverter_LongType.class)
+          .buildUnitTestRule();
+
+  @Test
+  public void testRoundTrip() {
+    TestEntity original = new TestEntity(VKey.createSql(TestEntity.class, 10L));
+    jpaTm().transact(() -> jpaTm().getEntityManager().persist(original));
+
+    TestEntity retrieved =
+        jpaTm().transact(() -> jpaTm().getEntityManager().find(TestEntity.class, "id"));
+    assertThat(retrieved.number.getSqlKey()).isEqualTo(10L);
+  }
+
+  @Entity(name = "TestEntity")
+  @WithLongVKey(classNameSuffix = "LongType")
+  static class TestEntity {
+    @Id String id = "id";
+
+    VKey<TestEntity> number;
+
+    TestEntity(VKey<TestEntity> number) {
+      this.number = number;
+    }
+
+    /** Default constructor, needed for hibernate. */
+    public TestEntity() {}
+  }
+}
@@ -18,9 +18,9 @@ import static com.google.common.truth.Truth.assertThat;
 import static google.registry.persistence.transaction.TransactionManagerFactory.jpaTm;

 import google.registry.persistence.VKey;
+import google.registry.persistence.WithStringVKey;
 import google.registry.persistence.transaction.JpaTestRules;
 import google.registry.persistence.transaction.JpaTestRules.JpaUnitTestRule;
-import javax.persistence.Convert;
 import javax.persistence.Entity;
 import javax.persistence.Id;
 import org.junit.Rule;
@@ -30,13 +30,15 @@ import org.junit.runners.JUnit4;

 /** Test SQL persistence of VKey. */
@RunWith(JUnit4.class)
-public class VKeyConverterTest {
+public class StringVKeyConverterTest {

  @Rule
  public final JpaUnitTestRule jpaRule =
-      new JpaTestRules.Builder().withEntityClass(TestEntity.class).buildUnitTestRule();
+      new JpaTestRules.Builder()
+          .withEntityClass(TestEntity.class, VKeyConverter_StringType.class)
+          .buildUnitTestRule();

-  public VKeyConverterTest() {}
+  public StringVKeyConverterTest() {}

  @Test
  public void testRoundTrip() {
@@ -50,21 +52,11 @@ public class VKeyConverterTest {
    assertThat(retrieved.other.getSqlKey()).isEqualTo("ImSpartacus!");
  }

-  static class TestEntityVKeyConverter extends VKeyConverter<TestEntity> {
-
-    @Override
-    protected Class<TestEntity> getAttributeClass() {
-      return TestEntity.class;
-    }
-  }
-
  @Entity(name = "TestEntity")
+  @WithStringVKey(classNameSuffix = "StringType")
  static class TestEntity {
    @Id String id;

-    // Specifying "@Converter(autoApply = true) on TestEntityVKeyConverter this doesn't seem to
-    // work.
-    @Convert(converter = TestEntityVKeyConverter.class)
    VKey<TestEntity> other;

    TestEntity(String id, VKey<TestEntity> other) {
@@ -34,6 +34,7 @@ import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Sets;
 import com.google.common.io.Files;
+import com.googlecode.objectify.Key;
 import com.googlecode.objectify.ObjectifyFilter;
 import google.registry.model.ofy.ObjectifyService;
 import google.registry.model.registrar.Registrar;
@@ -50,6 +51,7 @@ import java.io.File;
 import java.io.IOException;
 import java.util.HashSet;
 import java.util.List;
+import java.util.Optional;
 import java.util.Set;
 import java.util.logging.LogManager;
 import javax.annotation.Nullable;
@@ -127,10 +129,14 @@ public final class AppEngineRule extends ExternalResource
  private String taskQueueXml;
  private UserInfo userInfo;

+  // Test Objectify entity classes to be used with this AppEngineRule instance.
+  private ImmutableList<Class<?>> ofyTestEntities;
+
  /** Builder for {@link AppEngineRule}. */
  public static class Builder {

    private AppEngineRule rule = new AppEngineRule();
+    private ImmutableList.Builder<Class<?>> ofyTestEntities = new ImmutableList.Builder();

    /** Turn on the Datastore service and the Cloud SQL service. */
    public Builder withDatastoreAndCloudSql() {
@@ -181,10 +187,29 @@ public final class AppEngineRule extends ExternalResource
      return this;
    }

+    /**
+     * Declares test-only entities to be registered with {@code ObjectifyService}.
+     *
+     * <p>Note that {@code ObjectifyService} silently replaces the current registration for a given
+     * kind when a different class is registered for this kind. Since {@code ObjectifyService} does
+     * not support de-registration, each test entity class must be of a unique kind across the
+     * entire code base. Although this requirement can be worked around by using different {@code
+     * ObjectifyService} instances for each test (class), the setup overhead would rise
+     * significantly.
+     *
+     * @see AppEngineRule#register(Class)
+     */
+    @SafeVarargs
+    public final Builder withOfyTestEntities(Class<?>... entities) {
+      ofyTestEntities.add(entities);
+      return this;
+    }
+
    public AppEngineRule build() {
      checkState(
          !rule.enableJpaEntityCoverageCheck || rule.withDatastoreAndCloudSql,
          "withJpaEntityCoverageCheck enabled without Cloud SQL");
+      rule.ofyTestEntities = this.ofyTestEntities.build();
      return rule;
    }
  }
@@ -411,6 +436,7 @@ public final class AppEngineRule extends ExternalResource
      // Reset id allocation in ObjectifyService so that ids are deterministic in tests.
      ObjectifyService.resetNextTestId();
      loadInitialData();
+      this.ofyTestEntities.forEach(AppEngineRule::register);
    }
  }

@@ -443,6 +469,24 @@ public final class AppEngineRule extends ExternalResource
    }
  }

+  /**
+   * Registers test-only Objectify entities and checks for re-registrations for the same kind by
+   * different classes.
+   */
+  private static void register(Class<?> entityClass) {
+    String kind = Key.getKind(entityClass);
+    Optional.ofNullable(com.googlecode.objectify.ObjectifyService.factory().getMetadata(kind))
+        .ifPresent(
+            meta ->
+                checkState(
+                    meta.getEntityClass() == entityClass,
+                    "Cannot register %s. The Kind %s is already registered with %s.",
+                    entityClass.getName(),
+                    kind,
+                    meta.getEntityClass().getName()));
+    com.googlecode.objectify.ObjectifyService.register(entityClass);
+  }
+
  /** Install {@code testing/logging.properties} so logging is less noisy. */
  private static void setupLogging() throws IOException {
    LogManager.getLogManager()
@@ -18,6 +18,7 @@ import static java.nio.charset.StandardCharsets.UTF_8;
 import static org.junit.Assert.assertThrows;

 import com.google.common.base.Joiner;
+import com.googlecode.objectify.annotation.Entity;
 import java.io.File;
 import java.io.IOException;
 import org.junit.Before;
@@ -88,9 +89,29 @@ public class AppEngineRuleTest {
    assertThrows(AssertionError.class, () -> appEngineRule.after());
  }

+  @Test
+  public void testRegisterOfyEntities_failure() {
+    AppEngineRule appEngineRule =
+        AppEngineRule.builder()
+            .withDatastoreAndCloudSql()
+            .withOfyTestEntities(google.registry.testing.TestObject.class, TestObject.class)
+            .build();
+    String expectedErrorMessage =
+        String.format(
+            "Cannot register %s. The Kind %s is already registered with %s",
+            TestObject.class.getName(),
+            "TestObject",
+            google.registry.testing.TestObject.class.getName());
+    assertThrows(expectedErrorMessage, IllegalStateException.class, appEngineRule::before);
+    appEngineRule.after();
+  }
+
  private void writeAutoIndexFile(String content) throws IOException {
    com.google.common.io.Files.asCharSink(
            new File(temporaryFolder.getRoot(), "datastore-indexes-auto.xml"), UTF_8)
        .write(content);
  }
+
+  @Entity
+  private static final class TestObject {}
 }
@@ -17,7 +17,6 @@ package google.registry.testing;
 import static google.registry.model.common.EntityGroupRoot.getCrossTldKey;

 import com.googlecode.objectify.Key;
-import com.googlecode.objectify.ObjectifyService;
 import com.googlecode.objectify.annotation.Entity;
 import com.googlecode.objectify.annotation.Id;
 import com.googlecode.objectify.annotation.Parent;
@@ -30,9 +29,6 @@ import google.registry.model.common.EntityGroupRoot;
 */
@Entity
 public class TestObject extends ImmutableObject {
-  static {
-    ObjectifyService.register(TestObject.class);  // Register this kind on first reference.
-  }

  @Parent
  Key<EntityGroupRoot> parent;
@@ -70,9 +66,6 @@ public class TestObject extends ImmutableObject {
  @Entity
  @VirtualEntity
  public static class TestVirtualObject extends ImmutableObject {
-    static {
-      ObjectifyService.register(TestVirtualObject.class);  // Register this kind on first reference.
-    }

    @Id
    String id;
@@ -66,13 +66,12 @@ public class ContactSettingsTest extends RegistrarSettingsActionTestCase {

  @Test
  public void testPost_updateContacts_success() throws Exception {
-    // Remove all the contacts but the first by updating with list of
-    // just it.
+    // Remove all the contacts but one by updating with a list of just it
    ImmutableMap<String, String> adminContact1 =
        ImmutableMap.of(
-            "name", "contact1",
-            "emailAddress", "contact1@email.com",
-            "phoneNumber", "+1.2125650001",
+            "name", "Marla Singer",
+            "emailAddress", "Marla.Singer@crr.com",
+            "phoneNumber", "+1.2128675309",
            // Have to keep ADMIN or else expect FormException for at-least-one.
            "types", "ADMIN,TECH");

@@ -83,15 +82,12 @@ public class ContactSettingsTest extends RegistrarSettingsActionTestCase {
        action.handleJsonRequest(ImmutableMap.of("op", "update", "id", CLIENT_ID, "args", regMap));
    assertThat(response).containsEntry("status", "SUCCESS");

-    RegistrarContact newContact =
-        new RegistrarContact.Builder()
-            .setParent(registrar)
-            .setName(adminContact1.get("name"))
-            .setEmailAddress(adminContact1.get("emailAddress"))
-            .setPhoneNumber(adminContact1.get("phoneNumber"))
-            .setTypes(ImmutableList.of(Type.ADMIN, Type.TECH))
-            .build();
-    assertThat(loadRegistrar(CLIENT_ID).getContacts()).containsExactly(newContact);
+    RegistrarContact foundContact =
+        Iterables.getOnlyElement(loadRegistrar(CLIENT_ID).getContacts());
+    assertThat(foundContact.getName()).isEqualTo(adminContact1.get("name"));
+    assertThat(foundContact.getEmailAddress()).isEqualTo(adminContact1.get("emailAddress"));
+    assertThat(foundContact.getPhoneNumber()).isEqualTo(adminContact1.get("phoneNumber"));
+    assertThat(foundContact.getTypes()).containsExactly(Type.ADMIN, Type.TECH);
    assertMetric(CLIENT_ID, "update", "[OWNER]", "SUCCESS");
    verifyNotificationEmailsSent();
  }
@@ -188,41 +184,65 @@ public class ContactSettingsTest extends RegistrarSettingsActionTestCase {

  @Test
  public void testSuccess_setRegistryLockPassword() {
-    addPasswordToTechContact();
-    techContact = Iterables.getOnlyElement(loadRegistrar(CLIENT_ID).getContactsOfType(Type.TECH));
-    assertThat(techContact.verifyRegistryLockPassword("hellothere")).isTrue();
+    addPasswordToContactTwo();
+    String emailAddress = AppEngineRule.makeRegistrarContact2().getEmailAddress();
+    RegistrarContact newContactWithPassword =
+        loadRegistrar(CLIENT_ID).getContacts().stream()
+            .filter(rc -> rc.getEmailAddress().equals(emailAddress))
+            .findFirst()
+            .get();
+    assertThat(newContactWithPassword.verifyRegistryLockPassword("hellothere")).isTrue();
    assertMetric(CLIENT_ID, "update", "[OWNER]", "SUCCESS");
  }

  @Test
  public void testSuccess_setRegistryLockPassword_notOverriddenLater() {
-    addPasswordToTechContact();
-    techContact = Iterables.getOnlyElement(loadRegistrar(CLIENT_ID).getContactsOfType(Type.TECH));
-    assertThat(techContact.verifyRegistryLockPassword("hellothere")).isTrue();
+    addPasswordToContactTwo();
+    String emailAddress = AppEngineRule.makeRegistrarContact2().getEmailAddress();
+    RegistrarContact newContactWithPassword =
+        loadRegistrar(CLIENT_ID).getContacts().stream()
+            .filter(rc -> rc.getEmailAddress().equals(emailAddress))
+            .findFirst()
+            .get();
+    assertThat(newContactWithPassword.verifyRegistryLockPassword("hellothere")).isTrue();

-    techContact = Iterables.getOnlyElement(loadRegistrar(CLIENT_ID).getContactsOfType(Type.TECH));
-    Map<String, Object> techContactMap = techContact.toJsonMap();
-    techContactMap.put("name", "Some Other Name");
+    Map<String, Object> newContactMap = newContactWithPassword.toJsonMap();
+    newContactMap.put("name", "Some Other Name");
    Map<String, Object> reqJson = loadRegistrar(CLIENT_ID).toJsonMap();
    reqJson.put(
        "contacts",
-        ImmutableList.of(AppEngineRule.makeRegistrarContact2().toJsonMap(), techContactMap));
+        ImmutableList.of(
+            AppEngineRule.makeRegistrarContact1().toJsonMap(),
+            newContactMap,
+            AppEngineRule.makeRegistrarContact3().toJsonMap()));
    Map<String, Object> response =
        action.handleJsonRequest(ImmutableMap.of("op", "update", "id", CLIENT_ID, "args", reqJson));
    assertThat(response).containsAtLeastEntriesIn(ImmutableMap.of("status", "SUCCESS"));
-    techContact = Iterables.getOnlyElement(loadRegistrar(CLIENT_ID).getContactsOfType(Type.TECH));
-    assertThat(techContact.verifyRegistryLockPassword("hellothere")).isTrue();
+    newContactWithPassword =
+        loadRegistrar(CLIENT_ID).getContacts().stream()
+            .filter(rc -> rc.getEmailAddress().equals(emailAddress))
+            .findFirst()
+            .get();
+    assertThat(newContactWithPassword.verifyRegistryLockPassword("hellothere")).isTrue();
  }

-  private void addPasswordToTechContact() {
-    techContact =
-        persistResource(techContact.asBuilder().setAllowedToSetRegistryLockPassword(true).build());
-    Map<String, Object> contactMap = techContact.toJsonMap();
+  private void addPasswordToContactTwo() {
+    RegistrarContact contact =
+        persistResource(
+            AppEngineRule.makeRegistrarContact2()
+                .asBuilder()
+                .setRegistryLockEmailAddress("johndoe@theregistrar.com")
+                .setAllowedToSetRegistryLockPassword(true)
+                .build());
+    Map<String, Object> contactMap = contact.toJsonMap();
    contactMap.put("registryLockPassword", "hellothere");
    Map<String, Object> reqJson = loadRegistrar(CLIENT_ID).toJsonMap();
    reqJson.put(
        "contacts",
-        ImmutableList.of(AppEngineRule.makeRegistrarContact2().toJsonMap(), contactMap));
+        ImmutableList.of(
+            AppEngineRule.makeRegistrarContact1().toJsonMap(),
+            contactMap,
+            AppEngineRule.makeRegistrarContact3().toJsonMap()));
    Map<String, Object> response =
        action.handleJsonRequest(ImmutableMap.of("op", "update", "id", CLIENT_ID, "args", reqJson));
    assertThat(response).containsAtLeastEntriesIn(ImmutableMap.of("status", "SUCCESS"));
@@ -260,13 +280,16 @@ public class ContactSettingsTest extends RegistrarSettingsActionTestCase {
  public void testPost_failure_setRegistryLockPassword_notAllowed() {
    // "allowedToSetRegistryLockPassword" must be set through the back end first
    // before we can set a password through the UI
-    Map<String, Object> contactMap =
-        techContact.asBuilder().setAllowedToSetRegistryLockPassword(true).build().toJsonMap();
+    Map<String, Object> contactMap = AppEngineRule.makeRegistrarContact2().toJsonMap();
+    contactMap.put("allowedToSetRegistryLockPassword", true);
    contactMap.put("registryLockPassword", "hellothere");
    Map<String, Object> reqJson = loadRegistrar(CLIENT_ID).toJsonMap();
    reqJson.put(
        "contacts",
-        ImmutableList.of(AppEngineRule.makeRegistrarContact2().toJsonMap(), contactMap));
+        ImmutableList.of(
+            AppEngineRule.makeRegistrarContact1().toJsonMap(),
+            contactMap,
+            AppEngineRule.makeRegistrarContact3().toJsonMap()));

    Map<String, Object> response =
        action.handleJsonRequest(ImmutableMap.of("op", "update", "id", CLIENT_ID, "args", reqJson));
@@ -304,6 +327,30 @@ public class ContactSettingsTest extends RegistrarSettingsActionTestCase {
    assertMetric(CLIENT_ID, "update", "[OWNER]", "ERROR: FormException");
  }

+  @Test
+  public void testPost_failure_removingRegistryLockContact() {
+    ImmutableMap<String, String> contact =
+        ImmutableMap.of(
+            "name", "contact1",
+            "emailAddress", "contact1@email.com",
+            "phoneNumber", "+1.2125650001",
+            // Have to keep ADMIN or else expect FormException for at-least-one.
+            "types", "ADMIN,TECH");
+    Map<String, Object> regMap = loadRegistrar(CLIENT_ID).toJsonMap();
+    regMap.put("contacts", ImmutableList.of(contact));
+    Map<String, Object> response =
+        action.handleJsonRequest(ImmutableMap.of("op", "update", "id", CLIENT_ID, "args", regMap));
+    assertThat(response)
+        .containsExactly(
+            "status",
+            "ERROR",
+            "results",
+            ImmutableList.of(),
+            "message",
+            "Cannot delete the contact Marla.Singer@crr.com that has registry lock enabled");
+    assertMetric(CLIENT_ID, "update", "[OWNER]", "ERROR: FormException");
+  }
+
  @Test
  public void testPost_failure_setRegistryLock_passwordTooShort() {
    techContact =
@@ -14,6 +14,7 @@

 package google.registry.ui.server.registrar;

+import static com.google.appengine.repackaged.com.google.common.collect.Iterables.getOnlyElement;
 import static com.google.monitoring.metrics.contrib.LongMetricSubject.assertThat;
 import static google.registry.config.RegistryConfig.getGSuiteOutgoingEmailAddress;
 import static google.registry.config.RegistryConfig.getGSuiteOutgoingEmailDisplayName;
@@ -23,14 +24,12 @@ import static google.registry.security.JsonHttpTestUtils.createJsonPayload;
 import static google.registry.testing.DatastoreHelper.createTlds;
 import static google.registry.testing.DatastoreHelper.disallowRegistrarAccess;
 import static google.registry.testing.DatastoreHelper.loadRegistrar;
-import static google.registry.testing.DatastoreHelper.persistResource;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;

 import com.google.appengine.api.users.User;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
-import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.ImmutableSetMultimap;
 import com.google.common.truth.Truth;
 import google.registry.model.ofy.Ofy;
@@ -95,21 +94,13 @@ public abstract class RegistrarSettingsActionTestCase {
    // Add a technical contact to the registrar (in addition to the default admin contact created by
    // AppEngineRule).
    techContact =
-        persistResource(
-            new RegistrarContact.Builder()
-                .setParent(loadRegistrar(CLIENT_ID))
-                .setName("Jian-Yang")
-                .setEmailAddress("jyang@bachman.accelerator")
-                .setRegistryLockEmailAddress("jyang.registrylock@bachman.accelerator")
-                .setPhoneNumber("+1.1234567890")
-                .setTypes(ImmutableSet.of(RegistrarContact.Type.TECH))
-                .build());
+        getOnlyElement(loadRegistrar(CLIENT_ID).getContactsOfType(RegistrarContact.Type.TECH));

    action.registrarAccessor = null;
    action.appEngineServiceUtils = appEngineServiceUtils;
    when(appEngineServiceUtils.getCurrentVersionHostname("backend")).thenReturn("backend.hostname");
-    action.jsonActionRunner = new JsonActionRunner(
-        ImmutableMap.of(), new JsonResponse(new ResponseImpl(rsp)));
+    action.jsonActionRunner =
+        new JsonActionRunner(ImmutableMap.of(), new JsonResponse(new ResponseImpl(rsp)));
    action.sendEmailUtils =
        new SendEmailUtils(
            getGSuiteOutgoingEmailAddress(),
@@ -18,6 +18,17 @@
        "emailAddress": "etphonehome@example.com",
        "gaeUserId": null,
        "types": "ADMIN,BILLING,TECH,WHOIS"
+      },
+      {
+        "visibleInWhoisAsAdmin": false,
+        "faxNumber": null,
+        "phoneNumber": "+1.2128675309",
+        "name": "Marla Singer",
+        "visibleInWhoisAsTech": false,
+        "emailAddress": "Marla.Singer@crr.com",
+        "registryLockEmailAddress": "Marla.Singer.RegistryLock@crr.com",
+        "gaeUserId": "12345",
+        "types": "TECH"
      }
    ],
    "allowedTlds": [
@@ -13,8 +13,7 @@ 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}
    REMOVED:
-        {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=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=Jian-Yang, emailAddress=jyang@bachman.accelerator, registryLockEmailAddress=jyang.registrylock@bachman.accelerator, phoneNumber=+1.1234567890, faxNumber=null, types=[TECH], gaeUserId=null, visibleInWhoisAsAdmin=false, visibleInWhoisAsTech=false, visibleInDomainWhoisAsAbuse=false, allowedToSetRegistryLockPassword=false}
+        {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}
    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, 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}
@@ -78,6 +78,7 @@ ext {
      'com.googlecode.json-simple:json-simple:1.1.1',
      'com.ibm.icu:icu4j:57.1',
      'com.jcraft:jsch:0.1.55',
+      'com.squareup:javapoet:1.12.1',
      'com.sun.activation:javax.activation:1.2.0',
      'com.sun.xml.bind:jaxb-impl:2.2.11',
      'com.sun.xml.bind:jaxb-osgi:2.2.11',
@@ -0,0 +1,25 @@
+// 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.
+
+plugins {
+    id 'java-library'
+}
+
+dependencies {
+    def deps = rootProject.dependencyMap
+    compile deps['com.google.code.findbugs:jsr305']
+    compile deps['com.google.guava:guava']
+    compile deps['com.squareup:javapoet']
+    compile deps['javax.persistence:javax.persistence-api']
+}
@@ -0,0 +1,24 @@
+# This is a Gradle generated file for dependency locking.
+# Manual edits can break the build and are not advised.
+# This file is expected to be part of source control.
+com.github.kevinstern:software-and-algorithms:1.0
+com.github.stephenc.jcip:jcip-annotations:1.0-1
+com.google.auto:auto-common:0.10
+com.google.code.findbugs:jFormatString:3.0.0
+com.google.code.findbugs:jsr305:3.0.2
+com.google.errorprone:error_prone_annotation:2.3.3
+com.google.errorprone:error_prone_annotations:2.3.3
+com.google.errorprone:error_prone_check_api:2.3.3
+com.google.errorprone:error_prone_core:2.3.3
+com.google.errorprone:error_prone_type_annotations:2.3.3
+com.google.guava:failureaccess:1.0.1
+com.google.guava:guava:27.0.1-jre
+com.google.guava:listenablefuture:9999.0-empty-to-avoid-conflict-with-guava
+com.google.j2objc:j2objc-annotations:1.1
+com.google.protobuf:protobuf-java:3.4.0
+com.googlecode.java-diff-utils:diffutils:1.3.0
+org.checkerframework:checker-qual:2.5.3
+org.checkerframework:dataflow:2.5.3
+org.checkerframework:javacutil:2.5.3
+org.codehaus.mojo:animal-sniffer-annotations:1.17
+org.pcollections:pcollections:2.1.2
@@ -0,0 +1,3 @@
+# This is a Gradle generated file for dependency locking.
+# Manual edits can break the build and are not advised.
+# This file is expected to be part of source control.
@@ -0,0 +1,3 @@
+# This is a Gradle generated file for dependency locking.
+# Manual edits can break the build and are not advised.
+# This file is expected to be part of source control.
@@ -0,0 +1,3 @@
+# This is a Gradle generated file for dependency locking.
+# Manual edits can break the build and are not advised.
+# This file is expected to be part of source control.
@@ -0,0 +1,18 @@
+# This is a Gradle generated file for dependency locking.
+# Manual edits can break the build and are not advised.
+# This file is expected to be part of source control.
+antlr:antlr:2.7.7
+com.google.code.findbugs:jsr305:3.0.2
+com.google.errorprone:error_prone_annotations:2.3.2
+com.google.guava:failureaccess:1.0.1
+com.google.guava:guava:28.1-jre
+com.google.guava:listenablefuture:9999.0-empty-to-avoid-conflict-with-guava
+com.google.j2objc:j2objc-annotations:1.3
+com.puppycrawl.tools:checkstyle:8.27
+commons-beanutils:commons-beanutils:1.9.4
+commons-collections:commons-collections:3.2.2
+info.picocli:picocli:4.1.1
+net.sf.saxon:Saxon-HE:9.9.1-5
+org.antlr:antlr4-runtime:4.7.2
+org.checkerframework:checker-qual:2.8.1
+org.codehaus.mojo:animal-sniffer-annotations:1.18
@@ -0,0 +1,12 @@
+# This is a Gradle generated file for dependency locking.
+# Manual edits can break the build and are not advised.
+# This file is expected to be part of source control.
+com.google.code.findbugs:jsr305:3.0.2
+com.google.errorprone:error_prone_annotations:2.3.4
+com.google.guava:failureaccess:1.0.1
+com.google.guava:guava:28.2-jre
+com.google.guava:listenablefuture:9999.0-empty-to-avoid-conflict-with-guava
+com.google.j2objc:j2objc-annotations:1.3
+com.squareup:javapoet:1.12.1
+javax.persistence:javax.persistence-api:2.2
+org.checkerframework:checker-qual:2.10.0
@@ -0,0 +1,12 @@
+# This is a Gradle generated file for dependency locking.
+# Manual edits can break the build and are not advised.
+# This file is expected to be part of source control.
+com.google.code.findbugs:jsr305:3.0.2
+com.google.errorprone:error_prone_annotations:2.3.4
+com.google.guava:failureaccess:1.0.1
+com.google.guava:guava:28.2-jre
+com.google.guava:listenablefuture:9999.0-empty-to-avoid-conflict-with-guava
+com.google.j2objc:j2objc-annotations:1.3
+com.squareup:javapoet:1.12.1
+javax.persistence:javax.persistence-api:2.2
+org.checkerframework:checker-qual:2.10.0
@@ -0,0 +1,3 @@
+# This is a Gradle generated file for dependency locking.
+# Manual edits can break the build and are not advised.
+# This file is expected to be part of source control.
@@ -0,0 +1,12 @@
+# This is a Gradle generated file for dependency locking.
+# Manual edits can break the build and are not advised.
+# This file is expected to be part of source control.
+com.google.code.findbugs:jsr305:3.0.2
+com.google.errorprone:error_prone_annotations:2.3.4
+com.google.guava:failureaccess:1.0.1
+com.google.guava:guava:28.2-jre
+com.google.guava:listenablefuture:9999.0-empty-to-avoid-conflict-with-guava
+com.google.j2objc:j2objc-annotations:1.3
+com.squareup:javapoet:1.12.1
+javax.persistence:javax.persistence-api:2.2
+org.checkerframework:checker-qual:2.10.0
@@ -0,0 +1,12 @@
+# This is a Gradle generated file for dependency locking.
+# Manual edits can break the build and are not advised.
+# This file is expected to be part of source control.
+com.google.code.findbugs:jsr305:3.0.2
+com.google.errorprone:error_prone_annotations:2.3.4
+com.google.guava:failureaccess:1.0.1
+com.google.guava:guava:28.2-jre
+com.google.guava:listenablefuture:9999.0-empty-to-avoid-conflict-with-guava
+com.google.j2objc:j2objc-annotations:1.3
+com.squareup:javapoet:1.12.1
+javax.persistence:javax.persistence-api:2.2
+org.checkerframework:checker-qual:2.10.0
@@ -0,0 +1,24 @@
+# This is a Gradle generated file for dependency locking.
+# Manual edits can break the build and are not advised.
+# This file is expected to be part of source control.
+com.github.kevinstern:software-and-algorithms:1.0
+com.github.stephenc.jcip:jcip-annotations:1.0-1
+com.google.auto:auto-common:0.10
+com.google.code.findbugs:jFormatString:3.0.0
+com.google.code.findbugs:jsr305:3.0.2
+com.google.errorprone:error_prone_annotation:2.3.3
+com.google.errorprone:error_prone_annotations:2.3.3
+com.google.errorprone:error_prone_check_api:2.3.3
+com.google.errorprone:error_prone_core:2.3.3
+com.google.errorprone:error_prone_type_annotations:2.3.3
+com.google.guava:failureaccess:1.0.1
+com.google.guava:guava:27.0.1-jre
+com.google.guava:listenablefuture:9999.0-empty-to-avoid-conflict-with-guava
+com.google.j2objc:j2objc-annotations:1.1
+com.google.protobuf:protobuf-java:3.4.0
+com.googlecode.java-diff-utils:diffutils:1.3.0
+org.checkerframework:checker-qual:2.5.3
+org.checkerframework:dataflow:2.5.3
+org.checkerframework:javacutil:2.5.3
+org.codehaus.mojo:animal-sniffer-annotations:1.17
+org.pcollections:pcollections:2.1.2
@@ -0,0 +1,4 @@
+# This is a Gradle generated file for dependency locking.
+# Manual edits can break the build and are not advised.
+# This file is expected to be part of source control.
+com.google.errorprone:javac:9+181-r4173-1
@@ -0,0 +1,4 @@
+# This is a Gradle generated file for dependency locking.
+# Manual edits can break the build and are not advised.
+# This file is expected to be part of source control.
+org.jacoco:org.jacoco.agent:0.8.5
@@ -0,0 +1,11 @@
+# This is a Gradle generated file for dependency locking.
+# Manual edits can break the build and are not advised.
+# This file is expected to be part of source control.
+org.jacoco:org.jacoco.agent:0.8.5
+org.jacoco:org.jacoco.ant:0.8.5
+org.jacoco:org.jacoco.core:0.8.5
+org.jacoco:org.jacoco.report:0.8.5
+org.ow2.asm:asm-analysis:7.2
+org.ow2.asm:asm-commons:7.2
+org.ow2.asm:asm-tree:7.2
+org.ow2.asm:asm:7.2
@@ -0,0 +1,12 @@
+# This is a Gradle generated file for dependency locking.
+# Manual edits can break the build and are not advised.
+# This file is expected to be part of source control.
+com.google.code.findbugs:jsr305:3.0.2
+com.google.errorprone:error_prone_annotations:2.3.4
+com.google.guava:failureaccess:1.0.1
+com.google.guava:guava:28.2-jre
+com.google.guava:listenablefuture:9999.0-empty-to-avoid-conflict-with-guava
+com.google.j2objc:j2objc-annotations:1.3
+com.squareup:javapoet:1.12.1
+javax.persistence:javax.persistence-api:2.2
+org.checkerframework:checker-qual:2.10.0
@@ -0,0 +1,12 @@
+# This is a Gradle generated file for dependency locking.
+# Manual edits can break the build and are not advised.
+# This file is expected to be part of source control.
+com.google.code.findbugs:jsr305:3.0.2
+com.google.errorprone:error_prone_annotations:2.3.4
+com.google.guava:failureaccess:1.0.1
+com.google.guava:guava:28.2-jre
+com.google.guava:listenablefuture:9999.0-empty-to-avoid-conflict-with-guava
+com.google.j2objc:j2objc-annotations:1.3
+com.squareup:javapoet:1.12.1
+javax.persistence:javax.persistence-api:2.2
+org.checkerframework:checker-qual:2.10.0
@@ -0,0 +1,24 @@
+# This is a Gradle generated file for dependency locking.
+# Manual edits can break the build and are not advised.
+# This file is expected to be part of source control.
+com.github.kevinstern:software-and-algorithms:1.0
+com.github.stephenc.jcip:jcip-annotations:1.0-1
+com.google.auto:auto-common:0.10
+com.google.code.findbugs:jFormatString:3.0.0
+com.google.code.findbugs:jsr305:3.0.2
+com.google.errorprone:error_prone_annotation:2.3.3
+com.google.errorprone:error_prone_annotations:2.3.3
+com.google.errorprone:error_prone_check_api:2.3.3
+com.google.errorprone:error_prone_core:2.3.3
+com.google.errorprone:error_prone_type_annotations:2.3.3
+com.google.guava:failureaccess:1.0.1
+com.google.guava:guava:27.0.1-jre
+com.google.guava:listenablefuture:9999.0-empty-to-avoid-conflict-with-guava
+com.google.j2objc:j2objc-annotations:1.1
+com.google.protobuf:protobuf-java:3.4.0
+com.googlecode.java-diff-utils:diffutils:1.3.0
+org.checkerframework:checker-qual:2.5.3
+org.checkerframework:dataflow:2.5.3
+org.checkerframework:javacutil:2.5.3
+org.codehaus.mojo:animal-sniffer-annotations:1.17
+org.pcollections:pcollections:2.1.2
@@ -0,0 +1,3 @@
+# This is a Gradle generated file for dependency locking.
+# Manual edits can break the build and are not advised.
+# This file is expected to be part of source control.
@@ -0,0 +1,12 @@
+# This is a Gradle generated file for dependency locking.
+# Manual edits can break the build and are not advised.
+# This file is expected to be part of source control.
+com.google.code.findbugs:jsr305:3.0.2
+com.google.errorprone:error_prone_annotations:2.3.4
+com.google.guava:failureaccess:1.0.1
+com.google.guava:guava:28.2-jre
+com.google.guava:listenablefuture:9999.0-empty-to-avoid-conflict-with-guava
+com.google.j2objc:j2objc-annotations:1.3
+com.squareup:javapoet:1.12.1
+javax.persistence:javax.persistence-api:2.2
+org.checkerframework:checker-qual:2.10.0
@@ -0,0 +1,12 @@
+# This is a Gradle generated file for dependency locking.
+# Manual edits can break the build and are not advised.
+# This file is expected to be part of source control.
+com.google.code.findbugs:jsr305:3.0.2
+com.google.errorprone:error_prone_annotations:2.3.4
+com.google.guava:failureaccess:1.0.1
+com.google.guava:guava:28.2-jre
+com.google.guava:listenablefuture:9999.0-empty-to-avoid-conflict-with-guava
+com.google.j2objc:j2objc-annotations:1.3
+com.squareup:javapoet:1.12.1
+javax.persistence:javax.persistence-api:2.2
+org.checkerframework:checker-qual:2.10.0
@@ -0,0 +1,3 @@
+# This is a Gradle generated file for dependency locking.
+# Manual edits can break the build and are not advised.
+# This file is expected to be part of source control.
@@ -0,0 +1,12 @@
+# This is a Gradle generated file for dependency locking.
+# Manual edits can break the build and are not advised.
+# This file is expected to be part of source control.
+com.google.code.findbugs:jsr305:3.0.2
+com.google.errorprone:error_prone_annotations:2.3.4
+com.google.guava:failureaccess:1.0.1
+com.google.guava:guava:28.2-jre
+com.google.guava:listenablefuture:9999.0-empty-to-avoid-conflict-with-guava
+com.google.j2objc:j2objc-annotations:1.3
+com.squareup:javapoet:1.12.1
+javax.persistence:javax.persistence-api:2.2
+org.checkerframework:checker-qual:2.10.0
@@ -0,0 +1,12 @@
+# This is a Gradle generated file for dependency locking.
+# Manual edits can break the build and are not advised.
+# This file is expected to be part of source control.
+com.google.code.findbugs:jsr305:3.0.2
+com.google.errorprone:error_prone_annotations:2.3.4
+com.google.guava:failureaccess:1.0.1
+com.google.guava:guava:28.2-jre
+com.google.guava:listenablefuture:9999.0-empty-to-avoid-conflict-with-guava
+com.google.j2objc:j2objc-annotations:1.3
+com.squareup:javapoet:1.12.1
+javax.persistence:javax.persistence-api:2.2
+org.checkerframework:checker-qual:2.10.0
@@ -0,0 +1,160 @@
+// 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.processors;
+
+import static com.google.common.base.Preconditions.checkState;
+import static com.google.common.collect.ImmutableList.toImmutableList;
+
+import com.squareup.javapoet.AnnotationSpec;
+import com.squareup.javapoet.ClassName;
+import com.squareup.javapoet.JavaFile;
+import com.squareup.javapoet.MethodSpec;
+import com.squareup.javapoet.ParameterizedTypeName;
+import com.squareup.javapoet.TypeName;
+import com.squareup.javapoet.TypeSpec;
+import java.io.IOException;
+import java.io.UncheckedIOException;
+import java.util.List;
+import java.util.Set;
+import javax.annotation.processing.AbstractProcessor;
+import javax.annotation.processing.RoundEnvironment;
+import javax.lang.model.element.AnnotationMirror;
+import javax.lang.model.element.Element;
+import javax.lang.model.element.Modifier;
+import javax.lang.model.element.TypeElement;
+import javax.lang.model.type.DeclaredType;
+import javax.lang.model.type.TypeKind;
+import javax.lang.model.type.TypeMirror;
+import javax.lang.model.util.ElementFilter;
+import javax.lang.model.util.Elements;
+import javax.lang.model.util.Types;
+import javax.persistence.AttributeConverter;
+import javax.persistence.Converter;
+
+/** Abstract processor to generate {@link AttributeConverter} for VKey type. */
+public abstract class AbstractVKeyProcessor extends AbstractProcessor {
+
+  private static final String CONVERTER_CLASS_NAME_TEMP = "VKeyConverter_%s";
+  // The method with same name should be defined in StringVKey and LongVKey
+  private static final String CLASS_NAME_SUFFIX_KEY = "classNameSuffix";
+
+  abstract Class<?> getSqlColumnType();
+
+  abstract String getAnnotationSimpleName();
+
+  @Override
+  public boolean process(Set<? extends TypeElement> annotations, RoundEnvironment roundEnv) {
+    annotations.forEach(
+        vKeyAnnotationType -> {
+          ElementFilter.typesIn(roundEnv.getElementsAnnotatedWith(vKeyAnnotationType))
+              .forEach(
+                  annotatedTypeElement -> {
+                    DeclaredType entityType = getDeclaredType(annotatedTypeElement);
+
+                    List<AnnotationMirror> actualAnnotation =
+                        annotatedTypeElement.getAnnotationMirrors().stream()
+                            .filter(
+                                annotationType ->
+                                    annotationType
+                                        .getAnnotationType()
+                                        .asElement()
+                                        .equals(vKeyAnnotationType))
+                            .collect(toImmutableList());
+                    checkState(
+                        actualAnnotation.size() == 1,
+                        String.format(
+                            "type can have only 1 %s annotation", getAnnotationSimpleName()));
+                    String converterClassNameSuffix =
+                        actualAnnotation.get(0).getElementValues().entrySet().stream()
+                            .filter(
+                                entry ->
+                                    entry
+                                        .getKey()
+                                        .getSimpleName()
+                                        .toString()
+                                        .equals(CLASS_NAME_SUFFIX_KEY))
+                            .map(entry -> ((String) entry.getValue().getValue()).trim())
+                            .findFirst()
+                            .orElse("");
+                    if (converterClassNameSuffix.isEmpty()) {
+                      converterClassNameSuffix =
+                          getTypeUtils().asElement(entityType).getSimpleName().toString();
+                    }
+
+                    try {
+                      createJavaFile(
+                              getPackageName(annotatedTypeElement),
+                              String.format(CONVERTER_CLASS_NAME_TEMP, converterClassNameSuffix),
+                              entityType)
+                          .writeTo(processingEnv.getFiler());
+                    } catch (IOException e) {
+                      throw new UncheckedIOException(e);
+                    }
+                  });
+        });
+    return false;
+  }
+
+  private JavaFile createJavaFile(
+      String packageName, String converterClassName, TypeMirror entityTypeMirror) {
+    TypeName entityType = ClassName.get(entityTypeMirror);
+
+    ParameterizedTypeName attributeConverter =
+        ParameterizedTypeName.get(
+            ClassName.get("google.registry.persistence.converter", "VKeyConverter"),
+            entityType,
+            ClassName.get(getSqlColumnType()));
+
+    MethodSpec getAttributeClass =
+        MethodSpec.methodBuilder("getAttributeClass")
+            .addAnnotation(Override.class)
+            .addModifiers(Modifier.PROTECTED)
+            .returns(
+                ParameterizedTypeName.get(
+                    ClassName.get(Class.class), ClassName.get(entityTypeMirror)))
+            .addStatement("return $T.class", entityType)
+            .build();
+
+    TypeSpec vKeyConverter =
+        TypeSpec.classBuilder(converterClassName)
+            .addAnnotation(
+                AnnotationSpec.builder(ClassName.get(Converter.class))
+                    .addMember("autoApply", "true")
+                    .build())
+            .addModifiers(Modifier.FINAL)
+            .superclass(attributeConverter)
+            .addMethod(getAttributeClass)
+            .build();
+
+    return JavaFile.builder(packageName, vKeyConverter).build();
+  }
+
+  private DeclaredType getDeclaredType(Element element) {
+    checkState(element.asType().getKind() == TypeKind.DECLARED, "element is not a DeclaredType");
+    return (DeclaredType) element.asType();
+  }
+
+  private String getPackageName(Element element) {
+    return getElementUtils().getPackageOf(element).getQualifiedName().toString();
+  }
+
+  private Elements getElementUtils() {
+    return processingEnv.getElementUtils();
+  }
+
+  private Types getTypeUtils() {
+    return processingEnv.getTypeUtils();
+  }
+}
@@ -0,0 +1,37 @@
+// 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.processors;
+
+import javax.annotation.processing.SupportedAnnotationTypes;
+import javax.annotation.processing.SupportedSourceVersion;
+import javax.lang.model.SourceVersion;
+
+/** Annotation processor for entity that is annotated with WithLongVKey. */
+@SupportedAnnotationTypes("google.registry.persistence.WithLongVKey")
+@SupportedSourceVersion(SourceVersion.RELEASE_8)
+public class LongVKeyProcessor extends AbstractVKeyProcessor {
+
+  private static final String ANNOTATION_SIMPLE_NAME = "WithStringVKey";
+
+  @Override
+  Class<?> getSqlColumnType() {
+    return Long.class;
+  }
+
+  @Override
+  String getAnnotationSimpleName() {
+    return ANNOTATION_SIMPLE_NAME;
+  }
+}
@@ -0,0 +1,37 @@
+// 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.processors;
+
+import javax.annotation.processing.SupportedAnnotationTypes;
+import javax.annotation.processing.SupportedSourceVersion;
+import javax.lang.model.SourceVersion;
+
+/** Annotation processor for entity that is annotated with WithStringVKey. */
+@SupportedAnnotationTypes("google.registry.persistence.WithStringVKey")
+@SupportedSourceVersion(SourceVersion.RELEASE_8)
+public class StringVKeyProcessor extends AbstractVKeyProcessor {
+
+  private static final String ANNOTATION_SIMPLE_NAME = "WithStringVKey";
+
+  @Override
+  Class<?> getSqlColumnType() {
+    return String.class;
+  }
+
+  @Override
+  String getAnnotationSimpleName() {
+    return ANNOTATION_SIMPLE_NAME;
+  }
+}
@@ -0,0 +1,2 @@
+google.registry.processors.StringVKeyProcessor
+google.registry.processors.LongVKeyProcessor
@@ -32,6 +32,7 @@ include 'core'
 include 'db'
 include 'integration'
 include 'networking'
+include 'processor'
 include 'prober'
 include 'proxy'
 include 'util'
Author	SHA1	Message	Date
Shicong HuangandGitHub	19bc1c9c9c	Add annotation processor to generate converter for VKey (#566 )	2020-04-29 17:29:05 -04:00
gbrodmanandGitHub	c361c9e601	Remove email-editing footgun (#503 ) * Remove email-editing footgun Email address is used as the primary key so we should be very careful about changing it. This will have even more importance when this is the location to which we will be sending registry lock confirmation emails. Note: we allow addition or removal of contacts through the UI (and don't want to disable that) and because all edits are performed by saving the entire list of contacts, we can't explicitly prevent all possible edits of email address in the backend. So this doesn't technically prevent anything security-wise, but it makes it much more difficult to accidentally edit an email when you shouldn't. * Enforce non-deletion of registry-lock-enabled contacts * Fix tests * Specify contact	2020-04-29 11:44:51 -04:00
Weimin YuandGitHub	aa0dcea537	Fix flaky tests due to Entity name conflicts (#569 ) * Fix flaky tests due to Entity name conflicts Objectify siliently replaces current registration of a given kind when another class is registered for this kind. There are several TestObject classes in the current code base, which by default are all mapped to the same kind. Tests have only been flaky because impacted tests need to run in specific orders for failures to happen. Using multiple executors in Gradle also reduced the likely hood of errors. To reproduce the problem run the following tests in order (e.g., by putting them in a test suite): 1. ExportCommitLogDiffActionTest 2. CreateAutoTimestampTest 3. RestoreCommitLogsActionTest In this PR, we - Made sure all entities have unique kinds. - Made all test entities register with AppEngineRule instead of directly with ObjectifyService. - Added code in AppEngineRule to check for re-registrations. - Added presumit check for forbidden direct registration.	2020-04-28 15:32:42 -04:00
sarahcaseybotandGitHub	e920e4d201	Remove Lock Dual Read and Dual Write (#568 )	2020-04-27 17:30:51 -04:00