Remove the withoutBackup methods from TransactionManager (#1723)

* Remove the withoutBackup methods from TransactionManager

This doesn't do anything in Cloud SQL (it was for Ofy only).

core/src/main/java/google/registry/batch/DeleteProberDataAction.java

@@ -266,7 +266,7 @@ public class DeleteProberDataAction implements Runnable {
     // Note that we don't bother handling grace periods, billing events, pending transfers, poll
     // messages, or auto-renews because those will all be hard-deleted the next time the job runs
     // anyway.
-    tm().putAllWithoutBackup(ImmutableList.of(deletedDomain, historyEntry));
+    tm().putAll(ImmutableList.of(deletedDomain, historyEntry));
     // updating foreign keys is a no-op in SQL
     updateForeignKeyIndexDeletionTime(deletedDomain);
     dnsQueue.addDomainRefreshTask(deletedDomain.getDomainName());

core/src/main/java/google/registry/model/common/DatabaseMigrationStateSchedule.java

@@ -197,7 +197,7 @@ public class DatabaseMigrationStateSchedule extends CrossTldSingleton {
             MigrationState.DATASTORE_ONLY,
             "migrationTransitionMap must start with DATASTORE_ONLY");
     validateTransitionAtCurrentTime(transitions);
-    jpaTm().putWithoutBackup(new DatabaseMigrationStateSchedule(transitions));
+    jpaTm().put(new DatabaseMigrationStateSchedule(transitions));
     CACHE.invalidateAll();
   }
 

core/src/main/java/google/registry/model/server/ServerSecret.java

@@ -66,7 +66,7 @@ public class ServerSecret extends CrossTldSingleton {
               Optional<ServerSecret> secret = tm().loadSingleton(ServerSecret.class);
               if (!secret.isPresent()) {
                 secret = Optional.of(create(UUID.randomUUID()));
-                tm().insertWithoutBackup(secret.get());
+                tm().insert(secret.get());
               }
               return secret.get();
             });

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

@@ -56,7 +56,7 @@ public final class TmchCrl extends CrossTldSingleton {
               tmchCrl.updated = jpaTm().getTransactionTime();
               tmchCrl.crl = checkNotNull(crl, "crl");
               tmchCrl.url = checkNotNull(url, "url");
-              jpaTm().transactNew(() -> jpaTm().putWithoutBackup(tmchCrl));
+              jpaTm().put(tmchCrl);
             });
   }
 

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

@@ -293,16 +293,6 @@ public class JpaTransactionManagerImpl implements JpaTransactionManager {
     insertAll(ImmutableSet.copyOf(entities));
   }
 
-  @Override
-  public void insertWithoutBackup(ImmutableObject entity) {
-    insert(entity);
-  }
-
-  @Override
-  public void insertAllWithoutBackup(ImmutableCollection<?> entities) {
-    insertAll(entities);
-  }
-
   @Override
   public void put(Object entity) {
     checkArgumentNotNull(entity, "entity must be specified");
@@ -329,16 +319,6 @@ public class JpaTransactionManagerImpl implements JpaTransactionManager {
     entities.forEach(this::put);
   }
 
-  @Override
-  public void putWithoutBackup(ImmutableObject entity) {
-    put(entity);
-  }
-
-  @Override
-  public void putAllWithoutBackup(ImmutableCollection<?> entities) {
-    putAll(entities);
-  }
-
   @Override
   public void update(Object entity) {
     checkArgumentNotNull(entity, "entity must be specified");
@@ -362,16 +342,6 @@ public class JpaTransactionManagerImpl implements JpaTransactionManager {
     updateAll(ImmutableList.copyOf(entities));
   }
 
-  @Override
-  public void updateWithoutBackup(ImmutableObject entity) {
-    update(entity);
-  }
-
-  @Override
-  public void updateAllWithoutBackup(ImmutableCollection<?> entities) {
-    updateAll(entities);
-  }
-
   @Override
   public <T> boolean exists(VKey<T> key) {
     checkArgumentNotNull(key, "key must be specified");
@@ -549,21 +519,6 @@ public class JpaTransactionManagerImpl implements JpaTransactionManager {
     return managedEntity;
   }
 
-  @Override
-  public void deleteWithoutBackup(VKey<?> key) {
-    delete(key);
-  }
-
-  @Override
-  public void deleteWithoutBackup(Iterable<? extends VKey<?>> keys) {
-    delete(keys);
-  }
-
-  @Override
-  public void deleteWithoutBackup(Object entity) {
-    delete(entity);
-  }
-
   @Override
   public <T> QueryComposer<T> createQueryComposer(Class<T> entity) {
     return new JpaQueryComposerImpl<>(entity);

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

@@ -101,30 +101,6 @@ public interface TransactionManager {
   /** Persists all new entities in the database, throws exception if any entity already exists. */
   void insertAll(ImmutableObject... entities);
 
-  /**
-   * Persists a new entity in the database without writing any backup if the underlying database is
-   * Datastore.
-   *
-   * <p>This method is for the sake of keeping a single code path when replacing ofy() with tm() in
-   * the application code. When the method is invoked with Datastore, it won't write the commit log
-   * backup; when invoked with Cloud SQL, it behaves the same as the method which doesn't have
-   * "WithoutBackup" in its method name because it is not necessary to have the backup mechanism in
-   * SQL.
-   */
-  void insertWithoutBackup(ImmutableObject entity);
-
-  /**
-   * Persists all new entities in the database without writing any backup if the underlying database
-   * is Datastore.
-   *
-   * <p>This method is for the sake of keeping a single code path when replacing ofy() with tm() in
-   * the application code. When the method is invoked with Datastore, it won't write the commit log
-   * backup; when invoked with Cloud SQL, it behaves the same as the method which doesn't have
-   * "WithoutBackup" in its method name because it is not necessary to have the backup mechanism in
-   * SQL.
-   */
-  void insertAllWithoutBackup(ImmutableCollection<?> entities);
-
   /** Persists a new entity or update the existing entity in the database. */
   void put(Object entity);
 
@@ -134,30 +110,6 @@ public interface TransactionManager {
   /** Persists all new entities or updates the existing entities in the database. */
   void putAll(ImmutableCollection<?> entities);
 
-  /**
-   * Persists a new entity or update the existing entity in the database without writing commit logs
-   * if the underlying database is Datastore.
-   *
-   * <p>This method is for the sake of keeping a single code path when replacing ofy() with tm() in
-   * the application code. When the method is invoked with Datastore, it won't write the commit log
-   * backup; when invoked with Cloud SQL, it behaves the same as the method which doesn't have
-   * "WithoutBackup" in its method name because it is not necessary to have the backup mechanism in
-   * SQL.
-   */
-  void putWithoutBackup(ImmutableObject entity);
-
-  /**
-   * Persists all new entities or update the existing entities in the database without writing
-   * commit logs if the underlying database is Datastore.
-   *
-   * <p>This method is for the sake of keeping a single code path when replacing ofy() with tm() in
-   * the application code. When the method is invoked with Datastore, it won't write the commit log
-   * backup; when invoked with Cloud SQL, it behaves the same as the method which doesn't have
-   * "WithoutBackup" in its method name because it is not necessary to have the backup mechanism in
-   * SQL.
-   */
-  void putAllWithoutBackup(ImmutableCollection<?> entities);
-
   /** Updates an entity in the database, throws exception if the entity does not exist. */
   void update(Object entity);
 
@@ -167,30 +119,6 @@ public interface TransactionManager {
   /** Updates all entities in the database, throws exception if any entity does not exist. */
   void updateAll(ImmutableObject... entities);
 
-  /**
-   * Updates an entity in the database without writing commit logs if the underlying database is
-   * Datastore.
-   *
-   * <p>This method is for the sake of keeping a single code path when replacing ofy calls with tm()
-   * in the application code. When the method is invoked with Datastore, it won't write the commit
-   * log backup; when invoked with Cloud SQL, it behaves the same as the method which doesn't have
-   * "WithoutBackup" in its method name because it is not necessary to have the backup mechanism in
-   * SQL.
-   */
-  void updateWithoutBackup(ImmutableObject entity);
-
-  /**
-   * Updates all entities in the database without writing any backup if the underlying database is
-   * Datastore.
-   *
-   * <p>This method is for the sake of keeping a single code path when replacing ofy calls with tm()
-   * in the application code. When the method is invoked with Datastore, it won't write the commit
-   * log backup; when invoked with Cloud SQL, it behaves the same as the method which doesn't have
-   * "WithoutBackup" in its method name because it is not necessary to have the backup mechanism in
-   * SQL.
-   */
-  void updateAllWithoutBackup(ImmutableCollection<?> entities);
-
   /** Returns whether the given entity with same ID exists. */
   boolean exists(Object entity);
 
@@ -285,24 +213,6 @@ public interface TransactionManager {
    */
   <T> T delete(T entity);
 
-  /**
-   * Deletes the entity by its id without writing commit logs if the underlying database is
-   * Datastore.
-   */
-  void deleteWithoutBackup(VKey<?> key);
-
-  /**
-   * Deletes the set of entities by their key id without writing commit logs if the underlying
-   * database is Datastore.
-   */
-  void deleteWithoutBackup(Iterable<? extends VKey<?>> keys);
-
-  /**
-   * Deletes the given entity from the database without writing commit logs if the underlying
-   * database is Datastore.
-   */
-  void deleteWithoutBackup(Object entity);
-
   /** Returns a QueryComposer which can be used to perform queries against the current database. */
   <T> QueryComposer<T> createQueryComposer(Class<T> entity);
 

core/src/test/java/google/registry/export/SyncGroupMembersActionTest.java

@@ -130,8 +130,7 @@ public class SyncGroupMembersActionTest {
   void test_doPost_removesAllContactsFromGroup() throws Exception {
     when(connection.getMembersOfGroup("newregistrar-primary-contacts@domain-registry.example"))
         .thenReturn(ImmutableSet.of("defunct@example.com", "janedoe@theregistrar.com"));
-    tm().transact(
-            () -> loadRegistrar("NewRegistrar").getContacts().forEach(tm()::deleteWithoutBackup));
+    tm().transact(() -> loadRegistrar("NewRegistrar").getContacts().forEach(tm()::delete));
     runAction();
     verify(connection).removeMemberFromGroup(
         "newregistrar-primary-contacts@domain-registry.example", "defunct@example.com");

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

@@ -173,16 +173,6 @@ public class ReplicaSimulatingJpaTransactionManager implements JpaTransactionMan
     delegate.insertAll(entities);
   }
 
-  @Override
-  public void insertWithoutBackup(ImmutableObject entity) {
-    delegate.insertWithoutBackup(entity);
-  }
-
-  @Override
-  public void insertAllWithoutBackup(ImmutableCollection<?> entities) {
-    delegate.insertAllWithoutBackup(entities);
-  }
-
   @Override
   public void put(Object entity) {
     delegate.put(entity);
@@ -198,16 +188,6 @@ public class ReplicaSimulatingJpaTransactionManager implements JpaTransactionMan
     delegate.putAll(entities);
   }
 
-  @Override
-  public void putWithoutBackup(ImmutableObject entity) {
-    delegate.putWithoutBackup(entity);
-  }
-
-  @Override
-  public void putAllWithoutBackup(ImmutableCollection<?> entities) {
-    delegate.putAllWithoutBackup(entities);
-  }
-
   @Override
   public void update(Object entity) {
     delegate.update(entity);
@@ -223,16 +203,6 @@ public class ReplicaSimulatingJpaTransactionManager implements JpaTransactionMan
     delegate.updateAll(entities);
   }
 
-  @Override
-  public void updateWithoutBackup(ImmutableObject entity) {
-    delegate.updateWithoutBackup(entity);
-  }
-
-  @Override
-  public void updateAllWithoutBackup(ImmutableCollection<?> entities) {
-    delegate.updateAllWithoutBackup(entities);
-  }
-
   @Override
   public <T> boolean exists(VKey<T> key) {
     return delegate.exists(key);
@@ -310,21 +280,6 @@ public class ReplicaSimulatingJpaTransactionManager implements JpaTransactionMan
     return delegate.delete(entity);
   }
 
-  @Override
-  public void deleteWithoutBackup(VKey<?> key) {
-    delegate.deleteWithoutBackup(key);
-  }
-
-  @Override
-  public void deleteWithoutBackup(Iterable<? extends VKey<?>> keys) {
-    delegate.deleteWithoutBackup(keys);
-  }
-
-  @Override
-  public void deleteWithoutBackup(Object entity) {
-    delegate.deleteWithoutBackup(entity);
-  }
-
   @Override
   public <T> QueryComposer<T> createQueryComposer(Class<T> entity) {
     return delegate.createQueryComposer(entity);

core/src/test/java/google/registry/rde/RdeFixtures.java

@@ -18,7 +18,6 @@ import static com.google.common.io.BaseEncoding.base16;
 import static google.registry.testing.DatabaseHelper.generateNewContactHostRoid;
 import static google.registry.testing.DatabaseHelper.generateNewDomainRoid;
 import static google.registry.testing.DatabaseHelper.persistResource;
-import static google.registry.testing.DatabaseHelper.persistResourceWithBackup;
 import static google.registry.testing.DatabaseHelper.persistSimpleResource;
 import static google.registry.util.DateTimeUtils.END_OF_TIME;
 import static org.joda.money.CurrencyUnit.USD;
@@ -76,7 +75,7 @@ final class RdeFixtures {
                 .build());
     clock.advanceOneMilli();
     BillingEvent.OneTime billingEvent =
-        persistResourceWithBackup(
+        persistResource(
             new BillingEvent.OneTime.Builder()
                 .setReason(Reason.CREATE)
                 .setTargetId("example." + tld)
@@ -219,13 +218,13 @@ final class RdeFixtures {
                     .build())
             .build();
     clock.advanceOneMilli();
-    return persistResourceWithBackup(domain);
+    return persistResource(domain);
   }
 
   static ContactResource makeContactResource(
       FakeClock clock, String id, String name, String email) {
     clock.advanceOneMilli();
-    return persistResourceWithBackup(
+    return persistResource(
         new ContactResource.Builder()
             .setContactId(id)
             .setRepoId(generateNewContactHostRoid())
@@ -256,7 +255,7 @@ final class RdeFixtures {
 
   static HostResource makeHostResource(FakeClock clock, String fqhn, String ip) {
     clock.advanceOneMilli();
-    return persistResourceWithBackup(
+    return persistResource(
         new HostResource.Builder()
             .setRepoId(generateNewContactHostRoid())
             .setCreationRegistrarId("LawyerCat")

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

@@ -999,31 +999,9 @@ public class DatabaseHelper {
     return createRepoId(allocateId(), getContactAndHostRoidSuffix());
   }
 
-  /**
-   * Persists a test resource to Datastore and returns it.
-   *
-   * <p>Tests should always use this method (or the shortcut persist methods in this class) to
-   * persist test data, to avoid potentially subtle bugs related to race conditions and a stale ofy
-   * session cache. Specifically, this method calls .now() on the save to force the write to
-   * actually get sent to Datastore (although it does not force it to be applied) and clears the
-   * session cache. If necessary, this method also updates the relevant {@link EppResourceIndex},
-   * {@link ForeignKeyIndex}.
-   *
-   * <p><b>Note:</b> Your resource will not be enrolled in a commit log. If you want backups, use
-   * {@link #persistResourceWithBackup(Object)}.
-   */
-  public static <R extends ImmutableObject> R persistResource(final R resource) {
-    return persistResource(resource, false);
-  }
-
-  /** Same as {@link #persistResource(Object)} with backups enabled. */
-  public static <R extends ImmutableObject> R persistResourceWithBackup(final R resource) {
-    return persistResource(resource, true);
-  }
-
-  private static <R extends ImmutableObject> void saveResource(R resource, boolean wantBackup) {
+  private static <R extends ImmutableObject> void saveResource(R resource) {
     if (tm().isOfy()) {
-      Consumer<ImmutableObject> saver = wantBackup ? tm()::put : tm()::putWithoutBackup;
+      Consumer<ImmutableObject> saver = tm()::put;
       saver.accept(resource);
       if (resource instanceof EppResource) {
         EppResource eppResource = (EppResource) resource;
@@ -1046,12 +1024,12 @@ public class DatabaseHelper {
     }
   }
 
-  private static <R extends ImmutableObject> R persistResource(
-      final R resource, final boolean wantBackup) {
+  /** Persists an object in the DB for tests. */
+  public static <R extends ImmutableObject> R persistResource(final R resource) {
     assertWithMessage("Attempting to persist a Builder is almost certainly an error in test code")
         .that(resource)
         .isNotInstanceOf(Buildable.Builder.class);
-    tm().transact(() -> saveResource(resource, wantBackup));
+    tm().transact(() -> saveResource(resource));
     maybeAdvanceClock();
     // Force the session cache to be cleared so that when we read the resource back, we read from
     // Datastore and not from the session cache. This is needed to trigger Objectify's load process
@@ -1080,12 +1058,8 @@ public class DatabaseHelper {
     return tm().transact(() -> tm().loadByEntity(resource));
   }
 
+  /** Persists the specified resources to the DB. */
   public static <R extends ImmutableObject> void persistResources(final Iterable<R> resources) {
-    persistResources(resources, false);
-  }
-
-  private static <R extends ImmutableObject> void persistResources(
-      final Iterable<R> resources, final boolean wantBackup) {
     for (R resource : resources) {
       assertWithMessage("Attempting to persist a Builder is almost certainly an error in test code")
           .that(resource)
@@ -1093,7 +1067,7 @@ public class DatabaseHelper {
     }
     // Persist domains ten at a time, to avoid exceeding the entity group limit.
     for (final List<R> chunk : Iterables.partition(resources, 10)) {
-      tm().transact(() -> chunk.forEach(resource -> saveResource(resource, wantBackup)));
+      tm().transact(() -> chunk.forEach(DatabaseHelper::saveResource));
       maybeAdvanceClock();
     }
     // Force the session to be cleared so that when we read it back, we read from Datastore
@@ -1248,7 +1222,7 @@ public class DatabaseHelper {
    * entities.
    */
   public static <R> void insertSimpleResources(final Iterable<R> resources) {
-    tm().transact(() -> tm().putAllWithoutBackup(ImmutableList.copyOf(resources)));
+    tm().transact(() -> tm().putAll(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.
@@ -1256,7 +1230,7 @@ public class DatabaseHelper {
   }
 
   public static void deleteResource(final Object resource) {
-    tm().transact(() -> tm().deleteWithoutBackup(resource));
+    tm().transact(() -> tm().delete(resource));
     // 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();

core/src/test/java/google/registry/tools/MutatingCommandTest.java

@@ -122,7 +122,7 @@ public class MutatingCommandTest {
         Arrays.asList(host1, host2, registrar1, registrar2).stream()
             .map(entity -> VKey.from(Key.create(entity)))
             .collect(toImmutableList());
-    tm().transact(() -> tm().deleteWithoutBackup(keys));
+    tm().transact(() -> tm().delete(keys));
     MutatingCommand command = new MutatingCommand() {
       @Override
       protected void init() {