diff --git a/java/google/registry/flows/domain/DomainFlowUtils.java b/java/google/registry/flows/domain/DomainFlowUtils.java index 7aff94af9..aa5103c4f 100644 --- a/java/google/registry/flows/domain/DomainFlowUtils.java +++ b/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() { @Override public DomainResource run() { - // This is cacheable because we are outside of a transaction. - return loadByForeignKey(DomainResource.class, targetId, now); + // We want to load the ForeignKeyIndex and DomainResource from memcache if possible so that + // 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. diff --git a/java/google/registry/model/EppResourceUtils.java b/java/google/registry/model/EppResourceUtils.java index 89045bcf5..2868f1052 100644 --- a/java/google/registry/model/EppResourceUtils.java +++ b/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 loadByForeignKey( Class clazz, String foreignKey, DateTime now) { + return loadByForeignKeyInternal(clazz, foreignKey, now, false); + } + + /** + * Loads the last created version of an {@link EppResource} from Datastore or memcache. + * + *

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 loadByForeignKeyWithMemcache( + Class clazz, String foreignKey, DateTime now) { + return loadByForeignKeyInternal(clazz, foreignKey, now, true); + } + + @Nullable + private static T loadByForeignKeyInternal( + Class clazz, String foreignKey, DateTime now, boolean useMemcache) { checkArgument( ForeignKeyedEppResource.class.isAssignableFrom(clazz), "loadByForeignKey may only be called for foreign keyed EPP resources"); - Key resourceKey = loadAndGetKey(clazz, foreignKey, now); - if (resourceKey == null) { + Loader loader = useMemcache ? ofy().loadWithMemcache() : ofy().load(); + ForeignKeyIndex 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; } diff --git a/java/google/registry/model/index/ForeignKeyIndex.java b/java/google/registry/model/index/ForeignKeyIndex.java index 231f71464..1b54ecce2 100644 --- a/java/google/registry/model/index/ForeignKeyIndex.java +++ b/java/google/registry/model/index/ForeignKeyIndex.java @@ -62,7 +62,7 @@ public abstract class ForeignKeyIndex extends BackupGroup @Entity public static class ForeignKeyHostIndex extends ForeignKeyIndex {} - private static final ImmutableMap< + static final ImmutableMap< Class, Class>> RESOURCE_CLASS_TO_FKI_CLASS = ImmutableMap., Class>>of( @@ -102,14 +102,19 @@ public abstract class ForeignKeyIndex extends BackupGroup return topReference; } - /** - * Create a {@link ForeignKeyIndex} instance for a resource, expiring at a specified time. - */ + + @SuppressWarnings("unchecked") + public static Class> mapToFkiClass( + Class resourceClass) { + return (Class>) RESOURCE_CLASS_TO_FKI_CLASS.get(resourceClass); + } + + /** Create a {@link ForeignKeyIndex} instance for a resource, expiring at a specified time. */ public static ForeignKeyIndex create( E resource, DateTime deletionTime) { @SuppressWarnings("unchecked") - Class> fkiClass = - (Class>) RESOURCE_CLASS_TO_FKI_CLASS.get(resource.getClass()); + Class> fkiClass = + ForeignKeyIndex.mapToFkiClass((Class) resource.getClass()); ForeignKeyIndex instance = instantiate(fkiClass); instance.topReference = Key.create(resource); instance.foreignKey = resource.getForeignKey(); @@ -120,7 +125,7 @@ public abstract class ForeignKeyIndex extends BackupGroup /** Create a {@link ForeignKeyIndex} key for a resource. */ public static Key> createKey(EppResource resource) { return Key.>create( - RESOURCE_CLASS_TO_FKI_CLASS.get(resource.getClass()), resource.getForeignKey()); + mapToFkiClass(resource.getClass()), resource.getForeignKey()); } /** @@ -162,11 +167,10 @@ public abstract class ForeignKeyIndex extends BackupGroup *

The returned map will omit any keys for which the {@link ForeignKeyIndex} doesn't exist or * has been soft deleted. */ - @SuppressWarnings("unchecked") public static Map> load( Class clazz, Iterable foreignKeys, final DateTime now) { - return (Map>) filterValues( - ofy().load().type(RESOURCE_CLASS_TO_FKI_CLASS.get(clazz)).ids(foreignKeys), + return filterValues( + ofy().load().type(mapToFkiClass(clazz)).ids(foreignKeys), new Predicate>() { @Override public boolean apply(ForeignKeyIndex fki) { diff --git a/javatests/google/registry/flows/domain/DomainApplicationCreateFlowTest.java b/javatests/google/registry/flows/domain/DomainApplicationCreateFlowTest.java index 7e27bc466..182d112b9 100644 --- a/javatests/google/registry/flows/domain/DomainApplicationCreateFlowTest.java +++ b/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"); diff --git a/javatests/google/registry/flows/domain/DomainCreateFlowTest.java b/javatests/google/registry/flows/domain/DomainCreateFlowTest.java index 5f86fa64a..fc5bf2e82 100644 --- a/javatests/google/registry/flows/domain/DomainCreateFlowTest.java +++ b/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