Make failfastForCreate for domain and application creates explicitly hit memcache

TESTED=For all tests, I added @Cache to DomainBase because otherwise the tests will
    fail. We aren't ready to do this in prod yet, which is why the tests are still
    marked @Ignore. The new tests fail if you change line 134 in Ofy to not use memcache
    and either use the unchanged original DomainCreateFlow code, or use the new
    inlined code and change loadWithMemcache() to load(). They pass with the new
    inlined code that calls loadWithMemcache(), as long as the @Cache is added to
    DomainResource.

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

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

@@ -22,7 +22,7 @@ import static com.google.common.collect.Sets.difference;
 import static com.google.common.collect.Sets.intersection;
 import static com.google.common.collect.Sets.union;
 import static google.registry.flows.domain.DomainPricingLogic.getMatchingLrpToken;
-import static google.registry.model.EppResourceUtils.loadByForeignKey;
+import static google.registry.model.EppResourceUtils.loadByForeignKeyWithMemcache;
 import static google.registry.model.domain.DomainResource.MAX_REGISTRATION_YEARS;
 import static google.registry.model.ofy.ObjectifyService.ofy;
 import static google.registry.model.registry.Registries.findTldForName;
@@ -800,8 +800,11 @@ public class DomainFlowUtils {
     DomainResource domain = ofy().doTransactionless(new Work<DomainResource>() {
       @Override
       public DomainResource run() {
-        // This is cacheable because we are outside of a transaction.
+        // We want to load the ForeignKeyIndex and DomainResource from memcache if possible so that
-        return loadByForeignKey(DomainResource.class, targetId, now);
+        // repeated create attempts of the same domain will not put load on datastore. This is safe
+        // because this is only a failfast method, and if memcache is stale the worst case scenario
+        // is that we will fall through to the regular transactional flow and fail there.
+        return loadByForeignKeyWithMemcache(DomainResource.class, targetId, now);
       }});
     // If the domain exists already and isn't in the add grace period then there is no way it will
     // be suddenly deleted and therefore the create must fail.

java/google/registry/model/EppResourceUtils.java

@@ -16,7 +16,6 @@ package google.registry.model;
 
 import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.common.collect.Iterables.transform;
-import static google.registry.model.index.ForeignKeyIndex.loadAndGetKey;
 import static google.registry.model.ofy.ObjectifyService.ofy;
 import static google.registry.util.DateTimeUtils.isAtOrAfter;
 import static google.registry.util.DateTimeUtils.isBeforeOrAt;
@@ -25,6 +24,7 @@ import static google.registry.util.DateTimeUtils.latestOf;
 import com.google.common.base.Function;
 import com.googlecode.objectify.Key;
 import com.googlecode.objectify.Result;
+import com.googlecode.objectify.cmd.Loader;
 import com.googlecode.objectify.cmd.Query;
 import com.googlecode.objectify.util.ResultNow;
 import google.registry.model.EppResource.Builder;
@@ -93,14 +93,41 @@ public final class EppResourceUtils {
   @Nullable
   public static <T extends EppResource> T loadByForeignKey(
       Class<T> clazz, String foreignKey, DateTime now) {
+    return loadByForeignKeyInternal(clazz, foreignKey, now, false);
+  }
+
+  /**
+   * Loads the last created version of an {@link EppResource} from Datastore or memcache.
+   *
+   * <p>In general, prefer {@link #loadByForeignKey} over this method. Loading from memcache can, in
+   * rare instances, produce a stale result (when a memcache write fails and the previous result is
+   * not cleared out) and so using memcache should be avoided unless the caller can tolerate
+   * staleness until the memcache expiration time and there is a specific need for very low latency
+   * that is worth the extra complexity of reasoning about caching.
+   *
+   * @param clazz the resource type to load
+   * @param foreignKey id to match
+   * @param now the current logical time to project resources at
+   */
+  @Nullable
+  public static <T extends EppResource> T loadByForeignKeyWithMemcache(
+      Class<T> clazz, String foreignKey, DateTime now) {
+    return loadByForeignKeyInternal(clazz, foreignKey, now, true);
+  }
+
+  @Nullable
+  private static <T extends EppResource> T loadByForeignKeyInternal(
+      Class<T> clazz, String foreignKey, DateTime now, boolean useMemcache) {
     checkArgument(
         ForeignKeyedEppResource.class.isAssignableFrom(clazz),
         "loadByForeignKey may only be called for foreign keyed EPP resources");
-    Key<T> resourceKey = loadAndGetKey(clazz, foreignKey, now);
+    Loader loader = useMemcache ? ofy().loadWithMemcache() : ofy().load();
-    if (resourceKey == null) {
+    ForeignKeyIndex<T> fki = loader.type(ForeignKeyIndex.mapToFkiClass(clazz)).id(foreignKey).now();
+    // The value of fki.getResourceKey() might be null for hard-deleted prober data.
+    if (fki == null || isAtOrAfter(now, fki.getDeletionTime()) || fki.getResourceKey() == null) {
       return null;
     }
-    T resource = ofy().load().key(resourceKey).now();
+    T resource = loader.key(fki.getResourceKey()).now();
     if (resource == null || isAtOrAfter(now, resource.getDeletionTime())) {
       return null;
     }

java/google/registry/model/index/ForeignKeyIndex.java

@@ -62,7 +62,7 @@ public abstract class ForeignKeyIndex<E extends EppResource> extends BackupGroup
   @Entity
   public static class ForeignKeyHostIndex extends ForeignKeyIndex<HostResource> {}
 
-  private static final ImmutableMap<
+  static final ImmutableMap<
           Class<? extends EppResource>, Class<? extends ForeignKeyIndex<?>>>
       RESOURCE_CLASS_TO_FKI_CLASS =
           ImmutableMap.<Class<? extends EppResource>, Class<? extends ForeignKeyIndex<?>>>of(
@@ -102,14 +102,19 @@ public abstract class ForeignKeyIndex<E extends EppResource> extends BackupGroup
     return topReference;
   }
 
-  /**
+
-   * Create a {@link ForeignKeyIndex} instance for a resource, expiring at a specified time.
+  @SuppressWarnings("unchecked")
-   */
+  public static <T extends EppResource> Class<ForeignKeyIndex<T>> mapToFkiClass(
+      Class<T> resourceClass) {
+    return (Class<ForeignKeyIndex<T>>) RESOURCE_CLASS_TO_FKI_CLASS.get(resourceClass);
+  }
+
+  /** Create a {@link ForeignKeyIndex} instance for a resource, expiring at a specified time. */
   public static <E extends EppResource> ForeignKeyIndex<E> create(
       E resource, DateTime deletionTime) {
     @SuppressWarnings("unchecked")
-    Class<? extends ForeignKeyIndex<E>> fkiClass =
+    Class<ForeignKeyIndex<E>> fkiClass =
-        (Class<? extends ForeignKeyIndex<E>>) RESOURCE_CLASS_TO_FKI_CLASS.get(resource.getClass());
+        ForeignKeyIndex.mapToFkiClass((Class<E>) resource.getClass());
     ForeignKeyIndex<E> instance = instantiate(fkiClass);
     instance.topReference = Key.create(resource);
     instance.foreignKey = resource.getForeignKey();
@@ -120,7 +125,7 @@ public abstract class ForeignKeyIndex<E extends EppResource> extends BackupGroup
   /** Create a {@link ForeignKeyIndex} key for a resource. */
   public static Key<ForeignKeyIndex<?>> createKey(EppResource resource) {
     return Key.<ForeignKeyIndex<?>>create(
-        RESOURCE_CLASS_TO_FKI_CLASS.get(resource.getClass()), resource.getForeignKey());
+        mapToFkiClass(resource.getClass()), resource.getForeignKey());
   }
 
   /**
@@ -162,11 +167,10 @@ public abstract class ForeignKeyIndex<E extends EppResource> extends BackupGroup
    * <p>The returned map will omit any keys for which the {@link ForeignKeyIndex} doesn't exist or
    * has been soft deleted.
    */
-  @SuppressWarnings("unchecked")
   public static <E extends EppResource> Map<String, ForeignKeyIndex<E>> load(
       Class<E> clazz, Iterable<String> foreignKeys, final DateTime now) {
-    return (Map<String, ForeignKeyIndex<E>>) filterValues(
+    return filterValues(
-        ofy().load().type(RESOURCE_CLASS_TO_FKI_CLASS.get(clazz)).ids(foreignKeys),
+        ofy().load().type(mapToFkiClass(clazz)).ids(foreignKeys),
         new Predicate<ForeignKeyIndex<?>>() {
           @Override
           public boolean apply(ForeignKeyIndex<?> fki) {

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

@@ -18,6 +18,7 @@ import static com.google.common.collect.Iterables.getLast;
 import static com.google.common.io.BaseEncoding.base16;
 import static com.google.common.truth.Truth.assertThat;
 import static com.google.common.truth.Truth.assertWithMessage;
+import static com.google.common.truth.Truth.assert_;
 import static google.registry.model.index.DomainApplicationIndex.loadActiveApplicationsByDomainName;
 import static google.registry.model.ofy.ObjectifyService.ofy;
 import static google.registry.testing.DatastoreHelper.assertNoBillingEvents;
@@ -32,6 +33,7 @@ import static google.registry.testing.DatastoreHelper.persistReservedList;
 import static google.registry.testing.DatastoreHelper.persistResource;
 import static google.registry.testing.DomainApplicationSubject.assertAboutApplications;
 import static google.registry.testing.EppExceptionSubject.assertAboutEppExceptions;
+import static google.registry.testing.MemcacheHelper.setMemcacheContents;
 import static google.registry.util.DateTimeUtils.END_OF_TIME;
 import static google.registry.util.DateTimeUtils.START_OF_TIME;
 import static org.joda.money.CurrencyUnit.EUR;
@@ -106,6 +108,7 @@ import google.registry.flows.domain.DomainFlowUtils.UnsupportedFeeAttributeExcep
 import google.registry.flows.domain.DomainFlowUtils.UnsupportedMarkTypeException;
 import google.registry.flows.exceptions.ResourceAlreadyExistsException;
 import google.registry.model.domain.DomainApplication;
+import google.registry.model.domain.DomainResource;
 import google.registry.model.domain.GracePeriod;
 import google.registry.model.domain.LrpTokenEntity;
 import google.registry.model.domain.launch.ApplicationStatus;
@@ -113,6 +116,8 @@ import google.registry.model.domain.launch.LaunchNotice;
 import google.registry.model.domain.launch.LaunchPhase;
 import google.registry.model.domain.rgp.GracePeriodStatus;
 import google.registry.model.domain.secdns.DelegationSignerData;
+import google.registry.model.index.ForeignKeyIndex;
+import google.registry.model.ofy.RequestCapturingAsyncDatastoreService;
 import google.registry.model.registrar.Registrar;
 import google.registry.model.registry.Registry;
 import google.registry.model.registry.Registry.TldState;
@@ -1516,21 +1521,48 @@ public class DomainApplicationCreateFlowTest
     runFlow();
   }
 
+
   @Test
-  public void testFailure_alreadyExists() throws Exception {
+  public void testFailure_alreadyExists_triggersFailfast() throws Exception {
+    // This fails fast and throws DomainAlreadyExistsException from init() as a special case.
     persistContactsAndHosts();
     clock.advanceOneMilli();
     persistActiveDomain(getUniqueIdFromCommand());
     try {
       runFlow();
-      assertWithMessage("Expected ResourceAlreadyExistsException to be thrown").fail();
+      assert_().fail(
+          "Expected to throw ResourceAlreadyExistsException with message "
+              + "Object with given ID (%s) already exists",
+          getUniqueIdFromCommand());
     } catch (ResourceAlreadyExistsException e) {
       assertThat(e.isFailfast()).isTrue();
-      assertAboutEppExceptions().that(e).marshalsToXml().and().hasMessage(
-          String.format("Object with given ID (%s) already exists", getUniqueIdFromCommand()));
     }
   }
 
+  @Test
+  public void testFailfast_withMemcachedDomainAndFki_hitsMemcache() throws Exception {
+    persistContactsAndHosts();
+    clock.advanceOneMilli();
+    DomainResource domain = persistActiveDomain(getUniqueIdFromCommand());
+    setMemcacheContents(Key.create(domain), ForeignKeyIndex.createKey(domain));
+    int numPreviousReads = RequestCapturingAsyncDatastoreService.getReads().size();
+    try {
+      runFlow();
+      assert_().fail("Expected to throw ResourceAlreadyExistsException");
+    } catch (ResourceAlreadyExistsException e) {
+      assertThat(e.isFailfast()).isTrue();
+    }
+    // Everything should have been loaded from memcache so nothing should hit datastore.
+    int numReadsInFlow =
+        RequestCapturingAsyncDatastoreService.getReads().size() - numPreviousReads;
+    // TODO(b/27424173): This is 1 because there is no @Cache annotation on DomainBase, and we
+    // don't want to blindly add it because that's a production change that adds potentially
+    // dangerous caching. When the recommendations from the audit in b/27424173 are done and we've
+    // tested the new safer caching this should be set to 0.
+    assertThat(numReadsInFlow).isEqualTo(1);
+  }
+
+
   @Test
   public void testFailure_registrantNotWhitelisted() throws Exception {
     persistActiveContact("someone");

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

@@ -16,6 +16,7 @@ package google.registry.flows.domain;
 
 import static com.google.common.io.BaseEncoding.base16;
 import static com.google.common.truth.Truth.assertThat;
+import static com.google.common.truth.Truth.assert_;
 import static google.registry.model.domain.fee.Fee.FEE_EXTENSION_URIS;
 import static google.registry.model.eppcommon.StatusValue.OK;
 import static google.registry.model.eppcommon.StatusValue.SERVER_TRANSFER_PROHIBITED;
@@ -39,6 +40,7 @@ import static google.registry.testing.DatastoreHelper.persistDeletedDomain;
 import static google.registry.testing.DatastoreHelper.persistReservedList;
 import static google.registry.testing.DatastoreHelper.persistResource;
 import static google.registry.testing.DomainResourceSubject.assertAboutDomains;
+import static google.registry.testing.MemcacheHelper.setMemcacheContents;
 import static google.registry.testing.TaskQueueHelper.assertDnsTasksEnqueued;
 import static google.registry.testing.TaskQueueHelper.assertNoDnsTasksEnqueued;
 import static google.registry.testing.TaskQueueHelper.assertNoTasksEnqueued;
@@ -55,6 +57,7 @@ import com.google.common.base.Strings;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.ImmutableSortedMap;
+import com.googlecode.objectify.Key;
 import google.registry.flows.EppException.UnimplementedExtensionException;
 import google.registry.flows.EppRequestSource;
 import google.registry.flows.ExtensionManager.UndeclaredServiceExtensionException;
@@ -120,6 +123,8 @@ import google.registry.model.domain.launch.LaunchNotice;
 import google.registry.model.domain.rgp.GracePeriodStatus;
 import google.registry.model.domain.secdns.DelegationSignerData;
 import google.registry.model.eppcommon.StatusValue;
+import google.registry.model.index.ForeignKeyIndex;
+import google.registry.model.ofy.RequestCapturingAsyncDatastoreService;
 import google.registry.model.poll.PollMessage;
 import google.registry.model.registrar.Registrar;
 import google.registry.model.registry.Registry;
@@ -890,21 +895,43 @@ public class DomainCreateFlowTest extends ResourceFlowTestCase<DomainCreateFlow,
   }
 
   @Test
-  public void testFailure_alreadyExists() throws Exception {
+  public void testFailure_alreadyExists_triggersFailfast() throws Exception {
     // This fails fast and throws DomainAlreadyExistsException from init() as a special case.
     persistContactsAndHosts();
     persistActiveDomain(getUniqueIdFromCommand());
-    thrown.expect(
-        ResourceAlreadyExistsException.class,
-        String.format("Object with given ID (%s) already exists", getUniqueIdFromCommand()));
     try {
       runFlow();
+      assert_().fail(
+          "Expected to throw ResourceAlreadyExistsException with message "
+              + "Object with given ID (%s) already exists",
+          getUniqueIdFromCommand());
     } catch (ResourceAlreadyExistsException e) {
       assertThat(e.isFailfast()).isTrue();
-      throw e;
     }
   }
 
+  @Test
+  public void testFailfast_withMemcachedDomainAndFki_hitsMemcache() throws Exception {
+    persistContactsAndHosts();
+    DomainResource domain = persistActiveDomain(getUniqueIdFromCommand());
+    setMemcacheContents(Key.create(domain), ForeignKeyIndex.createKey(domain));
+    int numPreviousReads = RequestCapturingAsyncDatastoreService.getReads().size();
+    try {
+      runFlow();
+      assert_().fail("Expected to throw ResourceAlreadyExistsException");
+    } catch (ResourceAlreadyExistsException e) {
+      assertThat(e.isFailfast()).isTrue();
+    }
+    // Everything should have been loaded from memcache so nothing should hit datastore.
+    int numReadsInFlow =
+        RequestCapturingAsyncDatastoreService.getReads().size() - numPreviousReads;
+    // TODO(b/27424173): This is 1 because there is no @Cache annotation on DomainBase, and we
+    // don't want to blindly add it because that's a production change that adds potentially
+    // dangerous caching. When the recommendations from the audit in b/27424173 are done and we've
+    // tested the new safer caching this should be set to 0.
+    assertThat(numReadsInFlow).isEqualTo(1);
+  }
+
   /**
    * There is special logic that disallows a failfast for domains in add grace period and sunrush
    * add grace period, so make sure that they fail anyways in the actual flow.