Improve ListDomainsCommand to list domains on multiple TLDs

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=140053423

java/google/registry/tools/ListDomainsCommand.java

@@ -14,20 +14,24 @@
 
 package google.registry.tools;
 
+import static com.google.common.base.Preconditions.checkArgument;
+
 import com.beust.jcommander.Parameter;
 import com.beust.jcommander.Parameters;
+import com.google.common.base.Joiner;
 import com.google.common.collect.ImmutableMap;
 import google.registry.tools.server.ListDomainsAction;
+import java.util.List;
 
-/** Command to list all second-level domains associated with a TLD. */
+/** Command to list all second-level domains on specified TLD(s). */
-@Parameters(separators = " =", commandDescription = "List domains associated with a TLD.")
+@Parameters(separators = " =", commandDescription = "List domains on TLD(s).")
 final class ListDomainsCommand extends ListObjectsCommand {
 
   @Parameter(
-      names = {"-t", "--tld"},
+      names = {"-t", "--tld", "--tlds"},
-      description = "Top level domain whose second-level domains shall be listed.",
+      description = "Comma-delimited list of top-level domain(s) to list second-level domains of.",
       required = true)
-  private String tld;
+  private List<String> tlds;
 
   @Override
   String getCommandPath() {
@@ -38,6 +42,8 @@ final class ListDomainsCommand extends ListObjectsCommand {
    * (in addition to the usual ones). */
   @Override
   ImmutableMap<String, Object> getParameterMap() {
-    return ImmutableMap.<String, Object>of("tld", tld);
+    String tldsParam = Joiner.on(',').join(tlds);
+    checkArgument(tldsParam.length() < 1024, "Total length of TLDs is too long for URL parameter");
+    return ImmutableMap.<String, Object>of("tlds", tldsParam);
   }
 }

java/google/registry/tools/server/ListDomainsAction.java

@@ -14,28 +14,38 @@
 
 package google.registry.tools.server;
 
+import static com.google.common.base.Preconditions.checkArgument;
 import static google.registry.model.EppResourceUtils.queryNotDeleted;
 import static google.registry.model.registry.Registries.assertTldExists;
 import static google.registry.request.Action.Method.GET;
 import static google.registry.request.Action.Method.POST;
 
-import com.google.common.collect.FluentIterable;
 import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.ImmutableSortedSet;
+import com.google.common.collect.Lists;
 import google.registry.model.domain.DomainResource;
 import google.registry.request.Action;
 import google.registry.request.Parameter;
 import google.registry.util.Clock;
 import java.util.Comparator;
+import java.util.List;
 import javax.inject.Inject;
 
 /** An action that lists domains, for use by the {@code nomulus list_domains} command. */
 @Action(path = ListDomainsAction.PATH, method = {GET, POST})
 public final class ListDomainsAction extends ListObjectsAction<DomainResource> {
 
+  /** An App Engine limitation on how many subqueries can be used in a single query. */
+  private static final int MAX_NUM_SUBQUERIES = 30;
+  private static final Comparator<DomainResource> COMPARATOR =
+      new Comparator<DomainResource>() {
+          @Override
+          public int compare(DomainResource a, DomainResource b) {
+            return a.getFullyQualifiedDomainName().compareTo(b.getFullyQualifiedDomainName());
+          }};
   public static final String PATH = "/_dr/admin/list/domains";
-  public static final String TLD_PARAM = "tld";
 
-  @Inject @Parameter("tld") String tld;
+  @Inject @Parameter("tlds") ImmutableSet<String> tlds;
   @Inject Clock clock;
   @Inject ListDomainsAction() {}
 
@@ -46,12 +56,15 @@ public final class ListDomainsAction extends ListObjectsAction<DomainResource> {
 
   @Override
   public ImmutableSet<DomainResource> loadObjects() {
-    return FluentIterable
+    checkArgument(!tlds.isEmpty(), "Must specify TLDs to query");
-        .from(queryNotDeleted(DomainResource.class, clock.nowUtc(), "tld", assertTldExists(tld)))
+    for (String tld : tlds) {
-        .toSortedSet(new Comparator<DomainResource>() {
+      assertTldExists(tld);
-          @Override
+    }
-          public int compare(DomainResource a, DomainResource b) {
+    ImmutableSortedSet.Builder<DomainResource> builder =
-            return a.getFullyQualifiedDomainName().compareTo(b.getFullyQualifiedDomainName());
+        new ImmutableSortedSet.Builder<DomainResource>(COMPARATOR);
-          }});
+    for (List<String> batch : Lists.partition(tlds.asList(), MAX_NUM_SUBQUERIES)) {
+      builder.addAll(queryNotDeleted(DomainResource.class, clock.nowUtc(), "tld in", batch));
+    }
+    return builder.build();
   }
 }

java/google/registry/tools/server/ToolsServerModule.java

@@ -20,6 +20,8 @@ import static google.registry.request.RequestParameters.extractOptionalParameter
 import static google.registry.request.RequestParameters.extractRequiredParameter;
 
 import com.google.common.base.Optional;
+import com.google.common.base.Splitter;
+import com.google.common.collect.ImmutableSet;
 import dagger.Module;
 import dagger.Provides;
 import google.registry.request.Parameter;
@@ -81,6 +83,13 @@ public class ToolsServerModule {
     return extractRequiredParameter(req, "tld");
   }
 
+  @Provides
+  @Parameter("tlds")
+  static ImmutableSet<String> provideTlds(HttpServletRequest req) {
+    String tldsString = extractRequiredParameter(req, "tlds");
+    return ImmutableSet.copyOf(Splitter.on(',').split(tldsString));
+  }
+
   @Provides
   @Parameter("rawKeys")
   static String provideRawKeys(HttpServletRequest req) {

javatests/google/registry/tools/ListDomainsCommandTest.java

@@ -14,7 +14,11 @@
 
 package google.registry.tools;
 
+import com.google.common.base.Strings;
+import com.google.common.collect.ImmutableList;
 import google.registry.tools.server.ListDomainsAction;
+import java.util.List;
+import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.mockito.runners.MockitoJUnitRunner;
 
@@ -32,7 +36,15 @@ public class ListDomainsCommandTest extends ListObjectsCommandTestCase<ListDomai
   }
 
   @Override
-  final String getTld() {
+  protected List<String> getTlds() {
-    return "foo";
+    return ImmutableList.of("foo");
+  }
+
+  @Test
+  public void test_tldsParamTooLong() throws Exception {
+    String tldsParam = "--tld=foo,bar" + Strings.repeat(",baz", 300);
+    thrown.expect(
+        IllegalArgumentException.class, "Total length of TLDs is too long for URL parameter");
+    runCommand(tldsParam);
   }
 }

javatests/google/registry/tools/ListHostsCommandTest.java

@@ -27,9 +27,4 @@ public class ListHostsCommandTest extends ListObjectsCommandTestCase<ListHostsCo
   final String getTaskPath() {
     return ListHostsAction.PATH;
   }
-
-  @Override
-  final String getTld() {
-    return null;
-  }
 }

javatests/google/registry/tools/ListObjectsCommandTestCase.java

@@ -15,7 +15,6 @@
 package google.registry.tools;
 
 import static google.registry.request.JsonResponse.JSON_SAFETY_PREFIX;
-import static google.registry.testing.DatastoreHelper.createTld;
 import static google.registry.tools.server.ListObjectsAction.FIELDS_PARAM;
 import static google.registry.tools.server.ListObjectsAction.FULL_FIELD_NAMES_PARAM;
 import static google.registry.tools.server.ListObjectsAction.PRINT_HEADER_ROW_PARAM;
@@ -25,10 +24,14 @@ import static org.mockito.Matchers.eq;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
+import com.google.common.base.Joiner;
 import com.google.common.base.Optional;
+import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.net.MediaType;
 import google.registry.tools.ServerSideCommand.Connection;
+import java.util.List;
+import javax.annotation.Nullable;
 import org.junit.Before;
 import org.junit.Test;
 import org.mockito.Mock;
@@ -46,24 +49,20 @@ public abstract class ListObjectsCommandTestCase<C extends ListObjectsCommand>
   abstract String getTaskPath();
 
   /**
-   * The TLD to be used (for those subclasses that use TLDs; may be null).
+   * The TLD to be used (for those subclasses that use TLDs; defaults to empty).
    */
-  abstract String getTld();
+  protected List<String> getTlds() {
+    return ImmutableList.<String>of();
+  }
 
   /**
-   * The TLD argument to be passed on the command line; null if not needed.
+   * The TLDs argument to be passed on the command line; null if not needed.
    */
-  String tldArgumentString;
+  @Nullable String tldsParameter;
 
   @Before
   public void init() throws Exception {
-    String tld = getTld();
+    tldsParameter = getTlds().isEmpty() ? null : ("--tld=" + Joiner.on(',').join(getTlds()));
-    if (tld == null) {
-      tldArgumentString = null;
-    } else {
-      createTld(tld);
-      tldArgumentString = "--tld=" + tld;
-    }
     command.setConnection(connection);
     when(
         connection.send(
@@ -89,9 +88,8 @@ public abstract class ListObjectsCommandTestCase<C extends ListObjectsCommand>
     if (fullFieldNames.isPresent()) {
       params.put(FULL_FIELD_NAMES_PARAM, fullFieldNames.get());
     }
-    String tld = getTld();
+    if (!getTlds().isEmpty()) {
-    if (tld != null) {
+      params.put("tlds", Joiner.on(',').join(getTlds()));
-      params.put("tld", tld);
     }
     verify(connection).send(
         eq(getTaskPath()),
@@ -102,74 +100,74 @@ public abstract class ListObjectsCommandTestCase<C extends ListObjectsCommand>
 
   @Test
   public void testRun_noFields() throws Exception {
-    if (tldArgumentString == null) {
+    if (tldsParameter == null) {
       runCommand();
     } else {
-      runCommand(tldArgumentString);
+      runCommand(tldsParameter);
     }
     verifySent(null, Optional.<Boolean>absent(), Optional.<Boolean>absent());
   }
 
   @Test
   public void testRun_oneField() throws Exception {
-    if (tldArgumentString == null) {
+    if (tldsParameter == null) {
       runCommand("--fields=fieldName");
     } else {
-      runCommand("--fields=fieldName", tldArgumentString);
+      runCommand("--fields=fieldName", tldsParameter);
     }
     verifySent("fieldName", Optional.<Boolean>absent(), Optional.<Boolean>absent());
   }
 
   @Test
   public void testRun_wildcardField() throws Exception {
-    if (tldArgumentString == null) {
+    if (tldsParameter == null) {
       runCommand("--fields=*");
     } else {
-      runCommand("--fields=*", tldArgumentString);
+      runCommand("--fields=*", tldsParameter);
     }
     verifySent("*", Optional.<Boolean>absent(), Optional.<Boolean>absent());
   }
 
   @Test
   public void testRun_header() throws Exception {
-    if (tldArgumentString == null) {
+    if (tldsParameter == null) {
       runCommand("--fields=fieldName", "--header=true");
     } else {
-      runCommand("--fields=fieldName", "--header=true", tldArgumentString);
+      runCommand("--fields=fieldName", "--header=true", tldsParameter);
     }
     verifySent("fieldName", Optional.of(Boolean.TRUE), Optional.<Boolean>absent());
   }
 
   @Test
   public void testRun_noHeader() throws Exception {
-    if (tldArgumentString == null) {
+    if (tldsParameter == null) {
       runCommand("--fields=fieldName", "--header=false");
     } else {
-      runCommand("--fields=fieldName", "--header=false", tldArgumentString);
+      runCommand("--fields=fieldName", "--header=false", tldsParameter);
     }
     verifySent("fieldName", Optional.of(Boolean.FALSE), Optional.<Boolean>absent());
   }
 
   @Test
   public void testRun_fullFieldNames() throws Exception {
-    if (tldArgumentString == null) {
+    if (tldsParameter == null) {
       runCommand("--fields=fieldName", "--full_field_names");
     } else {
-      runCommand("--fields=fieldName", "--full_field_names", tldArgumentString);
+      runCommand("--fields=fieldName", "--full_field_names", tldsParameter);
     }
     verifySent("fieldName", Optional.<Boolean>absent(), Optional.of(Boolean.TRUE));
   }
 
   @Test
   public void testRun_allParameters() throws Exception {
-    if (tldArgumentString == null) {
+    if (tldsParameter == null) {
       runCommand("--fields=fieldName,otherFieldName,*", "--header=true", "--full_field_names");
     } else {
       runCommand(
           "--fields=fieldName,otherFieldName,*",
           "--header=true",
           "--full_field_names",
-          tldArgumentString);
+          tldsParameter);
     }
     verifySent(
         "fieldName,otherFieldName,*", Optional.of(Boolean.TRUE), Optional.of(Boolean.TRUE));

javatests/google/registry/tools/ListPremiumListsCommandTest.java

@@ -28,9 +28,4 @@ public class ListPremiumListsCommandTest
   final String getTaskPath() {
     return ListPremiumListsAction.PATH;
   }
-
-  @Override
-  final String getTld() {
-    return null;
-  }
 }

javatests/google/registry/tools/ListRegistrarsCommandTest.java

@@ -28,9 +28,4 @@ public class ListRegistrarsCommandTest
   final String getTaskPath() {
     return ListRegistrarsAction.PATH;
   }
-
-  @Override
-  final String getTld() {
-    return null;
-  }
 }

javatests/google/registry/tools/ListReservedListsCommandTest.java

@@ -28,9 +28,4 @@ public class ListReservedListsCommandTest
   final String getTaskPath() {
     return ListReservedListsAction.PATH;
   }
-
-  @Override
-  final String getTld() {
-    return null;
-  }
 }

javatests/google/registry/tools/ListTldsCommandTest.java

@@ -21,16 +21,10 @@ import google.registry.tools.server.ListTldsAction;
  *
  * @see ListObjectsCommandTestCase
  */
-public class ListTldsCommandTest
+public class ListTldsCommandTest extends ListObjectsCommandTestCase<ListTldsCommand> {
-    extends ListObjectsCommandTestCase<ListTldsCommand> {
 
   @Override
   final String getTaskPath() {
     return ListTldsAction.PATH;
   }
-
-  @Override
-  final String getTld() {
-    return null;
-  }
 }

javatests/google/registry/tools/server/ListDomainsActionTest.java

@@ -19,6 +19,7 @@ import static google.registry.testing.DatastoreHelper.createTlds;
 import static google.registry.testing.DatastoreHelper.persistActiveDomain;
 
 import com.google.common.base.Optional;
+import com.google.common.collect.ImmutableSet;
 import google.registry.testing.FakeClock;
 import org.joda.time.DateTime;
 import org.junit.Before;
@@ -42,19 +43,19 @@ public class ListDomainsActionTest extends ListActionTestCase {
   }
 
   @Test
-  public void testRun_invalidRequest_missingTld() throws Exception {
+  public void testRun_invalidRequest_missingTlds() throws Exception {
-    action.tld = null;
+    action.tlds = ImmutableSet.of();
     testRunError(
         action,
         null,
         null,
         null,
-        "^Null or empty TLD specified$");
+        "^Must specify TLDs to query$");
   }
 
   @Test
   public void testRun_invalidRequest_invalidTld() throws Exception {
-    action.tld = "%%%badtld%%%";
+    action.tlds = ImmutableSet.of("%%%badtld%%%");
     testRunError(
         action,
         null,
@@ -65,7 +66,7 @@ public class ListDomainsActionTest extends ListActionTestCase {
 
   @Test
   public void testRun_noParameters() throws Exception {
-    action.tld = "foo";
+    action.tlds = ImmutableSet.of("foo");
     testRunSuccess(
         action,
         null,
@@ -75,7 +76,7 @@ public class ListDomainsActionTest extends ListActionTestCase {
 
   @Test
   public void testRun_twoLinesWithIdOnly() throws Exception {
-    action.tld = "foo";
+    action.tlds = ImmutableSet.of("foo");
     createTlds("bar", "sim");
     persistActiveDomain("dontlist.bar");
     persistActiveDomain("example1.foo");
@@ -91,9 +92,28 @@ public class ListDomainsActionTest extends ListActionTestCase {
         "^example2.foo$");
   }
 
+  @Test
+  public void testRun_multipleTlds() throws Exception {
+    action.tlds = ImmutableSet.of("bar", "foo");
+    createTlds("bar", "sim");
+    persistActiveDomain("dolist.bar");
+    persistActiveDomain("example1.foo");
+    persistActiveDomain("example2.foo");
+    persistActiveDomain("notlistedaswell.sim");
+    testRunSuccess(
+        action,
+        null,
+        null,
+        null,
+        "^dolist.bar",
+        "^example1.foo$",
+        "^example2.foo$");
+  }
+
+
   @Test
   public void testRun_twoLinesWithIdOnlyNoHeader() throws Exception {
-    action.tld = "foo";
+    action.tlds = ImmutableSet.of("foo");
     persistActiveDomain("example1.foo");
     persistActiveDomain("example2.foo");
     testRunSuccess(
@@ -107,7 +127,7 @@ public class ListDomainsActionTest extends ListActionTestCase {
 
   @Test
   public void testRun_twoLinesWithIdOnlyExplicitHeader() throws Exception {
-    action.tld = "foo";
+    action.tlds = ImmutableSet.of("foo");
     persistActiveDomain("example1.foo");
     persistActiveDomain("example2.foo");
     testRunSuccess(
@@ -123,7 +143,7 @@ public class ListDomainsActionTest extends ListActionTestCase {
 
   @Test
   public void testRun_twoLinesWithRepoId() throws Exception {
-    action.tld = "foo";
+    action.tlds = ImmutableSet.of("foo");
     persistActiveDomain("example1.foo");
     persistActiveDomain("example3.foo");
     testRunSuccess(
@@ -139,7 +159,7 @@ public class ListDomainsActionTest extends ListActionTestCase {
 
   @Test
   public void testRun_twoLinesWithRepoIdNoHeader() throws Exception {
-    action.tld = "foo";
+    action.tlds = ImmutableSet.of("foo");
     persistActiveDomain("example1.foo");
     persistActiveDomain("example3.foo");
     testRunSuccess(
@@ -153,7 +173,7 @@ public class ListDomainsActionTest extends ListActionTestCase {
 
   @Test
   public void testRun_twoLinesWithRepoIdExplicitHeader() throws Exception {
-    action.tld = "foo";
+    action.tlds = ImmutableSet.of("foo");
     persistActiveDomain("example1.foo");
     persistActiveDomain("example3.foo");
     testRunSuccess(
@@ -169,7 +189,7 @@ public class ListDomainsActionTest extends ListActionTestCase {
 
   @Test
   public void testRun_twoLinesWithWildcard() throws Exception {
-    action.tld = "foo";
+    action.tlds = ImmutableSet.of("foo");
     persistActiveDomain("example1.foo");
     persistActiveDomain("example3.foo");
     testRunSuccess(
@@ -185,7 +205,7 @@ public class ListDomainsActionTest extends ListActionTestCase {
 
   @Test
   public void testRun_twoLinesWithWildcardAndAnotherField() throws Exception {
-    action.tld = "foo";
+    action.tlds = ImmutableSet.of("foo");
     persistActiveDomain("example1.foo");
     persistActiveDomain("example3.foo");
     testRunSuccess(
@@ -201,7 +221,7 @@ public class ListDomainsActionTest extends ListActionTestCase {
 
   @Test
   public void testRun_withBadField_returnsError() throws Exception {
-    action.tld = "foo";
+    action.tlds = ImmutableSet.of("foo");
     persistActiveDomain("example2.foo");
     persistActiveDomain("example1.foo");
     testRunError(