From abd207330bfc40bde37a70aa00b97b65ad16e2db Mon Sep 17 00:00:00 2001 From: Shicong Huang Date: Wed, 3 Jun 2020 10:02:48 -0400 Subject: [PATCH] Add deleteAll method to TransactionManager (#604) * Add deleteAll method to TransactionManager * Rename deleteAll to delete * Add bucket.getLastWrittenTime() before second mutation --- .../ofy/DatastoreTransactionManager.java | 16 +- .../JpaTransactionManagerImpl.java | 10 +- .../transaction/TransactionManager.java | 7 +- .../transaction/TransactionManagerTest.java | 165 ++++++++++-------- 4 files changed, 116 insertions(+), 82 deletions(-) diff --git a/core/src/main/java/google/registry/model/ofy/DatastoreTransactionManager.java b/core/src/main/java/google/registry/model/ofy/DatastoreTransactionManager.java index 011fe2e83..07b59328d 100644 --- a/core/src/main/java/google/registry/model/ofy/DatastoreTransactionManager.java +++ b/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 ImmutableList load(Iterable> keys) { Iterator> 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 void delete(VKey key) { + public void delete(VKey key) { getOfy().delete().key(key.getOfyKey()).now(); } + + @Override + public void delete(Iterable> 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> list = + StreamSupport.stream(vKeys.spliterator(), false) + .map(VKey::getOfyKey) + .collect(toImmutableList()); + getOfy().delete().keys(list).now(); + } } diff --git a/core/src/main/java/google/registry/persistence/transaction/JpaTransactionManagerImpl.java b/core/src/main/java/google/registry/persistence/transaction/JpaTransactionManagerImpl.java index 09c3af271..b38cc7a89 100644 --- a/core/src/main/java/google/registry/persistence/transaction/JpaTransactionManagerImpl.java +++ b/core/src/main/java/google/registry/persistence/transaction/JpaTransactionManagerImpl.java @@ -278,7 +278,7 @@ public class JpaTransactionManagerImpl implements JpaTransactionManager { .getResultList()); } - private int internalDelete(VKey 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 void delete(VKey key) { + public void delete(VKey key) { internalDelete(key); } + @Override + public void delete(Iterable> vKeys) { + checkArgumentNotNull(vKeys, "vKeys must be specified"); + vKeys.forEach(this::internalDelete); + } + @Override public void assertDelete(VKey key) { if (internalDelete(key) != 1) { diff --git a/core/src/main/java/google/registry/persistence/transaction/TransactionManager.java b/core/src/main/java/google/registry/persistence/transaction/TransactionManager.java index 5c49b6a4a..3c522891e 100644 --- a/core/src/main/java/google/registry/persistence/transaction/TransactionManager.java +++ b/core/src/main/java/google/registry/persistence/transaction/TransactionManager.java @@ -115,7 +115,7 @@ public interface TransactionManager { T load(VKey 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 { ImmutableList loadAll(Class clazz); /** Deletes the entity by its id. */ - void delete(VKey key); + void delete(VKey key); + + /** Deletes the set of entities by their key id. */ + void delete(Iterable> keys); } diff --git a/core/src/test/java/google/registry/persistence/transaction/TransactionManagerTest.java b/core/src/test/java/google/registry/persistence/transaction/TransactionManagerTest.java index e9d83c9e4..af5d99b1e 100644 --- a/core/src/test/java/google/registry/persistence/transaction/TransactionManagerTest.java +++ b/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() { - FakeClock txnClock = fakeClock; - txnClock.advanceOneMilli(); + void getTransactionTime_throwsExceptionWhenNotInTransaction() { 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() { - assertThat(tm().transact(() -> tm().checkExists(theEntity))).isFalse(); + void transact_hasNoEffectWithPartialSuccess() { + 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() { - assertThat(tm().transact(() -> tm().checkExists(theEntity))).isFalse(); - fakeClock.advanceOneMilli(); + void transact_reusesExistingTransaction() { + assertEntityNotExist(theEntity); tm().transact(() -> tm().transact(() -> tm().saveNew(theEntity))); - fakeClock.advanceOneMilli(); - assertThat(tm().transact(() -> tm().checkExists(theEntity))).isTrue(); + assertEntityExists(theEntity); } @TestTemplate - public void saveNew_succeeds() { - assertThat(tm().transact(() -> tm().checkExists(theEntity))).isFalse(); - fakeClock.advanceOneMilli(); + void saveNew_succeeds() { + assertEntityNotExist(theEntity); tm().transact(() -> tm().saveNew(theEntity)); - fakeClock.advanceOneMilli(); - assertThat(tm().transact(() -> tm().checkExists(theEntity))).isTrue(); - fakeClock.advanceOneMilli(); + assertEntityExists(theEntity); assertThat(tm().transact(() -> tm().load(theEntity.key()))).isEqualTo(theEntity); } @TestTemplate - public void saveAllNew_succeeds() { - moreEntities.forEach( - entity -> assertThat(tm().transact(() -> tm().checkExists(entity))).isFalse()); - fakeClock.advanceOneMilli(); + void saveAllNew_succeeds() { + assertAllEntitiesNotExist(moreEntities); tm().transact(() -> tm().saveAllNew(moreEntities)); - fakeClock.advanceOneMilli(); - moreEntities.forEach( - entity -> assertThat(tm().transact(() -> tm().checkExists(entity))).isTrue()); + assertAllEntitiesExist(moreEntities); } @TestTemplate - public void saveNewOrUpdate_persistsNewEntity() { - assertThat(tm().transact(() -> tm().checkExists(theEntity))).isFalse(); - fakeClock.advanceOneMilli(); + void saveNewOrUpdate_persistsNewEntity() { + assertEntityNotExist(theEntity); tm().transact(() -> tm().saveNewOrUpdate(theEntity)); - fakeClock.advanceOneMilli(); - assertThat(tm().transact(() -> tm().checkExists(theEntity))).isTrue(); - fakeClock.advanceOneMilli(); + assertEntityExists(theEntity); assertThat(tm().transact(() -> tm().load(theEntity.key()))).isEqualTo(theEntity); } @TestTemplate - public void saveNewOrUpdate_updatesExistingEntity() { - fakeClock.advanceOneMilli(); + void saveNewOrUpdate_updatesExistingEntity() { 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() { - moreEntities.forEach( - entity -> assertThat(tm().transact(() -> tm().checkExists(entity))).isFalse()); - fakeClock.advanceOneMilli(); + void saveNewOrUpdateAll_succeeds() { + assertAllEntitiesNotExist(moreEntities); tm().transact(() -> tm().saveNewOrUpdateAll(moreEntities)); - fakeClock.advanceOneMilli(); - moreEntities.forEach( - entity -> assertThat(tm().transact(() -> tm().checkExists(entity))).isTrue()); + assertAllEntitiesExist(moreEntities); } @TestTemplate - public void update_succeeds() { - fakeClock.advanceOneMilli(); + void update_succeeds() { 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() { - assertThat(tm().transact(() -> tm().checkExists(theEntity))).isFalse(); - fakeClock.advanceOneMilli(); + void load_succeeds() { + assertEntityNotExist(theEntity); 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() { - assertThat(tm().transact(() -> tm().checkExists(theEntity))).isFalse(); - fakeClock.advanceOneMilli(); + void load_throwsOnMissingElement() { + assertEntityNotExist(theEntity); assertThrows( NoSuchElementException.class, () -> tm().transact(() -> tm().load(theEntity.key()))); } @TestTemplate - public void maybeLoad_succeeds() { - assertThat(tm().transact(() -> tm().checkExists(theEntity))).isFalse(); - fakeClock.advanceOneMilli(); + void maybeLoad_succeeds() { + assertEntityNotExist(theEntity); 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() { - assertThat(tm().transact(() -> tm().checkExists(theEntity))).isFalse(); - fakeClock.advanceOneMilli(); + void maybeLoad_nonExistentObject() { + assertEntityNotExist(theEntity); assertThat(tm().transact(() -> tm().maybeLoad(theEntity.key())).isPresent()).isFalse(); } @TestTemplate - public void delete_succeeds() { - fakeClock.advanceOneMilli(); + void delete_succeeds() { tm().transact(() -> tm().saveNew(theEntity)); - fakeClock.advanceOneMilli(); - assertThat(tm().transact(() -> tm().checkExists(theEntity))).isTrue(); + assertEntityExists(theEntity); fakeClock.advanceOneMilli(); tm().transact(() -> tm().delete(theEntity.key())); - fakeClock.advanceOneMilli(); - assertThat(tm().transact(() -> tm().checkExists(theEntity))).isFalse(); + assertEntityNotExist(theEntity); } @TestTemplate - public void delete_returnsZeroWhenNoEntity() { - assertThat(tm().transact(() -> tm().checkExists(theEntity))).isFalse(); - fakeClock.advanceOneMilli(); + void delete_doNothingWhenEntityNotExist() { + assertEntityNotExist(theEntity); tm().transact(() -> tm().delete(theEntity.key())); + assertEntityNotExist(theEntity); + } + + @TestTemplate + void delete_succeedsForEntitySet() { + assertAllEntitiesNotExist(moreEntities); + tm().transact(() -> tm().saveAllNew(moreEntities)); + Set> 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> 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 entities) { + entities.forEach(TransactionManagerTest::assertEntityExists); + } + + private static void assertAllEntitiesNotExist(ImmutableList entities) { + entities.forEach(TransactionManagerTest::assertEntityNotExist); } @Entity(name = "TestEntity")