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 594590bf3..011fe2e83 100644 --- a/core/src/main/java/google/registry/model/ofy/DatastoreTransactionManager.java +++ b/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 boolean checkExists(VKey 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 Optional maybeLoad(VKey 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 ImmutableList loadAll(Class 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 int delete(VKey key) { - throw new UnsupportedOperationException("Not available in the Datastore transaction manager"); - } - - @Override - public void assertDelete(VKey key) { - throw new UnsupportedOperationException("Not available in the Datastore transaction manager"); + public void delete(VKey key) { + getOfy().delete().key(key.getOfyKey()).now(); } } diff --git a/core/src/main/java/google/registry/persistence/transaction/JpaTransactionManager.java b/core/src/main/java/google/registry/persistence/transaction/JpaTransactionManager.java index 4b4924edf..0cf189ab0 100644 --- a/core/src/main/java/google/registry/persistence/transaction/JpaTransactionManager.java +++ b/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 void assertDelete(VKey key); } 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 d57210d97..09c3af271 100644 --- a/core/src/main/java/google/registry/persistence/transaction/JpaTransactionManagerImpl.java +++ b/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 - public int delete(VKey key) { + private int internalDelete(VKey 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 void delete(VKey key) { + internalDelete(key); + } + @Override public void assertDelete(VKey key) { - if (delete(key) != 1) { + if (internalDelete(key) != 1) { throw new IllegalArgumentException( String.format("Error deleting the entity of the key: %s", key.getSqlKey())); } 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 3a10e40e3..e8081eb08 100644 --- a/core/src/main/java/google/registry/persistence/transaction/TransactionManager.java +++ b/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. */ ImmutableList loadAll(Class clazz); - /** Deletes the entity by its id, returns the number of deleted entity. */ - int delete(VKey key); - - /** Deletes the entity by its id, throws exception if the entity is not deleted. */ - void assertDelete(VKey key); + /** Deletes the entity by its id. */ + void delete(VKey key); } diff --git a/core/src/test/java/google/registry/model/ofy/DatastoreTransactionManagerTest.java b/core/src/test/java/google/registry/model/ofy/DatastoreTransactionManagerTest.java new file mode 100644 index 000000000..62755b35e --- /dev/null +++ b/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 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 key() { + return VKey.createOfy(TestEntity.class, Key.create(this)); + } + } +} diff --git a/core/src/test/java/google/registry/persistence/transaction/JpaTransactionManagerImplTest.java b/core/src/test/java/google/registry/persistence/transaction/JpaTransactionManagerImplTest.java index 43a233c4d..fef501ed1 100644 --- a/core/src/test/java/google/registry/persistence/transaction/JpaTransactionManagerImplTest.java +++ b/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(); }