From 05fcf734520f1fe9ff57f05299eae32d81b5ca29 Mon Sep 17 00:00:00 2001 From: Michael Muller Date: Wed, 4 May 2022 07:29:45 -0400 Subject: [PATCH] Add missing transaction for whois lookups (#1614) * Add missing transaction for whois lookups Nameserver whois lookups are failing under SQL for hosts with superordinate domains because the query in this case is not done in a transaction. We missed this during testing because a) we didn't have a test for lookups of hosts with superordinate domains and b) we missed converting NameserverWhoisResponseTest to a DualDatabaseTest. This PR fixes the problem and adds the requisite testing. * Use a single transaction to get host registrars * Replace streaming with Maps.toMap() --- .../whois/NameserverWhoisResponse.java | 22 ++++++++++-- .../whois/NameserverWhoisResponseTest.java | 35 +++++++++++++++++-- .../whois/whois_subord_nameserver.txt | 11 ++++++ 3 files changed, 62 insertions(+), 6 deletions(-) create mode 100644 core/src/test/resources/google/registry/whois/whois_subord_nameserver.txt diff --git a/core/src/main/java/google/registry/whois/NameserverWhoisResponse.java b/core/src/main/java/google/registry/whois/NameserverWhoisResponse.java index d272425ad..72b3c3f7b 100644 --- a/core/src/main/java/google/registry/whois/NameserverWhoisResponse.java +++ b/core/src/main/java/google/registry/whois/NameserverWhoisResponse.java @@ -16,9 +16,12 @@ package google.registry.whois; import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.base.Preconditions.checkState; +import static com.google.common.collect.ImmutableList.toImmutableList; import static google.registry.persistence.transaction.TransactionManagerFactory.tm; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; +import com.google.common.collect.Maps; import com.google.common.net.InetAddresses; import google.registry.model.host.HostResource; import google.registry.model.registrar.Registrar; @@ -44,14 +47,27 @@ final class NameserverWhoisResponse extends WhoisResponseImpl { @Override public WhoisResponseResults getResponse(boolean preferUnicode, String disclaimer) { + // If we have subordinate hosts, load their registrar ids in a single transaction up-front. + ImmutableList subordinateHosts = + hosts.stream().filter(HostResource::isSubordinate).collect(toImmutableList()); + ImmutableMap hostRegistrars = + subordinateHosts.isEmpty() + ? ImmutableMap.of() + : tm().transact( + () -> + Maps.toMap( + subordinateHosts.iterator(), + host -> + tm().loadByKey(host.getSuperordinateDomain()) + .cloneProjectedAtTime(getTimestamp()) + .getCurrentSponsorRegistrarId())); + BasicEmitter emitter = new BasicEmitter(); for (int i = 0; i < hosts.size(); i++) { HostResource host = hosts.get(i); String registrarId = host.isSubordinate() - ? tm().loadByKey(host.getSuperordinateDomain()) - .cloneProjectedAtTime(getTimestamp()) - .getCurrentSponsorRegistrarId() + ? hostRegistrars.get(host) : host.getPersistedCurrentSponsorRegistrarId(); Optional registrar = Registrar.loadByRegistrarIdCached(registrarId); checkState(registrar.isPresent(), "Could not load registrar %s", registrarId); diff --git a/core/src/test/java/google/registry/whois/NameserverWhoisResponseTest.java b/core/src/test/java/google/registry/whois/NameserverWhoisResponseTest.java index 81bae85da..083adaed2 100644 --- a/core/src/test/java/google/registry/whois/NameserverWhoisResponseTest.java +++ b/core/src/test/java/google/registry/whois/NameserverWhoisResponseTest.java @@ -17,6 +17,7 @@ package google.registry.whois; import static com.google.common.truth.Truth.assertThat; import static google.registry.testing.DatabaseHelper.createTld; import static google.registry.testing.DatabaseHelper.loadRegistrar; +import static google.registry.testing.DatabaseHelper.newDomainBase; import static google.registry.testing.DatabaseHelper.persistNewRegistrar; import static google.registry.testing.DatabaseHelper.persistResource; import static google.registry.whois.WhoisTestData.loadFile; @@ -24,17 +25,20 @@ import static google.registry.whois.WhoisTestData.loadFile; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import com.google.common.net.InetAddresses; +import google.registry.model.domain.DomainBase; import google.registry.model.host.HostResource; import google.registry.model.registrar.Registrar; import google.registry.testing.AppEngineExtension; +import google.registry.testing.DualDatabaseTest; import google.registry.testing.FakeClock; +import google.registry.testing.TestOfyAndSql; import google.registry.whois.WhoisResponse.WhoisResponseResults; import org.joda.time.DateTime; import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.RegisterExtension; /** Unit tests for {@link NameserverWhoisResponse}. */ +@DualDatabaseTest class NameserverWhoisResponseTest { @RegisterExtension @@ -43,6 +47,7 @@ class NameserverWhoisResponseTest { private HostResource hostResource1; private HostResource hostResource2; + private HostResource hostResource3; private final FakeClock clock = new FakeClock(DateTime.parse("2009-05-29T20:15:00Z")); @@ -51,6 +56,7 @@ class NameserverWhoisResponseTest { persistNewRegistrar("example", "Hänsel & Gretel Registrar, Inc.", Registrar.Type.REAL, 8L); persistResource(loadRegistrar("example").asBuilder().setUrl("http://my.fake.url").build()); createTld("tld"); + DomainBase domain = persistResource(newDomainBase("zobo.tld")); hostResource1 = new HostResource.Builder() @@ -73,9 +79,21 @@ class NameserverWhoisResponseTest { InetAddresses.forString("2001:0DB8::1"))) .setRepoId("2-EXAMPLE") .build(); + + hostResource3 = + new HostResource.Builder() + .setHostName("ns1.zobo.tld") + .setSuperordinateDomain(domain.createVKey()) + .setPersistedCurrentSponsorRegistrarId("example") + .setInetAddresses( + ImmutableSet.of( + InetAddresses.forString("192.0.2.123"), + InetAddresses.forString("2001:0DB8::1"))) + .setRepoId("3-EXAMPLE") + .build(); } - @Test + @TestOfyAndSql void testGetTextOutput() { NameserverWhoisResponse nameserverWhoisResponse = new NameserverWhoisResponse(hostResource1, clock.nowUtc()); @@ -86,7 +104,7 @@ class NameserverWhoisResponseTest { .isEqualTo(WhoisResponseResults.create(loadFile("whois_nameserver.txt"), 1)); } - @Test + @TestOfyAndSql void testGetMultipleNameserversResponse() { NameserverWhoisResponse nameserverWhoisResponse = new NameserverWhoisResponse(ImmutableList.of(hostResource1, hostResource2), clock.nowUtc()); @@ -96,4 +114,15 @@ class NameserverWhoisResponseTest { "Doodle Disclaimer\nI exist so that carriage return\nin disclaimer can be tested.")) .isEqualTo(WhoisResponseResults.create(loadFile("whois_multiple_nameservers.txt"), 2)); } + + @TestOfyAndSql + void testSubordinateDomains() { + NameserverWhoisResponse nameserverWhoisResponse = + new NameserverWhoisResponse(hostResource3, clock.nowUtc()); + assertThat( + nameserverWhoisResponse.getResponse( + false, + "Doodle Disclaimer\nI exist so that carriage return\nin disclaimer can be tested.")) + .isEqualTo(WhoisResponseResults.create(loadFile("whois_subord_nameserver.txt"), 1)); + } } diff --git a/core/src/test/resources/google/registry/whois/whois_subord_nameserver.txt b/core/src/test/resources/google/registry/whois/whois_subord_nameserver.txt new file mode 100644 index 000000000..cdc1bcb56 --- /dev/null +++ b/core/src/test/resources/google/registry/whois/whois_subord_nameserver.txt @@ -0,0 +1,11 @@ +Server Name: ns1.zobo.tld +IP Address: 192.0.2.123 +IP Address: 2001:db8::1 +Registrar: The Registrar +Registrar WHOIS Server: whois.nic.fakewhois.example +Registrar URL: http://my.fake.url +>>> Last update of WHOIS database: 2009-05-29T20:15:00Z <<< + +Doodle Disclaimer +I exist so that carriage return +in disclaimer can be tested.