fix: normalize the id used to key cached store records

.changeset/olive-pandas-listen.md

@@ -0,0 +1,5 @@
+---
+'@rocket.chat/meteor': patch
+---
+
+Fixes rooms showing up more than once in the sidebar, caused by the cached stores keeping a separate entry for every merge of a record whose `_id` does not arrive as a string.

apps/meteor/client/lib/cachedStores/DocumentMapStore.spec.ts

@@ -0,0 +1,121 @@
+import { createDocumentMapStore, toRecordId } from './DocumentMapStore';
+
+const hexId = '6a7fc7b8376b837b4b8aa6c3';
+const binaryId = Uint8Array.from(hexId.match(/../g) ?? [], (byte) => parseInt(byte, 16));
+
+interface ITestRecord {
+	_id: string;
+	rid: string;
+}
+
+const fromBinary = { _id: binaryId as unknown as string, rid: hexId };
+const fromString = { _id: hexId, rid: hexId };
+const objectId = { toHexString: () => hexId };
+
+describe('cachedStores/DocumentMapStore', () => {
+	describe('toRecordId', () => {
+		it('should keep a string id as it is', () => {
+			expect(toRecordId(hexId)).toBe(hexId);
+		});
+
+		it('should read the hexadecimal id out of a binary id', () => {
+			expect(toRecordId(binaryId)).toBe(hexId);
+		});
+
+		it('should read the hexadecimal id out of an ObjectId', () => {
+			expect(toRecordId(objectId)).toBe(hexId);
+		});
+	});
+
+	describe('createDocumentMapStore', () => {
+		it('should store a document carrying a binary id under its hexadecimal id', () => {
+			const store = createDocumentMapStore<ITestRecord>();
+
+			store.getState().store(fromBinary);
+
+			expect([...store.getState().records.keys()]).toEqual([hexId]);
+			expect(store.getState().has(hexId)).toBe(true);
+			expect(store.getState().has(fromBinary._id)).toBe(true);
+			expect(store.getState().get(hexId)?.rid).toBe(hexId);
+		});
+
+		it('should not store the same document twice when its id arrives in different representations', () => {
+			const store = createDocumentMapStore<ITestRecord>();
+
+			store.getState().store(fromBinary);
+			store.getState().store(fromString);
+
+			expect(store.getState().records.size).toBe(1);
+		});
+
+		it('should not duplicate a document when many records are stored at once', () => {
+			const store = createDocumentMapStore<ITestRecord>();
+
+			store.getState().storeMany([fromBinary, fromString]);
+
+			expect(store.getState().records.size).toBe(1);
+		});
+
+		it('should collapse the records of a cache written with mixed representations', () => {
+			const store = createDocumentMapStore<ITestRecord>();
+
+			store.getState().replaceAll([fromString, fromBinary, fromBinary, fromString]);
+
+			expect(store.getState().records.size).toBe(1);
+		});
+
+		it('should delete a document by any representation of its id', () => {
+			const store = createDocumentMapStore<ITestRecord>();
+
+			store.getState().store(fromBinary);
+			store.getState().delete(fromBinary._id);
+
+			expect(store.getState().records.size).toBe(0);
+		});
+
+		it('should merge a document carrying an ObjectId with the same document carrying a string id', () => {
+			const store = createDocumentMapStore<ITestRecord>();
+
+			store.getState().store({ _id: objectId as unknown as string, rid: hexId });
+			store.getState().store(fromString);
+
+			expect(store.getState().records.size).toBe(1);
+			expect([...store.getState().records.keys()]).toEqual([hexId]);
+		});
+
+		it('should keep an updated document keyed by its hexadecimal id', () => {
+			const store = createDocumentMapStore<ITestRecord>();
+
+			store.getState().store(fromBinary);
+			store.getState().update(
+				(record) => record.rid === hexId,
+				(record) => ({ ...record }),
+			);
+
+			expect(store.getState().records.size).toBe(1);
+			expect([...store.getState().records.keys()]).toEqual([hexId]);
+		});
+
+		it('should keep an asynchronously updated document keyed by its hexadecimal id', async () => {
+			const store = createDocumentMapStore<ITestRecord>();
+
+			store.getState().store(fromBinary);
+			await store.getState().updateAsync(
+				(record) => record.rid === hexId,
+				async (record) => ({ ...record }),
+			);
+
+			expect(store.getState().records.size).toBe(1);
+			expect([...store.getState().records.keys()]).toEqual([hexId]);
+		});
+
+		it('should remove a document that was stored under a binary id', () => {
+			const store = createDocumentMapStore<ITestRecord>();
+
+			store.getState().store(fromBinary);
+			store.getState().remove((record) => record.rid === hexId);
+
+			expect(store.getState().records.size).toBe(0);
+		});
+	});
+});

apps/meteor/client/lib/cachedStores/DocumentMapStore.ts

@@ -1,5 +1,46 @@
 import { create } from 'zustand';
 
+const bytesToHex = (bytes: Uint8Array): string => Array.from(bytes, (byte) => byte.toString(16).padStart(2, '0')).join('');
+
+/**
+ * Documents are identified by a string `_id`, but a document can reach the store
+ * carrying another representation of the same id, e.g. a BSON ObjectId or the raw
+ * bytes of an EJSON binary. Those are not stable map keys: every merge of such a
+ * document stores a new entry instead of replacing the previous one, which makes
+ * repeated records pile up in the store (and in the persisted cache).
+ *
+ * @param id - The id of a document, in any of the representations it may arrive in.
+ * @returns The id as a string.
+ */
+export const toRecordId = (id: unknown): string => {
+	if (typeof id === 'string') {
+		return id;
+	}
+
+	// an ObjectId carries the same 24-character id in its hexadecimal form
+	if (typeof (id as { toHexString?: unknown })?.toHexString === 'function') {
+		return (id as { toHexString: () => string }).toHexString();
+	}
+
+	if (ArrayBuffer.isView(id)) {
+		return bytesToHex(new Uint8Array(id.buffer, id.byteOffset, id.byteLength));
+	}
+
+	return JSON.stringify(id) ?? String(id);
+};
+
+const withStringId = <T extends { _id: string }>(record: T): T => {
+	const _id = toRecordId(record._id);
+
+	return _id === record._id ? record : { ...record, _id };
+};
+
+const toEntry = <T extends { _id: string }>(record: T): [string, T] => {
+	const stored = withStringId(record);
+
+	return [stored._id, stored];
+};
+
 export interface IDocumentMapStore<T extends { _id: string }> {
 	readonly records: ReadonlyMap<T['_id'], T>;
 	/**
@@ -169,8 +210,8 @@ export interface IDocumentMapStoreHooks<T extends { _id: string }> {
 export const createDocumentMapStore = <T extends { _id: string }>({ onInvalidate, onInvalidateAll }: IDocumentMapStoreHooks<T> = {}) =>
 	create<IDocumentMapStore<T>>()((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(toRecordId(id)),
+		get: (id: T['_id']) => get().records.get(toRecordId(id)),
 		some: (predicate: (record: T) => boolean) => {
 			for (const record of get().records.values()) {
 				if (predicate(record)) return true;
@@ -213,31 +254,39 @@ export const createDocumentMapStore = <T extends { _id: string }>({ onInvalidate
 			return index;
 		},
 		replaceAll: (records: T[]) => {
-			set({ records: new Map(records.map((record) => [record._id, record])) });
+			set({ records: new Map(records.map(toEntry)) });
 			onInvalidateAll?.();
 		},
 		store: (doc) => {
-			set((state) => ({ records: new Map(state.records).set(doc._id, doc) }));
-			onInvalidate?.(doc);
+			const record = withStringId(doc);
+
+			set((state) => ({ records: new Map(state.records).set(record._id, record) }));
+			onInvalidate?.(record);
 		},
 		storeMany: (docs) => {
+			const stored: T[] = [];
+
 			set((state) => {
 				const records = new Map(state.records);
 
 				for (const doc of docs) {
-					records.set(doc._id, doc);
+					const entry = toEntry(doc);
+
+					records.set(...entry);
+					stored.push(entry[1]);
 				}
 
 				return { records };
 			});
-			onInvalidate?.(...docs);
+			onInvalidate?.(...stored);
 		},
 		delete: (_id) => {
 			const affected: T[] = [];
+			const key = toRecordId(_id);
 			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);
@@ -249,9 +298,10 @@ export const createDocumentMapStore = <T extends { _id: string }>({ onInvalidate
 				const records = new Map<T['_id'], T>();
 				for (const record of state.records.values()) {
 					if (predicate(record)) {
-						const newRecord = modifier(record);
-						records.set(record._id, newRecord);
-						if (onInvalidate) affected.push(newRecord);
+						const entry = toEntry(modifier(record));
+
+						records.set(...entry);
+						if (onInvalidate) affected.push(entry[1]);
 					} else {
 						records.set(record._id, record);
 					}
@@ -268,9 +318,10 @@ export const createDocumentMapStore = <T extends { _id: string }>({ onInvalidate
 
 			for await (const record of get().records.values()) {
 				if (predicate(record)) {
-					const newRecord = await modifier(record);
-					records.set(record._id, newRecord);
-					if (onInvalidate) affected.push(newRecord);
+					const entry = toEntry(await modifier(record));
+
+					records.set(...entry);
+					if (onInvalidate) affected.push(entry[1]);
 				} else {
 					records.set(record._id, record);
 				}