diff --git a/core/src/main/java/google/registry/model/domain/DomainBase.java b/core/src/main/java/google/registry/model/domain/DomainBase.java index 621e888e3..a6767f705 100644 --- a/core/src/main/java/google/registry/model/domain/DomainBase.java +++ b/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. - * - *

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> nsHosts; - - @Ignore + /** References to hosts that are the nameservers for the domain. */ + @Index @ElementCollection @JoinTable(name = "DomainHost") - Set> nsHostVKeys; + Set> 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> getNameservers() { - // Since nsHostVKeys gets initialized both from setNameservers() and the OnLoad method, this - // should always be valid. - return nullToEmptyImmutableCopy(nsHostVKeys); + return nullToEmptyImmutableCopy(nsHosts); } 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 nameserver) { - Optional> nsKey = nameserver.maybeGetOfyKey(); - if (nsKey.isPresent()) { - getInstance().nsHosts = ImmutableSet.of(nsKey.get()); - } else { - getInstance().nsHosts = null; - } - getInstance().nsHostVKeys = ImmutableSet.of(nameserver); + getInstance().nsHosts = ImmutableSet.of(nameserver); return thisCastToDerived(); } public Builder setNameservers(ImmutableSet> nameservers) { - // If we have all of the ofy keys, we can set nsHosts. Otherwise, make it null. - 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); + getInstance().nsHosts = forceEmptyToNull(nameservers); return thisCastToDerived(); } diff --git a/core/src/main/java/google/registry/model/ofy/ObjectifyService.java b/core/src/main/java/google/registry/model/ofy/ObjectifyService.java index 56bc8fa52..d33ffcfd5 100644 --- a/core/src/main/java/google/registry/model/ofy/ObjectifyService.java +++ b/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.class), - new VKeyTranslatorFactory(HostResource.class), + new VKeyTranslatorFactory(HostResource.class, ContactResource.class), new UpdateAutoTimestampTranslatorFactory())) { factory().getTranslators().add(translatorFactory); } diff --git a/core/src/main/java/google/registry/model/translators/VKeyTranslatorFactory.java b/core/src/main/java/google/registry/model/translators/VKeyTranslatorFactory.java index 7b1cd2cc0..a60f45b88 100644 --- a/core/src/main/java/google/registry/model/translators/VKeyTranslatorFactory.java +++ b/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.net.URLDecoder; -import java.net.URLEncoder; +import java.util.List; +import java.util.stream.Stream; /** * Translator factory for VKey. @@ -28,57 +30,42 @@ import java.net.URLEncoder; *

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 extends AbstractSimpleTranslatorFactory { - private final Class refClass; +public class VKeyTranslatorFactory extends AbstractSimpleTranslatorFactory { - public VKeyTranslatorFactory(Class 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 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 nameComponent = Splitter.on('.').splitToList(clazz.getName()); + return nameComponent.get(nameComponent.size() - 1); + }, + identity())); } @Override - public SimpleTranslator createTranslator() { - return new SimpleTranslator() { + public SimpleTranslator createTranslator() { + return new SimpleTranslator() { @Override - public VKey loadValue(String datastoreValue) { - int pos = datastoreValue.indexOf('&'); - Key ofyKey = null; - String sqlKey = null; - if (pos > 0) { - // 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); + public VKey loadValue(Key datastoreValue) { + // TODO(mmuller): we need to call a method on refClass to also reconstitute the SQL key. + return VKey.createOfy( + classRegistry.get(datastoreValue.getKind()), + com.googlecode.objectify.Key.create(datastoreValue)); } @Override - public String saveValue(VKey key) { - return ((key.getOfyKey() == null) ? "" : key.getOfyKey().getString()) - + "&" - + ((key.getSqlKey() == null) ? "" : encode(key.getSqlKey().toString())); + public Key saveValue(VKey key) { + return key.getOfyKey().getRaw(); } }; } - - 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); - } - } } diff --git a/core/src/test/java/google/registry/model/EntityTestCase.java b/core/src/test/java/google/registry/model/EntityTestCase.java index 8e4227bbf..d36dfb204 100644 --- a/core/src/test/java/google/registry/model/EntityTestCase.java +++ b/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() diff --git a/core/src/test/java/google/registry/model/domain/DomainBaseTest.java b/core/src/test/java/google/registry/model/domain/DomainBaseTest.java index b03b7e0dc..bee7f2b4a 100644 --- a/core/src/test/java/google/registry/model/domain/DomainBaseTest.java +++ b/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> 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 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) null)); - - // Test that setting to a set of values works. - VKey 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>) 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(); - } } diff --git a/core/src/test/resources/google/registry/model/schema.txt b/core/src/test/resources/google/registry/model/schema.txt index 3b1120dc0..75870b662 100644 --- a/core/src/test/resources/google/registry/model/schema.txt +++ b/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> nsHosts; java.util.Set allContacts; java.util.Set gracePeriods; java.util.Set dsData; java.util.Set status; + java.util.Set> nsHosts; java.util.Set subordinateHosts; org.joda.time.DateTime deletionTime; org.joda.time.DateTime lastEppUpdateTime; diff --git a/db/src/main/resources/sql/flyway/V25__rename_vkey_fields.sql b/db/src/main/resources/sql/flyway/V25__rename_vkey_fields.sql new file mode 100644 index 000000000..6286b66b8 --- /dev/null +++ b/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"; + diff --git a/db/src/main/resources/sql/schema/db-schema.sql.generated b/db/src/main/resources/sql/schema/db-schema.sql.generated index 0cbb1a2ed..838a02aea 100644 --- a/db/src/main/resources/sql/schema/db-schema.sql.generated +++ b/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" ( diff --git a/db/src/main/resources/sql/schema/nomulus.golden.sql b/db/src/main/resources/sql/schema/nomulus.golden.sql index eaf58f105..3370beb18 100644 --- a/db/src/main/resources/sql/schema/nomulus.golden.sql +++ b/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); --