Refactor: Extract Redis cache key generation to RedisPrefixHelper (#11376)

* Initial plan

* Refactor Redis cache keys: delegate CacheHelper to RedisPrefixHelper and update callers

Co-authored-by: tommoor <380914+tommoor@users.noreply.github.com>

* Add JSDoc documentation to getCollectionDocumentsKey method

Co-authored-by: tommoor <380914+tommoor@users.noreply.github.com>

* Remove unused indirection

* Remove mock

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: tommoor <380914+tommoor@users.noreply.github.com>
Co-authored-by: Tom Moor <tom@getoutline.com>

server/models/Collection.ts

@@ -61,6 +61,7 @@ import { CollectionValidation } from "@shared/validations";
 import { ValidationError } from "@server/errors";
 import type { APIContext } from "@server/types";
 import { CacheHelper } from "@server/utils/CacheHelper";
+import { RedisPrefixHelper } from "@server/utils/RedisPrefixHelper";
 import removeIndexCollision from "@server/utils/removeIndexCollision";
 import { generateUrlId } from "@server/utils/url";
 import { ValidateIndex } from "@server/validation";
@@ -347,7 +348,7 @@ class Collection extends ParanoidModel<
     }
     if (model.changed("documentStructure")) {
       await CacheHelper.clearData(
-        CacheHelper.getCollectionDocumentsKey(model.id)
+        RedisPrefixHelper.getCollectionDocumentsKey(model.id)
       );
     }
   }
@@ -360,7 +361,7 @@ class Collection extends ParanoidModel<
     if (model.changed("documentStructure")) {
       const setData = () =>
         CacheHelper.setData(
-          CacheHelper.getCollectionDocumentsKey(model.id),
+          RedisPrefixHelper.getCollectionDocumentsKey(model.id),
           model.documentStructure,
           60
         );

server/queues/processors/IntegrationCreatedProcessor.ts

@@ -3,6 +3,7 @@ import { Integration } from "@server/models";
 import BaseProcessor from "@server/queues/processors/BaseProcessor";
 import type { IntegrationEvent, Event } from "@server/types";
 import { CacheHelper } from "@server/utils/CacheHelper";
+import { RedisPrefixHelper } from "@server/utils/RedisPrefixHelper";
 import CacheIssueSourcesTask from "../tasks/CacheIssueSourcesTask";
 
 export default class IntegrationCreatedProcessor extends BaseProcessor {
@@ -25,6 +26,6 @@ export default class IntegrationCreatedProcessor extends BaseProcessor {
     });
 
     // Clear the cache of unfurled data for the team as it may be stale now.
-    await CacheHelper.clearData(CacheHelper.getUnfurlKey(integration.teamId));
+    await CacheHelper.clearData(RedisPrefixHelper.getUnfurlKey(integration.teamId));
   }
 }

server/queues/processors/IntegrationDeletedProcessor.ts

@@ -3,6 +3,7 @@ import { Integration } from "@server/models";
 import BaseProcessor from "@server/queues/processors/BaseProcessor";
 import type { IntegrationEvent, Event } from "@server/types";
 import { CacheHelper } from "@server/utils/CacheHelper";
+import { RedisPrefixHelper } from "@server/utils/RedisPrefixHelper";
 import { Hook, PluginManager } from "@server/utils/PluginManager";
 
 export default class IntegrationDeletedProcessor extends BaseProcessor {
@@ -26,7 +27,7 @@ export default class IntegrationDeletedProcessor extends BaseProcessor {
 
     // Clear the cache of unfurled data for the team as it may be stale now.
     if (integration.type === IntegrationType.Embed) {
-      await CacheHelper.clearData(CacheHelper.getUnfurlKey(integration.teamId));
+      await CacheHelper.clearData(RedisPrefixHelper.getUnfurlKey(integration.teamId));
     }
 
     await integration.destroy({ force: true });

server/routes/api/collections/collections.ts

@@ -39,6 +39,7 @@ import {
 } from "@server/presenters";
 import type { APIContext } from "@server/types";
 import { CacheHelper } from "@server/utils/CacheHelper";
+import { RedisPrefixHelper } from "@server/utils/RedisPrefixHelper";
 import { RateLimiterStrategy } from "@server/utils/RateLimiter";
 import { collectionIndexing } from "@server/utils/indexing";
 import pagination from "../middlewares/pagination";
@@ -143,7 +144,7 @@ router.post(
     authorize(user, "readDocument", collection);
 
     const documentStructure = await CacheHelper.getDataOrSet(
-      CacheHelper.getCollectionDocumentsKey(collection.id),
+      RedisPrefixHelper.getCollectionDocumentsKey(collection.id),
       async () =>
         (
           await Collection.findByPk(collection.id, {

server/routes/api/urls/urls.ts

@@ -15,6 +15,7 @@ import { authorize, can } from "@server/policies";
 import presentUnfurl from "@server/presenters/unfurl";
 import type { APIContext, Unfurl } from "@server/types";
 import { CacheHelper, type CacheResult } from "@server/utils/CacheHelper";
+import { RedisPrefixHelper } from "@server/utils/RedisPrefixHelper";
 import { Hook, PluginManager } from "@server/utils/PluginManager";
 import { RateLimiterStrategy } from "@server/utils/RateLimiter";
 import {
@@ -134,7 +135,7 @@ router.post(
     // External resources
     // Use getDataOrSet which handles distributed locking to prevent thundering herd
     // when multiple clients request the same URL simultaneously
-    const cacheKey = CacheHelper.getUnfurlKey(actor.teamId, url);
+    const cacheKey = RedisPrefixHelper.getUnfurlKey(actor.teamId, url);
     const defaultCacheExpiry = 3600;
 
     const unfurlResult = await CacheHelper.getDataOrSet<
@@ -186,7 +187,7 @@ router.post(
     const { url } = ctx.input.body;
 
     const result = await CacheHelper.getDataOrSet<EmbedCheckResult>(
-      CacheHelper.getEmbedCheckKey(url),
+      RedisPrefixHelper.getEmbedCheckKey(url),
       () => checkEmbeddability(url),
       Day.seconds
     );

server/utils/CacheHelper.ts

@@ -141,31 +141,4 @@ export class CacheHelper {
       })
     );
   }
-
-  // keys
-
-  /**
-   * Gets key against which unfurl response for the given url is stored
-   *
-   * @param teamId The team ID to generate a key for
-   * @param url The url to generate a key for
-   */
-  public static getUnfurlKey(teamId: string, url = "") {
-    return `unfurl:${teamId}:${url}`;
-  }
-
-  public static getCollectionDocumentsKey(collectionId: string) {
-    return `cd:${collectionId}`;
-  }
-
-  /**
-   * Gets key for caching embed check results. This is a global cache key
-   * (not team-specific) since embed headers are the same for all users.
-   *
-   * @param url The URL to generate a cache key for.
-   * @returns the cache key string.
-   */
-  public static getEmbedCheckKey(url: string) {
-    return `embed:${url}`;
-  }
 }

server/utils/RedisPrefixHelper.test.ts

@@ -0,0 +1,61 @@
+import { RedisPrefixHelper } from "./RedisPrefixHelper";
+
+describe("RedisPrefixHelper", () => {
+  describe("getUnfurlKey", () => {
+    it("should generate key with teamId and url", () => {
+      const teamId = "team-123";
+      const url = "https://example.com";
+      const result = RedisPrefixHelper.getUnfurlKey(teamId, url);
+      expect(result).toBe("unfurl:team-123:https://example.com");
+    });
+
+    it("should generate key with teamId and empty url", () => {
+      const teamId = "team-456";
+      const result = RedisPrefixHelper.getUnfurlKey(teamId);
+      expect(result).toBe("unfurl:team-456:");
+    });
+
+    it("should handle special characters in url", () => {
+      const teamId = "team-789";
+      const url = "https://example.com/path?query=value&other=123";
+      const result = RedisPrefixHelper.getUnfurlKey(teamId, url);
+      expect(result).toBe(
+        "unfurl:team-789:https://example.com/path?query=value&other=123"
+      );
+    });
+  });
+
+  describe("getCollectionDocumentsKey", () => {
+    it("should generate key with collectionId", () => {
+      const collectionId = "col-abc123";
+      const result = RedisPrefixHelper.getCollectionDocumentsKey(collectionId);
+      expect(result).toBe("cd:col-abc123");
+    });
+
+    it("should handle uuid format", () => {
+      const collectionId = "550e8400-e29b-41d4-a716-446655440000";
+      const result = RedisPrefixHelper.getCollectionDocumentsKey(collectionId);
+      expect(result).toBe("cd:550e8400-e29b-41d4-a716-446655440000");
+    });
+  });
+
+  describe("getEmbedCheckKey", () => {
+    it("should generate key with url", () => {
+      const url = "https://example.com/embed";
+      const result = RedisPrefixHelper.getEmbedCheckKey(url);
+      expect(result).toBe("embed:https://example.com/embed");
+    });
+
+    it("should handle urls with query parameters", () => {
+      const url = "https://example.com/video?v=abc123";
+      const result = RedisPrefixHelper.getEmbedCheckKey(url);
+      expect(result).toBe("embed:https://example.com/video?v=abc123");
+    });
+
+    it("should handle urls with fragments", () => {
+      const url = "https://example.com/page#section";
+      const result = RedisPrefixHelper.getEmbedCheckKey(url);
+      expect(result).toBe("embed:https://example.com/page#section");
+    });
+  });
+});

server/utils/RedisPrefixHelper.ts

@@ -0,0 +1,35 @@
+/**
+ * Helper class for Redis cache key generation.
+ */
+export class RedisPrefixHelper {
+  /**
+   * Gets key against which unfurl response for the given url is stored.
+   *
+   * @param teamId The team ID to generate a key for.
+   * @param url The url to generate a key for.
+   */
+  public static getUnfurlKey(teamId: string, url = "") {
+    return `unfurl:${teamId}:${url}`;
+  }
+
+  /**
+   * Gets key for caching collection documents structure.
+   *
+   * @param collectionId The collection ID to generate a key for.
+   * @returns the cache key string.
+   */
+  public static getCollectionDocumentsKey(collectionId: string) {
+    return `cd:${collectionId}`;
+  }
+
+  /**
+   * Gets key for caching embed check results. This is a global cache key
+   * (not team-specific) since embed headers are the same for all users.
+   *
+   * @param url The URL to generate a cache key for.
+   * @returns the cache key string.
+   */
+  public static getEmbedCheckKey(url: string) {
+    return `embed:${url}`;
+  }
+}

server/utils/__mocks__/CacheHelper.ts

@@ -44,20 +44,4 @@ export class CacheHelper {
   public static async clearData(_prefix: string) {
     return;
   }
-
-  /**
-   * These are real methods that don't require mocking as they don't
-   * interact with Redis directly
-   */
-  public static getUnfurlKey(teamId: string, url = "") {
-    return `unfurl:${teamId}:${url}`;
-  }
-
-  public static getCollectionDocumentsKey(collectionId: string) {
-    return `cd:${collectionId}`;
-  }
-
-  public static getEmbedCheckKey(url: string) {
-    return `embed:${url}`;
-  }
 }