diff --git a/core/src/main/java/google/registry/batch/SyncRemoteCacheAction.java b/core/src/main/java/google/registry/batch/SyncRemoteCacheAction.java index 8877ac482..6ba502b24 100644 --- a/core/src/main/java/google/registry/batch/SyncRemoteCacheAction.java +++ b/core/src/main/java/google/registry/batch/SyncRemoteCacheAction.java @@ -63,25 +63,20 @@ public class SyncRemoteCacheAction implements Runnable { private final LockHandler lockHandler; private final Response response; - private final Optional> domainJedisClient; - private final Optional> hostJedisClient; + private final Optional jedisClient; @Inject public SyncRemoteCacheAction( - LockHandler lockHandler, - Response response, - Optional> domainJedisClient, - Optional> hostJedisClient) { + LockHandler lockHandler, Response response, Optional jedisClient) { this.lockHandler = lockHandler; this.response = response; - this.domainJedisClient = domainJedisClient; - this.hostJedisClient = hostJedisClient; + this.jedisClient = jedisClient; } @Override public void run() { response.setContentType(MediaType.PLAIN_TEXT_UTF_8); - if (domainJedisClient.isEmpty() || hostJedisClient.isEmpty()) { + if (jedisClient.isEmpty()) { response.setStatus(SC_NO_CONTENT); response.setPayload("No Jedis/Valkey configuration found"); return; @@ -134,7 +129,7 @@ public class SyncRemoteCacheAction implements Runnable { return 0; } logger.atInfo().log("Processing %d domains", domains.size()); - processResources(domainJedisClient.get(), domains, Domain::getDomainName); + processResources(Domain.class, domains, Domain::getDomainName); setNewCursorTime(domains, REMOTE_CACHE_DOMAIN_SYNC); return domains.size(); } @@ -154,13 +149,13 @@ public class SyncRemoteCacheAction implements Runnable { return 0; } logger.atInfo().log("Processing %d hosts", hosts.size()); - processResources(hostJedisClient.get(), hosts, Host::getRepoId); + processResources(Host.class, hosts, Host::getRepoId); setNewCursorTime(hosts, REMOTE_CACHE_HOST_SYNC); return hosts.size(); } private void processResources( - SimplifiedJedisClient jedisClient, List resources, Function getKeyFunction) { + Class clazz, List resources, Function getKeyFunction) { ImmutableList.Builder toDeleteBuilder = new ImmutableList.Builder<>(); ImmutableList.Builder> toSaveBuilder = new ImmutableList.Builder<>(); @@ -176,9 +171,9 @@ public class SyncRemoteCacheAction implements Runnable { ImmutableList toDelete = toDeleteBuilder.build(); ImmutableList> toSave = toSaveBuilder.build(); - jedisClient.deleteAll(toDelete); + jedisClient.get().deleteAll(clazz, toDelete); logger.atInfo().log("Invalidated %d from the remote cache", toDelete.size()); - jedisClient.setAll(toSave); + jedisClient.get().setAll(toSave); logger.atInfo().log("Set %d in the remote cache", toSave.size()); } diff --git a/core/src/main/java/google/registry/cache/CacheModule.java b/core/src/main/java/google/registry/cache/CacheModule.java index 3aee2ee8e..cd8e5d46e 100644 --- a/core/src/main/java/google/registry/cache/CacheModule.java +++ b/core/src/main/java/google/registry/cache/CacheModule.java @@ -85,22 +85,14 @@ public final class CacheModule { @Provides @Singleton - public static Optional> provideDomainJedisClient( - Optional jedis) { - return jedis.map(j -> SimplifiedJedisClient.create(Domain.class, j)); - } - - @Provides - @Singleton - public static Optional> provideHostJedisClient( - Optional jedis) { - return jedis.map(j -> SimplifiedJedisClient.create(Host.class, j)); + public static Optional provideJedisClient(Optional jedis) { + return jedis.map(SimplifiedJedisClient::new); } @Provides @Singleton public static DomainCache provideDomainCache( - Optional> domainJedisClient, Clock clock) { + Optional domainJedisClient, Clock clock) { if (domainJedisClient.isEmpty()) { return domainName -> ForeignKeyUtils.loadResourceByCache(Domain.class, domainName, clock.now()); @@ -110,7 +102,7 @@ public final class CacheModule { @Provides @Singleton - public static HostCache provideHostCache(Optional> hostJedisClient) { + public static HostCache provideHostCache(Optional hostJedisClient) { if (hostJedisClient.isEmpty()) { return repoId -> Optional.ofNullable(EppResource.loadByCache(VKey.create(Host.class, repoId))); diff --git a/core/src/main/java/google/registry/cache/MultilayerDomainCache.java b/core/src/main/java/google/registry/cache/MultilayerDomainCache.java index 5a364ba04..024c95102 100644 --- a/core/src/main/java/google/registry/cache/MultilayerDomainCache.java +++ b/core/src/main/java/google/registry/cache/MultilayerDomainCache.java @@ -32,14 +32,14 @@ public class MultilayerDomainCache extends MultilayerEppResourceCache private final Clock clock; - public MultilayerDomainCache(SimplifiedJedisClient jedisClient, Clock clock) { + public MultilayerDomainCache(SimplifiedJedisClient jedisClient, Clock clock) { super(jedisClient); this.clock = clock; } @Override public Optional loadByDomainName(String domainName) { - return loadFromCaches(domainName); + return loadFromCaches(Domain.class, domainName); } @Override @@ -56,11 +56,6 @@ public class MultilayerDomainCache extends MultilayerEppResourceCache .map(domain -> domain.cloneProjectedAtTime(now)); } - @Override - protected String getJedisPrefix() { - return "d_"; - } - @Override protected boolean shouldPersistToRemoteCache(Domain domain) { return Tld.get(domain.getTld()).getTldType().equals(Tld.TldType.REAL); diff --git a/core/src/main/java/google/registry/cache/MultilayerEppResourceCache.java b/core/src/main/java/google/registry/cache/MultilayerEppResourceCache.java index c6a314892..de53f60e4 100644 --- a/core/src/main/java/google/registry/cache/MultilayerEppResourceCache.java +++ b/core/src/main/java/google/registry/cache/MultilayerEppResourceCache.java @@ -35,21 +35,19 @@ public abstract class MultilayerEppResourceCache { .maximumSize(RegistryConfig.getEppResourceMaxCachedEntries()) .build(); - private final SimplifiedJedisClient jedisClient; + private final SimplifiedJedisClient jedisClient; - protected MultilayerEppResourceCache(SimplifiedJedisClient jedisClient) { + protected MultilayerEppResourceCache(SimplifiedJedisClient jedisClient) { this.jedisClient = jedisClient; } protected abstract Optional loadFromDatabase(String key); - protected abstract String getJedisPrefix(); - protected boolean shouldPersistToRemoteCache(V value) { return true; } - protected Optional loadFromCaches(String key) { + protected Optional loadFromCaches(Class clazz, String key) { // hopefully the resource is in the local cache Optional possibleValue = Optional.ofNullable(localCache.getIfPresent(key)); if (possibleValue.isPresent()) { @@ -57,8 +55,7 @@ public abstract class MultilayerEppResourceCache { } // if not, try the remote cache - String jedisKey = getJedisPrefix() + key; - possibleValue = jedisClient.get(jedisKey); + possibleValue = jedisClient.get(clazz, key); if (possibleValue.isPresent()) { localCache.put(key, possibleValue.get()); return possibleValue; @@ -70,7 +67,7 @@ public abstract class MultilayerEppResourceCache { v -> { // Optional has no direct "peek" functionality to fill the caches if (shouldPersistToRemoteCache(v)) { - jedisClient.set(new SimplifiedJedisClient.JedisResource<>(jedisKey, v)); + jedisClient.set(new SimplifiedJedisClient.JedisResource<>(key, v)); } localCache.put(key, v); return v; diff --git a/core/src/main/java/google/registry/cache/MultilayerHostCache.java b/core/src/main/java/google/registry/cache/MultilayerHostCache.java index 87660d238..8bb21be7d 100644 --- a/core/src/main/java/google/registry/cache/MultilayerHostCache.java +++ b/core/src/main/java/google/registry/cache/MultilayerHostCache.java @@ -27,13 +27,13 @@ import java.util.Optional; */ public class MultilayerHostCache extends MultilayerEppResourceCache implements HostCache { - public MultilayerHostCache(SimplifiedJedisClient jedisClient) { + public MultilayerHostCache(SimplifiedJedisClient jedisClient) { super(jedisClient); } @Override public Optional loadByRepoId(String repoId) { - return loadFromCaches(repoId); + return loadFromCaches(Host.class, repoId); } @Override @@ -41,9 +41,4 @@ public class MultilayerHostCache extends MultilayerEppResourceCache implem return replicaTm() .transact(() -> replicaTm().loadByKeyIfPresent(VKey.create(Host.class, repoId))); } - - @Override - protected String getJedisPrefix() { - return "h_"; - } } diff --git a/core/src/main/java/google/registry/cache/SimplifiedJedisClient.java b/core/src/main/java/google/registry/cache/SimplifiedJedisClient.java index 7c09b7003..2f2acc042 100644 --- a/core/src/main/java/google/registry/cache/SimplifiedJedisClient.java +++ b/core/src/main/java/google/registry/cache/SimplifiedJedisClient.java @@ -14,13 +14,17 @@ package google.registry.cache; +import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkNotNull; import com.google.common.collect.ImmutableCollection; +import com.google.common.collect.ImmutableMap; import com.google.common.collect.Iterables; import com.google.common.collect.Streams; import com.google.common.flogger.FluentLogger; import google.registry.model.EppResource; +import google.registry.model.domain.Domain; +import google.registry.model.host.Host; import io.protostuff.LinkedBuffer; import io.protostuff.ProtostuffIOUtil; import io.protostuff.Schema; @@ -39,54 +43,57 @@ import redis.clients.jedis.params.SetParams; *

{@link UnifiedJedis} pairs key-value types, so we need the key to be serialized to a byte * array as well. */ -public class SimplifiedJedisClient { +public class SimplifiedJedisClient { public record JedisResource(String key, V value) {} + private static final ImmutableMap, String> TYPE_PREFIXES = + ImmutableMap.of( + Domain.class, "d_", + Host.class, "h_"); + + private static final ImmutableMap, Schema> + VALUE_SCHEMAS = + ImmutableMap.of( + Domain.class, RuntimeSchema.getSchema(Domain.class), + Host.class, RuntimeSchema.getSchema(Host.class)); + private static final FluentLogger logger = FluentLogger.forEnclosingClass(); private static final int BATCH_SIZE = 500; - private final Schema valueSchema; private final UnifiedJedis jedis; - public static SimplifiedJedisClient create( - Class valueClass, UnifiedJedis jedis) { - Schema valueSchema = RuntimeSchema.getSchema(valueClass); - return new SimplifiedJedisClient<>(valueSchema, jedis); - } - - private SimplifiedJedisClient(Schema valueSchema, UnifiedJedis jedis) { - this.valueSchema = valueSchema; + SimplifiedJedisClient(UnifiedJedis jedis) { this.jedis = jedis; } /** Gets the value from the remote cache. Returns null if it does not exist. */ - public Optional get(String key) { + public Optional get(Class clazz, String key) { checkNotNull(key, "Key cannot be null"); - byte[] data = jedis.get(key.getBytes(StandardCharsets.UTF_8)); - return Optional.ofNullable(data).map(this::deserialize); + byte[] data = jedis.get(convertKey(clazz, key)); + return Optional.ofNullable(data).map(d -> deserialize(clazz, d)); } /** Sets the value in the remote cache. */ - public void set(JedisResource resource) { + public void set(JedisResource resource) { checkNotNull(resource.key, "Key cannot be null"); checkNotNull(resource.value, "Value cannot be null"); jedis.set( - resource.key.getBytes(StandardCharsets.UTF_8), + convertKey(resource.value.getClass(), resource.key), serialize(resource.value), new SetParams().pxAt(resource.value.getDeletionTime().toEpochMilli())); } /** Sets multiple values in the remote cache using a Jedis {@link AbstractPipeline}. */ - public void setAll(ImmutableCollection> resources) { + public void setAll(ImmutableCollection> resources) { logger.atInfo().log("Processing %d resources", resources.size()); for (Iterable> batch : Iterables.partition(resources, BATCH_SIZE)) { try (AbstractPipeline pipeline = jedis.pipelined()) { batch.forEach( resource -> pipeline.set( - resource.key.getBytes(StandardCharsets.UTF_8), + convertKey(resource.value.getClass(), resource.key), serialize(resource.value), new SetParams().pxAt(resource.value.getDeletionTime().toEpochMilli()))); pipeline.sync(); @@ -106,18 +113,18 @@ public class SimplifiedJedisClient { *

This could also be accomplished by using {@link #setAll(ImmutableCollection)} with * expiration times that are in the past, but this is clearer. */ - public void deleteAll(ImmutableCollection keys) { + public void deleteAll(Class valueType, ImmutableCollection keys) { // we use a reasonably small batch size to avoid overwhelming the network for (Iterable batch : Iterables.partition(keys, BATCH_SIZE)) { byte[][] keysToUnlink = - Streams.stream(batch) - .map(key -> key.getBytes(StandardCharsets.UTF_8)) - .toArray(byte[][]::new); + Streams.stream(batch).map(key -> convertKey(valueType, key)).toArray(byte[][]::new); jedis.unlink(keysToUnlink); } } - private byte[] serialize(V value) { + private byte[] serialize(V value) { + @SuppressWarnings("unchecked") + Schema valueSchema = (Schema) getValueSchema(value.getClass()); LinkedBuffer buffer = LinkedBuffer.allocate(LinkedBuffer.DEFAULT_BUFFER_SIZE); try { return ProtostuffIOUtil.toByteArray(value, valueSchema, buffer); @@ -126,10 +133,22 @@ public class SimplifiedJedisClient { } } - private V deserialize(byte[] data) { + private V deserialize(Class clazz, byte[] data) { // We use protobufs because other deserializers don't play nicely with immutable collections + Schema valueSchema = getValueSchema(clazz); V value = valueSchema.newMessage(); ProtostuffIOUtil.mergeFrom(data, value, valueSchema); return value; } + + private byte[] convertKey(Class clazz, String key) { + checkArgument(TYPE_PREFIXES.containsKey(clazz), "Unknown class type %s", clazz); + return (TYPE_PREFIXES.get(clazz) + key).getBytes(StandardCharsets.UTF_8); + } + + @SuppressWarnings("unchecked") + private Schema getValueSchema(Class clazz) { + checkArgument(VALUE_SCHEMAS.containsKey(clazz), "Unknown class type %s", clazz); + return (Schema) VALUE_SCHEMAS.get(clazz); + } } diff --git a/core/src/test/java/google/registry/batch/SyncRemoteCacheActionTest.java b/core/src/test/java/google/registry/batch/SyncRemoteCacheActionTest.java index cd08a4663..ae4d1747d 100644 --- a/core/src/test/java/google/registry/batch/SyncRemoteCacheActionTest.java +++ b/core/src/test/java/google/registry/batch/SyncRemoteCacheActionTest.java @@ -26,7 +26,6 @@ import static jakarta.servlet.http.HttpServletResponse.SC_INTERNAL_SERVER_ERROR; import static jakarta.servlet.http.HttpServletResponse.SC_NO_CONTENT; import static jakarta.servlet.http.HttpServletResponse.SC_OK; import static org.mockito.ArgumentMatchers.any; -import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoInteractions; @@ -65,8 +64,7 @@ class SyncRemoteCacheActionTest { final JpaIntegrationTestExtension jpa = new JpaTestExtensions.Builder().withClock(clock).buildIntegrationTestExtension(); - @Mock private SimplifiedJedisClient domainJedisClient; - @Mock private SimplifiedJedisClient hostJedisClient; + @Mock private SimplifiedJedisClient jedisClient; private final FakeResponse response = new FakeResponse(); private FakeLockHandler lockHandler = new FakeLockHandler(true); @@ -75,14 +73,12 @@ class SyncRemoteCacheActionTest { @BeforeEach void beforeEach() { createTld("tld"); - action = - new SyncRemoteCacheAction( - lockHandler, response, Optional.of(domainJedisClient), Optional.of(hostJedisClient)); + action = new SyncRemoteCacheAction(lockHandler, response, Optional.of(jedisClient)); } @Test void test_noJedisConfig() { - action = new SyncRemoteCacheAction(lockHandler, response, Optional.empty(), Optional.empty()); + action = new SyncRemoteCacheAction(lockHandler, response, Optional.empty()); action.run(); assertThat(response.getStatus()).isEqualTo(SC_NO_CONTENT); assertThat(response.getPayload()).contains("No Jedis/Valkey configuration found"); @@ -91,9 +87,7 @@ class SyncRemoteCacheActionTest { @Test void test_lockAcquisitionFails() { lockHandler = new FakeLockHandler(false); - action = - new SyncRemoteCacheAction( - lockHandler, response, Optional.of(domainJedisClient), Optional.of(hostJedisClient)); + action = new SyncRemoteCacheAction(lockHandler, response, Optional.of(jedisClient)); action.run(); assertThat(response.getStatus()).isEqualTo(SC_NO_CONTENT); assertThat(response.getPayload()).contains("Could not acquire lock"); @@ -101,7 +95,7 @@ class SyncRemoteCacheActionTest { @Test void test_exceptionThrown() { - doThrow(new RuntimeException("Redis failed")).when(domainJedisClient).deleteAll(any()); + doThrow(new RuntimeException("Redis failed")).when(jedisClient).deleteAll(any(), any()); persistActiveDomain("example.tld"); // So there is something to process action.run(); assertThat(response.getStatus()).isEqualTo(SC_INTERNAL_SERVER_ERROR); @@ -112,7 +106,7 @@ class SyncRemoteCacheActionTest { void test_syncDomains_noDomains() { action.run(); assertThat(response.getStatus()).isEqualTo(SC_OK); - verifyNoInteractions(domainJedisClient); + verifyNoInteractions(jedisClient); assertThat(DatabaseHelper.loadByKeyIfPresent(Cursor.createGlobalVKey(REMOTE_CACHE_DOMAIN_SYNC))) .isEmpty(); } @@ -126,12 +120,11 @@ class SyncRemoteCacheActionTest { action.run(); assertThat(response.getStatus()).isEqualTo(SC_OK); - verify(domainJedisClient) + verify(jedisClient) .setAll( - eq( - ImmutableList.of( - new SimplifiedJedisClient.JedisResource<>("example1.tld", domain1), - new SimplifiedJedisClient.JedisResource<>("example2.tld", domain2)))); + ImmutableList.of( + new SimplifiedJedisClient.JedisResource<>("example1.tld", domain1), + new SimplifiedJedisClient.JedisResource<>("example2.tld", domain2))); assertThat( DatabaseHelper.loadByKey(Cursor.createGlobalVKey(REMOTE_CACHE_DOMAIN_SYNC)) @@ -148,12 +141,11 @@ class SyncRemoteCacheActionTest { action.run(); assertThat(response.getStatus()).isEqualTo(SC_OK); - verify(domainJedisClient) + verify(jedisClient) .setAll( - eq( - ImmutableList.of( - new SimplifiedJedisClient.JedisResource<>("active.tld", activeDomain)))); - verify(domainJedisClient).deleteAll(eq(ImmutableList.of("deleted.tld"))); + ImmutableList.of( + new SimplifiedJedisClient.JedisResource<>("active.tld", activeDomain))); + verify(jedisClient).deleteAll(Domain.class, ImmutableList.of("deleted.tld")); } @Test @@ -171,18 +163,16 @@ class SyncRemoteCacheActionTest { action.run(); assertThat(response.getStatus()).isEqualTo(SC_OK); - verify(domainJedisClient) + verify(jedisClient) .setAll( - eq( - ImmutableList.of( - new SimplifiedJedisClient.JedisResource<>("example2.tld", domain2)))); + ImmutableList.of(new SimplifiedJedisClient.JedisResource<>("example2.tld", domain2))); } @Test void test_syncHosts_noHosts() { action.run(); assertThat(response.getStatus()).isEqualTo(SC_OK); - verifyNoInteractions(hostJedisClient); + verifyNoInteractions(jedisClient); assertThat(DatabaseHelper.loadByKeyIfPresent(Cursor.createGlobalVKey(REMOTE_CACHE_HOST_SYNC))) .isEmpty(); } @@ -196,12 +186,11 @@ class SyncRemoteCacheActionTest { action.run(); assertThat(response.getStatus()).isEqualTo(SC_OK); - verify(hostJedisClient) + verify(jedisClient) .setAll( - eq( - ImmutableList.of( - new SimplifiedJedisClient.JedisResource<>(host1.getRepoId(), host1), - new SimplifiedJedisClient.JedisResource<>(host2.getRepoId(), host2)))); + ImmutableList.of( + new SimplifiedJedisClient.JedisResource<>(host1.getRepoId(), host1), + new SimplifiedJedisClient.JedisResource<>(host2.getRepoId(), host2))); assertThat( DatabaseHelper.loadByKey(Cursor.createGlobalVKey(REMOTE_CACHE_HOST_SYNC)) @@ -218,11 +207,10 @@ class SyncRemoteCacheActionTest { action.run(); assertThat(response.getStatus()).isEqualTo(SC_OK); - verify(hostJedisClient) + verify(jedisClient) .setAll( - eq( - ImmutableList.of( - new SimplifiedJedisClient.JedisResource<>(active.getRepoId(), active)))); - verify(hostJedisClient).deleteAll(eq(ImmutableList.of(deleted.getRepoId()))); + ImmutableList.of( + new SimplifiedJedisClient.JedisResource<>(active.getRepoId(), active))); + verify(jedisClient).deleteAll(Host.class, ImmutableList.of(deleted.getRepoId())); } } diff --git a/core/src/test/java/google/registry/cache/MultilayerDomainCacheTest.java b/core/src/test/java/google/registry/cache/MultilayerDomainCacheTest.java index db79aded8..f09302e18 100644 --- a/core/src/test/java/google/registry/cache/MultilayerDomainCacheTest.java +++ b/core/src/test/java/google/registry/cache/MultilayerDomainCacheTest.java @@ -18,7 +18,6 @@ import static com.google.common.truth.Truth.assertThat; import static google.registry.testing.DatabaseHelper.createTld; import static google.registry.testing.DatabaseHelper.persistActiveDomain; import static google.registry.testing.DatabaseHelper.persistResource; -import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoMoreInteractions; @@ -42,7 +41,7 @@ public class MultilayerDomainCacheTest { final JpaIntegrationTestExtension jpa = new JpaTestExtensions.Builder().buildIntegrationTestExtension(); - private final SimplifiedJedisClient jedisClient = mock(SimplifiedJedisClient.class); + private final SimplifiedJedisClient jedisClient = mock(SimplifiedJedisClient.class); private final FakeClock clock = new FakeClock(); private MultilayerDomainCache cache; @@ -58,8 +57,8 @@ public class MultilayerDomainCacheTest { assertThat(cache.loadByDomainName("example.tld")).hasValue(domain); // We should have filled the caches after one attempt to load from Valkey - verify(jedisClient).get("d_example.tld"); - verify(jedisClient).set(new SimplifiedJedisClient.JedisResource<>("d_example.tld", domain)); + verify(jedisClient).get(Domain.class, "example.tld"); + verify(jedisClient).set(new SimplifiedJedisClient.JedisResource<>("example.tld", domain)); // Further loads hit the local cache assertThat(cache.loadByDomainName("example.tld")).hasValue(domain); @@ -71,7 +70,7 @@ public class MultilayerDomainCacheTest { // Note: we don't save the domain to SQL Domain domain = DatabaseHelper.newDomain("example.tld"); // We hit the Valkey cache first - when(jedisClient.get(eq("d_example.tld"))).thenReturn(Optional.of(domain)); + when(jedisClient.get(Domain.class, "example.tld")).thenReturn(Optional.of(domain)); assertThat(cache.loadByDomainName("example.tld")).hasValue(domain); } @@ -83,7 +82,7 @@ public class MultilayerDomainCacheTest { assertThat(cache.loadByDomainName("example.tld")).hasValue(domain); // This time, we don't populate the remote cache because it's prober data - verify(jedisClient).get("d_example.tld"); + verify(jedisClient).get(Domain.class, "example.tld"); verifyNoMoreInteractions(jedisClient); } diff --git a/core/src/test/java/google/registry/cache/MultilayerHostCacheTest.java b/core/src/test/java/google/registry/cache/MultilayerHostCacheTest.java index db0f11b52..3a2dedf38 100644 --- a/core/src/test/java/google/registry/cache/MultilayerHostCacheTest.java +++ b/core/src/test/java/google/registry/cache/MultilayerHostCacheTest.java @@ -16,7 +16,6 @@ package google.registry.cache; import static com.google.common.truth.Truth.assertThat; import static google.registry.testing.DatabaseHelper.persistActiveHost; -import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoMoreInteractions; @@ -38,7 +37,7 @@ public class MultilayerHostCacheTest { final JpaIntegrationTestExtension jpa = new JpaTestExtensions.Builder().buildIntegrationTestExtension(); - private final SimplifiedJedisClient jedisClient = mock(SimplifiedJedisClient.class); + private final SimplifiedJedisClient jedisClient = mock(SimplifiedJedisClient.class); private MultilayerHostCache cache; @BeforeEach @@ -52,9 +51,8 @@ public class MultilayerHostCacheTest { assertThat(cache.loadByRepoId(host.getRepoId())).hasValue(host); // We should have filled the caches after one attempt to load from Valkey - verify(jedisClient).get("h_" + host.getRepoId()); - verify(jedisClient) - .set(new SimplifiedJedisClient.JedisResource<>("h_" + host.getRepoId(), host)); + verify(jedisClient).get(Host.class, host.getRepoId()); + verify(jedisClient).set(new SimplifiedJedisClient.JedisResource<>(host.getRepoId(), host)); // Further loads hit the local cache assertThat(cache.loadByRepoId(host.getRepoId())).hasValue(host); @@ -66,7 +64,7 @@ public class MultilayerHostCacheTest { // Note: we don't save the host to SQL Host host = DatabaseHelper.newHost("ns1.example.tld"); // We hit the Valkey cache first - when(jedisClient.get(eq("h_" + host.getRepoId()))).thenReturn(Optional.of(host)); + when(jedisClient.get(Host.class, host.getRepoId())).thenReturn(Optional.of(host)); assertThat(cache.loadByRepoId(host.getRepoId())).hasValue(host); } diff --git a/core/src/test/java/google/registry/cache/SimplifiedJedisClientTest.java b/core/src/test/java/google/registry/cache/SimplifiedJedisClientTest.java index 67e5df3bb..0811e1f20 100644 --- a/core/src/test/java/google/registry/cache/SimplifiedJedisClientTest.java +++ b/core/src/test/java/google/registry/cache/SimplifiedJedisClientTest.java @@ -23,7 +23,6 @@ import static google.registry.testing.DatabaseHelper.persistDeletedDomain; import static org.joda.time.DateTimeZone.UTC; import com.google.common.collect.ImmutableList; -import google.registry.model.EppResource; import google.registry.model.domain.Domain; import google.registry.model.host.Host; import google.registry.persistence.transaction.JpaTestExtensions; @@ -38,7 +37,6 @@ import org.testcontainers.junit.jupiter.Container; import org.testcontainers.junit.jupiter.Testcontainers; import redis.clients.jedis.HostAndPort; import redis.clients.jedis.RedisClient; -import redis.clients.jedis.UnifiedJedis; /** Tests for {@link SimplifiedJedisClient}. */ @Testcontainers @@ -60,29 +58,29 @@ public class SimplifiedJedisClientTest { @Test void testClient_roundTrip_domain() { Domain domain = persistActiveDomain("example.tld"); - SimplifiedJedisClient client = createSimplifiedClient(Domain.class); - client.set(new SimplifiedJedisClient.JedisResource<>("d_example.tld", domain)); + SimplifiedJedisClient client = createJedisClient(); + client.set(new SimplifiedJedisClient.JedisResource<>("example.tld", domain)); // dsData and gracePeriods get serialized as null instead of the empty set, which is fine assertAboutImmutableObjects() - .that(client.get("d_example.tld").get()) + .that(client.get(Domain.class, "example.tld").get()) .isEqualExceptFields(domain, "dsData", "gracePeriods"); } @Test void testClient_roundTrip_host() { Host host = persistActiveHost("ns1.example.tld"); - SimplifiedJedisClient client = createSimplifiedClient(Host.class); - client.set(new SimplifiedJedisClient.JedisResource<>("h_repoId1", host)); - assertThat(client.get("h_repoId1")).hasValue(host); + SimplifiedJedisClient client = createJedisClient(); + client.set(new SimplifiedJedisClient.JedisResource<>("repoId1", host)); + assertThat(client.get(Host.class, "repoId1")).hasValue(host); } @Test void testSet_withExpiration() throws Exception { - SimplifiedJedisClient client = createSimplifiedClient(Domain.class); + SimplifiedJedisClient client = createJedisClient(); Domain pendingDelete = persistDeletedDomain("example.tld", DateTime.now(UTC).plusMillis(100)); - client.set(new SimplifiedJedisClient.JedisResource<>("d_example1.tld", pendingDelete)); + client.set(new SimplifiedJedisClient.JedisResource<>("example1.tld", pendingDelete)); Thread.sleep(101); - assertThat(client.get("d_example1.tld")).isEmpty(); + assertThat(client.get(Domain.class, "example1.tld")).isEmpty(); } @Test @@ -90,22 +88,22 @@ public class SimplifiedJedisClientTest { Domain domain1 = persistActiveDomain("example1.tld"); Domain domain2 = persistActiveDomain("example2.tld"); Domain domain3 = persistActiveDomain("example3.tld"); - SimplifiedJedisClient client = createSimplifiedClient(Domain.class); + SimplifiedJedisClient client = createJedisClient(); client.setAll( ImmutableList.of( - new SimplifiedJedisClient.JedisResource<>("d_example1.tld", domain1), - new SimplifiedJedisClient.JedisResource<>("d_example2.tld", domain2), - new SimplifiedJedisClient.JedisResource<>("d_example3.tld", domain3))); + new SimplifiedJedisClient.JedisResource<>("example1.tld", domain1), + new SimplifiedJedisClient.JedisResource<>("example2.tld", domain2), + new SimplifiedJedisClient.JedisResource<>("example3.tld", domain3))); assertAboutImmutableObjects() - .that(client.get("d_example1.tld").get()) + .that(client.get(Domain.class, "example1.tld").get()) .isEqualExceptFields(domain1, "dsData", "gracePeriods"); assertAboutImmutableObjects() - .that(client.get("d_example2.tld").get()) + .that(client.get(Domain.class, "example2.tld").get()) .isEqualExceptFields(domain2, "dsData", "gracePeriods"); assertAboutImmutableObjects() - .that(client.get("d_example3.tld").get()) + .that(client.get(Domain.class, "example3.tld").get()) .isEqualExceptFields(domain3, "dsData", "gracePeriods"); } @@ -114,17 +112,17 @@ public class SimplifiedJedisClientTest { Host host1 = persistActiveHost("ns1.example.tld"); Host host2 = persistActiveHost("ns2.example.tld"); Host host3 = persistActiveHost("ns3.example.tld"); - SimplifiedJedisClient client = createSimplifiedClient(Host.class); + SimplifiedJedisClient client = createJedisClient(); client.setAll( ImmutableList.of( - new SimplifiedJedisClient.JedisResource<>("h_repoId1", host1), - new SimplifiedJedisClient.JedisResource<>("h_repoId2", host2), - new SimplifiedJedisClient.JedisResource<>("h_repoId3", host3))); + new SimplifiedJedisClient.JedisResource<>("repoId1", host1), + new SimplifiedJedisClient.JedisResource<>("repoId2", host2), + new SimplifiedJedisClient.JedisResource<>("repoId3", host3))); - assertThat(client.get("h_repoId1")).hasValue(host1); - assertThat(client.get("h_repoId2")).hasValue(host2); - assertThat(client.get("h_repoId3")).hasValue(host3); + assertThat(client.get(Host.class, "repoId1")).hasValue(host1); + assertThat(client.get(Host.class, "repoId2")).hasValue(host2); + assertThat(client.get(Host.class, "repoId3")).hasValue(host3); } @Test @@ -132,35 +130,32 @@ public class SimplifiedJedisClientTest { Host host1 = persistActiveHost("ns1.example.tld"); Host host2 = persistActiveHost("ns2.example.tld"); Host host3 = persistActiveHost("ns3.example.tld"); - SimplifiedJedisClient client = createSimplifiedClient(Host.class); + SimplifiedJedisClient client = createJedisClient(); client.setAll( ImmutableList.of( - new SimplifiedJedisClient.JedisResource<>("h_repoId1", host1), - new SimplifiedJedisClient.JedisResource<>("h_repoId2", host2), - new SimplifiedJedisClient.JedisResource<>("h_repoId3", host3))); + new SimplifiedJedisClient.JedisResource<>("repoId1", host1), + new SimplifiedJedisClient.JedisResource<>("repoId2", host2), + new SimplifiedJedisClient.JedisResource<>("repoId3", host3))); - client.deleteAll(ImmutableList.of("h_repoId1", "h_repoId2", "h_nonexistent")); - assertThat(client.get("h_repoId1")).isEmpty(); - assertThat(client.get("h_repoId2")).isEmpty(); - assertThat(client.get("h_repoId3")).hasValue(host3); + client.deleteAll(Host.class, ImmutableList.of("repoId1", "repoId2", "nonexistent")); + assertThat(client.get(Host.class, "repoId1")).isEmpty(); + assertThat(client.get(Host.class, "repoId2")).isEmpty(); + assertThat(client.get(Host.class, "repoId3")).hasValue(host3); } @Test void testClient_nonexistent() { - SimplifiedJedisClient domainClient = createSimplifiedClient(Domain.class); - SimplifiedJedisClient hostClient = createSimplifiedClient(Host.class); - assertThat(domainClient.get("d_nonexistent.tld")).isEmpty(); - assertThat(hostClient.get("h_ns1.nonexistent.tld")).isEmpty(); + SimplifiedJedisClient domainClient = createJedisClient(); + SimplifiedJedisClient hostClient = createJedisClient(); + assertThat(domainClient.get(Domain.class, "nonexistent.tld")).isEmpty(); + assertThat(hostClient.get(Host.class, "ns1.nonexistent.tld")).isEmpty(); } - private SimplifiedJedisClient createSimplifiedClient(Class clazz) { - return SimplifiedJedisClient.create(clazz, createJedisClient()); - } - - private UnifiedJedis createJedisClient() { - return RedisClient.builder() - .hostAndPort(new HostAndPort(valkey.getHost(), valkey.getFirstMappedPort())) - .build(); + private SimplifiedJedisClient createJedisClient() { + return new SimplifiedJedisClient( + RedisClient.builder() + .hostAndPort(new HostAndPort(valkey.getHost(), valkey.getFirstMappedPort())) + .build()); } }