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

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
+      // If the next event falls within the bounds of the end time, then just update the eventTime
-      // preserve global uniqueness of poll messages across all time) if there are more events to be
+      // and re-save it for future autorenew poll messages to be delivered. Otherwise, this
-      // delivered.
+      // autorenew poll message has no more events to deliver and should be deleted.
-      ofy().delete().entity(autorenewPollMessage);
       if (nextEventTime.isBefore(autorenewPollMessage.getAutorenewEndTime())) {
-        PollMessage.Autorenew newAutorenewPollMessage =
+        ofy().save().entity(autorenewPollMessage.asBuilder().setEventTime(nextEventTime).build());
-            autorenewPollMessage
-                .asBuilder()
-                .setId(allocateId())
-                .setEventTime(nextEventTime)
-                .build();
-        Key<DomainResource> 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

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.

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<PollAckFlow> {
             .build());
   }
 
-  private PollMessage.Autorenew persistAutorenewPollMessage(DateTime eventTime, DateTime endTime) {
+  private void persistAutorenewPollMessage(DateTime eventTime, DateTime endTime) {
-    return persistResource(
+    persistResource(
         new PollMessage.Autorenew.Builder()
             .setId(MESSAGE_ID)
             .setClientId(getClientIdForFlow())
@@ -114,25 +109,14 @@ public class PollAckFlowTest extends FlowTestCase<PollAckFlow> {
 
   @Test
   public void testSuccess_recentActiveAutorenew() throws Exception {
-    PollMessage.Autorenew autorenew =
+    persistAutorenewPollMessage(clock.nowUtc().minusMonths(6), END_OF_TIME);
-        persistAutorenewPollMessage(clock.nowUtc().plusMonths(6), END_OF_TIME);
     assertTransactionalFlow(true);
-    MessageDoesNotExistException e =
+    runFlowAssertResponse(readFile("poll_ack_response_empty.xml"));
-        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 {
-    DateTime autorenewDate = clock.nowUtc().minusYears(2);
+    persistAutorenewPollMessage(clock.nowUtc().minusYears(2), END_OF_TIME);
-    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++) {
@@ -140,14 +124,6 @@ public class PollAckFlowTest extends FlowTestCase<PollAckFlow> {
     }
     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<Autorenew> newPollMessageKey = ofy().load().entity(domain).now().getAutorenewPollMessage();
-    assertThat(ofy().load().key(newPollMessageKey).now().getEventTime())
-        .isEqualTo(autorenewDate.plusYears(1));
-    assertThat(newPollMessageKey.getId()).isNotEqualTo(autorenewId);
   }
 
   @Test

javatests/google/registry/flows/testdata/poll_response_autorenew.xml

@@ -1,22 +0,0 @@
-<epp xmlns="urn:ietf:params:xml:ns:epp-1.0">
-  <response>
-    <result code="1301">
-      <msg>Command completed successfully; ack to dequeue</msg>
-    </result>
-    <msgQ count="1" id="%ID%">
-      <qDate>%QDATE%</qDate>
-      <msg>Domain was auto-renewed.</msg>
-    </msgQ>
-    <resData>
-      <domain:renData
-       xmlns:domain="urn:ietf:params:xml:ns:domain-1.0">
-        <domain:name>%DOMAIN%</domain:name>
-        <domain:exDate>%EXDATE%</domain:exDate>
-      </domain:renData>
-    </resData>
-    <trID>
-      <clTRID>ABC-12345</clTRID>
-      <svTRID>server-trid</svTRID>
-    </trID>
-  </response>
-</epp>

javatests/google/registry/flows/testdata/poll_response_empty.xml

@@ -1,11 +0,0 @@
-<epp xmlns="urn:ietf:params:xml:ns:epp-1.0">
-  <response>
-    <result code="1300">
-      <msg>Command completed successfully; no messages</msg>
-    </result>
-    <trID>
-      <clTRID>ABC-12345</clTRID>
-      <svTRID>server-trid</svTRID>
-    </trID>
-  </response>
-</epp>