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()

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<HostResource> subordinateHosts =
+        hosts.stream().filter(HostResource::isSubordinate).collect(toImmutableList());
+    ImmutableMap<HostResource, String> 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 = Registrar.loadByRegistrarIdCached(registrarId);
       checkState(registrar.isPresent(), "Could not load registrar %s", registrarId);

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));
+  }
 }

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.