Add check to prevent creating VKeys with incorrectly null parents (#774)

* Add check to prevent creating VKeys with incorrectly null parents

Datastore entities that are not the roots of entity groups are uniquely defined
by an inheritance chain in addition to the id/name, not just by id/name.

core/src/main/java/google/registry/persistence/VKey.java

@@ -14,10 +14,12 @@
 
 package google.registry.persistence;
 
+import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.common.base.Preconditions.checkState;
 import static google.registry.util.PreconditionsUtils.checkArgumentNotNull;
 
 import com.googlecode.objectify.Key;
+import google.registry.model.BackupGroupRoot;
 import google.registry.model.ImmutableObject;
 import google.registry.model.translators.VKeyTranslatorFactory;
 import java.io.Serializable;
@@ -75,11 +77,42 @@ public class VKey<T> extends ImmutableObject implements Serializable {
     return new VKey<T>(kind, ofyKey, sqlKey);
   }
 
-  /** Creates a symmetric {@link VKey} in which both sql and ofy keys are {@code id}. */
+  /**
+   * Creates a symmetric {@link VKey} in which both sql and ofy keys are {@code id}.
+   *
+   * <p>IMPORTANT USAGE NOTE: Datastore entities that are not roots of entity groups (i.e. those
+   * that do not have a null parent in their Objectify keys) require the full entity group
+   * inheritance chain to be specified and thus cannot use this create method. You need to use
+   * {@link #create(Class, Object, Key)} instead and pass in the full, valid parent field in the
+   * Datastore key.
+   */
   public static <T> VKey<T> create(Class<T> kind, long id) {
+    checkArgument(
+        kind.isAssignableFrom(BackupGroupRoot.class),
+        "The kind %s is not a BackupGroupRoot and thus needs its entire entity group chain"
+            + " specified in a parent",
+        kind.getCanonicalName());
     return new VKey<T>(kind, Key.create(kind, id), id);
   }
 
+  /**
+   * Creates a symmetric {@link VKey} in which both sql and ofy keys are {@code name}.
+   *
+   * <p>IMPORTANT USAGE NOTE: Datastore entities that are not roots of entity groups (i.e. those
+   * that do not have a null parent in their Objectify keys) require the full entity group
+   * inheritance chain to be specified and thus cannot use this create method. You need to use
+   * {@link #create(Class, Object, Key)} instead and pass in the full, valid parent field in the
+   * Datastore key.
+   */
+  public static <T> VKey<T> create(Class<T> kind, String name) {
+    checkArgument(
+        kind.isAssignableFrom(BackupGroupRoot.class),
+        "The kind %s is not a BackupGroupRoot and thus needs its entire entity group chain"
+            + " specified in a parent",
+        kind.getCanonicalName());
+    return new VKey<T>(kind, Key.create(kind, name), name);
+  }
+
   /** Returns the type of the entity. */
   public Class<? extends T> getKind() {
     return this.kind;

core/src/test/java/google/registry/model/contact/ContactResourceTest.java

@@ -31,7 +31,6 @@ import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
 import google.registry.model.EntityTestCase;
 import google.registry.model.ImmutableObjectSubject;
-import google.registry.model.billing.BillingEvent;
 import google.registry.model.contact.Disclose.PostalInfoChoice;
 import google.registry.model.contact.PostalInfo.Type;
 import google.registry.model.eppcommon.AuthInfo.PasswordAuth;
@@ -46,6 +45,7 @@ import org.junit.jupiter.api.Test;
 
 /** Unit tests for {@link ContactResource}. */
 public class ContactResourceTest extends EntityTestCase {
+
   private ContactResource originalContact;
   private ContactResource contactResource;
 
@@ -113,8 +113,6 @@ public class ContactResourceTest extends EntityTestCase {
                     .setGainingClientId("gaining")
                     .setLosingClientId("losing")
                     .setPendingTransferExpirationTime(fakeClock.nowUtc())
-                    .setServerApproveEntities(
-                        ImmutableSet.of(VKey.create(BillingEvent.OneTime.class, 1)))
                     .setTransferRequestTime(fakeClock.nowUtc())
                     .setTransferStatus(TransferStatus.SERVER_APPROVED)
                     .setTransferRequestTrid(Trid.create("client-trid", "server-trid"))

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

@@ -98,6 +98,11 @@ public class GracePeriodTest {
 
   @Test
   void testFailure_createForRecurring_notAutoRenew() {
+    Key<Recurring> recurringKey =
+        Key.create(
+            Key.create(Key.create(DomainBase.class, "1-TEST"), HistoryEntry.class, 343L),
+            Recurring.class,
+            12345);
     IllegalArgumentException thrown =
         assertThrows(
             IllegalArgumentException.class,
@@ -107,7 +112,7 @@ public class GracePeriodTest {
                     "1-TEST",
                     now.plusDays(1),
                     "TheRegistrar",
-                    VKey.create(Recurring.class, 12345)));
+                    VKey.create(Recurring.class, 12345, recurringKey)));
     assertThat(thrown).hasMessageThat().contains("autorenew");
   }
 }

core/src/test/java/google/registry/model/host/HostResourceTest.java

@@ -27,13 +27,11 @@ import static org.junit.jupiter.api.Assertions.assertThrows;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.net.InetAddresses;
 import google.registry.model.EntityTestCase;
-import google.registry.model.billing.BillingEvent;
 import google.registry.model.domain.DomainBase;
 import google.registry.model.eppcommon.StatusValue;
 import google.registry.model.eppcommon.Trid;
 import google.registry.model.transfer.DomainTransferData;
 import google.registry.model.transfer.TransferStatus;
-import google.registry.persistence.VKey;
 import org.joda.time.DateTime;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
@@ -62,8 +60,6 @@ class HostResourceTest extends EntityTestCase {
                         .setGainingClientId("gaining")
                         .setLosingClientId("losing")
                         .setPendingTransferExpirationTime(fakeClock.nowUtc())
-                        .setServerApproveEntities(
-                            ImmutableSet.of(VKey.create(BillingEvent.OneTime.class, 1)))
                         .setTransferRequestTime(fakeClock.nowUtc())
                         .setTransferStatus(TransferStatus.SERVER_APPROVED)
                         .setTransferRequestTrid(Trid.create("client-trid", "server-trid"))

core/src/test/java/google/registry/model/transfer/TransferDataTest.java

@@ -18,10 +18,13 @@ import static com.google.common.truth.Truth.assertThat;
 import static org.joda.time.DateTimeZone.UTC;
 
 import com.google.common.collect.ImmutableSet;
+import com.googlecode.objectify.Key;
 import google.registry.model.billing.BillingEvent;
+import google.registry.model.domain.DomainBase;
 import google.registry.model.domain.Period;
 import google.registry.model.eppcommon.Trid;
 import google.registry.model.poll.PollMessage;
+import google.registry.model.reporting.HistoryEntry;
 import google.registry.persistence.VKey;
 import google.registry.testing.AppEngineExtension;
 import org.joda.time.DateTime;
@@ -46,11 +49,33 @@ public class TransferDataTest {
 
   @BeforeEach
   void beforeEach() {
-    transferBillingEventKey = VKey.create(BillingEvent.OneTime.class, 12345);
+    Key<HistoryEntry> historyEntryKey =
-    otherServerApproveBillingEventKey = VKey.create(BillingEvent.Cancellation.class, 2468);
+        Key.create(Key.create(DomainBase.class, "4-TLD"), HistoryEntry.class, 1356L);
-    recurringBillingEventKey = VKey.create(BillingEvent.Recurring.class, 13579);
+    transferBillingEventKey =
-    autorenewPollMessageKey = VKey.create(PollMessage.Autorenew.class, 67890);
+        VKey.create(
-    otherServerApprovePollMessageKey = VKey.create(PollMessage.OneTime.class, 314159);
+            BillingEvent.OneTime.class,
+            12345,
+            Key.create(historyEntryKey, BillingEvent.OneTime.class, 12345));
+    otherServerApproveBillingEventKey =
+        VKey.create(
+            BillingEvent.Cancellation.class,
+            2468,
+            Key.create(historyEntryKey, BillingEvent.Cancellation.class, 2468));
+    recurringBillingEventKey =
+        VKey.create(
+            BillingEvent.Recurring.class,
+            13579,
+            Key.create(historyEntryKey, BillingEvent.Recurring.class, 13579));
+    autorenewPollMessageKey =
+        VKey.create(
+            PollMessage.Autorenew.class,
+            67890,
+            Key.create(historyEntryKey, PollMessage.Autorenew.class, 67890));
+    otherServerApprovePollMessageKey =
+        VKey.create(
+            PollMessage.OneTime.class,
+            314159,
+            Key.create(historyEntryKey, PollMessage.OneTime.class, 314159));
   }
 
   @Test

core/src/test/java/google/registry/persistence/VKeyTest.java

@@ -14,8 +14,12 @@
 package google.registry.persistence;
 
 import static com.google.common.truth.Truth.assertThat;
+import static com.google.common.truth.Truth8.assertThat;
+import static org.junit.jupiter.api.Assertions.assertThrows;
 
 import com.googlecode.objectify.Key;
+import google.registry.model.billing.BillingEvent.OneTime;
+import google.registry.model.registrar.RegistrarContact;
 import google.registry.testing.AppEngineExtension;
 import google.registry.testing.TestObject;
 import org.junit.jupiter.api.Test;
@@ -37,7 +41,22 @@ class VKeyTest {
         VKey.create(TestObject.class, "foo", Key.create(TestObject.create("foo")));
     assertThat(key.maybeGetSqlKey().isPresent()).isTrue();
     assertThat(key.maybeGetOfyKey().isPresent()).isTrue();
+    assertThat(VKey.createSql(TestObject.class, "foo").maybeGetSqlKey()).hasValue("foo");
+  }
 
-    assertThat(VKey.createSql(TestObject.class, "foo").maybeGetSqlKey().get()).isEqualTo("foo");
+  @Test
+  void testCreateById_failsWhenParentIsNullButShouldntBe() {
+    IllegalArgumentException thrown =
+        assertThrows(IllegalArgumentException.class, () -> VKey.create(OneTime.class, 134L));
+    assertThat(thrown).hasMessageThat().contains("BackupGroupRoot");
+  }
+
+  @Test
+  void testCreateByName_failsWhenParentIsNullButShouldntBe() {
+    IllegalArgumentException thrown =
+        assertThrows(
+            IllegalArgumentException.class,
+            () -> VKey.create(RegistrarContact.class, "fake@example.com"));
+    assertThat(thrown).hasMessageThat().contains("BackupGroupRoot");
   }
 }