From 603e0470cc823e054279ed23416085b53073df58 Mon Sep 17 00:00:00 2001 From: mcilwain Date: Fri, 17 Nov 2017 13:03:52 -0800 Subject: [PATCH] Use PollMessage IDs that are globally unique across all time The previous functionality was reusing the same PollMessage ID for Autorenews every year. This can potentially cause confusion at registrars if they were expecting these to be globall unique across all time. So this change simply changes the ID during autorenew. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=176149220 --- .../registry/flows/poll/PollAckFlow.java | 23 ++++++-- .../flows/EppLifecycleDomainTest.java | 55 +++++++++++++++++++ .../registry/flows/poll/PollAckFlowTest.java | 34 ++++++++++-- .../testdata/poll_response_autorenew.xml | 22 ++++++++ .../flows/testdata/poll_response_empty.xml | 11 ++++ 5 files changed, 134 insertions(+), 11 deletions(-) create mode 100644 javatests/google/registry/flows/testdata/poll_response_autorenew.xml create mode 100644 javatests/google/registry/flows/testdata/poll_response_empty.xml diff --git a/java/google/registry/flows/poll/PollAckFlow.java b/java/google/registry/flows/poll/PollAckFlow.java index ee0383fa2..9d0d55f42 100644 --- a/java/google/registry/flows/poll/PollAckFlow.java +++ b/java/google/registry/flows/poll/PollAckFlow.java @@ -18,6 +18,7 @@ import static com.google.common.base.Preconditions.checkState; import static google.registry.flows.FlowUtils.validateClientIsLoggedIn; import static google.registry.flows.poll.PollFlowUtils.getPollMessagesQuery; import static google.registry.model.eppoutput.Result.Code.SUCCESS_WITH_NO_MESSAGES; +import static google.registry.model.ofy.ObjectifyService.allocateId; import static google.registry.model.ofy.ObjectifyService.ofy; import static google.registry.util.DateTimeUtils.isBeforeOrAt; @@ -32,6 +33,7 @@ import google.registry.flows.ExtensionManager; import google.registry.flows.FlowModule.ClientId; import google.registry.flows.FlowModule.PollMessageId; import google.registry.flows.TransactionalFlow; +import google.registry.model.domain.DomainResource; import google.registry.model.eppoutput.EppResponse; import google.registry.model.poll.MessageQueueInfo; import google.registry.model.poll.PollMessage; @@ -106,14 +108,23 @@ public class PollAckFlow implements TransactionalFlow { // Move the eventTime of this autorenew poll message forward by a year. DateTime nextEventTime = autorenewPollMessage.getEventTime().plusYears(1); - // If the next event falls within the bounds of the end time, then just update the eventTime - // and re-save it for future autorenew poll messages to be delivered. Otherwise, this - // autorenew poll message has no more events to deliver and should be deleted. + // Delete the existing poll message, then optionally create a new one (with a different ID, to + // preserve global uniqueness of poll messages across all time) if there are more events to be + // delivered. + ofy().delete().entity(autorenewPollMessage); if (nextEventTime.isBefore(autorenewPollMessage.getAutorenewEndTime())) { - ofy().save().entity(autorenewPollMessage.asBuilder().setEventTime(nextEventTime).build()); + PollMessage.Autorenew newAutorenewPollMessage = + autorenewPollMessage + .asBuilder() + .setId(allocateId()) + .setEventTime(nextEventTime) + .build(); + Key domainKey = autorenewPollMessage.getParentKey().getParent(); + DomainResource domain = ofy().load().key(domainKey).now(); + DomainResource updatedDomain = + domain.asBuilder().setAutorenewPollMessage(Key.create(newAutorenewPollMessage)).build(); + ofy().save().entities(newAutorenewPollMessage, updatedDomain); includeAckedMessageInCount = isBeforeOrAt(nextEventTime, now); - } else { - ofy().delete().entity(autorenewPollMessage); } } // We need to return the new queue length. If this was the last message in the queue being diff --git a/javatests/google/registry/flows/EppLifecycleDomainTest.java b/javatests/google/registry/flows/EppLifecycleDomainTest.java index 8d0ce7904..e3d76755f 100644 --- a/javatests/google/registry/flows/EppLifecycleDomainTest.java +++ b/javatests/google/registry/flows/EppLifecycleDomainTest.java @@ -364,6 +364,61 @@ public class EppLifecycleDomainTest extends EppTestCase { assertCommandAndResponse("logout.xml", "logout_response.xml"); } + @Test + public void testDomainCreate_annualAutoRenewPollMessagesAreSent() throws Exception { + assertCommandAndResponse("login_valid.xml", "login_response.xml"); + // Create the domain. + createFakesite(); + + // The first autorenew poll message isn't seen until after the initial two years of registration + // are up. + assertCommandAndResponse( + "poll.xml", "poll_response_empty.xml", DateTime.parse("2001-01-01T00:01:00Z")); + assertCommandAndResponse( + "poll.xml", + ImmutableMap.of(), + "poll_response_autorenew.xml", + ImmutableMap.of( + "ID", "1-C-EXAMPLE-13-16", + "QDATE", "2002-06-01T00:04:00Z", + "DOMAIN", "fakesite.example", + "EXDATE", "2003-06-01T00:04:00Z"), + DateTime.parse("2002-07-01T00:01:00Z")); + assertCommandAndResponse( + "poll_ack.xml", + ImmutableMap.of("ID", "1-C-EXAMPLE-13-16"), + "poll_ack_response_empty.xml", + null, + DateTime.parse("2002-07-01T00:02:00Z")); + + // The second autorenew poll message isn't seen until after another year, and it should have a + // different ID. + assertCommandAndResponse( + "poll.xml", "poll_response_empty.xml", DateTime.parse("2002-07-01T00:05:00Z")); + assertCommandAndResponse( + "poll.xml", + ImmutableMap.of(), + "poll_response_autorenew.xml", + ImmutableMap.of( + "ID", "1-C-EXAMPLE-13-17", // Note -- different than the previous ID. + "QDATE", "2003-06-01T00:04:00Z", + "DOMAIN", "fakesite.example", + "EXDATE", "2004-06-01T00:04:00Z"), + DateTime.parse("2003-07-01T00:05:00Z")); + + // Ack the second poll message and verify that none remain. + assertCommandAndResponse( + "poll_ack.xml", + ImmutableMap.of("ID", "1-C-EXAMPLE-13-17"), + "poll_ack_response_empty.xml", + null, + DateTime.parse("2003-07-01T00:05:05Z")); + assertCommandAndResponse( + "poll.xml", "poll_response_empty.xml", DateTime.parse("2003-07-01T00:05:10Z")); + + assertCommandAndResponse("logout.xml", "logout_response.xml"); + } + @Test public void testDomainTransferPollMessage_serverApproved() throws Exception { // As the losing registrar, create the domain. diff --git a/javatests/google/registry/flows/poll/PollAckFlowTest.java b/javatests/google/registry/flows/poll/PollAckFlowTest.java index 5c0330b22..c3eb0d253 100644 --- a/javatests/google/registry/flows/poll/PollAckFlowTest.java +++ b/javatests/google/registry/flows/poll/PollAckFlowTest.java @@ -14,13 +14,17 @@ package google.registry.flows.poll; +import static com.google.common.truth.Truth.assertThat; +import static google.registry.model.ofy.ObjectifyService.ofy; import static google.registry.testing.DatastoreHelper.createHistoryEntryForEppResource; import static google.registry.testing.DatastoreHelper.createTld; import static google.registry.testing.DatastoreHelper.newDomainResource; import static google.registry.testing.DatastoreHelper.persistActiveContact; import static google.registry.testing.DatastoreHelper.persistResource; +import static google.registry.testing.JUnitBackports.expectThrows; import static google.registry.util.DateTimeUtils.END_OF_TIME; +import com.googlecode.objectify.Key; import google.registry.flows.FlowTestCase; import google.registry.flows.poll.PollAckFlow.InvalidMessageIdException; import google.registry.flows.poll.PollAckFlow.MessageDoesNotExistException; @@ -29,6 +33,7 @@ import google.registry.flows.poll.PollAckFlow.NotAuthorizedToAckMessageException import google.registry.model.contact.ContactResource; import google.registry.model.domain.DomainResource; import google.registry.model.poll.PollMessage; +import google.registry.model.poll.PollMessage.Autorenew; import org.joda.time.DateTime; import org.junit.Before; import org.junit.Test; @@ -66,8 +71,8 @@ public class PollAckFlowTest extends FlowTestCase { .build()); } - private void persistAutorenewPollMessage(DateTime eventTime, DateTime endTime) { - persistResource( + private PollMessage.Autorenew persistAutorenewPollMessage(DateTime eventTime, DateTime endTime) { + return persistResource( new PollMessage.Autorenew.Builder() .setId(MESSAGE_ID) .setClientId(getClientIdForFlow()) @@ -109,14 +114,25 @@ public class PollAckFlowTest extends FlowTestCase { @Test public void testSuccess_recentActiveAutorenew() throws Exception { - persistAutorenewPollMessage(clock.nowUtc().minusMonths(6), END_OF_TIME); + PollMessage.Autorenew autorenew = + persistAutorenewPollMessage(clock.nowUtc().plusMonths(6), END_OF_TIME); assertTransactionalFlow(true); - runFlowAssertResponse(readFile("poll_ack_response_empty.xml")); + MessageDoesNotExistException e = + expectThrows( + MessageDoesNotExistException.class, + () -> runFlowAssertResponse(readFile("poll_ack_response_empty.xml"))); + assertThat(e).hasMessageThat().contains("given ID (1-3-EXAMPLE-4-3) doesn't exist"); + // No changes should have been made to the autorenew since it wasn't delivered yet. + assertThat(ofy().load().entity(autorenew).now()).isEqualTo(autorenew); } @Test public void testSuccess_oldActiveAutorenew() throws Exception { - persistAutorenewPollMessage(clock.nowUtc().minusYears(2), END_OF_TIME); + DateTime autorenewDate = clock.nowUtc().minusYears(2); + PollMessage.Autorenew autorenew = + persistAutorenewPollMessage(autorenewDate, END_OF_TIME); + Long autorenewId = autorenew.getId(); + assertThat(ofy().load().entity(autorenew).now().getEventTime()).isEqualTo(autorenewDate); // Create three other messages to be queued for retrieval to get our count right, since the poll // ack response wants there to be 4 messages in the queue when the ack comes back. for (int i = 1; i < 4; i++) { @@ -124,6 +140,14 @@ public class PollAckFlowTest extends FlowTestCase { } assertTransactionalFlow(true); runFlowAssertResponse(readFile("poll_ack_response.xml")); + // The previous autorenew should have been deleted. + assertThat(ofy().load().entity(autorenew).now()).isNull(); + // Check that the domain now points to a new autorenew, with a different id, that will be + // delivered one year after the previous one. + Key newPollMessageKey = ofy().load().entity(domain).now().getAutorenewPollMessage(); + assertThat(ofy().load().key(newPollMessageKey).now().getEventTime()) + .isEqualTo(autorenewDate.plusYears(1)); + assertThat(newPollMessageKey.getId()).isNotEqualTo(autorenewId); } @Test diff --git a/javatests/google/registry/flows/testdata/poll_response_autorenew.xml b/javatests/google/registry/flows/testdata/poll_response_autorenew.xml new file mode 100644 index 000000000..d0e371056 --- /dev/null +++ b/javatests/google/registry/flows/testdata/poll_response_autorenew.xml @@ -0,0 +1,22 @@ + + + + Command completed successfully; ack to dequeue + + + %QDATE% + Domain was auto-renewed. + + + + %DOMAIN% + %EXDATE% + + + + ABC-12345 + server-trid + + + diff --git a/javatests/google/registry/flows/testdata/poll_response_empty.xml b/javatests/google/registry/flows/testdata/poll_response_empty.xml new file mode 100644 index 000000000..85911436a --- /dev/null +++ b/javatests/google/registry/flows/testdata/poll_response_empty.xml @@ -0,0 +1,11 @@ + + + + Command completed successfully; no messages + + + ABC-12345 + server-trid + + +