Ensure that all relevant Keys can be converted to VKeys (#883)

* Ensure that all relevant Keys can be converted to VKeys

When replaying commit logs to SQL (specifically deletes) we will need to
convert Datastore Keys to SQL VKeys in order to know what (if anything)
to delete.

The test added to EntityTest (and the associated code changes) mean that
for every relevant object we'll be able to call
VKeyTranslatorFactory.createVKey(Key<?>) for all possible keys that we
care about. Note that we do not care about entities that only exist in
Datastore or entities that are non-replicated -- by their nature,
deletes for those types of objects in Datastore are not relevant in SQL.

* Responses to code review

- changing comments / method names
- using ModelUtils

core/src/main/java/google/registry/model/ModelUtils.java

@@ -87,7 +87,7 @@ public class ModelUtils {
               });
 
   /** Lists all instance fields on an object, including non-public and inherited fields. */
-  static Map<String, Field> getAllFields(Class<?> clazz) {
+  public static Map<String, Field> getAllFields(Class<?> clazz) {
     return ALL_FIELDS_CACHE.getUnchecked(clazz);
   }
 

core/src/main/java/google/registry/model/billing/BillingEvent.java

@@ -24,7 +24,6 @@ import static google.registry.util.CollectionUtils.nullToEmptyImmutableCopy;
 import static google.registry.util.DateTimeUtils.END_OF_TIME;
 import static google.registry.util.PreconditionsUtils.checkArgumentNotNull;
 
-import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Sets;
@@ -50,8 +49,7 @@ import google.registry.model.transfer.TransferData.TransferServerApproveEntity;
 import google.registry.persistence.VKey;
 import google.registry.persistence.WithLongVKey;
 import google.registry.schema.replay.DatastoreAndSqlEntity;
-import google.registry.schema.replay.DatastoreEntity;
+import google.registry.schema.replay.DatastoreOnlyEntity;
-import google.registry.schema.replay.SqlEntity;
 import java.util.Objects;
 import java.util.Optional;
 import java.util.Set;
@@ -683,7 +681,7 @@ public abstract class BillingEvent extends ImmutableObject
   @ReportedOn
   @Entity
   @WithLongVKey
-  public static class Modification extends BillingEvent implements DatastoreEntity {
+  public static class Modification extends BillingEvent implements DatastoreOnlyEntity {
 
     /** The change in cost that should be applied to the original billing event. */
     Money cost;
@@ -745,11 +743,6 @@ public abstract class BillingEvent extends ImmutableObject
           .build();
     }
 
-    @Override
-    public ImmutableList<SqlEntity> toSqlEntities() {
-      return ImmutableList.of(); // not persisted in SQL
-    }
-
     /** A builder for {@link Modification} since it is immutable. */
     public static class Builder extends BillingEvent.Builder<Modification, Builder> {
 

core/src/main/java/google/registry/model/common/Cursor.java

@@ -21,7 +21,6 @@ import static google.registry.model.ofy.ObjectifyService.ofy;
 import static google.registry.util.DateTimeUtils.START_OF_TIME;
 
 import com.google.common.base.Splitter;
-import com.google.common.collect.ImmutableList;
 import com.googlecode.objectify.Key;
 import com.googlecode.objectify.annotation.Entity;
 import com.googlecode.objectify.annotation.Id;
@@ -29,8 +28,7 @@ import com.googlecode.objectify.annotation.Parent;
 import google.registry.model.ImmutableObject;
 import google.registry.model.UpdateAutoTimestamp;
 import google.registry.model.registry.Registry;
-import google.registry.schema.replay.DatastoreEntity;
+import google.registry.schema.replay.DatastoreOnlyEntity;
-import google.registry.schema.replay.SqlEntity;
 import java.util.List;
 import org.joda.time.DateTime;
 
@@ -40,7 +38,7 @@ import org.joda.time.DateTime;
  * scoped on {@link EntityGroupRoot}.
  */
 @Entity
-public class Cursor extends ImmutableObject implements DatastoreEntity {
+public class Cursor extends ImmutableObject implements DatastoreOnlyEntity {
 
   /** The types of cursors, used as the string id field for each cursor in Datastore. */
   public enum CursorType {
@@ -137,11 +135,6 @@ public class Cursor extends ImmutableObject implements DatastoreEntity {
     return CursorType.valueOf(String.join("_", id.subList(1, id.size())));
   }
 
-  @Override
-  public ImmutableList<SqlEntity> toSqlEntities() {
-    return ImmutableList.of(); // Cursors are not converted since they are ephemeral
-  }
-
   /**
    * Checks that the type of the scoped object (or null) matches the required type for the specified
    * cursor (or null, if the cursor is a global cursor).

core/src/main/java/google/registry/model/common/EntityGroupRoot.java

@@ -14,13 +14,11 @@
 
 package google.registry.model.common;
 
-import com.google.common.collect.ImmutableList;
 import com.googlecode.objectify.Key;
 import com.googlecode.objectify.annotation.Entity;
 import com.googlecode.objectify.annotation.Id;
 import google.registry.model.BackupGroupRoot;
-import google.registry.schema.replay.DatastoreEntity;
+import google.registry.schema.replay.DatastoreOnlyEntity;
-import google.registry.schema.replay.SqlEntity;
 
 /**
  * The root key for the entity group which is known as the cross-tld entity group for historical
@@ -37,7 +35,7 @@ import google.registry.schema.replay.SqlEntity;
  * entity group for the single namespace where global data applicable for all TLDs lived.
  */
 @Entity
-public class EntityGroupRoot extends BackupGroupRoot implements DatastoreEntity {
+public class EntityGroupRoot extends BackupGroupRoot implements DatastoreOnlyEntity {
 
   @SuppressWarnings("unused")
   @Id
@@ -47,9 +45,4 @@ public class EntityGroupRoot extends BackupGroupRoot implements DatastoreEntity
   public static Key<EntityGroupRoot> getCrossTldKey() {
     return Key.create(EntityGroupRoot.class, "cross-tld");
   }
-
-  @Override
-  public ImmutableList<SqlEntity> toSqlEntities() {
-    return ImmutableList.of(); // not persisted in SQL
-  }
 }

core/src/main/java/google/registry/model/contact/ContactHistory.java

@@ -95,9 +95,9 @@ public class ContactHistory extends HistoryEntry implements SqlEntity {
   }
 
   /** Creates a {@link VKey} instance for this entity. */
+  @SuppressWarnings("unchecked")
   public VKey<ContactHistory> createVKey() {
-    return VKey.create(
+    return (VKey<ContactHistory>) createVKey(Key.create(this));
-        ContactHistory.class, new ContactHistoryId(getContactRepoId(), getId()), Key.create(this));
   }
 
   @PostLoad
@@ -116,7 +116,7 @@ public class ContactHistory extends HistoryEntry implements SqlEntity {
   }
 
   /** Class to represent the composite primary key of {@link ContactHistory} entity. */
-  static class ContactHistoryId extends ImmutableObject implements Serializable {
+  public static class ContactHistoryId extends ImmutableObject implements Serializable {
 
     private String contactRepoId;
 
@@ -125,7 +125,7 @@ public class ContactHistory extends HistoryEntry implements SqlEntity {
     /** Hibernate requires this default constructor. */
     private ContactHistoryId() {}
 
-    ContactHistoryId(String contactRepoId, long id) {
+    public ContactHistoryId(String contactRepoId, long id) {
       this.contactRepoId = contactRepoId;
       this.id = id;
     }

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

@@ -143,12 +143,8 @@ public class DomainHistory extends HistoryEntry implements SqlEntity {
   @Nullable
   @Access(AccessType.PROPERTY)
   @AttributeOverrides({
-      @AttributeOverride(
+    @AttributeOverride(name = "unit", column = @Column(name = "historyPeriodUnit")),
-          name = "unit",
+    @AttributeOverride(name = "value", column = @Column(name = "historyPeriodValue"))
-          column = @Column(name = "historyPeriodUnit")),
-      @AttributeOverride(
-          name = "value",
-          column = @Column(name = "historyPeriodValue"))
   })
   public Period getPeriod() {
     return super.getPeriod();
@@ -231,9 +227,9 @@ public class DomainHistory extends HistoryEntry implements SqlEntity {
   }
 
   /** Creates a {@link VKey} instance for this entity. */
+  @SuppressWarnings("unchecked")
   public VKey<DomainHistory> createVKey() {
-    return VKey.create(
+    return (VKey<DomainHistory>) createVKey(Key.create(this));
-        DomainHistory.class, new DomainHistoryId(getDomainRepoId(), getId()), Key.create(this));
   }
 
   @PostLoad

core/src/main/java/google/registry/model/host/HostHistory.java

@@ -96,9 +96,9 @@ public class HostHistory extends HistoryEntry implements SqlEntity {
   }
 
   /** Creates a {@link VKey} instance for this entity. */
+  @SuppressWarnings("unchecked")
   public VKey<HostHistory> createVKey() {
-    return VKey.create(
+    return (VKey<HostHistory>) createVKey(Key.create(this));
-        HostHistory.class, new HostHistoryId(getHostRepoId(), getId()), Key.create(this));
   }
 
   @PostLoad
@@ -117,7 +117,7 @@ public class HostHistory extends HistoryEntry implements SqlEntity {
   }
 
   /** Class to represent the composite primary key of {@link HostHistory} entity. */
-  static class HostHistoryId extends ImmutableObject implements Serializable {
+  public static class HostHistoryId extends ImmutableObject implements Serializable {
 
     private String hostRepoId;
 
@@ -126,7 +126,7 @@ public class HostHistory extends HistoryEntry implements SqlEntity {
     /** Hibernate requires this default constructor. */
     private HostHistoryId() {}
 
-    HostHistoryId(String hostRepoId, long id) {
+    public HostHistoryId(String hostRepoId, long id) {
       this.hostRepoId = hostRepoId;
       this.id = id;
     }

core/src/main/java/google/registry/model/index/EppResourceIndex.java

@@ -17,7 +17,6 @@ package google.registry.model.index;
 import static google.registry.util.TypeUtils.instantiate;
 
 import com.google.common.annotations.VisibleForTesting;
-import com.google.common.collect.ImmutableList;
 import com.googlecode.objectify.Key;
 import com.googlecode.objectify.annotation.Entity;
 import com.googlecode.objectify.annotation.Id;
@@ -26,25 +25,21 @@ import com.googlecode.objectify.annotation.Parent;
 import google.registry.model.BackupGroupRoot;
 import google.registry.model.EppResource;
 import google.registry.model.annotations.ReportedOn;
-import google.registry.schema.replay.DatastoreEntity;
+import google.registry.schema.replay.DatastoreOnlyEntity;
-import google.registry.schema.replay.SqlEntity;
 
 /** An index that allows for quick enumeration of all EppResource entities (e.g. via map reduce). */
 @ReportedOn
 @Entity
-public class EppResourceIndex extends BackupGroupRoot implements DatastoreEntity {
+public class EppResourceIndex extends BackupGroupRoot implements DatastoreOnlyEntity {
 
-  @Id
+  @Id String id;
-  String id;
 
-  @Parent
+  @Parent Key<EppResourceIndexBucket> bucket;
-  Key<EppResourceIndexBucket> bucket;
 
   /** Although this field holds a {@link Key} it is named "reference" for historical reasons. */
   Key<? extends EppResource> reference;
 
-  @Index
+  @Index String kind;
-  String kind;
 
   public String getId() {
     return id;
@@ -69,7 +64,7 @@ public class EppResourceIndex extends BackupGroupRoot implements DatastoreEntity
     EppResourceIndex instance = instantiate(EppResourceIndex.class);
     instance.reference = resourceKey;
     instance.kind = resourceKey.getKind();
-    instance.id = resourceKey.getString();  // creates a web-safe key string
+    instance.id = resourceKey.getString(); // creates a web-safe key string
     instance.bucket = bucket;
     return instance;
   }
@@ -77,9 +72,4 @@ public class EppResourceIndex extends BackupGroupRoot implements DatastoreEntity
   public static <T extends EppResource> EppResourceIndex create(Key<T> resourceKey) {
     return create(EppResourceIndexBucket.getBucketKey(resourceKey), resourceKey);
   }
-
-  @Override
-  public ImmutableList<SqlEntity> toSqlEntities() {
-    return ImmutableList.of(); // not relevant in SQL
-  }
 }

core/src/main/java/google/registry/model/index/EppResourceIndexBucket.java

@@ -24,23 +24,17 @@ import com.googlecode.objectify.annotation.Id;
 import google.registry.model.EppResource;
 import google.registry.model.ImmutableObject;
 import google.registry.model.annotations.VirtualEntity;
-import google.registry.schema.replay.DatastoreEntity;
+import google.registry.schema.replay.DatastoreOnlyEntity;
-import google.registry.schema.replay.SqlEntity;
 
 /** A virtual entity to represent buckets to which EppResourceIndex objects are randomly added. */
 @Entity
 @VirtualEntity
-public class EppResourceIndexBucket extends ImmutableObject implements DatastoreEntity {
+public class EppResourceIndexBucket extends ImmutableObject implements DatastoreOnlyEntity {
 
   @SuppressWarnings("unused")
   @Id
   private long bucketId;
 
-  @Override
-  public ImmutableList<SqlEntity> toSqlEntities() {
-    return ImmutableList.of(); // not relevant in SQL
-  }
-
   /**
    * Deterministic function that returns a bucket id based on the resource's roid.
    * NB: At the moment, nothing depends on this being deterministic, so we have the ability to

core/src/main/java/google/registry/model/index/ForeignKeyIndex.java

@@ -42,8 +42,7 @@ import google.registry.model.contact.ContactResource;
 import google.registry.model.domain.DomainBase;
 import google.registry.model.host.HostResource;
 import google.registry.persistence.VKey;
-import google.registry.schema.replay.DatastoreEntity;
+import google.registry.schema.replay.DatastoreOnlyEntity;
-import google.registry.schema.replay.SqlEntity;
 import google.registry.util.NonFinalForTesting;
 import java.util.Map;
 import java.util.Optional;
@@ -63,34 +62,19 @@ public abstract class ForeignKeyIndex<E extends EppResource> extends BackupGroup
   @ReportedOn
   @Entity
   public static class ForeignKeyContactIndex extends ForeignKeyIndex<ContactResource>
-      implements DatastoreEntity {
+      implements DatastoreOnlyEntity {}
-    @Override
-    public ImmutableList<SqlEntity> toSqlEntities() {
-      return ImmutableList.of(); // not relevant in SQL
-    }
-  }
 
   /** The {@link ForeignKeyIndex} type for {@link DomainBase} entities. */
   @ReportedOn
   @Entity
   public static class ForeignKeyDomainIndex extends ForeignKeyIndex<DomainBase>
-      implements DatastoreEntity {
+      implements DatastoreOnlyEntity {}
-    @Override
-    public ImmutableList<SqlEntity> toSqlEntities() {
-      return ImmutableList.of(); // not relevant in SQL
-    }
-  }
 
   /** The {@link ForeignKeyIndex} type for {@link HostResource} entities. */
   @ReportedOn
   @Entity
   public static class ForeignKeyHostIndex extends ForeignKeyIndex<HostResource>
-      implements DatastoreEntity {
+      implements DatastoreOnlyEntity {}
-    @Override
-    public ImmutableList<SqlEntity> toSqlEntities() {
-      return ImmutableList.of(); // not relevant in SQL
-    }
-  }
 
   static final ImmutableMap<Class<? extends EppResource>, Class<? extends ForeignKeyIndex<?>>>
       RESOURCE_CLASS_TO_FKI_CLASS =

core/src/main/java/google/registry/model/ofy/CommitLogBucket.java

@@ -22,7 +22,6 @@ import static google.registry.model.ofy.ObjectifyService.ofy;
 import static google.registry.util.DateTimeUtils.START_OF_TIME;
 
 import com.google.common.collect.ContiguousSet;
-import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.ImmutableSortedSet;
 import com.google.common.collect.Range;
@@ -34,8 +33,7 @@ import google.registry.model.Buildable;
 import google.registry.model.ImmutableObject;
 import google.registry.model.annotations.NotBackedUp;
 import google.registry.model.annotations.NotBackedUp.Reason;
-import google.registry.schema.replay.DatastoreEntity;
+import google.registry.schema.replay.DatastoreOnlyEntity;
-import google.registry.schema.replay.SqlEntity;
 import google.registry.util.NonFinalForTesting;
 import java.util.Random;
 import java.util.function.Supplier;
@@ -53,7 +51,7 @@ import org.joda.time.DateTime;
  */
 @Entity
 @NotBackedUp(reason = Reason.COMMIT_LOGS)
-public class CommitLogBucket extends ImmutableObject implements Buildable, DatastoreEntity {
+public class CommitLogBucket extends ImmutableObject implements Buildable, DatastoreOnlyEntity {
 
   /**
    * Ranges from 1 to {@link RegistryConfig#getCommitLogBucketCount()}, inclusive; starts at 1 since
@@ -72,11 +70,6 @@ public class CommitLogBucket extends ImmutableObject implements Buildable, Datas
     return lastWrittenTime;
   }
 
-  @Override
-  public ImmutableList<SqlEntity> toSqlEntities() {
-    return ImmutableList.of(); // not persisted in SQL
-  }
-
   /**
    * Returns the key for the specified bucket ID.
    *

core/src/main/java/google/registry/model/ofy/CommitLogCheckpoint.java

@@ -27,8 +27,7 @@ import com.googlecode.objectify.annotation.Parent;
 import google.registry.model.ImmutableObject;
 import google.registry.model.annotations.NotBackedUp;
 import google.registry.model.annotations.NotBackedUp.Reason;
-import google.registry.schema.replay.DatastoreEntity;
+import google.registry.schema.replay.DatastoreOnlyEntity;
-import google.registry.schema.replay.SqlEntity;
 import java.util.ArrayList;
 import java.util.List;
 import java.util.Objects;
@@ -46,7 +45,7 @@ import org.joda.time.DateTime;
  */
 @Entity
 @NotBackedUp(reason = Reason.COMMIT_LOGS)
-public class CommitLogCheckpoint extends ImmutableObject implements DatastoreEntity {
+public class CommitLogCheckpoint extends ImmutableObject implements DatastoreOnlyEntity {
 
   /** Shared singleton parent entity for commit log checkpoints. */
   @Parent
@@ -73,11 +72,6 @@ public class CommitLogCheckpoint extends ImmutableObject implements DatastoreEnt
     return builder.build();
   }
 
-  @Override
-  public ImmutableList<SqlEntity> toSqlEntities() {
-    return ImmutableList.of(); // not persisted in SQL
-  }
-
   /**
    * Creates a CommitLogCheckpoint for the given wall time and bucket checkpoint times, specified as
    * a map from bucket ID to bucket commit timestamp.

core/src/main/java/google/registry/model/ofy/CommitLogCheckpointRoot.java

@@ -17,21 +17,19 @@ package google.registry.model.ofy;
 import static google.registry.model.ofy.ObjectifyService.ofy;
 import static google.registry.util.DateTimeUtils.START_OF_TIME;
 
-import com.google.common.collect.ImmutableList;
 import com.googlecode.objectify.Key;
 import com.googlecode.objectify.annotation.Entity;
 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.replay.DatastoreEntity;
+import google.registry.schema.replay.DatastoreOnlyEntity;
-import google.registry.schema.replay.SqlEntity;
 import org.joda.time.DateTime;
 
 /** Singleton parent entity for all commit log checkpoints. */
 @Entity
 @NotBackedUp(reason = Reason.COMMIT_LOGS)
-public class CommitLogCheckpointRoot extends ImmutableObject implements DatastoreEntity {
+public class CommitLogCheckpointRoot extends ImmutableObject implements DatastoreOnlyEntity {
 
   public static final long SINGLETON_ID = 1;  // There is always exactly one of these.
 
@@ -50,11 +48,6 @@ public class CommitLogCheckpointRoot extends ImmutableObject implements Datastor
     return lastWrittenTime;
   }
 
-  @Override
-  public ImmutableList<SqlEntity> toSqlEntities() {
-    return ImmutableList.of(); // not persisted in SQL
-  }
-
   public static CommitLogCheckpointRoot loadRoot() {
     CommitLogCheckpointRoot root = ofy().load().key(getKey()).now();
     return root == null ? new CommitLogCheckpointRoot() : root;

core/src/main/java/google/registry/model/ofy/CommitLogManifest.java

@@ -17,7 +17,6 @@ package google.registry.model.ofy;
 import static google.registry.util.CollectionUtils.nullToEmptyImmutableCopy;
 import static org.joda.time.DateTimeZone.UTC;
 
-import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
 import com.googlecode.objectify.Key;
 import com.googlecode.objectify.annotation.Entity;
@@ -26,8 +25,7 @@ import com.googlecode.objectify.annotation.Parent;
 import google.registry.model.ImmutableObject;
 import google.registry.model.annotations.NotBackedUp;
 import google.registry.model.annotations.NotBackedUp.Reason;
-import google.registry.schema.replay.DatastoreEntity;
+import google.registry.schema.replay.DatastoreOnlyEntity;
-import google.registry.schema.replay.SqlEntity;
 import java.util.LinkedHashSet;
 import java.util.Set;
 import org.joda.time.DateTime;
@@ -41,7 +39,7 @@ import org.joda.time.DateTime;
  */
 @Entity
 @NotBackedUp(reason = Reason.COMMIT_LOGS)
-public class CommitLogManifest extends ImmutableObject implements DatastoreEntity {
+public class CommitLogManifest extends ImmutableObject implements DatastoreOnlyEntity {
 
   /** Commit log manifests are parented on a random bucket. */
   @Parent
@@ -70,11 +68,6 @@ public class CommitLogManifest extends ImmutableObject implements DatastoreEntit
     return nullToEmptyImmutableCopy(deletions);
   }
 
-  @Override
-  public ImmutableList<SqlEntity> toSqlEntities() {
-    return ImmutableList.of(); // not persisted in SQL
-  }
-
   public static CommitLogManifest create(
       Key<CommitLogBucket> parent, DateTime commitTime, Set<Key<?>> deletions) {
     CommitLogManifest instance = new CommitLogManifest();

core/src/main/java/google/registry/model/ofy/CommitLogMutation.java

@@ -21,7 +21,6 @@ import static google.registry.model.ofy.ObjectifyService.ofy;
 
 import com.google.appengine.api.datastore.KeyFactory;
 import com.google.common.annotations.VisibleForTesting;
-import com.google.common.collect.ImmutableList;
 import com.googlecode.objectify.Key;
 import com.googlecode.objectify.annotation.Entity;
 import com.googlecode.objectify.annotation.Id;
@@ -29,13 +28,12 @@ import com.googlecode.objectify.annotation.Parent;
 import google.registry.model.ImmutableObject;
 import google.registry.model.annotations.NotBackedUp;
 import google.registry.model.annotations.NotBackedUp.Reason;
-import google.registry.schema.replay.DatastoreEntity;
+import google.registry.schema.replay.DatastoreOnlyEntity;
-import google.registry.schema.replay.SqlEntity;
 
 /** Representation of a saved entity in a {@link CommitLogManifest} (not deletes). */
 @Entity
 @NotBackedUp(reason = Reason.COMMIT_LOGS)
-public class CommitLogMutation extends ImmutableObject implements DatastoreEntity {
+public class CommitLogMutation extends ImmutableObject implements DatastoreOnlyEntity {
 
   /** The manifest this belongs to. */
   @Parent
@@ -61,11 +59,6 @@ public class CommitLogMutation extends ImmutableObject implements DatastoreEntit
     return createFromPbBytes(entityProtoBytes);
   }
 
-  @Override
-  public ImmutableList<SqlEntity> toSqlEntities() {
-    return ImmutableList.of(); // not persisted in SQL
-  }
-
   /**
    * Returns a new mutation entity created from an @Entity ImmutableObject instance.
    *

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

@@ -121,33 +121,34 @@ public class Registrar extends ImmutableObject
     REAL(Objects::nonNull),
 
     /**
-     * A registrar account used by a real third-party registrar undergoing operational testing
+     * A registrar account used by a real third-party registrar undergoing operational testing and
-     * and evaluation. Should only be created in sandbox, and should have null IANA/billing IDs.
+     * evaluation. Should only be created in sandbox, and should have null IANA/billing IDs.
      */
     OTE(Objects::isNull),
 
     /**
-     * A registrar used for predelegation testing.  Should have a null billing ID.  The IANA ID
+     * A registrar used for predelegation testing. Should have a null billing ID. The IANA ID should
-     * should be either 9995 or 9996, which are reserved for predelegation testing.
+     * be either 9995 or 9996, which are reserved for predelegation testing.
      */
     PDT(n -> ImmutableSet.of(9995L, 9996L).contains(n)),
 
     /**
-     * A registrar used for external monitoring by ICANN.  Should have IANA ID 9997 and a null
+     * A registrar used for external monitoring by ICANN. Should have IANA ID 9997 and a null
      * billing ID.
      */
     EXTERNAL_MONITORING(isEqual(9997L)),
 
     /**
-     * A registrar used for when the registry acts as a registrar.  Must have either IANA ID
+     * A registrar used for when the registry acts as a registrar. Must have either IANA ID 9998
-     * 9998 (for billable transactions) or 9999 (for non-billable transactions). */
+     * (for billable transactions) or 9999 (for non-billable transactions).
+     */
     // TODO(b/13786188): determine what billing ID for this should be, if any.
     INTERNAL(n -> ImmutableSet.of(9998L, 9999L).contains(n)),
 
-    /** A registrar used for internal monitoring.  Should have null IANA/billing IDs. */
+    /** A registrar used for internal monitoring. Should have null IANA/billing IDs. */
     MONITORING(Objects::isNull),
 
-    /** A registrar used for internal testing.  Should have null IANA/billing IDs. */
+    /** A registrar used for internal testing. Should have null IANA/billing IDs. */
     TEST(Objects::isNull);
 
     /**
@@ -225,10 +226,7 @@ public class Registrar extends ImmutableObject
    */
   private static final Supplier<ImmutableMap<String, Registrar>> CACHE_BY_CLIENT_ID =
       memoizeWithShortExpiration(
-          () ->
+          () -> tm().doTransactionless(() -> Maps.uniqueIndex(loadAll(), Registrar::getClientId)));
-              tm()
-                  .doTransactionless(
-                      () -> Maps.uniqueIndex(loadAll(), Registrar::getClientId)));
 
   @Parent @Transient Key<EntityGroupRoot> parent = getCrossTldKey();
 
@@ -383,12 +381,10 @@ public class Registrar extends ImmutableObject
   @Index @Nullable Long ianaIdentifier;
 
   /** Identifier of registrar used in external billing system (e.g. Oracle). */
-  @Nullable
+  @Nullable Long billingIdentifier;
-  Long billingIdentifier;
 
   /** Purchase Order number used for invoices in external billing system, if applicable. */
-  @Nullable
+  @Nullable String poNumber;
-  String poNumber;
 
   /**
    * Map of currency-to-billing account for the registrar.
@@ -498,9 +494,7 @@ public class Registrar extends ImmutableObject
     if (billingAccountMap == null) {
       return ImmutableMap.of();
     }
-    return billingAccountMap
+    return billingAccountMap.entrySet().stream()
-        .entrySet()
-        .stream()
         .collect(toImmutableSortedMap(natural(), Map.Entry::getKey, v -> v.getValue().accountId));
   }
 
@@ -722,14 +716,18 @@ public class Registrar extends ImmutableObject
 
   /** Creates a {@link VKey} for this instance. */
   public VKey<Registrar> createVKey() {
-    return VKey.create(Registrar.class, clientIdentifier, Key.create(this));
+    return createVKey(Key.create(this));
   }
 
   /** Creates a {@link VKey} for the given {@code registrarId}. */
   public static VKey<Registrar> createVKey(String registrarId) {
     checkArgumentNotNull(registrarId, "registrarId must be specified");
-    return VKey.create(
+    return createVKey(Key.create(getCrossTldKey(), Registrar.class, registrarId));
-        Registrar.class, registrarId, Key.create(getCrossTldKey(), Registrar.class, registrarId));
+  }
+
+  /** Creates a {@link VKey} instance from a {@link Key} instance. */
+  public static VKey<Registrar> createVKey(Key<Registrar> key) {
+    return VKey.create(Registrar.class, key.getName(), key);
   }
 
   /** A builder for constructing {@link Registrar}, since it is immutable. */
@@ -744,21 +742,22 @@ public class Registrar extends ImmutableObject
       // Client id must be [3,16] chars long. See "clIDType" in the base EPP schema of RFC 5730.
       // (Need to validate this here as there's no matching EPP XSD for validation.)
       checkArgument(
-          Range.closed(3,  16).contains(clientId.length()),
+          Range.closed(3, 16).contains(clientId.length()),
           "Client identifier must be 3-16 characters long.");
       getInstance().clientIdentifier = clientId;
       return this;
     }
 
     public Builder setIanaIdentifier(@Nullable Long ianaIdentifier) {
-      checkArgument(ianaIdentifier == null || ianaIdentifier > 0,
+      checkArgument(
-          "IANA ID must be a positive number");
+          ianaIdentifier == null || ianaIdentifier > 0, "IANA ID must be a positive number");
       getInstance().ianaIdentifier = ianaIdentifier;
       return this;
     }
 
     public Builder setBillingIdentifier(@Nullable Long billingIdentifier) {
-      checkArgument(billingIdentifier == null || billingIdentifier > 0,
+      checkArgument(
+          billingIdentifier == null || billingIdentifier > 0,
           "Billing ID must be a positive number");
       getInstance().billingIdentifier = billingIdentifier;
       return this;
@@ -774,9 +773,7 @@ public class Registrar extends ImmutableObject
         getInstance().billingAccountMap = null;
       } else {
         getInstance().billingAccountMap =
-            billingAccountMap
+            billingAccountMap.entrySet().stream()
-                .entrySet()
-                .stream()
                 .collect(toImmutableMap(Map.Entry::getKey, BillingAccountEntry::new));
       }
       return this;
@@ -900,16 +897,12 @@ public class Registrar extends ImmutableObject
     }
 
     public Builder setPhoneNumber(String phoneNumber) {
-      getInstance().phoneNumber = (phoneNumber == null)
+      getInstance().phoneNumber = (phoneNumber == null) ? null : checkValidPhoneNumber(phoneNumber);
-          ? null
-          : checkValidPhoneNumber(phoneNumber);
       return this;
     }
 
     public Builder setFaxNumber(String faxNumber) {
-      getInstance().faxNumber = (faxNumber == null)
+      getInstance().faxNumber = (faxNumber == null) ? null : checkValidPhoneNumber(faxNumber);
-          ? null
-          : checkValidPhoneNumber(faxNumber);
       return this;
     }
 
@@ -944,7 +937,8 @@ public class Registrar extends ImmutableObject
     }
 
     public Builder setDriveFolderId(@Nullable String driveFolderId) {
-      checkArgument(driveFolderId == null || !driveFolderId.contains("/"),
+      checkArgument(
+          driveFolderId == null || !driveFolderId.contains("/"),
           "Drive folder ID must not be a full URL");
       getInstance().driveFolderId = driveFolderId;
       return this;
@@ -962,9 +956,10 @@ public class Registrar extends ImmutableObject
 
     /** @throws IllegalArgumentException if provided passcode is not 5-digit numeric */
     public Builder setPhonePasscode(String phonePasscode) {
-      checkArgument(phonePasscode == null
+      checkArgument(
-          || PHONE_PASSCODE_PATTERN.matcher(phonePasscode).matches(),
+          phonePasscode == null || PHONE_PASSCODE_PATTERN.matcher(phonePasscode).matches(),
-          "Not a valid telephone passcode (must be 5 digits long): %s", phonePasscode);
+          "Not a valid telephone passcode (must be 5 digits long): %s",
+          phonePasscode);
       getInstance().phonePasscode = phonePasscode;
       return this;
     }
@@ -982,9 +977,11 @@ public class Registrar extends ImmutableObject
       checkArgument(
           getInstance().localizedAddress != null || getInstance().internationalizedAddress != null,
           "Must specify at least one of localized or internationalized address");
-      checkArgument(getInstance().type.isValidIanaId(getInstance().ianaIdentifier),
+      checkArgument(
-          String.format("Supplied IANA ID is not valid for %s registrar type: %s",
+          getInstance().type.isValidIanaId(getInstance().ianaIdentifier),
-            getInstance().type, getInstance().ianaIdentifier));
+          String.format(
+              "Supplied IANA ID is not valid for %s registrar type: %s",
+              getInstance().type, getInstance().ianaIdentifier));
       return cloneEmptyToNull(super.build());
     }
   }

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

@@ -119,9 +119,7 @@ public class RegistrarContact extends ImmutableObject
   String name;
 
   /** The email address of the contact. */
-  @Id
+  @Id @javax.persistence.Id String emailAddress;
-  @javax.persistence.Id
-  String emailAddress;
 
   @Ignore @javax.persistence.Id String registrarId;
 
@@ -147,8 +145,7 @@ public class RegistrarContact extends ImmutableObject
    *
    * @see com.google.appengine.api.users.User#getUserId()
    */
-  @Index
+  @Index String gaeUserId;
-  String gaeUserId;
 
   /**
    * Whether this contact is publicly visible in WHOIS registrar query results as an Admin contact.
@@ -202,8 +199,7 @@ public class RegistrarContact extends ImmutableObject
    */
   public static void updateContacts(
       final Registrar registrar, final Set<RegistrarContact> contacts) {
-    tm()
+    tm().transact(
-        .transact(
             () -> {
               ofy()
                   .delete()
@@ -364,8 +360,15 @@ public class RegistrarContact extends ImmutableObject
   }
 
   public VKey<RegistrarContact> createVKey() {
-    return VKey.create(
+    return createVKey(Key.create(this));
-        RegistrarContact.class, new RegistrarPocId(emailAddress, registrarId), Key.create(this));
+  }
+
+  /** Creates a {@link VKey} instance from a {@link Key} instance. */
+  public static VKey<RegistrarContact> createVKey(Key<RegistrarContact> key) {
+    Key<Registrar> parent = key.getParent();
+    String registrarId = parent.getName();
+    String emailAddress = key.getName();
+    return VKey.create(RegistrarContact.class, new RegistrarPocId(emailAddress, registrarId), key);
   }
 
   /** Class to represent the composite primary key for {@link RegistrarContact} entity. */

core/src/main/java/google/registry/model/registry/label/PremiumList.java

@@ -32,7 +32,6 @@ import com.google.common.cache.CacheBuilder;
 import com.google.common.cache.CacheLoader;
 import com.google.common.cache.CacheLoader.InvalidCacheLoadException;
 import com.google.common.cache.LoadingCache;
-import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.hash.BloomFilter;
 import com.google.common.util.concurrent.UncheckedExecutionException;
@@ -45,9 +44,8 @@ import google.registry.model.Buildable;
 import google.registry.model.ImmutableObject;
 import google.registry.model.annotations.ReportedOn;
 import google.registry.model.registry.Registry;
-import google.registry.schema.replay.DatastoreEntity;
+import google.registry.schema.replay.DatastoreOnlyEntity;
 import google.registry.schema.replay.NonReplicatedEntity;
-import google.registry.schema.replay.SqlEntity;
 import google.registry.schema.tld.PremiumListDao;
 import google.registry.util.NonFinalForTesting;
 import java.io.ByteArrayOutputStream;
@@ -114,7 +112,7 @@ public final class PremiumList extends BaseDomainLabelList<Money, PremiumList.Pr
   /** Virtual parent entity for premium list entry entities associated with a single revision. */
   @ReportedOn
   @Entity
-  public static class PremiumListRevision extends ImmutableObject implements DatastoreEntity {
+  public static class PremiumListRevision extends ImmutableObject implements DatastoreOnlyEntity {
 
     @Parent Key<PremiumList> parent;
 
@@ -171,11 +169,6 @@ public final class PremiumList extends BaseDomainLabelList<Money, PremiumList.Pr
       }
       return revision;
     }
-
-    @Override
-    public ImmutableList<SqlEntity> toSqlEntities() {
-      return ImmutableList.of(); // not persisted in SQL
-    }
   }
 
   /**
@@ -332,7 +325,7 @@ public final class PremiumList extends BaseDomainLabelList<Money, PremiumList.Pr
   @ReportedOn
   @Entity
   public static class PremiumListEntry extends DomainLabelEntry<Money, PremiumListEntry>
-      implements Buildable, DatastoreEntity {
+      implements Buildable, DatastoreOnlyEntity {
 
     @Parent
     Key<PremiumListRevision> parent;
@@ -349,11 +342,6 @@ public final class PremiumList extends BaseDomainLabelList<Money, PremiumList.Pr
       return new Builder(clone(this));
     }
 
-    @Override
-    public ImmutableList<SqlEntity> toSqlEntities() {
-      return ImmutableList.of();
-    }
-
     /** A builder for constructing {@link PremiumListEntry} objects, since they are immutable. */
     public static class Builder extends DomainLabelEntry.Builder<PremiumListEntry, Builder> {
 

core/src/main/java/google/registry/model/reporting/HistoryEntry.java

@@ -32,13 +32,17 @@ import google.registry.model.EppResource;
 import google.registry.model.ImmutableObject;
 import google.registry.model.annotations.ReportedOn;
 import google.registry.model.contact.ContactHistory;
+import google.registry.model.contact.ContactHistory.ContactHistoryId;
 import google.registry.model.contact.ContactResource;
 import google.registry.model.domain.DomainBase;
 import google.registry.model.domain.DomainHistory;
+import google.registry.model.domain.DomainHistory.DomainHistoryId;
 import google.registry.model.domain.Period;
 import google.registry.model.eppcommon.Trid;
 import google.registry.model.host.HostHistory;
+import google.registry.model.host.HostHistory.HostHistoryId;
 import google.registry.model.host.HostResource;
+import google.registry.persistence.VKey;
 import google.registry.schema.replay.DatastoreEntity;
 import google.registry.schema.replay.SqlEntity;
 import java.util.Set;
@@ -314,6 +318,33 @@ public class HistoryEntry extends ImmutableObject implements Buildable, Datastor
     return ImmutableList.of((SqlEntity) toChildHistoryEntity());
   }
 
+  /** Creates a {@link VKey} instance from a {@link Key} instance. */
+  public static VKey<? extends HistoryEntry> createVKey(Key<HistoryEntry> key) {
+    String repoId = key.getParent().getName();
+    long id = key.getId();
+    Key<EppResource> parent = key.getParent();
+    String parentKind = parent.getKind();
+    if (parentKind.equals(getKind(DomainBase.class))) {
+      return VKey.create(
+          DomainHistory.class,
+          new DomainHistoryId(repoId, id),
+          Key.create(parent, DomainHistory.class, id));
+    } else if (parentKind.equals(getKind(HostResource.class))) {
+      return VKey.create(
+          HostHistory.class,
+          new HostHistoryId(repoId, id),
+          Key.create(parent, HostHistory.class, id));
+    } else if (parentKind.equals(getKind(ContactResource.class))) {
+      return VKey.create(
+          ContactHistory.class,
+          new ContactHistoryId(repoId, id),
+          Key.create(parent, ContactHistory.class, id));
+    } else {
+      throw new IllegalStateException(
+          String.format("Unknown kind of HistoryEntry parent %s", parentKind));
+    }
+  }
+
   /** A builder for {@link HistoryEntry} since it is immutable */
   public static class Builder<T extends HistoryEntry, B extends Builder<?, ?>>
       extends GenericBuilder<T, B> {

core/src/main/java/google/registry/model/server/KmsSecret.java

@@ -16,7 +16,6 @@ package google.registry.model.server;
 
 import static google.registry.model.common.EntityGroupRoot.getCrossTldKey;
 
-import com.google.common.collect.ImmutableList;
 import com.googlecode.objectify.Key;
 import com.googlecode.objectify.annotation.Entity;
 import com.googlecode.objectify.annotation.Id;
@@ -24,13 +23,12 @@ import com.googlecode.objectify.annotation.Parent;
 import google.registry.model.ImmutableObject;
 import google.registry.model.annotations.ReportedOn;
 import google.registry.model.common.EntityGroupRoot;
-import google.registry.schema.replay.DatastoreEntity;
+import google.registry.schema.replay.DatastoreOnlyEntity;
-import google.registry.schema.replay.SqlEntity;
 
 /** Pointer to the latest {@link KmsSecretRevision}. */
 @Entity
 @ReportedOn
-public class KmsSecret extends ImmutableObject implements DatastoreEntity {
+public class KmsSecret extends ImmutableObject implements DatastoreOnlyEntity {
 
   /** The unique name of this {@link KmsSecret}. */
   @Id String name;
@@ -48,11 +46,6 @@ public class KmsSecret extends ImmutableObject implements DatastoreEntity {
     return latestRevision;
   }
 
-  @Override
-  public ImmutableList<SqlEntity> toSqlEntities() {
-    return ImmutableList.of(); // not persisted in SQL
-  }
-
   public static KmsSecret create(String name, KmsSecretRevision latestRevision) {
     KmsSecret instance = new KmsSecret();
     instance.name = name;

core/src/main/java/google/registry/model/server/Lock.java

@@ -22,15 +22,13 @@ import static google.registry.util.DateTimeUtils.isAtOrAfter;
 import com.google.auto.value.AutoValue;
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Strings;
-import com.google.common.collect.ImmutableList;
 import com.google.common.flogger.FluentLogger;
 import com.googlecode.objectify.annotation.Entity;
 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.replay.DatastoreEntity;
+import google.registry.schema.replay.DatastoreOnlyEntity;
-import google.registry.schema.replay.SqlEntity;
 import google.registry.util.RequestStatusChecker;
 import google.registry.util.RequestStatusCheckerImpl;
 import java.io.Serializable;
@@ -50,7 +48,7 @@ import org.joda.time.Duration;
  */
 @Entity
 @NotBackedUp(reason = Reason.TRANSIENT)
-public class Lock extends ImmutableObject implements DatastoreEntity, Serializable {
+public class Lock extends ImmutableObject implements DatastoreOnlyEntity, Serializable {
 
   private static final long serialVersionUID = 756397280691684645L;
   private static final FluentLogger logger = FluentLogger.forEnclosingClass();
@@ -259,9 +257,4 @@ public class Lock extends ImmutableObject implements DatastoreEntity, Serializab
               }
             });
   }
-
-  @Override
-  public ImmutableList<SqlEntity> toSqlEntities() {
-    return ImmutableList.of(); // Locks are not converted since they are ephemeral
-  }
 }

core/src/main/java/google/registry/model/server/ServerSecret.java

@@ -23,7 +23,6 @@ import com.google.common.annotations.VisibleForTesting;
 import com.google.common.cache.CacheBuilder;
 import com.google.common.cache.CacheLoader;
 import com.google.common.cache.LoadingCache;
-import com.google.common.collect.ImmutableList;
 import com.google.common.primitives.Longs;
 import com.googlecode.objectify.Key;
 import com.googlecode.objectify.annotation.Entity;
@@ -34,8 +33,7 @@ import google.registry.model.annotations.NotBackedUp;
 import google.registry.model.annotations.NotBackedUp.Reason;
 import google.registry.model.common.CrossTldSingleton;
 import google.registry.persistence.VKey;
-import google.registry.schema.replay.DatastoreEntity;
+import google.registry.schema.replay.NonReplicatedEntity;
-import google.registry.schema.replay.SqlEntity;
 import java.nio.ByteBuffer;
 import java.util.UUID;
 import java.util.concurrent.ExecutionException;
@@ -50,7 +48,7 @@ import javax.persistence.Transient;
 @Unindex
 @NotBackedUp(reason = Reason.AUTO_GENERATED)
 // TODO(b/27427316): Replace this with an entry in KMSKeyring
-public class ServerSecret extends CrossTldSingleton implements DatastoreEntity, SqlEntity {
+public class ServerSecret extends CrossTldSingleton implements NonReplicatedEntity {
 
   /**
    * Cache of the singleton ServerSecret instance that creates it if not present.
@@ -139,16 +137,6 @@ public class ServerSecret extends CrossTldSingleton implements DatastoreEntity,
         .array();
   }
 
-  @Override
-  public ImmutableList<SqlEntity> toSqlEntities() {
-    return ImmutableList.of(); // dually-written
-  }
-
-  @Override
-  public ImmutableList<DatastoreEntity> toDatastoreEntities() {
-    return ImmutableList.of(); // dually-written
-  }
-
   @VisibleForTesting
   static void resetCache() {
     CACHE.invalidateAll();

core/src/main/java/google/registry/model/tmch/ClaimsListShard.java

@@ -26,7 +26,6 @@ import static google.registry.util.DateTimeUtils.START_OF_TIME;
 
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Supplier;
-import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.MapDifference;
 import com.google.common.collect.MapDifference.ValueDifference;
@@ -46,9 +45,8 @@ import google.registry.model.annotations.NotBackedUp;
 import google.registry.model.annotations.NotBackedUp.Reason;
 import google.registry.model.annotations.VirtualEntity;
 import google.registry.model.common.CrossTldSingleton;
-import google.registry.schema.replay.DatastoreAndSqlEntity;
+import google.registry.schema.replay.DatastoreOnlyEntity;
-import google.registry.schema.replay.DatastoreEntity;
+import google.registry.schema.replay.NonReplicatedEntity;
-import google.registry.schema.replay.SqlEntity;
 import google.registry.util.CollectionUtils;
 import google.registry.util.Concurrent;
 import google.registry.util.Retrier;
@@ -97,7 +95,7 @@ import org.joda.time.DateTime;
 @NotBackedUp(reason = Reason.EXTERNALLY_SOURCED)
 @javax.persistence.Entity(name = "ClaimsList")
 @Table
-public class ClaimsListShard extends ImmutableObject implements DatastoreAndSqlEntity {
+public class ClaimsListShard extends ImmutableObject implements NonReplicatedEntity {
 
   private static final FluentLogger logger = FluentLogger.forEnclosingClass();
 
@@ -146,63 +144,62 @@ public class ClaimsListShard extends ImmutableObject implements DatastoreAndSqlE
   private static final Retrier LOADER_RETRIER = new Retrier(new SystemSleeper(), 2);
 
   private static ClaimsListShard loadClaimsListShard() {
-        // Find the most recent revision.
+    // Find the most recent revision.
-        Key<ClaimsListRevision> revisionKey = getCurrentRevision();
+    Key<ClaimsListRevision> revisionKey = getCurrentRevision();
 
-        Map<String, String> combinedLabelsToKeys = new HashMap<>();
+    Map<String, String> combinedLabelsToKeys = new HashMap<>();
-        DateTime creationTime = START_OF_TIME;
+    DateTime creationTime = START_OF_TIME;
-        if (revisionKey != null) {
+    if (revisionKey != null) {
-          // Grab all of the keys for the shards that belong to the current revision.
+      // Grab all of the keys for the shards that belong to the current revision.
-          final List<Key<ClaimsListShard>> shardKeys =
+      final List<Key<ClaimsListShard>> shardKeys =
-              ofy().load().type(ClaimsListShard.class).ancestor(revisionKey).keys().list();
+          ofy().load().type(ClaimsListShard.class).ancestor(revisionKey).keys().list();
 
-          List<ClaimsListShard> shards;
+      List<ClaimsListShard> shards;
-          try {
+      try {
-            // Load all of the shards concurrently, each in a separate transaction.
+        // Load all of the shards concurrently, each in a separate transaction.
-            shards =
+        shards =
-                Concurrent.transform(
+            Concurrent.transform(
-                    shardKeys,
+                shardKeys,
-                    key ->
+                key ->
-                        tm().transactNewReadOnly(
+                    tm().transactNewReadOnly(
-                                () -> {
+                            () -> {
-                                  ClaimsListShard claimsListShard = ofy().load().key(key).now();
+                              ClaimsListShard claimsListShard = ofy().load().key(key).now();
-                                  checkState(
+                              checkState(
-                                      claimsListShard != null,
+                                  claimsListShard != null,
-                                      "Key not found when loading claims list shards.");
+                                  "Key not found when loading claims list shards.");
-                                  return claimsListShard;
+                              return claimsListShard;
-                                }));
+                            }));
-          } catch (UncheckedExecutionException e) {
+      } catch (UncheckedExecutionException e) {
-            // We retry on IllegalStateException. However, there's a checkState inside the
+        // We retry on IllegalStateException. However, there's a checkState inside the
-            // Concurrent.transform, so if it's thrown it'll be wrapped in an
+        // Concurrent.transform, so if it's thrown it'll be wrapped in an
-            // UncheckedExecutionException. We want to unwrap it so it's caught by the retrier.
+        // UncheckedExecutionException. We want to unwrap it so it's caught by the retrier.
-            if (e.getCause() != null) {
+        if (e.getCause() != null) {
-              throwIfUnchecked(e.getCause());
+          throwIfUnchecked(e.getCause());
-            }
-            throw e;
-          }
-
-          // Combine the shards together and return the concatenated ClaimsList.
-          if (!shards.isEmpty()) {
-            creationTime = shards.get(0).creationTime;
-            for (ClaimsListShard shard : shards) {
-              combinedLabelsToKeys.putAll(shard.labelsToKeys);
-              checkState(
-                  creationTime.equals(shard.creationTime),
-                  "Inconsistent claims list shard creation times.");
-            }
-          }
         }
+        throw e;
+      }
 
-        ClaimsListShard datastoreList =
+      // Combine the shards together and return the concatenated ClaimsList.
-            create(creationTime, ImmutableMap.copyOf(combinedLabelsToKeys));
+      if (!shards.isEmpty()) {
-        // Also load the list from Cloud SQL, compare the two lists, and log if different.
+        creationTime = shards.get(0).creationTime;
-        try {
+        for (ClaimsListShard shard : shards) {
-          loadAndCompareCloudSqlList(datastoreList);
+          combinedLabelsToKeys.putAll(shard.labelsToKeys);
-        } catch (Throwable t) {
+          checkState(
-          logger.atSevere().withCause(t).log("Error comparing claims lists.");
+              creationTime.equals(shard.creationTime),
+              "Inconsistent claims list shard creation times.");
         }
-        return datastoreList;
+      }
-      };
+    }
+
+    ClaimsListShard datastoreList = create(creationTime, ImmutableMap.copyOf(combinedLabelsToKeys));
+    // Also load the list from Cloud SQL, compare the two lists, and log if different.
+    try {
+      loadAndCompareCloudSqlList(datastoreList);
+    } catch (Throwable t) {
+      logger.atSevere().withCause(t).log("Error comparing claims lists.");
+    }
+    return datastoreList;
+  };
 
   private static void loadAndCompareCloudSqlList(ClaimsListShard datastoreList) {
     Optional<ClaimsListShard> maybeCloudSqlList = ClaimsListDao.getLatestRevision();
@@ -304,8 +301,7 @@ public class ClaimsListShard extends ImmutableObject implements DatastoreAndSqlE
     Concurrent.transform(
         CollectionUtils.partitionMap(labelsToKeys, shardSize),
         (final ImmutableMap<String, String> labelsToKeysShard) ->
-            tm()
+            tm().transactNew(
-                .transactNew(
                     () -> {
                       ClaimsListShard shard = create(creationTime, labelsToKeysShard);
                       shard.isShard = true;
@@ -315,8 +311,7 @@ public class ClaimsListShard extends ImmutableObject implements DatastoreAndSqlE
                     }));
 
     // Persist the new revision, thus causing the newly created shards to go live.
-    tm()
+    tm().transactNew(
-        .transactNew(
             () -> {
               verify(
                   (getCurrentRevision() == null && oldRevision == null)
@@ -358,12 +353,10 @@ public class ClaimsListShard extends ImmutableObject implements DatastoreAndSqlE
   /** Virtual parent entity for claims list shards of a specific revision. */
   @Entity
   @VirtualEntity
-  public static class ClaimsListRevision extends ImmutableObject implements DatastoreEntity {
+  public static class ClaimsListRevision extends ImmutableObject implements DatastoreOnlyEntity {
-    @Parent
+    @Parent Key<ClaimsListSingleton> parent;
-    Key<ClaimsListSingleton> parent;
 
-    @Id
+    @Id long versionId;
-    long versionId;
 
     @VisibleForTesting
     public static Key<ClaimsListRevision> createKey(ClaimsListSingleton singleton) {
@@ -377,11 +370,6 @@ public class ClaimsListShard extends ImmutableObject implements DatastoreAndSqlE
     public static Key<ClaimsListRevision> createKey() {
       return createKey(new ClaimsListSingleton());
     }
-
-    @Override
-    public ImmutableList<SqlEntity> toSqlEntities() {
-      return ImmutableList.of(); // ClaimsLists are dually written
-    }
   }
 
   /**
@@ -390,7 +378,7 @@ public class ClaimsListShard extends ImmutableObject implements DatastoreAndSqlE
    */
   @Entity
   @NotBackedUp(reason = Reason.EXTERNALLY_SOURCED)
-  public static class ClaimsListSingleton extends CrossTldSingleton implements DatastoreEntity {
+  public static class ClaimsListSingleton extends CrossTldSingleton implements DatastoreOnlyEntity {
     Key<ClaimsListRevision> activeRevision;
 
     static ClaimsListSingleton create(Key<ClaimsListRevision> revision) {
@@ -403,11 +391,6 @@ public class ClaimsListShard extends ImmutableObject implements DatastoreAndSqlE
     public void setActiveRevision(Key<ClaimsListRevision> revision) {
       activeRevision = revision;
     }
-
-    @Override
-    public ImmutableList<SqlEntity> toSqlEntities() {
-      return ImmutableList.of(); // ClaimsLists are dually written
-    }
   }
 
   /**

core/src/main/java/google/registry/model/tmch/TmchCrl.java

@@ -29,6 +29,7 @@ import google.registry.model.common.CrossTldSingleton;
 import google.registry.model.tmch.TmchCrl.TmchCrlId;
 import google.registry.persistence.VKey;
 import google.registry.schema.replay.DatastoreEntity;
+import google.registry.schema.replay.NonReplicatedEntity;
 import google.registry.schema.replay.SqlEntity;
 import java.io.Serializable;
 import java.util.Optional;
@@ -44,7 +45,7 @@ import org.joda.time.DateTime;
 @Immutable
 @NotBackedUp(reason = Reason.EXTERNALLY_SOURCED)
 @IdClass(TmchCrlId.class)
-public final class TmchCrl extends CrossTldSingleton implements DatastoreEntity, SqlEntity {
+public final class TmchCrl extends CrossTldSingleton implements NonReplicatedEntity {
 
   @Id String crl;
 

core/src/main/java/google/registry/schema/replay/DatastoreOnlyEntity.java

@@ -0,0 +1,24 @@
+// 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.replay;
+
+import com.google.common.collect.ImmutableList;
+
+public interface DatastoreOnlyEntity extends DatastoreEntity {
+  @Override
+  default ImmutableList<SqlEntity> toSqlEntities() {
+    return ImmutableList.of();
+  }
+}

core/src/test/java/google/registry/beam/initsql/WriteToSqlTest.java

@@ -29,8 +29,8 @@ import google.registry.model.registrar.Registrar;
 import google.registry.persistence.transaction.JpaTestRules;
 import google.registry.persistence.transaction.JpaTestRules.JpaIntegrationTestExtension;
 import google.registry.testing.AppEngineExtension;
-import google.registry.testing.DatastoreEntityExtension;
 import google.registry.testing.DatabaseHelper;
+import google.registry.testing.DatastoreEntityExtension;
 import google.registry.testing.FakeClock;
 import google.registry.testing.InjectExtension;
 import java.io.Serializable;

core/src/test/java/google/registry/schema/replay/EntityTest.java

@@ -16,10 +16,16 @@ package google.registry.schema.replay;
 
 import static com.google.common.collect.ImmutableSet.toImmutableSet;
 import static com.google.common.truth.Truth.assertThat;
+import static com.google.common.truth.Truth.assertWithMessage;
 
 import com.google.common.collect.ImmutableSet;
+import com.googlecode.objectify.Key;
 import com.googlecode.objectify.annotation.Embed;
+import com.googlecode.objectify.annotation.Parent;
+import google.registry.model.ModelUtils;
 import google.registry.model.common.GaeUserIdConverter;
+import google.registry.persistence.VKey;
+import google.registry.testing.DatastoreEntityExtension;
 import io.github.classgraph.ClassGraph;
 import io.github.classgraph.ClassInfo;
 import io.github.classgraph.ClassInfoList;
@@ -28,13 +34,18 @@ import java.lang.annotation.ElementType;
 import java.lang.annotation.Retention;
 import java.lang.annotation.RetentionPolicy;
 import java.lang.annotation.Target;
+import java.lang.reflect.Method;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.RegisterExtension;
 
 /**
  * Test to verify classes implement {@link SqlEntity} and {@link DatastoreEntity} when they should.
  */
 public class EntityTest {
 
+  @RegisterExtension
+  final DatastoreEntityExtension datastoreEntityExtension = new DatastoreEntityExtension();
+
   private static final ImmutableSet<Class<?>> NON_CONVERTED_CLASSES =
       ImmutableSet.of(GaeUserIdConverter.class);
 
@@ -59,6 +70,47 @@ public class EntityTest {
     }
   }
 
+  @Test
+  void testDatastoreEntityVKeyCreation() {
+    // For replication, we need to be able to convert from Key -> VKey for the relevant classes.
+    // This means that the relevant classes must have non-composite Objectify keys or must have a
+    // createVKey method
+    try (ScanResult scanResult =
+        new ClassGraph().enableAnnotationInfo().whitelistPackages("google.registry").scan()) {
+      ImmutableSet<Class<?>> datastoreEntityClasses =
+          getClasses(scanResult.getClassesImplementing(DatastoreEntity.class.getName()));
+      // some classes aren't converted so they aren't relevant
+      ImmutableSet<Class<?>> vkeyConversionNecessaryClasses =
+          datastoreEntityClasses.stream()
+              .filter(clazz -> !DatastoreOnlyEntity.class.isAssignableFrom(clazz))
+              .filter(clazz -> !NonReplicatedEntity.class.isAssignableFrom(clazz))
+              .collect(toImmutableSet());
+
+      ImmutableSet.Builder<Class<?>> failedClasses = new ImmutableSet.Builder<>();
+      for (Class<?> clazz : vkeyConversionNecessaryClasses) {
+        if (hasKeyWithParent(clazz)) {
+          try {
+            Method createVKeyMethod = clazz.getMethod("createVKey", Key.class);
+            if (!createVKeyMethod.getReturnType().equals(VKey.class)) {
+              failedClasses.add(clazz);
+            }
+          } catch (NoSuchMethodException e) {
+            failedClasses.add(clazz);
+          }
+        }
+      }
+      assertWithMessage(
+              "Some DatastoreEntity classes with parents were missing createVKey methods: ")
+          .that(failedClasses.build())
+          .isEmpty();
+    }
+  }
+
+  private boolean hasKeyWithParent(Class<?> clazz) {
+    return ModelUtils.getAllFields(clazz).values().stream()
+        .anyMatch(field -> field.getAnnotation(Parent.class) != null);
+  }
+
   private ImmutableSet<String> getAllClassesWithAnnotation(
       ScanResult scanResult, String annotation) {
     ImmutableSet.Builder<String> result = new ImmutableSet.Builder<>();
@@ -70,17 +122,20 @@ public class EntityTest {
     return result.build();
   }
 
-  private ImmutableSet<String> getClassNames(ClassInfoList classInfoList) {
+  private ImmutableSet<Class<?>> getClasses(ClassInfoList classInfoList) {
     return classInfoList.stream()
         .filter(ClassInfo::isStandardClass)
         .map(ClassInfo::loadClass)
         .filter(clazz -> !clazz.isAnnotationPresent(EntityForTesting.class))
         .filter(clazz -> !clazz.isAnnotationPresent(Embed.class))
         .filter(clazz -> !NON_CONVERTED_CLASSES.contains(clazz))
-        .map(Class::getName)
         .collect(toImmutableSet());
   }
 
+  private ImmutableSet<String> getClassNames(ClassInfoList classInfoList) {
+    return getClasses(classInfoList).stream().map(Class::getName).collect(toImmutableSet());
+  }
+
   /** Entities that are solely used for testing, to avoid scanning them in {@link EntityTest}. */
   @Target(ElementType.TYPE)
   @Retention(RetentionPolicy.RUNTIME)