Implement all DatastoreTransactionManager methods (#581)

* Implement all DatastoreTransactionManager methods

In the course of this:

- Make assertDelete() specific to JpaTransactionManager, remove the return
  value from delete()
- Converter "in transaction" assertion to IllegalStateException, which is less
  JPA specific.

* Upgraded DatastoreTransactionManagerTest to junit5

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

@@ -15,6 +15,7 @@
 package google.registry.model.ofy;
 
 import static google.registry.model.ofy.ObjectifyService.ofy;
+import static google.registry.util.PreconditionsUtils.checkArgumentNotNull;
 
 import com.google.common.collect.ImmutableCollection;
 import com.google.common.collect.ImmutableList;
@@ -94,42 +95,45 @@ public class DatastoreTransactionManager implements TransactionManager {
 
   @Override
   public void saveNew(Object entity) {
-    throw new UnsupportedOperationException("Not available in the Datastore transaction manager");
+    checkArgumentNotNull(entity, "entity must be specified");
+    getOfy().save().entity(entity);
   }
 
   @Override
   public void saveAllNew(ImmutableCollection<?> entities) {
-    throw new UnsupportedOperationException("Not available in the Datastore transaction manager");
+    getOfy().save().entities(entities);
   }
 
   @Override
   public void saveNewOrUpdate(Object entity) {
-    throw new UnsupportedOperationException("Not available in the Datastore transaction manager");
+    checkArgumentNotNull(entity, "entity must be specified");
+    getOfy().save().entity(entity);
   }
 
   @Override
   public void saveNewOrUpdateAll(ImmutableCollection<?> entities) {
-    throw new UnsupportedOperationException("Not available in the Datastore transaction manager");
+    getOfy().save().entities(entities);
   }
 
   @Override
   public void update(Object entity) {
-    throw new UnsupportedOperationException("Not available in the Datastore transaction manager");
+    checkArgumentNotNull(entity, "entity must be specified");
+    getOfy().save().entity(entity);
   }
 
   @Override
   public void updateAll(ImmutableCollection<?> entities) {
-    throw new UnsupportedOperationException("Not available in the Datastore transaction manager");
+    getOfy().save().entities(entities);
   }
 
   @Override
   public boolean checkExists(Object entity) {
-    throw new UnsupportedOperationException("Not available in the Datastore transaction manager");
+    return getOfy().load().key(Key.create(entity)).now() != null;
   }
 
   @Override
   public <T> boolean checkExists(VKey<T> key) {
-    throw new UnsupportedOperationException("Not available in the Datastore transaction manager");
+    return getOfy().load().key(key.getOfyKey()).now() != null;
   }
 
   // TODO: add tests for these methods.  They currently have some degree of test coverage because
@@ -138,7 +142,7 @@ public class DatastoreTransactionManager implements TransactionManager {
   // interface tests that are applied to both the datastore and SQL implementations.
   @Override
   public <T> Optional<T> maybeLoad(VKey<T> key) {
-    return Optional.of(getOfy().load().key(key.getOfyKey()).now());
+    return Optional.ofNullable(getOfy().load().key(key.getOfyKey()).now());
   }
 
   @Override
@@ -161,16 +165,12 @@ public class DatastoreTransactionManager implements TransactionManager {
 
   @Override
   public <T> ImmutableList<T> loadAll(Class<T> clazz) {
+    // We can do a ofy().load().type(clazz), but this doesn't work in a transaction.
     throw new UnsupportedOperationException("Not available in the Datastore transaction manager");
   }
 
   @Override
-  public <T> int delete(VKey<T> key) {
+  public <T> void delete(VKey<T> key) {
-    throw new UnsupportedOperationException("Not available in the Datastore transaction manager");
+    getOfy().delete().key(key.getOfyKey()).now();
-  }
-
-  @Override
-  public <T> void assertDelete(VKey<T> key) {
-    throw new UnsupportedOperationException("Not available in the Datastore transaction manager");
   }
 }

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

@@ -14,6 +14,7 @@
 
 package google.registry.persistence.transaction;
 
+import google.registry.persistence.VKey;
 import javax.persistence.EntityManager;
 
 /** Sub-interface of {@link TransactionManager} which defines JPA related methods. */
@@ -21,4 +22,7 @@ public interface JpaTransactionManager extends TransactionManager {
 
   /** Returns the {@link EntityManager} for the current request. */
   EntityManager getEntityManager();
+
+  /** Deletes the entity by its id, throws exception if the entity is not deleted. */
+  public abstract <T> void assertDelete(VKey<T> key);
 }

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

@@ -77,7 +77,7 @@ public class JpaTransactionManagerImpl implements JpaTransactionManager {
   @Override
   public void assertInTransaction() {
     if (!inTransaction()) {
-      throw new PersistenceException("Not in a transaction");
+      throw new IllegalStateException("Not in a transaction");
     }
   }
 
@@ -278,8 +278,7 @@ public class JpaTransactionManagerImpl implements JpaTransactionManager {
             .getResultList());
   }
 
-  @Override
+  private <T> int internalDelete(VKey<T> key) {
-  public <T> int delete(VKey<T> key) {
     checkArgumentNotNull(key, "key must be specified");
     assertInTransaction();
     EntityType<?> entityType = getEntityType(key.getKind());
@@ -291,9 +290,14 @@ public class JpaTransactionManagerImpl implements JpaTransactionManager {
     return query.executeUpdate();
   }
 
+  @Override
+  public <T> void delete(VKey<T> key) {
+    internalDelete(key);
+  }
+
   @Override
   public <T> void assertDelete(VKey<T> key) {
-    if (delete(key) != 1) {
+    if (internalDelete(key) != 1) {
       throw new IllegalArgumentException(
           String.format("Error deleting the entity of the key: %s", key.getSqlKey()));
     }

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

@@ -123,9 +123,6 @@ public interface TransactionManager {
   /** Loads all entities of the given type, returns empty if there is no such entity. */
   <T> ImmutableList<T> loadAll(Class<T> clazz);
 
-  /** Deletes the entity by its id, returns the number of deleted entity. */
+  /** Deletes the entity by its id. */
-  <T> int delete(VKey<T> key);
+  <T> void delete(VKey<T> key);
-
-  /** Deletes the entity by its id, throws exception if the entity is not deleted. */
-  <T> void assertDelete(VKey<T> key);
 }

core/src/test/java/google/registry/model/ofy/DatastoreTransactionManagerTest.java

@@ -0,0 +1,268 @@
+// Copyright 2019 The Nomulus Authors. All Rights Reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package google.registry.model.ofy;
+
+import static com.google.common.truth.Truth.assertThat;
+import static google.registry.persistence.transaction.TransactionManagerFactory.tm;
+import static org.junit.Assert.assertThrows;
+
+import com.google.common.collect.ImmutableList;
+import com.googlecode.objectify.Key;
+import com.googlecode.objectify.annotation.Entity;
+import com.googlecode.objectify.annotation.Id;
+import google.registry.model.ImmutableObject;
+import google.registry.persistence.VKey;
+import google.registry.testing.AppEngineRule;
+import google.registry.testing.FakeClock;
+import google.registry.testing.InjectRule;
+import java.util.NoSuchElementException;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.extension.RegisterExtension;
+
+public class DatastoreTransactionManagerTest {
+
+  private final FakeClock fakeClock = new FakeClock();
+
+  private final TestEntity theEntity = new TestEntity("theEntity", "foo");
+  private final ImmutableList<TestEntity> moreEntities =
+      ImmutableList.of(
+          new TestEntity("entity1", "foo"),
+          new TestEntity("entity2", "bar"),
+          new TestEntity("entity3", "qux"));
+
+  @RegisterExtension public InjectRule inject = new InjectRule();
+
+  @RegisterExtension
+  public final AppEngineRule appEngine =
+      AppEngineRule.builder()
+          .withClock(fakeClock)
+          .withDatastoreAndCloudSql()
+          .withOfyTestEntities(TestEntity.class)
+          .build();
+
+  public DatastoreTransactionManagerTest() {}
+
+  @BeforeEach
+  public void setUp() {
+    inject.setStaticField(Ofy.class, "clock", fakeClock);
+  }
+
+  // TODO(mmuller): The tests below are just copy-pasted from JpaTransactionManagerImplTest
+  // (excluding the CompoundId tests and native query tests, which are not relevant to datastore,
+  // and the test methods using "count" which doesn't work for datastore, as well as tests for
+  // functionality that doesn't exist in datastore, like failures based on whether a newly saved or
+  // updated object exists or not). We need to merge these into a single test suite, but first we
+  // should move the JpaUnitTestRule functionality into AppEngineTest and migrate the whole thing
+  // to junit5.
+
+  @Test
+  public void inTransaction_returnsCorrespondingResult() {
+    assertThat(tm().inTransaction()).isFalse();
+    tm().transact(() -> assertThat(tm().inTransaction()).isTrue());
+    assertThat(tm().inTransaction()).isFalse();
+  }
+
+  @Test
+  public void assertInTransaction_throwsExceptionWhenNotInTransaction() {
+    assertThrows(IllegalStateException.class, () -> tm().assertInTransaction());
+    tm().transact(() -> tm().assertInTransaction());
+    assertThrows(IllegalStateException.class, () -> tm().assertInTransaction());
+  }
+
+  @Test
+  public void getTransactionTime_throwsExceptionWhenNotInTransaction() {
+    FakeClock txnClock = fakeClock;
+    txnClock.advanceOneMilli();
+    assertThrows(IllegalStateException.class, () -> tm().getTransactionTime());
+    tm().transact(() -> assertThat(tm().getTransactionTime()).isEqualTo(txnClock.nowUtc()));
+    assertThrows(IllegalStateException.class, () -> tm().getTransactionTime());
+  }
+
+  @Test
+  public void transact_hasNoEffectWithPartialSuccess() {
+    assertThat(tm().transact(() -> tm().checkExists(theEntity))).isFalse();
+    assertThrows(
+        RuntimeException.class,
+        () ->
+            tm()
+                .transact(
+                    () -> {
+                      tm().saveNew(theEntity);
+                      throw new RuntimeException();
+                    }));
+    assertThat(tm().transact(() -> tm().checkExists(theEntity))).isFalse();
+  }
+
+  @Test
+  public void transact_reusesExistingTransaction() {
+    assertThat(tm().transact(() -> tm().checkExists(theEntity))).isFalse();
+    fakeClock.advanceOneMilli();
+    tm().transact(() -> tm().transact(() -> tm().saveNew(theEntity)));
+    fakeClock.advanceOneMilli();
+    assertThat(tm().transact(() -> tm().checkExists(theEntity))).isTrue();
+  }
+
+  @Test
+  public void saveNew_succeeds() {
+    assertThat(tm().transact(() -> tm().checkExists(theEntity))).isFalse();
+    fakeClock.advanceOneMilli();
+    tm().transact(() -> tm().saveNew(theEntity));
+    fakeClock.advanceOneMilli();
+    assertThat(tm().transact(() -> tm().checkExists(theEntity))).isTrue();
+    fakeClock.advanceOneMilli();
+    assertThat(tm().transact(() -> tm().load(theEntity.key()))).isEqualTo(theEntity);
+  }
+
+  @Test
+  public void saveAllNew_succeeds() {
+    moreEntities.forEach(
+        entity -> assertThat(tm().transact(() -> tm().checkExists(entity))).isFalse());
+    fakeClock.advanceOneMilli();
+    tm().transact(() -> tm().saveAllNew(moreEntities));
+    fakeClock.advanceOneMilli();
+    moreEntities.forEach(
+        entity -> assertThat(tm().transact(() -> tm().checkExists(entity))).isTrue());
+  }
+
+  @Test
+  public void saveNewOrUpdate_persistsNewEntity() {
+    assertThat(tm().transact(() -> tm().checkExists(theEntity))).isFalse();
+    fakeClock.advanceOneMilli();
+    tm().transact(() -> tm().saveNewOrUpdate(theEntity));
+    fakeClock.advanceOneMilli();
+    assertThat(tm().transact(() -> tm().checkExists(theEntity))).isTrue();
+    fakeClock.advanceOneMilli();
+    assertThat(tm().transact(() -> tm().load(theEntity.key()))).isEqualTo(theEntity);
+  }
+
+  @Test
+  public 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";
+    tm().transact(() -> tm().saveNewOrUpdate(theEntity));
+    fakeClock.advanceOneMilli();
+    persisted = tm().transact(() -> tm().load(theEntity.key()));
+    fakeClock.advanceOneMilli();
+    assertThat(persisted.data).isEqualTo("bar");
+  }
+
+  @Test
+  public void saveNewOrUpdateAll_succeeds() {
+    moreEntities.forEach(
+        entity -> assertThat(tm().transact(() -> tm().checkExists(entity))).isFalse());
+    fakeClock.advanceOneMilli();
+    tm().transact(() -> tm().saveNewOrUpdateAll(moreEntities));
+    fakeClock.advanceOneMilli();
+    moreEntities.forEach(
+        entity -> assertThat(tm().transact(() -> tm().checkExists(entity))).isTrue());
+  }
+
+  @Test
+  public void update_succeeds() {
+    fakeClock.advanceOneMilli();
+    tm().transact(() -> tm().saveNew(theEntity));
+    fakeClock.advanceOneMilli();
+    TestEntity persisted =
+        tm().transact(() -> tm().load(VKey.createOfy(TestEntity.class, Key.create(theEntity))));
+    fakeClock.advanceOneMilli();
+    assertThat(persisted.data).isEqualTo("foo");
+    theEntity.data = "bar";
+    tm().transact(() -> tm().update(theEntity));
+    fakeClock.advanceOneMilli();
+    persisted = tm().transact(() -> tm().load(theEntity.key()));
+    assertThat(persisted.data).isEqualTo("bar");
+  }
+
+  @Test
+  public void load_succeeds() {
+    assertThat(tm().transact(() -> tm().checkExists(theEntity))).isFalse();
+    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");
+  }
+
+  @Test
+  public void load_throwsOnMissingElement() {
+    assertThat(tm().transact(() -> tm().checkExists(theEntity))).isFalse();
+    fakeClock.advanceOneMilli();
+    assertThrows(
+        NoSuchElementException.class, () -> tm().transact(() -> tm().load(theEntity.key())));
+  }
+
+  @Test
+  public void maybeLoad_succeeds() {
+    assertThat(tm().transact(() -> tm().checkExists(theEntity))).isFalse();
+    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");
+  }
+
+  @Test
+  public void maybeLoad_nonExistentObject() {
+    assertThat(tm().transact(() -> tm().checkExists(theEntity))).isFalse();
+    fakeClock.advanceOneMilli();
+    assertThat(tm().transact(() -> tm().maybeLoad(theEntity.key())).isPresent()).isFalse();
+  }
+
+  @Test
+  public void delete_succeeds() {
+    fakeClock.advanceOneMilli();
+    tm().transact(() -> tm().saveNew(theEntity));
+    fakeClock.advanceOneMilli();
+    assertThat(tm().transact(() -> tm().checkExists(theEntity))).isTrue();
+    fakeClock.advanceOneMilli();
+    tm().transact(() -> tm().delete(theEntity.key()));
+    fakeClock.advanceOneMilli();
+    assertThat(tm().transact(() -> tm().checkExists(theEntity))).isFalse();
+  }
+
+  @Test
+  public void delete_returnsZeroWhenNoEntity() {
+    assertThat(tm().transact(() -> tm().checkExists(theEntity))).isFalse();
+    fakeClock.advanceOneMilli();
+    tm().transact(() -> tm().delete(theEntity.key()));
+    fakeClock.advanceOneMilli();
+    assertThat(tm().transact(() -> tm().checkExists(theEntity))).isFalse();
+  }
+
+  @Entity(name = "TestEntity")
+  private static class TestEntity extends ImmutableObject {
+    @Id private String name;
+
+    private String data;
+
+    private TestEntity() {}
+
+    private TestEntity(String name, String data) {
+      this.name = name;
+      this.data = data;
+    }
+
+    public VKey<TestEntity> key() {
+      return VKey.createOfy(TestEntity.class, Key.create(this));
+    }
+  }
+}

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

@@ -31,7 +31,6 @@ import javax.persistence.Entity;
 import javax.persistence.EntityManager;
 import javax.persistence.Id;
 import javax.persistence.IdClass;
-import javax.persistence.PersistenceException;
 import javax.persistence.RollbackException;
 import org.junit.Rule;
 import org.junit.Test;
@@ -72,18 +71,18 @@ public class JpaTransactionManagerImplTest {
 
   @Test
   public void assertInTransaction_throwsExceptionWhenNotInTransaction() {
-    assertThrows(PersistenceException.class, () -> jpaTm().assertInTransaction());
+    assertThrows(IllegalStateException.class, () -> jpaTm().assertInTransaction());
     jpaTm().transact(() -> jpaTm().assertInTransaction());
-    assertThrows(PersistenceException.class, () -> jpaTm().assertInTransaction());
+    assertThrows(IllegalStateException.class, () -> jpaTm().assertInTransaction());
   }
 
   @Test
   public void getTransactionTime_throwsExceptionWhenNotInTransaction() {
     FakeClock txnClock = fakeClock;
     txnClock.advanceOneMilli();
-    assertThrows(PersistenceException.class, () -> jpaTm().getTransactionTime());
+    assertThrows(IllegalStateException.class, () -> jpaTm().getTransactionTime());
     jpaTm().transact(() -> assertThat(jpaTm().getTransactionTime()).isEqualTo(txnClock.nowUtc()));
-    assertThrows(PersistenceException.class, () -> jpaTm().getTransactionTime());
+    assertThrows(IllegalStateException.class, () -> jpaTm().getTransactionTime());
   }
 
   @Test
@@ -333,14 +332,14 @@ public class JpaTransactionManagerImplTest {
   public void delete_succeeds() {
     jpaTm().transact(() -> jpaTm().saveNew(theEntity));
     assertThat(jpaTm().transact(() -> jpaTm().checkExists(theEntity))).isTrue();
-    assertThat(jpaTm().transact(() -> jpaTm().delete(theEntityKey))).isEqualTo(1);
+    jpaTm().transact(() -> jpaTm().delete(theEntityKey));
     assertThat(jpaTm().transact(() -> jpaTm().checkExists(theEntity))).isFalse();
   }
 
   @Test
   public void delete_returnsZeroWhenNoEntity() {
     assertThat(jpaTm().transact(() -> jpaTm().checkExists(theEntity))).isFalse();
-    assertThat(jpaTm().transact(() -> jpaTm().delete(theEntityKey))).isEqualTo(0);
+    jpaTm().transact(() -> jpaTm().delete(theEntityKey));
     assertThat(jpaTm().transact(() -> jpaTm().checkExists(theEntity))).isFalse();
   }