Allow square bracket expansion when specifying nameservers

I'm finally fed up enough with all the nameserver changes we've had to make on our
self-allocated domains to improve the command.  Now you can simply run:

$ nomulus ... update_domain ... -n ns[1-4].foo.bar

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

java/google/registry/tools/CreateOrUpdateDomainCommand.java

@@ -29,7 +29,9 @@ import com.google.common.collect.ImmutableSet;
 import com.google.common.io.BaseEncoding;
 import com.google.template.soy.data.SoyListData;
 import com.google.template.soy.data.SoyMapData;
+import google.registry.tools.params.NameserversParameter;
 import java.util.ArrayList;
+import java.util.HashSet;
 import java.util.List;
 import java.util.Set;
 
@@ -48,9 +50,10 @@ abstract class CreateOrUpdateDomainCommand extends MutatingEppToolCommand {
 
   @Parameter(
       names = {"-n", "--nameservers"},
-    description = "Comma-separated list of nameservers, up to 13."
+      description = "Comma-delimited list of nameservers, up to 13.",
-  )
+      converter = NameserversParameter.class,
-  List<String> nameservers = new ArrayList<>();
+      validateWith = NameserversParameter.class)
+  Set<String> nameservers = new HashSet<>();
 
   @Parameter(
     names = {"-r", "--registrant"},

java/google/registry/tools/UpdateDomainCommand.java

@@ -32,11 +32,13 @@ import com.google.template.soy.data.SoyMapData;
 import google.registry.model.domain.DesignatedContact;
 import google.registry.model.domain.DomainResource;
 import google.registry.model.eppcommon.StatusValue;
+import google.registry.tools.params.NameserversParameter;
 import google.registry.tools.soy.DomainUpdateSoyInfo;
 import java.util.ArrayList;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Set;
+import java.util.TreeSet;
 import org.joda.time.DateTime;
 
 /** A command to update a new domain via EPP. */
@@ -51,10 +53,11 @@ final class UpdateDomainCommand extends CreateOrUpdateDomainCommand {
   @Parameter(
       names = "--add_nameservers",
       description =
-        "Comma-separated list of nameservers to add, up to 13. "
+          "Comma-delimited list of nameservers to add, up to 13. "
-            + "Cannot be set if --nameservers is set."
+              + "Cannot be set if --nameservers is set.",
-  )
+      converter = NameserversParameter.class,
-  private List<String> addNameservers = new ArrayList<>();
+      validateWith = NameserversParameter.class)
+  private Set<String> addNameservers = new HashSet<>();
 
   @Parameter(
     names = "--add_admins",
@@ -81,10 +84,11 @@ final class UpdateDomainCommand extends CreateOrUpdateDomainCommand {
   @Parameter(
       names = "--remove_nameservers",
       description =
-        "Comma-separated list of nameservers to remove, up to 13. "
+          "Comma-delimited list of nameservers to remove, up to 13. "
-            + "Cannot be set if --nameservers is set."
+              + "Cannot be set if --nameservers is set.",
-  )
+      converter = NameserversParameter.class,
-  private List<String> removeNameservers = new ArrayList<>();
+      validateWith = NameserversParameter.class)
+  private Set<String> removeNameservers = new HashSet<>();
 
   @Parameter(
     names = "--remove_admins",
@@ -156,14 +160,15 @@ final class UpdateDomainCommand extends CreateOrUpdateDomainCommand {
     }
 
     for (String domain : domains) {
-      Set<String> addAdminsThisDomain = new HashSet<>(addAdmins);
+      // Use TreeSets so that the results are always in the same order (this makes testing easier).
-      Set<String> removeAdminsThisDomain = new HashSet<>(removeAdmins);
+      Set<String> addAdminsThisDomain = new TreeSet<>(addAdmins);
-      Set<String> addTechsThisDomain = new HashSet<>(addTechs);
+      Set<String> removeAdminsThisDomain = new TreeSet<>(removeAdmins);
-      Set<String> removeTechsThisDomain = new HashSet<>(removeTechs);
+      Set<String> addTechsThisDomain = new TreeSet<>(addTechs);
-      Set<String> addNameserversThisDomain = new HashSet<>(addNameservers);
+      Set<String> removeTechsThisDomain = new TreeSet<>(removeTechs);
-      Set<String> removeNameserversThisDomain = new HashSet<>(removeNameservers);
+      Set<String> addNameserversThisDomain = new TreeSet<>(addNameservers);
-      Set<String> addStatusesThisDomain = new HashSet<>(addStatuses);
+      Set<String> removeNameserversThisDomain = new TreeSet<>(removeNameservers);
-      Set<String> removeStatusesThisDomain = new HashSet<>(removeStatuses);
+      Set<String> addStatusesThisDomain = new TreeSet<>(addStatuses);
+      Set<String> removeStatusesThisDomain = new TreeSet<>(removeStatuses);
 
       if (!nameservers.isEmpty() || !admins.isEmpty() || !techs.isEmpty() || !statuses.isEmpty()) {
         DateTime now = DateTime.now(UTC);

java/google/registry/tools/params/InternetDomainNameParameter.java

@@ -16,7 +16,7 @@ package google.registry.tools.params;
 
 import com.google.common.net.InternetDomainName;
 
-/** Duration CLI parameter converter/validator. */
+/** InternetDomainName CLI parameter converter/validator. */
 public final class InternetDomainNameParameter
     extends ParameterConverterValidator<InternetDomainName> {
 

java/google/registry/tools/params/NameserversParameter.java

@@ -0,0 +1,80 @@
+// Copyright 2018 The Nomulus Authors. All Rights Reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package google.registry.tools.params;
+
+import static com.google.common.base.Preconditions.checkArgument;
+import static com.google.common.collect.ImmutableSet.toImmutableSet;
+import static java.lang.Integer.parseInt;
+
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Splitter;
+import com.google.common.base.Strings;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
+import com.google.re2j.Matcher;
+import com.google.re2j.Pattern;
+import java.util.Set;
+import java.util.stream.Stream;
+
+/**
+ * Nameservers CLI parameter converter/validator.
+ *
+ * <p>This accepts a String containing a comma-delimited list of nameservers. Square bracket
+ * notation can be used to expand multiple nameservers, e.g. "ns[1-2].googledomains.com" will be
+ * expanded to "ns1.googledomains.com,ns2.googledomains.com".
+ */
+public final class NameserversParameter extends ParameterConverterValidator<Set<String>> {
+
+  private static final Pattern FORMAT_BRACKETS = Pattern.compile("^(.+)\\[(\\d+)-(\\d+)\\](.+)$");
+
+  public NameserversParameter() {
+    super(
+        "Must be a comma-delimited list of nameservers, "
+            + "optionally using square bracket expansion e.g. foo[1-4].bar.baz notation");
+  }
+
+  @Override
+  public Set<String> convert(String value) {
+    if (Strings.isNullOrEmpty(value)) {
+      return ImmutableSet.of();
+    }
+    return Splitter.on(',')
+        .trimResults()
+        .omitEmptyStrings()
+        .splitToList(value)
+        .stream()
+        .flatMap(NameserversParameter::splitNameservers)
+        .collect(toImmutableSet());
+  }
+
+  @VisibleForTesting
+  static Stream<String> splitNameservers(String ns) {
+    Matcher matcher = FORMAT_BRACKETS.matcher(ns);
+    if (!matcher.matches()) {
+      checkArgument(
+          !ns.contains("[") && !ns.contains("]"), "Could not parse square brackets in %s", ns);
+      return ImmutableList.of(ns).stream();
+    }
+
+    ImmutableList.Builder<String> nameservers = new ImmutableList.Builder<>();
+    int start = parseInt(matcher.group(2));
+    int end = parseInt(matcher.group(3));
+    checkArgument(start <= end, "Number range [%s-%s] is invalid", start, end);
+    for (int i = start; i <= end; i++) {
+      nameservers.add(String.format("%s%d%s", matcher.group(1), i, matcher.group(4)));
+    }
+    return nameservers.build().stream();
+  }
+}

javatests/google/registry/tools/CreateDomainCommandTest.java

@@ -47,6 +47,22 @@ public class CreateDomainCommandTest extends EppToolCommandTestCase<CreateDomain
     eppVerifier.verifySent("domain_create_complete.xml");
   }
 
+  @Test
+  public void testSuccess_completeWithSquareBrackets() throws Exception {
+    runCommandForced(
+        "--client=NewRegistrar",
+        "--period=1",
+        "--nameservers=ns[1-4].zdns.google",
+        "--registrant=crr-admin",
+        "--admins=crr-admin",
+        "--techs=crr-tech",
+        "--password=2fooBAR",
+        "--ds_records=1 2 3 abcd,4 5 6 EF01",
+        "--ds_records=60485 5  2  D4B7D520E7BB5F0F67674A0CCEB1E3E0614B93C4F9E99B8383F6A1E4469DA50A",
+        "example.tld");
+    eppVerifier.verifySent("domain_create_complete.xml");
+  }
+
   @Test
   public void testSuccess_minimal() throws Exception {
     // Test that each optional field can be omitted. Also tests the auto-gen password.
@@ -216,6 +232,22 @@ public class CreateDomainCommandTest extends EppToolCommandTestCase<CreateDomain
     assertThat(thrown).hasMessageThat().contains("There can be at most 13 nameservers");
   }
 
+  @Test
+  public void testFailure_tooManyNameServers_usingSquareBracketRange() {
+    IllegalArgumentException thrown =
+        assertThrows(
+            IllegalArgumentException.class,
+            () ->
+                runCommandForced(
+                    "--client=NewRegistrar",
+                    "--registrant=crr-admin",
+                    "--admins=crr-admin",
+                    "--techs=crr-tech",
+                    "--nameservers=ns[1-14].zdns.google",
+                    "example.tld"));
+    assertThat(thrown).hasMessageThat().contains("There can be at most 13 nameservers");
+  }
+
   @Test
   public void testFailure_badPeriod() {
     ParameterException thrown =

javatests/google/registry/tools/UpdateDomainCommandTest.java

@@ -40,12 +40,32 @@ public class UpdateDomainCommandTest extends EppToolCommandTestCase<UpdateDomain
   public void testSuccess_complete() throws Exception {
     runCommandForced(
         "--client=NewRegistrar",
-        "--add_nameservers=ns2.zdns.google,ns3.zdns.google",
+        "--add_nameservers=ns1.zdns.google,ns2.zdns.google",
         "--add_admins=crr-admin2",
         "--add_techs=crr-tech2",
         "--add_statuses=serverDeleteProhibited",
         "--add_ds_records=1 2 3 abcd,4 5 6 EF01",
-        "--remove_nameservers=ns4.zdns.google",
+        "--remove_nameservers=ns3.zdns.google,ns4.zdns.google",
+        "--remove_admins=crr-admin1",
+        "--remove_techs=crr-tech1",
+        "--remove_statuses=serverHold",
+        "--remove_ds_records=7 8 9 12ab,6 5 4 34CD",
+        "--registrant=crr-admin",
+        "--password=2fooBAR",
+        "example.tld");
+    eppVerifier.verifySent("domain_update_complete.xml");
+  }
+
+  @Test
+  public void testSuccess_completeWithSquareBrackets() throws Exception {
+    runCommandForced(
+        "--client=NewRegistrar",
+        "--add_nameservers=ns[1-2].zdns.google",
+        "--add_admins=crr-admin2",
+        "--add_techs=crr-tech2",
+        "--add_statuses=serverDeleteProhibited",
+        "--add_ds_records=1 2 3 abcd,4 5 6 EF01",
+        "--remove_nameservers=ns[3-4].zdns.google",
         "--remove_admins=crr-admin1",
         "--remove_techs=crr-tech1",
         "--remove_statuses=serverHold",
@@ -60,12 +80,12 @@ public class UpdateDomainCommandTest extends EppToolCommandTestCase<UpdateDomain
   public void testSuccess_multipleDomains() throws Exception {
     runCommandForced(
         "--client=NewRegistrar",
-        "--add_nameservers=ns2.zdns.google,ns3.zdns.google",
+        "--add_nameservers=ns1.zdns.google,ns2.zdns.google",
         "--add_admins=crr-admin2",
         "--add_techs=crr-tech2",
         "--add_statuses=serverDeleteProhibited",
         "--add_ds_records=1 2 3 abcd,4 5 6 EF01",
-        "--remove_nameservers=ns4.zdns.google",
+        "--remove_nameservers=ns[3-4].zdns.google",
         "--remove_admins=crr-admin1",
         "--remove_techs=crr-tech1",
         "--remove_statuses=serverHold",
@@ -81,6 +101,15 @@ public class UpdateDomainCommandTest extends EppToolCommandTestCase<UpdateDomain
 
   @Test
   public void testSuccess_multipleDomains_setNameservers() throws Exception {
+    runTest_multipleDomains_setNameservers("-n ns1.foo.fake,ns2.foo.fake");
+  }
+
+  @Test
+  public void testSuccess_multipleDomains_setNameserversWithSquareBrackets() throws Exception {
+    runTest_multipleDomains_setNameservers("-n ns[1-2].foo.fake");
+  }
+
+  private void runTest_multipleDomains_setNameservers(String nsParam) throws Exception {
     createTlds("abc", "tld");
     HostResource host1 = persistActiveHost("foo.bar.tld");
     HostResource host2 = persistActiveHost("baz.bar.tld");
@@ -95,7 +124,7 @@ public class UpdateDomainCommandTest extends EppToolCommandTestCase<UpdateDomain
             .setNameservers(ImmutableSet.of(Key.create(host2)))
             .build());
     runCommandForced(
-        "--client=NewRegistrar", "-n ns1.foo.fake,ns2.foo.fake", "example.abc", "example.tld");
+        "--client=NewRegistrar", nsParam, "example.abc", "example.tld");
     eppVerifier
         .verifySent(
             "domain_update_add_two_hosts_remove_one.xml",

javatests/google/registry/tools/params/NameserversParameterTest.java

@@ -0,0 +1,122 @@
+// Copyright 2018 The Nomulus Authors. All Rights Reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package google.registry.tools.params;
+
+import static com.google.common.truth.Truth.assertThat;
+import static com.google.common.truth.Truth8.assertThat;
+import static google.registry.testing.JUnitBackports.assertThrows;
+import static google.registry.tools.params.NameserversParameter.splitNameservers;
+
+import com.beust.jcommander.ParameterException;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
+
+/** Unit tests for {@link NameserversParameter}. */
+@RunWith(JUnit4.class)
+public class NameserversParameterTest {
+
+  private final NameserversParameter instance = new NameserversParameter();
+
+  @Test
+  public void testConvert_singleBasicNameserver() {
+    assertThat(instance.convert("ns11.goes.to")).containsExactly("ns11.goes.to");
+  }
+
+  @Test
+  public void testConvert_multipleBasicNameservers() {
+    assertThat(instance.convert("ns1.tim.buktu, ns2.tim.buktu, ns3.tim.buktu"))
+        .containsExactly("ns1.tim.buktu", "ns2.tim.buktu", "ns3.tim.buktu");
+  }
+
+  @Test
+  public void testConvert_kitchenSink() {
+    assertThat(instance.convert(" ns1.foo.bar, ,ns2.foo.bar,, ns[1-3].range.baz "))
+        .containsExactly(
+            "ns1.foo.bar", "ns2.foo.bar", "ns1.range.baz", "ns2.range.baz", "ns3.range.baz");
+  }
+
+  @Test
+  public void testConvert_invalid() {
+    assertThat(instance.convert("ns1.foo.bar,ns2.foo.baz,ns3.foo.bar"))
+        .containsExactly("ns1.foo.bar", "ns3.foo.bar", "ns2.foo.baz");
+  }
+
+  @Test
+  public void testValidate_sillyString_throws() {
+    ParameterException thrown =
+        assertThrows(ParameterException.class, () -> instance.validate("nameservers", "[[ns]]"));
+    assertThat(thrown).hasMessageThat().contains("Must be a comma-delimited list of nameservers");
+    assertThat(thrown).hasCauseThat().hasMessageThat().contains("Could not parse square brackets");
+  }
+
+  @Test
+  public void testConvert_empty_returnsEmpty() {
+    assertThat(instance.convert("")).isEmpty();
+  }
+
+  @Test
+  public void testConvert_nullString_returnsNull() {
+    assertThat(instance.convert(null)).isEmpty();
+  }
+
+  @Test
+  public void testSplitNameservers_noopWithNoBrackets() {
+    assertThat(splitNameservers("ns9.fake.example")).containsExactly("ns9.fake.example");
+  }
+
+  @Test
+  public void testSplitNameservers_worksWithBrackets() {
+    assertThat(splitNameservers("ns[1-4].zan.zibar"))
+        .containsExactly("ns1.zan.zibar", "ns2.zan.zibar", "ns3.zan.zibar", "ns4.zan.zibar");
+  }
+
+  @Test
+  public void testSplitNameservers_worksWithBrackets_soloRange() {
+    assertThat(splitNameservers("ns[1-1].zan.zibar")).containsExactly("ns1.zan.zibar");
+  }
+
+  @Test
+  public void testSplitNameservers_throwsOnInvalidRange() {
+    IllegalArgumentException thrown =
+        assertThrows(IllegalArgumentException.class, () -> splitNameservers("ns[9-2].foo.bar"));
+    assertThat(thrown).hasMessageThat().isEqualTo("Number range [9-2] is invalid");
+  }
+
+  @Test
+  public void testSplitNameservers_throwsOnInvalidHostname_missingPrefix() {
+    IllegalArgumentException thrown =
+        assertThrows(IllegalArgumentException.class, () -> splitNameservers("[1-4].foo.bar"));
+    assertThat(thrown)
+        .hasMessageThat()
+        .isEqualTo("Could not parse square brackets in [1-4].foo.bar");
+  }
+
+  @Test
+  public void testSplitNameservers_throwsOnInvalidHostname_missingDomainName() {
+    IllegalArgumentException thrown =
+        assertThrows(IllegalArgumentException.class, () -> splitNameservers("this.is.ns[1-5]"));
+    assertThat(thrown)
+        .hasMessageThat()
+        .isEqualTo("Could not parse square brackets in this.is.ns[1-5]");
+  }
+
+  @Test
+  public void testSplitNameservers_throwsOnInvalidRangeSyntax() {
+    IllegalArgumentException thrown =
+        assertThrows(IllegalArgumentException.class, () -> splitNameservers("ns[1-4[.foo.bar"));
+    assertThat(thrown).hasMessageThat().contains("Could not parse square brackets");
+  }
+}

javatests/google/registry/tools/server/testdata/domain_update_complete.xml

@@ -7,8 +7,8 @@
         <domain:name>example.tld</domain:name>
         <domain:add>
           <domain:ns>
+            <domain:hostObj>ns1.zdns.google</domain:hostObj>
             <domain:hostObj>ns2.zdns.google</domain:hostObj>
-            <domain:hostObj>ns3.zdns.google</domain:hostObj>
           </domain:ns>
           <domain:contact type="admin">crr-admin2</domain:contact>
           <domain:contact type="tech">crr-tech2</domain:contact>
@@ -16,6 +16,7 @@
         </domain:add>
         <domain:rem>
           <domain:ns>
+            <domain:hostObj>ns3.zdns.google</domain:hostObj>
             <domain:hostObj>ns4.zdns.google</domain:hostObj>
           </domain:ns>
           <domain:contact type="admin">crr-admin1</domain:contact>

javatests/google/registry/tools/server/testdata/domain_update_complete_abc.xml

@@ -7,8 +7,8 @@
         <domain:name>example.abc</domain:name>
         <domain:add>
           <domain:ns>
+            <domain:hostObj>ns1.zdns.google</domain:hostObj>
             <domain:hostObj>ns2.zdns.google</domain:hostObj>
-            <domain:hostObj>ns3.zdns.google</domain:hostObj>
           </domain:ns>
           <domain:contact type="admin">crr-admin2</domain:contact>
           <domain:contact type="tech">crr-tech2</domain:contact>
@@ -16,6 +16,7 @@
         </domain:add>
         <domain:rem>
           <domain:ns>
+            <domain:hostObj>ns3.zdns.google</domain:hostObj>
             <domain:hostObj>ns4.zdns.google</domain:hostObj>
           </domain:ns>
           <domain:contact type="admin">crr-admin1</domain:contact>