From cb80a0df1ee6cc913038cb24167b1d96cba3b1b5 Mon Sep 17 00:00:00 2001
From: mcilwain <mcilwain@google.com>
Date: Thu, 22 Feb 2018 12:25:09 -0800
Subject: [PATCH] Batch contact/host loads when checking pending delete

I'm actually surprised that we had this in our code, as it seems like a huge
oversight, but we were individually loading each and every referenced contact
and host during domain/application create/update/allocate flows. Loading them
all as a single batch should reduce round trips to Datastore by a good deal,
thus improving performance.

We aren't giving up any transactional consistency in doing so and the only
potential downside I can think of is that we're always loading all contacts/
hosts instead of only some of them, in the rare case that one of the earlier
contacts/hosts is actually in pending delete (which allowed short-circuiting).
However, the gains from only making one round-trip should swamp the potential
losses in occasionally loading more data than is strictly necessary.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=186656118
---
 .../flows/domain/DomainFlowUtils.java         | 30 +++++++++++--------
 1 file changed, 17 insertions(+), 13 deletions(-)

diff --git a/java/google/registry/flows/domain/DomainFlowUtils.java b/java/google/registry/flows/domain/DomainFlowUtils.java
index bb166424a..3b18df8a2 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.Iterables.any;
 import static com.google.common.collect.Sets.difference;
@@ -270,22 +271,25 @@ public class DomainFlowUtils {
       Key<ContactResource> registrant,
       Set<Key<HostResource>> nameservers)
       throws EppException {
-    for (DesignatedContact contact : nullToEmpty(contacts)) {
-      verifyNotInPendingDelete(contact.getContactKey());
-    }
-    if (registrant != null) {
-      verifyNotInPendingDelete(registrant);
-    }
-    for (Key<HostResource> host : nullToEmpty(nameservers)) {
-      verifyNotInPendingDelete(host);
-    }
+    ImmutableList.Builder<Key<? extends EppResource>> keysToLoad = new ImmutableList.Builder<>();
+    nullToEmpty(contacts).stream().map(DesignatedContact::getContactKey).forEach(keysToLoad::add);
+    Optional.ofNullable(registrant).ifPresent(keysToLoad::add);
+    keysToLoad.addAll(nullToEmpty(nameservers));
+    // This cast is safe because, in Objectify, Key<? extends EppResource> can also be
+    // treated as a Key<EppResource>.
+    @SuppressWarnings("unchecked")
+    ImmutableList<Key<EppResource>> typedKeys =
+        keysToLoad.build().stream().map(key -> (Key<EppResource>) key).collect(toImmutableList());
+    verifyNotInPendingDelete(ofy().load().keys(typedKeys).values());
   }
 
-  private static void verifyNotInPendingDelete(Key<? extends EppResource> resourceKey)
+  private static void verifyNotInPendingDelete(Iterable<EppResource> resources)
       throws EppException {
-    EppResource resource = ofy().load().key(resourceKey).now();
-    if (resource.getStatusValues().contains(StatusValue.PENDING_DELETE)) {
-      throw new LinkedResourceInPendingDeleteProhibitsOperationException(resource.getForeignKey());
+    for (EppResource resource : resources) {
+      if (resource.getStatusValues().contains(StatusValue.PENDING_DELETE)) {
+        throw new LinkedResourceInPendingDeleteProhibitsOperationException(
+            resource.getForeignKey());
+      }
     }
   }