From 0109d5e4739c786a74877d6fabc5ce4cf07d065f Mon Sep 17 00:00:00 2001 From: Shicong Huang Date: Fri, 8 Jan 2021 10:28:22 -0500 Subject: [PATCH] Convert HostUpdateFlow to tm() (#923) --- .../registry/flows/host/HostUpdateFlow.java | 26 +- .../flows/host/HostUpdateFlowTest.java | 224 ++++++++++-------- 2 files changed, 139 insertions(+), 111 deletions(-) diff --git a/core/src/main/java/google/registry/flows/host/HostUpdateFlow.java b/core/src/main/java/google/registry/flows/host/HostUpdateFlow.java index 99ad5743e..9776dbb24 100644 --- a/core/src/main/java/google/registry/flows/host/HostUpdateFlow.java +++ b/core/src/main/java/google/registry/flows/host/HostUpdateFlow.java @@ -27,7 +27,6 @@ import static google.registry.flows.host.HostFlowUtils.validateHostName; import static google.registry.flows.host.HostFlowUtils.verifySuperordinateDomainNotInPendingDelete; import static google.registry.flows.host.HostFlowUtils.verifySuperordinateDomainOwnership; import static google.registry.model.index.ForeignKeyIndex.loadAndGetKey; -import static google.registry.model.ofy.ObjectifyService.ofy; import static google.registry.persistence.transaction.TransactionManagerFactory.tm; import static google.registry.util.CollectionUtils.isNullOrEmpty; @@ -191,23 +190,26 @@ public final class HostUpdateFlow implements TransactionalFlow { .setPersistedCurrentSponsorClientId(newPersistedClientId) .build(); verifyHasIpsIffIsExternal(command, existingHost, newHost); - ImmutableSet.Builder entitiesToSave = new ImmutableSet.Builder<>(); - entitiesToSave.add(newHost); + ImmutableSet.Builder entitiesToInsert = new ImmutableSet.Builder<>(); + ImmutableSet.Builder entitiesToUpdate = new ImmutableSet.Builder<>(); + entitiesToUpdate.add(newHost); // Keep the {@link ForeignKeyIndex} for this host up to date. if (isHostRename) { // Update the foreign key for the old host name and save one for the new host name. - entitiesToSave.add( - ForeignKeyIndex.create(existingHost, now), - ForeignKeyIndex.create(newHost, newHost.getDeletionTime())); + entitiesToUpdate.add(ForeignKeyIndex.create(existingHost, now)); + entitiesToUpdate.add(ForeignKeyIndex.create(newHost, newHost.getDeletionTime())); updateSuperordinateDomains(existingHost, newHost); } enqueueTasks(existingHost, newHost); - entitiesToSave.add(historyBuilder - .setType(HistoryEntry.Type.HOST_UPDATE) - .setModificationTime(now) - .setParent(Key.create(existingHost)) - .build()); - ofy().save().entities(entitiesToSave.build()); + entitiesToInsert.add( + historyBuilder + .setType(HistoryEntry.Type.HOST_UPDATE) + .setModificationTime(now) + .setParent(Key.create(existingHost)) + .build() + .toChildHistoryEntity()); + tm().updateAll(entitiesToUpdate.build()); + tm().insertAll(entitiesToInsert.build()); return responseBuilder.build(); } diff --git a/core/src/test/java/google/registry/flows/host/HostUpdateFlowTest.java b/core/src/test/java/google/registry/flows/host/HostUpdateFlowTest.java index 813efb4ca..8a81a3e04 100644 --- a/core/src/test/java/google/registry/flows/host/HostUpdateFlowTest.java +++ b/core/src/test/java/google/registry/flows/host/HostUpdateFlowTest.java @@ -18,7 +18,8 @@ import static com.google.common.base.Strings.nullToEmpty; import static com.google.common.truth.Truth.assertThat; import static google.registry.batch.AsyncTaskEnqueuer.QUEUE_ASYNC_HOST_RENAME; import static google.registry.model.EppResourceUtils.loadByForeignKey; -import static google.registry.model.ofy.ObjectifyService.ofy; +import static google.registry.persistence.transaction.TransactionManagerFactory.tm; +import static google.registry.persistence.transaction.TransactionManagerUtil.transactIfJpaTm; import static google.registry.testing.DatabaseHelper.assertNoBillingEvents; import static google.registry.testing.DatabaseHelper.createTld; import static google.registry.testing.DatabaseHelper.getOnlyHistoryEntryOfType; @@ -28,6 +29,7 @@ import static google.registry.testing.DatabaseHelper.persistActiveDomain; import static google.registry.testing.DatabaseHelper.persistActiveHost; import static google.registry.testing.DatabaseHelper.persistActiveSubordinateHost; import static google.registry.testing.DatabaseHelper.persistDeletedHost; +import static google.registry.testing.DatabaseHelper.persistNewRegistrar; import static google.registry.testing.DatabaseHelper.persistResource; import static google.registry.testing.DomainBaseSubject.assertAboutDomains; import static google.registry.testing.EppExceptionSubject.assertAboutEppExceptions; @@ -76,12 +78,14 @@ import google.registry.model.registry.Registry; import google.registry.model.reporting.HistoryEntry; import google.registry.model.transfer.DomainTransferData; import google.registry.model.transfer.TransferStatus; +import google.registry.testing.DualDatabaseTest; import google.registry.testing.TaskQueueHelper.TaskMatcher; +import google.registry.testing.TestOfyAndSql; import javax.annotation.Nullable; import org.joda.time.DateTime; -import org.junit.jupiter.api.Test; /** Unit tests for {@link HostUpdateFlow}. */ +@DualDatabaseTest class HostUpdateFlowTest extends ResourceFlowTestCase { private void setEppHostUpdateInput( @@ -132,7 +136,7 @@ class HostUpdateFlowTest extends ResourceFlowTestCase oldFkiBeforeRename = - ForeignKeyIndex.load(HostResource.class, oldHostName(), clock.nowUtc().minusMillis(1)); - assertThat(oldFkiBeforeRename.getResourceKey()).isEqualTo(renamedHost.createVKey()); - assertThat(oldFkiBeforeRename.getDeletionTime()).isEqualTo(clock.nowUtc()); + // As we don't actually store ForeignKeyIndex entity in SQL database(it is + // reconstructed by reading the information from Host table), we can not reconstruct the entity + // for the old host name because of the rename so we just skip the check for SQL database. + if (tm().isOfy()) { + // The old ForeignKeyIndex is invalidated at the time we did the rename. + ForeignKeyIndex oldFkiBeforeRename = + ForeignKeyIndex.load(HostResource.class, oldHostName(), clock.nowUtc().minusMillis(1)); + assertThat(oldFkiBeforeRename.getResourceKey()).isEqualTo(renamedHost.createVKey()); + assertThat(oldFkiBeforeRename.getDeletionTime()).isEqualTo(clock.nowUtc()); + } ForeignKeyIndex oldFkiAfterRename = ForeignKeyIndex.load(HostResource.class, oldHostName(), clock.nowUtc()); assertThat(oldFkiAfterRename).isNull(); } - @Test + @TestOfyAndSql void testSuccess_withReferencingDomain() throws Exception { createTld("tld"); createTld("xn--q9jyb4c"); @@ -207,7 +216,7 @@ class HostUpdateFlowTest extends ResourceFlowTestCase tm().loadByEntity(domain)).cloneProjectedAtTime(now); assertThat(reloadedDomain.getSubordinateHosts()).containsExactly("ns2.example.tld"); assertDnsTasksEnqueued("ns1.example.tld", "ns2.example.tld"); } - @Test + @TestOfyAndSql void testSuccess_internalToInternalOnSameTld() throws Exception { setEppHostUpdateInput( "ns2.foo.tld", @@ -320,14 +330,20 @@ class HostUpdateFlowTest extends ResourceFlowTestCase tm().loadByEntity(foo)) + .cloneProjectedAtTime(now) + .getSubordinateHosts()) .isEmpty(); - assertThat(ofy().load().entity(example).now().cloneProjectedAtTime(now).getSubordinateHosts()) + assertThat( + transactIfJpaTm(() -> tm().loadByEntity(example)) + .cloneProjectedAtTime(now) + .getSubordinateHosts()) .containsExactly("ns2.example.tld"); assertDnsTasksEnqueued("ns2.foo.tld", "ns2.example.tld"); } - @Test + @TestOfyAndSql void testSuccess_internalToInternalOnDifferentTld() throws Exception { setEppHostUpdateInput( "ns1.example.foo", @@ -358,15 +374,15 @@ class HostUpdateFlowTest extends ResourceFlowTestCase tm().loadByEntity(fooDomain)).cloneProjectedAtTime(now); assertThat(reloadedFooDomain.getSubordinateHosts()).isEmpty(); DomainBase reloadedTldDomain = - ofy().load().entity(tldDomain).now().cloneProjectedAtTime(now); + transactIfJpaTm(() -> tm().loadByEntity(tldDomain)).cloneProjectedAtTime(now); assertThat(reloadedTldDomain.getSubordinateHosts()).containsExactly("ns2.example.tld"); assertDnsTasksEnqueued("ns1.example.foo", "ns2.example.tld"); } - @Test + @TestOfyAndSql void testSuccess_internalToExternal() throws Exception { setEppHostUpdateInput( "ns1.example.foo", @@ -374,6 +390,8 @@ class HostUpdateFlowTest extends ResourceFlowTestCase1080:0:0:0:8:800:200C:417A"); createTld("foo"); + // This registrar should be superseded by domain's registrar. + persistNewRegistrar("Superseded"); DomainBase domain = persistResource( newDomainBase("example.foo") @@ -386,7 +404,7 @@ class HostUpdateFlowTest extends ResourceFlowTestCase tm().loadByEntity(domain)).cloneProjectedAtTime(clock.nowUtc()); assertThat(reloadedDomain.getSubordinateHosts()).isEmpty(); assertDnsTasksEnqueued("ns1.example.foo"); } - @Test + @TestOfyAndSql void testFailure_externalToInternal() throws Exception { setEppHostUpdateInput( "ns1.example.foo", "ns2.example.tld", "192.0.2.22", null); @@ -420,7 +438,7 @@ class HostUpdateFlowTest extends ResourceFlowTestCase192.0.2.22", null); @@ -439,12 +457,15 @@ class HostUpdateFlowTest extends ResourceFlowTestCase tm().loadByEntity(domain)) + .cloneProjectedAtTime(now) + .getSubordinateHosts()) .containsExactly("ns2.example.tld"); assertDnsTasksEnqueued("ns2.example.tld"); } - @Test + @TestOfyAndSql void testFailure_externalToExternal() throws Exception { setEppHostUpdateInput("ns1.example.foo", "ns2.example.tld", null, null); persistActiveHost(oldHostName()); @@ -452,7 +473,7 @@ class HostUpdateFlowTest extends ResourceFlowTestCase1080:0:0:0:8:800:200C:417A"); createTld("tld"); DomainBase domain = - newDomainBase("foo.tld") - .asBuilder() - .setLastTransferTime(clock.nowUtc().minusDays(5)) - .build(); + persistResource( + newDomainBase("foo.tld") + .asBuilder() + .setLastTransferTime(clock.nowUtc().minusDays(5)) + .build()); // Set the new domain to have a last transfer time that is different than the last transfer // time on the host in question. persistResource( @@ -565,7 +588,7 @@ class HostUpdateFlowTest extends ResourceFlowTestCase1080:0:0:0:8:800:200C:417A"); createTld("tld"); DomainBase foo = - newDomainBase("foo.tld") - .asBuilder() - .setLastTransferTime(clock.nowUtc().minusDays(5)) - .build(); + persistResource( + newDomainBase("foo.tld") + .asBuilder() + .setLastTransferTime(clock.nowUtc().minusDays(5)) + .build()); // Set the new domain to have a null last transfer time. persistResource(newDomainBase("example.tld").asBuilder().setLastTransferTime(null).build()); DateTime lastTransferTime = clock.nowUtc().minusDays(20); @@ -600,7 +624,7 @@ class HostUpdateFlowTest extends ResourceFlowTestCase192.0.2.22", "1080:0:0:0:8:800:200C:417A"); createTld("tld"); - DomainBase foo = newDomainBase("foo.tld").asBuilder().setLastTransferTime(null).build(); + DomainBase foo = + persistResource(newDomainBase("foo.tld").asBuilder().setLastTransferTime(null).build()); // Set the new domain to have a null last transfer time. persistResource(newDomainBase("example.tld").asBuilder().setLastTransferTime(null).build()); DateTime lastTransferTime = clock.nowUtc().minusDays(20); @@ -631,7 +656,7 @@ class HostUpdateFlowTest extends ResourceFlowTestCase1080:0:0:0:8:800:200C:417A"); createTld("tld"); DomainBase foo = - newDomainBase("foo.tld") - .asBuilder() - .setLastTransferTime(clock.nowUtc().minusDays(5)) - .build(); + persistResource( + newDomainBase("foo.tld") + .asBuilder() + .setLastTransferTime(clock.nowUtc().minusDays(5)) + .build()); // Set the new domain to have a null last transfer time. persistResource(newDomainBase("example.tld").asBuilder().setLastTransferTime(null).build()); persistResource( @@ -662,7 +688,7 @@ class HostUpdateFlowTest extends ResourceFlowTestCase192.0.2.22", null); @@ -966,7 +992,7 @@ class HostUpdateFlowTest extends ResourceFlowTestCase192.0.2.22", null); @@ -1179,7 +1205,7 @@ class HostUpdateFlowTest extends ResourceFlowTestCase192.0.2.22", null); @@ -1207,7 +1233,7 @@ class HostUpdateFlowTest extends ResourceFlowTestCase192.0.2.22", null); @@ -1246,17 +1272,17 @@ class HostUpdateFlowTest extends ResourceFlowTestCase