From d9349be18e5fcc3a70c18683fa99121192f672ca Mon Sep 17 00:00:00 2001 From: gbrodman Date: Wed, 29 Oct 2025 15:21:27 -0400 Subject: [PATCH] Modify the way we load resources via foreign keys (#2852) Previously, we would have separate database calls for mapping from foreign key to repo ID and then from repo ID to object. This PR modifies those calls to load the resource directly (the old system was an artifact of the Datastore key-value storage system). In this PR, we merge the load-resource-by-foreign-key calls into a single database load, as well as adding a separate cache object for (foreign key) -> (resource). Now we cache, and have separate cleaner code paths, for fk -> resource, fk -> repo ID, and repo ID -> resource. Also removes the unused RdeFragmenter class --- .../registry/bsa/BsaValidateAction.java | 14 +- .../bsa/persistence/DomainsRefresher.java | 2 +- .../bsa/persistence/LabelDiffUpdates.java | 5 +- .../google/registry/flows/CheckApiAction.java | 4 +- .../registry/flows/ResourceFlowUtils.java | 14 +- .../flows/contact/ContactCheckFlow.java | 4 +- .../flows/domain/DomainCheckFlow.java | 2 +- .../registry/flows/domain/DomainInfoFlow.java | 7 +- .../registry/flows/host/HostCheckFlow.java | 5 +- .../registry/flows/host/HostUpdateFlow.java | 2 +- .../registry/model/EppResourceUtils.java | 120 +------- .../registry/model/ForeignKeyUtils.java | 291 +++++++++++++----- .../registry/model/domain/DomainCommand.java | 2 +- .../registry/rdap/RdapDomainAction.java | 4 +- .../registry/rdap/RdapDomainSearchAction.java | 45 +-- .../registry/rdap/RdapNameserverAction.java | 4 +- .../rdap/RdapNameserverSearchAction.java | 10 +- .../google/registry/rde/RdeFragmenter.java | 103 ------- .../registry/tools/CommandUtilities.java | 5 +- .../registry/tools/DomainLockUtils.java | 4 +- .../tools/EnqueuePollMessageCommand.java | 11 +- .../tools/GetHistoryEntriesCommand.java | 2 - .../registry/tools/RenewDomainCommand.java | 12 +- .../tools/UniformRapidSuspensionCommand.java | 14 +- .../registry/tools/UnrenewDomainCommand.java | 2 +- .../registry/tools/UpdateDomainCommand.java | 11 +- .../console/ConsoleDomainGetAction.java | 4 +- .../registry/bsa/BsaValidateActionTest.java | 4 +- .../flows/domain/DomainDeleteFlowTest.java | 6 +- .../flows/host/HostUpdateFlowTest.java | 4 +- .../registry/model/ForeignKeyUtilsTest.java | 54 ++-- .../registry/testing/TestCacheExtension.java | 9 +- .../registry/tmch/NordnUploadActionTest.java | 33 +- .../tools/EnqueuePollMessageCommandTest.java | 7 +- .../tools/RenewDomainCommandTest.java | 19 +- 35 files changed, 373 insertions(+), 466 deletions(-) delete mode 100644 core/src/main/java/google/registry/rde/RdeFragmenter.java diff --git a/core/src/main/java/google/registry/bsa/BsaValidateAction.java b/core/src/main/java/google/registry/bsa/BsaValidateAction.java index 158dd45f0..4ade7527e 100644 --- a/core/src/main/java/google/registry/bsa/BsaValidateAction.java +++ b/core/src/main/java/google/registry/bsa/BsaValidateAction.java @@ -28,7 +28,6 @@ import static google.registry.bsa.persistence.Queries.queryMissedRegisteredUnblo import static google.registry.bsa.persistence.Queries.queryUnblockableDomainByLabels; import static google.registry.model.tld.Tld.isEnrolledWithBsa; import static google.registry.model.tld.Tlds.getTldEntitiesOfType; -import static google.registry.persistence.transaction.TransactionManagerFactory.replicaTm; import static google.registry.request.Action.Method.GET; import static google.registry.request.Action.Method.POST; import static google.registry.util.BatchedStreams.toBatches; @@ -53,7 +52,6 @@ import google.registry.model.ForeignKeyUtils; import google.registry.model.domain.Domain; import google.registry.model.tld.Tld; import google.registry.model.tld.Tld.TldType; -import google.registry.persistence.VKey; import google.registry.request.Action; import google.registry.request.Action.GaeService; import google.registry.request.Response; @@ -185,8 +183,8 @@ public class BsaValidateAction implements Runnable { ImmutableList batch; do { batch = Queries.batchReadUnblockableDomains(lastRead, transactionBatchSize); - ImmutableMap> activeDomains = - ForeignKeyUtils.load( + ImmutableMap activeDomains = + ForeignKeyUtils.loadResources( Domain.class, batch.stream().map(UnblockableDomain::domainName).collect(toImmutableList()), clock.nowUtc()); @@ -201,7 +199,7 @@ public class BsaValidateAction implements Runnable { } Optional verifyDomainStillUnblockableWithReason( - UnblockableDomain domain, ImmutableMap> activeDomains) { + UnblockableDomain domain, ImmutableMap activeDomains) { DateTime now = clock.nowUtc(); boolean isRegistered = activeDomains.containsKey(domain.domainName()); boolean isReserved = isReservedDomain(domain.domainName(), now); @@ -230,10 +228,8 @@ public class BsaValidateAction implements Runnable { domain.reason())); } - boolean isStalenessAllowed(VKey domainVKey) { - Domain domain = bsaQuery(() -> replicaTm().loadByKey(domainVKey)); - var now = clock.nowUtc(); - return domain.getCreationTime().plus(maxStaleness).isAfter(now); + boolean isStalenessAllowed(Domain domain) { + return domain.getCreationTime().plus(maxStaleness).isAfter(clock.nowUtc()); } /** Returns unique labels across all block lists in the download specified by {@code jobName}. */ diff --git a/core/src/main/java/google/registry/bsa/persistence/DomainsRefresher.java b/core/src/main/java/google/registry/bsa/persistence/DomainsRefresher.java index dc638df85..aa684c735 100644 --- a/core/src/main/java/google/registry/bsa/persistence/DomainsRefresher.java +++ b/core/src/main/java/google/registry/bsa/persistence/DomainsRefresher.java @@ -156,7 +156,7 @@ public final class DomainsRefresher { .collect(toImmutableSet()); ImmutableSet currRegistered = ImmutableSet.copyOf( - ForeignKeyUtils.load(Domain.class, nameToEntity.keySet(), now).keySet()); + ForeignKeyUtils.loadKeys(Domain.class, nameToEntity.keySet(), now).keySet()); SetView noLongerRegistered = Sets.difference(prevRegistered, currRegistered); SetView newlyRegistered = Sets.difference(currRegistered, prevRegistered); diff --git a/core/src/main/java/google/registry/bsa/persistence/LabelDiffUpdates.java b/core/src/main/java/google/registry/bsa/persistence/LabelDiffUpdates.java index c376d9287..21b08fb59 100644 --- a/core/src/main/java/google/registry/bsa/persistence/LabelDiffUpdates.java +++ b/core/src/main/java/google/registry/bsa/persistence/LabelDiffUpdates.java @@ -145,11 +145,10 @@ public final class LabelDiffUpdates { ImmutableSet validDomainNames = labels.stream() - .map(label -> validDomainNamesForLabel(label, idnChecker)) - .flatMap(x -> x) + .flatMap(label -> validDomainNamesForLabel(label, idnChecker)) .collect(toImmutableSet()); ImmutableSet registeredDomainNames = - ImmutableSet.copyOf(ForeignKeyUtils.load(Domain.class, validDomainNames, now).keySet()); + ForeignKeyUtils.loadKeys(Domain.class, validDomainNames, now).keySet(); for (String domain : registeredDomainNames) { nonBlockedDomains.add(new UnblockableDomain(domain, Reason.REGISTERED)); tm().put(BsaUnblockableDomain.of(domain, BsaUnblockableDomain.Reason.REGISTERED)); diff --git a/core/src/main/java/google/registry/flows/CheckApiAction.java b/core/src/main/java/google/registry/flows/CheckApiAction.java index 950747254..a4bec0a5a 100644 --- a/core/src/main/java/google/registry/flows/CheckApiAction.java +++ b/core/src/main/java/google/registry/flows/CheckApiAction.java @@ -38,7 +38,6 @@ import static google.registry.pricing.PricingEngineProxy.isDomainPremium; import static google.registry.util.DomainNameUtils.canonicalizeHostname; import static org.json.simple.JSONValue.toJSONString; -import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.flogger.FluentLogger; @@ -185,8 +184,7 @@ public class CheckApiAction implements Runnable { } private boolean checkExists(String domainString, DateTime now) { - return !ForeignKeyUtils.loadByCache(Domain.class, ImmutableList.of(domainString), now) - .isEmpty(); + return ForeignKeyUtils.loadKeyByCache(Domain.class, domainString, now).isPresent(); } private Optional checkReserved(InternetDomainName domainName) { diff --git a/core/src/main/java/google/registry/flows/ResourceFlowUtils.java b/core/src/main/java/google/registry/flows/ResourceFlowUtils.java index 66ff051dd..890fe8676 100644 --- a/core/src/main/java/google/registry/flows/ResourceFlowUtils.java +++ b/core/src/main/java/google/registry/flows/ResourceFlowUtils.java @@ -71,10 +71,9 @@ public final class ResourceFlowUtils { */ public static void checkLinkedDomains( final String targetId, final DateTime now, final Class resourceClass) throws EppException { - VKey key = ForeignKeyUtils.load(resourceClass, targetId, now); - if (key == null) { - throw new ResourceDoesNotExistException(resourceClass, targetId); - } + VKey key = + ForeignKeyUtils.loadKey(resourceClass, targetId, now) + .orElseThrow(() -> new ResourceDoesNotExistException(resourceClass, targetId)); if (isLinked(key, now)) { throw new ResourceToDeleteIsReferencedException(); } @@ -106,11 +105,10 @@ public final class ResourceFlowUtils { public static void verifyResourceDoesNotExist( Class clazz, String targetId, DateTime now, String registrarId) throws EppException { - VKey key = ForeignKeyUtils.load(clazz, targetId, now); - if (key != null) { - R resource = tm().loadByKey(key); + Optional resource = ForeignKeyUtils.loadResource(clazz, targetId, now); + if (resource.isPresent()) { // These are similar exceptions, but we can track them internally as log-based metrics. - if (Objects.equals(registrarId, resource.getPersistedCurrentSponsorRegistrarId())) { + if (Objects.equals(registrarId, resource.get().getPersistedCurrentSponsorRegistrarId())) { throw new ResourceAlreadyExistsForThisClientException(targetId); } else { throw new ResourceCreateContentionException(targetId); diff --git a/core/src/main/java/google/registry/flows/contact/ContactCheckFlow.java b/core/src/main/java/google/registry/flows/contact/ContactCheckFlow.java index fe5a9da54..74f5f535c 100644 --- a/core/src/main/java/google/registry/flows/contact/ContactCheckFlow.java +++ b/core/src/main/java/google/registry/flows/contact/ContactCheckFlow.java @@ -16,7 +16,6 @@ package google.registry.flows.contact; import static google.registry.flows.FlowUtils.validateRegistrarIsLoggedIn; import static google.registry.flows.ResourceFlowUtils.verifyTargetIdCount; -import static google.registry.model.EppResourceUtils.checkResourcesExist; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; @@ -26,6 +25,7 @@ import google.registry.flows.ExtensionManager; import google.registry.flows.FlowModule.RegistrarId; import google.registry.flows.TransactionalFlow; import google.registry.flows.annotations.ReportingSpec; +import google.registry.model.ForeignKeyUtils; import google.registry.model.contact.Contact; import google.registry.model.contact.ContactCommand.Check; import google.registry.model.eppinput.ResourceCommand; @@ -62,7 +62,7 @@ public final class ContactCheckFlow implements TransactionalFlow { ImmutableList targetIds = ((Check) resourceCommand).getTargetIds(); verifyTargetIdCount(targetIds, maxChecks); ImmutableSet existingIds = - checkResourcesExist(Contact.class, targetIds, clock.nowUtc()); + ForeignKeyUtils.loadKeys(Contact.class, targetIds, clock.nowUtc()).keySet(); ImmutableList.Builder checks = new ImmutableList.Builder<>(); for (String id : targetIds) { boolean unused = !existingIds.contains(id); diff --git a/core/src/main/java/google/registry/flows/domain/DomainCheckFlow.java b/core/src/main/java/google/registry/flows/domain/DomainCheckFlow.java index 87b662c9c..d93da4fa1 100644 --- a/core/src/main/java/google/registry/flows/domain/DomainCheckFlow.java +++ b/core/src/main/java/google/registry/flows/domain/DomainCheckFlow.java @@ -181,7 +181,7 @@ public final class DomainCheckFlow implements TransactionalFlow { .setAsOfDate(now) .build()); ImmutableMap> existingDomains = - ForeignKeyUtils.load(Domain.class, domainNames, now); + ForeignKeyUtils.loadKeys(Domain.class, domainNames, now); // Check block labels only when there are unregistered domains, since "In use" goes before // "Blocked by BSA". ImmutableSet bsaBlockedDomainNames = diff --git a/core/src/main/java/google/registry/flows/domain/DomainInfoFlow.java b/core/src/main/java/google/registry/flows/domain/DomainInfoFlow.java index a6a34ec9b..8522fe8d5 100644 --- a/core/src/main/java/google/registry/flows/domain/DomainInfoFlow.java +++ b/core/src/main/java/google/registry/flows/domain/DomainInfoFlow.java @@ -15,7 +15,7 @@ package google.registry.flows.domain; import static google.registry.flows.FlowUtils.validateRegistrarIsLoggedIn; -import static google.registry.flows.ResourceFlowUtils.verifyExistence; +import static google.registry.flows.ResourceFlowUtils.loadAndVerifyExistence; import static google.registry.flows.ResourceFlowUtils.verifyOptionalAuthInfo; import static google.registry.flows.domain.DomainFlowUtils.addSecDnsExtensionIfPresent; import static google.registry.flows.domain.DomainFlowUtils.handleFeeRequest; @@ -37,7 +37,6 @@ import google.registry.flows.custom.DomainInfoFlowCustomLogic; import google.registry.flows.custom.DomainInfoFlowCustomLogic.AfterValidationParameters; import google.registry.flows.custom.DomainInfoFlowCustomLogic.BeforeResponseParameters; import google.registry.flows.custom.DomainInfoFlowCustomLogic.BeforeResponseReturnData; -import google.registry.model.ForeignKeyUtils; import google.registry.model.domain.Domain; import google.registry.model.domain.DomainCommand.Info; import google.registry.model.domain.DomainCommand.Info.HostsRequest; @@ -108,9 +107,7 @@ public final class DomainInfoFlow implements MutatingFlow { validateRegistrarIsLoggedIn(registrarId); extensionManager.validate(); DateTime now = clock.nowUtc(); - Domain domain = - verifyExistence( - Domain.class, targetId, ForeignKeyUtils.loadResource(Domain.class, targetId, now)); + Domain domain = loadAndVerifyExistence(Domain.class, targetId, now); verifyOptionalAuthInfo(authInfo, domain); flowCustomLogic.afterValidation( AfterValidationParameters.newBuilder().setDomain(domain).build()); diff --git a/core/src/main/java/google/registry/flows/host/HostCheckFlow.java b/core/src/main/java/google/registry/flows/host/HostCheckFlow.java index 790e7bf1c..dc00a363b 100644 --- a/core/src/main/java/google/registry/flows/host/HostCheckFlow.java +++ b/core/src/main/java/google/registry/flows/host/HostCheckFlow.java @@ -16,7 +16,6 @@ package google.registry.flows.host; import static google.registry.flows.FlowUtils.validateRegistrarIsLoggedIn; import static google.registry.flows.ResourceFlowUtils.verifyTargetIdCount; -import static google.registry.model.EppResourceUtils.checkResourcesExist; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; @@ -26,6 +25,7 @@ import google.registry.flows.ExtensionManager; import google.registry.flows.FlowModule.RegistrarId; import google.registry.flows.TransactionalFlow; import google.registry.flows.annotations.ReportingSpec; +import google.registry.model.ForeignKeyUtils; import google.registry.model.eppinput.ResourceCommand; import google.registry.model.eppoutput.CheckData.HostCheck; import google.registry.model.eppoutput.CheckData.HostCheckData; @@ -61,7 +61,8 @@ public final class HostCheckFlow implements TransactionalFlow { extensionManager.validate(); // There are no legal extensions for this flow. ImmutableList hostnames = ((Check) resourceCommand).getTargetIds(); verifyTargetIdCount(hostnames, maxChecks); - ImmutableSet existingIds = checkResourcesExist(Host.class, hostnames, clock.nowUtc()); + ImmutableSet existingIds = + ForeignKeyUtils.loadKeys(Host.class, hostnames, clock.nowUtc()).keySet(); ImmutableList.Builder checks = new ImmutableList.Builder<>(); for (String hostname : hostnames) { boolean unused = !existingIds.contains(hostname); diff --git a/core/src/main/java/google/registry/flows/host/HostUpdateFlow.java b/core/src/main/java/google/registry/flows/host/HostUpdateFlow.java index 63088ccc4..0a553df9b 100644 --- a/core/src/main/java/google/registry/flows/host/HostUpdateFlow.java +++ b/core/src/main/java/google/registry/flows/host/HostUpdateFlow.java @@ -154,7 +154,7 @@ public final class HostUpdateFlow implements MutatingFlow { EppResource owningResource = firstNonNull(oldSuperordinateDomain, existingHost); verifyUpdateAllowed( command, existingHost, newSuperordinateDomain.orElse(null), owningResource, isHostRename); - if (isHostRename && ForeignKeyUtils.load(Host.class, newHostName, now) != null) { + if (isHostRename && ForeignKeyUtils.loadKey(Host.class, newHostName, now).isPresent()) { throw new HostAlreadyExistsException(newHostName); } AddRemove add = command.getInnerAdd(); diff --git a/core/src/main/java/google/registry/model/EppResourceUtils.java b/core/src/main/java/google/registry/model/EppResourceUtils.java index 64a71e09b..f235ac078 100644 --- a/core/src/main/java/google/registry/model/EppResourceUtils.java +++ b/core/src/main/java/google/registry/model/EppResourceUtils.java @@ -16,19 +16,14 @@ package google.registry.model; import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.collect.ImmutableSet.toImmutableSet; -import static google.registry.persistence.transaction.TransactionManagerFactory.replicaTm; import static google.registry.persistence.transaction.TransactionManagerFactory.tm; import static google.registry.util.DateTimeUtils.START_OF_TIME; import static google.registry.util.DateTimeUtils.isAtOrAfter; import static google.registry.util.DateTimeUtils.isBeforeOrAt; -import static google.registry.util.DateTimeUtils.latestOf; -import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import com.google.common.flogger.FluentLogger; -import google.registry.config.RegistryConfig; import google.registry.model.EppResource.BuilderWithTransferData; -import google.registry.model.EppResource.ForeignKeyedEppResource; import google.registry.model.EppResource.ResourceWithTransferData; import google.registry.model.contact.Contact; import google.registry.model.domain.Domain; @@ -41,13 +36,9 @@ import google.registry.model.transfer.DomainTransferData; import google.registry.model.transfer.TransferData; import google.registry.model.transfer.TransferStatus; import google.registry.persistence.VKey; -import google.registry.persistence.transaction.TransactionManager; import jakarta.persistence.Query; -import java.util.Collection; import java.util.Comparator; -import java.util.Optional; import java.util.function.Function; -import java.util.function.Supplier; import javax.annotation.Nullable; import org.joda.time.DateTime; import org.joda.time.Interval; @@ -90,98 +81,6 @@ public final class EppResourceUtils { return (T) resource.cloneProjectedAtTime(now); } - /** - * Loads the last created version of an {@link EppResource} from the database by foreign key, - * using a cache, if caching is enabled in config settings. - * - *

Returns null if no resource with this foreign key was ever created, or if the most recently - * created resource was deleted before time "now". - * - *

Loading an {@link EppResource} by itself is not sufficient to know its current state since - * it may have various expirable conditions and status values that might implicitly change its - * state as time progresses even if it has not been updated in the database. Rather, the resource - * must be combined with a timestamp to view its current state. We use a global last updated - * timestamp to guarantee monotonically increasing write times, and forward our projected time to - * the greater of this timestamp or "now". This guarantees that we're not projecting into the - * past. - * - *

Do not call this cached version for anything that needs transactional consistency. It should - * only be used when it's OK if the data is potentially being out of date, e.g. RDAP. - * - * @param foreignKey id to match - * @param now the current logical time to project resources at - */ - public static Optional loadByForeignKeyByCacheIfEnabled( - Class clazz, String foreignKey, DateTime now) { - return loadByForeignKeyHelper( - tm(), clazz, foreignKey, now, RegistryConfig.isEppResourceCachingEnabled()); - } - - /** - * Loads the last created version of an {@link EppResource} from the replica database by foreign - * key, using a cache. - * - *

This method ignores the config setting for caching, and is reserved for use cases that can - * tolerate slightly stale data. - */ - public static Optional loadByForeignKeyByCache( - Class clazz, String foreignKey, DateTime now) { - return loadByForeignKeyHelper(replicaTm(), clazz, foreignKey, now, true); - } - - static Optional loadByForeignKeyHelper( - TransactionManager txnManager, - Class clazz, - String foreignKey, - DateTime now, - boolean useCache) { - checkArgument( - ForeignKeyedEppResource.class.isAssignableFrom(clazz), - "loadByForeignKey may only be called for foreign keyed EPP resources"); - VKey key = - useCache - ? ForeignKeyUtils.loadByCache(clazz, ImmutableList.of(foreignKey), now).get(foreignKey) - : ForeignKeyUtils.load(clazz, foreignKey, now); - // The returned key is null if the resource is hard deleted or soft deleted by the given time. - if (key == null) { - return Optional.empty(); - } - T resource = - useCache - ? EppResource.loadByCache(key) - // This transaction is buried very deeply inside many outer nested calls, hence merits - // the use of reTransact() for now pending a substantial refactoring. - : txnManager.reTransact(() -> txnManager.loadByKeyIfPresent(key).orElse(null)); - if (resource == null || isAtOrAfter(now, resource.getDeletionTime())) { - return Optional.empty(); - } - // When setting status values based on a time, choose the greater of "now" and the resource's - // UpdateAutoTimestamp. For non-mutating uses (info, RDAP, etc.), this is equivalent to rolling - // "now" forward to at least the last update on the resource, so that a read right after a write - // doesn't appear stale. For mutating flows, if we had to roll now forward then the flow will - // fail when it tries to save anything, since "now" is needed to be > the last update time for - // writes. - return Optional.of( - cloneProjectedAtTime( - resource, latestOf(now, resource.getUpdateTimestamp().getTimestamp()))); - } - - /** - * Checks multiple {@link EppResource} objects from the database by unique ids. - * - *

There are currently no resources that support checks and do not use foreign keys. If we need - * to support that case in the future, we can loosen the type to allow any {@link EppResource} and - * add code to do the lookup by id directly. - * - * @param clazz the resource type to load - * @param uniqueIds a list of ids to match - * @param now the logical time of the check - */ - public static ImmutableSet checkResourcesExist( - Class clazz, Collection uniqueIds, final DateTime now) { - return ForeignKeyUtils.load(clazz, uniqueIds, now).keySet(); - } - /** * Returns a Function that transforms an EppResource to the given DateTime, suitable for use with * Iterables.transform() over a collection of EppResources. @@ -281,21 +180,6 @@ public final class EppResourceUtils { : null); } - /** - * Rewinds an {@link EppResource} object to a given point in time. - * - *

This method costs nothing if {@code resource} is already current. Otherwise, it returns an - * async operation that performs a single fetch operation. - * - * @return an asynchronous operation returning resource at {@code timestamp} or {@code null} if - * resource is deleted or not yet created - * @see #loadAtPointInTime(EppResource, DateTime) - */ - public static Supplier loadAtPointInTimeAsync( - final T resource, final DateTime timestamp) { - return () -> loadAtPointInTime(resource, timestamp); - } - /** * Returns the most recent revision of a given EppResource before or at the provided timestamp, * falling back to using the resource as-is if there are no revisions. @@ -370,13 +254,11 @@ public final class EppResourceUtils { /** * Returns whether the given contact or host is linked to (that is, referenced by) a domain. * - *

This is an eventually consistent query. - * * @param key the referent key * @param now the logical time of the check */ public static boolean isLinked(VKey key, DateTime now) { - return getLinkedDomainKeys(key, now, 1).size() > 0; + return !getLinkedDomainKeys(key, now, 1).isEmpty(); } private EppResourceUtils() {} diff --git a/core/src/main/java/google/registry/model/ForeignKeyUtils.java b/core/src/main/java/google/registry/model/ForeignKeyUtils.java index f1ae846e4..e00d07a0b 100644 --- a/core/src/main/java/google/registry/model/ForeignKeyUtils.java +++ b/core/src/main/java/google/registry/model/ForeignKeyUtils.java @@ -18,7 +18,6 @@ import static com.google.common.collect.ImmutableList.toImmutableList; import static com.google.common.collect.ImmutableMap.toImmutableMap; import static google.registry.config.RegistryConfig.getEppResourceCachingDuration; import static google.registry.config.RegistryConfig.getEppResourceMaxCachedEntries; -import static google.registry.model.EppResourceUtils.loadByForeignKeyHelper; import static google.registry.persistence.transaction.TransactionManagerFactory.replicaTm; import static google.registry.persistence.transaction.TransactionManagerFactory.tm; @@ -42,13 +41,17 @@ import java.util.Map; import java.util.Map.Entry; import java.util.Optional; import java.util.Set; -import javax.annotation.Nullable; import org.joda.time.DateTime; /** - * Util class to map a foreign key to the {@link VKey} to the active instance of {@link EppResource} - * whose unique repoId matches the foreign key string at a given time. The instance is never - * deleted, but it is updated if a newer entity becomes the active entity. + * Util class for mapping a foreign key to a {@link VKey} or {@link EppResource}. + * + *

We return the resource that matches the foreign key at a given time (this may vary over time + * e.g. if a domain expires and is re-registered). The instance is never deleted, but it is updated + * if a newer entity becomes the active entity. + * + *

One can retrieve either the {@link VKey} (the repo ID) in the situations where that's + * sufficient, or the resource itself, either with or without caching. */ public final class ForeignKeyUtils { @@ -61,35 +64,17 @@ public final class ForeignKeyUtils { Domain.class, "domainName", Host.class, "hostName"); - /** - * Loads a {@link VKey} to an {@link EppResource} from the database by foreign key. - * - *

Returns null if no resource with this foreign key was ever created, or if the most recently - * created resource was deleted before time "now". - * - * @param clazz the resource type to load - * @param foreignKey foreign key to match - * @param now the current logical time to use when checking for soft deletion of the foreign key - * index - */ - @Nullable - public static VKey load( - Class clazz, String foreignKey, DateTime now) { - return load(clazz, ImmutableList.of(foreignKey), now).get(foreignKey); - } + public record MostRecentResource(String repoId, DateTime deletionTime) {} /** - * Load a map of {@link String} foreign keys to {@link VKey}s to {@link EppResource} that are - * active at or after the specified moment in time. + * Loads an optional {@link VKey} to an {@link EppResource} from the database by foreign key. * - *

The returned map will omit any foreign keys for which the {@link EppResource} doesn't exist - * or has been soft-deleted. + *

Returns empty if no resource with this foreign key was ever created, or if the most recently + * created resource was deleted before time "now". */ - public static ImmutableMap> load( - Class clazz, Collection foreignKeys, final DateTime now) { - return loadMostRecentResources(clazz, foreignKeys, false).entrySet().stream() - .filter(e -> now.isBefore(e.getValue().deletionTime())) - .collect(toImmutableMap(Entry::getKey, e -> VKey.create(clazz, e.getValue().repoId()))); + public static Optional> loadKey( + Class clazz, String foreignKey, DateTime now) { + return Optional.ofNullable(loadKeys(clazz, ImmutableList.of(foreignKey), now).get(foreignKey)); } /** @@ -100,7 +85,38 @@ public final class ForeignKeyUtils { */ public static Optional loadResource( Class clazz, String foreignKey, DateTime now) { - return loadByForeignKeyHelper(tm(), clazz, foreignKey, now, false); + // Note: no need to project to "now" because loadResources already does + return Optional.ofNullable( + loadResources(clazz, ImmutableList.of(foreignKey), now).get(foreignKey)); + } + + /** + * Load a map of {@link String} foreign keys to {@link VKey}s to {@link EppResource} that are + * active at or after the specified moment in time. + * + *

The returned map will omit any foreign keys for which the {@link EppResource} doesn't exist + * or has been soft-deleted. + */ + public static ImmutableMap> loadKeys( + Class clazz, Collection foreignKeys, DateTime now) { + return loadMostRecentResources(clazz, foreignKeys, false).entrySet().stream() + .filter(e -> now.isBefore(e.getValue().deletionTime())) + .collect(toImmutableMap(Entry::getKey, e -> VKey.create(clazz, e.getValue().repoId()))); + } + + /** + * Load a map of {@link String} foreign keys to the {@link EppResource} that are active at or + * after the specified moment in time. + * + *

The returned map will omit any foreign keys for which the {@link EppResource} doesn't exist + * or has been soft-deleted. + */ + @SuppressWarnings("unchecked") + public static ImmutableMap loadResources( + Class clazz, Collection foreignKeys, DateTime now) { + return loadMostRecentResourceObjects(clazz, foreignKeys, false).entrySet().stream() + .filter(e -> now.isBefore(e.getValue().getDeletionTime())) + .collect(toImmutableMap(Entry::getKey, e -> (E) e.getValue().cloneProjectedAtTime(now))); } /** @@ -136,22 +152,49 @@ public final class ForeignKeyUtils { .collect( toImmutableMap( row -> (String) row[0], - row -> MostRecentResource.create((String) row[1], (DateTime) row[2])))); + row -> new MostRecentResource((String) row[1], (DateTime) row[2])))); } - private static final CacheLoader, Optional> - CACHE_LOADER = - new CacheLoader<>() { + /** Method to load the most recent {@link EppResource}s for the given foreign keys. */ + private static ImmutableMap loadMostRecentResourceObjects( + Class clazz, Collection foreignKeys, boolean useReplicaTm) { + String fkProperty = RESOURCE_TYPE_TO_FK_PROPERTY.get(clazz); + JpaTransactionManager tmToUse = useReplicaTm ? replicaTm() : tm(); + return tmToUse.reTransact( + () -> + tmToUse + .query( + ("FROM %entity% WHERE (%fkProperty%, deletionTime) IN (SELECT %fkProperty%, " + + "MAX(deletionTime) FROM %entity% WHERE %fkProperty% IN (:fks) " + + "GROUP BY %fkProperty%)") + .replace("%fkProperty%", fkProperty) + .replace("%entity%", clazz.getSimpleName()), + clazz) + .setParameter("fks", foreignKeys) + .getResultStream() + .collect(toImmutableMap(EppResource::getForeignKey, e -> e))); + } + /** + * Cache loader for loading repo IDs and deletion times for the given foreign keys. + * + *

Note: while this is given a {@link VKey}, one cannot use that key to load directly from the + * database. That key is basically used to signify a foreign-key + resource-type pairing. + */ + private static final CacheLoader, Optional> + REPO_ID_CACHE_LOADER = + new CacheLoader<>() { @Override public Optional load(VKey key) { - return loadAll(ImmutableSet.of(key)).get(key); + String foreignKey = (String) key.getKey(); + return Optional.ofNullable( + loadMostRecentResources(key.getKind(), ImmutableList.of(foreignKey), true) + .get(foreignKey)); } @Override - public Map< - ? extends VKey, ? extends Optional> - loadAll(Set> keys) { + public Map, ? extends Optional> loadAll( + Set> keys) { if (keys.isEmpty()) { return ImmutableMap.of(); } @@ -161,7 +204,7 @@ public final class ForeignKeyUtils { ImmutableList foreignKeys = keys.stream().map(key -> (String) key.getKey()).collect(toImmutableList()); ImmutableMap existingKeys = - ForeignKeyUtils.loadMostRecentResources(clazz, foreignKeys, true); + loadMostRecentResources(clazz, foreignKeys, true); // The above map only contains keys that exist in the database, so we re-add the // missing ones with Optional.empty() values for caching. return Maps.asMap( @@ -171,11 +214,10 @@ public final class ForeignKeyUtils { }; /** - * A limited size, limited time cache for foreign-keyed entities. + * A limited size, limited time cache for mapping foreign keys to repo IDs. * - *

This is only used to cache foreign-keyed entities for the purposes of checking whether they - * exist (and if so, what entity they point to) during a few domain flows. Any other operations on - * foreign keys should not use this cache. + *

This is only used to cache foreign-keyed entity keys for the purposes of checking whether + * they exist (and if so, what entity they point to). * *

Note that here the key of the {@link LoadingCache} is of type {@code VKey}, but they are not legal {@link VKey}s to {@link EppResource}s, whose keys are the @@ -189,22 +231,26 @@ public final class ForeignKeyUtils { * our system that actually exist. So we cache the fact that they *don't* exist by using * Optional.empty(), then several layers up the EPP command will fail with an error message like * "The contact with given IDs (blah) don't exist." + * + *

If one wishes to load the entity itself via cache, use the {@link + * #foreignKeyToResourceCache} instead, as that loads the entity instead. This cache is used for + * situations where the repo ID, or the existence of the repo ID, is sufficient. */ @NonFinalForTesting private static LoadingCache, Optional> - foreignKeyCache = createForeignKeyMapCache(getEppResourceCachingDuration()); + foreignKeyToRepoIdCache = createForeignKeyToRepoIdCache(getEppResourceCachingDuration()); private static LoadingCache, Optional> - createForeignKeyMapCache(Duration expiry) { + createForeignKeyToRepoIdCache(Duration expiry) { return CacheUtils.newCacheBuilder(expiry) .maximumSize(getEppResourceMaxCachedEntries()) - .build(CACHE_LOADER); + .build(REPO_ID_CACHE_LOADER); } @VisibleForTesting - public static void setCacheForTest(Optional expiry) { + public static void setRepoIdCacheForTest(Optional expiry) { Duration effectiveExpiry = expiry.orElse(getEppResourceCachingDuration()); - foreignKeyCache = createForeignKeyMapCache(effectiveExpiry); + foreignKeyToRepoIdCache = createForeignKeyToRepoIdCache(effectiveExpiry); } /** @@ -217,26 +263,12 @@ public final class ForeignKeyUtils { *

Don't use the cached version of this method unless you really need it for performance * reasons, and are OK with the trade-offs in loss of transactional consistency. */ - public static ImmutableMap> loadByCacheIfEnabled( - Class clazz, Collection foreignKeys, final DateTime now) { + public static ImmutableMap> loadKeysByCacheIfEnabled( + Class clazz, Collection foreignKeys, DateTime now) { if (!RegistryConfig.isEppResourceCachingEnabled()) { - return load(clazz, foreignKeys, now); + return loadKeys(clazz, foreignKeys, now); } - return loadByCache(clazz, foreignKeys, now); - } - - /** - * Load a list of {@link VKey} to {@link EppResource} instances by class and foreign key strings - * that are active at or after the specified moment in time, using the cache. - * - *

The returned map will omit any keys for which the {@link EppResource} doesn't exist or has - * been soft-deleted. - * - *

This method is reserved for use cases that can tolerate slightly stale data. - */ - public static ImmutableMap> loadByCache( - Class clazz, Collection foreignKeys, final DateTime now) { - return foreignKeyCache + return foreignKeyToRepoIdCache .getAll(foreignKeys.stream().map(fk -> VKey.create(clazz, fk)).collect(toImmutableList())) .entrySet() .stream() @@ -247,10 +279,127 @@ public final class ForeignKeyUtils { e -> VKey.create(clazz, e.getValue().get().repoId()))); } - public record MostRecentResource(String repoId, DateTime deletionTime) { + /** Loads an optional {@link VKey} to an {@link EppResource} using the cache. */ + public static Optional> loadKeyByCache( + Class clazz, String foreignKey, DateTime now) { + return foreignKeyToRepoIdCache + .get(VKey.create(clazz, foreignKey)) + .filter(mrr -> now.isBefore(mrr.deletionTime())) + .map(mrr -> VKey.create(clazz, mrr.repoId())); + } - static MostRecentResource create(String repoId, DateTime deletionTime) { - return new MostRecentResource(repoId, deletionTime); - } + /** + * Cache loader for loading {@link EppResource}s for the given foreign keys. + * + *

Note: while this is given a {@link VKey}, one cannot use that key to load directly from the + * database. That key is basically used to signify a foreign-key + resource-type pairing. + */ + private static final CacheLoader, Optional> + RESOURCE_CACHE_LOADER = + new CacheLoader<>() { + @Override + public Optional load(VKey key) { + String foreignKey = (String) key.getKey(); + return Optional.ofNullable( + loadMostRecentResourceObjects(key.getKind(), ImmutableList.of(foreignKey), true) + .get(foreignKey)); + } + + @Override + public Map, Optional> loadAll( + Set> keys) { + if (keys.isEmpty()) { + return ImmutableMap.of(); + } + // It is safe to use the resource type of first element because when this function is + // called, it is always passed with a list of VKeys with the same type. + Class clazz = keys.iterator().next().getKind(); + ImmutableList foreignKeys = + keys.stream().map(key -> (String) key.getKey()).collect(toImmutableList()); + ImmutableMap existingResources = + loadMostRecentResourceObjects(clazz, foreignKeys, true); + // The above map only contains resources that exist in the database, so we re-add the + // missing ones with Optional.empty() values for caching. + return Maps.asMap( + ImmutableSet.copyOf(keys), + key -> Optional.ofNullable(existingResources.get((String) key.getKey()))); + } + }; + + /** + * An additional limited size, limited time cache for foreign-keyed entities. + * + *

Note that here the key of the {@link LoadingCache} is of type {@code VKey}, but they are not legal {@link VKey}s to {@link EppResource}s, whose keys are the + * SQL primary keys, i.e., the {@code repoId}s. Instead, their keys are the foreign keys used to + * query the database. We use {@link VKey} here because it is a convenient composite class that + * contains both the resource type and the foreign key, which are needed to for the query and + * caching. + * + *

Also note that the value type of this cache is {@link Optional} because the foreign keys in + * question are coming from external commands, and thus don't necessarily represent entities in + * our system that actually exist. So we cache the fact that they *don't* exist by using + * Optional.empty(), then several layers up the EPP command will fail with an error message like + * "The contact with given IDs (blah) don't exist." + * + *

This cache bypasses the foreign-key-to-repo-ID lookup and maps directly from the foreign key + * to the entity itself (at least, at this point in time). + */ + private static LoadingCache, Optional> + foreignKeyToResourceCache = createForeignKeyToResourceCache(getEppResourceCachingDuration()); + + private static LoadingCache, Optional> + createForeignKeyToResourceCache(Duration expiry) { + return CacheUtils.newCacheBuilder(expiry) + .maximumSize(getEppResourceMaxCachedEntries()) + .build(RESOURCE_CACHE_LOADER); + } + + @VisibleForTesting + public static void setResourceCacheForTest(Optional expiry) { + Duration effectiveExpiry = expiry.orElse(getEppResourceCachingDuration()); + foreignKeyToResourceCache = createForeignKeyToResourceCache(effectiveExpiry); + } + + /** + * Loads the last created version of an {@link EppResource} from the database by foreign key, + * using a cache, if caching is enabled in config settings. + * + *

Returns null if no resource with this foreign key was ever created, or if the most recently + * created resource was deleted before time "now". + * + *

Loading an {@link EppResource} by itself is not sufficient to know its current state since + * it may have various expirable conditions and status values that might implicitly change its + * state as time progresses even if it has not been updated in the database. Rather, the resource + * must be combined with a timestamp to view its current state. We use a global last updated + * timestamp to guarantee monotonically increasing write times, and forward our projected time to + * the greater of this timestamp or "now". This guarantees that we're not projecting into the + * past. + * + *

Do not call this cached version for anything that needs transactional consistency. It should + * only be used when it's OK if the data is potentially being out of date, e.g. RDAP. + */ + public static Optional loadResourceByCacheIfEnabled( + Class clazz, String foreignKey, DateTime now) { + return RegistryConfig.isEppResourceCachingEnabled() + ? loadResourceByCache(clazz, foreignKey, now) + : loadResource(clazz, foreignKey, now); + } + + /** + * Loads the last created version of an {@link EppResource} from the replica database by foreign + * key, using a cache. + * + *

This method ignores the config setting for caching, and is reserved for use cases that can + * tolerate slightly stale data. + */ + @SuppressWarnings("unchecked") + public static Optional loadResourceByCache( + Class clazz, String foreignKey, DateTime now) { + return (Optional) + foreignKeyToResourceCache + .get(VKey.create(clazz, foreignKey)) + .filter(e -> now.isBefore(e.getDeletionTime())) + .map(e -> e.cloneProjectedAtTime(now)); } } diff --git a/core/src/main/java/google/registry/model/domain/DomainCommand.java b/core/src/main/java/google/registry/model/domain/DomainCommand.java index fb8441c5e..4d23db7f7 100644 --- a/core/src/main/java/google/registry/model/domain/DomainCommand.java +++ b/core/src/main/java/google/registry/model/domain/DomainCommand.java @@ -442,7 +442,7 @@ public class DomainCommand { final Set foreignKeys, final Class clazz, final DateTime now) throws InvalidReferencesException { ImmutableMap> fks = - ForeignKeyUtils.loadByCacheIfEnabled(clazz, foreignKeys, now); + ForeignKeyUtils.loadKeysByCacheIfEnabled(clazz, foreignKeys, now); if (!fks.keySet().equals(foreignKeys)) { throw new InvalidReferencesException( clazz, ImmutableSet.copyOf(difference(foreignKeys, fks.keySet()))); diff --git a/core/src/main/java/google/registry/rdap/RdapDomainAction.java b/core/src/main/java/google/registry/rdap/RdapDomainAction.java index a20b5b625..6ab9e5a37 100644 --- a/core/src/main/java/google/registry/rdap/RdapDomainAction.java +++ b/core/src/main/java/google/registry/rdap/RdapDomainAction.java @@ -15,7 +15,6 @@ package google.registry.rdap; import static google.registry.flows.domain.DomainFlowUtils.validateDomainName; -import static google.registry.model.EppResourceUtils.loadByForeignKeyByCache; import static google.registry.request.Action.Method.GET; import static google.registry.request.Action.Method.HEAD; import static google.registry.util.DateTimeUtils.START_OF_TIME; @@ -23,6 +22,7 @@ import static google.registry.util.DateTimeUtils.START_OF_TIME; import com.google.common.net.InternetDomainName; import google.registry.flows.EppException; import google.registry.flows.domain.DomainFlowUtils; +import google.registry.model.ForeignKeyUtils; import google.registry.model.domain.Domain; import google.registry.model.tld.Tld; import google.registry.rdap.RdapJsonFormatter.OutputDataType; @@ -68,7 +68,7 @@ public class RdapDomainAction extends RdapActionBase { } // The query string is not used; the RDAP syntax is /rdap/domain/mydomain.com. Optional domain = - loadByForeignKeyByCache( + ForeignKeyUtils.loadResourceByCache( Domain.class, pathSearchString, shouldIncludeDeleted() ? START_OF_TIME : rdapJsonFormatter.getRequestTime()); diff --git a/core/src/main/java/google/registry/rdap/RdapDomainSearchAction.java b/core/src/main/java/google/registry/rdap/RdapDomainSearchAction.java index 7917feaa8..f4e13050b 100644 --- a/core/src/main/java/google/registry/rdap/RdapDomainSearchAction.java +++ b/core/src/main/java/google/registry/rdap/RdapDomainSearchAction.java @@ -15,7 +15,6 @@ package google.registry.rdap; import static com.google.common.collect.ImmutableSet.toImmutableSet; -import static google.registry.model.EppResourceUtils.loadByForeignKeyByCache; import static google.registry.persistence.transaction.TransactionManagerFactory.replicaTm; import static google.registry.request.Action.Method.GET; import static google.registry.request.Action.Method.HEAD; @@ -59,6 +58,7 @@ import java.util.List; import java.util.Optional; import java.util.stream.Stream; import org.hibernate.Hibernate; +import org.joda.time.DateTime; /** * RDAP action for domain search requests. @@ -183,7 +183,7 @@ public class RdapDomainSearchAction extends RdapSearchActionBase { private DomainSearchResponse searchByDomainNameWithoutWildcard( final RdapSearchPattern partialStringQuery) { Optional domain = - loadByForeignKeyByCache( + ForeignKeyUtils.loadResourceByCache( Domain.class, partialStringQuery.getInitialString(), getRequestTime()); return makeSearchResults( shouldBeVisible(domain) ? ImmutableList.of(domain.get()) : ImmutableList.of()); @@ -333,40 +333,35 @@ public class RdapDomainSearchAction extends RdapSearchActionBase { /** Assembles a list of {@link Host} keys by name when the pattern has no wildcard. */ private ImmutableList> getNameserverRefsByLdhNameWithoutWildcard( final RdapSearchPattern partialStringQuery) { + DateTime timeToQuery = shouldIncludeDeleted() ? START_OF_TIME : getRequestTime(); // If we need to check the sponsoring registrar, we need to load the resource rather than just // the key. Optional desiredRegistrar = getDesiredRegistrar(); + String queryString = partialStringQuery.getInitialString(); if (desiredRegistrar.isPresent()) { Optional host = - loadByForeignKeyByCache( - Host.class, - partialStringQuery.getInitialString(), - shouldIncludeDeleted() ? START_OF_TIME : getRequestTime()); + ForeignKeyUtils.loadResourceByCache(Host.class, queryString, timeToQuery); return (host.isEmpty() || !desiredRegistrar.get().equals(host.get().getPersistedCurrentSponsorRegistrarId())) ? ImmutableList.of() : ImmutableList.of(host.get().createVKey()); } else { - VKey hostKey = - ForeignKeyUtils.load( - Host.class, - partialStringQuery.getInitialString(), - shouldIncludeDeleted() ? START_OF_TIME : getRequestTime()); - return (hostKey == null) ? ImmutableList.of() : ImmutableList.of(hostKey); + Optional> hostKey = + ForeignKeyUtils.loadKeyByCache(Host.class, queryString, timeToQuery); + return hostKey.map(ImmutableList::of).orElseGet(ImmutableList::of); } } /** Assembles a list of {@link Host} keys by name using a superordinate domain suffix. */ private ImmutableList> getNameserverRefsByLdhNameWithSuffix( - final RdapSearchPattern partialStringQuery) { + RdapSearchPattern partialStringQuery) { + DateTime timeToQuery = shouldIncludeDeleted() ? START_OF_TIME : getRequestTime(); // The suffix must be a domain that we manage. That way, we can look up the domain and search // through the subordinate hosts. This is more efficient, and lets us permit wildcard searches // with no initial string. Domain domain = - loadByForeignKeyByCache( - Domain.class, - partialStringQuery.getSuffix(), - shouldIncludeDeleted() ? START_OF_TIME : getRequestTime()) + ForeignKeyUtils.loadResourceByCache( + Domain.class, partialStringQuery.getSuffix(), timeToQuery) .orElseThrow( () -> new UnprocessableEntityException( @@ -379,9 +374,7 @@ public class RdapDomainSearchAction extends RdapSearchActionBase { // then the query ns.exam*.example.com would match against nameserver ns.example.com. if (partialStringQuery.matches(fqhn)) { if (desiredRegistrar.isPresent()) { - Optional host = - loadByForeignKeyByCache( - Host.class, fqhn, shouldIncludeDeleted() ? START_OF_TIME : getRequestTime()); + Optional host = ForeignKeyUtils.loadResourceByCache(Host.class, fqhn, timeToQuery); if (host.isPresent() && desiredRegistrar .get() @@ -389,14 +382,10 @@ public class RdapDomainSearchAction extends RdapSearchActionBase { builder.add(host.get().createVKey()); } } else { - VKey hostKey = - ForeignKeyUtils.load( - Host.class, fqhn, shouldIncludeDeleted() ? START_OF_TIME : getRequestTime()); - if (hostKey != null) { - builder.add(hostKey); - } else { - logger.atWarning().log("Host key unexpectedly null."); - } + Optional> hostKey = + ForeignKeyUtils.loadKeyByCache(Host.class, fqhn, timeToQuery); + hostKey.ifPresentOrElse( + builder::add, () -> logger.atWarning().log("Host key unexpectedly null.")); } } } diff --git a/core/src/main/java/google/registry/rdap/RdapNameserverAction.java b/core/src/main/java/google/registry/rdap/RdapNameserverAction.java index 82608a0dd..de734e17b 100644 --- a/core/src/main/java/google/registry/rdap/RdapNameserverAction.java +++ b/core/src/main/java/google/registry/rdap/RdapNameserverAction.java @@ -15,12 +15,12 @@ package google.registry.rdap; import static google.registry.flows.host.HostFlowUtils.validateHostName; -import static google.registry.model.EppResourceUtils.loadByForeignKeyByCache; import static google.registry.request.Action.Method.GET; import static google.registry.request.Action.Method.HEAD; import static google.registry.util.DateTimeUtils.START_OF_TIME; import google.registry.flows.EppException; +import google.registry.model.ForeignKeyUtils; import google.registry.model.host.Host; import google.registry.rdap.RdapJsonFormatter.OutputDataType; import google.registry.rdap.RdapMetrics.EndpointType; @@ -63,7 +63,7 @@ public class RdapNameserverAction extends RdapActionBase { // If there are no undeleted nameservers with the given name, the foreign key should point to // the most recently deleted one. Optional host = - loadByForeignKeyByCache( + ForeignKeyUtils.loadResourceByCache( Host.class, pathSearchString, shouldIncludeDeleted() ? START_OF_TIME : getRequestTime()); diff --git a/core/src/main/java/google/registry/rdap/RdapNameserverSearchAction.java b/core/src/main/java/google/registry/rdap/RdapNameserverSearchAction.java index c7fef6248..f4284b670 100644 --- a/core/src/main/java/google/registry/rdap/RdapNameserverSearchAction.java +++ b/core/src/main/java/google/registry/rdap/RdapNameserverSearchAction.java @@ -14,7 +14,6 @@ package google.registry.rdap; -import static google.registry.model.EppResourceUtils.loadByForeignKeyByCache; import static google.registry.persistence.transaction.TransactionManagerFactory.replicaTm; import static google.registry.request.Action.Method.GET; import static google.registry.request.Action.Method.HEAD; @@ -25,6 +24,7 @@ import com.google.common.collect.ImmutableSortedSet; import com.google.common.collect.Iterables; import com.google.common.net.InetAddresses; import com.google.common.primitives.Booleans; +import google.registry.model.ForeignKeyUtils; import google.registry.model.domain.Domain; import google.registry.model.host.Host; import google.registry.persistence.transaction.CriteriaQueryBuilder; @@ -159,7 +159,7 @@ public class RdapNameserverSearchAction extends RdapSearchActionBase { .setIncompletenessWarningType(IncompletenessWarningType.COMPLETE); Optional host = - loadByForeignKeyByCache( + ForeignKeyUtils.loadResourceByCache( Host.class, partialStringQuery.getInitialString(), getRequestTime()); metricInformationBuilder.setNumHostsRetrieved(host.isPresent() ? 1 : 0); @@ -176,7 +176,8 @@ public class RdapNameserverSearchAction extends RdapSearchActionBase { private NameserverSearchResponse searchByNameUsingSuperordinateDomain( RdapSearchPattern partialStringQuery) { Optional domain = - loadByForeignKeyByCache(Domain.class, partialStringQuery.getSuffix(), getRequestTime()); + ForeignKeyUtils.loadResourceByCache( + Domain.class, partialStringQuery.getSuffix(), getRequestTime()); if (domain.isEmpty()) { // Don't allow wildcards with suffixes which are not domains we manage. That would risk a // table scan in many easily foreseeable cases. The user might ask for ns*.zombo.com, @@ -194,7 +195,8 @@ public class RdapNameserverSearchAction extends RdapSearchActionBase { // We can't just check that the host name starts with the initial query string, because // then the query ns.exam*.example.com would match against nameserver ns.example.com. if (partialStringQuery.matches(fqhn)) { - Optional host = loadByForeignKeyByCache(Host.class, fqhn, getRequestTime()); + Optional host = + ForeignKeyUtils.loadResourceByCache(Host.class, fqhn, getRequestTime()); if (shouldBeVisible(host)) { hostList.add(host.get()); if (hostList.size() > rdapResultSetMaxSize) { diff --git a/core/src/main/java/google/registry/rde/RdeFragmenter.java b/core/src/main/java/google/registry/rde/RdeFragmenter.java deleted file mode 100644 index c13d03dcb..000000000 --- a/core/src/main/java/google/registry/rde/RdeFragmenter.java +++ /dev/null @@ -1,103 +0,0 @@ -// Copyright 2021 The Nomulus Authors. All Rights Reserved. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package google.registry.rde; - -import static google.registry.model.EppResourceUtils.loadAtPointInTime; -import static google.registry.persistence.transaction.TransactionManagerFactory.tm; - -import com.google.common.collect.ImmutableMap; -import google.registry.model.EppResource; -import google.registry.model.contact.Contact; -import google.registry.model.domain.Domain; -import google.registry.model.host.Host; -import google.registry.model.rde.RdeMode; -import java.util.HashMap; -import java.util.Map; -import java.util.Objects; -import java.util.Optional; -import java.util.function.Supplier; -import org.joda.time.DateTime; - -/** Loading cache that turns a resource into XML for the various points in time and modes. */ -public class RdeFragmenter { - private final Map> cache = new HashMap<>(); - private final ImmutableMap> resourceAtTimes; - private final RdeMarshaller marshaller; - - long cacheHits = 0; - long resourcesNotFound = 0; - long resourcesFound = 0; - - public RdeFragmenter( - ImmutableMap> resourceAtTimes, RdeMarshaller marshaller) { - this.resourceAtTimes = resourceAtTimes; - this.marshaller = marshaller; - } - - public Optional marshal(DateTime watermark, RdeMode mode) { - Optional result = cache.get(WatermarkModePair.create(watermark, mode)); - if (result != null) { - cacheHits++; - return result; - } - EppResource resource = resourceAtTimes.get(watermark).get(); - if (resource == null) { - result = Optional.empty(); - cache.put(WatermarkModePair.create(watermark, RdeMode.FULL), result); - cache.put(WatermarkModePair.create(watermark, RdeMode.THIN), result); - resourcesNotFound++; - return result; - } - resourcesFound++; - if (resource instanceof Domain) { - result = Optional.of(marshaller.marshalDomain((Domain) resource, mode)); - cache.put(WatermarkModePair.create(watermark, mode), result); - return result; - } else if (resource instanceof Contact) { - result = Optional.of(marshaller.marshalContact((Contact) resource)); - cache.put(WatermarkModePair.create(watermark, RdeMode.FULL), result); - cache.put(WatermarkModePair.create(watermark, RdeMode.THIN), result); - return result; - } else if (resource instanceof Host host) { - result = - Optional.of( - host.isSubordinate() - ? marshaller.marshalSubordinateHost( - host, - // Note that loadAtPointInTime() does cloneProjectedAtTime(watermark) for - // us. - Objects.requireNonNull( - loadAtPointInTime( - tm().loadByKey(host.getSuperordinateDomain()), watermark))) - : marshaller.marshalExternalHost(host)); - cache.put(WatermarkModePair.create(watermark, RdeMode.FULL), result); - cache.put(WatermarkModePair.create(watermark, RdeMode.THIN), result); - return result; - } else { - throw new IllegalStateException( - String.format( - "Resource %s of type %s cannot be converted to XML.", - resource, resource.getClass().getSimpleName())); - } - } - - /** Map key for {@link RdeFragmenter} cache. */ - record WatermarkModePair(DateTime watermark, RdeMode mode) { - - static WatermarkModePair create(DateTime watermark, RdeMode mode) { - return new WatermarkModePair(watermark, mode); - } - } -} diff --git a/core/src/main/java/google/registry/tools/CommandUtilities.java b/core/src/main/java/google/registry/tools/CommandUtilities.java index 084e306a4..ad00c76c1 100644 --- a/core/src/main/java/google/registry/tools/CommandUtilities.java +++ b/core/src/main/java/google/registry/tools/CommandUtilities.java @@ -41,7 +41,10 @@ class CommandUtilities { } public VKey getKey(String uniqueId, DateTime now) { - return ForeignKeyUtils.load(clazz, uniqueId, now); + return ForeignKeyUtils.loadKey(clazz, uniqueId, now) + .orElseThrow( + () -> + new IllegalArgumentException(String.format("Invalid resource ID %s", uniqueId))); } } diff --git a/core/src/main/java/google/registry/tools/DomainLockUtils.java b/core/src/main/java/google/registry/tools/DomainLockUtils.java index 4eb87f1f2..26194e8fc 100644 --- a/core/src/main/java/google/registry/tools/DomainLockUtils.java +++ b/core/src/main/java/google/registry/tools/DomainLockUtils.java @@ -16,7 +16,6 @@ package google.registry.tools; import static com.google.common.base.Preconditions.checkArgument; import static google.registry.batch.AsyncTaskEnqueuer.QUEUE_ASYNC_ACTIONS; -import static google.registry.model.EppResourceUtils.loadByForeignKeyByCacheIfEnabled; import static google.registry.persistence.transaction.TransactionManagerFactory.tm; import static google.registry.tools.LockOrUnlockDomainCommand.REGISTRY_LOCK_STATUSES; @@ -26,6 +25,7 @@ import com.google.common.collect.Sets; import google.registry.batch.CloudTasksUtils; import google.registry.batch.RelockDomainAction; import google.registry.config.RegistryConfig.Config; +import google.registry.model.ForeignKeyUtils; import google.registry.model.billing.BillingBase.Reason; import google.registry.model.billing.BillingEvent; import google.registry.model.domain.Domain; @@ -327,7 +327,7 @@ public final class DomainLockUtils { private Domain getDomain(String domainName, String registrarId, DateTime now) { Domain domain = - loadByForeignKeyByCacheIfEnabled(Domain.class, domainName, now) + ForeignKeyUtils.loadResource(Domain.class, domainName, now) .orElseThrow(() -> new IllegalArgumentException("Domain doesn't exist")); // The user must have specified either the correct registrar ID or the admin registrar ID checkArgument( diff --git a/core/src/main/java/google/registry/tools/EnqueuePollMessageCommand.java b/core/src/main/java/google/registry/tools/EnqueuePollMessageCommand.java index 6d32fed38..b9c1bc260 100644 --- a/core/src/main/java/google/registry/tools/EnqueuePollMessageCommand.java +++ b/core/src/main/java/google/registry/tools/EnqueuePollMessageCommand.java @@ -24,7 +24,7 @@ import com.beust.jcommander.Parameters; import com.google.common.collect.ImmutableList; import com.google.common.collect.Streams; import google.registry.config.RegistryConfig.Config; -import google.registry.model.ForeignKeyUtils; +import google.registry.flows.ResourceFlowUtils; import google.registry.model.domain.Domain; import google.registry.model.domain.DomainHistory; import google.registry.model.poll.PollMessage; @@ -33,7 +33,6 @@ import google.registry.model.reporting.HistoryEntry; import google.registry.util.DomainNameUtils; import jakarta.inject.Inject; import java.util.List; -import java.util.Optional; /** * Tool to enqueue a poll message for a registrar. @@ -85,11 +84,9 @@ class EnqueuePollMessageCommand extends MutatingCommand { !sendToAll || isNullOrEmpty(clientIds), "Cannot specify both --all and --clients"); tm().transact( () -> { - Optional domainOpt = - ForeignKeyUtils.loadResource(Domain.class, domainName, tm().getTransactionTime()); - checkArgument( - domainOpt.isPresent(), "Domain %s doesn't exist or isn't active", domainName); - Domain domain = domainOpt.get(); + Domain domain = + ResourceFlowUtils.loadAndVerifyExistence( + Domain.class, domainName, tm().getTransactionTime()); ImmutableList registrarIds; if (sendToAll) { registrarIds = diff --git a/core/src/main/java/google/registry/tools/GetHistoryEntriesCommand.java b/core/src/main/java/google/registry/tools/GetHistoryEntriesCommand.java index 09da533ec..394cbc702 100644 --- a/core/src/main/java/google/registry/tools/GetHistoryEntriesCommand.java +++ b/core/src/main/java/google/registry/tools/GetHistoryEntriesCommand.java @@ -17,7 +17,6 @@ package google.registry.tools; import static com.google.common.base.Preconditions.checkArgument; import static google.registry.util.DateTimeUtils.END_OF_TIME; import static google.registry.util.DateTimeUtils.START_OF_TIME; -import static google.registry.util.PreconditionsUtils.checkArgumentNotNull; import static org.joda.time.DateTimeZone.UTC; import com.beust.jcommander.Parameter; @@ -60,7 +59,6 @@ final class GetHistoryEntriesCommand implements Command { type != null && uniqueId != null, "If either of 'type' or 'id' are set then both must be"); VKey parentKey = type.getKey(uniqueId, DateTime.now(UTC)); - checkArgumentNotNull(parentKey, "Invalid resource ID"); historyEntries = HistoryEntryDao.loadHistoryObjectsForResource(parentKey, after, before); } else { historyEntries = HistoryEntryDao.loadAllHistoryObjects(after, before); diff --git a/core/src/main/java/google/registry/tools/RenewDomainCommand.java b/core/src/main/java/google/registry/tools/RenewDomainCommand.java index c5d569cea..81f262634 100644 --- a/core/src/main/java/google/registry/tools/RenewDomainCommand.java +++ b/core/src/main/java/google/registry/tools/RenewDomainCommand.java @@ -18,19 +18,17 @@ import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Strings.isNullOrEmpty; import static google.registry.util.CollectionUtils.findDuplicates; import static google.registry.util.PreconditionsUtils.checkArgumentNotNull; -import static google.registry.util.PreconditionsUtils.checkArgumentPresent; import com.beust.jcommander.Parameter; import com.beust.jcommander.Parameters; import com.google.common.base.Joiner; import com.google.template.soy.data.SoyMapData; -import google.registry.model.ForeignKeyUtils; +import google.registry.flows.ResourceFlowUtils; import google.registry.model.domain.Domain; import google.registry.tools.soy.DomainRenewSoyInfo; import google.registry.util.Clock; import jakarta.inject.Inject; import java.util.List; -import java.util.Optional; import org.joda.time.DateTime; import org.joda.time.format.DateTimeFormat; import org.joda.time.format.DateTimeFormatter; @@ -71,17 +69,15 @@ final class RenewDomainCommand extends MutatingEppToolCommand { private static final DateTimeFormatter DATE_FORMATTER = DateTimeFormat.forPattern("YYYY-MM-dd"); @Override - protected void initMutatingEppToolCommand() { + protected void initMutatingEppToolCommand() + throws ResourceFlowUtils.ResourceDoesNotExistException { String duplicates = Joiner.on(", ").join(findDuplicates(mainParameters)); checkArgument(duplicates.isEmpty(), "Duplicate domain arguments found: '%s'", duplicates); checkArgument(period < 10, "Cannot renew domains for 10 or more years"); DateTime now = clock.nowUtc(); for (String domainName : mainParameters) { - Optional domainOptional = ForeignKeyUtils.loadResource(Domain.class, domainName, now); - checkArgumentPresent(domainOptional, "Domain '%s' does not exist or is deleted", domainName); + Domain domain = ResourceFlowUtils.loadAndVerifyExistence(Domain.class, domainName, now); setSoyTemplate(DomainRenewSoyInfo.getInstance(), DomainRenewSoyInfo.RENEWDOMAIN); - Domain domain = domainOptional.get(); - SoyMapData soyMapData = new SoyMapData( "domainName", domain.getDomainName(), diff --git a/core/src/main/java/google/registry/tools/UniformRapidSuspensionCommand.java b/core/src/main/java/google/registry/tools/UniformRapidSuspensionCommand.java index 329594cc3..1f3368ef2 100644 --- a/core/src/main/java/google/registry/tools/UniformRapidSuspensionCommand.java +++ b/core/src/main/java/google/registry/tools/UniformRapidSuspensionCommand.java @@ -17,9 +17,7 @@ package google.registry.tools; import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.collect.ImmutableList.toImmutableList; import static com.google.common.collect.Sets.difference; -import static google.registry.model.EppResourceUtils.checkResourcesExist; import static google.registry.persistence.transaction.TransactionManagerFactory.tm; -import static google.registry.util.PreconditionsUtils.checkArgumentPresent; import com.beust.jcommander.Parameter; import com.beust.jcommander.Parameters; @@ -30,6 +28,7 @@ import com.google.common.collect.ImmutableSet; import com.google.common.collect.ImmutableSortedSet; import com.google.template.soy.data.SoyListData; import com.google.template.soy.data.SoyMapData; +import google.registry.flows.ResourceFlowUtils; import google.registry.model.ForeignKeyUtils; import google.registry.model.domain.Domain; import google.registry.model.domain.secdns.DomainDsData; @@ -44,7 +43,6 @@ import jakarta.xml.bind.annotation.adapters.HexBinaryAdapter; import java.util.ArrayList; import java.util.HashSet; import java.util.List; -import java.util.Optional; import java.util.Set; import org.joda.time.DateTime; import org.joda.time.format.DateTimeFormat; @@ -127,13 +125,13 @@ final class UniformRapidSuspensionCommand extends MutatingEppToolCommand { @Inject Clock clock; @Override - protected void initMutatingEppToolCommand() { + protected void initMutatingEppToolCommand() + throws ResourceFlowUtils.ResourceDoesNotExistException { superuser = true; DateTime now = clock.nowUtc(); - Optional domainOpt = ForeignKeyUtils.loadResource(Domain.class, domainName, now); - checkArgumentPresent(domainOpt, "Domain '%s' does not exist or is deleted", domainName); - Domain domain = domainOpt.get(); - Set missingHosts = difference(newHosts, checkResourcesExist(Host.class, newHosts, now)); + Domain domain = ResourceFlowUtils.loadAndVerifyExistence(Domain.class, domainName, now); + Set missingHosts = + difference(newHosts, ForeignKeyUtils.loadKeys(Host.class, newHosts, now).keySet()); checkArgument(missingHosts.isEmpty(), "Hosts do not exist: %s", missingHosts); checkArgument( locksToPreserve.isEmpty() || undo, diff --git a/core/src/main/java/google/registry/tools/UnrenewDomainCommand.java b/core/src/main/java/google/registry/tools/UnrenewDomainCommand.java index 95aab1f7a..29613a8ec 100644 --- a/core/src/main/java/google/registry/tools/UnrenewDomainCommand.java +++ b/core/src/main/java/google/registry/tools/UnrenewDomainCommand.java @@ -87,7 +87,7 @@ class UnrenewDomainCommand extends ConfirmingCommand { new ImmutableMap.Builder<>(); for (String domainName : mainParameters) { - if (ForeignKeyUtils.load(Domain.class, domainName, START_OF_TIME) == null) { + if (ForeignKeyUtils.loadKey(Domain.class, domainName, START_OF_TIME).isEmpty()) { domainsNonexistentBuilder.add(domainName); continue; } diff --git a/core/src/main/java/google/registry/tools/UpdateDomainCommand.java b/core/src/main/java/google/registry/tools/UpdateDomainCommand.java index 0c8fb4b6a..0336d01c0 100644 --- a/core/src/main/java/google/registry/tools/UpdateDomainCommand.java +++ b/core/src/main/java/google/registry/tools/UpdateDomainCommand.java @@ -20,7 +20,6 @@ import static google.registry.model.domain.rgp.GracePeriodStatus.AUTO_RENEW; import static google.registry.model.eppcommon.StatusValue.PENDING_DELETE; import static google.registry.model.eppcommon.StatusValue.SERVER_UPDATE_PROHIBITED; import static google.registry.persistence.transaction.TransactionManagerFactory.tm; -import static google.registry.util.PreconditionsUtils.checkArgumentPresent; import static java.util.function.Predicate.isEqual; import com.beust.jcommander.Parameter; @@ -30,7 +29,7 @@ import com.google.common.collect.ImmutableSortedSet; import com.google.common.collect.Sets; import com.google.common.flogger.FluentLogger; import com.google.template.soy.data.SoyMapData; -import google.registry.model.ForeignKeyUtils; +import google.registry.flows.ResourceFlowUtils; import google.registry.model.domain.DesignatedContact; import google.registry.model.domain.Domain; import google.registry.model.domain.GracePeriodBase; @@ -42,7 +41,6 @@ import jakarta.inject.Inject; import java.util.ArrayList; import java.util.HashSet; import java.util.List; -import java.util.Optional; import java.util.Set; import java.util.TreeSet; import javax.annotation.Nullable; @@ -146,7 +144,8 @@ final class UpdateDomainCommand extends CreateOrUpdateDomainCommand { boolean forceInPendingDelete; @Override - protected void initMutatingEppToolCommand() { + protected void initMutatingEppToolCommand() + throws ResourceFlowUtils.ResourceDoesNotExistException { if (!nameservers.isEmpty()) { checkArgument( addNameservers.isEmpty() && removeNameservers.isEmpty(), @@ -184,9 +183,7 @@ final class UpdateDomainCommand extends CreateOrUpdateDomainCommand { ImmutableSet.Builder autorenewGracePeriodWarningDomains = new ImmutableSet.Builder<>(); DateTime now = clock.nowUtc(); for (String domainName : domains) { - Optional domainOptional = ForeignKeyUtils.loadResource(Domain.class, domainName, now); - checkArgumentPresent(domainOptional, "Domain '%s' does not exist or is deleted", domainName); - Domain domain = domainOptional.get(); + Domain domain = ResourceFlowUtils.loadAndVerifyExistence(Domain.class, domainName, now); checkArgument( !domain.getStatusValues().contains(SERVER_UPDATE_PROHIBITED), "The domain '%s' has status SERVER_UPDATE_PROHIBITED. Verify that you are allowed " diff --git a/core/src/main/java/google/registry/ui/server/console/ConsoleDomainGetAction.java b/core/src/main/java/google/registry/ui/server/console/ConsoleDomainGetAction.java index 5d2e6269b..faf43f189 100644 --- a/core/src/main/java/google/registry/ui/server/console/ConsoleDomainGetAction.java +++ b/core/src/main/java/google/registry/ui/server/console/ConsoleDomainGetAction.java @@ -18,7 +18,7 @@ import static google.registry.persistence.transaction.TransactionManagerFactory. import static jakarta.servlet.http.HttpServletResponse.SC_NOT_FOUND; import static jakarta.servlet.http.HttpServletResponse.SC_OK; -import google.registry.model.EppResourceUtils; +import google.registry.model.ForeignKeyUtils; import google.registry.model.console.ConsolePermission; import google.registry.model.console.User; import google.registry.model.domain.Domain; @@ -55,7 +55,7 @@ public class ConsoleDomainGetAction extends ConsoleApiAction { Optional possibleDomain = tm().transact( () -> - EppResourceUtils.loadByForeignKeyByCacheIfEnabled( + ForeignKeyUtils.loadResourceByCacheIfEnabled( Domain.class, paramDomain, tm().getTransactionTime())); if (possibleDomain.isEmpty()) { consoleApiParams.response().setStatus(SC_NOT_FOUND); diff --git a/core/src/test/java/google/registry/bsa/BsaValidateActionTest.java b/core/src/test/java/google/registry/bsa/BsaValidateActionTest.java index ea3f92e3b..16bd37e23 100644 --- a/core/src/test/java/google/registry/bsa/BsaValidateActionTest.java +++ b/core/src/test/java/google/registry/bsa/BsaValidateActionTest.java @@ -239,7 +239,7 @@ public class BsaValidateActionTest { persistBsaLabel("label"); Domain domain = persistActiveDomain("label.app", fakeClock.nowUtc()); fakeClock.advanceBy(MAX_STALENESS.minus(Duration.standardSeconds(1))); - assertThat(action.isStalenessAllowed(domain.createVKey())).isTrue(); + assertThat(action.isStalenessAllowed(domain)).isTrue(); } @Test @@ -247,7 +247,7 @@ public class BsaValidateActionTest { persistBsaLabel("label"); Domain domain = persistActiveDomain("label.app", fakeClock.nowUtc()); fakeClock.advanceBy(MAX_STALENESS); - assertThat(action.isStalenessAllowed(domain.createVKey())).isFalse(); + assertThat(action.isStalenessAllowed(domain)).isFalse(); } @Test diff --git a/core/src/test/java/google/registry/flows/domain/DomainDeleteFlowTest.java b/core/src/test/java/google/registry/flows/domain/DomainDeleteFlowTest.java index a641539dd..c84d7b012 100644 --- a/core/src/test/java/google/registry/flows/domain/DomainDeleteFlowTest.java +++ b/core/src/test/java/google/registry/flows/domain/DomainDeleteFlowTest.java @@ -752,11 +752,7 @@ class DomainDeleteFlowTest extends ResourceFlowTestCase { Host renamedHost = doSuccessfulTest(); assertThat(renamedHost.isSubordinate()).isTrue(); assertHostDnsRequests("ns1.example.tld", "ns2.example.tld"); - VKey oldVKeyAfterRename = ForeignKeyUtils.load(Host.class, oldHostName(), clock.nowUtc()); - assertThat(oldVKeyAfterRename).isNull(); + assertThat(ForeignKeyUtils.loadKey(Host.class, oldHostName(), clock.nowUtc())).isEmpty(); } @Test diff --git a/core/src/test/java/google/registry/model/ForeignKeyUtilsTest.java b/core/src/test/java/google/registry/model/ForeignKeyUtilsTest.java index 165599567..a3e620a3e 100644 --- a/core/src/test/java/google/registry/model/ForeignKeyUtilsTest.java +++ b/core/src/test/java/google/registry/model/ForeignKeyUtilsTest.java @@ -48,7 +48,10 @@ class ForeignKeyUtilsTest { @RegisterExtension public final TestCacheExtension testCacheExtension = - new TestCacheExtension.Builder().withForeignKeyCache(Duration.ofDays(1)).build(); + new TestCacheExtension.Builder() + .withForeignKeyRepoIdCache(Duration.ofDays(1)) + .withForeignKeyResourceCache(Duration.ofDays(1)) + .build(); @BeforeEach void setUp() { @@ -56,46 +59,48 @@ class ForeignKeyUtilsTest { } @Test - void testSuccess_loadHost() { + void testSuccess_loadHostKey() { Host host = persistActiveHost("ns1.example.com"); - assertThat(ForeignKeyUtils.load(Host.class, "ns1.example.com", fakeClock.nowUtc())) - .isEqualTo(host.createVKey()); + assertThat(ForeignKeyUtils.loadKey(Host.class, "ns1.example.com", fakeClock.nowUtc())) + .hasValue(host.createVKey()); } @Test - void testSuccess_loadDomain() { + void testSuccess_loadDomainKey() { Domain domain = persistActiveDomain("example.com"); - assertThat(ForeignKeyUtils.load(Domain.class, "example.com", fakeClock.nowUtc())) - .isEqualTo(domain.createVKey()); + assertThat(ForeignKeyUtils.loadKey(Domain.class, "example.com", fakeClock.nowUtc())) + .hasValue(domain.createVKey()); } @Test - void testSuccess_loadContact() { + void testSuccess_loadContactKey() { Contact contact = persistActiveContact("john-doe"); - assertThat(ForeignKeyUtils.load(Contact.class, "john-doe", fakeClock.nowUtc())) - .isEqualTo(contact.createVKey()); + assertThat(ForeignKeyUtils.loadKey(Contact.class, "john-doe", fakeClock.nowUtc())) + .hasValue(contact.createVKey()); } @Test - void testSuccess_loadMostRecentResource() { + void testSuccess_loadKeyMostRecentResource() { Host host = persistActiveHost("ns1.example.com"); persistResource(host.asBuilder().setDeletionTime(fakeClock.nowUtc().minusDays(1)).build()); fakeClock.advanceOneMilli(); Host newHost = persistActiveHost("ns1.example.com"); - assertThat(ForeignKeyUtils.load(Host.class, "ns1.example.com", fakeClock.nowUtc())) - .isEqualTo(newHost.createVKey()); + assertThat(ForeignKeyUtils.loadKey(Host.class, "ns1.example.com", fakeClock.nowUtc())) + .hasValue(newHost.createVKey()); } @Test - void testSuccess_loadNonexistentForeignKey_returnsNull() { - assertThat(ForeignKeyUtils.load(Host.class, "ns1.example.com", fakeClock.nowUtc())).isNull(); + void testSuccess_loadKeyNonexistentForeignKey_returnsNull() { + assertThat(ForeignKeyUtils.loadKey(Host.class, "ns1.example.com", fakeClock.nowUtc())) + .isEmpty(); } @Test - void testSuccess_loadDeletedForeignKey_returnsNull() { + void testSuccess_loadKeyDeletedForeignKey_returnsNull() { Host host = persistActiveHost("ns1.example.com"); persistResource(host.asBuilder().setDeletionTime(fakeClock.nowUtc().minusDays(1)).build()); - assertThat(ForeignKeyUtils.load(Host.class, "ns1.example.com", fakeClock.nowUtc())).isNull(); + assertThat(ForeignKeyUtils.loadKey(Host.class, "ns1.example.com", fakeClock.nowUtc())) + .isEmpty(); } @Test @@ -103,16 +108,17 @@ class ForeignKeyUtilsTest { Host host1 = persistActiveHost("ns1.example.com"); fakeClock.advanceOneMilli(); persistResource(host1.asBuilder().setDeletionTime(fakeClock.nowUtc()).build()); - assertThat(ForeignKeyUtils.load(Host.class, "ns1.example.com", fakeClock.nowUtc())).isNull(); + assertThat(ForeignKeyUtils.loadKey(Host.class, "ns1.example.com", fakeClock.nowUtc())) + .isEmpty(); } @Test - void testSuccess_batchLoad_skipsDeletedAndNonexistent() { + void testSuccess_batchLoadKeys_skipsDeletedAndNonexistent() { Host host1 = persistActiveHost("ns1.example.com"); Host host2 = persistActiveHost("ns2.example.com"); persistResource(host2.asBuilder().setDeletionTime(fakeClock.nowUtc().minusDays(1)).build()); assertThat( - ForeignKeyUtils.load( + ForeignKeyUtils.loadKeys( Host.class, ImmutableList.of("ns1.example.com", "ns2.example.com", "ns3.example.com"), fakeClock.nowUtc())) @@ -121,7 +127,7 @@ class ForeignKeyUtilsTest { fakeClock.advanceOneMilli(); Host newHost1 = persistActiveHost("ns1.example.com"); assertThat( - ForeignKeyUtils.loadByCacheIfEnabled( + ForeignKeyUtils.loadKeysByCacheIfEnabled( Host.class, ImmutableList.of("ns1.example.com", "ns2.example.com", "ns3.example.com"), fakeClock.nowUtc())) @@ -129,12 +135,12 @@ class ForeignKeyUtilsTest { } @Test - void testSuccess_loadHostsCached_cacheIsStale() { + void testSuccess_loadHostKeysCached_cacheIsStale() { Host host1 = persistActiveHost("ns1.example.com"); Host host2 = persistActiveHost("ns2.example.com"); persistResource(host2.asBuilder().setDeletionTime(fakeClock.nowUtc().minusDays(1)).build()); assertThat( - ForeignKeyUtils.loadByCacheIfEnabled( + ForeignKeyUtils.loadKeysByCacheIfEnabled( Host.class, ImmutableList.of("ns1.example.com", "ns2.example.com", "ns3.example.com"), fakeClock.nowUtc())) @@ -144,7 +150,7 @@ class ForeignKeyUtilsTest { persistActiveHost("ns1.example.com"); // Even though a new host1 is now live, the cache still returns the VKey to the old one. assertThat( - ForeignKeyUtils.loadByCacheIfEnabled( + ForeignKeyUtils.loadKeysByCacheIfEnabled( Host.class, ImmutableList.of("ns1.example.com", "ns2.example.com", "ns3.example.com"), fakeClock.nowUtc())) diff --git a/core/src/test/java/google/registry/testing/TestCacheExtension.java b/core/src/test/java/google/registry/testing/TestCacheExtension.java index cdfd3cb21..267177cb4 100644 --- a/core/src/test/java/google/registry/testing/TestCacheExtension.java +++ b/core/src/test/java/google/registry/testing/TestCacheExtension.java @@ -60,8 +60,13 @@ public class TestCacheExtension implements BeforeEachCallback, AfterEachCallback return this; } - public Builder withForeignKeyCache(Duration expiry) { - cacheHandlers.add(new TestCacheHandler(ForeignKeyUtils::setCacheForTest, expiry)); + public Builder withForeignKeyRepoIdCache(Duration expiry) { + cacheHandlers.add(new TestCacheHandler(ForeignKeyUtils::setRepoIdCacheForTest, expiry)); + return this; + } + + public Builder withForeignKeyResourceCache(Duration expiry) { + cacheHandlers.add(new TestCacheHandler(ForeignKeyUtils::setResourceCacheForTest, expiry)); return this; } diff --git a/core/src/test/java/google/registry/tmch/NordnUploadActionTest.java b/core/src/test/java/google/registry/tmch/NordnUploadActionTest.java index 66ddd89b9..d1e09a9bb 100644 --- a/core/src/test/java/google/registry/tmch/NordnUploadActionTest.java +++ b/core/src/test/java/google/registry/tmch/NordnUploadActionTest.java @@ -19,9 +19,7 @@ import static com.google.common.net.HttpHeaders.CONTENT_TYPE; import static com.google.common.net.HttpHeaders.LOCATION; import static com.google.common.net.MediaType.FORM_DATA; import static com.google.common.truth.Truth.assertThat; -import static google.registry.model.ForeignKeyUtils.load; import static google.registry.testing.DatabaseHelper.createTld; -import static google.registry.testing.DatabaseHelper.loadByKey; import static google.registry.testing.DatabaseHelper.loadRegistrar; import static google.registry.testing.DatabaseHelper.newDomain; import static google.registry.testing.DatabaseHelper.persistResource; @@ -40,10 +38,10 @@ import static org.mockito.Mockito.when; import com.google.common.base.VerifyException; import google.registry.batch.CloudTasksUtils; +import google.registry.model.ForeignKeyUtils; import google.registry.model.domain.Domain; import google.registry.model.domain.launch.LaunchNotice; import google.registry.model.tld.Tld; -import google.registry.persistence.VKey; import google.registry.persistence.transaction.JpaTestExtensions; import google.registry.persistence.transaction.JpaTestExtensions.JpaIntegrationTestExtension; import google.registry.request.RequestParameters; @@ -72,19 +70,19 @@ class NordnUploadActionTest { private static final String CLAIMS_CSV = """ - 1,2010-05-04T10:11:12.000Z,2 - roid,domain-name,notice-id,registrar-id,registration-datetime,ack-datetime,application-datetime - 6-TLD,claims-landrush2.tld,landrush2tcn,88888,2010-05-03T10:11:12.000Z,2010-05-03T08:11:12.000Z - 8-TLD,claims-landrush1.tld,landrush1tcn,99999,2010-05-04T10:11:12.000Z,2010-05-04T09:11:12.000Z - """; + 1,2010-05-04T10:11:12.000Z,2 + roid,domain-name,notice-id,registrar-id,registration-datetime,ack-datetime,application-datetime + 6-TLD,claims-landrush2.tld,landrush2tcn,88888,2010-05-03T10:11:12.000Z,2010-05-03T08:11:12.000Z + 8-TLD,claims-landrush1.tld,landrush1tcn,99999,2010-05-04T10:11:12.000Z,2010-05-04T09:11:12.000Z + """; private static final String SUNRISE_CSV = """ - 1,2010-05-04T10:11:12.000Z,2 - roid,domain-name,SMD-id,registrar-id,registration-datetime,application-datetime - 2-TLD,sunrise2.tld,new-smdid,88888,2010-05-01T10:11:12.000Z - 4-TLD,sunrise1.tld,my-smdid,99999,2010-05-02T10:11:12.000Z - """; + 1,2010-05-04T10:11:12.000Z,2 + roid,domain-name,SMD-id,registrar-id,registration-datetime,application-datetime + 2-TLD,sunrise2.tld,new-smdid,88888,2010-05-01T10:11:12.000Z + 4-TLD,sunrise1.tld,my-smdid,99999,2010-05-02T10:11:12.000Z + """; private static final String LOCATION_URL = "http://trololol"; @@ -142,12 +140,14 @@ class NordnUploadActionTest { @Test void testSuccess_nothingScheduled() { persistResource( - loadByKey(load(Domain.class, "claims-landrush1.tld", clock.nowUtc())) + ForeignKeyUtils.loadResource(Domain.class, "claims-landrush1.tld", clock.nowUtc()) + .get() .asBuilder() .setLordnPhase(LordnPhase.NONE) .build()); persistResource( - loadByKey(load(Domain.class, "claims-landrush2.tld", clock.nowUtc())) + ForeignKeyUtils.loadResource(Domain.class, "claims-landrush2.tld", clock.nowUtc()) + .get() .asBuilder() .setLordnPhase(LordnPhase.NONE) .build()); @@ -233,8 +233,7 @@ class NordnUploadActionTest { } private void verifyColumnCleared(String domainName) { - VKey domainKey = load(Domain.class, domainName, clock.nowUtc()); - Domain domain = loadByKey(domainKey); + Domain domain = ForeignKeyUtils.loadResource(Domain.class, domainName, clock.nowUtc()).get(); assertThat(domain.getLordnPhase()).isEqualTo(LordnPhase.NONE); } diff --git a/core/src/test/java/google/registry/tools/EnqueuePollMessageCommandTest.java b/core/src/test/java/google/registry/tools/EnqueuePollMessageCommandTest.java index 85b29dd0a..f80213456 100644 --- a/core/src/test/java/google/registry/tools/EnqueuePollMessageCommandTest.java +++ b/core/src/test/java/google/registry/tools/EnqueuePollMessageCommandTest.java @@ -169,13 +169,14 @@ class EnqueuePollMessageCommandTest extends CommandTestCase runCommandForced("--domain=example2.tld", "--message=This domain needs help")); assertThat(thrown) + .hasCauseThat() .hasMessageThat() - .isEqualTo("Domain example2.tld doesn't exist or isn't active"); + .isEqualTo("The domain with given ID (example2.tld) doesn't exist."); } @Test diff --git a/core/src/test/java/google/registry/tools/RenewDomainCommandTest.java b/core/src/test/java/google/registry/tools/RenewDomainCommandTest.java index 3fece1d76..800548609 100644 --- a/core/src/test/java/google/registry/tools/RenewDomainCommandTest.java +++ b/core/src/test/java/google/registry/tools/RenewDomainCommandTest.java @@ -24,6 +24,7 @@ import static org.junit.jupiter.api.Assertions.assertThrows; import com.beust.jcommander.ParameterException; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; +import google.registry.flows.ResourceFlowUtils; import google.registry.model.domain.Domain; import google.registry.model.registrar.Registrar; import google.registry.testing.DatabaseHelper; @@ -174,19 +175,23 @@ public class RenewDomainCommandTest extends EppToolCommandTestCase runCommandForced("nonexistent.tld")); - assertThat(e) + assertThat( + assertThrows( + ResourceFlowUtils.ResourceDoesNotExistException.class, + () -> runCommandForced("nonexistent.tld"))) .hasMessageThat() - .isEqualTo("Domain 'nonexistent.tld' does not exist or is deleted"); + .isEqualTo("The domain with given ID (nonexistent.tld) doesn't exist."); } @Test void testFailure_domainIsDeleted() { persistDeletedDomain("deleted.tld", DateTime.parse("2012-10-05T05:05:05Z")); - IllegalArgumentException e = - assertThrows(IllegalArgumentException.class, () -> runCommandForced("deleted.tld")); - assertThat(e).hasMessageThat().isEqualTo("Domain 'deleted.tld' does not exist or is deleted"); + assertThat( + assertThrows( + ResourceFlowUtils.ResourceDoesNotExistException.class, + () -> runCommandForced("deleted.tld"))) + .hasMessageThat() + .isEqualTo("The domain with given ID (deleted.tld) doesn't exist."); } @Test