diff --git a/java/google/registry/flows/domain/DomainFlowUtils.java b/java/google/registry/flows/domain/DomainFlowUtils.java index c07f04b5b..e93bff6d5 100644 --- a/java/google/registry/flows/domain/DomainFlowUtils.java +++ b/java/google/registry/flows/domain/DomainFlowUtils.java @@ -17,6 +17,7 @@ package google.registry.flows.domain; 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.ImmutableList.toImmutableList; import static com.google.common.collect.ImmutableMap.toImmutableMap; import static com.google.common.collect.ImmutableSetMultimap.toImmutableSetMultimap; import static com.google.common.collect.Iterables.any; @@ -41,6 +42,8 @@ import static google.registry.util.DateTimeUtils.END_OF_TIME; import static google.registry.util.DateTimeUtils.isAtOrAfter; import static google.registry.util.DateTimeUtils.leapSafeAddYears; import static google.registry.util.DomainNameUtils.ACE_PREFIX; +import static java.util.Comparator.comparing; +import static java.util.stream.Collectors.joining; import com.google.common.base.Ascii; import com.google.common.base.CharMatcher; @@ -51,7 +54,9 @@ 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.Maps; +import com.google.common.collect.Multimap; +import com.google.common.collect.Multimaps; import com.google.common.collect.Sets; import com.google.common.collect.Streams; import com.google.common.net.InternetDomainName; @@ -127,7 +132,6 @@ import java.util.Map; import java.util.Map.Entry; import java.util.Optional; import java.util.Set; -import java.util.stream.Collectors; import javax.annotation.Nullable; import org.joda.money.CurrencyUnit; import org.joda.money.Money; @@ -317,9 +321,9 @@ public class DomainFlowUtils { Set> nameservers) throws EppException { ImmutableList.Builder> keysToLoad = new ImmutableList.Builder<>(); - nullToEmpty(contacts).stream().map(DesignatedContact::getContactKey).forEach(keysToLoad::add); + contacts.stream().map(DesignatedContact::getContactKey).forEach(keysToLoad::add); Optional.ofNullable(registrant).ifPresent(keysToLoad::add); - keysToLoad.addAll(nullToEmpty(nameservers)); + keysToLoad.addAll(nameservers); verifyNotInPendingDelete(EppResource.loadCached(keysToLoad.build()).values()); } @@ -335,7 +339,7 @@ public class DomainFlowUtils { static void validateContactsHaveTypes(Set contacts) throws ParameterValuePolicyErrorException { - for (DesignatedContact contact : nullToEmpty(contacts)) { + for (DesignatedContact contact : contacts) { if (contact.getType() == null) { throw new MissingContactTypeException(); } @@ -365,32 +369,39 @@ public class DomainFlowUtils { static void validateNoDuplicateContacts(Set contacts) throws ParameterValuePolicyErrorException { - Map> roleToDupContacts = - nullToEmpty(contacts).stream() + ImmutableMultimap> contactsByType = + contacts.stream() .collect( 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())); + DesignatedContact::getType, DesignatedContact::getContactKey)); - if (!roleToDupContacts.isEmpty()) { - throw new DuplicateContactForRoleException(roleToDupContacts); + // If any contact type has multiple contacts: + if (contactsByType.asMap().values().stream().anyMatch(v -> v.size() > 1)) { + // Find the duplicates. + Map>> dupeKeysMap = + Maps.filterEntries(contactsByType.asMap(), e -> e.getValue().size() > 1); + ImmutableList> dupeKeys = + dupeKeysMap.values().stream().flatMap(Collection::stream).collect(toImmutableList()); + // Load the duplicates in one batch. + Map, ContactResource> dupeContacts = ofy().load().keys(dupeKeys); + ImmutableMultimap.Builder> typesMap = + new ImmutableMultimap.Builder<>(); + dupeKeysMap.forEach(typesMap::putAll); + // Create an error message showing the type and contact IDs of the duplicates. + throw new DuplicateContactForRoleException( + Multimaps.transformValues(typesMap.build(), key -> dupeContacts.get(key).getContactId())); } } static void validateRequiredContactsPresent( - Key registrant, Set contacts) + @Nullable Key registrant, Set contacts) throws RequiredParameterMissingException { if (registrant == null) { throw new MissingRegistrantException(); } Set roles = new HashSet<>(); - for (DesignatedContact contact : nullToEmpty(contacts)) { + for (DesignatedContact contact : contacts) { roles.add(contact.getType()); } if (!roles.contains(Type.ADMIN)) { @@ -912,8 +923,7 @@ public class DomainFlowUtils { validateRegistrantAllowedOnTld(tld, command.getRegistrantContactId()); validateNoDuplicateContacts(command.getContacts()); validateRequiredContactsPresent(command.getRegistrant(), command.getContacts()); - Set fullyQualifiedHostNames = - nullToEmpty(command.getNameserverFullyQualifiedHostNames()); + ImmutableSet fullyQualifiedHostNames = command.getNameserverFullyQualifiedHostNames(); validateNameserversCountForTld(tld, domainName, fullyQualifiedHostNames.size()); validateNameserversAllowedOnTld(tld, fullyQualifiedHostNames); validateNameserversAllowedOnDomain(domainName, fullyQualifiedHostNames); @@ -1154,32 +1164,23 @@ public class DomainFlowUtils { /** More than one contact for a given role is not allowed. */ static class DuplicateContactForRoleException extends ParameterValuePolicyErrorException { - public DuplicateContactForRoleException(Map> roleToDupContacts) { - super(constructErrorMessageFrom(roleToDupContacts)); + + public DuplicateContactForRoleException(Multimap dupeContactsByType) { + super( + String.format( + "More than one contact for a given role is not allowed: %s", + dupeContactsByType.asMap().entrySet().stream() + .sorted(comparing(e -> e.getKey().name())) + .map( + e -> + String.format( + "role [%s] has contacts [%s]", + Ascii.toLowerCase(e.getKey().name()), + e.getValue().stream().sorted().collect(joining(", ")))) + .collect(joining(", ")))); } } - private static String constructErrorMessageFrom(Map> 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> entry) { - return String.format( - "contacts [%s] have same role [%s]", - Joiner.on(", ").join(sort(entry.getValue())), Ascii.toLowerCase(entry.getKey().toString())); - } - - private static ImmutableSortedSet> sort(Collection 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() { @@ -1418,7 +1419,7 @@ public class DomainFlowUtils { String.format( "The fee description \"%s\" passed in the transform matches multiple fee types: %s", description, - types.stream().map(FeeType::toString).collect(Collectors.joining(", ")))); + types.stream().map(FeeType::toString).collect(joining(", ")))); } } diff --git a/java/google/registry/model/domain/DomainCommand.java b/java/google/registry/model/domain/DomainCommand.java index d8c824b61..49ec1b2f3 100644 --- a/java/google/registry/model/domain/DomainCommand.java +++ b/java/google/registry/model/domain/DomainCommand.java @@ -43,6 +43,7 @@ import google.registry.model.host.HostResource; import google.registry.model.index.ForeignKeyIndex; import java.util.Map; import java.util.Set; +import javax.annotation.Nullable; import javax.xml.bind.annotation.XmlAttribute; import javax.xml.bind.annotation.XmlElement; import javax.xml.bind.annotation.XmlElementWrapper; @@ -90,6 +91,7 @@ public class DomainCommand { return registrantContactId; } + @Nullable public Key getRegistrant() { return registrant; } @@ -153,15 +155,15 @@ public class DomainCommand { } public ImmutableSet getNameserverFullyQualifiedHostNames() { - return nullSafeImmutableCopy(nameserverFullyQualifiedHostNames); + return nullToEmptyImmutableCopy(nameserverFullyQualifiedHostNames); } public ImmutableSet> getNameservers() { - return nullSafeImmutableCopy(nameservers); + return nullToEmptyImmutableCopy(nameservers); } public ImmutableSet getContacts() { - return nullSafeImmutableCopy(contacts); + return nullToEmptyImmutableCopy(contacts); } @Override diff --git a/javatests/google/registry/flows/domain/DomainUpdateFlowTest.java b/javatests/google/registry/flows/domain/DomainUpdateFlowTest.java index 511d94207..7b1a8b382 100644 --- a/javatests/google/registry/flows/domain/DomainUpdateFlowTest.java +++ b/javatests/google/registry/flows/domain/DomainUpdateFlowTest.java @@ -786,7 +786,7 @@ public class DomainUpdateFlowTest extends ResourceFlowTestCase