Convert DomainCreateFlow to use generic tm() methods (#1026)

Various necessary changes included as part of this:

- Make ForeignKeyIndex completely generic. Previously, only the load()
method that took a DateTime as input could use SQL, and the cached flow
was particular to Objectify Keys. Now, the cached flow and the
non-cached flow can use the same (ish) piece of code to load / create
the relevant index objects before filtering or modifying them as
necessary.
- EntityChanges should use VKeys
- FlowUtils should persist entity changes using tm(), however not all
object types are storable in SQL.
- Filling out PollMessage fields with the proper object type when
loading from SQL
- Changing a few tm() calls to ofyTm() calls when using objectify. This
is because creating a read-only transaction in SQL is quite a footgun at
the moment, because it makes the entire transaction you're in (if you
were already in one) a read-only transaction.

core/src/main/java/google/registry/flows/FlowUtils.java

@@ -15,7 +15,7 @@
 package google.registry.flows;
 
 import static com.google.common.base.Preconditions.checkState;
-import static google.registry.model.ofy.ObjectifyService.ofy;
+import static google.registry.persistence.transaction.TransactionManagerFactory.tm;
 import static google.registry.xml.ValidationMode.LENIENT;
 import static google.registry.xml.ValidationMode.STRICT;
 import static java.nio.charset.StandardCharsets.UTF_8;
@@ -51,8 +51,8 @@ public final class FlowUtils {
 
   /** Persists the saves and deletes in an {@link EntityChanges} to Datastore. */
   public static void persistEntityChanges(EntityChanges entityChanges) {
-    ofy().save().entities(entityChanges.getSaves());
+    tm().putAll(entityChanges.getSaves());
-    ofy().delete().keys(entityChanges.getDeletes());
+    tm().delete(entityChanges.getDeletes());
   }
 
   /**

core/src/main/java/google/registry/flows/custom/EntityChanges.java

@@ -16,8 +16,8 @@ package google.registry.flows.custom;
 
 import com.google.auto.value.AutoValue;
 import com.google.common.collect.ImmutableSet;
-import com.googlecode.objectify.Key;
 import google.registry.model.ImmutableObject;
+import google.registry.persistence.VKey;
 
 /** A wrapper class that encapsulates Datastore entities to both save and delete. */
 @AutoValue
@@ -25,7 +25,7 @@ public abstract class EntityChanges {
 
   public abstract ImmutableSet<ImmutableObject> getSaves();
 
-  public abstract ImmutableSet<Key<ImmutableObject>> getDeletes();
+  public abstract ImmutableSet<VKey<ImmutableObject>> getDeletes();
 
   public static Builder newBuilder() {
     // Default both entities to save and entities to delete to empty sets, so that the build()
@@ -48,11 +48,11 @@ public abstract class EntityChanges {
       return this;
     }
 
-    public abstract Builder setDeletes(ImmutableSet<Key<ImmutableObject>> entitiesToDelete);
+    public abstract Builder setDeletes(ImmutableSet<VKey<ImmutableObject>> entitiesToDelete);
 
-    public abstract ImmutableSet.Builder<Key<ImmutableObject>> deletesBuilder();
+    public abstract ImmutableSet.Builder<VKey<ImmutableObject>> deletesBuilder();
 
-    public Builder addDelete(Key<ImmutableObject> entityToDelete) {
+    public Builder addDelete(VKey<ImmutableObject> entityToDelete) {
       deletesBuilder().add(entityToDelete);
       return this;
     }

core/src/main/java/google/registry/model/index/ForeignKeyIndex.java

@@ -16,17 +16,20 @@ package google.registry.model.index;
 
 import static com.google.common.collect.ImmutableList.toImmutableList;
 import static com.google.common.collect.ImmutableMap.toImmutableMap;
+import static com.google.common.collect.ImmutableSet.toImmutableSet;
 import static google.registry.config.RegistryConfig.getEppResourceCachingDuration;
 import static google.registry.config.RegistryConfig.getEppResourceMaxCachedEntries;
 import static google.registry.model.ofy.ObjectifyService.ofy;
 import static google.registry.persistence.transaction.TransactionManagerFactory.jpaTm;
 import static google.registry.persistence.transaction.TransactionManagerFactory.tm;
+import static google.registry.util.CollectionUtils.entriesToImmutableMap;
 import static google.registry.util.TypeUtils.instantiate;
 
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.cache.CacheBuilder;
 import com.google.common.cache.CacheLoader;
 import com.google.common.cache.LoadingCache;
+import com.google.common.collect.ImmutableBiMap;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
@@ -48,7 +51,6 @@ import google.registry.persistence.VKey;
 import google.registry.schema.replay.DatastoreOnlyEntity;
 import google.registry.util.NonFinalForTesting;
 import java.util.Comparator;
-import java.util.List;
 import java.util.Map;
 import java.util.Optional;
 import java.util.concurrent.ExecutionException;
@@ -81,10 +83,10 @@ public abstract class ForeignKeyIndex<E extends EppResource> extends BackupGroup
   public static class ForeignKeyHostIndex extends ForeignKeyIndex<HostResource>
       implements DatastoreOnlyEntity {}
 
-  private static final ImmutableMap<
+  private static final ImmutableBiMap<
           Class<? extends EppResource>, Class<? extends ForeignKeyIndex<?>>>
       RESOURCE_CLASS_TO_FKI_CLASS =
-          ImmutableMap.of(
+          ImmutableBiMap.of(
               ContactResource.class, ForeignKeyContactIndex.class,
               DomainBase.class, ForeignKeyDomainIndex.class,
               HostResource.class, ForeignKeyHostIndex.class);
@@ -184,7 +186,7 @@ public abstract class ForeignKeyIndex<E extends EppResource> extends BackupGroup
   }
 
   /**
-   * Load a list of {@link ForeignKeyIndex} instances by class and id strings that are active at or
+   * Load a map of {@link ForeignKeyIndex} instances by class and id strings that are active at or
    * after the specified moment in time.
    *
    * <p>The returned map will omit any keys for which the {@link ForeignKeyIndex} doesn't exist or
@@ -192,13 +194,26 @@ public abstract class ForeignKeyIndex<E extends EppResource> extends BackupGroup
    */
   public static <E extends EppResource> ImmutableMap<String, ForeignKeyIndex<E>> load(
       Class<E> clazz, Iterable<String> foreignKeys, final DateTime now) {
+    return loadIndexesFromStore(clazz, foreignKeys).entrySet().stream()
+        .filter(e -> now.isBefore(e.getValue().getDeletionTime()))
+        .collect(entriesToImmutableMap());
+  }
+
+  /**
+   * Helper method to load all of the most recent {@link ForeignKeyIndex}es for the given foreign
+   * keys, regardless of whether or not they have been soft-deleted.
+   *
+   * <p>Used by both the cached (w/o deletion check) and the non-cached (with deletion check) calls.
+   */
+  private static <E extends EppResource>
+      ImmutableMap<String, ForeignKeyIndex<E>> loadIndexesFromStore(
+          Class<E> clazz, Iterable<String> foreignKeys) {
     if (tm().isOfy()) {
-      return ofy().load().type(mapToFkiClass(clazz)).ids(foreignKeys).entrySet().stream()
+      return ImmutableMap.copyOf(
-          .filter(e -> now.isBefore(e.getValue().deletionTime))
+          tm().doTransactionless(() -> ofy().load().type(mapToFkiClass(clazz)).ids(foreignKeys)));
-          .collect(toImmutableMap(Map.Entry::getKey, Map.Entry::getValue));
     } else {
       String property = RESOURCE_CLASS_TO_FKI_PROPERTY.get(clazz);
-      List<E> entities =
+      ImmutableList<ForeignKeyIndex<E>> indexes =
           tm().transact(
                   () -> {
                     String entityName =
@@ -206,49 +221,58 @@ public abstract class ForeignKeyIndex<E extends EppResource> extends BackupGroup
                     return jpaTm()
                         .query(
                             String.format(
-                                "FROM %s WHERE %s IN :propertyValue and deletionTime > :now ",
+                                "FROM %s WHERE %s IN :propertyValue", entityName, property),
-                                entityName, property),
                             clazz)
                         .setParameter("propertyValue", foreignKeys)
-                        .setParameter("now", now)
+                        .getResultStream()
-                        .getResultList();
+                        .map(e -> ForeignKeyIndex.create(e, e.getDeletionTime()))
+                        .collect(toImmutableList());
                   });
       // We need to find and return the entities with the maximum deletionTime for each foreign key.
-      return Multimaps.index(entities, EppResource::getForeignKey).asMap().entrySet().stream()
+      return Multimaps.index(indexes, ForeignKeyIndex::getForeignKey).asMap().entrySet().stream()
           .map(
               entry ->
                   Maps.immutableEntry(
                       entry.getKey(),
                       entry.getValue().stream()
-                          .max(Comparator.comparing(EppResource::getDeletionTime))
+                          .max(Comparator.comparing(ForeignKeyIndex::getDeletionTime))
                           .get()))
-          .collect(
+          .collect(entriesToImmutableMap());
-              toImmutableMap(
-                  Map.Entry::getKey,
-                  entry -> create(entry.getValue(), entry.getValue().getDeletionTime())));
     }
   }
 
-  static final CacheLoader<Key<ForeignKeyIndex<?>>, Optional<ForeignKeyIndex<?>>> CACHE_LOADER =
+  static final CacheLoader<VKey<ForeignKeyIndex<?>>, Optional<ForeignKeyIndex<?>>> CACHE_LOADER =
-      new CacheLoader<Key<ForeignKeyIndex<?>>, Optional<ForeignKeyIndex<?>>>() {
+      new CacheLoader<VKey<ForeignKeyIndex<?>>, Optional<ForeignKeyIndex<?>>>() {
 
         @Override
-        public Optional<ForeignKeyIndex<?>> load(Key<ForeignKeyIndex<?>> key) {
+        public Optional<ForeignKeyIndex<?>> load(VKey<ForeignKeyIndex<?>> key) {
-          return Optional.ofNullable(tm().doTransactionless(() -> ofy().load().key(key).now()));
+          String foreignKey = key.getSqlKey().toString();
+          return Optional.ofNullable(
+              loadIndexesFromStore(
+                      RESOURCE_CLASS_TO_FKI_CLASS.inverse().get(key.getKind()),
+                      ImmutableSet.of(foreignKey))
+                  .get(foreignKey));
         }
 
         @Override
-        public Map<Key<ForeignKeyIndex<?>>, Optional<ForeignKeyIndex<?>>> loadAll(
+        public Map<VKey<ForeignKeyIndex<?>>, Optional<ForeignKeyIndex<?>>> loadAll(
-            Iterable<? extends Key<ForeignKeyIndex<?>>> keys) {
+            Iterable<? extends VKey<ForeignKeyIndex<?>>> keys) {
-          ImmutableSet<Key<ForeignKeyIndex<?>>> typedKeys = ImmutableSet.copyOf(keys);
+          if (!keys.iterator().hasNext()) {
-          Map<Key<ForeignKeyIndex<?>>, ForeignKeyIndex<?>> existingFkis =
+            return ImmutableMap.of();
-              tm().doTransactionless(() -> ofy().load().keys(typedKeys));
+          }
+          Class<? extends EppResource> resourceClass =
+              RESOURCE_CLASS_TO_FKI_CLASS.inverse().get(keys.iterator().next().getKind());
+          ImmutableSet<String> foreignKeys =
+              Streams.stream(keys).map(v -> v.getSqlKey().toString()).collect(toImmutableSet());
+          ImmutableSet<VKey<ForeignKeyIndex<?>>> typedKeys = ImmutableSet.copyOf(keys);
+          ImmutableMap<String, ? extends ForeignKeyIndex<? extends EppResource>> existingFkis =
+              loadIndexesFromStore(resourceClass, foreignKeys);
           // ofy() omits keys that don't have values in Datastore, so re-add them in
           // here with Optional.empty() values.
           return Maps.asMap(
               typedKeys,
-              (Key<ForeignKeyIndex<?>> key) ->
+              (VKey<ForeignKeyIndex<?>> key) ->
-                  Optional.ofNullable(existingFkis.getOrDefault(key, null)));
+                  Optional.ofNullable(existingFkis.getOrDefault(key.getSqlKey().toString(), null)));
         }
       };
 
@@ -266,10 +290,10 @@ public abstract class ForeignKeyIndex<E extends EppResource> extends BackupGroup
    * given IDs (blah) don't exist."
    */
   @NonFinalForTesting
-  private static LoadingCache<Key<ForeignKeyIndex<?>>, Optional<ForeignKeyIndex<?>>>
+  private static LoadingCache<VKey<ForeignKeyIndex<?>>, Optional<ForeignKeyIndex<?>>>
       cacheForeignKeyIndexes = createForeignKeyIndexesCache(getEppResourceCachingDuration());
 
-  private static LoadingCache<Key<ForeignKeyIndex<?>>, Optional<ForeignKeyIndex<?>>>
+  private static LoadingCache<VKey<ForeignKeyIndex<?>>, Optional<ForeignKeyIndex<?>>>
       createForeignKeyIndexesCache(Duration expiry) {
     return CacheBuilder.newBuilder()
         .expireAfterWrite(java.time.Duration.ofMillis(expiry.getMillis()))
@@ -298,21 +322,24 @@ public abstract class ForeignKeyIndex<E extends EppResource> extends BackupGroup
     if (!RegistryConfig.isEppResourceCachingEnabled()) {
       return tm().doTransactionless(() -> load(clazz, foreignKeys, now));
     }
-    ImmutableList<Key<ForeignKeyIndex<?>>> fkiKeys =
+    Class<? extends ForeignKeyIndex<?>> fkiClass = mapToFkiClass(clazz);
+    // Safe to cast VKey<FKI<E>> to VKey<FKI<?>>
+    @SuppressWarnings("unchecked")
+    ImmutableList<VKey<ForeignKeyIndex<?>>> fkiVKeys =
         Streams.stream(foreignKeys)
-            .map(fk -> Key.<ForeignKeyIndex<?>>create(mapToFkiClass(clazz), fk))
+            .map(fk -> (VKey<ForeignKeyIndex<?>>) VKey.create(fkiClass, fk))
             .collect(toImmutableList());
     try {
       // This cast is safe because when we loaded ForeignKeyIndexes above we used type clazz, which
       // is scoped to E.
       @SuppressWarnings("unchecked")
       ImmutableMap<String, ForeignKeyIndex<E>> fkisFromCache =
-          cacheForeignKeyIndexes.getAll(fkiKeys).entrySet().stream()
+          cacheForeignKeyIndexes.getAll(fkiVKeys).entrySet().stream()
               .filter(entry -> entry.getValue().isPresent())
               .filter(entry -> now.isBefore(entry.getValue().get().getDeletionTime()))
               .collect(
                   toImmutableMap(
-                      entry -> entry.getKey().getName(),
+                      entry -> entry.getKey().getSqlKey().toString(),
                       entry -> (ForeignKeyIndex<E>) entry.getValue().get()));
       return fkisFromCache;
     } catch (ExecutionException e) {

core/src/main/java/google/registry/model/poll/PollMessage.java

@@ -398,6 +398,7 @@ public abstract class PollMessage extends ImmutableObject
       }
       if (!isNullOrEmpty(domainPendingActionNotificationResponses)) {
         pendingActionNotificationResponse = domainPendingActionNotificationResponses.get(0);
+        fullyQualifiedDomainName = pendingActionNotificationResponse.nameOrId.value;
       }
       if (!isNullOrEmpty(domainTransferResponses)) {
         fullyQualifiedDomainName = domainTransferResponses.get(0).getFullyQualifiedDomainName();
@@ -414,21 +415,23 @@ public abstract class PollMessage extends ImmutableObject
       // Take the SQL-specific fields and map them to the Objectify-specific fields, if applicable
       if (pendingActionNotificationResponse != null) {
         if (contactId != null) {
-          contactPendingActionNotificationResponses =
+          ContactPendingActionNotificationResponse contactPendingResponse =
-              ImmutableList.of(
               ContactPendingActionNotificationResponse.create(
                   pendingActionNotificationResponse.nameOrId.value,
                   pendingActionNotificationResponse.getActionResult(),
                   pendingActionNotificationResponse.getTrid(),
-                      pendingActionNotificationResponse.processedDate));
+                  pendingActionNotificationResponse.processedDate);
+          pendingActionNotificationResponse = contactPendingResponse;
+          contactPendingActionNotificationResponses = ImmutableList.of(contactPendingResponse);
         } else if (fullyQualifiedDomainName != null) {
-          domainPendingActionNotificationResponses =
+          DomainPendingActionNotificationResponse domainPendingResponse =
-              ImmutableList.of(
               DomainPendingActionNotificationResponse.create(
                   pendingActionNotificationResponse.nameOrId.value,
                   pendingActionNotificationResponse.getActionResult(),
                   pendingActionNotificationResponse.getTrid(),
-                      pendingActionNotificationResponse.processedDate));
+                  pendingActionNotificationResponse.processedDate);
+          pendingActionNotificationResponse = domainPendingResponse;
+          domainPendingActionNotificationResponses = ImmutableList.of(domainPendingResponse);
         }
       }
       if (transferResponse != null) {
@@ -474,38 +477,35 @@ public abstract class PollMessage extends ImmutableObject
       }
 
       public Builder setResponseData(ImmutableList<? extends ResponseData> responseData) {
-        getInstance().contactPendingActionNotificationResponses =
+        OneTime instance = getInstance();
+        instance.contactPendingActionNotificationResponses =
             forceEmptyToNull(
-                responseData
+                responseData.stream()
-                    .stream()
                     .filter(ContactPendingActionNotificationResponse.class::isInstance)
                     .map(ContactPendingActionNotificationResponse.class::cast)
                     .collect(toImmutableList()));
 
-        getInstance().contactTransferResponses =
+        instance.contactTransferResponses =
             forceEmptyToNull(
-                responseData
+                responseData.stream()
-                    .stream()
                     .filter(ContactTransferResponse.class::isInstance)
                     .map(ContactTransferResponse.class::cast)
                     .collect(toImmutableList()));
 
-        getInstance().domainPendingActionNotificationResponses =
+        instance.domainPendingActionNotificationResponses =
             forceEmptyToNull(
-                responseData
+                responseData.stream()
-                    .stream()
                     .filter(DomainPendingActionNotificationResponse.class::isInstance)
                     .map(DomainPendingActionNotificationResponse.class::cast)
                     .collect(toImmutableList()));
-        getInstance().domainTransferResponses =
+        instance.domainTransferResponses =
             forceEmptyToNull(
-                responseData
+                responseData.stream()
-                    .stream()
                     .filter(DomainTransferResponse.class::isInstance)
                     .map(DomainTransferResponse.class::cast)
                     .collect(toImmutableList()));
 
-        getInstance().hostPendingActionNotificationResponses =
+        instance.hostPendingActionNotificationResponses =
             forceEmptyToNull(
                 responseData.stream()
                     .filter(HostPendingActionNotificationResponse.class::isInstance)
@@ -513,26 +513,30 @@ public abstract class PollMessage extends ImmutableObject
                     .collect(toImmutableList()));
 
         // Set the generic pending-action field as appropriate
-        if (getInstance().contactPendingActionNotificationResponses != null) {
+        if (instance.contactPendingActionNotificationResponses != null) {
-          getInstance().pendingActionNotificationResponse =
+          instance.pendingActionNotificationResponse =
-              getInstance().contactPendingActionNotificationResponses.get(0);
+              instance.contactPendingActionNotificationResponses.get(0);
-        } else if (getInstance().domainPendingActionNotificationResponses != null) {
+          instance.contactId =
-          getInstance().pendingActionNotificationResponse =
+              instance.contactPendingActionNotificationResponses.get(0).nameOrId.value;
-              getInstance().domainPendingActionNotificationResponses.get(0);
+        } else if (instance.domainPendingActionNotificationResponses != null) {
-        } else if (getInstance().hostPendingActionNotificationResponses != null) {
+          instance.pendingActionNotificationResponse =
-          getInstance().pendingActionNotificationResponse =
+              instance.domainPendingActionNotificationResponses.get(0);
-              getInstance().hostPendingActionNotificationResponses.get(0);
+          instance.fullyQualifiedDomainName =
+              instance.domainPendingActionNotificationResponses.get(0).nameOrId.value;
+        } else if (instance.hostPendingActionNotificationResponses != null) {
+          instance.pendingActionNotificationResponse =
+              instance.hostPendingActionNotificationResponses.get(0);
         }
         // Set the generic transfer response field as appropriate
-        if (getInstance().contactTransferResponses != null) {
+        if (instance.contactTransferResponses != null) {
-          getInstance().contactId = getInstance().contactTransferResponses.get(0).getContactId();
+          instance.contactId = getInstance().contactTransferResponses.get(0).getContactId();
-          getInstance().transferResponse = getInstance().contactTransferResponses.get(0);
+          instance.transferResponse = getInstance().contactTransferResponses.get(0);
-        } else if (getInstance().domainTransferResponses != null) {
+        } else if (instance.domainTransferResponses != null) {
-          getInstance().fullyQualifiedDomainName =
+          instance.fullyQualifiedDomainName =
-              getInstance().domainTransferResponses.get(0).getFullyQualifiedDomainName();
+              instance.domainTransferResponses.get(0).getFullyQualifiedDomainName();
-          getInstance().transferResponse = getInstance().domainTransferResponses.get(0);
+          instance.transferResponse = getInstance().domainTransferResponses.get(0);
-          getInstance().extendedRegistrationExpirationTime =
+          instance.extendedRegistrationExpirationTime =
-              getInstance().domainTransferResponses.get(0).getExtendedRegistrationExpirationTime();
+              instance.domainTransferResponses.get(0).getExtendedRegistrationExpirationTime();
         }
         return this;
       }

core/src/main/java/google/registry/model/smd/SignedMarkRevocationListDao.java

@@ -25,6 +25,7 @@ import static google.registry.model.ofy.ObjectifyService.allocateId;
 import static google.registry.model.ofy.ObjectifyService.ofy;
 import static google.registry.model.smd.SignedMarkRevocationList.SHARD_SIZE;
 import static google.registry.persistence.transaction.TransactionManagerFactory.jpaTm;
+import static google.registry.persistence.transaction.TransactionManagerFactory.ofyTm;
 import static google.registry.persistence.transaction.TransactionManagerFactory.tm;
 import static google.registry.util.CollectionUtils.isNullOrEmpty;
 import static google.registry.util.DateTimeUtils.START_OF_TIME;
@@ -127,7 +128,8 @@ public class SignedMarkRevocationListDao {
 
   /** Loads the shards from Datastore and combines them into one list. */
   private static Optional<SignedMarkRevocationList> loadFromDatastore() {
-    return tm().transactNewReadOnly(
+    return ofyTm()
+        .transactNewReadOnly(
             () -> {
               Iterable<SignedMarkRevocationList> shards =
                   ofy().load().type(SignedMarkRevocationList.class).ancestor(getCrossTldKey());

core/src/main/java/google/registry/model/tmch/ClaimsListShard.java

@@ -20,7 +20,7 @@ import static com.google.common.base.Throwables.throwIfUnchecked;
 import static com.google.common.base.Verify.verify;
 import static google.registry.model.ofy.ObjectifyService.allocateId;
 import static google.registry.model.ofy.ObjectifyService.ofy;
-import static google.registry.persistence.transaction.TransactionManagerFactory.tm;
+import static google.registry.persistence.transaction.TransactionManagerFactory.ofyTm;
 import static google.registry.util.DateTimeUtils.START_OF_TIME;
 
 import com.google.common.annotations.VisibleForTesting;
@@ -157,7 +157,8 @@ public class ClaimsListShard extends ImmutableObject implements NonReplicatedEnt
           Concurrent.transform(
               shardKeys,
               key ->
-                  tm().transactNewReadOnly(
+                  ofyTm()
+                      .transactNewReadOnly(
                           () -> {
                             ClaimsListShard claimsListShard = ofy().load().key(key).now();
                             checkState(
@@ -244,7 +245,8 @@ public class ClaimsListShard extends ImmutableObject implements NonReplicatedEnt
     Concurrent.transform(
         CollectionUtils.partitionMap(labelsToKeys, shardSize),
         (final ImmutableMap<String, String> labelsToKeysShard) ->
-            tm().transact(
+            ofyTm()
+                .transact(
                     () -> {
                       ClaimsListShard shard = create(creationTime, labelsToKeysShard);
                       shard.isShard = true;
@@ -254,7 +256,8 @@ public class ClaimsListShard extends ImmutableObject implements NonReplicatedEnt
                     }));
 
     // Persist the new revision, thus causing the newly created shards to go live.
-    tm().transact(
+    ofyTm()
+        .transact(
             () -> {
               verify(
                   (getCurrentRevision() == null && oldRevision == null)

core/src/main/java/google/registry/persistence/transaction/JpaTransactionManagerImpl.java

@@ -219,16 +219,15 @@ public class JpaTransactionManagerImpl implements JpaTransactionManager {
     transact(work);
   }
 
+  // For now, read-only transactions and "transactNew" methods only create (or use existing)
+  // standard transactions. Attempting to use a read-only transaction can break larger transactions
+  // (if we were already in one) so we don't set read-only mode.
+  //
+  // TODO(gbrodman): If necessary, implement transactNew and readOnly transactions using Postgres
+  // savepoints, see https://www.postgresql.org/docs/8.1/sql-savepoint.html
   @Override
   public <T> T transactNewReadOnly(Supplier<T> work) {
-    return retrier.callWithRetry(
+    return retrier.callWithRetry(() -> transact(work), JpaRetries::isFailedQueryRetriable);
-        () ->
-            transact(
-                () -> {
-                  getEntityManager().createNativeQuery("SET TRANSACTION READ ONLY").executeUpdate();
-                  return work.get();
-                }),
-        JpaRetries::isFailedQueryRetriable);
   }
 
   @Override

core/src/test/java/google/registry/flows/ResourceFlowTestCase.java

@@ -121,6 +121,10 @@ public abstract class ResourceFlowTestCase<F extends Flow, R extends EppResource
    * Confirms that an EppResourceIndex entity exists in Datastore for a given resource.
    */
   protected static <T extends EppResource> void assertEppResourceIndexEntityFor(final T resource) {
+    if (!tm().isOfy()) {
+      // Indices aren't explicitly stored as objects in SQL
+      return;
+    }
     ImmutableList<EppResourceIndex> indices =
         Streams.stream(
                 ofy()

core/src/test/java/google/registry/persistence/transaction/TransactionManagerTest.java

@@ -137,7 +137,7 @@ public class TransactionManagerTest {
     assertThat(persisted).isEqualTo(theEntity);
   }
 
-  @TestOfyAndSql
+  @TestOfyOnly // read-only not implemented in SQL yet
   void transactNewReadOnly_throwsWhenWritingEntity() {
     assertEntityNotExist(theEntity);
     assertThrows(