Add detailed log when DuplicateContactForRoleException is thrown

This change also added a test to verify that EPP request to modify
both contacts and registrant at same time can be handled as expected.

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

java/google/registry/flows/domain/DomainFlowUtils.java

@@ -18,6 +18,7 @@ import static com.google.common.base.Preconditions.checkNotNull;
 import static com.google.common.base.Preconditions.checkState;
 import static com.google.common.base.Predicates.equalTo;
 import static com.google.common.collect.ImmutableMap.toImmutableMap;
+import static com.google.common.collect.ImmutableSetMultimap.toImmutableSetMultimap;
 import static com.google.common.collect.Iterables.any;
 import static com.google.common.collect.Sets.difference;
 import static com.google.common.collect.Sets.intersection;
@@ -41,6 +42,7 @@ import static google.registry.util.DateTimeUtils.isAtOrAfter;
 import static google.registry.util.DateTimeUtils.leapSafeAddYears;
 import static google.registry.util.DomainNameUtils.ACE_PREFIX;
 
+import com.google.common.base.Ascii;
 import com.google.common.base.CharMatcher;
 import com.google.common.base.Joiner;
 import com.google.common.base.Splitter;
@@ -49,6 +51,7 @@ import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableMultimap;
 import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.ImmutableSortedSet;
 import com.google.common.collect.Sets;
 import com.google.common.collect.Streams;
 import com.google.common.net.InternetDomainName;
@@ -116,9 +119,11 @@ import google.registry.model.tmch.ClaimsListShard;
 import google.registry.tldconfig.idn.IdnLabelValidator;
 import google.registry.util.Idn;
 import java.math.BigDecimal;
+import java.util.Collection;
 import java.util.Comparator;
 import java.util.HashSet;
 import java.util.List;
+import java.util.Map;
 import java.util.Map.Entry;
 import java.util.Optional;
 import java.util.Set;
@@ -360,11 +365,20 @@ public class DomainFlowUtils {
 
   static void validateNoDuplicateContacts(Set<DesignatedContact> contacts)
       throws ParameterValuePolicyErrorException {
-    Set<Type> roles = new HashSet<>();
+    Map<Type, Collection<String>> roleToDupContacts =
-    for (DesignatedContact contact : nullToEmpty(contacts)) {
+        nullToEmpty(contacts).stream()
-      if (!roles.add(contact.getType())) {
+            .collect(
-        throw new DuplicateContactForRoleException();
+                toImmutableSetMultimap(
-      }
+                    contact -> contact.getType(),
+                    contact -> ofy().load().key(contact.getContactKey()).now().getContactId()))
+            .asMap()
+            .entrySet()
+            .stream()
+            .filter(entry -> entry.getValue().size() >= 2)
+            .collect(Collectors.toMap(entry -> entry.getKey(), entry -> entry.getValue()));
+
+    if (!roleToDupContacts.isEmpty()) {
+      throw new DuplicateContactForRoleException(roleToDupContacts);
     }
   }
 
@@ -1140,11 +1154,32 @@ public class DomainFlowUtils {
 
   /** More than one contact for a given role is not allowed. */
   static class DuplicateContactForRoleException extends ParameterValuePolicyErrorException {
-    public DuplicateContactForRoleException() {
+    public DuplicateContactForRoleException(Map<Type, Collection<String>> roleToDupContacts) {
-      super("More than one contact for a given role is not allowed");
+      super(constructErrorMessageFrom(roleToDupContacts));
     }
   }
 
+  private static String constructErrorMessageFrom(Map<Type, Collection<String>> roleToDupContacts) {
+    String detailError =
+        Joiner.on(", ")
+            .join(
+                sort(
+                    roleToDupContacts.entrySet().stream()
+                        .map(DomainFlowUtils::formatDetailMessage)
+                        .collect(Collectors.toSet())));
+    return String.format("More than one contact for a given role is not allowed: %s", detailError);
+  }
+
+  private static String formatDetailMessage(Map.Entry<Type, Collection<String>> entry) {
+    return String.format(
+        "contacts [%s] have same role [%s]",
+        Joiner.on(", ").join(sort(entry.getValue())), Ascii.toLowerCase(entry.getKey().toString()));
+  }
+
+  private static ImmutableSortedSet<Comparable<?>> sort(Collection<String> unsorted) {
+    return ImmutableSortedSet.naturalOrder().addAll(unsorted).build();
+  }
+
   /** Declared launch extension phase does not match the current registry phase. */
   static class LaunchPhaseMismatchException extends ParameterValuePolicyErrorException {
     public LaunchPhaseMismatchException() {

javatests/google/registry/flows/domain/DomainUpdateFlowTest.java

@@ -120,6 +120,31 @@ public class DomainUpdateFlowTest extends ResourceFlowTestCase<DomainUpdateFlow,
     unusedContact = persistActiveContact("unused");
   }
 
+  private DomainBase persistDomainWithRegistrant() throws Exception {
+    HostResource host =
+        loadByForeignKey(HostResource.class, "ns1.example.foo", clock.nowUtc()).get();
+    DomainBase domain =
+        persistResource(
+            newDomainBase(getUniqueIdFromCommand())
+                .asBuilder()
+                .setContacts(
+                    ImmutableSet.of(
+                        DesignatedContact.create(Type.TECH, Key.create(mak21Contact)),
+                        DesignatedContact.create(Type.ADMIN, Key.create(mak21Contact)),
+                        DesignatedContact.create(Type.BILLING, Key.create(mak21Contact))))
+                .setRegistrant(Key.create(mak21Contact))
+                .setNameservers(ImmutableSet.of(Key.create(host)))
+                .build());
+    historyEntryDomainCreate =
+        persistResource(
+            new HistoryEntry.Builder()
+                .setType(HistoryEntry.Type.DOMAIN_CREATE)
+                .setParent(domain)
+                .build());
+    clock.advanceOneMilli();
+    return domain;
+  }
+
   private DomainBase persistDomain() throws Exception {
     HostResource host =
         loadByForeignKey(HostResource.class, "ns1.example.foo", clock.nowUtc()).get();
@@ -758,6 +783,10 @@ public class DomainUpdateFlowTest extends ResourceFlowTestCase<DomainUpdateFlow,
             .build());
     EppException thrown = assertThrows(DuplicateContactForRoleException.class, this::runFlow);
     assertAboutEppExceptions().that(thrown).marshalsToXml();
+    assertThat(thrown.getResult().getMsg())
+        .isEqualTo(
+            "More than one contact for a given role is not allowed: "
+                + "contacts [foo, mak21] have same role [tech]");
   }
 
   @Test
@@ -868,6 +897,19 @@ public class DomainUpdateFlowTest extends ResourceFlowTestCase<DomainUpdateFlow,
     assertAboutEppExceptions().that(thrown).marshalsToXml();
   }
 
+  @Test
+  public void testFailure_multipleDuplicateContactInCommand() throws Exception {
+    setEppInput("domain_update_multiple_duplicate_contacts.xml");
+    persistReferencedEntities();
+    persistDomain();
+    EppException thrown = assertThrows(DuplicateContactForRoleException.class, this::runFlow);
+    assertThat(thrown.getMessage())
+        .isEqualTo("More than one contact for a given role is not allowed: "
+            + "contacts [mak21, sh8013] have same role [billing], "
+            + "contacts [mak21, sh8013] have same role [tech]");
+    assertAboutEppExceptions().that(thrown).marshalsToXml();
+  }
+
   @Test
   public void testFailure_missingContactType() throws Exception {
     // We need to test for missing type, but not for invalid - the schema enforces that for us.
@@ -1091,6 +1133,35 @@ public class DomainUpdateFlowTest extends ResourceFlowTestCase<DomainUpdateFlow,
         .isEqualTo("sh8013");
   }
 
+  @Test
+  public void testSuccess_changeContactsAndRegistrant() throws Exception {
+    setEppInput("domain_update_contacts_and_registrant.xml");
+    persistReferencedEntities();
+    persistDomainWithRegistrant();
+
+    reloadResourceByForeignKey()
+        .getContacts()
+        .forEach(
+            contact -> {
+              assertThat(ofy().load().key(contact.getContactKey()).now().getContactId())
+                  .isEqualTo("mak21");
+            });
+    assertThat(ofy().load().key(reloadResourceByForeignKey().getRegistrant()).now().getContactId())
+        .isEqualTo("mak21");
+
+    runFlow();
+
+    reloadResourceByForeignKey()
+        .getContacts()
+        .forEach(
+            contact -> {
+              assertThat(ofy().load().key(contact.getContactKey()).now().getContactId())
+                  .isEqualTo("sh8013");
+            });
+    assertThat(ofy().load().key(reloadResourceByForeignKey().getRegistrant()).now().getContactId())
+        .isEqualTo("sh8013");
+  }
+
   @Test
   public void testSuccess_nameserverAndRegistrantWhitelisted() throws Exception {
     persistReferencedEntities();

javatests/google/registry/flows/domain/testdata/domain_update_contacts_and_registrant.xml

@@ -0,0 +1,24 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<epp xmlns="urn:ietf:params:xml:ns:epp-1.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="urn:ietf:params:xml:ns:epp-1.0 epp-1.0.xsd">
+  <command>
+    <update>
+      <domain:update xmlns:domain="urn:ietf:params:xml:ns:domain-1.0" xsi:schemaLocation="urn:ietf:params:xml:ns:domain-1.0 domain-1.0.xsd">
+        <domain:name>example.tld</domain:name>
+        <domain:add>
+          <domain:contact type="admin">sh8013</domain:contact>
+          <domain:contact type="tech">sh8013</domain:contact>
+          <domain:contact type="billing">sh8013</domain:contact>
+        </domain:add>
+        <domain:rem>
+          <domain:contact type="admin">mak21</domain:contact>
+          <domain:contact type="tech">mak21</domain:contact>
+          <domain:contact type="billing">mak21</domain:contact>
+        </domain:rem>
+        <domain:chg>
+          <domain:registrant>sh8013</domain:registrant>
+        </domain:chg>
+      </domain:update>
+    </update>
+    <clTRID>ABC-12345</clTRID>
+  </command>
+</epp>

javatests/google/registry/flows/domain/testdata/domain_update_multiple_duplicate_contacts.xml

@@ -0,0 +1,34 @@
+<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>example.tld</domain:name>
+        <domain:add>
+          <domain:ns>
+            <domain:hostObj>ns2.example.foo</domain:hostObj>
+          </domain:ns>
+          <domain:contact type="tech">mak21</domain:contact>
+          <domain:contact type="tech">sh8013</domain:contact>
+          <domain:contact type="billing">mak21</domain:contact>
+          <domain:contact type="billing">sh8013</domain:contact>
+          <domain:status s="clientHold"
+           lang="en">Payment overdue.</domain:status>
+        </domain:add>
+        <domain:rem>
+          <domain:ns>
+            <domain:hostObj>ns1.example.foo</domain:hostObj>
+          </domain:ns>
+          <domain:status s="clientUpdateProhibited"/>
+        </domain:rem>
+        <domain:chg>
+          <domain:registrant>sh8013</domain:registrant>
+          <domain:authInfo>
+            <domain:pw>2BARfoo</domain:pw>
+          </domain:authInfo>
+        </domain:chg>
+      </domain:update>
+    </update>
+    <clTRID>ABC-12345</clTRID>
+  </command>
+</epp>