From c2ed33ad3cfcfeea00ac0e3afa2bd8df387fb46a Mon Sep 17 00:00:00 2001 From: gbrodman Date: Mon, 26 Jul 2021 19:14:49 -0400 Subject: [PATCH] Use raw EntityManager to load during beforeSqlSave (#1253) If we use the transaction manager methods, JpaTransactionManagerImpl will attempt to detach the EppResource in question that we're loading -- this fails because that entity has been saved in the same transaction already. We don't need detaching during these methods (it's just for resource population) so we can use the raw loads to get around it. --- .../backup/ReplayCommitLogsToSqlAction.java | 2 +- .../model/contact/ContactHistory.java | 8 ++-- .../registry/model/domain/DomainHistory.java | 35 +++++++++------ .../registry/model/host/HostHistory.java | 8 ++-- .../schema/replay/ReplaySpecializer.java | 14 ++---- .../registry/schema/replay/SqlEntity.java | 3 ++ .../CreateSyntheticHistoryEntriesAction.java | 2 +- .../model/history/ContactHistoryTest.java | 45 +++++++++++++++---- .../model/history/DomainHistoryTest.java | 38 ++++++++++++++++ .../model/history/HostHistoryTest.java | 43 +++++++++++++++--- .../history/LegacyHistoryObjectTest.java | 16 ++----- .../google/registry/testing/TestObject.java | 3 +- 12 files changed, 158 insertions(+), 59 deletions(-) diff --git a/core/src/main/java/google/registry/backup/ReplayCommitLogsToSqlAction.java b/core/src/main/java/google/registry/backup/ReplayCommitLogsToSqlAction.java index 245eda28b..e98073d36 100644 --- a/core/src/main/java/google/registry/backup/ReplayCommitLogsToSqlAction.java +++ b/core/src/main/java/google/registry/backup/ReplayCommitLogsToSqlAction.java @@ -239,7 +239,7 @@ public class ReplayCommitLogsToSqlAction implements Runnable { .toSqlEntity() .ifPresent( sqlEntity -> { - ReplaySpecializer.beforeSqlSave(sqlEntity); + sqlEntity.beforeSqlSaveOnReplay(); jpaTm().put(sqlEntity); }); } else { diff --git a/core/src/main/java/google/registry/model/contact/ContactHistory.java b/core/src/main/java/google/registry/model/contact/ContactHistory.java index 57f5deadb..970eee673 100644 --- a/core/src/main/java/google/registry/model/contact/ContactHistory.java +++ b/core/src/main/java/google/registry/model/contact/ContactHistory.java @@ -136,9 +136,11 @@ public class ContactHistory extends HistoryEntry implements SqlEntity { } // Used to fill out the contactBase field during asynchronous replay - public static void beforeSqlSave(ContactHistory contactHistory) { - contactHistory.contactBase = - jpaTm().loadByKey(VKey.createSql(ContactResource.class, contactHistory.getContactRepoId())); + @Override + public void beforeSqlSaveOnReplay() { + if (contactBase == null) { + contactBase = jpaTm().getEntityManager().find(ContactResource.class, getContactRepoId()); + } } /** Class to represent the composite primary key of {@link ContactHistory} entity. */ diff --git a/core/src/main/java/google/registry/model/domain/DomainHistory.java b/core/src/main/java/google/registry/model/domain/DomainHistory.java index 68e841c89..ff6b24a21 100644 --- a/core/src/main/java/google/registry/model/domain/DomainHistory.java +++ b/core/src/main/java/google/registry/model/domain/DomainHistory.java @@ -294,9 +294,26 @@ public class DomainHistory extends HistoryEntry implements SqlEntity { } // Used to fill out the domainContent field during asynchronous replay - public static void beforeSqlSave(DomainHistory domainHistory) { - domainHistory.domainContent = - jpaTm().loadByKey(VKey.createSql(DomainBase.class, domainHistory.getDomainRepoId())); + @Override + public void beforeSqlSaveOnReplay() { + if (domainContent == null) { + domainContent = jpaTm().getEntityManager().find(DomainBase.class, getDomainRepoId()); + fillAuxiliaryFieldsFromDomain(this); + } + } + + private static void fillAuxiliaryFieldsFromDomain(DomainHistory domainHistory) { + if (domainHistory.domainContent != null) { + domainHistory.nsHosts = nullToEmptyImmutableCopy(domainHistory.domainContent.nsHosts); + domainHistory.dsDataHistories = + nullToEmptyImmutableCopy(domainHistory.domainContent.getDsData()).stream() + .map(dsData -> DomainDsDataHistory.createFrom(domainHistory.id, dsData)) + .collect(toImmutableSet()); + domainHistory.gracePeriodHistories = + nullToEmptyImmutableCopy(domainHistory.domainContent.getGracePeriods()).stream() + .map(gracePeriod -> GracePeriodHistory.createFrom(domainHistory.id, gracePeriod)) + .collect(toImmutableSet()); + } } /** Class to represent the composite primary key of {@link DomainHistory} entity. */ @@ -391,17 +408,7 @@ public class DomainHistory extends HistoryEntry implements SqlEntity { // Note that we cannot assert that instance.domainContent is not null here because this // builder is also used to convert legacy HistoryEntry objects to DomainHistory, when // domainContent is not available. - if (instance.domainContent != null) { - instance.nsHosts = nullToEmptyImmutableCopy(instance.domainContent.nsHosts); - instance.dsDataHistories = - nullToEmptyImmutableCopy(instance.domainContent.getDsData()).stream() - .map(dsData -> DomainDsDataHistory.createFrom(instance.id, dsData)) - .collect(toImmutableSet()); - instance.gracePeriodHistories = - nullToEmptyImmutableCopy(instance.domainContent.getGracePeriods()).stream() - .map(gracePeriod -> GracePeriodHistory.createFrom(instance.id, gracePeriod)) - .collect(toImmutableSet()); - } + fillAuxiliaryFieldsFromDomain(instance); return instance; } } diff --git a/core/src/main/java/google/registry/model/host/HostHistory.java b/core/src/main/java/google/registry/model/host/HostHistory.java index 02d0661c3..52592d69e 100644 --- a/core/src/main/java/google/registry/model/host/HostHistory.java +++ b/core/src/main/java/google/registry/model/host/HostHistory.java @@ -136,9 +136,11 @@ public class HostHistory extends HistoryEntry implements SqlEntity { } // Used to fill out the hostBase field during asynchronous replay - public static void beforeSqlSave(HostHistory hostHistory) { - hostHistory.hostBase = - jpaTm().loadByKey(VKey.createSql(HostResource.class, hostHistory.getHostRepoId())); + @Override + public void beforeSqlSaveOnReplay() { + if (hostBase == null) { + hostBase = jpaTm().getEntityManager().find(HostResource.class, getHostRepoId()); + } } /** Class to represent the composite primary key of {@link HostHistory} entity. */ diff --git a/core/src/main/java/google/registry/schema/replay/ReplaySpecializer.java b/core/src/main/java/google/registry/schema/replay/ReplaySpecializer.java index 2220376b4..d780b9404 100644 --- a/core/src/main/java/google/registry/schema/replay/ReplaySpecializer.java +++ b/core/src/main/java/google/registry/schema/replay/ReplaySpecializer.java @@ -28,17 +28,11 @@ import java.lang.reflect.Method; public class ReplaySpecializer { public static void beforeSqlDelete(VKey key) { - invokeMethod(key.getKind(), "beforeSqlDelete", key); - } - - public static void beforeSqlSave(SqlEntity sqlEntity) { - invokeMethod(sqlEntity.getClass(), "beforeSqlSave", sqlEntity); - } - - private static void invokeMethod(Class clazz, String methodName, Object argument) { + String methodName = "beforeSqlDelete"; + Class clazz = key.getKind(); try { - Method method = clazz.getMethod(methodName, argument.getClass()); - method.invoke(null, argument); + Method method = clazz.getMethod(methodName, VKey.class); + method.invoke(null, key); } catch (NoSuchMethodException e) { // Ignore, this just means that the class doesn't need this hook. } catch (IllegalAccessException e) { diff --git a/core/src/main/java/google/registry/schema/replay/SqlEntity.java b/core/src/main/java/google/registry/schema/replay/SqlEntity.java index 0fec88168..55e43b19d 100644 --- a/core/src/main/java/google/registry/schema/replay/SqlEntity.java +++ b/core/src/main/java/google/registry/schema/replay/SqlEntity.java @@ -26,4 +26,7 @@ import java.util.Optional; public interface SqlEntity { Optional toDatastoreEntity(); + + /** A method that will ber called before the object is saved to SQL in asynchronous replay. */ + default void beforeSqlSaveOnReplay() {} } diff --git a/core/src/main/java/google/registry/tools/javascrap/CreateSyntheticHistoryEntriesAction.java b/core/src/main/java/google/registry/tools/javascrap/CreateSyntheticHistoryEntriesAction.java index 1ba24ab33..89d704d9e 100644 --- a/core/src/main/java/google/registry/tools/javascrap/CreateSyntheticHistoryEntriesAction.java +++ b/core/src/main/java/google/registry/tools/javascrap/CreateSyntheticHistoryEntriesAction.java @@ -53,7 +53,7 @@ import javax.inject.Inject; * However, since this is meant to be run during the Datastore-primary, SQL-secondary stage of the * migration, we want to make sure that we are using the most up-to-date version of the data. The * resource field of the history objects will be populated during asynchronous migration, e.g. in - * {@link DomainHistory#beforeSqlSave(DomainHistory)}. + * {@link DomainHistory#beforeSqlSaveOnReplay}. */ @Action( service = Action.Service.BACKEND, diff --git a/core/src/test/java/google/registry/model/history/ContactHistoryTest.java b/core/src/test/java/google/registry/model/history/ContactHistoryTest.java index cbdd7153e..82a75b695 100644 --- a/core/src/test/java/google/registry/model/history/ContactHistoryTest.java +++ b/core/src/test/java/google/registry/model/history/ContactHistoryTest.java @@ -18,6 +18,7 @@ import static com.google.common.truth.Truth.assertThat; import static google.registry.model.ImmutableObjectSubject.assertAboutImmutableObjects; import static google.registry.persistence.transaction.TransactionManagerFactory.jpaTm; import static google.registry.persistence.transaction.TransactionManagerFactory.tm; +import static google.registry.testing.DatabaseHelper.newContactResource; import static google.registry.testing.DatabaseHelper.newContactResourceWithRoid; import static google.registry.testing.SqlHelper.saveRegistrar; import static java.nio.charset.StandardCharsets.UTF_8; @@ -47,7 +48,7 @@ public class ContactHistoryTest extends EntityTestCase { jpaTm().transact(() -> jpaTm().insert(contact)); VKey contactVKey = contact.createVKey(); ContactResource contactFromDb = jpaTm().transact(() -> jpaTm().loadByKey(contactVKey)); - ContactHistory contactHistory = createContactHistory(contactFromDb, contact.getRepoId()); + ContactHistory contactHistory = createContactHistory(contactFromDb); jpaTm().transact(() -> jpaTm().insert(contactHistory)); jpaTm() .transact( @@ -67,10 +68,7 @@ public class ContactHistoryTest extends EntityTestCase { VKey contactVKey = contact.createVKey(); ContactResource contactFromDb = jpaTm().transact(() -> jpaTm().loadByKey(contactVKey)); ContactHistory contactHistory = - createContactHistory(contactFromDb, contact.getRepoId()) - .asBuilder() - .setContact(null) - .build(); + createContactHistory(contactFromDb).asBuilder().setContact(null).build(); jpaTm().transact(() -> jpaTm().insert(contactHistory)); jpaTm() @@ -91,7 +89,7 @@ public class ContactHistoryTest extends EntityTestCase { VKey contactVKey = contact.createVKey(); ContactResource contactFromDb = tm().transact(() -> tm().loadByKey(contactVKey)); fakeClock.advanceOneMilli(); - ContactHistory contactHistory = createContactHistory(contactFromDb, contact.getRepoId()); + ContactHistory contactHistory = createContactHistory(contactFromDb); tm().transact(() -> tm().insert(contactHistory)); // retrieving a HistoryEntry or a ContactHistory with the same key should return the same object @@ -106,7 +104,38 @@ public class ContactHistoryTest extends EntityTestCase { assertThat(hostHistoryFromDb).isEqualTo(historyEntryFromDb); } - private ContactHistory createContactHistory(ContactBase contact, String contactRepoId) { + @Test + void testBeforeSqlSave_afterContactPersisted() { + saveRegistrar("TheRegistrar"); + ContactResource contactResource = newContactResource("contactId"); + ContactHistory contactHistory = + new ContactHistory.Builder() + .setType(HistoryEntry.Type.HOST_CREATE) + .setXmlBytes("".getBytes(UTF_8)) + .setModificationTime(fakeClock.nowUtc()) + .setClientId("TheRegistrar") + .setTrid(Trid.create("ABC-123", "server-trid")) + .setBySuperuser(false) + .setReason("reason") + .setRequestedByRegistrar(true) + .setContactRepoId(contactResource.getRepoId()) + .build(); + jpaTm() + .transact( + () -> { + jpaTm().put(contactResource); + contactHistory.beforeSqlSaveOnReplay(); + jpaTm().put(contactHistory); + }); + jpaTm() + .transact( + () -> + assertAboutImmutableObjects() + .that(jpaTm().loadByEntity(contactResource)) + .hasFieldsEqualTo(jpaTm().loadByEntity(contactHistory).getContactBase().get())); + } + + private ContactHistory createContactHistory(ContactBase contact) { return new ContactHistory.Builder() .setType(HistoryEntry.Type.HOST_CREATE) .setXmlBytes("".getBytes(UTF_8)) @@ -117,7 +146,7 @@ public class ContactHistoryTest extends EntityTestCase { .setReason("reason") .setRequestedByRegistrar(true) .setContact(contact) - .setContactRepoId(contactRepoId) + .setContactRepoId(contact.getRepoId()) .build(); } diff --git a/core/src/test/java/google/registry/model/history/DomainHistoryTest.java b/core/src/test/java/google/registry/model/history/DomainHistoryTest.java index e8b149e33..198b3c772 100644 --- a/core/src/test/java/google/registry/model/history/DomainHistoryTest.java +++ b/core/src/test/java/google/registry/model/history/DomainHistoryTest.java @@ -190,6 +190,44 @@ public class DomainHistoryTest extends EntityTestCase { jpaTm().transact(() -> jpaTm().put(domainHistoryFromDb2)); } + @TestSqlOnly + void testBeforeSqlSave_afterDomainPersisted() { + DomainBase domain = createDomainWithContactsAndHosts(); + DomainHistory historyWithoutResource = + new DomainHistory.Builder() + .setType(HistoryEntry.Type.DOMAIN_CREATE) + .setXmlBytes("".getBytes(UTF_8)) + .setModificationTime(fakeClock.nowUtc()) + .setClientId("TheRegistrar") + .setTrid(Trid.create("ABC-123", "server-trid")) + .setBySuperuser(false) + .setReason("reason") + .setRequestedByRegistrar(true) + .setDomainRepoId(domain.getRepoId()) + .setOtherClientId("otherClient") + .setPeriod(Period.create(1, Period.Unit.YEARS)) + .build(); + jpaTm() + .transact( + () -> { + jpaTm() + .put( + domain + .asBuilder() + .setPersistedCurrentSponsorClientId("NewRegistrar") + .build()); + historyWithoutResource.beforeSqlSaveOnReplay(); + jpaTm().put(historyWithoutResource); + }); + jpaTm() + .transact( + () -> + assertAboutImmutableObjects() + .that(jpaTm().loadByEntity(domain)) + .hasFieldsEqualTo( + jpaTm().loadByEntity(historyWithoutResource).getDomainContent().get())); + } + static DomainBase createDomainWithContactsAndHosts() { createTld("tld"); HostResource host = newHostResourceWithRoid("ns1.example.com", "host1"); diff --git a/core/src/test/java/google/registry/model/history/HostHistoryTest.java b/core/src/test/java/google/registry/model/history/HostHistoryTest.java index 6cd296e10..70cc8cd4c 100644 --- a/core/src/test/java/google/registry/model/history/HostHistoryTest.java +++ b/core/src/test/java/google/registry/model/history/HostHistoryTest.java @@ -18,6 +18,7 @@ import static com.google.common.truth.Truth.assertThat; import static google.registry.model.ImmutableObjectSubject.assertAboutImmutableObjects; import static google.registry.persistence.transaction.TransactionManagerFactory.jpaTm; import static google.registry.persistence.transaction.TransactionManagerFactory.tm; +import static google.registry.testing.DatabaseHelper.newHostResource; import static google.registry.testing.DatabaseHelper.newHostResourceWithRoid; import static google.registry.testing.SqlHelper.saveRegistrar; import static java.nio.charset.StandardCharsets.UTF_8; @@ -48,7 +49,7 @@ public class HostHistoryTest extends EntityTestCase { VKey hostVKey = VKey.create(HostResource.class, "host1", Key.create(HostResource.class, "host1")); HostResource hostFromDb = jpaTm().transact(() -> jpaTm().loadByKey(hostVKey)); - HostHistory hostHistory = createHostHistory(hostFromDb, host.getRepoId()); + HostHistory hostHistory = createHostHistory(hostFromDb); jpaTm().transact(() -> jpaTm().insert(hostHistory)); jpaTm() .transact( @@ -66,8 +67,7 @@ public class HostHistoryTest extends EntityTestCase { jpaTm().transact(() -> jpaTm().insert(host)); HostResource hostFromDb = jpaTm().transact(() -> jpaTm().loadByKey(host.createVKey())); - HostHistory hostHistory = - createHostHistory(hostFromDb, host.getRepoId()).asBuilder().setHost(null).build(); + HostHistory hostHistory = createHostHistory(hostFromDb).asBuilder().setHost(null).build(); jpaTm().transact(() -> jpaTm().insert(hostHistory)); jpaTm() @@ -88,7 +88,7 @@ public class HostHistoryTest extends EntityTestCase { VKey hostVKey = VKey.create(HostResource.class, "host1", Key.create(HostResource.class, "host1")); HostResource hostFromDb = tm().transact(() -> tm().loadByKey(hostVKey)); - HostHistory hostHistory = createHostHistory(hostFromDb, host.getRepoId()); + HostHistory hostHistory = createHostHistory(hostFromDb); fakeClock.advanceOneMilli(); tm().transact(() -> tm().insert(hostHistory)); @@ -104,6 +104,37 @@ public class HostHistoryTest extends EntityTestCase { assertThat(hostHistoryFromDb).isEqualTo(historyEntryFromDb); } + @Test + void testBeforeSqlSave_afterHostPersisted() { + saveRegistrar("TheRegistrar"); + HostResource hostResource = newHostResource("ns1.example.tld"); + HostHistory hostHistory = + new HostHistory.Builder() + .setType(HistoryEntry.Type.HOST_CREATE) + .setXmlBytes("".getBytes(UTF_8)) + .setModificationTime(fakeClock.nowUtc()) + .setClientId("TheRegistrar") + .setTrid(Trid.create("ABC-123", "server-trid")) + .setBySuperuser(false) + .setReason("reason") + .setRequestedByRegistrar(true) + .setHostRepoId(hostResource.getRepoId()) + .build(); + jpaTm() + .transact( + () -> { + jpaTm().put(hostResource); + hostHistory.beforeSqlSaveOnReplay(); + jpaTm().put(hostHistory); + }); + jpaTm() + .transact( + () -> + assertAboutImmutableObjects() + .that(jpaTm().loadByEntity(hostResource)) + .hasFieldsEqualTo(jpaTm().loadByEntity(hostHistory).getHostBase().get())); + } + private void assertHostHistoriesEqual(HostHistory one, HostHistory two) { assertAboutImmutableObjects().that(one).isEqualExceptFields(two, "hostBase"); assertAboutImmutableObjects() @@ -111,7 +142,7 @@ public class HostHistoryTest extends EntityTestCase { .isEqualExceptFields(two.getHostBase().orElse(null), "repoId"); } - private HostHistory createHostHistory(HostBase hostBase, String hostRepoId) { + private HostHistory createHostHistory(HostBase hostBase) { return new HostHistory.Builder() .setType(HistoryEntry.Type.HOST_CREATE) .setXmlBytes("".getBytes(UTF_8)) @@ -122,7 +153,7 @@ public class HostHistoryTest extends EntityTestCase { .setReason("reason") .setRequestedByRegistrar(true) .setHost(hostBase) - .setHostRepoId(hostRepoId) + .setHostRepoId(hostBase.getRepoId()) .build(); } } diff --git a/core/src/test/java/google/registry/model/history/LegacyHistoryObjectTest.java b/core/src/test/java/google/registry/model/history/LegacyHistoryObjectTest.java index 3bc35b5d9..4f5254fec 100644 --- a/core/src/test/java/google/registry/model/history/LegacyHistoryObjectTest.java +++ b/core/src/test/java/google/registry/model/history/LegacyHistoryObjectTest.java @@ -74,7 +74,7 @@ public class LegacyHistoryObjectTest extends EntityTestCase { assertAboutImmutableObjects() .that(legacyHistoryEntry) .isEqualExceptFields(fromObjectify, "contactBase", "contactRepoId"); - assertThat(fromObjectify instanceof ContactHistory).isTrue(); + assertThat(fromObjectify).isInstanceOf(ContactHistory.class); ContactHistory legacyContactHistory = (ContactHistory) fromObjectify; // Next, save that from-Datastore object in SQL and verify we can load it back in @@ -124,7 +124,7 @@ public class LegacyHistoryObjectTest extends EntityTestCase { "nsHosts", "dsDataHistories", "gracePeriodHistories"); - assertThat(fromObjectify instanceof DomainHistory).isTrue(); + assertThat(fromObjectify).isInstanceOf(DomainHistory.class); DomainHistory legacyDomainHistory = (DomainHistory) fromObjectify; // Next, save that from-Datastore object in SQL and verify we can load it back in @@ -137,15 +137,7 @@ public class LegacyHistoryObjectTest extends EntityTestCase { // Don't compare nsHosts directly because one is null and the other is empty assertAboutImmutableObjects() .that(legacyDomainHistory) - .isEqualExceptFields( - // NB: period, transaction records, and other client ID are added in #794 - legacyHistoryFromSql, - "period", - "domainTransactionRecords", - "otherClientId", - "nsHosts", - "dsDataHistories", - "gracePeriodHistories"); + .isEqualExceptFields(legacyHistoryFromSql, "nsHosts"); assertThat(nullToEmpty(legacyDomainHistory.getNsHosts())) .isEqualTo(nullToEmpty(legacyHistoryFromSql.getNsHosts())); }); @@ -174,7 +166,7 @@ public class LegacyHistoryObjectTest extends EntityTestCase { assertAboutImmutableObjects() .that(legacyHistoryEntry) .isEqualExceptFields(fromObjectify, "hostBase", "hostRepoId"); - assertThat(fromObjectify instanceof HostHistory).isTrue(); + assertThat(fromObjectify).isInstanceOf(HostHistory.class); HostHistory legacyHostHistory = (HostHistory) fromObjectify; // Next, save that from-Datastore object in SQL and verify we can load it back in diff --git a/core/src/test/java/google/registry/testing/TestObject.java b/core/src/test/java/google/registry/testing/TestObject.java index f6695b7cd..88198b1c6 100644 --- a/core/src/test/java/google/registry/testing/TestObject.java +++ b/core/src/test/java/google/registry/testing/TestObject.java @@ -75,7 +75,8 @@ public class TestObject extends ImmutableObject implements DatastoreAndSqlEntity beforeSqlDeleteCallCount++; } - public static void beforeSqlSave(TestObject testObject) { + @Override + public void beforeSqlSaveOnReplay() { beforeSqlSaveCallCount++; }