From e2dfb6488dc2e41e5442739aadc974f34b73ce6a Mon Sep 17 00:00:00 2001 From: Michael Muller Date: Mon, 4 May 2020 10:49:36 -0400 Subject: [PATCH] Improve return value semantices for tm().load() (#576) Since we rarely (if ever) want to check the result of a single element load, make TransactionManager.load(VKey) return non-optional, non-nullable and just throw a NoSuchElementException if the entity is not found. Also add a maybeLoad() that does return an optional in case we ever want to do this (exists() should suffice for an existence check). --- .../ofy/DatastoreTransactionManager.java | 12 ++++- .../JpaTransactionManagerImpl.java | 13 ++++- .../transaction/TransactionManager.java | 5 +- .../model/contact/ContactResourceTest.java | 3 +- .../EntityCallbacksListenerTest.java | 4 +- .../JpaTransactionManagerImplTest.java | 49 +++++++++++++------ .../schema/registrar/RegistrarDaoTest.java | 6 +-- .../tools/UpdateRegistrarCommandTest.java | 1 - 8 files changed, 68 insertions(+), 25 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 a50f1714b..594590bf3 100644 --- a/core/src/main/java/google/registry/model/ofy/DatastoreTransactionManager.java +++ b/core/src/main/java/google/registry/model/ofy/DatastoreTransactionManager.java @@ -22,6 +22,7 @@ import com.googlecode.objectify.Key; import google.registry.persistence.VKey; import google.registry.persistence.transaction.TransactionManager; import java.util.Iterator; +import java.util.NoSuchElementException; import java.util.Optional; import java.util.function.Supplier; import java.util.stream.StreamSupport; @@ -136,10 +137,19 @@ public class DatastoreTransactionManager implements TransactionManager { // VKey instead of by ofy Key. But ideally, there should be one set of TransactionManager // interface tests that are applied to both the datastore and SQL implementations. @Override - public Optional load(VKey key) { + public Optional maybeLoad(VKey key) { return Optional.of(getOfy().load().key(key.getOfyKey()).now()); } + @Override + public T load(VKey key) { + T result = getOfy().load().key(key.getOfyKey()).now(); + if (result == null) { + throw new NoSuchElementException(key.toString()); + } + return result; + } + @Override public ImmutableList load(Iterable> keys) { Iterator> iter = 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 f51dce6aa..d57210d97 100644 --- a/core/src/main/java/google/registry/persistence/transaction/JpaTransactionManagerImpl.java +++ b/core/src/main/java/google/registry/persistence/transaction/JpaTransactionManagerImpl.java @@ -232,12 +232,23 @@ public class JpaTransactionManagerImpl implements JpaTransactionManager { } @Override - public Optional load(VKey key) { + public Optional maybeLoad(VKey key) { checkArgumentNotNull(key, "key must be specified"); assertInTransaction(); return Optional.ofNullable(getEntityManager().find(key.getKind(), key.getSqlKey())); } + @Override + public T load(VKey key) { + checkArgumentNotNull(key, "key must be specified"); + assertInTransaction(); + T result = getEntityManager().find(key.getKind(), key.getSqlKey()); + if (result == null) { + throw new NoSuchElementException(key.toString()); + } + return result; + } + @Override public ImmutableList load(Iterable> keys) { checkArgumentNotNull(keys, "keys must be specified"); 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 fc28ec11f..3a10e40e3 100644 --- a/core/src/main/java/google/registry/persistence/transaction/TransactionManager.java +++ b/core/src/main/java/google/registry/persistence/transaction/TransactionManager.java @@ -108,7 +108,10 @@ public interface TransactionManager { boolean checkExists(VKey key); /** Loads the entity by its id, returns empty if the entity doesn't exist. */ - Optional load(VKey key); + Optional maybeLoad(VKey key); + + /** Loads the entity by its id, throws NoSuchElementException if it doesn't exist. */ + T load(VKey key); /** * Leads the set of entities by their key id. diff --git a/core/src/test/java/google/registry/model/contact/ContactResourceTest.java b/core/src/test/java/google/registry/model/contact/ContactResourceTest.java index 897bc2cc1..799057faf 100644 --- a/core/src/test/java/google/registry/model/contact/ContactResourceTest.java +++ b/core/src/test/java/google/registry/model/contact/ContactResourceTest.java @@ -140,8 +140,7 @@ public class ContactResourceTest extends EntityTestCase { .transact( () -> jpaTm() - .load(VKey.createSql(ContactResource.class, originalContact.getRepoId()))) - .get(); + .load(VKey.createSql(ContactResource.class, originalContact.getRepoId()))); // TODO(b/153378849): Remove the hard code for postal info after resolving the issue that // @PostLoad doesn't work in Address ContactResource fixed = diff --git a/core/src/test/java/google/registry/persistence/EntityCallbacksListenerTest.java b/core/src/test/java/google/registry/persistence/EntityCallbacksListenerTest.java index 6f6989dea..c432643eb 100644 --- a/core/src/test/java/google/registry/persistence/EntityCallbacksListenerTest.java +++ b/core/src/test/java/google/registry/persistence/EntityCallbacksListenerTest.java @@ -69,14 +69,14 @@ public class EntityCallbacksListenerTest { checkAll(updated, 0, 1, 0, 1); TestEntity testLoad = - jpaTm().transact(() -> jpaTm().load(VKey.createSql(TestEntity.class, "id"))).get(); + jpaTm().transact(() -> jpaTm().load(VKey.createSql(TestEntity.class, "id"))); checkAll(testLoad, 0, 0, 0, 1); TestEntity testRemove = jpaTm() .transact( () -> { - TestEntity removed = jpaTm().load(VKey.createSql(TestEntity.class, "id")).get(); + TestEntity removed = jpaTm().load(VKey.createSql(TestEntity.class, "id")); jpaTm().getEntityManager().remove(removed); return removed; }); 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 e3366cfd4..43a233c4d 100644 --- a/core/src/test/java/google/registry/persistence/transaction/JpaTransactionManagerImplTest.java +++ b/core/src/test/java/google/registry/persistence/transaction/JpaTransactionManagerImplTest.java @@ -26,6 +26,7 @@ import google.registry.persistence.transaction.JpaTestRules.JpaUnitTestRule; import google.registry.testing.FakeClock; import java.io.Serializable; import java.math.BigInteger; +import java.util.NoSuchElementException; import javax.persistence.Entity; import javax.persistence.EntityManager; import javax.persistence.Id; @@ -147,7 +148,7 @@ public class JpaTransactionManagerImplTest { assertThat(jpaTm().transact(() -> jpaTm().checkExists(theEntity))).isFalse(); jpaTm().transact(() -> jpaTm().saveNew(theEntity)); assertThat(jpaTm().transact(() -> jpaTm().checkExists(theEntity))).isTrue(); - assertThat(jpaTm().transact(() -> jpaTm().load(theEntityKey).get())).isEqualTo(theEntity); + assertThat(jpaTm().transact(() -> jpaTm().load(theEntityKey))).isEqualTo(theEntity); } @Test @@ -155,7 +156,7 @@ public class JpaTransactionManagerImplTest { assertThat(jpaTm().transact(() -> jpaTm().checkExists(theEntity))).isFalse(); jpaTm().transact(() -> jpaTm().saveNew(theEntity)); assertThat(jpaTm().transact(() -> jpaTm().checkExists(theEntity))).isTrue(); - assertThat(jpaTm().transact(() -> jpaTm().load(theEntityKey).get())).isEqualTo(theEntity); + assertThat(jpaTm().transact(() -> jpaTm().load(theEntityKey))).isEqualTo(theEntity); assertThrows(RollbackException.class, () -> jpaTm().transact(() -> jpaTm().saveNew(theEntity))); } @@ -164,7 +165,7 @@ public class JpaTransactionManagerImplTest { assertThat(jpaTm().transact(() -> jpaTm().checkExists(compoundIdEntity))).isFalse(); jpaTm().transact(() -> jpaTm().saveNew(compoundIdEntity)); assertThat(jpaTm().transact(() -> jpaTm().checkExists(compoundIdEntity))).isTrue(); - assertThat(jpaTm().transact(() -> jpaTm().load(compoundIdEntityKey).get())) + assertThat(jpaTm().transact(() -> jpaTm().load(compoundIdEntityKey))) .isEqualTo(compoundIdEntity); } @@ -196,17 +197,17 @@ public class JpaTransactionManagerImplTest { assertThat(jpaTm().transact(() -> jpaTm().checkExists(theEntity))).isFalse(); jpaTm().transact(() -> jpaTm().saveNewOrUpdate(theEntity)); assertThat(jpaTm().transact(() -> jpaTm().checkExists(theEntity))).isTrue(); - assertThat(jpaTm().transact(() -> jpaTm().load(theEntityKey).get())).isEqualTo(theEntity); + assertThat(jpaTm().transact(() -> jpaTm().load(theEntityKey))).isEqualTo(theEntity); } @Test public void saveNewOrUpdate_updatesExistingEntity() { jpaTm().transact(() -> jpaTm().saveNew(theEntity)); - TestEntity persisted = jpaTm().transact(() -> jpaTm().load(theEntityKey)).get(); + TestEntity persisted = jpaTm().transact(() -> jpaTm().load(theEntityKey)); assertThat(persisted.data).isEqualTo("foo"); theEntity.data = "bar"; jpaTm().transact(() -> jpaTm().saveNewOrUpdate(theEntity)); - persisted = jpaTm().transact(() -> jpaTm().load(theEntityKey)).get(); + persisted = jpaTm().transact(() -> jpaTm().load(theEntityKey)); assertThat(persisted.data).isEqualTo("bar"); } @@ -225,23 +226,22 @@ public class JpaTransactionManagerImplTest { public void update_succeeds() { jpaTm().transact(() -> jpaTm().saveNew(theEntity)); TestEntity persisted = - jpaTm().transact(() -> jpaTm().load(VKey.createSql(TestEntity.class, "theEntity"))).get(); + jpaTm().transact(() -> jpaTm().load(VKey.createSql(TestEntity.class, "theEntity"))); assertThat(persisted.data).isEqualTo("foo"); theEntity.data = "bar"; jpaTm().transact(() -> jpaTm().update(theEntity)); - persisted = jpaTm().transact(() -> jpaTm().load(theEntityKey)).get(); + persisted = jpaTm().transact(() -> jpaTm().load(theEntityKey)); assertThat(persisted.data).isEqualTo("bar"); } @Test public void updateCompoundIdEntity_succeeds() { jpaTm().transact(() -> jpaTm().saveNew(compoundIdEntity)); - TestCompoundIdEntity persisted = - jpaTm().transact(() -> jpaTm().load(compoundIdEntityKey)).get(); + TestCompoundIdEntity persisted = jpaTm().transact(() -> jpaTm().load(compoundIdEntityKey)); assertThat(persisted.data).isEqualTo("foo"); compoundIdEntity.data = "bar"; jpaTm().transact(() -> jpaTm().update(compoundIdEntity)); - persisted = jpaTm().transact(() -> jpaTm().load(compoundIdEntityKey).get()); + persisted = jpaTm().transact(() -> jpaTm().load(compoundIdEntityKey)); assertThat(persisted.data).isEqualTo("bar"); } @@ -285,17 +285,38 @@ public class JpaTransactionManagerImplTest { public void load_succeeds() { assertThat(jpaTm().transact(() -> jpaTm().checkExists(theEntity))).isFalse(); jpaTm().transact(() -> jpaTm().saveNew(theEntity)); - TestEntity persisted = jpaTm().transact(() -> jpaTm().load(theEntityKey)).get(); + TestEntity persisted = jpaTm().transact(() -> jpaTm().load(theEntityKey)); assertThat(persisted.name).isEqualTo("theEntity"); assertThat(persisted.data).isEqualTo("foo"); } + @Test + public void load_throwsOnMissingElement() { + assertThat(jpaTm().transact(() -> jpaTm().checkExists(theEntity))).isFalse(); + assertThrows( + NoSuchElementException.class, () -> jpaTm().transact(() -> jpaTm().load(theEntityKey))); + } + + @Test + public void maybeLoad_succeeds() { + assertThat(jpaTm().transact(() -> jpaTm().checkExists(theEntity))).isFalse(); + jpaTm().transact(() -> jpaTm().saveNew(theEntity)); + TestEntity persisted = jpaTm().transact(() -> jpaTm().maybeLoad(theEntityKey).get()); + assertThat(persisted.name).isEqualTo("theEntity"); + assertThat(persisted.data).isEqualTo("foo"); + } + + @Test + public void maybeLoad_nonExistentObject() { + assertThat(jpaTm().transact(() -> jpaTm().checkExists(theEntity))).isFalse(); + assertThat(jpaTm().transact(() -> jpaTm().maybeLoad(theEntityKey)).isPresent()).isFalse(); + } + @Test public void loadCompoundIdEntity_succeeds() { assertThat(jpaTm().transact(() -> jpaTm().checkExists(compoundIdEntity))).isFalse(); jpaTm().transact(() -> jpaTm().saveNew(compoundIdEntity)); - TestCompoundIdEntity persisted = - jpaTm().transact(() -> jpaTm().load(compoundIdEntityKey)).get(); + TestCompoundIdEntity persisted = jpaTm().transact(() -> jpaTm().load(compoundIdEntityKey)); assertThat(persisted.name).isEqualTo("compoundIdEntity"); assertThat(persisted.age).isEqualTo(10); assertThat(persisted.data).isEqualTo("foo"); diff --git a/core/src/test/java/google/registry/schema/registrar/RegistrarDaoTest.java b/core/src/test/java/google/registry/schema/registrar/RegistrarDaoTest.java index bc0826aa5..0c7353663 100644 --- a/core/src/test/java/google/registry/schema/registrar/RegistrarDaoTest.java +++ b/core/src/test/java/google/registry/schema/registrar/RegistrarDaoTest.java @@ -78,7 +78,7 @@ public class RegistrarDaoTest { @Test public void update_worksSuccessfully() { jpaTm().transact(() -> jpaTm().saveNew(testRegistrar)); - Registrar persisted = jpaTm().transact(() -> jpaTm().load(registrarKey)).get(); + Registrar persisted = jpaTm().transact(() -> jpaTm().load(registrarKey)); assertThat(persisted.getRegistrarName()).isEqualTo("registrarName"); jpaTm() .transact( @@ -86,7 +86,7 @@ public class RegistrarDaoTest { jpaTm() .update( persisted.asBuilder().setRegistrarName("changedRegistrarName").build())); - Registrar updated = jpaTm().transact(() -> jpaTm().load(registrarKey)).get(); + Registrar updated = jpaTm().transact(() -> jpaTm().load(registrarKey)); assertThat(updated.getRegistrarName()).isEqualTo("changedRegistrarName"); } @@ -102,7 +102,7 @@ public class RegistrarDaoTest { public void load_worksSuccessfully() { assertThat(jpaTm().transact(() -> jpaTm().checkExists(testRegistrar))).isFalse(); jpaTm().transact(() -> jpaTm().saveNew(testRegistrar)); - Registrar persisted = jpaTm().transact(() -> jpaTm().load(registrarKey)).get(); + Registrar persisted = jpaTm().transact(() -> jpaTm().load(registrarKey)); assertThat(persisted.getClientId()).isEqualTo("registrarId"); assertThat(persisted.getRegistrarName()).isEqualTo("registrarName"); diff --git a/core/src/test/java/google/registry/tools/UpdateRegistrarCommandTest.java b/core/src/test/java/google/registry/tools/UpdateRegistrarCommandTest.java index ab101ce87..6f9aa4fe6 100644 --- a/core/src/test/java/google/registry/tools/UpdateRegistrarCommandTest.java +++ b/core/src/test/java/google/registry/tools/UpdateRegistrarCommandTest.java @@ -53,7 +53,6 @@ public class UpdateRegistrarCommandTest extends CommandTestCase jpaTm().load(VKey.createSql(Registrar.class, "NewRegistrar"))) - .get() .verifyPassword("some_password")) .isTrue(); }