diff --git a/core/src/main/java/google/registry/flows/domain/DomainRestoreRequestFlow.java b/core/src/main/java/google/registry/flows/domain/DomainRestoreRequestFlow.java index 0292cc95a..d8f44b4f3 100644 --- a/core/src/main/java/google/registry/flows/domain/DomainRestoreRequestFlow.java +++ b/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 entitiesToSave = new ImmutableSet.Builder<>(); - entitiesToSave.addAll( - createRestoreAndRenewBillingEvents( - historyEntry, feesAndCredits.getRestoreCost(), feesAndCredits.getRenewCost(), now)); - // We don't preserve the original expiration time of the domain when we restore, since doing so - // would require us to know if they received a grace period refund when they deleted the domain, - // and to charge them for that again. Instead, we just say that all restores get a fresh year of - // registration and bill them for that accordingly. - DateTime newExpirationTime = now.plusYears(1); - BillingEvent.Recurring autorenewEvent = newAutorenewBillingEvent(existingDomain) - .setEventTime(newExpirationTime) - .setRecurrenceEndTime(END_OF_TIME) - .setParent(historyEntry) - .build(); - PollMessage.Autorenew autorenewPollMessage = newAutorenewPollMessage(existingDomain) - .setEventTime(newExpirationTime) - .setAutorenewEndTime(END_OF_TIME) - .setParent(historyEntry) - .build(); + + // Restore the expiration time on the deleted domain, except if that's already passed, then add + // a year and bill for it immediately, with no grace period. + DateTime newExpirationTime = existingDomain.getRegistrationExpirationTime(); + if (newExpirationTime.isBefore(now)) { + entitiesToSave.add(createRenewBillingEvent(historyEntry, feesAndCredits.getRenewCost(), now)); + newExpirationTime = newExpirationTime.plusYears(1); + } + // Always bill for the restore itself. + entitiesToSave.add( + createRestoreBillingEvent(historyEntry, feesAndCredits.getRestoreCost(), now)); + + BillingEvent.Recurring autorenewEvent = + newAutorenewBillingEvent(existingDomain) + .setEventTime(newExpirationTime) + .setRecurrenceEndTime(END_OF_TIME) + .setParent(historyEntry) + .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 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, diff --git a/core/src/main/java/google/registry/model/common/Cursor.java b/core/src/main/java/google/registry/model/common/Cursor.java index 1339076af..2937d613e 100644 --- a/core/src/main/java/google/registry/model/common/Cursor.java +++ b/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 - * of a given type, such as a TLD) and global (i.e. one per environment) cursors, defined internally - * as scoped on {@link EntityGroupRoot}. + * Shared entity for date cursors. This type supports both "scoped" cursors (i.e. per resource of a + * given type, such as a TLD) and global (i.e. one per environment) cursors, defined internally as + * scoped on {@link EntityGroupRoot}. */ @Entity public class Cursor extends ImmutableObject implements DatastoreEntity { diff --git a/core/src/main/java/google/registry/model/common/EntityGroupRoot.java b/core/src/main/java/google/registry/model/common/EntityGroupRoot.java index 11a9af336..3c753ea89 100644 --- a/core/src/main/java/google/registry/model/common/EntityGroupRoot.java +++ b/core/src/main/java/google/registry/model/common/EntityGroupRoot.java @@ -27,14 +27,14 @@ import google.registry.schema.replay.SqlEntity; * reasons. * *

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 - * a potential ballooning in the number of entity groups enlisted in transactions. + * updates across this seldomly updated data. This shared entity group also helps cut down on a + * potential ballooning in the number of entity groups enlisted in transactions. * - *

Historically, each TLD used to have a separate namespace, and all entities for a TLD were in - * a single EntityGroupRoot for that TLD. Hence why there was a "cross-tld" entity group -- it was - * the entity group for the single namespace where global data applicable for all TLDs lived. + *

Historically, each TLD used to have a separate namespace, and all entities for a TLD were in a + * single EntityGroupRoot for that TLD. Hence why there was a "cross-tld" entity group -- it was the + * entity group for the single namespace where global data applicable for all TLDs lived. */ @Entity public class EntityGroupRoot extends BackupGroupRoot implements DatastoreEntity { diff --git a/core/src/main/java/google/registry/model/ofy/CommitLogBucket.java b/core/src/main/java/google/registry/model/ofy/CommitLogBucket.java index bf9266e61..418188761 100644 --- a/core/src/main/java/google/registry/model/ofy/CommitLogBucket.java +++ b/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. * - *

This is used to shard {@link CommitLogManifest} objects into - * {@link RegistryConfig#getCommitLogBucketCount() N} entity groups. This increases - * transaction throughput, while maintaining the ability to perform strongly-consistent ancestor - * queries. + *

This is used to shard {@link CommitLogManifest} objects into {@link + * RegistryConfig#getCommitLogBucketCount() N} entity groups. This increases transaction throughput, + * while maintaining the ability to perform strongly-consistent ancestor queries. * * @see Avoiding Datastore * contention diff --git a/core/src/main/java/google/registry/model/ofy/CommitLogCheckpoint.java b/core/src/main/java/google/registry/model/ofy/CommitLogCheckpoint.java index 6a9fb712b..ec863b511 100644 --- a/core/src/main/java/google/registry/model/ofy/CommitLogCheckpoint.java +++ b/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. * *

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 - * to be unique upon checkpoint creation) and also represents the approximate wall time of the - * consistent Datastore view this checkpoint represents. The latter is really a mapping from - * bucket ID to timestamp, where the timestamp dictates the upper bound (inclusive) on commit logs - * from that bucket to include when restoring Datastore to this checkpoint. + * and a set of bucket checkpoint times. The former is the ID for this checkpoint (constrained to be + * unique upon checkpoint creation) and also represents the approximate wall time of the consistent + * Datastore view this checkpoint represents. The latter is really a mapping from bucket ID to + * timestamp, where the timestamp dictates the upper bound (inclusive) on commit logs from that + * bucket to include when restoring Datastore to this checkpoint. */ @Entity @NotBackedUp(reason = Reason.COMMIT_LOGS) diff --git a/core/src/main/java/google/registry/model/ofy/CommitLogCheckpointRoot.java b/core/src/main/java/google/registry/model/ofy/CommitLogCheckpointRoot.java index 79bc14b7c..c07dc16b0 100644 --- a/core/src/main/java/google/registry/model/ofy/CommitLogCheckpointRoot.java +++ b/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 { diff --git a/core/src/main/java/google/registry/model/registry/label/PremiumList.java b/core/src/main/java/google/registry/model/registry/label/PremiumList.java index e94ce784c..edc6fe68a 100644 --- a/core/src/main/java/google/registry/model/registry/label/PremiumList.java +++ b/core/src/main/java/google/registry/model/registry/label/PremiumList.java @@ -248,7 +248,7 @@ public final class PremiumList extends BaseDomainLabelList implements - DatastoreEntity { + extends BaseDomainLabelList + implements DatastoreEntity { private static final FluentLogger logger = FluentLogger.forEnclosingClass(); diff --git a/core/src/main/java/google/registry/model/tmch/ClaimsListShard.java b/core/src/main/java/google/registry/model/tmch/ClaimsListShard.java index 3e81b862b..9b70e572b 100644 --- a/core/src/main/java/google/registry/model/tmch/ClaimsListShard.java +++ b/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} - * that is live. + * Serves as the coordinating claims list singleton linking to the {@link ClaimsListRevision} that + * is live. */ @Entity @NotBackedUp(reason = Reason.EXTERNALLY_SOURCED) diff --git a/core/src/main/java/google/registry/tools/LevelDbLogReader.java b/core/src/main/java/google/registry/tools/LevelDbLogReader.java index 0a0dad9b5..0244420d3 100644 --- a/core/src/main/java/google/registry/tools/LevelDbLogReader.java +++ b/core/src/main/java/google/registry/tools/LevelDbLogReader.java @@ -88,8 +88,8 @@ public final class LevelDbLogReader implements Iterator { } /** - * Returns the next {@link #BLOCK_SIZE} bytes from the input channel, or {@link - * Optional#empty()} if there is no more data. + * Returns the next {@link #BLOCK_SIZE} bytes from the input channel, or {@link Optional#empty()} + * if there is no more data. */ // TODO(weiminyu): use ByteBuffer directly. private Optional readFromChannel() throws IOException { diff --git a/core/src/test/java/google/registry/flows/EppLifecycleDomainTest.java b/core/src/test/java/google/registry/flows/EppLifecycleDomainTest.java index 614751a5e..24d4b2491 100644 --- a/core/src/test/java/google/registry/flows/EppLifecycleDomainTest.java +++ b/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, - // but this is old behavior of being 1 year after the moment of the restore. - "EXDATE", "2001-07-01T00:03:00Z", + "EXDATE", "2002-06-01T00:02: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 - // 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", + "EXDATE", "2003-06-01T00:02: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 - // current registration expiration time on the (deleted) domain, but for now is - // 1 year after restore. - "EXDATE", "2001-06-20T00:00:00Z", + "EXDATE", "2002-06-01T00:02:00Z", "UPDATE", "2000-06-20T00:00:00Z")); assertThatLogoutSucceeds(); diff --git a/core/src/test/java/google/registry/flows/domain/DomainRestoreRequestFlowTest.java b/core/src/test/java/google/registry/flows/domain/DomainRestoreRequestFlowTest.java index b73887069..87a0d45ff 100644 --- a/core/src/test/java/google/registry/flows/domain/DomainRestoreRequestFlowTest.java +++ b/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(), diff --git a/core/src/test/java/google/registry/persistence/transaction/TransactionManagerTest.java b/core/src/test/java/google/registry/persistence/transaction/TransactionManagerTest.java index af5d99b1e..90c85bdca 100644 --- a/core/src/test/java/google/registry/persistence/transaction/TransactionManagerTest.java +++ b/core/src/test/java/google/registry/persistence/transaction/TransactionManagerTest.java @@ -101,8 +101,7 @@ public class TransactionManagerTest { assertThrows( RuntimeException.class, () -> - tm() - .transact( + tm().transact( () -> { tm().saveNew(theEntity); throw new RuntimeException(); diff --git a/core/src/test/resources/google/registry/flows/domain_info_response_fakesite_restored_ok.xml b/core/src/test/resources/google/registry/flows/domain_info_response_fakesite_restored_ok.xml index b2cc3b0f6..8c749cb12 100644 --- a/core/src/test/resources/google/registry/flows/domain_info_response_fakesite_restored_ok.xml +++ b/core/src/test/resources/google/registry/flows/domain_info_response_fakesite_restored_ok.xml @@ -21,7 +21,7 @@ 2000-06-01T00:04:00Z NewRegistrar 2002-05-30T01:03:00Z - 2003-05-30T01:03:00Z + 2002-06-01T00:04:00Z 2fooBAR