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
This commit is contained in:
cgoldfeder 2017-01-25 12:59:26 -08:00 committed by Ben McIlwain
parent 4a730e0c9e
commit 0b1781b110
10 changed files with 68 additions and 73 deletions

View file

@ -14,10 +14,12 @@
package google.registry.model; 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.base.Preconditions.checkState;
import static com.google.common.collect.Sets.difference; import static com.google.common.collect.Sets.difference;
import static com.google.common.collect.Sets.union; import static com.google.common.collect.Sets.union;
import static google.registry.util.CollectionUtils.difference; 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.CollectionUtils.nullToEmptyImmutableCopy;
import static google.registry.util.DateTimeUtils.END_OF_TIME; 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.Key;
import com.googlecode.objectify.annotation.Id; import com.googlecode.objectify.annotation.Id;
import com.googlecode.objectify.annotation.Index; import com.googlecode.objectify.annotation.Index;
import com.googlecode.objectify.annotation.OnLoad;
import google.registry.model.eppcommon.StatusValue; import google.registry.model.eppcommon.StatusValue;
import google.registry.model.ofy.CommitLogManifest; import google.registry.model.ofy.CommitLogManifest;
import google.registry.model.transfer.TransferData; import google.registry.model.transfer.TransferData;
@ -240,6 +243,9 @@ public abstract class EppResource extends BackupGroupRoot implements Buildable {
/** Set this resource's status values. */ /** Set this resource's status values. */
public B setStatusValues(ImmutableSet<StatusValue> statusValues) { public B setStatusValues(ImmutableSet<StatusValue> statusValues) {
checkArgument(
!nullToEmpty(statusValues).contains(StatusValue.LINKED),
"LINKED is a virtual status value that should never be set on an EppResource");
getInstance().status = statusValues; getInstance().status = statusValues;
return thisCastToDerived(); 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. */ /** Build the resource, nullifying empty strings and sets and setting defaults. */
@Override @Override
public T build() { public T build() {
// An EPP object has an implicit status of OK if no pending operations or prohibitions exist // 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).
removeStatusValue(StatusValue.OK); removeStatusValue(StatusValue.OK);
if (difference(getInstance().getStatusValues(), StatusValue.LINKED).isEmpty()) { if (getInstance().getStatusValues().isEmpty()) {
addStatusValue(StatusValue.OK); addStatusValue(StatusValue.OK);
} }
// If there is no deletion time, set it to END_OF_TIME. // 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()); 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);
}
}
} }

View file

@ -44,7 +44,16 @@ public enum StatusValue implements EppEnum {
CLIENT_TRANSFER_PROHIBITED, CLIENT_TRANSFER_PROHIBITED,
CLIENT_UPDATE_PROHIBITED, CLIENT_UPDATE_PROHIBITED,
INACTIVE, INACTIVE,
/**
* A status that means a resource has an incoming reference from an active domain.
*
* <p>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, LINKED,
OK, OK,
PENDING_CREATE, PENDING_CREATE,
PENDING_DELETE, PENDING_DELETE,

View file

@ -14,10 +14,12 @@
package google.registry.rde.imports; 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.base.Function;
import com.google.common.collect.FluentIterable;
import com.google.common.collect.ImmutableList; 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 com.google.common.collect.Lists;
import google.registry.model.contact.ContactAddress; import google.registry.model.contact.ContactAddress;
import google.registry.model.contact.ContactPhoneNumber; import google.registry.model.contact.ContactPhoneNumber;
@ -56,21 +58,24 @@ final class XjcToContactResourceConverter {
} }
}; };
private static final Function<XjcContactStatusType, StatusValue> STATUS_CONVERTER = private static final Function<XjcContactStatusType, StatusValue> STATUS_VALUE_CONVERTER =
new Function<XjcContactStatusType, StatusValue>() { new Function<XjcContactStatusType, StatusValue>() {
@Override @Override
public StatusValue apply(XjcContactStatusType status) { public StatusValue apply(XjcContactStatusType status) {
return convertStatusValue(status); return convertStatusValue(status);
} }
};
};
/** Converts {@link XjcRdeContact} to {@link ContactResource}. */ /** Converts {@link XjcRdeContact} to {@link ContactResource}. */
static ContactResource convertContact(XjcRdeContact contact) { static ContactResource convertContact(XjcRdeContact contact) {
return new ContactResource.Builder() return new ContactResource.Builder()
.setRepoId(contact.getRoid()) .setRepoId(contact.getRoid())
.setStatusValues( .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( .setLocalizedPostalInfo(
getPostalInfoOfType(contact.getPostalInfos(), XjcContactPostalInfoEnumType.LOC)) getPostalInfoOfType(contact.getPostalInfos(), XjcContactPostalInfoEnumType.LOC))
.setInternationalizedPostalInfo( .setInternationalizedPostalInfo(

View file

@ -14,7 +14,11 @@
package google.registry.rde.imports; 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.base.Function;
import com.google.common.collect.FluentIterable;
import com.google.common.collect.ImmutableSet; import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Lists; import com.google.common.collect.Lists;
import com.google.common.net.InetAddresses; import com.google.common.net.InetAddresses;
@ -55,7 +59,11 @@ public class XjcToHostResourceConverter {
.setCreationClientId(host.getCrRr().getValue()) .setCreationClientId(host.getCrRr().getValue())
.setLastEppUpdateClientId(host.getUpRr() == null ? null : host.getUpRr().getValue()) .setLastEppUpdateClientId(host.getUpRr() == null ? null : host.getUpRr().getValue())
.setStatusValues( .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))) .setInetAddresses(ImmutableSet.copyOf(Lists.transform(host.getAddrs(), ADDR_CONVERTER)))
.build(); .build();
} }

View file

@ -15,7 +15,9 @@
package google.registry.flows.domain; package google.registry.flows.domain;
import static com.google.common.truth.Truth.assertThat; 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.EppResourceUtils.loadByForeignKey;
import static google.registry.model.index.ForeignKeyIndex.loadAndGetKey;
import static google.registry.testing.DatastoreHelper.assertNoBillingEvents; import static google.registry.testing.DatastoreHelper.assertNoBillingEvents;
import static google.registry.testing.DatastoreHelper.createTld; import static google.registry.testing.DatastoreHelper.createTld;
import static google.registry.testing.DatastoreHelper.newDomainApplication; 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.persistActiveContact;
import static google.registry.testing.DatastoreHelper.persistActiveDomainApplication; import static google.registry.testing.DatastoreHelper.persistActiveDomainApplication;
import static google.registry.testing.DatastoreHelper.persistResource; 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.ImmutableMap;
import com.google.common.collect.ImmutableSet; import com.google.common.collect.ImmutableSet;
import com.googlecode.objectify.Key; import com.googlecode.objectify.Key;
@ -94,10 +96,10 @@ public class DomainApplicationDeleteFlowTest
loadByForeignKey(HostResource.class, "ns1.example.net", clock.nowUtc())))) loadByForeignKey(HostResource.class, "ns1.example.net", clock.nowUtc()))))
.build()); .build());
doSuccessfulTest(); doSuccessfulTest();
for (EppResource resource : new EppResource[]{ for (Key<? extends EppResource> key : ImmutableList.of(
loadByForeignKey(ContactResource.class, "sh8013", clock.nowUtc()), loadAndGetKey(ContactResource.class, "sh8013", clock.nowUtc()),
loadByForeignKey(HostResource.class, "ns1.example.net", clock.nowUtc()) }) { loadAndGetKey(HostResource.class, "ns1.example.net", clock.nowUtc()))) {
assertAboutEppResources().that(resource).doesNotHaveStatusValue(StatusValue.LINKED); assertThat(isLinked(key, clock.nowUtc())).isFalse();
} }
} }

View file

@ -172,37 +172,21 @@ public class ContactResourceTest extends EntityTestCase {
@Test @Test
public void testImplicitStatusValues() { public void testImplicitStatusValues() {
// OK is implicit if there's no other statuses. // OK is implicit if there's no other statuses.
StatusValue[] statuses = {StatusValue.OK};
assertAboutContacts() assertAboutContacts()
.that(new ContactResource.Builder().build()) .that(new ContactResource.Builder().build())
.hasExactlyStatusValues(statuses); .hasExactlyStatusValues(StatusValue.OK);
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};
// If there are other status values, OK should be suppressed. // If there are other status values, OK should be suppressed.
assertAboutContacts() assertAboutContacts()
.that(new ContactResource.Builder() .that(new ContactResource.Builder()
.setStatusValues(ImmutableSet.of(StatusValue.CLIENT_HOLD)) .setStatusValues(ImmutableSet.of(StatusValue.CLIENT_HOLD))
.build()) .build())
.hasExactlyStatusValues(statuses2); .hasExactlyStatusValues(StatusValue.CLIENT_HOLD);
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};
// When OK is suppressed, it should be removed even if it was originally there. // When OK is suppressed, it should be removed even if it was originally there.
assertAboutContacts() assertAboutContacts()
.that(new ContactResource.Builder() .that(new ContactResource.Builder()
.setStatusValues(ImmutableSet.of(StatusValue.OK, StatusValue.CLIENT_HOLD)) .setStatusValues(ImmutableSet.of(StatusValue.OK, StatusValue.CLIENT_HOLD))
.build()) .build())
.hasExactlyStatusValues(statuses4); .hasExactlyStatusValues(StatusValue.CLIENT_HOLD);
} }
@Test @Test

View file

@ -152,36 +152,21 @@ public class HostResourceTest extends EntityTestCase {
@Test @Test
public void testImplicitStatusValues() { public void testImplicitStatusValues() {
// OK is implicit if there's no other statuses. // OK is implicit if there's no other statuses.
StatusValue[] statuses = {StatusValue.OK};
assertAboutHosts() assertAboutHosts()
.that(new HostResource.Builder().build()) .that(new HostResource.Builder().build())
.hasExactlyStatusValues(statuses); .hasExactlyStatusValues(StatusValue.OK);
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};
// If there are other status values, OK should be suppressed. // If there are other status values, OK should be suppressed.
assertAboutHosts() assertAboutHosts()
.that(new HostResource.Builder() .that(new HostResource.Builder()
.setStatusValues(ImmutableSet.of(StatusValue.CLIENT_HOLD)) .setStatusValues(ImmutableSet.of(StatusValue.CLIENT_HOLD))
.build()) .build())
.hasExactlyStatusValues(statuses2); .hasExactlyStatusValues(StatusValue.CLIENT_HOLD);
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};
// When OK is suppressed, it should be removed even if it was originally there. // When OK is suppressed, it should be removed even if it was originally there.
assertAboutHosts() assertAboutHosts()
.that(new HostResource.Builder() .that(new HostResource.Builder()
.setStatusValues(ImmutableSet.of(StatusValue.OK, StatusValue.CLIENT_HOLD)) .setStatusValues(ImmutableSet.of(StatusValue.OK, StatusValue.CLIENT_HOLD))
.build()) .build())
.hasExactlyStatusValues(statuses4); .hasExactlyStatusValues(StatusValue.CLIENT_HOLD);
} }
@Nullable @Nullable

View file

@ -64,6 +64,7 @@ public class XjcToContactResourceConverterTest {
ContactResource resource = XjcToContactResourceConverter.convertContact(contact); ContactResource resource = XjcToContactResourceConverter.convertContact(contact);
assertThat(resource.getContactId()).isEqualTo("love-id"); assertThat(resource.getContactId()).isEqualTo("love-id");
assertThat(resource.getRepoId()).isEqualTo("2-ROID"); assertThat(resource.getRepoId()).isEqualTo("2-ROID");
// The imported XML also had LINKED status, but that should have been dropped on import.
assertThat(resource.getStatusValues()) assertThat(resource.getStatusValues())
.containsExactly( .containsExactly(
StatusValue.CLIENT_DELETE_PROHIBITED, StatusValue.CLIENT_DELETE_PROHIBITED,

View file

@ -20,11 +20,11 @@ import static google.registry.testing.DatastoreHelper.createTld;
import com.google.appengine.repackaged.com.google.common.net.InetAddresses; import com.google.appengine.repackaged.com.google.common.net.InetAddresses;
import com.google.common.base.Joiner; import com.google.common.base.Joiner;
import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.common.io.ByteSource; import com.google.common.io.ByteSource;
import google.registry.model.eppcommon.StatusValue; import google.registry.model.eppcommon.StatusValue;
import google.registry.model.host.HostResource; import google.registry.model.host.HostResource;
import google.registry.testing.AppEngineRule; import google.registry.testing.AppEngineRule;
import google.registry.testing.ShardableTestCase;
import google.registry.xjc.rdehost.XjcRdeHost; import google.registry.xjc.rdehost.XjcRdeHost;
import google.registry.xjc.rdehost.XjcRdeHostElement; import google.registry.xjc.rdehost.XjcRdeHostElement;
import java.io.InputStream; import java.io.InputStream;
@ -41,7 +41,7 @@ import org.junit.runners.JUnit4;
* Unit tests for {@link XjcToHostResourceConverter} * Unit tests for {@link XjcToHostResourceConverter}
*/ */
@RunWith(JUnit4.class) @RunWith(JUnit4.class)
public class XjcToHostResourceConverterTest { public class XjcToHostResourceConverterTest extends ShardableTestCase {
private static final ByteSource HOST_XML = RdeImportsTestData.get("host_fragment.xml"); private static final ByteSource HOST_XML = RdeImportsTestData.get("host_fragment.xml");
@ -82,13 +82,12 @@ public class XjcToHostResourceConverterTest {
HostResource host = XjcToHostResourceConverter.convert(xjcHost); HostResource host = XjcToHostResourceConverter.convert(xjcHost);
assertThat(host.getFullyQualifiedHostName()).isEqualTo("ns1.example1.test"); assertThat(host.getFullyQualifiedHostName()).isEqualTo("ns1.example1.test");
assertThat(host.getRepoId()).isEqualTo("Hns1_example1_test-TEST"); assertThat(host.getRepoId()).isEqualTo("Hns1_example1_test-TEST");
assertThat(host.getStatusValues()) // The imported XML also had LINKED status, but that should have been dropped on import.
.isEqualTo(ImmutableSet.of(StatusValue.OK, StatusValue.LINKED)); assertThat(host.getStatusValues()).containsExactly(StatusValue.OK);
assertThat(host.getInetAddresses()).isEqualTo( assertThat(host.getInetAddresses()).containsExactly(
ImmutableSet.of( InetAddresses.forString("192.0.2.2"),
InetAddresses.forString("192.0.2.2"), InetAddresses.forString("192.0.2.29"),
InetAddresses.forString("192.0.2.29"), InetAddresses.forString("1080:0:0:0:8:800:200C:417A"));
InetAddresses.forString("1080:0:0:0:8:800:200C:417A")));
assertThat(host.getCurrentSponsorClientId()).isEqualTo("RegistrarX"); assertThat(host.getCurrentSponsorClientId()).isEqualTo("RegistrarX");
assertThat(host.getCreationClientId()).isEqualTo("RegistrarX"); assertThat(host.getCreationClientId()).isEqualTo("RegistrarX");
assertThat(host.getCreationTime()).isEqualTo(DateTime.parse("1999-05-08T12:10:00.0Z")); 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")); 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 { private XjcRdeHost getHost() throws Exception {
try (InputStream ins = HOST_XML.openStream()) { try (InputStream ins = HOST_XML.openStream()) {
return ((XjcRdeHostElement) unmarshaller.unmarshal(ins)).getValue(); return ((XjcRdeHostElement) unmarshaller.unmarshal(ins)).getValue();

View file

@ -3,6 +3,7 @@
xmlns:contact="urn:ietf:params:xml:ns:contact-1.0"> xmlns:contact="urn:ietf:params:xml:ns:contact-1.0">
<rdeContact:id>love-id</rdeContact:id> <rdeContact:id>love-id</rdeContact:id>
<rdeContact:roid>2-ROID</rdeContact:roid> <rdeContact:roid>2-ROID</rdeContact:roid>
<rdeContact:status s="linked"/>
<rdeContact:status s="clientDeleteProhibited"/> <rdeContact:status s="clientDeleteProhibited"/>
<rdeContact:status s="serverUpdateProhibited"/> <rdeContact:status s="serverUpdateProhibited"/>
<rdeContact:postalInfo type="int"> <rdeContact:postalInfo type="int">