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