From c23bbe35bb6ec7adce50bc1e6e9afc62ba84414b Mon Sep 17 00:00:00 2001 From: cgoldfeder Date: Thu, 16 Feb 2017 18:54:16 -0800 Subject: [PATCH] Improve handling of lastSubordinateChange and beef up tests See [] for the comments that led to this. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=147796087 --- .../registry/flows/host/HostUpdateFlow.java | 20 +- .../registry/model/host/HostResource.java | 7 +- .../flows/host/HostCreateFlowTest.java | 27 ++- .../flows/host/HostUpdateFlowTest.java | 213 +++++++++--------- .../registry/testing/HostResourceSubject.java | 16 ++ 5 files changed, 159 insertions(+), 124 deletions(-) diff --git a/java/google/registry/flows/host/HostUpdateFlow.java b/java/google/registry/flows/host/HostUpdateFlow.java index 9a85b7928..15352b619 100644 --- a/java/google/registry/flows/host/HostUpdateFlow.java +++ b/java/google/registry/flows/host/HostUpdateFlow.java @@ -128,9 +128,10 @@ public final class HostUpdateFlow implements TransactionalFlow { boolean isHostRename = suppliedNewHostName != null; String oldHostName = targetId; String newHostName = firstNonNull(suppliedNewHostName, oldHostName); - Optional superordinateDomain = + // Note that lookupSuperordinateDomain calls cloneProjectedAtTime on the domain for us. + Optional newSuperordinateDomain = Optional.fromNullable(lookupSuperordinateDomain(validateHostName(newHostName), now)); - verifyUpdateAllowed(command, existingHost, superordinateDomain.orNull()); + verifyUpdateAllowed(command, existingHost, newSuperordinateDomain.orNull()); if (isHostRename && loadAndGetKey(HostResource.class, newHostName, now) != null) { throw new HostAlreadyExistsException(newHostName); } @@ -138,6 +139,13 @@ public final class HostUpdateFlow implements TransactionalFlow { AddRemove remove = command.getInnerRemove(); checkSameValuesNotAddedAndRemoved(add.getStatusValues(), remove.getStatusValues()); checkSameValuesNotAddedAndRemoved(add.getInetAddresses(), remove.getInetAddresses()); + Key newSuperordinateDomainKey = + newSuperordinateDomain.isPresent() ? Key.create(newSuperordinateDomain.get()) : null; + // If the superordinateDomain field is changing, set the lastSuperordinateChange to now. + DateTime lastSuperordinateChange = + Objects.equals(newSuperordinateDomainKey, existingHost.getSuperordinateDomain()) + ? existingHost.getLastSuperordinateChange() + : now; HostResource newHost = existingHost.asBuilder() .setFullyQualifiedHostName(newHostName) .addStatusValues(add.getStatusValues()) @@ -146,12 +154,8 @@ public final class HostUpdateFlow implements TransactionalFlow { .removeInetAddresses(remove.getInetAddresses()) .setLastEppUpdateTime(now) .setLastEppUpdateClientId(clientId) - // The superordinateDomain can be missing if the new name is external. - // Note that the value of superordinateDomain is projected to the current time inside of - // the lookupSuperordinateDomain(...) call above, so that it will never be stale. - .setSuperordinateDomain( - superordinateDomain.isPresent() ? Key.create(superordinateDomain.get()) : null) - .setLastSuperordinateChange(superordinateDomain.isPresent() ? now : null) + .setSuperordinateDomain(newSuperordinateDomainKey) + .setLastSuperordinateChange(lastSuperordinateChange) .build() // Rely on the host's cloneProjectedAtTime() method to handle setting of transfer data. .cloneProjectedAtTime(now); diff --git a/java/google/registry/model/host/HostResource.java b/java/google/registry/model/host/HostResource.java index fb4d1cd62..7caaba54c 100644 --- a/java/google/registry/model/host/HostResource.java +++ b/java/google/registry/model/host/HostResource.java @@ -85,8 +85,11 @@ public class HostResource extends EppResource implements ForeignKeyedEppResource DateTime lastTransferTime; /** - * The most recent time that the superordinate domain was changed, or null if this host is - * external. + * The most recent time that the {@link #superordinateDomain} field was changed. + * + *

This should be updated whenever the superordinate domain changes, including when it is set + * to null. This field will be null for new hosts that have never experienced a change of + * superordinate domain. */ DateTime lastSuperordinateChange; diff --git a/javatests/google/registry/flows/host/HostCreateFlowTest.java b/javatests/google/registry/flows/host/HostCreateFlowTest.java index f91ce5a99..0665b6a4a 100644 --- a/javatests/google/registry/flows/host/HostCreateFlowTest.java +++ b/javatests/google/registry/flows/host/HostCreateFlowTest.java @@ -15,7 +15,7 @@ package google.registry.flows.host; import static com.google.common.truth.Truth.assertThat; -import static google.registry.model.ofy.ObjectifyService.ofy; +import static google.registry.model.EppResourceUtils.loadByForeignKey; import static google.registry.testing.DatastoreHelper.assertNoBillingEvents; import static google.registry.testing.DatastoreHelper.createTld; import static google.registry.testing.DatastoreHelper.persistActiveDomain; @@ -27,6 +27,7 @@ import static google.registry.testing.TaskQueueHelper.assertNoDnsTasksEnqueued; import com.google.common.base.Strings; import com.google.common.collect.ImmutableMap; +import com.googlecode.objectify.Key; import google.registry.flows.EppXmlTransformer.IpAddressVersionMismatchException; import google.registry.flows.ResourceFlowTestCase; import google.registry.flows.exceptions.ResourceAlreadyExistsException; @@ -39,6 +40,7 @@ import google.registry.flows.host.HostFlowUtils.HostNameTooLongException; import google.registry.flows.host.HostFlowUtils.HostNameTooShallowException; import google.registry.flows.host.HostFlowUtils.InvalidHostNameException; import google.registry.flows.host.HostFlowUtils.SuperordinateDomainDoesNotExistException; +import google.registry.model.domain.DomainResource; import google.registry.model.host.HostResource; import google.registry.model.reporting.HistoryEntry; import org.joda.time.DateTime; @@ -80,6 +82,7 @@ public class HostCreateFlowTest extends ResourceFlowTestCase192.0.2.22", "1080:0:0:0:8:800:200C:417A"); createTld("tld"); - DomainResource domain = persistActiveDomain("example.tld"); - persistActiveSubordinateHost(oldHostName(), domain); - persistResource( - domain.asBuilder() + DomainResource domain = persistResource(newDomainResource("example.tld") + .asBuilder() .setSubordinateHosts(ImmutableSet.of(oldHostName())) .build()); - assertThat( - loadByForeignKey( - DomainResource.class, "example.tld", clock.nowUtc()).getSubordinateHosts()) - .containsExactly("ns1.example.tld"); + HostResource oldHost = persistActiveSubordinateHost(oldHostName(), domain); + assertThat(domain.getSubordinateHosts()).containsExactly("ns1.example.tld"); HostResource renamedHost = doSuccessfulTest(); - assertThat(renamedHost.getSuperordinateDomain()).isEqualTo(Key.create(domain)); - assertThat( - loadByForeignKey( - DomainResource.class, "example.tld", clock.nowUtc()).getSubordinateHosts()) - .containsExactly("ns2.example.tld"); + assertAboutHosts().that(renamedHost) + .hasSuperordinateDomain(Key.create(domain)).and() + .hasLastSuperordinateChange(oldHost.getLastSuperordinateChange()); + DomainResource reloadedDomain = + ofy().load().entity(domain).now().cloneProjectedAtTime(clock.nowUtc()); + assertThat(reloadedDomain.getSubordinateHosts()).containsExactly("ns2.example.tld"); assertDnsTasksEnqueued("ns1.example.tld", "ns2.example.tld"); } @Test - public void testSuccess_internalToSameInternal() throws Exception { + public void testSuccess_internalToInternalOnSameTld() throws Exception { setEppHostUpdateInput( "ns2.foo.tld", "ns2.example.tld", "192.0.2.22", "1080:0:0:0:8:800:200C:417A"); createTld("tld"); - DomainResource foo = persistActiveDomain("foo.tld"); DomainResource example = persistActiveDomain("example.tld"); + DomainResource foo = persistResource(newDomainResource("foo.tld").asBuilder() + .setSubordinateHosts(ImmutableSet.of(oldHostName())) + .build()); persistActiveSubordinateHost(oldHostName(), foo); - persistResource( - foo.asBuilder() - .setSubordinateHosts(ImmutableSet.of(oldHostName())) - .build()); - assertThat( - loadByForeignKey( - DomainResource.class, "foo.tld", clock.nowUtc()).getSubordinateHosts()) - .containsExactly("ns2.foo.tld"); - assertThat( - loadByForeignKey( - DomainResource.class, "example.tld", clock.nowUtc()).getSubordinateHosts()) - .isEmpty(); + assertThat(foo.getSubordinateHosts()).containsExactly("ns2.foo.tld"); + assertThat(example.getSubordinateHosts()).isEmpty(); HostResource renamedHost = doSuccessfulTest(); - assertThat(renamedHost.getSuperordinateDomain()).isEqualTo(Key.create(example)); - assertThat( - loadByForeignKey( - DomainResource.class, "foo.tld", clock.nowUtc()).getSubordinateHosts()) - .isEmpty(); - assertThat( - loadByForeignKey( - DomainResource.class, "example.tld", clock.nowUtc()).getSubordinateHosts()) - .containsExactly("ns2.example.tld"); + DateTime now = clock.nowUtc(); + assertAboutHosts().that(renamedHost) + .hasSuperordinateDomain(Key.create(example)).and() + .hasLastSuperordinateChange(now); + assertThat(ofy().load().entity(foo).now().cloneProjectedAtTime(now).getSubordinateHosts()) + .isEmpty(); + assertThat(ofy().load().entity(example).now().cloneProjectedAtTime(now).getSubordinateHosts()) + .containsExactly("ns2.example.tld"); assertDnsTasksEnqueued("ns2.foo.tld", "ns2.example.tld"); } + @Test + public void testSuccess_internalToInternalOnDifferentTld() throws Exception { + setEppHostUpdateInput( + "ns1.example.foo", + "ns2.example.tld", + "192.0.2.22", + "1080:0:0:0:8:800:200C:417A"); + createTld("foo"); + createTld("tld"); + DomainResource fooDomain = persistResource(newDomainResource("example.foo").asBuilder() + .setSubordinateHosts(ImmutableSet.of(oldHostName())) + .build()); + DomainResource tldDomain = persistActiveDomain("example.tld"); + HostResource oldHost = persistActiveSubordinateHost(oldHostName(), fooDomain); + assertThat(oldHost.getCurrentSponsorClientId()).isEqualTo("TheRegistrar"); + assertThat(fooDomain.getSubordinateHosts()).containsExactly("ns1.example.foo"); + assertThat(tldDomain.getSubordinateHosts()).isEmpty(); + HostResource renamedHost = doSuccessfulTest(); + DateTime now = clock.nowUtc(); + assertAboutHosts().that(renamedHost) + .hasSuperordinateDomain(Key.create(tldDomain)).and() + .hasLastSuperordinateChange(now); + DomainResource reloadedFooDomain = + ofy().load().entity(fooDomain).now().cloneProjectedAtTime(now); + assertThat(reloadedFooDomain.getSubordinateHosts()).isEmpty(); + DomainResource reloadedTldDomain = + ofy().load().entity(tldDomain).now().cloneProjectedAtTime(now); + assertThat(reloadedTldDomain.getSubordinateHosts()).containsExactly("ns2.example.tld"); + assertDnsTasksEnqueued("ns1.example.foo", "ns2.example.tld"); + // Ensure that the client id is read off the domain because this is a subordinate host now. + persistResource( + tldDomain.asBuilder().setCurrentSponsorClientId("it_should_be_this").build()); + assertThat( + renamedHost.cloneProjectedAtTime(clock.nowUtc().plusMinutes(1)).getCurrentSponsorClientId()) + .isEqualTo("it_should_be_this"); + } + @Test public void testSuccess_internalToExternal() throws Exception { setEppHostUpdateInput( @@ -256,62 +284,28 @@ public class HostUpdateFlowTest extends ResourceFlowTestCase1080:0:0:0:8:800:200C:417A"); createTld("foo"); - DomainResource domain = persistActiveDomain("example.foo"); - persistActiveSubordinateHost(oldHostName(), domain); - persistResource( - domain.asBuilder() - .setSubordinateHosts(ImmutableSet.of(oldHostName())) + DomainResource domain = persistResource(newDomainResource("example.foo").asBuilder() + .setSubordinateHosts(ImmutableSet.of(oldHostName())) + .build()); + assertThat(domain.getCurrentSponsorClientId()).isEqualTo("TheRegistrar"); + HostResource oldHost = persistResource( + persistActiveSubordinateHost(oldHostName(), domain).asBuilder() + .setCurrentSponsorClientId("ClientThatShouldBeSupersededByDomainClient") .build()); - assertThat( - loadByForeignKey( - DomainResource.class, "example.foo", clock.nowUtc()).getSubordinateHosts()) - .containsExactly("ns1.example.foo"); + assertThat(oldHost.isSubordinate()).isTrue(); + assertThat(domain.getSubordinateHosts()).containsExactly("ns1.example.foo"); HostResource renamedHost = doSuccessfulTest(); - assertThat(renamedHost.isSubordinate()).isFalse(); - // Ensure that the client id is set to the new registrar correctly (and that this necessarily - // comes from the field on the host itself, because the superordinate domain is null). - assertThat(renamedHost.getCurrentSponsorClientId()).isEqualTo("TheRegistrar"); - assertThat( - loadByForeignKey( - DomainResource.class, "example.foo", clock.nowUtc()).getSubordinateHosts()) - .isEmpty(); + assertAboutHosts().that(renamedHost) + // Ensure that the client id is copied off of the superordinate domain. + .hasCurrentSponsorClientId("TheRegistrar").and() + .hasSuperordinateDomain(null).and() + .hasLastSuperordinateChange(clock.nowUtc()); + DomainResource reloadedDomain = + ofy().load().entity(domain).now().cloneProjectedAtTime(clock.nowUtc()); + assertThat(reloadedDomain.getSubordinateHosts()).isEmpty(); assertDnsTasksEnqueued("ns1.example.foo"); } - @Test - public void testSuccess_internalToDifferentInternal() throws Exception { - setEppHostUpdateInput( - "ns1.example.foo", - "ns2.example.tld", - "192.0.2.22", - "1080:0:0:0:8:800:200C:417A"); - createTld("foo"); - persistActiveDomain("example.foo"); - createTld("tld"); - DomainResource tldDomain = persistActiveDomain("example.tld"); - persistActiveHost(oldHostName()); - - assertThat(loadByForeignKey( - HostResource.class, oldHostName(), clock.nowUtc()).getCurrentSponsorClientId()) - .isEqualTo("TheRegistrar"); - assertThat( - loadByForeignKey( - DomainResource.class, "example.tld", clock.nowUtc()).getSubordinateHosts()) - .isEmpty(); - HostResource renamedHost = doSuccessfulTest(); - assertThat(renamedHost.getSuperordinateDomain()).isEqualTo(Key.create(tldDomain)); - assertThat(loadByForeignKey( - DomainResource.class, "example.tld", clock.nowUtc()).getSubordinateHosts()) - .containsExactly("ns2.example.tld"); - assertDnsTasksEnqueued("ns2.example.tld"); - // Ensure that the client id is read off the domain because this is a subordinate host now. - persistResource( - tldDomain.asBuilder().setCurrentSponsorClientId("it_should_be_this").build()); - assertThat( - renamedHost.cloneProjectedAtTime(clock.nowUtc().plusMinutes(1)).getCurrentSponsorClientId()) - .isEqualTo("it_should_be_this"); - } - @Test public void testSuccess_externalToInternal() throws Exception { setEppHostUpdateInput( @@ -321,28 +315,41 @@ public class HostUpdateFlowTest extends ResourceFlowTestCase hasLastSuperordinateChange(DateTime lastSuperordinateChange) { + return hasValue( + lastSuperordinateChange, + actual().getLastSuperordinateChange(), + "has lastSuperordinateChange"); + } + + public And hasSuperordinateDomain(Key superordinateDomain) { + return hasValue( + superordinateDomain, + actual().getSuperordinateDomain(), + "has superordinateDomain"); + } }