diff --git a/.changeset/fix-duplicate-rooms-non-string-id.md b/.changeset/fix-duplicate-rooms-non-string-id.md new file mode 100644 index 0000000000000..66a98b48adb0f --- /dev/null +++ b/.changeset/fix-duplicate-rooms-non-string-id.md @@ -0,0 +1,5 @@ +--- +'@rocket.chat/meteor': patch +--- + +Fixes duplicate rooms/subscriptions accumulating in the client-side cache (and sidebar) when a record's `_id` arrives as something other than a plain string (e.g. a BSON ObjectId instance or an EJSON binary wrapper, as can happen with manually inserted or migrated documents). `DocumentMapStore` now normalizes `_id` to a stable string before using it as the underlying Map key, so repeated merges of the same record replace the previous entry instead of piling up a new one each time. diff --git a/apps/meteor/client/lib/cachedStores/DocumentMapStore.spec.ts b/apps/meteor/client/lib/cachedStores/DocumentMapStore.spec.ts new file mode 100644 index 0000000000000..ef7bccfd335d2 --- /dev/null +++ b/apps/meteor/client/lib/cachedStores/DocumentMapStore.spec.ts @@ -0,0 +1,64 @@ +import { createDocumentMapStore } from './DocumentMapStore'; + +interface ITestRecord { + _id: string; + name: string; +} + +describe('DocumentMapStore', () => { + describe('non-string _id normalization', () => { + it('replaces the previous entry when the same BSON ObjectId-like _id is stored twice', () => { + const useStore = createDocumentMapStore(); + + // A Mongo ObjectId instance: not a string, but exposes toHexString(). + const objectIdLike = { toHexString: () => '507f1f77bcf86cd799439011' }; + + useStore.getState().store({ _id: objectIdLike as unknown as string, name: 'first' }); + // A distinct object instance representing the *same* id, as would arrive on a second DDP merge. + const secondInstance = { toHexString: () => '507f1f77bcf86cd799439011' }; + useStore.getState().store({ _id: secondInstance as unknown as string, name: 'second' }); + + expect(useStore.getState().records.size).toBe(1); + expect(Array.from(useStore.getState().records.values())[0].name).toBe('second'); + }); + + it('replaces the previous entry when the same EJSON binary _id is stored twice', () => { + const useStore = createDocumentMapStore(); + + const binaryId = { buffer: { $binary: 'an/HuDdrg3tLiqbD' } }; + + useStore.getState().store({ _id: binaryId as unknown as string, name: 'first' }); + useStore.getState().store({ _id: { buffer: { $binary: 'an/HuDdrg3tLiqbD' } } as unknown as string, name: 'second' }); + + expect(useStore.getState().records.size).toBe(1); + }); + + it('deletes a record stored with a non-string _id when given an equivalent but distinct instance', () => { + const useStore = createDocumentMapStore(); + + useStore.getState().store({ _id: { toHexString: () => 'abc123' } as unknown as string, name: 'first' }); + useStore.getState().delete({ toHexString: () => 'abc123' } as unknown as string); + + expect(useStore.getState().records.size).toBe(0); + }); + + it('does not collapse different ids into the same entry', () => { + const useStore = createDocumentMapStore(); + + useStore.getState().store({ _id: { toHexString: () => 'aaa' } as unknown as string, name: 'first' }); + useStore.getState().store({ _id: { toHexString: () => 'bbb' } as unknown as string, name: 'second' }); + + expect(useStore.getState().records.size).toBe(2); + }); + + it('still works for plain string ids (no regression)', () => { + const useStore = createDocumentMapStore(); + + useStore.getState().store({ _id: 'room-1', name: 'first' }); + useStore.getState().store({ _id: 'room-1', name: 'updated' }); + + expect(useStore.getState().records.size).toBe(1); + expect(useStore.getState().get('room-1')?.name).toBe('updated'); + }); + }); +}); diff --git a/apps/meteor/client/lib/cachedStores/DocumentMapStore.ts b/apps/meteor/client/lib/cachedStores/DocumentMapStore.ts index 551fa25915e52..61dc4de3f69a0 100644 --- a/apps/meteor/client/lib/cachedStores/DocumentMapStore.ts +++ b/apps/meteor/client/lib/cachedStores/DocumentMapStore.ts @@ -1,5 +1,38 @@ import { create } from 'zustand'; +const isBinaryLike = (value: unknown): value is { $binary: string } => + typeof value === 'object' && value !== null && typeof (value as { $binary?: unknown }).$binary === 'string'; + +/** + * Derives a stable string key for a document's `_id`. + * + * `_id` is typed as `string`, but records coming from DDP/EJSON, imports or manual database + * writes can carry other representations of an id (a BSON ObjectId instance, an EJSON binary + * wrapper, etc). Using such a value directly as a `Map` key relies on object identity: every + * time an "equal" but distinct instance of the same id arrives, it collides with nothing and a + * duplicate entry is stored instead of replacing the previous one. Normalizing to a string here + * keeps `store`/`storeMany`/`has`/`get`/`delete` consistent regardless of how the id arrived. + */ +const normalizeId = (id: TId): TId | string => { + if (typeof id === 'string' || typeof id !== 'object' || id === null) { + return id; + } + + if (typeof (id as { toHexString?: unknown }).toHexString === 'function') { + return (id as unknown as { toHexString: () => string }).toHexString(); + } + + if (isBinaryLike(id)) { + return id.$binary; + } + + if ('buffer' in id && isBinaryLike((id as { buffer: unknown }).buffer)) { + return (id as unknown as { buffer: { $binary: string } }).buffer.$binary; + } + + return JSON.stringify(id); +}; + export interface IDocumentMapStore { readonly records: ReadonlyMap; /** @@ -169,8 +202,8 @@ export interface IDocumentMapStoreHooks { export const createDocumentMapStore = ({ onInvalidate, onInvalidateAll }: IDocumentMapStoreHooks = {}) => create>()((set, get) => ({ records: new Map(), - has: (id: T['_id']) => get().records.has(id), - get: (id: T['_id']) => get().records.get(id), + has: (id: T['_id']) => get().records.has(normalizeId(id)), + get: (id: T['_id']) => get().records.get(normalizeId(id)), some: (predicate: (record: T) => boolean) => { for (const record of get().records.values()) { if (predicate(record)) return true; @@ -213,11 +246,11 @@ export const createDocumentMapStore = ({ onInvalidate return index; }, replaceAll: (records: T[]) => { - set({ records: new Map(records.map((record) => [record._id, record])) }); + set({ records: new Map(records.map((record) => [normalizeId(record._id), record])) }); onInvalidateAll?.(); }, store: (doc) => { - set((state) => ({ records: new Map(state.records).set(doc._id, doc) })); + set((state) => ({ records: new Map(state.records).set(normalizeId(doc._id), doc) })); onInvalidate?.(doc); }, storeMany: (docs) => { @@ -225,7 +258,7 @@ export const createDocumentMapStore = ({ onInvalidate const records = new Map(state.records); for (const doc of docs) { - records.set(doc._id, doc); + records.set(normalizeId(doc._id), doc); } return { records }; @@ -233,11 +266,12 @@ export const createDocumentMapStore = ({ onInvalidate onInvalidate?.(...docs); }, delete: (_id) => { + const key = normalizeId(_id); const affected: T[] = []; set((state) => { const records = new Map(state.records); - if (onInvalidate) affected.push(state.records.get(_id)!); - records.delete(_id); + if (onInvalidate) affected.push(state.records.get(key)!); + records.delete(key); return { records }; }); onInvalidate?.(...affected); @@ -250,10 +284,10 @@ export const createDocumentMapStore = ({ onInvalidate for (const record of state.records.values()) { if (predicate(record)) { const newRecord = modifier(record); - records.set(record._id, newRecord); + records.set(normalizeId(newRecord._id), newRecord); if (onInvalidate) affected.push(newRecord); } else { - records.set(record._id, record); + records.set(normalizeId(record._id), record); } } @@ -269,10 +303,10 @@ export const createDocumentMapStore = ({ onInvalidate for await (const record of get().records.values()) { if (predicate(record)) { const newRecord = await modifier(record); - records.set(record._id, newRecord); + records.set(normalizeId(newRecord._id), newRecord); if (onInvalidate) affected.push(newRecord); } else { - records.set(record._id, record); + records.set(normalizeId(record._id), record); } } @@ -289,7 +323,7 @@ export const createDocumentMapStore = ({ onInvalidate if (onInvalidate) affected.push(record); continue; } - records.set(record._id, record); + records.set(normalizeId(record._id), record); } return { records };