Restore the original expiration time on domain restore (#601)

* Restore the original expiration time on domain restore

Except if that time is now in the past, then add a year to it.

* Apply auto-formatter changes to fix my local build

* Merge branch 'master' into restore-expiry-date

* Fix reversed comments

core/src/main/java/google/registry/flows/domain/DomainRestoreRequestFlow.java

@@ -143,24 +143,30 @@ public final class DomainRestoreRequestFlow implements TransactionalFlow  {
     verifyRestoreAllowed(command, existingDomain, feeUpdate, feesAndCredits, now);
     HistoryEntry historyEntry = buildHistoryEntry(existingDomain, now);
     ImmutableSet.Builder<ImmutableObject> entitiesToSave = new ImmutableSet.Builder<>();
-    entitiesToSave.addAll(
+
-        createRestoreAndRenewBillingEvents(
+    // Restore the expiration time on the deleted domain, except if that's already passed, then add
-            historyEntry, feesAndCredits.getRestoreCost(), feesAndCredits.getRenewCost(), now));
+    // a year and bill for it immediately, with no grace period.
-    // We don't preserve the original expiration time of the domain when we restore, since doing so
+    DateTime newExpirationTime = existingDomain.getRegistrationExpirationTime();
-    // would require us to know if they received a grace period refund when they deleted the domain,
+    if (newExpirationTime.isBefore(now)) {
-    // and to charge them for that again. Instead, we just say that all restores get a fresh year of
+      entitiesToSave.add(createRenewBillingEvent(historyEntry, feesAndCredits.getRenewCost(), now));
-    // registration and bill them for that accordingly.
+      newExpirationTime = newExpirationTime.plusYears(1);
-    DateTime newExpirationTime = now.plusYears(1);
+    }
-    BillingEvent.Recurring autorenewEvent = newAutorenewBillingEvent(existingDomain)
+    // Always bill for the restore itself.
-        .setEventTime(newExpirationTime)
+    entitiesToSave.add(
-        .setRecurrenceEndTime(END_OF_TIME)
+        createRestoreBillingEvent(historyEntry, feesAndCredits.getRestoreCost(), now));
-        .setParent(historyEntry)
+
-        .build();
+    BillingEvent.Recurring autorenewEvent =
-    PollMessage.Autorenew autorenewPollMessage = newAutorenewPollMessage(existingDomain)
+        newAutorenewBillingEvent(existingDomain)
-        .setEventTime(newExpirationTime)
+            .setEventTime(newExpirationTime)
-        .setAutorenewEndTime(END_OF_TIME)
+            .setRecurrenceEndTime(END_OF_TIME)
-        .setParent(historyEntry)
+            .setParent(historyEntry)
-        .build();
+            .build();
+    PollMessage.Autorenew autorenewPollMessage =
+        newAutorenewPollMessage(existingDomain)
+            .setEventTime(newExpirationTime)
+            .setAutorenewEndTime(END_OF_TIME)
+            .setParent(historyEntry)
+            .build();
     DomainBase newDomain =
         performRestore(
             existingDomain, newExpirationTime, autorenewEvent, autorenewPollMessage, now, clientId);
@@ -212,18 +218,6 @@ public final class DomainRestoreRequestFlow implements TransactionalFlow  {
     validateFeeChallenge(targetId, now, feeUpdate, feesAndCredits);
   }
 
-  private ImmutableSet<BillingEvent.OneTime> createRestoreAndRenewBillingEvents(
-      HistoryEntry historyEntry, Money restoreCost, Money renewCost, DateTime now) {
-    // Bill for the restore.
-    BillingEvent.OneTime restoreEvent = createRestoreBillingEvent(historyEntry, restoreCost, now);
-    // Create a new autorenew billing event and poll message starting at the new expiration time.
-    // Also bill for the 1 year cost of a domain renew. This is to avoid registrants being able to
-    // game the system for premium names by renewing, deleting, and then restoring to get a free
-    // year. Note that this billing event has no grace period; it is effective immediately.
-    BillingEvent.OneTime renewEvent = createRenewBillingEvent(historyEntry, renewCost, now);
-    return ImmutableSet.of(restoreEvent, renewEvent);
-  }
-
   private static DomainBase performRestore(
       DomainBase existingDomain,
       DateTime newExpirationTime,

core/src/main/java/google/registry/model/common/Cursor.java

@@ -35,9 +35,9 @@ import java.util.List;
 import org.joda.time.DateTime;
 
 /**
- * Shared entity for date cursors. This type supports both "scoped" cursors (i.e. per resource
+ * Shared entity for date cursors. This type supports both "scoped" cursors (i.e. per resource of a
- * of a given type, such as a TLD) and global (i.e. one per environment) cursors, defined internally
+ * given type, such as a TLD) and global (i.e. one per environment) cursors, defined internally as
- * as scoped on {@link EntityGroupRoot}.
+ * scoped on {@link EntityGroupRoot}.
  */
 @Entity
 public class Cursor extends ImmutableObject implements DatastoreEntity {

core/src/main/java/google/registry/model/common/EntityGroupRoot.java

@@ -27,14 +27,14 @@ import google.registry.schema.replay.SqlEntity;
  * reasons.
  *
  * <p>This exists as a storage place for common configuration options and global settings that
- * aren't updated too frequently.  Entities in this entity group are usually cached upon load.  The
+ * aren't updated too frequently. Entities in this entity group are usually cached upon load. The
  * reason this common entity group exists is because it enables strongly consistent queries and
- * updates across this seldomly updated data.  This shared entity group also helps cut down on
+ * updates across this seldomly updated data. This shared entity group also helps cut down on a
- * a potential ballooning in the number of entity groups enlisted in transactions.
+ * potential ballooning in the number of entity groups enlisted in transactions.
  *
- * <p>Historically, each TLD used to have a separate namespace, and all entities for a TLD were in
+ * <p>Historically, each TLD used to have a separate namespace, and all entities for a TLD were in a
- * a single EntityGroupRoot for that TLD.  Hence why there was a "cross-tld" entity group -- it was
+ * single EntityGroupRoot for that TLD. Hence why there was a "cross-tld" entity group -- it was the
- * the entity group for the single namespace where global data applicable for all TLDs lived.
+ * entity group for the single namespace where global data applicable for all TLDs lived.
  */
 @Entity
 public class EntityGroupRoot extends BackupGroupRoot implements DatastoreEntity {

core/src/main/java/google/registry/model/ofy/CommitLogBucket.java

@@ -44,10 +44,9 @@ import org.joda.time.DateTime;
 /**
  * Root for a random commit log bucket.
  *
- * <p>This is used to shard {@link CommitLogManifest} objects into
+ * <p>This is used to shard {@link CommitLogManifest} objects into {@link
- * {@link RegistryConfig#getCommitLogBucketCount() N} entity groups. This increases
+ * RegistryConfig#getCommitLogBucketCount() N} entity groups. This increases transaction throughput,
- * transaction throughput, while maintaining the ability to perform strongly-consistent ancestor
+ * while maintaining the ability to perform strongly-consistent ancestor queries.
- * queries.
  *
  * @see <a href="https://cloud.google.com/appengine/articles/scaling/contention">Avoiding Datastore
  *     contention</a>

core/src/main/java/google/registry/model/ofy/CommitLogCheckpoint.java

@@ -38,11 +38,11 @@ import org.joda.time.DateTime;
  * Entity representing a point-in-time consistent view of Datastore, based on commit logs.
  *
  * <p>Conceptually, this entity consists of two pieces of information: the checkpoint "wall" time
- * and a set of bucket checkpoint times.  The former is the ID for this checkpoint (constrained
+ * and a set of bucket checkpoint times. The former is the ID for this checkpoint (constrained to be
- * to be unique upon checkpoint creation) and also represents the approximate wall time of the
+ * unique upon checkpoint creation) and also represents the approximate wall time of the consistent
- * consistent Datastore view this checkpoint represents.  The latter is really a mapping from
+ * Datastore view this checkpoint represents. The latter is really a mapping from bucket ID to
- * bucket ID to timestamp, where the timestamp dictates the upper bound (inclusive) on commit logs
+ * timestamp, where the timestamp dictates the upper bound (inclusive) on commit logs from that
- * from that bucket to include when restoring Datastore to this checkpoint.
+ * bucket to include when restoring Datastore to this checkpoint.
  */
 @Entity
 @NotBackedUp(reason = Reason.COMMIT_LOGS)

core/src/main/java/google/registry/model/ofy/CommitLogCheckpointRoot.java

@@ -28,9 +28,7 @@ import google.registry.schema.replay.DatastoreEntity;
 import google.registry.schema.replay.SqlEntity;
 import org.joda.time.DateTime;
 
-/**
+/** Singleton parent entity for all commit log checkpoints. */
- * Singleton parent entity for all commit log checkpoints.
- */
 @Entity
 @NotBackedUp(reason = Reason.COMMIT_LOGS)
 public class CommitLogCheckpointRoot extends ImmutableObject implements DatastoreEntity {

core/src/main/java/google/registry/model/registry/label/PremiumList.java

@@ -248,7 +248,7 @@ public final class PremiumList extends BaseDomainLabelList<Money, PremiumList.Pr
   }
 
   /**
-   * A premium list entry entity, persisted to Datastore.  Each instance represents the price of a
+   * A premium list entry entity, persisted to Datastore. Each instance represents the price of a
    * single label on a given TLD.
    */
   @ReportedOn

core/src/main/java/google/registry/model/registry/label/ReservedList.java

@@ -62,8 +62,8 @@ import org.joda.time.DateTime;
  */
 @Entity
 public final class ReservedList
-    extends BaseDomainLabelList<ReservationType, ReservedList.ReservedListEntry> implements
+    extends BaseDomainLabelList<ReservationType, ReservedList.ReservedListEntry>
-    DatastoreEntity {
+    implements DatastoreEntity {
 
   private static final FluentLogger logger = FluentLogger.forEnclosingClass();
 

core/src/main/java/google/registry/model/tmch/ClaimsListShard.java

@@ -327,8 +327,8 @@ public class ClaimsListShard extends ImmutableObject implements DatastoreEntity
   }
 
   /**
-   * Serves as the coordinating claims list singleton linking to the {@link ClaimsListRevision}
+   * Serves as the coordinating claims list singleton linking to the {@link ClaimsListRevision} that
-   * that is live.
+   * is live.
    */
   @Entity
   @NotBackedUp(reason = Reason.EXTERNALLY_SOURCED)

core/src/main/java/google/registry/tools/LevelDbLogReader.java

@@ -88,8 +88,8 @@ public final class LevelDbLogReader implements Iterator<byte[]> {
   }
 
   /**
-   * Returns the next {@link #BLOCK_SIZE} bytes from the input channel, or {@link
+   * Returns the next {@link #BLOCK_SIZE} bytes from the input channel, or {@link Optional#empty()}
-   * Optional#empty()} if there is no more data.
+   * if there is no more data.
    */
   // TODO(weiminyu): use ByteBuffer directly.
   private Optional<byte[]> readFromChannel() throws IOException {

core/src/test/java/google/registry/flows/EppLifecycleDomainTest.java

@@ -127,9 +127,7 @@ public class EppLifecycleDomainTest extends EppTestCase {
             ImmutableMap.of(
                 "DOMAIN", "example.tld",
                 "CRDATE", "2000-06-01T00:02:00Z",
-                // TODO(mcilwain): The exp. date should be restored back to 2002-06-01T00:02:00Z,
+                "EXDATE", "2002-06-01T00:02:00Z",
-                // but this is old behavior of being 1 year after the moment of the restore.
-                "EXDATE", "2001-07-01T00:03:00Z",
                 "UPDATE", "2000-07-01T00:03:00Z"));
 
     assertThatLogoutSucceeds();
@@ -203,11 +201,7 @@ public class EppLifecycleDomainTest extends EppTestCase {
             ImmutableMap.of(
                 "DOMAIN", "example.tld",
                 "CRDATE", "2000-06-01T00:02:00Z",
-                // TODO(mcilwain): The exp. date should be 2003-06-01T00:02:00Z, the same as its
+                "EXDATE", "2003-06-01T00:02:00Z",
-                // value prior to the deletion, because the year that was taken off when the
-                // autorenew was canceled will be re-added in renewal during the restore.
-                // For now though, the current behavior is 1 year after restore.
-                "EXDATE", "2003-07-05T00:03:00Z",
                 "UPDATE", "2002-07-05T00:03:00Z"));
 
     assertThatLogoutSucceeds();
@@ -289,10 +283,7 @@ public class EppLifecycleDomainTest extends EppTestCase {
             ImmutableMap.of(
                 "DOMAIN", "example.tld",
                 "CRDATE", "2000-06-01T00:02:00Z",
-                // TODO(mcilwain): The exp. date should be 2002-06-01T00:02:00Z, which is the
+                "EXDATE", "2002-06-01T00:02:00Z",
-                // current registration expiration time on the (deleted) domain, but for now is
-                // 1 year after restore.
-                "EXDATE", "2001-06-20T00:00:00Z",
                 "UPDATE", "2000-06-20T00:00:00Z"));
 
     assertThatLogoutSucceeds();

core/src/test/java/google/registry/flows/domain/DomainRestoreRequestFlowTest.java

@@ -72,6 +72,7 @@ import google.registry.model.reporting.DomainTransactionRecord.TransactionReport
 import google.registry.model.reporting.HistoryEntry;
 import java.util.Map;
 import org.joda.money.Money;
+import org.joda.time.DateTime;
 import org.junit.Before;
 import org.junit.Test;
 
@@ -93,6 +94,10 @@ public class DomainRestoreRequestFlowTest
   }
 
   void persistPendingDeleteDomain() throws Exception {
+    persistPendingDeleteDomain(clock.nowUtc().plusYears(5).plusDays(45));
+  }
+
+  void persistPendingDeleteDomain(DateTime expirationTime) throws Exception {
     DomainBase domain = newDomainBase(getUniqueIdFromCommand());
     HistoryEntry historyEntry =
         persistResource(
@@ -103,7 +108,7 @@ public class DomainRestoreRequestFlowTest
     persistResource(
         domain
             .asBuilder()
-            .setRegistrationExpirationTime(clock.nowUtc().plusYears(5).plusDays(45))
+            .setRegistrationExpirationTime(expirationTime)
             .setDeletionTime(clock.nowUtc().plusDays(35))
             .addGracePeriod(
                 GracePeriod.create(
@@ -129,9 +134,10 @@ public class DomainRestoreRequestFlowTest
   }
 
   @Test
-  public void testSuccess() throws Exception {
+  public void testSuccess_expiryStillInFuture_notExtended() throws Exception {
     setEppInput("domain_update_restore_request.xml", ImmutableMap.of("DOMAIN", "example.tld"));
-    persistPendingDeleteDomain();
+    DateTime expirationTime = clock.nowUtc().plusYears(5).plusDays(45);
+    persistPendingDeleteDomain(expirationTime);
     assertTransactionalFlow(true);
     // Double check that we see a poll message in the future for when the delete happens.
     assertThat(getPollMessages("TheRegistrar", clock.nowUtc().plusMonths(1))).hasSize(1);
@@ -140,11 +146,79 @@ public class DomainRestoreRequestFlowTest
     HistoryEntry historyEntryDomainRestore =
         getOnlyHistoryEntryOfType(domain, HistoryEntry.Type.DOMAIN_RESTORE);
     assertThat(ofy().load().key(domain.getAutorenewBillingEvent()).now().getEventTime())
-        .isEqualTo(clock.nowUtc().plusYears(1));
+        .isEqualTo(expirationTime);
+    assertAboutDomains()
+        .that(domain)
+        // New expiration time should be the same as from before the deletion.
+        .hasRegistrationExpirationTime(expirationTime)
+        .and()
+        .doesNotHaveStatusValue(StatusValue.PENDING_DELETE)
+        .and()
+        .hasDeletionTime(END_OF_TIME)
+        .and()
+        .hasOneHistoryEntryEachOfTypes(
+            HistoryEntry.Type.DOMAIN_DELETE, HistoryEntry.Type.DOMAIN_RESTORE)
+        .and()
+        .hasLastEppUpdateTime(clock.nowUtc())
+        .and()
+        .hasLastEppUpdateClientId("TheRegistrar");
+    assertThat(domain.getGracePeriods()).isEmpty();
+    assertDnsTasksEnqueued("example.tld");
+    // The poll message for the delete should now be gone. The only poll message should be the new
+    // autorenew poll message.
+    assertPollMessages(
+        "TheRegistrar",
+        new PollMessage.Autorenew.Builder()
+            .setTargetId("example.tld")
+            .setClientId("TheRegistrar")
+            .setEventTime(domain.getRegistrationExpirationTime())
+            .setAutorenewEndTime(END_OF_TIME)
+            .setMsg("Domain was auto-renewed.")
+            .setParent(historyEntryDomainRestore)
+            .build());
+    // There should be a onetime for the restore and a new recurring billing event, but no renew
+    // onetime.
+    assertBillingEvents(
+        new BillingEvent.Recurring.Builder()
+            .setReason(Reason.RENEW)
+            .setFlags(ImmutableSet.of(Flag.AUTO_RENEW))
+            .setTargetId("example.tld")
+            .setClientId("TheRegistrar")
+            .setEventTime(expirationTime)
+            .setRecurrenceEndTime(END_OF_TIME)
+            .setParent(historyEntryDomainRestore)
+            .build(),
+        new BillingEvent.OneTime.Builder()
+            .setReason(Reason.RESTORE)
+            .setTargetId("example.tld")
+            .setClientId("TheRegistrar")
+            .setCost(Money.of(USD, 17))
+            .setPeriodYears(1)
+            .setEventTime(clock.nowUtc())
+            .setBillingTime(clock.nowUtc())
+            .setParent(historyEntryDomainRestore)
+            .build());
+  }
+
+  @Test
+  public void testSuccess_expiryInPast_extendedByOneYear() throws Exception {
+    setEppInput("domain_update_restore_request.xml", ImmutableMap.of("DOMAIN", "example.tld"));
+    DateTime expirationTime = clock.nowUtc().minusDays(20);
+    DateTime newExpirationTime = expirationTime.plusYears(1);
+    persistPendingDeleteDomain(expirationTime);
+    assertTransactionalFlow(true);
+    // Double check that we see a poll message in the future for when the delete happens.
+    assertThat(getPollMessages("TheRegistrar", clock.nowUtc().plusMonths(1))).hasSize(1);
+    runFlowAssertResponse(loadFile("generic_success_response.xml"));
+    DomainBase domain = reloadResourceByForeignKey();
+    HistoryEntry historyEntryDomainRestore =
+        getOnlyHistoryEntryOfType(domain, HistoryEntry.Type.DOMAIN_RESTORE);
+    assertThat(ofy().load().key(domain.getAutorenewBillingEvent()).now().getEventTime())
+        .isEqualTo(newExpirationTime);
     assertAboutDomains()
         .that(domain)
         // New expiration time should be exactly a year from now.
-        .hasRegistrationExpirationTime(clock.nowUtc().plusYears(1))
+        .hasRegistrationExpirationTime(newExpirationTime)
         .and()
         .doesNotHaveStatusValue(StatusValue.PENDING_DELETE)
         .and()
@@ -178,7 +252,7 @@ public class DomainRestoreRequestFlowTest
             .setFlags(ImmutableSet.of(Flag.AUTO_RENEW))
             .setTargetId("example.tld")
             .setClientId("TheRegistrar")
-            .setEventTime(domain.getRegistrationExpirationTime())
+            .setEventTime(newExpirationTime)
             .setRecurrenceEndTime(END_OF_TIME)
             .setParent(historyEntryDomainRestore)
             .build(),

core/src/test/java/google/registry/persistence/transaction/TransactionManagerTest.java

@@ -101,8 +101,7 @@ public class TransactionManagerTest {
     assertThrows(
         RuntimeException.class,
         () ->
-            tm()
+            tm().transact(
-                .transact(
                     () -> {
                       tm().saveNew(theEntity);
                       throw new RuntimeException();

core/src/test/resources/google/registry/flows/domain_info_response_fakesite_restored_ok.xml

@@ -21,7 +21,7 @@
         <domain:crDate>2000-06-01T00:04:00Z</domain:crDate>
         <domain:upID>NewRegistrar</domain:upID>
         <domain:upDate>2002-05-30T01:03:00Z</domain:upDate>
-        <domain:exDate>2003-05-30T01:03:00Z</domain:exDate>
+        <domain:exDate>2002-06-01T00:04:00Z</domain:exDate>
         <domain:authInfo>
           <domain:pw>2fooBAR</domain:pw>
         </domain:authInfo>