Change loadActiveApplicationsByDomainName to non-transactional

We can easily end up enlisting too many entity groups (separate
DomainApplications) in a TransactionalFlow when loading all applications
tracked by the DomainApplicationIndex. This makes the load operation
transactionless, to avoid overenlisting.

Potential problems:
1. We fail to prevent landrush applications, if a single sunrise application
exists. This is likely fine, except for a brief moment in Sunrush when a
sunrise application is made immediately prior to a landrush application. The
result is we accept an invalid application- which can be mediated manually.

2. We fail to prevent a domain create for a domain with an open application.
This is a little more sinister, but also unlikely unless someone submits an
application immediately before someone tries to create the same domain (sans
application?)

3. We return an invalid DomainCheck response (instead of 'pending allocation').
Not the worst outcome.

4. We reduce the AuctionStatusCommand and GetApplicationIdsCommand to
eventual consistency. Since they're internal tools, that's not too big a deal.

A better solution:
DomainApplications really should just be normalized under a virtualEntityGroup
by fullyQualifiedDomainName, or a hash-bucket like EppResources are. The
DomainApplication -> DomainBase -> EppResource hierarchy seems to be purely for
code reuse, at the cost of Datastore consistency. This would, however, require
quite some refactoring, and a custom resave operation across all
DomainApplications.

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

java/google/registry/model/index/DomainApplicationIndex.java

@@ -21,6 +21,7 @@ import static google.registry.util.CollectionUtils.isNullOrEmpty;
 
 import com.google.common.collect.ImmutableSet;
 import com.googlecode.objectify.Key;
+import com.googlecode.objectify.Work;
 import com.googlecode.objectify.annotation.Entity;
 import com.googlecode.objectify.annotation.Id;
 import google.registry.model.BackupGroupRoot;
@@ -32,9 +33,10 @@ import javax.annotation.Nullable;
 import org.joda.time.DateTime;
 
 /**
- * Entity for tracking all domain applications with a given fully qualified domain name. Since this
+ * Entity for tracking all domain applications with a given fully qualified domain name.
- * resource is always kept up to date as additional domain applications are created, it is never
+ *
- * necessary to query them explicitly from Datastore.
+ * <p>Since this resource is always kept up to date as additional domain applications are created,
+ * it is never necessary to query them explicitly from Datastore.
  */
 @ReportedOn
 @Entity
@@ -60,12 +62,12 @@ public class DomainApplicationIndex extends BackupGroupRoot {
   }
 
   /**
-   * Creates a DomainApplicationIndex with the specified list of keys. Only use this method for data
+   * Creates a DomainApplicationIndex with the specified list of keys.
-   * migrations. You probably want {@link #createUpdatedInstance}.
+   *
+   * <p>Only use this method for data migrations. You probably want {@link #createUpdatedInstance}.
    */
   public static DomainApplicationIndex createWithSpecifiedKeys(
-      String fullyQualifiedDomainName,
+      String fullyQualifiedDomainName, ImmutableSet<Key<DomainApplication>> keys) {
-      ImmutableSet<Key<DomainApplication>> keys) {
     checkArgument(!isNullOrEmpty(fullyQualifiedDomainName),
         "fullyQualifiedDomainName must not be null or empty.");
     checkArgument(!isNullOrEmpty(keys), "Keys must not be null or empty.");
@@ -80,42 +82,54 @@ public class DomainApplicationIndex extends BackupGroupRoot {
   }
 
   /**
-   * Returns the set of all DomainApplications for the given fully qualified domain name that do
+   * Returns the set of all active DomainApplications for the given fully qualified domain name.
-   * not have a deletion time before the supplied DateTime.
+   *
+   * <p>Note that loading the individual applications referenced by the keys are explicitly
+   * non-transactional. This is to avoid potentially over-enlisting multiple entity groups within a
+   * transaction.
+   *
+   * <p>Consequently within a transaction this method will not return any applications that are not
+   * yet committed to datastore, even if called on an updated DomainApplicationIndex instance
+   * storing keys to those applications.
+   *
    */
   public static ImmutableSet<DomainApplication> loadActiveApplicationsByDomainName(
-      String fullyQualifiedDomainName, DateTime now) {
+      String fullyQualifiedDomainName, final DateTime now) {
-    DomainApplicationIndex index = load(fullyQualifiedDomainName);
+    final DomainApplicationIndex index = load(fullyQualifiedDomainName);
     if (index == null) {
       return ImmutableSet.of();
     }
-    ImmutableSet.Builder<DomainApplication> apps = new ImmutableSet.Builder<>();
+    // Perform eventually consistent query, to avoid overenlisting cross entity groups
-    for (DomainApplication app : ofy().load().keys(index.getKeys()).values()) {
+    return ofy().doTransactionless(new Work<ImmutableSet<DomainApplication>>() {
-      if (app.getDeletionTime().isAfter(now)) {
+        @Override
-        apps.add(app);
+        public ImmutableSet<DomainApplication> run() {
-      }
+          ImmutableSet.Builder<DomainApplication> apps = new ImmutableSet.Builder<>();
-    }
+          for (DomainApplication app : ofy().load().keys(index.getKeys()).values()) {
-    return apps.build();
+            if (app.getDeletionTime().isAfter(now)) {
+              apps.add(app);
+            }
+          }
+          return apps.build();
+        }});
   }
 
   /**
-   * Returns the DomainApplicationIndex for the given fully qualified domain name. Note that this
+   * Returns the DomainApplicationIndex for the given fully qualified domain name.
-   * can return null if there are no domain applications for this fully qualified domain name.
+   *
+   * <p>Note that this can return null if there are no domain applications for this fully qualified
+   * domain name.
    */
   @Nullable
   public static DomainApplicationIndex load(String fullyQualifiedDomainName) {
-    return ofy()
+    return ofy().load().type(DomainApplicationIndex.class).id(fullyQualifiedDomainName).now();
-        .load()
-        .type(DomainApplicationIndex.class)
-        .id(fullyQualifiedDomainName)
-        .now();
   }
 
   /**
-   * Saves a new DomainApplicationIndex for this resource or updates the existing one. This is
+   * Saves a new DomainApplicationIndex for this resource or updates the existing one.
-   * the preferred method for creating an instance of DomainApplicationIndex because this performs
+   *
-   * the correct merging logic to add the given domain application to an existing index if there
+   * <p>This is the preferred method for creating an instance of DomainApplicationIndex because this
-   * is one.
+   * performs the correct merging logic to add the given domain application to an existing index if
+   * there is one.
    */
   public static DomainApplicationIndex createUpdatedInstance(DomainApplication application) {
     DomainApplicationIndex existing = load(application.getFullyQualifiedDomainName());

javatests/google/registry/model/index/DomainApplicationIndexTest.java

@@ -18,14 +18,17 @@ import static com.google.common.truth.Truth.assertThat;
 import static google.registry.model.index.DomainApplicationIndex.createUpdatedInstance;
 import static google.registry.model.index.DomainApplicationIndex.createWithSpecifiedKeys;
 import static google.registry.model.index.DomainApplicationIndex.loadActiveApplicationsByDomainName;
+import static google.registry.model.ofy.ObjectifyService.ofy;
 import static google.registry.testing.DatastoreHelper.createTld;
 import static google.registry.testing.DatastoreHelper.newDomainApplication;
 import static google.registry.testing.DatastoreHelper.persistResource;
 import static google.registry.testing.DatastoreHelper.persistSimpleResource;
 import static org.joda.time.DateTimeZone.UTC;
 
+import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
 import com.googlecode.objectify.Key;
+import com.googlecode.objectify.VoidWork;
 import google.registry.model.EntityTestCase;
 import google.registry.model.domain.DomainApplication;
 import google.registry.testing.ExceptionRule;
@@ -118,4 +121,22 @@ public class DomainApplicationIndexTest extends EntityTestCase {
     assertThat(loadActiveApplicationsByDomainName("example.com", DateTime.now(UTC)))
         .containsExactly(application1);
   }
+
+  /** Ensure loading over 25 applications still succeeds (despite being in a transaction.) */
+  @Test
+  public void testSuccess_overCrossTransactionLimit() {
+    ImmutableList.Builder<DomainApplication> applicationsBuilder = new ImmutableList.Builder<>();
+    for (int i = 0; i < 30; i++) {
+      DomainApplication application = persistSimpleResource(newDomainApplication("example.com"));
+      persistResource(createUpdatedInstance(application));
+      applicationsBuilder.add(application);
+    }
+    ofy().transact(new VoidWork() {
+      @Override
+      public void vrun() {
+        assertThat(DomainApplicationIndex.load("example.com")).isNotNull();
+        assertThat(loadActiveApplicationsByDomainName("example.com", clock.nowUtc()))
+            .containsExactlyElementsIn(applicationsBuilder.build());
+      }});
+  }
 }