Make VKey persist to datastore as a key (#591)

* Make VKey persist to datastore as a key

Convert nsHosts entirely to VKey as a proof-of-concept.

Tested as follows:
    1) Deployed to crash, verified that nameservers were visible for several
       domains (indicating that we are able to load a set of Keys as VKeys)
    2) Updated the set of nameservers for a domain (removing some initial
       hosts) and verified that the changes went through.
    3) Deployed the old version to crash, verified that I was able to retrieve
       the newly saved VKeys as Keys.
    4) Modified the hosts for the same domain (adding back one of the hosts)
       and verified that the change took effect.
    5) Redeployed this change to crash, again updated the nameservers to add
       another host.
    6) Again restored the old version, verified that the new hosts were
       visible.

* Changes in response to review

* Convert to a single VKeyTranslatorFactory instance

* Moved vkey field rename to V25

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

@@ -136,18 +136,11 @@ public class DomainBase extends EppResource
   @Index
   String tld;
 
-  /**
+  /** References to hosts that are the nameservers for the domain. */
-   * References to hosts that are the nameservers for the domain.
+  @Index
-   *
-   * <p>This is a legacy field: we have to preserve it because it is still persisted and indexed in
-   * the datastore, but all external references go through nsHostVKeys.
-   */
-  @Index @ElementCollection @Transient Set<Key<HostResource>> nsHosts;
-
-  @Ignore
   @ElementCollection
   @JoinTable(name = "DomainHost")
-  Set<VKey<HostResource>> nsHostVKeys;
+  Set<VKey<HostResource>> nsHosts;
 
   /**
    * The union of the contacts visible via {@link #getContacts} and {@link #getRegistrant}.
@@ -269,11 +262,6 @@ public class DomainBase extends EppResource
 
   @OnLoad
   void load() {
-    nsHostVKeys =
-        nullToEmptyImmutableCopy(nsHosts).stream()
-            .map(hostKey -> VKey.createOfy(HostResource.class, hostKey))
-            .collect(toImmutableSet());
-
     // Reconstitute all of the contacts so that they have VKeys.
     allContacts =
         allContacts.stream().map(contact -> contact.reconstitute()).collect(toImmutableSet());
@@ -363,9 +351,7 @@ public class DomainBase extends EppResource
   }
 
   public ImmutableSet<VKey<HostResource>> getNameservers() {
-    // Since nsHostVKeys gets initialized both from setNameservers() and the OnLoad method, this
+    return nullToEmptyImmutableCopy(nsHosts);
-    // should always be valid.
-    return nullToEmptyImmutableCopy(nsHostVKeys);
   }
 
   public final String getCurrentSponsorClientId() {
@@ -645,14 +631,6 @@ public class DomainBase extends EppResource
 
     Builder(DomainBase instance) {
       super(instance);
-
-      // Convert nsHosts to nsHostVKeys.
-      if (instance.nsHosts != null) {
-        instance.nsHostVKeys =
-            instance.nsHosts.stream()
-                .map(key -> VKey.createOfy(HostResource.class, key))
-                .collect(toImmutableSet());
-      }
     }
 
     @Override
@@ -710,27 +688,12 @@ public class DomainBase extends EppResource
     }
 
     public Builder setNameservers(VKey<HostResource> nameserver) {
-      Optional<Key<HostResource>> nsKey = nameserver.maybeGetOfyKey();
+      getInstance().nsHosts = ImmutableSet.of(nameserver);
-      if (nsKey.isPresent()) {
-        getInstance().nsHosts = ImmutableSet.of(nsKey.get());
-      } else {
-        getInstance().nsHosts = null;
-      }
-      getInstance().nsHostVKeys = ImmutableSet.of(nameserver);
       return thisCastToDerived();
     }
 
     public Builder setNameservers(ImmutableSet<VKey<HostResource>> nameservers) {
-      // If we have all of the ofy keys, we can set nsHosts.  Otherwise, make it null.
+      getInstance().nsHosts = forceEmptyToNull(nameservers);
-      if (nameservers != null
-          && nameservers.stream().allMatch(key -> key.maybeGetOfyKey().isPresent())) {
-        getInstance().nsHosts =
-            nameservers.stream().map(key -> key.getOfyKey()).collect(toImmutableSet());
-      } else {
-        getInstance().nsHosts = null;
-      }
-
-      getInstance().nsHostVKeys = forceEmptyToNull(nameservers);
       return thisCastToDerived();
     }
 

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

@@ -131,8 +131,7 @@ public class ObjectifyService {
             new InetAddressTranslatorFactory(),
             new MoneyStringTranslatorFactory(),
             new ReadableInstantUtcTranslatorFactory(),
-            new VKeyTranslatorFactory<ContactResource>(ContactResource.class),
+            new VKeyTranslatorFactory(HostResource.class, ContactResource.class),
-            new VKeyTranslatorFactory<HostResource>(HostResource.class),
             new UpdateAutoTimestampTranslatorFactory())) {
       factory().getTranslators().add(translatorFactory);
     }

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

@@ -14,13 +14,15 @@
 
 package google.registry.model.translators;
 
-import static java.nio.charset.StandardCharsets.UTF_8;
+import static com.google.common.base.Functions.identity;
+import static com.google.common.collect.ImmutableMap.toImmutableMap;
 
-import com.googlecode.objectify.Key;
+import com.google.appengine.api.datastore.Key;
+import com.google.common.base.Splitter;
+import com.google.common.collect.ImmutableMap;
 import google.registry.persistence.VKey;
-import java.io.UnsupportedEncodingException;
+import java.util.List;
-import java.net.URLDecoder;
+import java.util.stream.Stream;
-import java.net.URLEncoder;
 
 /**
  * Translator factory for VKey.
@@ -28,57 +30,42 @@ import java.net.URLEncoder;
  * <p>These get translated to a string containing the URL safe encoding of the objectify key
  * followed by a (url-unsafe) ampersand delimiter and the SQL key.
  */
-public class VKeyTranslatorFactory<T> extends AbstractSimpleTranslatorFactory<VKey, String> {
+public class VKeyTranslatorFactory extends AbstractSimpleTranslatorFactory<VKey, Key> {
-  private final Class<T> refClass;
 
-  public VKeyTranslatorFactory(Class<T> refClass) {
+  // Class registry allowing us to restore the original class object from the unqualified class
+  // name, which is all the datastore key gives us.
+  private final ImmutableMap<String, Class> classRegistry;
+
+  public VKeyTranslatorFactory(Class... refClasses) {
     super(VKey.class);
-    this.refClass = refClass;
+
+    // Store a registry of all classes by their unqualified name.
+    classRegistry =
+        Stream.of(refClasses)
+            .collect(
+                toImmutableMap(
+                    clazz -> {
+                      List<String> nameComponent = Splitter.on('.').splitToList(clazz.getName());
+                      return nameComponent.get(nameComponent.size() - 1);
+                    },
+                    identity()));
   }
 
   @Override
-  public SimpleTranslator<VKey, String> createTranslator() {
+  public SimpleTranslator<VKey, Key> createTranslator() {
-    return new SimpleTranslator<VKey, String>() {
+    return new SimpleTranslator<VKey, Key>() {
       @Override
-      public VKey loadValue(String datastoreValue) {
+      public VKey loadValue(Key datastoreValue) {
-        int pos = datastoreValue.indexOf('&');
+        // TODO(mmuller): we need to call a method on refClass to also reconstitute the SQL key.
-        Key ofyKey = null;
+        return VKey.createOfy(
-        String sqlKey = null;
+            classRegistry.get(datastoreValue.getKind()),
-        if (pos > 0) {
+            com.googlecode.objectify.Key.create(datastoreValue));
-          // We have an objectify key.
-          ofyKey = Key.create(datastoreValue.substring(0, pos));
-        }
-
-        if (pos < datastoreValue.length() - 1) {
-          // We have an SQL key.
-          sqlKey = decode(datastoreValue.substring(pos + 1));
-        }
-
-        return VKey.create(refClass, sqlKey, ofyKey);
       }
 
       @Override
-      public String saveValue(VKey key) {
+      public Key saveValue(VKey key) {
-        return ((key.getOfyKey() == null) ? "" : key.getOfyKey().getString())
+        return key.getOfyKey().getRaw();
-            + "&"
-            + ((key.getSqlKey() == null) ? "" : encode(key.getSqlKey().toString()));
       }
     };
   }
-
-  private static String encode(String val) {
-    try {
-      return URLEncoder.encode(val, UTF_8.toString());
-    } catch (UnsupportedEncodingException e) {
-      throw new RuntimeException(e);
-    }
-  }
-
-  private static String decode(String encoded) {
-    try {
-      return URLDecoder.decode(encoded, UTF_8.toString());
-    } catch (UnsupportedEncodingException e) {
-      throw new RuntimeException(e);
-    }
-  }
 }

core/src/test/java/google/registry/model/EntityTestCase.java

@@ -28,6 +28,7 @@ import com.googlecode.objectify.annotation.Parent;
 import com.googlecode.objectify.annotation.Serialize;
 import com.googlecode.objectify.cmd.Query;
 import google.registry.model.ofy.Ofy;
+import google.registry.persistence.VKey;
 import google.registry.testing.AppEngineRule;
 import google.registry.testing.FakeClock;
 import google.registry.testing.InjectRule;
@@ -164,7 +165,8 @@ public abstract class EntityTestCase {
                     : (Class<?>) inner;
           }
           // Descend into persisted ImmutableObject classes, but not anything else.
-          if (ImmutableObject.class.isAssignableFrom(fieldClass)) {
+          if (ImmutableObject.class.isAssignableFrom(fieldClass)
+              && !VKey.class.isAssignableFrom(fieldClass)) {
             getAllPotentiallyIndexedFieldPaths(fieldClass).stream()
                 .map(subfield -> field.getName() + "." + subfield)
                 .distinct()

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

@@ -771,60 +771,4 @@ public class DomainBaseTest extends EntityTestCase {
     assertThat(getOnlyElement(clone.getGracePeriods()).getType())
         .isEqualTo(GracePeriodStatus.TRANSFER);
   }
-
-  private static ImmutableSet<Key<HostResource>> getOfyNameservers(DomainBase domain) {
-    return domain.getNameservers().stream().map(key -> key.getOfyKey()).collect(toImmutableSet());
-  }
-
-  @Test
-  public void testNameservers_nsHostsOfyKeys() {
-    assertThat(domain.nsHosts).isEqualTo(getOfyNameservers(domain));
-
-    // Test the setNameserver that functions on a function.
-    VKey<HostResource> host1Key =
-        persistResource(
-                new HostResource.Builder()
-                    .setFullyQualifiedHostName("ns2.example.com")
-                    .setSuperordinateDomain(domainKey)
-                    .setRepoId("2-COM")
-                    .build())
-            .createKey();
-
-    DomainBase dom = new DomainBase.Builder(domain).setNameservers(host1Key).build();
-    assertThat(dom.getNameservers()).isEqualTo(ImmutableSet.of(host1Key));
-    assertThat(getOfyNameservers(dom)).isEqualTo(ImmutableSet.of(host1Key.getOfyKey()));
-
-    // Test that setting to a single host of null throws an NPE.
-    assertThrows(
-        NullPointerException.class,
-        () -> new DomainBase.Builder(domain).setNameservers((VKey<HostResource>) null));
-
-    // Test that setting to a set of values works.
-    VKey<HostResource> host2Key =
-        persistResource(
-                new HostResource.Builder()
-                    .setFullyQualifiedHostName("ns3.example.com")
-                    .setSuperordinateDomain(domainKey)
-                    .setRepoId("3-COM")
-                    .build())
-            .createKey();
-    dom =
-        new DomainBase.Builder(domain).setNameservers(ImmutableSet.of(host1Key, host2Key)).build();
-    assertThat(dom.getNameservers()).isEqualTo(ImmutableSet.of(host1Key, host2Key));
-    assertThat(getOfyNameservers(dom))
-        .isEqualTo(ImmutableSet.of(host1Key.getOfyKey(), host2Key.getOfyKey()));
-
-    // Set of values, passing null.
-    dom =
-        new DomainBase.Builder(domain)
-            .setNameservers((ImmutableSet<VKey<HostResource>>) null)
-            .build();
-    assertThat(dom.nsHostVKeys).isNull();
-    assertThat(dom.nsHosts).isNull();
-
-    // Empty set of values gets translated to null.
-    dom = new DomainBase.Builder(domain).setNameservers(ImmutableSet.of()).build();
-    assertThat(dom.nsHostVKeys).isNull();
-    assertThat(dom.nsHosts).isNull();
-  }
 }

core/src/test/resources/google/registry/model/schema.txt

@@ -179,11 +179,11 @@ class google.registry.model.domain.DomainBase {
   java.lang.String lastEppUpdateClientId;
   java.lang.String smdId;
   java.lang.String tld;
-  java.util.Set<com.googlecode.objectify.Key<google.registry.model.host.HostResource>> nsHosts;
   java.util.Set<google.registry.model.domain.DesignatedContact> allContacts;
   java.util.Set<google.registry.model.domain.GracePeriod> gracePeriods;
   java.util.Set<google.registry.model.domain.secdns.DelegationSignerData> dsData;
   java.util.Set<google.registry.model.eppcommon.StatusValue> status;
+  java.util.Set<google.registry.persistence.VKey<google.registry.model.host.HostResource>> nsHosts;
   java.util.Set<java.lang.String> subordinateHosts;
   org.joda.time.DateTime deletionTime;
   org.joda.time.DateTime lastEppUpdateTime;

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

@@ -0,0 +1,22 @@
+-- Copyright 2020 The Nomulus Authors. All Rights Reserved.
+--
+-- Licensed under the Apache License, Version 2.0 (the "License");
+-- you may not use this file except in compliance with the License.
+-- You may obtain a copy of the License at
+--
+--     http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+
+ALTER TABLE "DomainHost" DROP CONSTRAINT FK_DomainHost_host_valid;
+ALTER TABLE "DomainHost" ADD COLUMN ns_hosts text;
+ALTER TABLE "DomainHost" DROP COLUMN ns_host_v_keys;
+ALTER TABLE "DomainHost"
+  ADD CONSTRAINT FK_DomainHost_host_valid
+  FOREIGN KEY (ns_hosts)
+  REFERENCES "HostResource";
+

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

@@ -122,7 +122,7 @@
 
     create table "DomainHost" (
        domain_repo_id text not null,
-        ns_host_v_keys text
+        ns_hosts text
     );
 
     create table "GracePeriod" (

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

@@ -179,7 +179,7 @@ CREATE TABLE public."Domain" (
 
 CREATE TABLE public."DomainHost" (
     domain_repo_id text NOT NULL,
-    ns_host_v_keys text
+    ns_hosts text
 );
 
 
@@ -788,7 +788,7 @@ ALTER TABLE ONLY public."Domain"
 --
 
 ALTER TABLE ONLY public."DomainHost"
-    ADD CONSTRAINT fk_domainhost_host_valid FOREIGN KEY (ns_host_v_keys) REFERENCES public."HostResource"(repo_id);
+    ADD CONSTRAINT fk_domainhost_host_valid FOREIGN KEY (ns_hosts) REFERENCES public."HostResource"(repo_id);
 
 
 --