From c9da36be9f90a9ae244506e663e9d60dc730f02b Mon Sep 17 00:00:00 2001 From: Michael Muller Date: Wed, 9 Feb 2022 08:48:51 -0500 Subject: [PATCH] Fix create/update timestamp replay problems (#1515) * Fix create/update timestamp replay problems When CreateAutoTimestamp and UpdateAutoTimestamp are inserted into a Transaction, their values are not populated in the same way as when they are stored in the course of an SQL commit. This results in different timestamp values between SQL and datastore during the SQL -> DS replay. Fix this by providing these values from the JPA transaction time when we're doing transaction serialization. This change also removes the initialization of the Ofy clock in ExpandRecurringBillingEventsActionTest. It's not necessary as the ReplayExtension already takes care of this and doing it after the ReplayExtension as we were breaks a test now that the update timestamps are correct. --- .../CreateAutoTimestampTranslatorFactory.java | 18 ++++---- .../UpdateAutoTimestampTranslatorFactory.java | 13 +++++- ...xpandRecurringBillingEventsActionTest.java | 7 --- .../flows/domain/DomainCreateFlowTest.java | 10 ++++- .../flows/domain/DomainUpdateFlowTest.java | 11 ++++- .../registry/testing/ReplayExtension.java | 43 +++++++++++++++++++ 6 files changed, 81 insertions(+), 21 deletions(-) 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();