Fix add/remove calculations when updating multiple domains

Lists used as accumulators were being updated individually for each domain
without starting over from a fresh list each time, so the number of changes
would grow for each additional domain and potentially be wrong if the previous
domains were set up differently.

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

java/google/registry/tools/UpdateDomainCommand.java

@@ -19,6 +19,7 @@ import static com.google.common.collect.ImmutableSet.toImmutableSet;
 import static google.registry.model.EppResourceUtils.loadByForeignKey;
 import static google.registry.model.eppcommon.StatusValue.SERVER_UPDATE_PROHIBITED;
 import static google.registry.model.ofy.ObjectifyService.ofy;
+import static google.registry.util.PreconditionsUtils.checkArgumentNotNull;
 import static org.joda.time.DateTimeZone.UTC;
 
 import com.beust.jcommander.Parameter;
@@ -155,10 +156,19 @@ final class UpdateDomainCommand extends CreateOrUpdateDomainCommand {
     }
 
     for (String domain : domains) {
+      Set<String> addAdminsThisDomain = new HashSet<>(addAdmins);
+      Set<String> removeAdminsThisDomain = new HashSet<>(removeAdmins);
+      Set<String> addTechsThisDomain = new HashSet<>(addTechs);
+      Set<String> removeTechsThisDomain = new HashSet<>(removeTechs);
+      Set<String> addNameserversThisDomain = new HashSet<>(addNameservers);
+      Set<String> removeNameserversThisDomain = new HashSet<>(removeNameservers);
+      Set<String> addStatusesThisDomain = new HashSet<>(addStatuses);
+      Set<String> removeStatusesThisDomain = new HashSet<>(removeStatuses);
+
       if (!nameservers.isEmpty() || !admins.isEmpty() || !techs.isEmpty() || !statuses.isEmpty()) {
         DateTime now = DateTime.now(UTC);
         DomainResource domainResource = loadByForeignKey(DomainResource.class, domain, now);
-        checkArgument(domainResource != null, "Domain '%s' does not exist", domain);
+        checkArgumentNotNull(domainResource, "Domain '%s' does not exist", domain);
         checkArgument(
             !domainResource.getStatusValues().contains(SERVER_UPDATE_PROHIBITED),
             "The domain '%s' has status SERVER_UPDATE_PROHIBITED. Verify that you are allowed "
@@ -170,10 +180,14 @@ final class UpdateDomainCommand extends CreateOrUpdateDomainCommand {
           populateAddRemoveLists(
               ImmutableSet.copyOf(nameservers),
               existingNameservers,
-              addNameservers,
+              addNameserversThisDomain,
-              removeNameservers);
+              removeNameserversThisDomain);
+          int numNameservers =
+              existingNameservers.size()
+                  + addNameserversThisDomain.size()
+                  - removeNameserversThisDomain.size();
           checkArgument(
-              existingNameservers.size() + addNameservers.size() - removeNameservers.size() <= 13,
+              numNameservers <= 13,
               "The resulting nameservers count for domain %s would be more than 13",
               domain);
         }
@@ -185,11 +199,17 @@ final class UpdateDomainCommand extends CreateOrUpdateDomainCommand {
 
           if (!admins.isEmpty()) {
             populateAddRemoveLists(
-                ImmutableSet.copyOf(admins), existingAdmins, addAdmins, removeAdmins);
+                ImmutableSet.copyOf(admins),
+                existingAdmins,
+                addAdminsThisDomain,
+                removeAdminsThisDomain);
           }
           if (!techs.isEmpty()) {
             populateAddRemoveLists(
-                ImmutableSet.copyOf(techs), existingTechs, addTechs, removeTechs);
+                ImmutableSet.copyOf(techs),
+                existingTechs,
+                addTechsThisDomain,
+                removeTechsThisDomain);
           }
         }
         if (!statuses.isEmpty()) {
@@ -198,21 +218,24 @@ final class UpdateDomainCommand extends CreateOrUpdateDomainCommand {
             currentStatusValues.add(statusValue.getXmlName());
           }
           populateAddRemoveLists(
-              ImmutableSet.copyOf(statuses), currentStatusValues, addStatuses, removeStatuses);
+              ImmutableSet.copyOf(statuses),
+              currentStatusValues,
+              addStatusesThisDomain,
+              removeStatusesThisDomain);
         }
       }
 
       boolean add =
-          !addNameservers.isEmpty()
+          !addNameserversThisDomain.isEmpty()
-              || !addAdmins.isEmpty()
+              || !addAdminsThisDomain.isEmpty()
-              || !addTechs.isEmpty()
+              || !addTechsThisDomain.isEmpty()
-              || !addStatuses.isEmpty();
+              || !addStatusesThisDomain.isEmpty();
 
       boolean remove =
-          !removeNameservers.isEmpty()
+          !removeNameserversThisDomain.isEmpty()
-              || !removeAdmins.isEmpty()
+              || !removeAdminsThisDomain.isEmpty()
-              || !removeTechs.isEmpty()
+              || !removeTechsThisDomain.isEmpty()
-              || !removeStatuses.isEmpty();
+              || !removeStatusesThisDomain.isEmpty();
 
       boolean change = registrant != null || password != null;
 
@@ -233,15 +256,15 @@ final class UpdateDomainCommand extends CreateOrUpdateDomainCommand {
           new SoyMapData(
               "domain", domain,
               "add", add,
-              "addNameservers", addNameservers,
+              "addNameservers", addNameserversThisDomain,
-              "addAdmins", addAdmins,
+              "addAdmins", addAdminsThisDomain,
-              "addTechs", addTechs,
+              "addTechs", addTechsThisDomain,
-              "addStatuses", addStatuses,
+              "addStatuses", addStatusesThisDomain,
               "remove", remove,
-              "removeNameservers", removeNameservers,
+              "removeNameservers", removeNameserversThisDomain,
-              "removeAdmins", removeAdmins,
+              "removeAdmins", removeAdminsThisDomain,
-              "removeTechs", removeTechs,
+              "removeTechs", removeTechsThisDomain,
-              "removeStatuses", removeStatuses,
+              "removeStatuses", removeStatusesThisDomain,
               "change", change,
               "registrant", registrant,
               "password", password,
@@ -253,9 +276,9 @@ final class UpdateDomainCommand extends CreateOrUpdateDomainCommand {
   }
 
   protected void populateAddRemoveLists(
-      Set<String> targetSet, Set<String> oldSet, List<String> addList, List<String> removeList) {
+      Set<String> targetSet, Set<String> oldSet, Set<String> addSet, Set<String> removeSet) {
-    addList.addAll(Sets.difference(targetSet, oldSet));
+    addSet.addAll(Sets.difference(targetSet, oldSet));
-    removeList.addAll(Sets.difference(oldSet, targetSet));
+    removeSet.addAll(Sets.difference(oldSet, targetSet));
   }
 
   ImmutableSet<String> getContactsOfType(

javatests/google/registry/tools/UpdateDomainCommandTest.java

@@ -16,6 +16,7 @@ package google.registry.tools;
 
 import static com.google.common.truth.Truth.assertThat;
 import static google.registry.model.eppcommon.StatusValue.SERVER_UPDATE_PROHIBITED;
+import static google.registry.testing.DatastoreHelper.createTlds;
 import static google.registry.testing.DatastoreHelper.newContactResource;
 import static google.registry.testing.DatastoreHelper.newDomainResource;
 import static google.registry.testing.DatastoreHelper.persistActiveHost;
@@ -23,6 +24,7 @@ import static google.registry.testing.DatastoreHelper.persistResource;
 import static google.registry.testing.JUnitBackports.assertThrows;
 
 import com.beust.jcommander.ParameterException;
+import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.googlecode.objectify.Key;
 import google.registry.model.contact.ContactResource;
@@ -77,6 +79,32 @@ public class UpdateDomainCommandTest extends EppToolCommandTestCase<UpdateDomain
         .verifySent("domain_update_complete_abc.xml");
   }
 
+  @Test
+  public void testSuccess_multipleDomains_setNameservers() throws Exception {
+    createTlds("abc", "tld");
+    HostResource host1 = persistActiveHost("foo.bar.tld");
+    HostResource host2 = persistActiveHost("baz.bar.tld");
+    persistResource(
+        newDomainResource("example.abc")
+            .asBuilder()
+            .setNameservers(ImmutableSet.of(Key.create(host1)))
+            .build());
+    persistResource(
+        newDomainResource("example.tld")
+            .asBuilder()
+            .setNameservers(ImmutableSet.of(Key.create(host2)))
+            .build());
+    runCommandForced(
+        "--client=NewRegistrar", "-n ns1.foo.fake,ns2.foo.fake", "example.abc", "example.tld");
+    eppVerifier
+        .verifySent(
+            "domain_update_add_two_hosts_remove_one.xml",
+            ImmutableMap.of("DOMAIN", "example.abc", "REMOVEHOST", "foo.bar.tld"))
+        .verifySent(
+            "domain_update_add_two_hosts_remove_one.xml",
+            ImmutableMap.of("DOMAIN", "example.tld", "REMOVEHOST", "baz.bar.tld"));
+  }
+
   @Test
   public void testSuccess_add() throws Exception {
     runCommandForced(

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

@@ -0,0 +1,22 @@
+<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
+<epp xmlns="urn:ietf:params:xml:ns:epp-1.0">
+  <command>
+    <update>
+      <domain:update xmlns:domain="urn:ietf:params:xml:ns:domain-1.0">
+        <domain:name>%DOMAIN%</domain:name>
+        <domain:add>
+          <domain:ns>
+            <domain:hostObj>ns1.foo.fake</domain:hostObj>
+            <domain:hostObj>ns2.foo.fake</domain:hostObj>
+          </domain:ns>
+        </domain:add>
+        <domain:rem>
+          <domain:ns>
+            <domain:hostObj>%REMOVEHOST%</domain:hostObj>
+          </domain:ns>
+        </domain:rem>
+      </domain:update>
+    </update>
+    <clTRID>RegistryTool</clTRID>
+  </command>
+</epp>