From b54c19e6c4ac634e34cd3e96b2751c8237dcdffa Mon Sep 17 00:00:00 2001 From: Michael Muller Date: Tue, 19 May 2020 14:10:28 -0400 Subject: [PATCH] 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 --- .../registry/model/domain/DomainBase.java | 49 ++---------- .../registry/model/ofy/ObjectifyService.java | 3 +- .../translators/VKeyTranslatorFactory.java | 79 ++++++++----------- .../google/registry/model/EntityTestCase.java | 4 +- .../registry/model/domain/DomainBaseTest.java | 56 ------------- .../google/registry/model/schema.txt | 2 +- .../sql/flyway/V25__rename_vkey_fields.sql | 22 ++++++ .../sql/schema/db-schema.sql.generated | 2 +- .../resources/sql/schema/nomulus.golden.sql | 4 +- 9 files changed, 69 insertions(+), 152 deletions(-) create mode 100644 db/src/main/resources/sql/flyway/V25__rename_vkey_fields.sql 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); --