From 0935ba645006b412498cd5d3ae18fec1d3feee0a Mon Sep 17 00:00:00 2001 From: mcilwain Date: Mon, 27 Nov 2017 15:35:34 -0800 Subject: [PATCH] Revert poll message ID changes (going with a different approach) *** Reason for rollback *** Going with a safer approach to using fresh poll message IDs that doesn't mutate domains themselves. *** Original change description *** 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=177081870 --- .../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, 11 insertions(+), 134 deletions(-) delete mode 100644 javatests/google/registry/flows/testdata/poll_response_autorenew.xml delete 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 a1fd6ceae..90cd3bc74 100644 --- a/java/google/registry/flows/poll/PollAckFlow.java +++ b/java/google/registry/flows/poll/PollAckFlow.java @@ -18,7 +18,6 @@ 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,7 +31,6 @@ 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; @@ -107,23 +105,14 @@ public class PollAckFlow implements TransactionalFlow { // Move the eventTime of this autorenew poll message forward by a year. DateTime nextEventTime = autorenewPollMessage.getEventTime().plusYears(1); - // 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 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. if (nextEventTime.isBefore(autorenewPollMessage.getAutorenewEndTime())) { - 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); + ofy().save().entity(autorenewPollMessage.asBuilder().setEventTime(nextEventTime).build()); 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 e3d76755f..8d0ce7904 100644 --- a/javatests/google/registry/flows/EppLifecycleDomainTest.java +++ b/javatests/google/registry/flows/EppLifecycleDomainTest.java @@ -364,61 +364,6 @@ 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 c3eb0d253..5c0330b22 100644 --- a/javatests/google/registry/flows/poll/PollAckFlowTest.java +++ b/javatests/google/registry/flows/poll/PollAckFlowTest.java @@ -14,17 +14,13 @@ 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; @@ -33,7 +29,6 @@ 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; @@ -71,8 +66,8 @@ public class PollAckFlowTest extends FlowTestCase { .build()); } - private PollMessage.Autorenew persistAutorenewPollMessage(DateTime eventTime, DateTime endTime) { - return persistResource( + private void persistAutorenewPollMessage(DateTime eventTime, DateTime endTime) { + persistResource( new PollMessage.Autorenew.Builder() .setId(MESSAGE_ID) .setClientId(getClientIdForFlow()) @@ -114,25 +109,14 @@ public class PollAckFlowTest extends FlowTestCase { @Test public void testSuccess_recentActiveAutorenew() throws Exception { - PollMessage.Autorenew autorenew = - persistAutorenewPollMessage(clock.nowUtc().plusMonths(6), END_OF_TIME); + persistAutorenewPollMessage(clock.nowUtc().minusMonths(6), END_OF_TIME); assertTransactionalFlow(true); - 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); + runFlowAssertResponse(readFile("poll_ack_response_empty.xml")); } @Test public void testSuccess_oldActiveAutorenew() throws Exception { - 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); + persistAutorenewPollMessage(clock.nowUtc().minusYears(2), END_OF_TIME); // 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++) { @@ -140,14 +124,6 @@ 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 deleted file mode 100644 index d0e371056..000000000 --- a/javatests/google/registry/flows/testdata/poll_response_autorenew.xml +++ /dev/null @@ -1,22 +0,0 @@ - - - - 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 deleted file mode 100644 index 85911436a..000000000 --- a/javatests/google/registry/flows/testdata/poll_response_empty.xml +++ /dev/null @@ -1,11 +0,0 @@ - - - - Command completed successfully; no messages - - - ABC-12345 - server-trid - - -