From 184b2b56ac2c4dc210c123ae30cd4544bb19d822 Mon Sep 17 00:00:00 2001 From: nickfelt Date: Wed, 11 Oct 2017 12:23:32 -0700 Subject: [PATCH] Persist transferredRegistrationExpirationTime (exDate) on TransferData This CL adds transferredRegistrationExpirationTime as a TransferData field persisted to Datastore. It's only relevant for domains, and it represents the registration expiration time resulting from the approval of the most recent transfer request. For pending transfers, we assume the transfer will be server-approved, and thus in DomainTransferRequestFlow we set this field to the existing computed value serverApproveNewExpirationTime, which is what we use for setting up the server-approve autorenew billing event and poll message. In DomainTransferApproveFlow we overwrite this field with the freshly computed newExpirationTime, whereas in DomainTransferCancel/RejectFlow (and in the implicit cancel of DomainDeleteFlow during a pending transfer) we null it out. There are two key benefits to having this field, which are described in more detail in b/36405140. 1) b/25084229 - it allows storage of a frozen value to back the "exDate" field of DomainTransferResponse, which we can use to fix various errors with how exDate display currently works. 2) b/36354434 - it allows DomainResource.cloneProjectedAtTime() to just directly set the registrationExpirationTime to this value, without computing it de novo, which reduces duplicated logic and ensures that the new expiration time matches the autorenew child objects. This CL only starts writing the field on TransferData as persisted directly on the DomainResource itself. We'll then want to backfill the field for at least pending transfers, whether expired or not (so we can do (2) above), but I think we might as well backfill it for all pending and approved transfers so that we also fix (1) even for historical transfers. And then we can start actually reading the field for both purposes. (Note that for (1), this will only fix synchronous transfer responses served via DomainTransferQueryFlow, not async transfer responses served via poll messages, since these have already been persisted with a potentially bad exDate, but I don't think it's worth a backfill for those). One last naming note: I chose the verbose transferredRegistrationExpirationTime rather than the extendedRegistrationExpirationTime of DomainTransferResponse because (as is the case in autorenew grace, or for a superuser transfer) the new registration time isn't necessarily extended at all; it may be the same as the pre-transfer expiration time. Also, including "registration" helps clarify w.r.t. pendingTransferExpirationTime which refers confusingly to the expiry of the transfer itself, rather than the domain registration. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=171858083 --- .../domain/DomainTransferApproveFlow.java | 11 +- .../domain/DomainTransferRequestFlow.java | 19 ++- .../flows/domain/DomainTransferUtils.java | 24 ++-- .../registry/model/transfer/TransferData.java | 108 ++++++++++++------ .../imports/XjcToDomainResourceConverter.java | 2 +- .../domain/DomainTransferApproveFlowTest.java | 1 + .../domain/DomainTransferCancelFlowTest.java | 6 +- .../domain/DomainTransferRequestFlowTest.java | 13 ++- .../model/domain/DomainResourceTest.java | 1 + javatests/google/registry/model/schema.txt | 1 + .../registry/rdap/RdapJsonFormatterTest.java | 2 + .../registry/testing/DatastoreHelper.java | 28 +++-- 12 files changed, 132 insertions(+), 84 deletions(-) diff --git a/java/google/registry/flows/domain/DomainTransferApproveFlow.java b/java/google/registry/flows/domain/DomainTransferApproveFlow.java index 69e78b2d4..a7be9b598 100644 --- a/java/google/registry/flows/domain/DomainTransferApproveFlow.java +++ b/java/google/registry/flows/domain/DomainTransferApproveFlow.java @@ -172,9 +172,18 @@ public final class DomainTransferApproveFlow implements TransactionalFlow { .setMsg("Domain was auto-renewed.") .setParent(historyEntry) .build(); + // Construct the post-transfer domain. + DomainResource partiallyApprovedDomain = + approvePendingTransfer(existingDomain, TransferStatus.CLIENT_APPROVED, now); DomainResource newDomain = - approvePendingTransfer(existingDomain, TransferStatus.CLIENT_APPROVED, now) + partiallyApprovedDomain .asBuilder() + // Update the transferredRegistrationExpirationTime here since approvePendingTransfer() + // doesn't know what to set it to and leaves it null. + .setTransferData( + partiallyApprovedDomain.getTransferData().asBuilder() + .setTransferredRegistrationExpirationTime(newExpirationTime) + .build()) .setRegistrationExpirationTime(newExpirationTime) .setAutorenewBillingEvent(Key.create(autorenewEvent)) .setAutorenewPollMessage(Key.create(gainingClientAutorenewPollMessage)) diff --git a/java/google/registry/flows/domain/DomainTransferRequestFlow.java b/java/google/registry/flows/domain/DomainTransferRequestFlow.java index 588dd6ceb..bd33cea82 100644 --- a/java/google/registry/flows/domain/DomainTransferRequestFlow.java +++ b/java/google/registry/flows/domain/DomainTransferRequestFlow.java @@ -69,7 +69,6 @@ import google.registry.model.reporting.DomainTransactionRecord.TransactionReport import google.registry.model.reporting.HistoryEntry; import google.registry.model.reporting.IcannReportingTypes.ActivityReportField; import google.registry.model.transfer.TransferData; -import google.registry.model.transfer.TransferData.Builder; import google.registry.model.transfer.TransferData.TransferServerApproveEntity; import google.registry.model.transfer.TransferResponse.DomainTransferResponse; import google.registry.model.transfer.TransferStatus; @@ -206,7 +205,13 @@ public final class DomainTransferRequestFlow implements TransactionalFlow { // Create the transfer data that represents the pending transfer. TransferData pendingTransferData = createPendingTransferData( - createTransferDataBuilder(existingDomain, automaticTransferTime, now), + new TransferData.Builder() + .setTransferRequestTrid(trid) + .setTransferRequestTime(now) + .setGainingClientId(gainingClientId) + .setLosingClientId(existingDomain.getCurrentSponsorClientId()) + .setPendingTransferExpirationTime(automaticTransferTime) + .setTransferredRegistrationExpirationTime(serverApproveNewExpirationTime), serverApproveEntities, period); // Create a poll message to notify the losing registrar that a transfer was requested. @@ -316,16 +321,6 @@ public final class DomainTransferRequestFlow implements TransactionalFlow { .build(); } - private Builder createTransferDataBuilder( - DomainResource existingDomain, DateTime automaticTransferTime, DateTime now) { - return new TransferData.Builder() - .setTransferRequestTrid(trid) - .setTransferRequestTime(now) - .setGainingClientId(gainingClientId) - .setLosingClientId(existingDomain.getCurrentSponsorClientId()) - .setPendingTransferExpirationTime(automaticTransferTime); - } - private DomainTransferResponse createResponse( Period period, DomainResource existingDomain, DomainResource newDomain, DateTime now) { // If the registration were approved this instant, this is what the new expiration would be, diff --git a/java/google/registry/flows/domain/DomainTransferUtils.java b/java/google/registry/flows/domain/DomainTransferUtils.java index 9b977055a..697f4312b 100644 --- a/java/google/registry/flows/domain/DomainTransferUtils.java +++ b/java/google/registry/flows/domain/DomainTransferUtils.java @@ -35,7 +35,6 @@ import google.registry.model.poll.PollMessage; import google.registry.model.registry.Registry; import google.registry.model.reporting.HistoryEntry; import google.registry.model.transfer.TransferData; -import google.registry.model.transfer.TransferData.Builder; import google.registry.model.transfer.TransferData.TransferServerApproveEntity; import google.registry.model.transfer.TransferResponse.DomainTransferResponse; import google.registry.model.transfer.TransferStatus; @@ -115,8 +114,13 @@ public final class DomainTransferUtils { String targetId = existingDomain.getFullyQualifiedDomainName(); // Create a TransferData for the server-approve case to use for the speculative poll messages. TransferData serverApproveTransferData = - createTransferDataBuilder( - existingDomain, trid, gainingClientId, automaticTransferTime, now) + new TransferData.Builder() + .setTransferRequestTrid(trid) + .setTransferRequestTime(now) + .setGainingClientId(gainingClientId) + .setLosingClientId(existingDomain.getCurrentSponsorClientId()) + .setPendingTransferExpirationTime(automaticTransferTime) + .setTransferredRegistrationExpirationTime(serverApproveNewExpirationTime) .setTransferStatus(TransferStatus.SERVER_APPROVED) .build(); Registry registry = Registry.get(existingDomain.getTld()); @@ -291,19 +295,5 @@ public final class DomainTransferUtils { .build(); } - private static Builder createTransferDataBuilder( - DomainResource existingDomain, - Trid trid, - String gainingClientId, - DateTime automaticTransferTime, - DateTime now) { - return new TransferData.Builder() - .setTransferRequestTrid(trid) - .setTransferRequestTime(now) - .setGainingClientId(gainingClientId) - .setLosingClientId(existingDomain.getCurrentSponsorClientId()) - .setPendingTransferExpirationTime(automaticTransferTime); - } - private DomainTransferUtils() {} } diff --git a/java/google/registry/model/transfer/TransferData.java b/java/google/registry/model/transfer/TransferData.java index 97e75df71..d2dd5aeeb 100644 --- a/java/google/registry/model/transfer/TransferData.java +++ b/java/google/registry/model/transfer/TransferData.java @@ -30,6 +30,8 @@ import google.registry.model.domain.Period.Unit; import google.registry.model.eppcommon.Trid; import google.registry.model.poll.PollMessage; import java.util.Set; +import javax.annotation.Nullable; +import org.joda.time.DateTime; /** * Common transfer data for {@link EppResource} types. Only applies to domains and contacts; @@ -41,6 +43,34 @@ public class TransferData extends BaseTransferObject implements Buildable { public static final TransferData EMPTY = new TransferData(); + /** The transaction id of the most recent transfer request (or null if there never was one). */ + Trid transferRequestTrid; + + /** + * The period to extend the registration upon completion of the transfer. + * + *

By default, domain transfers are for one year. This can be changed to zero by using the + * superuser EPP extension. + */ + Period transferPeriod = Period.create(1, Unit.YEARS); + + /** + * The registration expiration time resulting from the approval - speculative or actual - of the + * most recent transfer request, applicable for domains only. + * + *

For pending transfers, this is the expiration time that will take effect under a projected + * server approval. For approved transfers, this is the actual expiration time of the domain as + * of the moment of transfer completion. For rejected or cancelled transfers, this field will be + * reset to null. + * + *

Note that even when this field is set, it does not necessarily mean that the post-transfer + * domain has a new expiration time. Superuser transfers may not include a bundled 1 year renewal + * at all, or even when a renewal is bundled, for a transfer during the autorenew grace period the + * bundled renewal simply subsumes the recent autorenewal, resulting in the same expiration time. + */ + // TODO(b/36405140): backfill this field for existing domains to which it should apply. + DateTime transferredRegistrationExpirationTime; + /** * The billing event and poll messages associated with a server-approved transfer. * @@ -80,33 +110,7 @@ public class TransferData extends BaseTransferObject implements Buildable { @IgnoreSave(IfNull.class) Key serverApproveAutorenewPollMessage; - /** The transaction id of the most recent transfer request (or null if there never was one). */ - Trid transferRequestTrid; - - /** - * The period to extend the registration upon completion of the transfer. - * - *

By default, domain transfers are for one year. This can be changed to zero by using the - * superuser EPP extension. - */ - Period transferPeriod = Period.create(1, Unit.YEARS); - - public ImmutableSet> getServerApproveEntities() { - return nullToEmptyImmutableCopy(serverApproveEntities); - } - - public Key getServerApproveBillingEvent() { - return serverApproveBillingEvent; - } - - public Key getServerApproveAutorenewEvent() { - return serverApproveAutorenewEvent; - } - - public Key getServerApproveAutorenewPollMessage() { - return serverApproveAutorenewPollMessage; - } - + @Nullable public Trid getTransferRequestTrid() { return transferRequestTrid; } @@ -115,6 +119,30 @@ public class TransferData extends BaseTransferObject implements Buildable { return transferPeriod; } + @Nullable + public DateTime getTransferredRegistrationExpirationTime() { + return transferredRegistrationExpirationTime; + } + + public ImmutableSet> getServerApproveEntities() { + return nullToEmptyImmutableCopy(serverApproveEntities); + } + + @Nullable + public Key getServerApproveBillingEvent() { + return serverApproveBillingEvent; + } + + @Nullable + public Key getServerApproveAutorenewEvent() { + return serverApproveAutorenewEvent; + } + + @Nullable + public Key getServerApproveAutorenewPollMessage() { + return serverApproveAutorenewPollMessage; + } + @Override public Builder asBuilder() { return new Builder(clone(this)); @@ -153,6 +181,22 @@ public class TransferData extends BaseTransferObject implements Buildable { super(instance); } + public Builder setTransferRequestTrid(Trid transferRequestTrid) { + getInstance().transferRequestTrid = transferRequestTrid; + return this; + } + + public Builder setTransferPeriod(Period transferPeriod) { + getInstance().transferPeriod = transferPeriod; + return this; + } + + public Builder setTransferredRegistrationExpirationTime( + DateTime transferredRegistrationExpirationTime) { + getInstance().transferredRegistrationExpirationTime = transferredRegistrationExpirationTime; + return this; + } + public Builder setServerApproveEntities( ImmutableSet> serverApproveEntities) { getInstance().serverApproveEntities = serverApproveEntities; @@ -176,16 +220,6 @@ public class TransferData extends BaseTransferObject implements Buildable { getInstance().serverApproveAutorenewPollMessage = serverApproveAutorenewPollMessage; return this; } - - public Builder setTransferRequestTrid(Trid transferRequestTrid) { - getInstance().transferRequestTrid = transferRequestTrid; - return this; - } - - public Builder setTransferPeriod(Period transferPeriod) { - getInstance().transferPeriod = transferPeriod; - return this; - } } /** diff --git a/java/google/registry/rde/imports/XjcToDomainResourceConverter.java b/java/google/registry/rde/imports/XjcToDomainResourceConverter.java index 7838df602..3fac19418 100644 --- a/java/google/registry/rde/imports/XjcToDomainResourceConverter.java +++ b/java/google/registry/rde/imports/XjcToDomainResourceConverter.java @@ -236,13 +236,13 @@ final class XjcToDomainResourceConverter extends XjcToEppResourceConverter { if (data == null) { return TransferData.EMPTY; } - // TODO(b/25084229): read in the exDate and store it somewhere. return new TransferData.Builder() .setTransferStatus(TRANSFER_STATUS_MAPPER.xmlToEnum(data.getTrStatus().value())) .setGainingClientId(data.getReRr().getValue()) .setLosingClientId(data.getAcRr().getValue()) .setTransferRequestTime(data.getReDate()) .setPendingTransferExpirationTime(data.getAcDate()) + .setTransferredRegistrationExpirationTime(data.getExDate()) .build(); } diff --git a/javatests/google/registry/flows/domain/DomainTransferApproveFlowTest.java b/javatests/google/registry/flows/domain/DomainTransferApproveFlowTest.java index 8cf5211c4..433e360db 100644 --- a/javatests/google/registry/flows/domain/DomainTransferApproveFlowTest.java +++ b/javatests/google/registry/flows/domain/DomainTransferApproveFlowTest.java @@ -116,6 +116,7 @@ public class DomainTransferApproveFlowTest oldTransferData.copyConstantFieldsToBuilder() .setTransferStatus(TransferStatus.CLIENT_APPROVED) .setPendingTransferExpirationTime(clock.nowUtc()) + .setTransferredRegistrationExpirationTime(domain.getRegistrationExpirationTime()) .build()); } diff --git a/javatests/google/registry/flows/domain/DomainTransferCancelFlowTest.java b/javatests/google/registry/flows/domain/DomainTransferCancelFlowTest.java index 07c4502ff..c027e754b 100644 --- a/javatests/google/registry/flows/domain/DomainTransferCancelFlowTest.java +++ b/javatests/google/registry/flows/domain/DomainTransferCancelFlowTest.java @@ -98,7 +98,8 @@ public class DomainTransferCancelFlowTest "NewRegistrar", TRANSFER_REQUEST_TIME, TRANSFER_EXPIRATION_TIME, - TRANSFER_REQUEST_TIME)); + TRANSFER_REQUEST_TIME, + EXTENDED_REGISTRATION_EXPIRATION_TIME)); assertPollMessages( "TheRegistrar", createPollMessageForImplicitTransfer( @@ -107,7 +108,8 @@ public class DomainTransferCancelFlowTest "TheRegistrar", TRANSFER_REQUEST_TIME, TRANSFER_EXPIRATION_TIME, - TRANSFER_REQUEST_TIME)); + TRANSFER_REQUEST_TIME, + EXTENDED_REGISTRATION_EXPIRATION_TIME)); clock.advanceOneMilli(); // Setup done; run the test. diff --git a/javatests/google/registry/flows/domain/DomainTransferRequestFlowTest.java b/javatests/google/registry/flows/domain/DomainTransferRequestFlowTest.java index 187399cc2..aeb729e0f 100644 --- a/javatests/google/registry/flows/domain/DomainTransferRequestFlowTest.java +++ b/javatests/google/registry/flows/domain/DomainTransferRequestFlowTest.java @@ -130,8 +130,10 @@ public class DomainTransferRequestFlowTest } private void assertTransferRequested( - DomainResource domain, DateTime automaticTransferTime, Period expectedPeriod) - throws Exception { + DomainResource domain, + DateTime automaticTransferTime, + Period expectedPeriod, + DateTime expectedExpirationTime) throws Exception { assertAboutDomains().that(domain) .hasCurrentSponsorClientId("TheRegistrar").and() .hasStatusValue(StatusValue.PENDING_TRANSFER); @@ -152,6 +154,7 @@ public class DomainTransferRequestFlowTest .setTransferPeriod(expectedPeriod) .setTransferStatus(TransferStatus.PENDING) .setPendingTransferExpirationTime(automaticTransferTime) + .setTransferredRegistrationExpirationTime(expectedExpirationTime) // Don't compare the server-approve entity fields; they're hard to reconstruct // and logic later will check them. .build()); @@ -178,6 +181,7 @@ public class DomainTransferRequestFlowTest .setTransferPeriod(expectedPeriod) .setTransferStatus(TransferStatus.SERVER_APPROVED) .setPendingTransferExpirationTime(automaticTransferTime) + .setTransferredRegistrationExpirationTime(domain.getRegistrationExpirationTime()) // Server-approve entity fields should all be nulled out. .build()); } @@ -219,7 +223,8 @@ public class DomainTransferRequestFlowTest .and() .hasOtherClientId("TheRegistrar"); // Verify correct fields were set. - assertTransferRequested(domain, implicitTransferTime, Period.create(1, Unit.YEARS)); + assertTransferRequested( + domain, implicitTransferTime, Period.create(1, Unit.YEARS), expectedExpirationTime); subordinateHost = reloadResourceAndCloneAtTime(subordinateHost, clock.nowUtc()); assertAboutHosts().that(subordinateHost).hasNoHistoryEntries(); @@ -530,7 +535,7 @@ public class DomainTransferRequestFlowTest .and() .hasOtherClientId("TheRegistrar"); // Verify correct fields were set. - assertTransferRequested(domain, implicitTransferTime, expectedPeriod); + assertTransferRequested(domain, implicitTransferTime, expectedPeriod, expectedExpirationTime); subordinateHost = reloadResourceAndCloneAtTime(subordinateHost, clock.nowUtc()); assertAboutHosts().that(subordinateHost).hasNoHistoryEntries(); diff --git a/javatests/google/registry/model/domain/DomainResourceTest.java b/javatests/google/registry/model/domain/DomainResourceTest.java index 031b8ea8f..5f1e55a7c 100644 --- a/javatests/google/registry/model/domain/DomainResourceTest.java +++ b/javatests/google/registry/model/domain/DomainResourceTest.java @@ -139,6 +139,7 @@ public class DomainResourceTest extends EntityTestCase { .setTransferRequestTime(clock.nowUtc().plusDays(1)) .setTransferStatus(TransferStatus.SERVER_APPROVED) .setTransferRequestTrid(Trid.create("client-trid", "server-trid")) + .setTransferredRegistrationExpirationTime(clock.nowUtc().plusYears(2)) .build()) .setDeletePollMessage(onetimePollKey) .setAutorenewBillingEvent(recurringBillKey) diff --git a/javatests/google/registry/model/schema.txt b/javatests/google/registry/model/schema.txt index 327f2a4f9..8897d7620 100644 --- a/javatests/google/registry/model/schema.txt +++ b/javatests/google/registry/model/schema.txt @@ -913,6 +913,7 @@ class google.registry.model.transfer.TransferData { java.util.Set> serverApproveEntities; org.joda.time.DateTime pendingTransferExpirationTime; org.joda.time.DateTime transferRequestTime; + org.joda.time.DateTime transferredRegistrationExpirationTime; } class google.registry.model.transfer.TransferResponse$ContactTransferResponse { google.registry.model.transfer.TransferStatus transferStatus; diff --git a/javatests/google/registry/rdap/RdapJsonFormatterTest.java b/javatests/google/registry/rdap/RdapJsonFormatterTest.java index e196f8205..2def5aaf3 100644 --- a/javatests/google/registry/rdap/RdapJsonFormatterTest.java +++ b/javatests/google/registry/rdap/RdapJsonFormatterTest.java @@ -164,6 +164,8 @@ public class RdapJsonFormatterTest { .setTransferRequestTime(clock.nowUtc().minusDays(1)) .setLosingClientId("TheRegistrar") .setPendingTransferExpirationTime(clock.nowUtc().plusYears(100)) + .setTransferredRegistrationExpirationTime( + DateTime.parse("2111-10-08T00:44:59Z")) .build()) .build()))) .build()); diff --git a/javatests/google/registry/testing/DatastoreHelper.java b/javatests/google/registry/testing/DatastoreHelper.java index 87e0fc468..929e6cceb 100644 --- a/javatests/google/registry/testing/DatastoreHelper.java +++ b/javatests/google/registry/testing/DatastoreHelper.java @@ -103,6 +103,7 @@ import google.registry.model.transfer.TransferStatus; import google.registry.tmch.LordnTask; import java.util.Arrays; import java.util.List; +import javax.annotation.Nullable; import org.joda.money.Money; import org.joda.time.DateTime; @@ -457,20 +458,22 @@ public class DatastoreHelper { } public static PollMessage.OneTime createPollMessageForImplicitTransfer( - EppResource contact, + EppResource resource, HistoryEntry historyEntry, String clientId, DateTime requestTime, DateTime expirationTime, - DateTime now) { + DateTime now, + @Nullable DateTime extendedRegistrationExpirationTime) { + TransferData transferData = + createTransferDataBuilder(requestTime, expirationTime) + .setTransferredRegistrationExpirationTime(extendedRegistrationExpirationTime) + .build(); return new PollMessage.OneTime.Builder() .setClientId(clientId) .setEventTime(expirationTime) .setMsg("Transfer server approved.") - .setResponseData(ImmutableList.of( - createTransferResponse(contact, createTransferDataBuilder(requestTime, expirationTime) - .build(), - now))) + .setResponseData(ImmutableList.of(createTransferResponse(resource, transferData, now))) .setParent(historyEntry) .build(); } @@ -519,7 +522,8 @@ public class DatastoreHelper { "NewRegistrar", requestTime, expirationTime, - now))), + now, + null))), Key.create(persistResource( createPollMessageForImplicitTransfer( contact, @@ -527,7 +531,8 @@ public class DatastoreHelper { "TheRegistrar", requestTime, expirationTime, - now))))) + now, + null))))) .setTransferRequestTrid(Trid.create("transferClient-trid", "transferServer-trid")) .build()) .build()); @@ -591,6 +596,7 @@ public class DatastoreHelper { .addStatusValue(StatusValue.PENDING_TRANSFER) .setTransferData(transferDataBuilder .setPendingTransferExpirationTime(expirationTime) + .setTransferredRegistrationExpirationTime(extendedRegistrationExpirationTime) .setServerApproveBillingEvent(Key.create(transferBillingEvent)) .setServerApproveAutorenewEvent(Key.create(gainingClientAutorenewEvent)) .setServerApproveAutorenewPollMessage(Key.create(gainingClientAutorenewPollMessage)) @@ -605,7 +611,8 @@ public class DatastoreHelper { "NewRegistrar", requestTime, expirationTime, - now))), + now, + extendedRegistrationExpirationTime))), Key.create(persistResource( createPollMessageForImplicitTransfer( domain, @@ -613,7 +620,8 @@ public class DatastoreHelper { "TheRegistrar", requestTime, expirationTime, - now))))) + now, + extendedRegistrationExpirationTime))))) .setTransferRequestTrid(Trid.create("transferClient-trid", "transferServer-trid")) .build()) .build());