diff --git a/core/src/main/java/google/registry/model/translators/CreateAutoTimestampTranslatorFactory.java b/core/src/main/java/google/registry/model/translators/CreateAutoTimestampTranslatorFactory.java index a7c28061e..934691752 100644 --- a/core/src/main/java/google/registry/model/translators/CreateAutoTimestampTranslatorFactory.java +++ b/core/src/main/java/google/registry/model/translators/CreateAutoTimestampTranslatorFactory.java @@ -14,12 +14,10 @@ package google.registry.model.translators; -import static com.google.common.base.MoreObjects.firstNonNull; -import static google.registry.persistence.transaction.TransactionManagerFactory.ofyTm; +import static google.registry.persistence.transaction.TransactionManagerFactory.tm; import static org.joda.time.DateTimeZone.UTC; import google.registry.model.CreateAutoTimestamp; -import google.registry.persistence.transaction.Transaction; import java.util.Date; import org.joda.time.DateTime; @@ -47,13 +45,13 @@ public class CreateAutoTimestampTranslatorFactory /** Save a timestamp, setting it to the current time if it did not have a previous value. */ @Override public Date saveValue(CreateAutoTimestamp pojoValue) { - - // Don't do this if we're in the course of transaction serialization. - if (Transaction.inSerializationMode()) { - return pojoValue.getTimestamp() == null ? null : pojoValue.getTimestamp().toDate(); - } - - return firstNonNull(pojoValue.getTimestamp(), ofyTm().getTransactionTime()).toDate(); + // Note that we use the current transaction manager -- we need to do this under JPA when we + // serialize the entity from a Transaction object, but we need to use the JPA transaction + // manager in that case. + return (pojoValue.getTimestamp() == null + ? tm().getTransactionTime() + : pojoValue.getTimestamp()) + .toDate(); } }; } diff --git a/core/src/main/java/google/registry/model/translators/UpdateAutoTimestampTranslatorFactory.java b/core/src/main/java/google/registry/model/translators/UpdateAutoTimestampTranslatorFactory.java index 7c99c5a58..94d29c634 100644 --- a/core/src/main/java/google/registry/model/translators/UpdateAutoTimestampTranslatorFactory.java +++ b/core/src/main/java/google/registry/model/translators/UpdateAutoTimestampTranslatorFactory.java @@ -14,6 +14,8 @@ package google.registry.model.translators; +import static com.google.common.base.Preconditions.checkState; +import static google.registry.persistence.transaction.TransactionManagerFactory.jpaTm; import static google.registry.persistence.transaction.TransactionManagerFactory.ofyTm; import static org.joda.time.DateTimeZone.UTC; @@ -48,9 +50,16 @@ public class UpdateAutoTimestampTranslatorFactory @Override public Date saveValue(UpdateAutoTimestamp pojoValue) { - // Don't do this if we're in the course of transaction serialization. + // If we're in the course of Transaction serialization, we have to use the transaction time + // here and the JPA transaction manager which is what will ultimately be saved during the + // commit. + // Note that this branch doesn't respect "auto update disabled", as this state is + // specifically to address replay, so we add a runtime check for this. if (Transaction.inSerializationMode()) { - return pojoValue.getTimestamp() == null ? null : pojoValue.getTimestamp().toDate(); + checkState( + UpdateAutoTimestamp.autoUpdateEnabled(), + "Auto-update disabled during transaction serialization."); + return jpaTm().getTransactionTime().toDate(); } return UpdateAutoTimestamp.autoUpdateEnabled() diff --git a/core/src/test/java/google/registry/batch/ExpandRecurringBillingEventsActionTest.java b/core/src/test/java/google/registry/batch/ExpandRecurringBillingEventsActionTest.java index 924ae4c87..29ac23f75 100644 --- a/core/src/test/java/google/registry/batch/ExpandRecurringBillingEventsActionTest.java +++ b/core/src/test/java/google/registry/batch/ExpandRecurringBillingEventsActionTest.java @@ -48,7 +48,6 @@ import google.registry.model.common.Cursor; import google.registry.model.domain.DomainBase; import google.registry.model.domain.DomainHistory; import google.registry.model.domain.Period; -import google.registry.model.ofy.Ofy; import google.registry.model.reporting.DomainTransactionRecord; import google.registry.model.reporting.DomainTransactionRecord.TransactionReportField; import google.registry.model.reporting.HistoryEntry; @@ -56,7 +55,6 @@ import google.registry.model.tld.Registry; import google.registry.testing.DualDatabaseTest; import google.registry.testing.FakeClock; import google.registry.testing.FakeResponse; -import google.registry.testing.InjectExtension; import google.registry.testing.ReplayExtension; import google.registry.testing.TestOfyAndSql; import google.registry.testing.TestOfyOnly; @@ -78,11 +76,6 @@ public class ExpandRecurringBillingEventsActionTest private DateTime currentTestTime = DateTime.parse("1999-01-05T00:00:00Z"); private final FakeClock clock = new FakeClock(currentTestTime); - @Order(Order.DEFAULT - 1) - @RegisterExtension - public final InjectExtension inject = - new InjectExtension().withStaticFieldOverride(Ofy.class, "clock", clock); - @Order(Order.DEFAULT - 2) @RegisterExtension public final ReplayExtension replayExtension = ReplayExtension.createWithDoubleReplay(clock); diff --git a/core/src/test/java/google/registry/flows/domain/DomainCreateFlowTest.java b/core/src/test/java/google/registry/flows/domain/DomainCreateFlowTest.java index c0ffb21ee..97744b53c 100644 --- a/core/src/test/java/google/registry/flows/domain/DomainCreateFlowTest.java +++ b/core/src/test/java/google/registry/flows/domain/DomainCreateFlowTest.java @@ -347,6 +347,12 @@ class DomainCreateFlowTest extends ResourceFlowTestCase DS replay. + // Added to confirm that timestamps get updated correctly. + replayExtension.enableDomainTimestampChecks(); } private void assertNoLordn() throws Exception { @@ -574,6 +580,7 @@ class DomainCreateFlowTest extends ResourceFlowTestCase DS replay. + // Added to confirm that timestamps get updated correctly. + replayExtension.enableDomainTimestampChecks(); } @TestOfyAndSql @@ -308,6 +315,8 @@ class DomainUpdateFlowTest extends ResourceFlowTestCase expectedUpdates = new ArrayList<>(); + boolean enableDomainTimestampChecks; private ReplayExtension( FakeClock clock, boolean compare, @Nullable ReplicateToDatastoreAction sqlToDsReplicator) { @@ -108,6 +112,30 @@ public class ReplayExtension implements BeforeEachCallback, AfterEachCallback { } } + /** + * Enable checking of domain timestamps during replay. + * + *

This was added to facilitate testing of a very specific bug wherein create/update + * auto-timestamps serialized to the SQL -> DS Transaction table had different values from those + * actually stored in SQL. + * + *

In order to use this, you also need to use expectUpdateFor() to store the states of a + * DomainBase object at a given point in time. + */ + public void enableDomainTimestampChecks() { + enableDomainTimestampChecks = true; + } + + /** + * If we're doing domain time checks, add the current state of a domain to check against. + * + *

A null argument is a placeholder to deal with b/217952766. Basically it allows us to ignore + * one particular state in the sequence (where the timestamp is not what we expect it to be). + */ + public void expectUpdateFor(@Nullable DomainBase domain) { + expectedUpdates.add(domain); + } + @Override public void beforeEach(ExtensionContext context) { // Use a single bucket to expose timestamp inversion problems. This typically happens when @@ -253,6 +281,21 @@ public class ReplayExtension implements BeforeEachCallback, AfterEachCallback { assertAboutImmutableObjects() .that(fromDatastore) .isEqualAcrossDatabases(fromTransactionEntity); + + // Check DomainBase timestamps if appropriate. + if (enableDomainTimestampChecks && fromTransactionEntity instanceof DomainBase) { + DomainBase expectedDomain = expectedUpdates.remove(0); + + // Just skip it if the expectedDomain is null. + if (expectedDomain == null) { + continue; + } + + DomainBase domainEntity = (DomainBase) fromTransactionEntity; + assertThat(domainEntity.getCreationTime()).isEqualTo(expectedDomain.getCreationTime()); + assertThat(domainEntity.getUpdateTimestamp()) + .isEqualTo(expectedDomain.getUpdateTimestamp()); + } } else { Delete delete = (Delete) mutation; VKey key = delete.getKey();