Merge pull request #1540 from javabudd/SESSION-1533

fix: ignore invalid conversation records by requiring id to be a string

Author: Bilb

ts/data/data.ts

@@ -142,7 +142,9 @@ async function getAllConversations(): Promise<Array<ConversationModel>> {
   const conversationsAttrs =
     (await channels.getAllConversations()) as Array<ConversationAttributes>;
 
-  return conversationsAttrs.map(attr => new ConversationModel(attr));
+  return conversationsAttrs
+    .filter(attr => typeof attr?.id === 'string' && attr.id.length)
+    .map(attr => new ConversationModel(attr));
 }
 
 /**

ts/node/sql.ts

@@ -196,6 +196,7 @@ async function initializeSql({
 
     console.info('total message count before cleaning: ', getMessageCount());
     console.info('total conversation count before cleaning: ', getConversationCount());
+    cleanUpInvalidConversationIds();
     cleanUpOldOpengroupsOnStart();
     cleanUpUnusedNodeForKeyEntriesOnStart();
     cleanUpUnreadExpiredDaRMessages();
@@ -1851,6 +1852,19 @@ function cleanUpUnreadExpiredDaRMessages() {
   );
 }
 
+/**
+ * Clean up invalid conversation ids.
+ * Even though the `ID` in the `CONVERSATIONS_TABLE` is defined in the sqlite3 schema as being a string primary key,
+ * this does not enforce NOT NULL in sqlite3. TODO: consider a database migration to fix this long-term
+ */
+function cleanUpInvalidConversationIds() {
+  const deleteResult = assertGlobalInstance()
+    .prepare(`DELETE FROM ${CONVERSATIONS_TABLE} WHERE id = '' OR id IS NULL OR typeof(id) != 'text';`)
+    .run();
+
+  console.info(`cleanUpInvalidConversationIds removed ${deleteResult.changes} rows`);
+}
+
 function getOutgoingWithoutExpiresAt() {
   const rows = assertGlobalInstance()
     .prepare(

ts/test/data/data_test.ts

@@ -0,0 +1,60 @@
+import { afterEach, beforeEach, describe } from 'mocha';
+import Sinon from 'sinon';
+import { expect } from 'chai';
+import { Data } from '../../data/data';
+import { ConversationModel } from '../../models/conversation';
+import { channels } from '../../data/channels';
+
+describe('data', () => {
+  afterEach(() => {
+    Sinon.restore();
+  });
+
+  describe('getAllConversations', () => {
+    let getAllConversationsStub: Record<string, any>;
+    beforeEach(() => {
+      channels.getAllConversations = () => {};
+
+      getAllConversationsStub = Sinon.stub(channels, 'getAllConversations');
+    });
+
+    it('returns empty array when channels.getAllConversations yields invalid conversations', async () => {
+      getAllConversationsStub.resolves([{} as any, { id: 123 } as any]);
+
+      const conversations = await Data.getAllConversations();
+
+      expect(conversations).to.be.an('array');
+      expect(conversations).to.deep.equal([]);
+      expect(getAllConversationsStub.calledOnce).to.eq(true);
+    });
+
+    it('returns array of ConversationModel for valid conversations', async () => {
+      const attrs = [{ id: 'abc' } as any, { id: 'def' } as any];
+      const stub = getAllConversationsStub.resolves(attrs);
+
+      const conversations = await Data.getAllConversations();
+
+      expect(conversations).to.be.an('array');
+      expect(conversations).to.have.length(2);
+      expect(conversations[0]).to.be.instanceOf(ConversationModel);
+      expect(conversations[1]).to.be.instanceOf(ConversationModel);
+      expect(stub.calledOnce).to.eq(true);
+    });
+
+    it('filters out invalid and keeps valid when mixed', async () => {
+      const stub = getAllConversationsStub.resolves([
+        { id: 'ok' } as any,
+        {} as any,
+        { id: null } as any,
+        { id: 'ok2' } as any,
+      ]);
+
+      const conversations = await Data.getAllConversations();
+
+      expect(conversations).to.be.an('array');
+      expect(conversations).to.have.length(2);
+      expect(conversations.every(c => c instanceof ConversationModel)).to.eq(true);
+      expect(stub.calledOnce).to.eq(true);
+    });
+  });
+});