From 0b1781b1104ef4e56b49a1004f6491929dacc8ed Mon Sep 17 00:00:00 2001 From: cgoldfeder Date: Wed, 25 Jan 2017 12:59:26 -0800 Subject: [PATCH] Make LINKED into a virtual status value * Remove LINKED when loading an EppResource * Enforce that you can't add it to a resource * Ignore LINKED on xjc import of contacts and hosts After running ResaveAllEppResourcesAction we will no longer have persisted LINKED statuses in datastore. In the process of writing this I discovered that RDAP treats LINKED like any other status value and returns the persisted value rather than the derived one. Since this is an existing bug and is orthogonal to the changes in this CL, I am addressing it in a separate CL. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=145585227 --- java/google/registry/model/EppResource.java | 19 ++++++++++-- .../registry/model/eppcommon/StatusValue.java | 9 ++++++ .../XjcToContactResourceConverter.java | 17 +++++++---- .../imports/XjcToHostResourceConverter.java | 10 ++++++- .../DomainApplicationDeleteFlowTest.java | 12 ++++---- .../model/contact/ContactResourceTest.java | 22 ++------------ .../registry/model/host/HostResourceTest.java | 21 ++------------ .../XjcToContactResourceConverterTest.java | 1 + .../XjcToHostResourceConverterTest.java | 29 +++++-------------- .../rde/imports/testdata/contact_fragment.xml | 1 + 10 files changed, 68 insertions(+), 73 deletions(-) diff --git a/java/google/registry/model/EppResource.java b/java/google/registry/model/EppResource.java index 741e69c19..04ed155ee 100644 --- a/java/google/registry/model/EppResource.java +++ b/java/google/registry/model/EppResource.java @@ -14,10 +14,12 @@ package google.registry.model; +import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkState; import static com.google.common.collect.Sets.difference; import static com.google.common.collect.Sets.union; import static google.registry.util.CollectionUtils.difference; +import static google.registry.util.CollectionUtils.nullToEmpty; import static google.registry.util.CollectionUtils.nullToEmptyImmutableCopy; import static google.registry.util.DateTimeUtils.END_OF_TIME; @@ -28,6 +30,7 @@ import com.google.common.collect.ImmutableSortedMap; import com.googlecode.objectify.Key; import com.googlecode.objectify.annotation.Id; import com.googlecode.objectify.annotation.Index; +import com.googlecode.objectify.annotation.OnLoad; import google.registry.model.eppcommon.StatusValue; import google.registry.model.ofy.CommitLogManifest; import google.registry.model.transfer.TransferData; @@ -240,6 +243,9 @@ public abstract class EppResource extends BackupGroupRoot implements Buildable { /** Set this resource's status values. */ public B setStatusValues(ImmutableSet statusValues) { + checkArgument( + !nullToEmpty(statusValues).contains(StatusValue.LINKED), + "LINKED is a virtual status value that should never be set on an EppResource"); getInstance().status = statusValues; return thisCastToDerived(); } @@ -275,10 +281,9 @@ public abstract class EppResource extends BackupGroupRoot implements Buildable { /** Build the resource, nullifying empty strings and sets and setting defaults. */ @Override public T build() { - // An EPP object has an implicit status of OK if no pending operations or prohibitions exist - // (i.e. no other status value besides LINKED is present). + // An EPP object has an implicit status of OK if no pending operations or prohibitions exist. removeStatusValue(StatusValue.OK); - if (difference(getInstance().getStatusValues(), StatusValue.LINKED).isEmpty()) { + if (getInstance().getStatusValues().isEmpty()) { addStatusValue(StatusValue.OK); } // If there is no deletion time, set it to END_OF_TIME. @@ -286,4 +291,12 @@ public abstract class EppResource extends BackupGroupRoot implements Buildable { return ImmutableObject.cloneEmptyToNull(super.build()); } } + + // TODO(b/34664935): remove this once LINKED has been removed from all persisted resources. + @OnLoad + void onLoadRemoveLinked() { + if (status != null) { + status = difference(status, StatusValue.LINKED); + } + } } diff --git a/java/google/registry/model/eppcommon/StatusValue.java b/java/google/registry/model/eppcommon/StatusValue.java index a27b59c69..f5ea764ad 100644 --- a/java/google/registry/model/eppcommon/StatusValue.java +++ b/java/google/registry/model/eppcommon/StatusValue.java @@ -44,7 +44,16 @@ public enum StatusValue implements EppEnum { CLIENT_TRANSFER_PROHIBITED, CLIENT_UPDATE_PROHIBITED, INACTIVE, + + /** + * A status that means a resource has an incoming reference from an active domain. + * + *

LINKED is a "virtual" status value that should never be persisted to Datastore on any + * resource. It must be computed on the fly when we need it, as the set of domains using a + * resource can change at any time. + */ LINKED, + OK, PENDING_CREATE, PENDING_DELETE, diff --git a/java/google/registry/rde/imports/XjcToContactResourceConverter.java b/java/google/registry/rde/imports/XjcToContactResourceConverter.java index 507307902..a5df831fa 100644 --- a/java/google/registry/rde/imports/XjcToContactResourceConverter.java +++ b/java/google/registry/rde/imports/XjcToContactResourceConverter.java @@ -14,10 +14,12 @@ package google.registry.rde.imports; +import static com.google.common.base.Predicates.equalTo; +import static com.google.common.base.Predicates.not; + import com.google.common.base.Function; +import com.google.common.collect.FluentIterable; import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableSet; -import com.google.common.collect.Iterables; import com.google.common.collect.Lists; import google.registry.model.contact.ContactAddress; import google.registry.model.contact.ContactPhoneNumber; @@ -56,21 +58,24 @@ final class XjcToContactResourceConverter { } }; - private static final Function STATUS_CONVERTER = + private static final Function STATUS_VALUE_CONVERTER = new Function() { @Override public StatusValue apply(XjcContactStatusType status) { return convertStatusValue(status); } - - }; + }; /** Converts {@link XjcRdeContact} to {@link ContactResource}. */ static ContactResource convertContact(XjcRdeContact contact) { return new ContactResource.Builder() .setRepoId(contact.getRoid()) .setStatusValues( - ImmutableSet.copyOf(Iterables.transform(contact.getStatuses(), STATUS_CONVERTER))) + FluentIterable.from(contact.getStatuses()) + .transform(STATUS_VALUE_CONVERTER) + // LINKED is implicit and should not be imported onto the new contact. + .filter(not(equalTo(StatusValue.LINKED))) + .toSet()) .setLocalizedPostalInfo( getPostalInfoOfType(contact.getPostalInfos(), XjcContactPostalInfoEnumType.LOC)) .setInternationalizedPostalInfo( diff --git a/java/google/registry/rde/imports/XjcToHostResourceConverter.java b/java/google/registry/rde/imports/XjcToHostResourceConverter.java index bf50f6aa5..3fa60603a 100644 --- a/java/google/registry/rde/imports/XjcToHostResourceConverter.java +++ b/java/google/registry/rde/imports/XjcToHostResourceConverter.java @@ -14,7 +14,11 @@ package google.registry.rde.imports; +import static com.google.common.base.Predicates.equalTo; +import static com.google.common.base.Predicates.not; + import com.google.common.base.Function; +import com.google.common.collect.FluentIterable; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Lists; import com.google.common.net.InetAddresses; @@ -55,7 +59,11 @@ public class XjcToHostResourceConverter { .setCreationClientId(host.getCrRr().getValue()) .setLastEppUpdateClientId(host.getUpRr() == null ? null : host.getUpRr().getValue()) .setStatusValues( - ImmutableSet.copyOf(Lists.transform(host.getStatuses(), STATUS_VALUE_CONVERTER))) + FluentIterable.from(host.getStatuses()) + .transform(STATUS_VALUE_CONVERTER) + // LINKED is implicit and should not be imported onto the new host. + .filter(not(equalTo(StatusValue.LINKED))) + .toSet()) .setInetAddresses(ImmutableSet.copyOf(Lists.transform(host.getAddrs(), ADDR_CONVERTER))) .build(); } diff --git a/javatests/google/registry/flows/domain/DomainApplicationDeleteFlowTest.java b/javatests/google/registry/flows/domain/DomainApplicationDeleteFlowTest.java index 8da20d810..918cf00b0 100644 --- a/javatests/google/registry/flows/domain/DomainApplicationDeleteFlowTest.java +++ b/javatests/google/registry/flows/domain/DomainApplicationDeleteFlowTest.java @@ -15,7 +15,9 @@ package google.registry.flows.domain; import static com.google.common.truth.Truth.assertThat; +import static google.registry.model.EppResourceUtils.isLinked; import static google.registry.model.EppResourceUtils.loadByForeignKey; +import static google.registry.model.index.ForeignKeyIndex.loadAndGetKey; import static google.registry.testing.DatastoreHelper.assertNoBillingEvents; import static google.registry.testing.DatastoreHelper.createTld; import static google.registry.testing.DatastoreHelper.newDomainApplication; @@ -23,8 +25,8 @@ import static google.registry.testing.DatastoreHelper.newHostResource; import static google.registry.testing.DatastoreHelper.persistActiveContact; import static google.registry.testing.DatastoreHelper.persistActiveDomainApplication; import static google.registry.testing.DatastoreHelper.persistResource; -import static google.registry.testing.GenericEppResourceSubject.assertAboutEppResources; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.googlecode.objectify.Key; @@ -94,10 +96,10 @@ public class DomainApplicationDeleteFlowTest loadByForeignKey(HostResource.class, "ns1.example.net", clock.nowUtc())))) .build()); doSuccessfulTest(); - for (EppResource resource : new EppResource[]{ - loadByForeignKey(ContactResource.class, "sh8013", clock.nowUtc()), - loadByForeignKey(HostResource.class, "ns1.example.net", clock.nowUtc()) }) { - assertAboutEppResources().that(resource).doesNotHaveStatusValue(StatusValue.LINKED); + for (Key key : ImmutableList.of( + loadAndGetKey(ContactResource.class, "sh8013", clock.nowUtc()), + loadAndGetKey(HostResource.class, "ns1.example.net", clock.nowUtc()))) { + assertThat(isLinked(key, clock.nowUtc())).isFalse(); } } diff --git a/javatests/google/registry/model/contact/ContactResourceTest.java b/javatests/google/registry/model/contact/ContactResourceTest.java index ad00d3e96..cf228b616 100644 --- a/javatests/google/registry/model/contact/ContactResourceTest.java +++ b/javatests/google/registry/model/contact/ContactResourceTest.java @@ -172,37 +172,21 @@ public class ContactResourceTest extends EntityTestCase { @Test public void testImplicitStatusValues() { // OK is implicit if there's no other statuses. - StatusValue[] statuses = {StatusValue.OK}; assertAboutContacts() .that(new ContactResource.Builder().build()) - .hasExactlyStatusValues(statuses); - StatusValue[] statuses1 = {StatusValue.OK, StatusValue.LINKED}; - // OK is also implicit if the only other status is LINKED. - assertAboutContacts() - .that(new ContactResource.Builder() - .setStatusValues(ImmutableSet.of(StatusValue.LINKED)) - .build()) - .hasExactlyStatusValues(statuses1); - StatusValue[] statuses2 = {StatusValue.CLIENT_HOLD}; + .hasExactlyStatusValues(StatusValue.OK); // If there are other status values, OK should be suppressed. assertAboutContacts() .that(new ContactResource.Builder() .setStatusValues(ImmutableSet.of(StatusValue.CLIENT_HOLD)) .build()) - .hasExactlyStatusValues(statuses2); - StatusValue[] statuses3 = {StatusValue.LINKED, StatusValue.CLIENT_HOLD}; - assertAboutContacts() - .that(new ContactResource.Builder() - .setStatusValues(ImmutableSet.of(StatusValue.LINKED, StatusValue.CLIENT_HOLD)) - .build()) - .hasExactlyStatusValues(statuses3); - StatusValue[] statuses4 = {StatusValue.CLIENT_HOLD}; + .hasExactlyStatusValues(StatusValue.CLIENT_HOLD); // When OK is suppressed, it should be removed even if it was originally there. assertAboutContacts() .that(new ContactResource.Builder() .setStatusValues(ImmutableSet.of(StatusValue.OK, StatusValue.CLIENT_HOLD)) .build()) - .hasExactlyStatusValues(statuses4); + .hasExactlyStatusValues(StatusValue.CLIENT_HOLD); } @Test diff --git a/javatests/google/registry/model/host/HostResourceTest.java b/javatests/google/registry/model/host/HostResourceTest.java index a81ed6244..e73d1d384 100644 --- a/javatests/google/registry/model/host/HostResourceTest.java +++ b/javatests/google/registry/model/host/HostResourceTest.java @@ -152,36 +152,21 @@ public class HostResourceTest extends EntityTestCase { @Test public void testImplicitStatusValues() { // OK is implicit if there's no other statuses. - StatusValue[] statuses = {StatusValue.OK}; assertAboutHosts() .that(new HostResource.Builder().build()) - .hasExactlyStatusValues(statuses); - StatusValue[] statuses1 = {StatusValue.OK, StatusValue.LINKED}; - // OK is also implicit if the only other status is LINKED. - assertAboutHosts() - .that(new HostResource.Builder() - .setStatusValues(ImmutableSet.of(StatusValue.LINKED)).build()) - .hasExactlyStatusValues(statuses1); - StatusValue[] statuses2 = {StatusValue.CLIENT_HOLD}; + .hasExactlyStatusValues(StatusValue.OK); // If there are other status values, OK should be suppressed. assertAboutHosts() .that(new HostResource.Builder() .setStatusValues(ImmutableSet.of(StatusValue.CLIENT_HOLD)) .build()) - .hasExactlyStatusValues(statuses2); - StatusValue[] statuses3 = {StatusValue.LINKED, StatusValue.CLIENT_HOLD}; - assertAboutHosts() - .that(new HostResource.Builder() - .setStatusValues(ImmutableSet.of(StatusValue.LINKED, StatusValue.CLIENT_HOLD)) - .build()) - .hasExactlyStatusValues(statuses3); - StatusValue[] statuses4 = {StatusValue.CLIENT_HOLD}; + .hasExactlyStatusValues(StatusValue.CLIENT_HOLD); // When OK is suppressed, it should be removed even if it was originally there. assertAboutHosts() .that(new HostResource.Builder() .setStatusValues(ImmutableSet.of(StatusValue.OK, StatusValue.CLIENT_HOLD)) .build()) - .hasExactlyStatusValues(statuses4); + .hasExactlyStatusValues(StatusValue.CLIENT_HOLD); } @Nullable diff --git a/javatests/google/registry/rde/imports/XjcToContactResourceConverterTest.java b/javatests/google/registry/rde/imports/XjcToContactResourceConverterTest.java index 4b346cafd..f74a287ee 100644 --- a/javatests/google/registry/rde/imports/XjcToContactResourceConverterTest.java +++ b/javatests/google/registry/rde/imports/XjcToContactResourceConverterTest.java @@ -64,6 +64,7 @@ public class XjcToContactResourceConverterTest { ContactResource resource = XjcToContactResourceConverter.convertContact(contact); assertThat(resource.getContactId()).isEqualTo("love-id"); assertThat(resource.getRepoId()).isEqualTo("2-ROID"); + // The imported XML also had LINKED status, but that should have been dropped on import. assertThat(resource.getStatusValues()) .containsExactly( StatusValue.CLIENT_DELETE_PROHIBITED, diff --git a/javatests/google/registry/rde/imports/XjcToHostResourceConverterTest.java b/javatests/google/registry/rde/imports/XjcToHostResourceConverterTest.java index 348c70551..6166cee07 100644 --- a/javatests/google/registry/rde/imports/XjcToHostResourceConverterTest.java +++ b/javatests/google/registry/rde/imports/XjcToHostResourceConverterTest.java @@ -20,11 +20,11 @@ import static google.registry.testing.DatastoreHelper.createTld; import com.google.appengine.repackaged.com.google.common.net.InetAddresses; import com.google.common.base.Joiner; import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableSet; import com.google.common.io.ByteSource; import google.registry.model.eppcommon.StatusValue; import google.registry.model.host.HostResource; import google.registry.testing.AppEngineRule; +import google.registry.testing.ShardableTestCase; import google.registry.xjc.rdehost.XjcRdeHost; import google.registry.xjc.rdehost.XjcRdeHostElement; import java.io.InputStream; @@ -41,7 +41,7 @@ import org.junit.runners.JUnit4; * Unit tests for {@link XjcToHostResourceConverter} */ @RunWith(JUnit4.class) -public class XjcToHostResourceConverterTest { +public class XjcToHostResourceConverterTest extends ShardableTestCase { private static final ByteSource HOST_XML = RdeImportsTestData.get("host_fragment.xml"); @@ -82,13 +82,12 @@ public class XjcToHostResourceConverterTest { HostResource host = XjcToHostResourceConverter.convert(xjcHost); assertThat(host.getFullyQualifiedHostName()).isEqualTo("ns1.example1.test"); assertThat(host.getRepoId()).isEqualTo("Hns1_example1_test-TEST"); - assertThat(host.getStatusValues()) - .isEqualTo(ImmutableSet.of(StatusValue.OK, StatusValue.LINKED)); - assertThat(host.getInetAddresses()).isEqualTo( - ImmutableSet.of( - InetAddresses.forString("192.0.2.2"), - InetAddresses.forString("192.0.2.29"), - InetAddresses.forString("1080:0:0:0:8:800:200C:417A"))); + // The imported XML also had LINKED status, but that should have been dropped on import. + assertThat(host.getStatusValues()).containsExactly(StatusValue.OK); + assertThat(host.getInetAddresses()).containsExactly( + InetAddresses.forString("192.0.2.2"), + InetAddresses.forString("192.0.2.29"), + InetAddresses.forString("1080:0:0:0:8:800:200C:417A")); assertThat(host.getCurrentSponsorClientId()).isEqualTo("RegistrarX"); assertThat(host.getCreationClientId()).isEqualTo("RegistrarX"); assertThat(host.getCreationTime()).isEqualTo(DateTime.parse("1999-05-08T12:10:00.0Z")); @@ -97,18 +96,6 @@ public class XjcToHostResourceConverterTest { assertThat(host.getLastTransferTime()).isEqualTo(DateTime.parse("2008-10-03T09:34:00.0Z")); } - // included to pass the round-robin sharding filter - @Test - public void testNothing1() {} - - // included to pass the round-robin sharding filter - @Test - public void testNothing2() {} - - // included to pass the round-robin sharding filter - @Test - public void testNothing3() {} - private XjcRdeHost getHost() throws Exception { try (InputStream ins = HOST_XML.openStream()) { return ((XjcRdeHostElement) unmarshaller.unmarshal(ins)).getValue(); diff --git a/javatests/google/registry/rde/imports/testdata/contact_fragment.xml b/javatests/google/registry/rde/imports/testdata/contact_fragment.xml index be8a13f07..3f9549506 100644 --- a/javatests/google/registry/rde/imports/testdata/contact_fragment.xml +++ b/javatests/google/registry/rde/imports/testdata/contact_fragment.xml @@ -3,6 +3,7 @@ xmlns:contact="urn:ietf:params:xml:ns:contact-1.0"> love-id 2-ROID +