diff --git a/core/src/main/java/google/registry/config/RegistryConfig.java b/core/src/main/java/google/registry/config/RegistryConfig.java index 97fda5615..3280d42ed 100644 --- a/core/src/main/java/google/registry/config/RegistryConfig.java +++ b/core/src/main/java/google/registry/config/RegistryConfig.java @@ -1427,6 +1427,11 @@ public final class RegistryConfig { return CONFIG_SETTINGS.get().caching.eppResourceMaxCachedEntries; } + /** Returns the amount of time that a particular claims list should be cached. */ + public static java.time.Duration getClaimsListCacheDuration() { + return java.time.Duration.ofSeconds(CONFIG_SETTINGS.get().caching.claimsListCachingSeconds); + } + /** Returns the email address that outgoing emails from the app are sent from. */ public static InternetAddress getGSuiteOutgoingEmailAddress() { return parseEmailAddress(CONFIG_SETTINGS.get().gSuite.outgoingEmailAddress); diff --git a/core/src/main/java/google/registry/config/RegistryConfigSettings.java b/core/src/main/java/google/registry/config/RegistryConfigSettings.java index 0f3c029b5..be48858e2 100644 --- a/core/src/main/java/google/registry/config/RegistryConfigSettings.java +++ b/core/src/main/java/google/registry/config/RegistryConfigSettings.java @@ -155,6 +155,7 @@ public class RegistryConfigSettings { public boolean eppResourceCachingEnabled; public int eppResourceCachingSeconds; public int eppResourceMaxCachedEntries; + public int claimsListCachingSeconds; } /** Configuration for ICANN monthly reporting. */ diff --git a/core/src/main/java/google/registry/config/files/default-config.yaml b/core/src/main/java/google/registry/config/files/default-config.yaml index 49c8a29a9..36118a77c 100644 --- a/core/src/main/java/google/registry/config/files/default-config.yaml +++ b/core/src/main/java/google/registry/config/files/default-config.yaml @@ -294,6 +294,10 @@ caching: # have to be very large to achieve the vast majority of possible gains. eppResourceMaxCachedEntries: 500 + # Length of time that a claims list will be cached after retrieval. A fairly + # long duration is acceptable because claims lists don't change frequently. + claimsListCachingSeconds: 21600 # six hours + oAuth: # OAuth scopes to detect on access tokens. Superset of requiredOauthScopes. availableOauthScopes: diff --git a/core/src/main/java/google/registry/model/tld/label/PremiumListDao.java b/core/src/main/java/google/registry/model/tld/label/PremiumListDao.java index 6118ff8e1..021aa81f1 100644 --- a/core/src/main/java/google/registry/model/tld/label/PremiumListDao.java +++ b/core/src/main/java/google/registry/model/tld/label/PremiumListDao.java @@ -73,7 +73,7 @@ public class PremiumListDao { } /** - * In-memory price cache for for a given premium list revision and domain label. + * In-memory price cache for a given premium list revision and domain label. * *

Note that premium list revision ids are globally unique, so this cache is specific to a * given premium list. Premium list entries might not be present, as indicated by the Optional diff --git a/core/src/main/java/google/registry/model/tld/label/ReservedList.java b/core/src/main/java/google/registry/model/tld/label/ReservedList.java index 75f1ee129..297409fb0 100644 --- a/core/src/main/java/google/registry/model/tld/label/ReservedList.java +++ b/core/src/main/java/google/registry/model/tld/label/ReservedList.java @@ -90,12 +90,13 @@ public final class ReservedList * *

We need to persist the list entries, but only on the initial insert (not on update) since * the entries themselves never get changed, so we only annotate it with {@link PostPersist}, not - * {@link PostUpdate}. + * PostUpdate. */ @PostPersist void postPersist() { if (reservedListMap != null) { - reservedListMap.values().stream() + reservedListMap + .values() .forEach( entry -> { // We can safely change the revision id since it's "Insignificant". diff --git a/core/src/main/java/google/registry/model/tmch/ClaimsList.java b/core/src/main/java/google/registry/model/tmch/ClaimsList.java index 4cc23702f..f4c428d1e 100644 --- a/core/src/main/java/google/registry/model/tmch/ClaimsList.java +++ b/core/src/main/java/google/registry/model/tmch/ClaimsList.java @@ -20,7 +20,10 @@ import static com.google.common.collect.ImmutableMap.toImmutableMap; import static google.registry.persistence.transaction.QueryComposer.Comparator.EQ; import static google.registry.persistence.transaction.TransactionManagerFactory.jpaTm; +import com.github.benmanes.caffeine.cache.LoadingCache; +import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.ImmutableMap; +import google.registry.model.CacheUtils; import google.registry.model.CreateAutoTimestamp; import google.registry.model.ImmutableObject; import java.util.Map; @@ -81,9 +84,26 @@ public class ClaimsList extends ImmutableObject { *

This field requires special treatment since we want to lazy load it. We have to remove it * from the immutability contract so we can modify it after construction and we have to handle the * database processing on our own so we can detach it after load. + * + *

Unlike the cache below, this is guaranteed to contain all mappings from labels to claim keys + * if it is not null. */ @Insignificant @Transient ImmutableMap labelsToKeys; + /** + * A not-necessarily-complete cache of labels to claim keys. + * + *

At any point in time this might or might not contain none, some, or all of the mappings from + * labels to claim keys. Exists so that repeated calls to {@link #getClaimKey(String)} can be + * quick. + * + *

Note: this cache has no expiration because while claims list revisions can be added over + * time, each instance of a claims list is immutable. + */ + @Insignificant @Transient @VisibleForTesting + final LoadingCache> claimKeyCache = + CacheUtils.newCacheBuilder().build(this::getClaimKeyUncached); + @PreRemove void preRemove() { jpaTm() @@ -139,18 +159,7 @@ public class ClaimsList extends ImmutableObject { * entries and cache them locally. */ public Optional getClaimKey(String label) { - if (labelsToKeys != null) { - return Optional.ofNullable(labelsToKeys.get(label)); - } - return jpaTm() - .transact( - () -> - jpaTm() - .createQueryComposer(ClaimsEntry.class) - .where("revisionId", EQ, revisionId) - .where("domainLabel", EQ, label) - .first() - .map(ClaimsEntry::getClaimKey)); + return claimKeyCache.get(label); } /** @@ -192,6 +201,27 @@ public class ClaimsList extends ImmutableObject { return labelsToKeys.size(); } + /** + * Returns the claim key for a given domain if there is one, empty otherwise. + * + *

This attempts to load from the base {@link #labelsToKeys} if possible, otherwise it will + * query the database for the entry requested. + */ + private Optional getClaimKeyUncached(String label) { + if (labelsToKeys != null) { + return Optional.ofNullable(labelsToKeys.get(label)); + } + return jpaTm() + .transact( + () -> + jpaTm() + .createQueryComposer(ClaimsEntry.class) + .where("revisionId", EQ, revisionId) + .where("domainLabel", EQ, label) + .first() + .map(ClaimsEntry::getClaimKey)); + } + public static ClaimsList create( DateTime tmdbGenerationTime, ImmutableMap labelsToKeys) { ClaimsList instance = new ClaimsList(); diff --git a/core/src/main/java/google/registry/model/tmch/ClaimsListDao.java b/core/src/main/java/google/registry/model/tmch/ClaimsListDao.java index ff7df8671..69697a078 100644 --- a/core/src/main/java/google/registry/model/tmch/ClaimsListDao.java +++ b/core/src/main/java/google/registry/model/tmch/ClaimsListDao.java @@ -14,25 +14,57 @@ package google.registry.model.tmch; +import static google.registry.config.RegistryConfig.getClaimsListCacheDuration; import static google.registry.persistence.transaction.QueryComposer.Comparator.EQ; import static google.registry.persistence.transaction.TransactionManagerFactory.jpaTm; import static google.registry.util.DateTimeUtils.START_OF_TIME; +import com.github.benmanes.caffeine.cache.LoadingCache; +import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.ImmutableMap; +import google.registry.model.CacheUtils; +import java.time.Duration; +import java.util.Optional; /** Data access object for {@link ClaimsList}. */ public class ClaimsListDao { + /** + * Cache of the {@link ClaimsList} instance. + * + *

The key is meaningless since we only have one active claims list, this is essentially a + * memoizing Supplier that can be reset. + */ + @VisibleForTesting + static LoadingCache, ClaimsList> CACHE = + createCache(getClaimsListCacheDuration()); + + @VisibleForTesting + public static void setCacheForTest(Optional expiry) { + Duration effectiveExpiry = expiry.orElse(getClaimsListCacheDuration()); + CACHE = createCache(effectiveExpiry); + } + + private static LoadingCache, ClaimsList> createCache(Duration expiry) { + return CacheUtils.newCacheBuilder(expiry).build(ignored -> ClaimsListDao.getUncached()); + } + /** Saves the given {@link ClaimsList} to Cloud SQL. */ public static void save(ClaimsList claimsList) { jpaTm().transact(() -> jpaTm().insert(claimsList)); + CACHE.put(ClaimsListDao.class, claimsList); + } + + /** Returns the most recent revision of the {@link ClaimsList} from the cache. */ + public static ClaimsList get() { + return CACHE.get(ClaimsListDao.class); } /** * Returns the most recent revision of the {@link ClaimsList} in SQL or an empty list if it * doesn't exist. */ - public static ClaimsList get() { + private static ClaimsList getUncached() { return jpaTm() .transact( () -> { diff --git a/core/src/main/java/google/registry/tools/GetClaimsListCommand.java b/core/src/main/java/google/registry/tools/GetClaimsListCommand.java index 67dcd5e6f..272b0aa9a 100644 --- a/core/src/main/java/google/registry/tools/GetClaimsListCommand.java +++ b/core/src/main/java/google/registry/tools/GetClaimsListCommand.java @@ -30,8 +30,8 @@ import java.nio.file.Paths; /** * A command to download the current claims list. * - *

This is not the original file we fetched from TMCH. It is just a representation of what we - * are currently storing in Datastore. + *

This is not the original file we fetched from TMCH. It is just a representation of what we are + * currently storing in SQL. */ @Parameters(separators = " =", commandDescription = "Download the current claims list") final class GetClaimsListCommand implements CommandWithRemoteApi { diff --git a/core/src/test/java/google/registry/flows/domain/DomainClaimsCheckFlowTest.java b/core/src/test/java/google/registry/flows/domain/DomainClaimsCheckFlowTest.java index b49904ac0..c1c5a3a90 100644 --- a/core/src/test/java/google/registry/flows/domain/DomainClaimsCheckFlowTest.java +++ b/core/src/test/java/google/registry/flows/domain/DomainClaimsCheckFlowTest.java @@ -41,13 +41,20 @@ import google.registry.flows.exceptions.TooManyResourceChecksException; import google.registry.model.domain.Domain; import google.registry.model.tld.Registry; import google.registry.model.tld.Registry.TldState; +import google.registry.testing.TestCacheExtension; +import java.time.Duration; import org.joda.money.Money; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.RegisterExtension; /** Unit tests for {@link DomainClaimsCheckFlow}. */ public class DomainClaimsCheckFlowTest extends ResourceFlowTestCase { + @RegisterExtension + public final TestCacheExtension testCacheExtension = + new TestCacheExtension.Builder().withClaimsListCache(Duration.ofHours(6)).build(); + DomainClaimsCheckFlowTest() { setEppInput("domain_check_claims.xml"); } diff --git a/core/src/test/java/google/registry/model/tmch/ClaimsListDaoTest.java b/core/src/test/java/google/registry/model/tmch/ClaimsListDaoTest.java index 973dda958..43ccc4aac 100644 --- a/core/src/test/java/google/registry/model/tmch/ClaimsListDaoTest.java +++ b/core/src/test/java/google/registry/model/tmch/ClaimsListDaoTest.java @@ -15,12 +15,16 @@ package google.registry.model.tmch; import static com.google.common.truth.Truth.assertThat; +import static google.registry.persistence.transaction.TransactionManagerFactory.jpaTm; import static org.junit.jupiter.api.Assertions.assertThrows; import com.google.common.collect.ImmutableMap; +import com.google.common.truth.Truth8; import google.registry.persistence.transaction.JpaTestExtensions; import google.registry.persistence.transaction.JpaTestExtensions.JpaIntegrationWithCoverageExtension; import google.registry.testing.FakeClock; +import google.registry.testing.TestCacheExtension; +import java.time.Duration; import javax.persistence.PersistenceException; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.RegisterExtension; @@ -37,6 +41,11 @@ public class ClaimsListDaoTest { .withoutCannedData() .buildIntegrationWithCoverageExtension(); + // Set long persist times on the cache so it can be tested (cache times default to 0 in tests). + @RegisterExtension + public final TestCacheExtension testCacheExtension = + new TestCacheExtension.Builder().withClaimsListCache(Duration.ofHours(6)).build(); + @Test void save_insertsClaimsListSuccessfully() { ClaimsList claimsList = @@ -83,6 +92,40 @@ public class ClaimsListDaoTest { assertClaimsListEquals(newClaimsList, ClaimsListDao.get()); } + @Test + void testDaoCaching_savesAndUpdates() { + assertThat(ClaimsListDao.CACHE.getIfPresent(ClaimsListDao.class)).isNull(); + ClaimsList oldList = + ClaimsList.create(fakeClock.nowUtc(), ImmutableMap.of("label1", "key1", "label2", "key2")); + ClaimsListDao.save(oldList); + assertThat(ClaimsListDao.CACHE.getIfPresent(ClaimsListDao.class)).isEqualTo(oldList); + ClaimsList newList = + ClaimsList.create(fakeClock.nowUtc(), ImmutableMap.of("label3", "key3", "label4", "key4")); + ClaimsListDao.save(newList); + assertThat(ClaimsListDao.CACHE.getIfPresent(ClaimsListDao.class)).isEqualTo(newList); + } + + @Test + void testEntryCaching_savesAndUpdates() { + ClaimsList claimsList = + ClaimsList.create(fakeClock.nowUtc(), ImmutableMap.of("label1", "key1", "label2", "key2")); + // Bypass the DAO to avoid the cache + jpaTm().transact(() -> jpaTm().insert(claimsList)); + ClaimsList fromDatabase = ClaimsListDao.get(); + // At first, we haven't loaded any entries + assertThat(fromDatabase.claimKeyCache.getIfPresent("label1")).isNull(); + Truth8.assertThat(fromDatabase.getClaimKey("label1")).hasValue("key1"); + // After retrieval, the key exists + Truth8.assertThat(fromDatabase.claimKeyCache.getIfPresent("label1")).hasValue("key1"); + assertThat(fromDatabase.claimKeyCache.getIfPresent("label2")).isNull(); + // Loading labels-to-keys should still work + assertThat(fromDatabase.getLabelsToKeys()).containsExactly("label1", "key1", "label2", "key2"); + // We should also cache nonexistent values + assertThat(fromDatabase.claimKeyCache.getIfPresent("nonexistent")).isNull(); + Truth8.assertThat(fromDatabase.getClaimKey("nonexistent")).isEmpty(); + Truth8.assertThat(fromDatabase.claimKeyCache.getIfPresent("nonexistent")).isEmpty(); + } + private void assertClaimsListEquals(ClaimsList left, ClaimsList right) { assertThat(left.getRevisionId()).isEqualTo(right.getRevisionId()); assertThat(left.getTmdbGenerationTime()).isEqualTo(right.getTmdbGenerationTime()); diff --git a/core/src/test/java/google/registry/testing/TestCacheExtension.java b/core/src/test/java/google/registry/testing/TestCacheExtension.java index 1ad79756a..2c5c06673 100644 --- a/core/src/test/java/google/registry/testing/TestCacheExtension.java +++ b/core/src/test/java/google/registry/testing/TestCacheExtension.java @@ -19,6 +19,7 @@ import com.google.common.collect.Maps; import google.registry.model.EppResource; import google.registry.model.index.ForeignKeyIndex; import google.registry.model.tld.label.PremiumListDao; +import google.registry.model.tmch.ClaimsListDao; import java.time.Duration; import java.util.Map; import java.util.Optional; @@ -75,6 +76,12 @@ public class TestCacheExtension implements BeforeEachCallback, AfterEachCallback return this; } + public Builder withClaimsListCache(Duration expiry) { + cacheHandlerMap.put( + "ClaimsListDao.CACHE", new TestCacheHandler(ClaimsListDao::setCacheForTest, expiry)); + return this; + } + public TestCacheExtension build() { return new TestCacheExtension(ImmutableList.copyOf(cacheHandlerMap.values())); } diff --git a/core/src/test/java/google/registry/tools/EppLifecycleToolsTest.java b/core/src/test/java/google/registry/tools/EppLifecycleToolsTest.java index eed5f31e7..a149ee570 100644 --- a/core/src/test/java/google/registry/tools/EppLifecycleToolsTest.java +++ b/core/src/test/java/google/registry/tools/EppLifecycleToolsTest.java @@ -29,7 +29,9 @@ import google.registry.model.domain.Domain; import google.registry.model.domain.DomainHistory; import google.registry.model.reporting.HistoryEntry.Type; import google.registry.testing.AppEngineExtension; +import google.registry.testing.TestCacheExtension; import google.registry.util.Clock; +import java.time.Duration; import java.util.List; import org.joda.money.Money; import org.joda.time.DateTime; @@ -44,6 +46,10 @@ class EppLifecycleToolsTest extends EppTestCase { final AppEngineExtension appEngine = AppEngineExtension.builder().withClock(clock).withCloudSql().withTaskQueue().build(); + @RegisterExtension + public final TestCacheExtension testCacheExtension = + new TestCacheExtension.Builder().withClaimsListCache(Duration.ofHours(6)).build(); + @BeforeEach void beforeEach() { createTlds("example", "tld");