Add some missing @Nullables and types (#753)

* Add some missing @Nullables and types

Also deletes two unused VKey.createOfy() methods that simply don't work, because
a kind and an id is not enough to create a Datastore key; you also need the full
entity group inheritance chain for entities that are not roots themselves
(which is most of the entities in our schema).

* Merge branch 'master' into add-missing-nullables

* Throw UnsupportedOperationException for contacts/hosts too

* Merge branch 'master' into add-missing-nullables

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

@@ -347,7 +347,7 @@ public abstract class BillingEvent extends ImmutableObject
 
     @Override
     public VKey<OneTime> createVKey() {
-      return VKey.create(getClass(), getId(), Key.create(this));
+      return VKey.create(OneTime.class, getId(), Key.create(this));
     }
 
     public static VKey<OneTime> createVKey(Key<OneTime> key) {
@@ -489,7 +489,7 @@ public abstract class BillingEvent extends ImmutableObject
 
     @Override
     public VKey<Recurring> createVKey() {
-      return VKey.create(getClass(), getId(), Key.create(this));
+      return VKey.create(Recurring.class, getId(), Key.create(this));
     }
 
     public static VKey<Recurring> createVKey(Key<Recurring> key) {
@@ -613,7 +613,7 @@ public abstract class BillingEvent extends ImmutableObject
 
     @Override
     public VKey<Cancellation> createVKey() {
-      return VKey.create(getClass(), getId(), Key.create(this));
+      return VKey.create(Cancellation.class, getId(), Key.create(this));
     }
 
     public static VKey<Cancellation> createVKey(Key<Cancellation> key) {
@@ -697,7 +697,7 @@ public abstract class BillingEvent extends ImmutableObject
 
     @Override
     public VKey<Modification> createVKey() {
-      return VKey.create(getClass(), getId(), Key.create(this));
+      return VKey.create(Modification.class, getId(), Key.create(this));
     }
 
     public static VKey<Modification> createVKey(Key<Modification> key) {

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

@@ -19,7 +19,6 @@ import static com.google.common.collect.ImmutableList.toImmutableList;
 import static google.registry.model.EppResourceUtils.projectResourceOntoBuilderAtTime;
 
 import com.google.common.collect.ImmutableList;
-import com.googlecode.objectify.Key;
 import com.googlecode.objectify.annotation.IgnoreSave;
 import com.googlecode.objectify.annotation.Index;
 import com.googlecode.objectify.condition.IfNull;
@@ -185,8 +184,9 @@ public class ContactBase extends EppResource implements ResourceWithTransferData
 
   @Override
   public VKey<? extends ContactBase> createVKey() {
-    // TODO(mmuller): create symmetric keys if we can ever reload both sides.
+    throw new UnsupportedOperationException(
-    return VKey.create(ContactBase.class, getRepoId(), Key.create(this));
+        "ContactBase is not an actual persisted entity you can create a key to;"
+            + " use ContactResource instead");
   }
 
   public String getContactId() {

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

@@ -81,7 +81,8 @@ public class ContactHistory extends HistoryEntry {
     @Override
     public Builder setParent(Key<? extends EppResource> parent) {
       super.setParent(parent);
-      getInstance().contactRepoId = VKey.create(ContactResource.class, parent.getName(), parent);
+      getInstance().contactRepoId =
+          VKey.create(ContactResource.class, parent.getName(), (Key<ContactResource>) parent);
       return this;
     }
   }

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

@@ -100,7 +100,7 @@ public class DomainBase extends DomainContent
     return cloneDomainProjectedAtTime(this, now);
   }
 
-  public static VKey<DomainBase> createVKey(Key key) {
+  public static VKey<DomainBase> createVKey(Key<DomainBase> key) {
     return VKey.create(DomainBase.class, key.getName(), key);
   }
 

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

@@ -40,7 +40,6 @@ import com.google.common.collect.ImmutableSortedSet;
 import com.google.common.collect.Ordering;
 import com.google.common.collect.Sets;
 import com.google.common.collect.Streams;
-import com.googlecode.objectify.Key;
 import com.googlecode.objectify.annotation.Ignore;
 import com.googlecode.objectify.annotation.IgnoreSave;
 import com.googlecode.objectify.annotation.Index;
@@ -419,7 +418,7 @@ public class DomainContent extends EppResource
    * parallels the logic in {@code DomainTransferApproveFlow} which handles explicit client
    * approvals.
    */
-  protected static <T extends DomainContent> T cloneDomainProjectedAtTime(T domain, DateTime now) {
+  static <T extends DomainContent> T cloneDomainProjectedAtTime(T domain, DateTime now) {
     DomainTransferData transferData = domain.getTransferData();
     DateTime transferExpirationTime = transferData.getPendingTransferExpirationTime();
 
@@ -608,7 +607,7 @@ public class DomainContent extends EppResource
    * <p>The registrant field is only set if {@code includeRegistrant} is true, as this field needs
    * to be set in some circumstances but not in others.
    */
-  protected void setContactFields(Set<DesignatedContact> contacts, boolean includeRegistrant) {
+  void setContactFields(Set<DesignatedContact> contacts, boolean includeRegistrant) {
     // Set the individual contact fields.
     for (DesignatedContact contact : contacts) {
       switch (contact.getType()) {
@@ -634,15 +633,13 @@ public class DomainContent extends EppResource
 
   @Override
   public VKey<DomainBase> createVKey() {
-    return VKey.create(DomainBase.class, getRepoId(), Key.create(this));
+    throw new UnsupportedOperationException(
-  }
+        "DomainContent is not an actual persisted entity you can create a key to;"
-
+            + " use DomainBase instead");
-  public static VKey<DomainBase> createVKey(Key key) {
-    return VKey.create(DomainBase.class, key.getName(), key);
   }
 
   /** Predicate to determine if a given {@link DesignatedContact} is the registrant. */
-  protected static final Predicate<DesignatedContact> IS_REGISTRANT =
+  static final Predicate<DesignatedContact> IS_REGISTRANT =
       (DesignatedContact contact) -> DesignatedContact.Type.REGISTRANT.equals(contact.type);
 
   /** An override of {@link EppResource#asBuilder} with tighter typing. */

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

@@ -104,7 +104,8 @@ public class DomainHistory extends HistoryEntry {
     @Override
     public Builder setParent(Key<? extends EppResource> parent) {
       super.setParent(parent);
-      getInstance().domainRepoId = VKey.create(DomainBase.class, parent.getName(), parent);
+      getInstance().domainRepoId =
+          VKey.create(DomainBase.class, parent.getName(), (Key<DomainBase>) parent);
       return this;
     }
   }

core/src/main/java/google/registry/model/domain/token/AllocationToken.java

@@ -208,7 +208,7 @@ public class AllocationToken extends BackupGroupRoot implements Buildable {
   }
 
   public VKey<AllocationToken> createVKey() {
-    return VKey.create(getClass(), getToken(), Key.create(this));
+    return VKey.create(AllocationToken.class, getToken(), Key.create(this));
   }
 
   @Override

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

@@ -22,7 +22,6 @@ import static google.registry.util.DateTimeUtils.START_OF_TIME;
 import static google.registry.util.DomainNameUtils.canonicalizeDomainName;
 
 import com.google.common.collect.ImmutableSet;
-import com.googlecode.objectify.Key;
 import com.googlecode.objectify.annotation.IgnoreSave;
 import com.googlecode.objectify.annotation.Index;
 import com.googlecode.objectify.condition.IfNull;
@@ -126,7 +125,9 @@ public class HostBase extends EppResource {
 
   @Override
   public VKey<? extends HostBase> createVKey() {
-    return VKey.create(HostBase.class, getRepoId(), Key.create(this));
+    throw new UnsupportedOperationException(
+        "HostBase is not an actual persisted entity you can create a key to;"
+            + " use HostResource instead");
   }
 
   @Deprecated

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

@@ -83,7 +83,8 @@ public class HostHistory extends HistoryEntry {
     @Override
     public Builder setParent(Key<? extends EppResource> parent) {
       super.setParent(parent);
-      getInstance().hostRepoId = VKey.create(HostResource.class, parent.getName(), parent);
+      getInstance().hostRepoId =
+          VKey.create(HostResource.class, parent.getName(), (Key<HostResource>) parent);
       return this;
     }
   }

core/src/main/java/google/registry/model/poll/PollMessage.java

@@ -301,7 +301,7 @@ public abstract class PollMessage extends ImmutableObject
 
     @Override
     public VKey<OneTime> createVKey() {
-      return VKey.create(this.getClass(), getId(), Key.create(this));
+      return VKey.create(OneTime.class, getId(), Key.create(this));
     }
 
     @Override
@@ -402,7 +402,7 @@ public abstract class PollMessage extends ImmutableObject
 
     @Override
     public VKey<Autorenew> createVKey() {
-      return VKey.create(this.getClass(), getId(), Key.create(this));
+      return VKey.create(Autorenew.class, getId(), Key.create(this));
     }
 
     @Override

core/src/main/java/google/registry/model/translators/VKeyTranslatorFactory.java

@@ -25,6 +25,7 @@ import com.googlecode.objectify.annotation.EntitySubclass;
 import google.registry.persistence.VKey;
 import java.lang.reflect.InvocationTargetException;
 import java.lang.reflect.Method;
+import javax.annotation.Nullable;
 
 /**
  * Translator factory for VKey.
@@ -38,7 +39,7 @@ public class VKeyTranslatorFactory extends AbstractSimpleTranslatorFactory<VKey,
   // name, which is all the datastore key gives us.
   // Note that entities annotated with @EntitySubclass are removed because they share the same
   // kind of the key with their parent class.
-  private static final ImmutableMap<String, Class> CLASS_REGISTRY =
+  private static final ImmutableMap<String, Class<?>> CLASS_REGISTRY =
       ALL_CLASSES.stream()
           .filter(clazz -> !clazz.isAnnotationPresent(EntitySubclass.class))
           .collect(toImmutableMap(com.googlecode.objectify.Key::getKind, identity()));
@@ -49,18 +50,23 @@ public class VKeyTranslatorFactory extends AbstractSimpleTranslatorFactory<VKey,
   }
 
   /** Create a VKey from a raw datastore key. */
-  public static VKey<?> createVKey(Key datastoreKey) {
+  @Nullable
+  public static VKey<?> createVKey(@Nullable Key datastoreKey) {
+    if (datastoreKey == null) {
+      return null;
+    }
     return createVKey(com.googlecode.objectify.Key.create(datastoreKey));
   }
 
   /** Create a VKey from an objectify Key. */
-  public static <T> VKey<T> createVKey(com.googlecode.objectify.Key<T> key) {
+  @Nullable
+  public static <T> VKey<T> createVKey(@Nullable com.googlecode.objectify.Key<T> key) {
     if (key == null) {
       return null;
     }
 
     // Try to create the VKey from its reference type.
-    Class clazz = CLASS_REGISTRY.get(key.getKind());
+    Class<T> clazz = (Class<T>) CLASS_REGISTRY.get(key.getKind());
     checkArgument(clazz != null, "Unknown Key type: %s", key.getKind());
     try {
       Method createVKeyMethod =
@@ -93,13 +99,16 @@ public class VKeyTranslatorFactory extends AbstractSimpleTranslatorFactory<VKey,
   @Override
   public SimpleTranslator<VKey, Key> createTranslator() {
     return new SimpleTranslator<VKey, Key>() {
+
+      @Nullable
       @Override
-      public VKey loadValue(Key datastoreValue) {
+      public VKey loadValue(@Nullable Key datastoreValue) {
         return createVKey(datastoreValue);
       }
 
+      @Nullable
       @Override
-      public Key saveValue(VKey key) {
+      public Key saveValue(@Nullable VKey key) {
         return key == null ? null : key.getOfyKey().getRaw();
       }
     };

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

@@ -22,6 +22,7 @@ import google.registry.model.ImmutableObject;
 import google.registry.model.translators.VKeyTranslatorFactory;
 import java.io.Serializable;
 import java.util.Optional;
+import javax.annotation.Nullable;
 
 /**
  * VKey is an abstraction that encapsulates the key concept.
@@ -52,49 +53,31 @@ public class VKey<T> extends ImmutableObject implements Serializable {
    *
    * <p>Deprecated. Create symmetric keys with create() instead.
    */
-  public static <T> VKey<T> createSql(Class<? extends T> kind, Object sqlKey) {
+  public static <T> VKey<T> createSql(Class<T> kind, Object sqlKey) {
     checkArgumentNotNull(kind, "kind must not be null");
     checkArgumentNotNull(sqlKey, "sqlKey must not be null");
-    return new VKey(kind, null, sqlKey);
+    return new VKey<T>(kind, null, sqlKey);
   }
 
   /** Creates a {@link VKey} which only contains the ofy primary key. */
-  public static <T> VKey<T> createOfy(
+  public static <T> VKey<T> createOfy(Class<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);
+    return new VKey<T>(kind, ofyKey, null);
-  }
-
-  /**
-   * Creates a {@link VKey} which only contains the ofy primary key by specifying the id of the
-   * {@link Key}.
-   */
-  public static <T> VKey<T> createOfy(Class<? extends T> kind, long id) {
-    return createOfy(kind, Key.create(kind, id));
-  }
-
-  /**
-   * Creates a {@link VKey} which only contains the ofy primary key by specifying the name of the
-   * {@link Key}.
-   */
-  public static <T> VKey<T> createOfy(Class<? extends T> kind, String name) {
-    checkArgumentNotNull(kind, "name must not be null");
-    return createOfy(kind, Key.create(kind, name));
   }
 
   /** Creates a {@link VKey} which only contains both sql and ofy primary key. */
   public static <T> VKey<T> create(
-      Class<? extends T> kind, Object sqlKey, com.googlecode.objectify.Key ofyKey) {
+      Class<T> kind, Object sqlKey, com.googlecode.objectify.Key<T> 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);
+    return new VKey<T>(kind, ofyKey, sqlKey);
   }
 
   /** Creates a symmetric {@link VKey} in which both sql and ofy keys are {@code id}. */
-  public static <T> VKey<T> create(Class<? extends T> kind, long id) {
+  public static <T> VKey<T> create(Class<T> kind, long id) {
-    return new VKey(kind, Key.create(kind, id), id);
+    return new VKey<T>(kind, Key.create(kind, id), id);
   }
 
   /** Returns the type of the entity. */
@@ -125,6 +108,7 @@ public class VKey<T> extends ImmutableObject implements Serializable {
   }
 
   /** Convenience method to construct a VKey from an objectify Key. */
+  @Nullable
   public static <T> VKey<T> from(Key<T> key) {
     return VKeyTranslatorFactory.createVKey(key);
   }

core/src/main/java/google/registry/rdap/RdapDomainSearchAction.java

@@ -295,7 +295,7 @@ public class RdapDomainSearchAction extends RdapSearchActionBase {
       query = query.filter("currentSponsorClientId", desiredRegistrar.get());
     }
     return StreamSupport.stream(query.keys().spliterator(), false)
-        .map(key -> VKey.from(key))
+        .map(VKey::from)
         .collect(toImmutableSet());
   }
 
@@ -406,7 +406,7 @@ public class RdapDomainSearchAction extends RdapSearchActionBase {
     }
     return searchByNameserverRefs(
         StreamSupport.stream(query.keys().spliterator(), false)
-            .map(key -> VKey.from(key))
+            .map(VKey::from)
             .collect(toImmutableSet()));
   }
 

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

@@ -51,7 +51,7 @@ public class RegistrarDaoTest {
   private Registrar testRegistrar;
 
   @BeforeEach
-  public void setUp() {
+  void setUp() {
     testRegistrar =
         new Registrar.Builder()
             .setType(Registrar.Type.TEST)
@@ -69,14 +69,14 @@ public class RegistrarDaoTest {
   }
 
   @Test
-  public void saveNew_worksSuccessfully() {
+  void saveNew_worksSuccessfully() {
     assertThat(jpaTm().transact(() -> jpaTm().checkExists(testRegistrar))).isFalse();
     jpaTm().transact(() -> jpaTm().saveNew(testRegistrar));
     assertThat(jpaTm().transact(() -> jpaTm().checkExists(testRegistrar))).isTrue();
   }
 
   @Test
-  public void update_worksSuccessfully() {
+  void update_worksSuccessfully() {
     jpaTm().transact(() -> jpaTm().saveNew(testRegistrar));
     Registrar persisted = jpaTm().transact(() -> jpaTm().load(registrarKey));
     assertThat(persisted.getRegistrarName()).isEqualTo("registrarName");
@@ -91,7 +91,7 @@ public class RegistrarDaoTest {
   }
 
   @Test
-  public void update_throwsExceptionWhenEntityDoesNotExist() {
+  void update_throwsExceptionWhenEntityDoesNotExist() {
     assertThat(jpaTm().transact(() -> jpaTm().checkExists(testRegistrar))).isFalse();
     assertThrows(
         IllegalArgumentException.class,
@@ -99,7 +99,7 @@ public class RegistrarDaoTest {
   }
 
   @Test
-  public void load_worksSuccessfully() {
+  void load_worksSuccessfully() {
     assertThat(jpaTm().transact(() -> jpaTm().checkExists(testRegistrar))).isFalse();
     jpaTm().transact(() -> jpaTm().saveNew(testRegistrar));
     Registrar persisted = jpaTm().transact(() -> jpaTm().load(registrarKey));