Merge pull request #27716 from eipc16/pp/upgrade-keyvredis

Fixed cache key resolution for redis store with useRedisSets set to false

packages/backend-defaults/config.d.ts

@@ -486,11 +486,6 @@ export interface Config {
           connection: string;
           /** An optional default TTL (in milliseconds, if given as a number). */
           defaultTtl?: number | HumanDuration | string;
-          /**
-           * Whether or not [useRedisSets](https://github.com/jaredwray/keyv/tree/main/packages/redis#useredissets) should be configured to this redis cache.
-           * Defaults to true if unspecified.
-           */
-          useRedisSets?: boolean;
         }
       | {
           store: 'memcache';

packages/backend-defaults/package.json

@@ -136,8 +136,8 @@
     "@backstage/plugin-permission-node": "workspace:^",
     "@backstage/types": "workspace:^",
     "@google-cloud/storage": "^7.0.0",
-    "@keyv/memcache": "^1.3.5",
-    "@keyv/redis": "^2.5.3",
+    "@keyv/memcache": "^2.0.1",
+    "@keyv/redis": "^4.0.1",
     "@manypkg/get-packages": "^1.1.3",
     "@octokit/rest": "^19.0.3",
     "@opentelemetry/api": "^1.3.0",
@@ -158,7 +158,7 @@
     "helmet": "^6.0.0",
     "isomorphic-git": "^1.23.0",
     "jose": "^5.0.0",
-    "keyv": "^4.5.2",
+    "keyv": "^5.2.1",
     "knex": "^3.0.0",
     "lodash": "^4.17.21",
     "logform": "^2.3.2",

packages/backend-defaults/src/entrypoints/cache/CacheManager.test.ts

@@ -25,15 +25,21 @@ import { CacheManager } from './CacheManager';
 // Contrived code because it's hard to spy on a default export
 jest.mock('@keyv/redis', () => {
   const Actual = jest.requireActual('@keyv/redis');
-  return jest.fn((...args: any[]) => {
-    return new Actual(...args);
-  });
+  const DefaultConstructor = Actual.default;
+  return {
+    ...Actual,
+    __esModule: true,
+    default: jest.fn((...args: any[]) => new DefaultConstructor(...args)),
+  };
 });
 jest.mock('@keyv/memcache', () => {
   const Actual = jest.requireActual('@keyv/memcache');
-  return jest.fn((...args: any[]) => {
-    return new Actual(...args);
-  });
+  const DefaultConstructor = Actual.default;
+  return {
+    ...Actual,
+    __esModule: true,
+    default: jest.fn((...args: any[]) => new DefaultConstructor(...args)),
+  };
 });
 
 describe('CacheManager integration', () => {
@@ -42,7 +48,7 @@ describe('CacheManager integration', () => {
   afterEach(jest.clearAllMocks);
 
   it.each(caches.eachSupportedId())(
-    'only creates one underlying connection, %p',
+    'only creates one underlying connection per plugin, %p',
     async cacheId => {
       const { store, connection } = await caches.init(cacheId);
 
@@ -59,10 +65,10 @@ describe('CacheManager integration', () => {
 
       if (store === 'redis') {
         // eslint-disable-next-line jest/no-conditional-expect
-        expect(KeyvRedis).toHaveBeenCalledTimes(1);
+        expect(KeyvRedis).toHaveBeenCalledTimes(3);
       } else if (store === 'memcache') {
         // eslint-disable-next-line jest/no-conditional-expect
-        expect(KeyvMemcache).toHaveBeenCalledTimes(1);
+        expect(KeyvMemcache).toHaveBeenCalledTimes(3);
       }
     },
   );

packages/backend-defaults/src/entrypoints/cache/CacheManager.ts

@@ -49,7 +49,6 @@ export class CacheManager {
   private readonly logger?: LoggerService;
   private readonly store: keyof CacheManager['storeFactories'];
   private readonly connection: string;
-  private readonly useRedisSets: boolean;
   private readonly errorHandler: CacheManagerOptions['onError'];
   private readonly defaultTtl?: number;
 
@@ -69,12 +68,16 @@ export class CacheManager {
     const defaultTtlConfig = config.getOptional('backend.cache.defaultTtl');
     const connectionString =
       config.getOptionalString('backend.cache.connection') || '';
-    const useRedisSets =
-      config.getOptionalBoolean('backend.cache.useRedisSets') ?? true;
     const logger = options.logger?.child({
       type: 'cacheManager',
     });
 
+    if (config.has('backend.cache.useRedisSets')) {
+      logger?.warn(
+        "The 'backend.cache.useRedisSets' configuration key is deprecated and no longer has any effect. The underlying '@keyv/redis' library no longer supports redis sets.",
+      );
+    }
+
     let defaultTtl: number | undefined;
     if (defaultTtlConfig !== undefined) {
       if (typeof defaultTtlConfig === 'number') {
@@ -89,7 +92,6 @@ export class CacheManager {
     return new CacheManager(
       store,
       connectionString,
-      useRedisSets,
       options.onError,
       logger,
       defaultTtl,
@@ -100,7 +102,6 @@ export class CacheManager {
   constructor(
     store: string,
     connectionString: string,
-    useRedisSets: boolean,
     errorHandler: CacheManagerOptions['onError'],
     logger?: LoggerService,
     defaultTtl?: number,
@@ -111,7 +112,6 @@ export class CacheManager {
     this.logger = logger;
     this.store = store as keyof CacheManager['storeFactories'];
     this.connection = connectionString;
-    this.useRedisSets = useRedisSets;
     this.errorHandler = errorHandler;
     this.defaultTtl = defaultTtl;
   }
@@ -139,15 +139,16 @@ export class CacheManager {
   }
 
   private createRedisStoreFactory(): StoreFactory {
-    const KeyvRedis = require('@keyv/redis');
-    let store: typeof KeyvRedis | undefined;
+    const KeyvRedis = require('@keyv/redis').default;
+    const stores: Record<string, typeof KeyvRedis> = {};
+
     return (pluginId, defaultTtl) => {
-      if (!store) {
-        store = new KeyvRedis(this.connection, {
-          useRedisSets: this.useRedisSets,
+      if (!stores[pluginId]) {
+        stores[pluginId] = new KeyvRedis(this.connection, {
+          keyPrefixSeparator: ':',
         });
         // Always provide an error handler to avoid stopping the process
-        store.on('error', (err: Error) => {
+        stores[pluginId].on('error', (err: Error) => {
           this.logger?.error('Failed to create redis cache client', err);
           this.errorHandler?.(err);
         });
@@ -155,21 +156,22 @@ export class CacheManager {
       return new Keyv({
         namespace: pluginId,
         ttl: defaultTtl,
-        store,
+        store: stores[pluginId],
         emitErrors: false,
-        useRedisSets: this.useRedisSets,
+        useKeyPrefix: false,
       });
     };
   }
 
   private createMemcacheStoreFactory(): StoreFactory {
-    const KeyvMemcache = require('@keyv/memcache');
-    let store: typeof KeyvMemcache | undefined;
+    const KeyvMemcache = require('@keyv/memcache').default;
+    const stores: Record<string, typeof KeyvMemcache> = {};
+
     return (pluginId, defaultTtl) => {
-      if (!store) {
-        store = new KeyvMemcache(this.connection);
+      if (!stores[pluginId]) {
+        stores[pluginId] = new KeyvMemcache(this.connection);
         // Always provide an error handler to avoid stopping the process
-        store.on('error', (err: Error) => {
+        stores[pluginId].on('error', (err: Error) => {
           this.logger?.error('Failed to create memcache cache client', err);
           this.errorHandler?.(err);
         });
@@ -178,7 +180,7 @@ export class CacheManager {
         namespace: pluginId,
         ttl: defaultTtl,
         emitErrors: false,
-        store,
+        store: stores[pluginId],
       });
     };
   }