From 97e3aebcff716c398194d4411da4e3533a9ff2ce Mon Sep 17 00:00:00 2001 From: jzaffiro <110866475+jzaffiro@users.noreply.github.com> Date: Thu, 28 Aug 2025 21:18:41 +0000 Subject: [PATCH 1/7] Update cache entry syntax and fix typos --- .../drivers/odsp-driver-definitions/src/odspCache.ts | 9 +++++++-- packages/drivers/odsp-driver/src/odspDocumentService.ts | 2 +- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/packages/drivers/odsp-driver-definitions/src/odspCache.ts b/packages/drivers/odsp-driver-definitions/src/odspCache.ts index 09a2181d93c5..0334c638ab2b 100644 --- a/packages/drivers/odsp-driver-definitions/src/odspCache.ts +++ b/packages/drivers/odsp-driver-definitions/src/odspCache.ts @@ -114,7 +114,7 @@ export interface IPersistedCache { put(entry: ICacheEntry, value: any): Promise; /** - * Removes the entries from the cache for given parametres. + * Removes the entries from the cache for given parameters. * @param file - file entry to be deleted. */ removeEntries(file: IFileEntry): Promise; @@ -127,5 +127,10 @@ export interface IPersistedCache { * @internal */ export function getKeyForCacheEntry(entry: ICacheEntry): string { - return `${entry.file.docId}_${entry.type}_${entry.key}`; + const version = + "fileVersion" in entry.file.resolvedUrl && entry.file.resolvedUrl.fileVersion !== undefined + ? `_${entry.file.resolvedUrl.fileVersion}` + : ""; + const suffix = entry.type === "snapshot" ? "" : `_${entry.key}`; + return `${entry.file.docId}${version}_${entry.type}${suffix}`; } diff --git a/packages/drivers/odsp-driver/src/odspDocumentService.ts b/packages/drivers/odsp-driver/src/odspDocumentService.ts index 9e36bae5457a..9ed2d7865acd 100644 --- a/packages/drivers/odsp-driver/src/odspDocumentService.ts +++ b/packages/drivers/odsp-driver/src/odspDocumentService.ts @@ -348,7 +348,7 @@ export class OdspDocumentService return this._opsCache; } - // Called whenever re receive ops through any channel for this document (snapshot, delta connection, delta storage) + // Called whenever we receive ops through any channel for this document (snapshot, delta connection, delta storage) // We use it to notify caching layer of how stale is snapshot stored in cache. protected opsReceived(ops: ISequencedDocumentMessage[]): void { // No need for two clients to save same ops From 7972883767457500976a587cecf7df0bc3fcb25c Mon Sep 17 00:00:00 2001 From: jzaffiro <110866475+jzaffiro@users.noreply.github.com> Date: Thu, 28 Aug 2025 21:32:59 +0000 Subject: [PATCH 2/7] Use snapshotKey instead of string --- packages/drivers/odsp-driver-definitions/src/odspCache.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/drivers/odsp-driver-definitions/src/odspCache.ts b/packages/drivers/odsp-driver-definitions/src/odspCache.ts index 0334c638ab2b..d9212ef0a6e4 100644 --- a/packages/drivers/odsp-driver-definitions/src/odspCache.ts +++ b/packages/drivers/odsp-driver-definitions/src/odspCache.ts @@ -131,6 +131,6 @@ export function getKeyForCacheEntry(entry: ICacheEntry): string { "fileVersion" in entry.file.resolvedUrl && entry.file.resolvedUrl.fileVersion !== undefined ? `_${entry.file.resolvedUrl.fileVersion}` : ""; - const suffix = entry.type === "snapshot" ? "" : `_${entry.key}`; + const suffix = entry.type === snapshotKey ? "" : `_${entry.key}`; return `${entry.file.docId}${version}_${entry.type}${suffix}`; } From a5f5ac5d10c84418730f2a84834d719ee1ec74fc Mon Sep 17 00:00:00 2001 From: jzaffiro <110866475+jzaffiro@users.noreply.github.com> Date: Thu, 28 Aug 2025 21:35:48 +0000 Subject: [PATCH 3/7] Format snapshots with loading group ids the same as plain snapshots --- packages/drivers/odsp-driver/src/odspCache.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/packages/drivers/odsp-driver/src/odspCache.ts b/packages/drivers/odsp-driver/src/odspCache.ts index 83bd22a8ef1e..1cfc9a6e8695 100644 --- a/packages/drivers/odsp-driver/src/odspCache.ts +++ b/packages/drivers/odsp-driver/src/odspCache.ts @@ -46,6 +46,10 @@ export class LocalPersistentCache implements IPersistedCache { return this.cache.get(key); } + /** + * + * this is the put call that the write call goes into and calls getKeyForCacheEntry + */ async put(entry: ICacheEntry, value: unknown): Promise { const key = getKeyForCacheEntry(entry); this.cache.set(key, value); From dc402b00fe16fd8f17a03b45a6e6013580cdd056 Mon Sep 17 00:00:00 2001 From: jzaffiro <110866475+jzaffiro@users.noreply.github.com> Date: Thu, 28 Aug 2025 21:53:31 +0000 Subject: [PATCH 4/7] Use loading group id and update fluid cache entry function --- .../driver-web-cache/src/FluidCacheIndexedDb.ts | 16 ++++++++++++++-- .../odsp-driver-definitions/src/odspCache.ts | 5 ++++- packages/drivers/odsp-driver/src/odspCache.ts | 4 ---- 3 files changed, 18 insertions(+), 7 deletions(-) diff --git a/packages/drivers/driver-web-cache/src/FluidCacheIndexedDb.ts b/packages/drivers/driver-web-cache/src/FluidCacheIndexedDb.ts index e23a20e2ec3b..f094158a655b 100644 --- a/packages/drivers/driver-web-cache/src/FluidCacheIndexedDb.ts +++ b/packages/drivers/driver-web-cache/src/FluidCacheIndexedDb.ts @@ -4,7 +4,11 @@ */ import { ITelemetryBaseLogger } from "@fluidframework/core-interfaces"; -import { ICacheEntry } from "@fluidframework/odsp-driver-definitions/internal"; +import { + ICacheEntry, + snapshotKey, + snapshotWithLoadingGroupIdKey, +} from "@fluidframework/odsp-driver-definitions/internal"; import { createChildLogger } from "@fluidframework/telemetry-utils/internal"; import { DBSchema, DeleteDBCallbacks, IDBPDatabase, deleteDB, openDB } from "idb"; @@ -25,7 +29,15 @@ export const oldVersionNameMapping: Partial<{ [key: number]: string }> = { }; export function getKeyForCacheEntry(entry: ICacheEntry) { - return `${entry.file.docId}_${entry.type}_${entry.key}`; + const version = + "fileVersion" in entry.file.resolvedUrl && entry.file.resolvedUrl.fileVersion !== undefined + ? `_${entry.file.resolvedUrl.fileVersion}` + : ""; + const suffix = + entry.type === snapshotKey || entry.type === snapshotWithLoadingGroupIdKey + ? "" + : `_${entry.key}`; + return `${entry.file.docId}${version}_${entry.type}${suffix}`; } export function getFluidCacheIndexedDbInstance( diff --git a/packages/drivers/odsp-driver-definitions/src/odspCache.ts b/packages/drivers/odsp-driver-definitions/src/odspCache.ts index d9212ef0a6e4..37931f20d2c8 100644 --- a/packages/drivers/odsp-driver-definitions/src/odspCache.ts +++ b/packages/drivers/odsp-driver-definitions/src/odspCache.ts @@ -131,6 +131,9 @@ export function getKeyForCacheEntry(entry: ICacheEntry): string { "fileVersion" in entry.file.resolvedUrl && entry.file.resolvedUrl.fileVersion !== undefined ? `_${entry.file.resolvedUrl.fileVersion}` : ""; - const suffix = entry.type === snapshotKey ? "" : `_${entry.key}`; + const suffix = + entry.type === snapshotKey || entry.type === snapshotWithLoadingGroupIdKey + ? "" + : `_${entry.key}`; return `${entry.file.docId}${version}_${entry.type}${suffix}`; } diff --git a/packages/drivers/odsp-driver/src/odspCache.ts b/packages/drivers/odsp-driver/src/odspCache.ts index 1cfc9a6e8695..83bd22a8ef1e 100644 --- a/packages/drivers/odsp-driver/src/odspCache.ts +++ b/packages/drivers/odsp-driver/src/odspCache.ts @@ -46,10 +46,6 @@ export class LocalPersistentCache implements IPersistedCache { return this.cache.get(key); } - /** - * - * this is the put call that the write call goes into and calls getKeyForCacheEntry - */ async put(entry: ICacheEntry, value: unknown): Promise { const key = getKeyForCacheEntry(entry); this.cache.set(key, value); From 72ba0bcaad25281023f3c23da3e33427724138c8 Mon Sep 17 00:00:00 2001 From: jzaffiro <110866475+jzaffiro@users.noreply.github.com> Date: Thu, 28 Aug 2025 22:16:57 +0000 Subject: [PATCH 5/7] Update suffix to give the same value as the old function --- packages/drivers/driver-web-cache/src/FluidCacheIndexedDb.ts | 2 +- packages/drivers/odsp-driver-definitions/src/odspCache.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/drivers/driver-web-cache/src/FluidCacheIndexedDb.ts b/packages/drivers/driver-web-cache/src/FluidCacheIndexedDb.ts index f094158a655b..cceaa6dad444 100644 --- a/packages/drivers/driver-web-cache/src/FluidCacheIndexedDb.ts +++ b/packages/drivers/driver-web-cache/src/FluidCacheIndexedDb.ts @@ -35,7 +35,7 @@ export function getKeyForCacheEntry(entry: ICacheEntry) { : ""; const suffix = entry.type === snapshotKey || entry.type === snapshotWithLoadingGroupIdKey - ? "" + ? "_" : `_${entry.key}`; return `${entry.file.docId}${version}_${entry.type}${suffix}`; } diff --git a/packages/drivers/odsp-driver-definitions/src/odspCache.ts b/packages/drivers/odsp-driver-definitions/src/odspCache.ts index 37931f20d2c8..989c097f3e30 100644 --- a/packages/drivers/odsp-driver-definitions/src/odspCache.ts +++ b/packages/drivers/odsp-driver-definitions/src/odspCache.ts @@ -133,7 +133,7 @@ export function getKeyForCacheEntry(entry: ICacheEntry): string { : ""; const suffix = entry.type === snapshotKey || entry.type === snapshotWithLoadingGroupIdKey - ? "" + ? "_" : `_${entry.key}`; return `${entry.file.docId}${version}_${entry.type}${suffix}`; } From ae735ef7694610cc7e1c394e8010dab0dd7b225c Mon Sep 17 00:00:00 2001 From: jzaffiro <110866475+jzaffiro@users.noreply.github.com> Date: Thu, 28 Aug 2025 22:41:26 +0000 Subject: [PATCH 6/7] Use switch statement to clarify entry format --- .../src/FluidCacheIndexedDb.ts | 26 ++++++++++------ .../odsp-driver-definitions/src/odspCache.ts | 31 +++++++++++++------ 2 files changed, 39 insertions(+), 18 deletions(-) diff --git a/packages/drivers/driver-web-cache/src/FluidCacheIndexedDb.ts b/packages/drivers/driver-web-cache/src/FluidCacheIndexedDb.ts index cceaa6dad444..95c67974643a 100644 --- a/packages/drivers/driver-web-cache/src/FluidCacheIndexedDb.ts +++ b/packages/drivers/driver-web-cache/src/FluidCacheIndexedDb.ts @@ -29,15 +29,23 @@ export const oldVersionNameMapping: Partial<{ [key: number]: string }> = { }; export function getKeyForCacheEntry(entry: ICacheEntry) { - const version = - "fileVersion" in entry.file.resolvedUrl && entry.file.resolvedUrl.fileVersion !== undefined - ? `_${entry.file.resolvedUrl.fileVersion}` - : ""; - const suffix = - entry.type === snapshotKey || entry.type === snapshotWithLoadingGroupIdKey - ? "_" - : `_${entry.key}`; - return `${entry.file.docId}${version}_${entry.type}${suffix}`; + switch (entry.type) { + case snapshotWithLoadingGroupIdKey: + case snapshotKey: { + const version = entry.key === undefined ? "" : `_${entry.key}`; + return `${entry.file.docId}${version}_${entry.type}_`; + } + case "ops": { + const version = + "fileVersion" in entry.file.resolvedUrl && + entry.file.resolvedUrl.fileVersion !== undefined + ? `_${entry.file.resolvedUrl.fileVersion}` + : ""; + return `${entry.file.docId}${version}_${entry.type}_${entry.key}`; + } + default: + return `${entry.file.docId}_${entry.type}_${entry.key}`; + } } export function getFluidCacheIndexedDbInstance( diff --git a/packages/drivers/odsp-driver-definitions/src/odspCache.ts b/packages/drivers/odsp-driver-definitions/src/odspCache.ts index 989c097f3e30..ebc6e2534888 100644 --- a/packages/drivers/odsp-driver-definitions/src/odspCache.ts +++ b/packages/drivers/odsp-driver-definitions/src/odspCache.ts @@ -127,13 +127,26 @@ export interface IPersistedCache { * @internal */ export function getKeyForCacheEntry(entry: ICacheEntry): string { - const version = - "fileVersion" in entry.file.resolvedUrl && entry.file.resolvedUrl.fileVersion !== undefined - ? `_${entry.file.resolvedUrl.fileVersion}` - : ""; - const suffix = - entry.type === snapshotKey || entry.type === snapshotWithLoadingGroupIdKey - ? "_" - : `_${entry.key}`; - return `${entry.file.docId}${version}_${entry.type}${suffix}`; + switch (entry.type) { + case snapshotWithLoadingGroupIdKey: + case snapshotKey: { + const version = entry.key === undefined ? "" : `_${entry.key}`; + // example versioned entry: docId_4.0_snapshot_ + // example non-versioned entry: docId_snapshot_ + // The trailing '_' is included for consistency with existing cache entries + return `${entry.file.docId}${version}_${entry.type}_`; + } + case "ops": { + const version = + "fileVersion" in entry.file.resolvedUrl && + entry.file.resolvedUrl.fileVersion !== undefined + ? `_${entry.file.resolvedUrl.fileVersion}` + : ""; + // example versioned entry: docId_4.0_ops_100_3 + // example non-versioned entry: docId_ops_100_3 + return `${entry.file.docId}${version}_${entry.type}_${entry.key}`; + } + default: + return `${entry.file.docId}_${entry.type}_${entry.key}`; + } } From 04c8817cec08bec48a1dce0ab727b4b11f8e536a Mon Sep 17 00:00:00 2001 From: jzaffiro <110866475+jzaffiro@users.noreply.github.com> Date: Tue, 2 Sep 2025 22:23:47 +0000 Subject: [PATCH 7/7] Apply pr feedback --- .../src/FluidCacheIndexedDb.ts | 21 +--- .../odsp-driver-definitions/src/odspCache.ts | 10 +- packages/drivers/odsp-driver/src/odspUtils.ts | 2 +- .../src/test/getKeyForCacheEntry.spec.ts | 98 +++++++++++++++++++ 4 files changed, 105 insertions(+), 26 deletions(-) create mode 100644 packages/drivers/odsp-driver/src/test/getKeyForCacheEntry.spec.ts diff --git a/packages/drivers/driver-web-cache/src/FluidCacheIndexedDb.ts b/packages/drivers/driver-web-cache/src/FluidCacheIndexedDb.ts index 95c67974643a..6b4fed7fce44 100644 --- a/packages/drivers/driver-web-cache/src/FluidCacheIndexedDb.ts +++ b/packages/drivers/driver-web-cache/src/FluidCacheIndexedDb.ts @@ -6,8 +6,7 @@ import { ITelemetryBaseLogger } from "@fluidframework/core-interfaces"; import { ICacheEntry, - snapshotKey, - snapshotWithLoadingGroupIdKey, + getKeyForCacheEntry as odspGetKeyForCacheEntry, } from "@fluidframework/odsp-driver-definitions/internal"; import { createChildLogger } from "@fluidframework/telemetry-utils/internal"; import { DBSchema, DeleteDBCallbacks, IDBPDatabase, deleteDB, openDB } from "idb"; @@ -29,23 +28,7 @@ export const oldVersionNameMapping: Partial<{ [key: number]: string }> = { }; export function getKeyForCacheEntry(entry: ICacheEntry) { - switch (entry.type) { - case snapshotWithLoadingGroupIdKey: - case snapshotKey: { - const version = entry.key === undefined ? "" : `_${entry.key}`; - return `${entry.file.docId}${version}_${entry.type}_`; - } - case "ops": { - const version = - "fileVersion" in entry.file.resolvedUrl && - entry.file.resolvedUrl.fileVersion !== undefined - ? `_${entry.file.resolvedUrl.fileVersion}` - : ""; - return `${entry.file.docId}${version}_${entry.type}_${entry.key}`; - } - default: - return `${entry.file.docId}_${entry.type}_${entry.key}`; - } + return odspGetKeyForCacheEntry(entry); } export function getFluidCacheIndexedDbInstance( diff --git a/packages/drivers/odsp-driver-definitions/src/odspCache.ts b/packages/drivers/odsp-driver-definitions/src/odspCache.ts index ebc6e2534888..2352346f9b69 100644 --- a/packages/drivers/odsp-driver-definitions/src/odspCache.ts +++ b/packages/drivers/odsp-driver-definitions/src/odspCache.ts @@ -127,21 +127,19 @@ export interface IPersistedCache { * @internal */ export function getKeyForCacheEntry(entry: ICacheEntry): string { + const version = + "fileVersion" in entry.file.resolvedUrl && entry.file.resolvedUrl.fileVersion !== undefined + ? `_${entry.file.resolvedUrl.fileVersion}` + : ""; switch (entry.type) { case snapshotWithLoadingGroupIdKey: case snapshotKey: { - const version = entry.key === undefined ? "" : `_${entry.key}`; // example versioned entry: docId_4.0_snapshot_ // example non-versioned entry: docId_snapshot_ // The trailing '_' is included for consistency with existing cache entries return `${entry.file.docId}${version}_${entry.type}_`; } case "ops": { - const version = - "fileVersion" in entry.file.resolvedUrl && - entry.file.resolvedUrl.fileVersion !== undefined - ? `_${entry.file.resolvedUrl.fileVersion}` - : ""; // example versioned entry: docId_4.0_ops_100_3 // example non-versioned entry: docId_ops_100_3 return `${entry.file.docId}${version}_${entry.type}_${entry.key}`; diff --git a/packages/drivers/odsp-driver/src/odspUtils.ts b/packages/drivers/odsp-driver/src/odspUtils.ts index aaa5ad66c010..5b73d4d115f0 100644 --- a/packages/drivers/odsp-driver/src/odspUtils.ts +++ b/packages/drivers/odsp-driver/src/odspUtils.ts @@ -467,7 +467,7 @@ export function createCacheSnapshotKey( ): ICacheEntry { const cacheEntry: ICacheEntry = { type: snapshotWithLoadingGroupId ? snapshotWithLoadingGroupIdKey : snapshotKey, - key: odspResolvedUrl.fileVersion ?? "", + key: "", file: { resolvedUrl: odspResolvedUrl, docId: odspResolvedUrl.hashedDocumentId, diff --git a/packages/drivers/odsp-driver/src/test/getKeyForCacheEntry.spec.ts b/packages/drivers/odsp-driver/src/test/getKeyForCacheEntry.spec.ts new file mode 100644 index 000000000000..8c1946dc6594 --- /dev/null +++ b/packages/drivers/odsp-driver/src/test/getKeyForCacheEntry.spec.ts @@ -0,0 +1,98 @@ +/*! + * Copyright (c) Microsoft Corporation and contributors. All rights reserved. + * Licensed under the MIT License. + */ + +import { strict as assert } from "node:assert"; + +import { + getKeyForCacheEntry, + type CacheContentType, + type ICacheEntry, + type IOdspResolvedUrl, +} from "@fluidframework/odsp-driver-definitions/internal"; + +function createMockCacheEntry( + url: IOdspResolvedUrl, + type: CacheContentType, + key: string, +): ICacheEntry { + return { + type, + key, + file: { + resolvedUrl: url, + docId: "test-doc-id", + }, + }; +} + +// Define both cache entry types here for readability +const opType = "ops"; +const snapshotType = "snapshot"; + +describe("getKeyForCacheEntry", () => { + const odspResolvedUrlWithoutVersion: IOdspResolvedUrl = { + type: "fluid", + odspResolvedUrl: true, + id: "1", + siteUrl: "fakeUrl", + driveId: "1", + itemId: "1", + url: "fakeUrl", + hashedDocumentId: "1", + endpoints: { + snapshotStorageUrl: "fakeUrl", + attachmentPOSTStorageUrl: "fakeUrl", + attachmentGETStorageUrl: "fakeUrl", + deltaStorageUrl: "fakeUrl", + }, + tokens: {}, + fileName: "fakeName", + summarizer: false, + fileVersion: undefined, + }; + + const odspResolvedUrlWithVersion: IOdspResolvedUrl = { + type: "fluid", + odspResolvedUrl: true, + id: "1", + siteUrl: "fakeUrl", + driveId: "1", + itemId: "1", + url: "fakeUrl", + hashedDocumentId: "1", + endpoints: { + snapshotStorageUrl: "fakeUrl", + attachmentPOSTStorageUrl: "fakeUrl", + attachmentGETStorageUrl: "fakeUrl", + deltaStorageUrl: "fakeUrl", + }, + tokens: {}, + fileName: "fakeName", + summarizer: false, + fileVersion: "3.0", + }; + + it("creates a non-versioned snapshot cache entry", () => { + const entry = createMockCacheEntry(odspResolvedUrlWithoutVersion, snapshotType, ""); + const key = getKeyForCacheEntry(entry); + assert.equal(key, "test-doc-id_snapshot_"); + }); + it("creates a versioned snapshot cache entry", () => { + const entry = createMockCacheEntry(odspResolvedUrlWithVersion, snapshotType, ""); + const key = getKeyForCacheEntry(entry); + assert.equal(key, "test-doc-id_3.0_snapshot_"); + }); + + it("creates a non-versioned op cache entry", () => { + const entry = createMockCacheEntry(odspResolvedUrlWithoutVersion, opType, "100_5"); + const key = getKeyForCacheEntry(entry); + assert.equal(key, "test-doc-id_ops_100_5"); + }); + it("creates a versioned op cache entry", () => { + const entry = createMockCacheEntry(odspResolvedUrlWithVersion, opType, "100_5"); + const key = getKeyForCacheEntry(entry); + assert.equal(key, "test-doc-id_3.0_ops_100_5"); + }); +});