Add deleteAll method to TransactionManager (#604)

* Add deleteAll method to TransactionManager

* Rename deleteAll to delete

* Add bucket.getLastWrittenTime() before second mutation

core/src/main/java/google/registry/model/ofy/DatastoreTransactionManager.java

@@ -14,6 +14,7 @@
 
 package google.registry.model.ofy;
 
+import static com.google.common.collect.ImmutableList.toImmutableList;
 import static google.registry.model.ofy.ObjectifyService.ofy;
 import static google.registry.util.PreconditionsUtils.checkArgumentNotNull;
 
@@ -157,7 +158,7 @@ public class DatastoreTransactionManager implements TransactionManager {
   @Override
   public <T> ImmutableList<T> load(Iterable<VKey<T>> keys) {
     Iterator<Key<T>> iter =
-        StreamSupport.stream(keys.spliterator(), false).map(key -> key.getOfyKey()).iterator();
+        StreamSupport.stream(keys.spliterator(), false).map(VKey::getOfyKey).iterator();
 
     // The lambda argument to keys() effectively converts Iterator -> Iterable.
     return ImmutableList.copyOf(getOfy().load().keys(() -> iter).values());
@@ -170,7 +171,18 @@ public class DatastoreTransactionManager implements TransactionManager {
   }
 
   @Override
-  public <T> void delete(VKey<T> key) {
+  public void delete(VKey<?> key) {
     getOfy().delete().key(key.getOfyKey()).now();
   }
+
+  @Override
+  public void delete(Iterable<? extends VKey<?>> vKeys) {
+    // We have to create a list to work around the wildcard capture issue here.
+    // See https://docs.oracle.com/javase/tutorial/java/generics/capture.html
+    ImmutableList<Key<?>> list =
+        StreamSupport.stream(vKeys.spliterator(), false)
+            .map(VKey::getOfyKey)
+            .collect(toImmutableList());
+    getOfy().delete().keys(list).now();
+  }
 }

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

@@ -278,7 +278,7 @@ public class JpaTransactionManagerImpl implements JpaTransactionManager {
             .getResultList());
   }
 
-  private <T> int internalDelete(VKey<T> key) {
+  private int internalDelete(VKey<?> key) {
     checkArgumentNotNull(key, "key must be specified");
     assertInTransaction();
     EntityType<?> entityType = getEntityType(key.getKind());
@@ -291,10 +291,16 @@ public class JpaTransactionManagerImpl implements JpaTransactionManager {
   }
 
   @Override
-  public <T> void delete(VKey<T> key) {
+  public void delete(VKey<?> key) {
     internalDelete(key);
   }
 
+  @Override
+  public void delete(Iterable<? extends VKey<?>> vKeys) {
+    checkArgumentNotNull(vKeys, "vKeys must be specified");
+    vKeys.forEach(this::internalDelete);
+  }
+
   @Override
   public <T> void assertDelete(VKey<T> key) {
     if (internalDelete(key) != 1) {

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

@@ -115,7 +115,7 @@ public interface TransactionManager {
   <T> T load(VKey<T> key);
 
   /**
-   * Leads the set of entities by their key id.
+   * Loads the set of entities by their key id.
    *
    * @throws NoSuchElementException if any of the keys are not found.
    */
@@ -125,5 +125,8 @@ public interface TransactionManager {
   <T> ImmutableList<T> loadAll(Class<T> clazz);
 
   /** Deletes the entity by its id. */
-  <T> void delete(VKey<T> key);
+  void delete(VKey<?> key);
+
+  /** Deletes the set of entities by their key id. */
+  void delete(Iterable<? extends VKey<?>> keys);
 }

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

@@ -14,6 +14,8 @@
 
 package google.registry.persistence.transaction;
 
+import static com.google.common.collect.ImmutableList.toImmutableList;
+import static com.google.common.collect.ImmutableSet.toImmutableSet;
 import static com.google.common.truth.Truth.assertThat;
 import static google.registry.persistence.transaction.TransactionManagerFactory.tm;
 import static org.junit.Assert.assertThrows;
@@ -30,7 +32,9 @@ import google.registry.testing.AppEngineRule;
 import google.registry.testing.DualDatabaseTest;
 import google.registry.testing.FakeClock;
 import google.registry.testing.InjectRule;
+import java.util.List;
 import java.util.NoSuchElementException;
+import java.util.Set;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.TestTemplate;
 import org.junit.jupiter.api.extension.RegisterExtension;
@@ -62,39 +66,38 @@ public class TransactionManagerTest {
           .withJpaUnitTestEntities(TestEntity.class)
           .build();
 
-  public TransactionManagerTest() {}
+  TransactionManagerTest() {}
 
   @BeforeEach
-  public void setUp() {
+  void setUp() {
     inject.setStaticField(Ofy.class, "clock", fakeClock);
+    fakeClock.advanceOneMilli();
   }
 
   @TestTemplate
-  public void inTransaction_returnsCorrespondingResult() {
+  void inTransaction_returnsCorrespondingResult() {
     assertThat(tm().inTransaction()).isFalse();
     tm().transact(() -> assertThat(tm().inTransaction()).isTrue());
     assertThat(tm().inTransaction()).isFalse();
   }
 
   @TestTemplate
-  public void assertInTransaction_throwsExceptionWhenNotInTransaction() {
+  void assertInTransaction_throwsExceptionWhenNotInTransaction() {
     assertThrows(IllegalStateException.class, () -> tm().assertInTransaction());
     tm().transact(() -> tm().assertInTransaction());
     assertThrows(IllegalStateException.class, () -> tm().assertInTransaction());
   }
 
   @TestTemplate
-  public void getTransactionTime_throwsExceptionWhenNotInTransaction() {
+  void getTransactionTime_throwsExceptionWhenNotInTransaction() {
-    FakeClock txnClock = fakeClock;
-    txnClock.advanceOneMilli();
     assertThrows(IllegalStateException.class, () -> tm().getTransactionTime());
-    tm().transact(() -> assertThat(tm().getTransactionTime()).isEqualTo(txnClock.nowUtc()));
+    tm().transact(() -> assertThat(tm().getTransactionTime()).isEqualTo(fakeClock.nowUtc()));
     assertThrows(IllegalStateException.class, () -> tm().getTransactionTime());
   }
 
   @TestTemplate
-  public void transact_hasNoEffectWithPartialSuccess() {
+  void transact_hasNoEffectWithPartialSuccess() {
-    assertThat(tm().transact(() -> tm().checkExists(theEntity))).isFalse();
+    assertEntityNotExist(theEntity);
     assertThrows(
         RuntimeException.class,
         () ->
@@ -104,152 +107,162 @@ public class TransactionManagerTest {
                       tm().saveNew(theEntity);
                       throw new RuntimeException();
                     }));
-    assertThat(tm().transact(() -> tm().checkExists(theEntity))).isFalse();
+    assertEntityNotExist(theEntity);
   }
 
   @TestTemplate
-  public void transact_reusesExistingTransaction() {
+  void transact_reusesExistingTransaction() {
-    assertThat(tm().transact(() -> tm().checkExists(theEntity))).isFalse();
+    assertEntityNotExist(theEntity);
-    fakeClock.advanceOneMilli();
     tm().transact(() -> tm().transact(() -> tm().saveNew(theEntity)));
-    fakeClock.advanceOneMilli();
+    assertEntityExists(theEntity);
-    assertThat(tm().transact(() -> tm().checkExists(theEntity))).isTrue();
   }
 
   @TestTemplate
-  public void saveNew_succeeds() {
+  void saveNew_succeeds() {
-    assertThat(tm().transact(() -> tm().checkExists(theEntity))).isFalse();
+    assertEntityNotExist(theEntity);
-    fakeClock.advanceOneMilli();
     tm().transact(() -> tm().saveNew(theEntity));
-    fakeClock.advanceOneMilli();
+    assertEntityExists(theEntity);
-    assertThat(tm().transact(() -> tm().checkExists(theEntity))).isTrue();
-    fakeClock.advanceOneMilli();
     assertThat(tm().transact(() -> tm().load(theEntity.key()))).isEqualTo(theEntity);
   }
 
   @TestTemplate
-  public void saveAllNew_succeeds() {
+  void saveAllNew_succeeds() {
-    moreEntities.forEach(
+    assertAllEntitiesNotExist(moreEntities);
-        entity -> assertThat(tm().transact(() -> tm().checkExists(entity))).isFalse());
-    fakeClock.advanceOneMilli();
     tm().transact(() -> tm().saveAllNew(moreEntities));
-    fakeClock.advanceOneMilli();
+    assertAllEntitiesExist(moreEntities);
-    moreEntities.forEach(
-        entity -> assertThat(tm().transact(() -> tm().checkExists(entity))).isTrue());
   }
 
   @TestTemplate
-  public void saveNewOrUpdate_persistsNewEntity() {
+  void saveNewOrUpdate_persistsNewEntity() {
-    assertThat(tm().transact(() -> tm().checkExists(theEntity))).isFalse();
+    assertEntityNotExist(theEntity);
-    fakeClock.advanceOneMilli();
     tm().transact(() -> tm().saveNewOrUpdate(theEntity));
-    fakeClock.advanceOneMilli();
+    assertEntityExists(theEntity);
-    assertThat(tm().transact(() -> tm().checkExists(theEntity))).isTrue();
-    fakeClock.advanceOneMilli();
     assertThat(tm().transact(() -> tm().load(theEntity.key()))).isEqualTo(theEntity);
   }
 
   @TestTemplate
-  public void saveNewOrUpdate_updatesExistingEntity() {
+  void saveNewOrUpdate_updatesExistingEntity() {
-    fakeClock.advanceOneMilli();
     tm().transact(() -> tm().saveNew(theEntity));
-    fakeClock.advanceOneMilli();
     TestEntity persisted = tm().transact(() -> tm().load(theEntity.key()));
     assertThat(persisted.data).isEqualTo("foo");
     theEntity.data = "bar";
+    fakeClock.advanceOneMilli();
     tm().transact(() -> tm().saveNewOrUpdate(theEntity));
-    fakeClock.advanceOneMilli();
     persisted = tm().transact(() -> tm().load(theEntity.key()));
-    fakeClock.advanceOneMilli();
     assertThat(persisted.data).isEqualTo("bar");
   }
 
   @TestTemplate
-  public void saveNewOrUpdateAll_succeeds() {
+  void saveNewOrUpdateAll_succeeds() {
-    moreEntities.forEach(
+    assertAllEntitiesNotExist(moreEntities);
-        entity -> assertThat(tm().transact(() -> tm().checkExists(entity))).isFalse());
-    fakeClock.advanceOneMilli();
     tm().transact(() -> tm().saveNewOrUpdateAll(moreEntities));
-    fakeClock.advanceOneMilli();
+    assertAllEntitiesExist(moreEntities);
-    moreEntities.forEach(
-        entity -> assertThat(tm().transact(() -> tm().checkExists(entity))).isTrue());
   }
 
   @TestTemplate
-  public void update_succeeds() {
+  void update_succeeds() {
-    fakeClock.advanceOneMilli();
     tm().transact(() -> tm().saveNew(theEntity));
-    fakeClock.advanceOneMilli();
     TestEntity persisted =
         tm().transact(
                 () ->
                     tm().load(
                             VKey.create(TestEntity.class, theEntity.name, Key.create(theEntity))));
-    fakeClock.advanceOneMilli();
     assertThat(persisted.data).isEqualTo("foo");
     theEntity.data = "bar";
-    tm().transact(() -> tm().update(theEntity));
     fakeClock.advanceOneMilli();
+    tm().transact(() -> tm().update(theEntity));
     persisted = tm().transact(() -> tm().load(theEntity.key()));
     assertThat(persisted.data).isEqualTo("bar");
   }
 
   @TestTemplate
-  public void load_succeeds() {
+  void load_succeeds() {
-    assertThat(tm().transact(() -> tm().checkExists(theEntity))).isFalse();
+    assertEntityNotExist(theEntity);
-    fakeClock.advanceOneMilli();
     tm().transact(() -> tm().saveNew(theEntity));
-    fakeClock.advanceOneMilli();
     TestEntity persisted = tm().transact(() -> tm().load(theEntity.key()));
     assertThat(persisted.name).isEqualTo("theEntity");
     assertThat(persisted.data).isEqualTo("foo");
   }
 
   @TestTemplate
-  public void load_throwsOnMissingElement() {
+  void load_throwsOnMissingElement() {
-    assertThat(tm().transact(() -> tm().checkExists(theEntity))).isFalse();
+    assertEntityNotExist(theEntity);
-    fakeClock.advanceOneMilli();
     assertThrows(
         NoSuchElementException.class, () -> tm().transact(() -> tm().load(theEntity.key())));
   }
 
   @TestTemplate
-  public void maybeLoad_succeeds() {
+  void maybeLoad_succeeds() {
-    assertThat(tm().transact(() -> tm().checkExists(theEntity))).isFalse();
+    assertEntityNotExist(theEntity);
-    fakeClock.advanceOneMilli();
     tm().transact(() -> tm().saveNew(theEntity));
-    fakeClock.advanceOneMilli();
     TestEntity persisted = tm().transact(() -> tm().maybeLoad(theEntity.key()).get());
     assertThat(persisted.name).isEqualTo("theEntity");
     assertThat(persisted.data).isEqualTo("foo");
   }
 
   @TestTemplate
-  public void maybeLoad_nonExistentObject() {
+  void maybeLoad_nonExistentObject() {
-    assertThat(tm().transact(() -> tm().checkExists(theEntity))).isFalse();
+    assertEntityNotExist(theEntity);
-    fakeClock.advanceOneMilli();
     assertThat(tm().transact(() -> tm().maybeLoad(theEntity.key())).isPresent()).isFalse();
   }
 
   @TestTemplate
-  public void delete_succeeds() {
+  void delete_succeeds() {
-    fakeClock.advanceOneMilli();
     tm().transact(() -> tm().saveNew(theEntity));
-    fakeClock.advanceOneMilli();
+    assertEntityExists(theEntity);
-    assertThat(tm().transact(() -> tm().checkExists(theEntity))).isTrue();
     fakeClock.advanceOneMilli();
     tm().transact(() -> tm().delete(theEntity.key()));
-    fakeClock.advanceOneMilli();
+    assertEntityNotExist(theEntity);
-    assertThat(tm().transact(() -> tm().checkExists(theEntity))).isFalse();
   }
 
   @TestTemplate
-  public void delete_returnsZeroWhenNoEntity() {
+  void delete_doNothingWhenEntityNotExist() {
-    assertThat(tm().transact(() -> tm().checkExists(theEntity))).isFalse();
+    assertEntityNotExist(theEntity);
-    fakeClock.advanceOneMilli();
     tm().transact(() -> tm().delete(theEntity.key()));
+    assertEntityNotExist(theEntity);
+  }
+
+  @TestTemplate
+  void delete_succeedsForEntitySet() {
+    assertAllEntitiesNotExist(moreEntities);
+    tm().transact(() -> tm().saveAllNew(moreEntities));
+    Set<VKey<TestEntity>> keys =
+        moreEntities.stream().map(TestEntity::key).collect(toImmutableSet());
+    assertAllEntitiesExist(moreEntities);
     fakeClock.advanceOneMilli();
-    assertThat(tm().transact(() -> tm().checkExists(theEntity))).isFalse();
+    tm().transact(() -> tm().delete(keys));
+    assertAllEntitiesNotExist(moreEntities);
+  }
+
+  @TestTemplate
+  void delete_ignoreNonExistentEntity() {
+    assertAllEntitiesNotExist(moreEntities);
+    tm().transact(() -> tm().saveAllNew(moreEntities));
+    List<VKey<TestEntity>> keys =
+        moreEntities.stream().map(TestEntity::key).collect(toImmutableList());
+    assertAllEntitiesExist(moreEntities);
+    fakeClock.advanceOneMilli();
+    tm().transact(() -> tm().delete(keys.get(0)));
+    assertEntityNotExist(moreEntities.get(0));
+    fakeClock.advanceOneMilli();
+    tm().transact(() -> tm().delete(keys));
+    assertAllEntitiesNotExist(moreEntities);
+  }
+
+  private static void assertEntityExists(TestEntity entity) {
+    assertThat(tm().transact(() -> tm().checkExists(entity))).isTrue();
+  }
+
+  private static void assertEntityNotExist(TestEntity entity) {
+    assertThat(tm().transact(() -> tm().checkExists(entity))).isFalse();
+  }
+
+  private static void assertAllEntitiesExist(ImmutableList<TestEntity> entities) {
+    entities.forEach(TransactionManagerTest::assertEntityExists);
+  }
+
+  private static void assertAllEntitiesNotExist(ImmutableList<TestEntity> entities) {
+    entities.forEach(TransactionManagerTest::assertEntityNotExist);
   }
 
   @Entity(name = "TestEntity")