Fix remote cache prefixing by handling it all in the Jedis client (#3035)

Found this out while testing metrics. In hindsight, not the best idea to
handle prefixing outside of the client itself. Instead, we'll enforce
the prefixing closer to Valkey, all in one place.

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<Domain> domainJedisClient;
-  @Mock private SimplifiedJedisClient<Host> 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()));
   }
 }

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<Domain> 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);
   }
 

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<Host> 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);
   }
 

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<Domain> 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<Host> 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<Domain> 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<Domain> 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<Host> 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<Host> 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<Domain> domainClient = createSimplifiedClient(Domain.class);
-    SimplifiedJedisClient<Host> 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 <T extends EppResource> SimplifiedJedisClient<T> createSimplifiedClient(Class<T> 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());
   }
 }