From ff3c848def0499ab3c16cb75c40929e1dfd5c8b9 Mon Sep 17 00:00:00 2001 From: Ben McIlwain Date: Mon, 27 Sep 2021 11:32:15 -0400 Subject: [PATCH] Add handling for UpdateAutoTimestamp when not in a transaction (#1341) * Add handling for UpdateAutoTimestamp when not in a transaction It's not clear why this is sometimes causing test flakes, but getting better logging involved should help clear it up. This also changes AppEngineExtension to insert without reloading the initial test data, rather than putting it (potentially involving a merge) and reloading it in a separate transaction. This should hopefully reduce the chance of weird conflicts. --- .../registry/model/UpdateAutoTimestamp.java | 17 ++++++++++++++++- .../registry/model/registrar/RegistrarTest.java | 4 ++-- .../registry/schema/replay/SqlEntityTest.java | 10 ++++++---- .../registry/testing/AppEngineExtension.java | 4 ++-- .../google/registry/testing/DatabaseHelper.java | 14 +++++++++++--- 5 files changed, 37 insertions(+), 12 deletions(-) diff --git a/core/src/main/java/google/registry/model/UpdateAutoTimestamp.java b/core/src/main/java/google/registry/model/UpdateAutoTimestamp.java index 6e3114ba9..fcd254d7e 100644 --- a/core/src/main/java/google/registry/model/UpdateAutoTimestamp.java +++ b/core/src/main/java/google/registry/model/UpdateAutoTimestamp.java @@ -15,8 +15,12 @@ package google.registry.model; import static google.registry.persistence.transaction.TransactionManagerFactory.jpaTm; +import static google.registry.persistence.transaction.TransactionManagerFactory.ofyTm; import static google.registry.util.DateTimeUtils.START_OF_TIME; +import static org.joda.time.DateTimeZone.UTC; +import com.google.common.flogger.FluentLogger; +import com.google.common.flogger.StackSize; import com.googlecode.objectify.annotation.Ignore; import com.googlecode.objectify.annotation.OnLoad; import google.registry.model.translators.UpdateAutoTimestampTranslatorFactory; @@ -40,6 +44,8 @@ import org.joda.time.DateTime; @Embeddable public class UpdateAutoTimestamp extends ImmutableObject { + private static final FluentLogger logger = FluentLogger.forEnclosingClass(); + // When set to true, database converters/translators should do the auto update. When set to // false, auto update should be suspended (this exists to allow us to preserve the original value // during a replay). @@ -57,7 +63,16 @@ public class UpdateAutoTimestamp extends ImmutableObject { @PrePersist @PreUpdate void setTimestamp() { - if (autoUpdateEnabled() || lastUpdateTime == null) { + // On the off chance that this is called outside of a transaction, log it instead of failing + // with an exception from attempting to call jpaTm().getTransactionTime(), and then fall back + // to DateTime.now(UTC). + if (!jpaTm().inTransaction()) { + logger.atSevere().withStackTrace(StackSize.MEDIUM).log( + "Failed to update automatic timestamp because this wasn't called in a JPA transaction%s.", + ofyTm().inTransaction() ? " (but there is an open Ofy transaction)" : ""); + timestamp = DateTime.now(UTC); + lastUpdateTime = DateTimeUtils.toZonedDateTime(timestamp); + } else if (autoUpdateEnabled() || lastUpdateTime == null) { timestamp = jpaTm().getTransactionTime(); lastUpdateTime = DateTimeUtils.toZonedDateTime(timestamp); } diff --git a/core/src/test/java/google/registry/model/registrar/RegistrarTest.java b/core/src/test/java/google/registry/model/registrar/RegistrarTest.java index 94ad9b3de..0fb78f682 100644 --- a/core/src/test/java/google/registry/model/registrar/RegistrarTest.java +++ b/core/src/test/java/google/registry/model/registrar/RegistrarTest.java @@ -270,8 +270,8 @@ class RegistrarTest extends EntityTestCase { persistSimpleResource( new RegistrarContact.Builder() .setParent(registrar) - .setName("John Abused") - .setEmailAddress("johnabuse@example.com") + .setName("John Abussy") + .setEmailAddress("johnabussy@example.com") .setPhoneNumber("+1.2125551213") .setFaxNumber("+1.2125551213") // No setTypes(...) diff --git a/core/src/test/java/google/registry/schema/replay/SqlEntityTest.java b/core/src/test/java/google/registry/schema/replay/SqlEntityTest.java index 03e155055..0bc61bb05 100644 --- a/core/src/test/java/google/registry/schema/replay/SqlEntityTest.java +++ b/core/src/test/java/google/registry/schema/replay/SqlEntityTest.java @@ -38,7 +38,8 @@ public class SqlEntityTest { final DatastoreEntityExtension datastoreEntityExtension = new DatastoreEntityExtension(); @RegisterExtension - final AppEngineExtension database = new AppEngineExtension.Builder().withCloudSql().build(); + final AppEngineExtension database = + new AppEngineExtension.Builder().withCloudSql().withoutCannedData().build(); @BeforeEach void setup() throws Exception { @@ -54,9 +55,10 @@ public class SqlEntityTest { @Test void getPrimaryKeyString_oneIdColumn() { // AppEngineExtension canned data: Registrar1 - VKey key = Registrar.createVKey("NewRegistrar"); - String expected = "NewRegistrar"; - assertThat(tm().transact(() -> tm().loadByKey(key)).getPrimaryKeyString()).contains(expected); + assertThat( + tm().transact(() -> tm().loadByKey(Registrar.createVKey("NewRegistrar"))) + .getPrimaryKeyString()) + .contains("NewRegistrar"); } @Test diff --git a/core/src/test/java/google/registry/testing/AppEngineExtension.java b/core/src/test/java/google/registry/testing/AppEngineExtension.java index ea71c2c63..ce5954702 100644 --- a/core/src/test/java/google/registry/testing/AppEngineExtension.java +++ b/core/src/test/java/google/registry/testing/AppEngineExtension.java @@ -18,7 +18,7 @@ import static com.google.common.base.Preconditions.checkState; import static com.google.common.io.Files.asCharSink; import static com.google.common.truth.Truth.assertWithMessage; import static google.registry.persistence.transaction.TransactionManagerFactory.tm; -import static google.registry.testing.DatabaseHelper.persistSimpleResources; +import static google.registry.testing.DatabaseHelper.insertSimpleResources; import static google.registry.testing.DualDatabaseTestInvocationContextProvider.injectTmForDualDatabaseTest; import static google.registry.testing.DualDatabaseTestInvocationContextProvider.restoreTmAfterDualDatabaseTest; import static google.registry.util.PreconditionsUtils.checkArgumentNotNull; @@ -629,7 +629,7 @@ public final class AppEngineExtension implements BeforeEachCallback, AfterEachCa /** Create some fake registrars. */ public static void loadInitialData() { - persistSimpleResources( + insertSimpleResources( ImmutableList.of( makeRegistrar1(), makeRegistrarContact1(), diff --git a/core/src/test/java/google/registry/testing/DatabaseHelper.java b/core/src/test/java/google/registry/testing/DatabaseHelper.java index 75272ff86..3b613a1cb 100644 --- a/core/src/test/java/google/registry/testing/DatabaseHelper.java +++ b/core/src/test/java/google/registry/testing/DatabaseHelper.java @@ -1205,19 +1205,27 @@ public class DatabaseHelper { * ForeignKeyedEppResources. */ public static ImmutableList persistSimpleResources(final Iterable resources) { + insertSimpleResources(resources); + return transactIfJpaTm(() -> tm().loadByEntities(resources)); + } + + /** + * Like {@link #persistSimpleResources(Iterable)} but without reloading/returning the saved + * entities. + */ + public static void insertSimpleResources(final Iterable resources) { tm().transact( () -> { if (alwaysSaveWithBackup) { - tm().putAll(ImmutableList.copyOf(resources)); + tm().insertAll(ImmutableList.copyOf(resources)); } else { - tm().putAllWithoutBackup(ImmutableList.copyOf(resources)); + tm().insertAllWithoutBackup(ImmutableList.copyOf(resources)); } }); maybeAdvanceClock(); // Force the session to be cleared so that when we read it back, we read from Datastore // and not from the transaction's session cache. tm().clearSessionCache(); - return transactIfJpaTm(() -> tm().loadByEntities(resources)); } public static void deleteResource(final Object resource) {