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.

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);
     }

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")
+            .setName("John Abussy")
-            .setEmailAddress("johnabuse@example.com")
+            .setEmailAddress("johnabussy@example.com")
             .setPhoneNumber("+1.2125551213")
             .setFaxNumber("+1.2125551213")
             // No setTypes(...)

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<Registrar> key = Registrar.createVKey("NewRegistrar");
+    assertThat(
-    String expected = "NewRegistrar";
+            tm().transact(() -> tm().loadByKey(Registrar.createVKey("NewRegistrar")))
-    assertThat(tm().transact(() -> tm().loadByKey(key)).getPrimaryKeyString()).contains(expected);
+                .getPrimaryKeyString())
+        .contains("NewRegistrar");
   }
 
   @Test

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(),

core/src/test/java/google/registry/testing/DatabaseHelper.java

@@ -1205,19 +1205,27 @@ public class DatabaseHelper {
    * ForeignKeyedEppResources.
    */
   public static <R> ImmutableList<R> persistSimpleResources(final Iterable<R> resources) {
+    insertSimpleResources(resources);
+    return transactIfJpaTm(() -> tm().loadByEntities(resources));
+  }
+
+  /**
+   * Like {@link #persistSimpleResources(Iterable)} but without reloading/returning the saved
+   * entities.
+   */
+  public static <R> void insertSimpleResources(final Iterable<R> 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) {