Add an assertion when updating conversations; update cleanData

ts/sql/Client.ts

@@ -16,15 +16,17 @@ import {
   get,
   groupBy,
   isFunction,
-  isObject,
   last,
   map,
+  omit,
   set,
 } from 'lodash';
 
 import { arrayBufferToBase64, base64ToArrayBuffer } from '../Crypto';
 import { CURRENT_SCHEMA_VERSION } from '../../js/modules/types/message';
 import { createBatcher } from '../util/batcher';
+import { assert } from '../util/assert';
+import { cleanDataForIpc } from './cleanDataForIpc';
 
 import {
   ConversationModelCollectionType,
@@ -231,7 +233,6 @@ const dataInterface: ClientInterface = {
   // Client-side only, and test-only
 
   _removeConversations,
-  _cleanData,
   _jobs,
 };
 
@@ -251,55 +252,22 @@ const channelsAsUnknown = fromPairs(
 
 const channels: ServerInterface = channelsAsUnknown;
 
-// When IPC arguments are prepared for the cross-process send, they are serialized with
-//   the [structured clone algorithm][0]. We can't send some values, like BigNumbers and
-//   functions (both of which come from protobufjs), so we clean them up.
-// [0]: https://developer.mozilla.org/en-US/docs/Web/API/Web_Workers_API/Structured_clone_algorithm
-function _cleanData(data: any, path = 'root') {
-  if (data === null || data === undefined) {
-    window.log.warn(`_cleanData: null or undefined value at path ${path}`);
+function _cleanData(
+  data: unknown
+): ReturnType<typeof cleanDataForIpc>['cleaned'] {
+  const { cleaned, pathsChanged } = cleanDataForIpc(data);
 
-    return data;
+  if (pathsChanged.length) {
+    window.log.info(
+      `_cleanData cleaned the following paths: ${pathsChanged.join(', ')}`
+    );
   }
 
-  if (
-    typeof data === 'string' ||
-    typeof data === 'number' ||
-    typeof data === 'boolean'
-  ) {
-    return data;
-  }
+  return cleaned;
+}
 
-  const keys = Object.keys(data);
-  const max = keys.length;
-  for (let index = 0; index < max; index += 1) {
-    const key = keys[index];
-    const value = data[key];
-
-    if (value === null || value === undefined) {
-      continue;
-    }
-
-    if (isFunction(value)) {
-      delete data[key];
-    } else if (isFunction(value.toNumber)) {
-      data[key] = value.toNumber();
-    } else if (Array.isArray(value)) {
-      data[key] = value.map((item, mapIndex) =>
-        _cleanData(item, `${path}.${key}.${mapIndex}`)
-      );
-    } else if (isObject(value)) {
-      data[key] = _cleanData(value, `${path}.${key}`);
-    } else if (
-      typeof value !== 'string' &&
-      typeof value !== 'number' &&
-      typeof value !== 'boolean'
-    ) {
-      window.log.info(`_cleanData: key ${key} had type ${typeof value}`);
-    }
-  }
-
-  return data;
+function _cleanMessageData(data: MessageType): MessageType {
+  return _cleanData(omit(data, ['dataMessage']));
 }
 
 async function _shutdown() {
@@ -764,7 +732,12 @@ function updateConversation(data: ConversationType) {
 }
 
 async function updateConversations(array: Array<ConversationType>) {
-  await channels.updateConversations(array);
+  const { cleaned, pathsChanged } = cleanDataForIpc(array);
+  assert(
+    !pathsChanged.length,
+    `Paths were cleaned: ${JSON.stringify(pathsChanged)}`
+  );
+  await channels.updateConversations(cleaned);
 }
 
 async function removeConversation(
@@ -884,7 +857,9 @@ async function saveMessage(
   data: MessageType,
   { forceSave, Message }: { forceSave?: boolean; Message: typeof MessageModel }
 ) {
-  const id = await channels.saveMessage(_cleanData(data), { forceSave });
+  const id = await channels.saveMessage(_cleanMessageData(data), {
+    forceSave,
+  });
   Message.updateTimers();
 
   return id;
@@ -894,7 +869,10 @@ async function saveMessages(
   arrayOfMessages: Array<MessageType>,
   { forceSave }: { forceSave?: boolean } = {}
 ) {
-  await channels.saveMessages(_cleanData(arrayOfMessages), { forceSave });
+  await channels.saveMessages(
+    arrayOfMessages.map(message => _cleanMessageData(message)),
+    { forceSave }
+  );
 }
 
 async function removeMessage(

ts/sql/Interface.ts

@@ -393,7 +393,6 @@ export type ClientInterface = DataInterface & {
   // Client-side only, and test-only
 
   _removeConversations: (ids: Array<string>) => Promise<void>;
-  _cleanData: (data: any, path?: string) => any;
   _jobs: { [id: string]: ClientJobType };
 };
 

ts/sql/cleanDataForIpc.ts

@@ -0,0 +1,163 @@
+// Copyright 2021 Signal Messenger, LLC
+// SPDX-License-Identifier: AGPL-3.0-only
+
+import { isPlainObject } from 'lodash';
+
+import { isIterable } from '../util/isIterable';
+
+/**
+ * IPC arguments are serialized with the [structured clone algorithm][0], but we can only
+ * save some data types to disk.
+ *
+ * This cleans the data so it's roughly JSON-serializable, though it does not handle
+ * every case. You can see the expected behavior in the tests. Notably, we try to convert
+ * protobufjs numbers to JavaScript numbers, and we don't touch ArrayBuffers.
+ *
+ * [0]: https://developer.mozilla.org/en-US/docs/Web/API/Web_Workers_API/Structured_clone_algorithm
+ */
+export function cleanDataForIpc(
+  data: unknown
+): {
+  // `any`s are dangerous but it's difficult (impossible?) to type this with generics.
+  // eslint-disable-next-line @typescript-eslint/no-explicit-any
+  cleaned: any;
+  pathsChanged: Array<string>;
+} {
+  const pathsChanged: Array<string> = [];
+  const cleaned = cleanDataInner(data, 'root', pathsChanged);
+  return { cleaned, pathsChanged };
+}
+
+// These type definitions are lifted from [this GitHub comment][1].
+//
+// [1]: https://github.com/Microsoft/TypeScript/issues/3496#issuecomment-128553540
+type CleanedDataValue =
+  | string
+  | number
+  | boolean
+  | null
+  | undefined
+  | CleanedObject
+  | CleanedArray;
+/* eslint-disable no-restricted-syntax */
+interface CleanedObject {
+  [x: string]: CleanedDataValue;
+}
+// eslint-disable-next-line @typescript-eslint/no-empty-interface
+interface CleanedArray extends Array<CleanedDataValue> {}
+/* eslint-enable no-restricted-syntax */
+
+function cleanDataInner(
+  data: unknown,
+  path: string,
+  pathsChanged: Array<string>
+): CleanedDataValue {
+  switch (typeof data) {
+    case 'undefined':
+    case 'boolean':
+    case 'number':
+    case 'string':
+      return data;
+    case 'bigint':
+      pathsChanged.push(path);
+      return data.toString();
+    case 'function':
+      // For backwards compatibility with previous versions of this function, we clean
+      //   functions but don't mark them as cleaned.
+      return undefined;
+    case 'object': {
+      if (data === null) {
+        return null;
+      }
+
+      if (Array.isArray(data)) {
+        const result: CleanedArray = [];
+        data.forEach((item, index) => {
+          const indexPath = `${path}.${index}`;
+          if (item === undefined || item === null) {
+            pathsChanged.push(indexPath);
+          } else {
+            result.push(cleanDataInner(item, indexPath, pathsChanged));
+          }
+        });
+        return result;
+      }
+
+      if (data instanceof Map) {
+        const result: CleanedObject = {};
+        pathsChanged.push(path);
+        data.forEach((value, key) => {
+          if (typeof key === 'string') {
+            result[key] = cleanDataInner(
+              value,
+              `${path}.<map value at ${key}>`,
+              pathsChanged
+            );
+          } else {
+            pathsChanged.push(`${path}.<map key ${String(key)}>`);
+          }
+        });
+        return result;
+      }
+
+      if (data instanceof Date) {
+        pathsChanged.push(path);
+        return Number.isNaN(data.valueOf()) ? undefined : data.toISOString();
+      }
+
+      if (data instanceof ArrayBuffer) {
+        pathsChanged.push(path);
+        return undefined;
+      }
+
+      const dataAsRecord = data as Record<string, unknown>;
+
+      if (
+        'toNumber' in dataAsRecord &&
+        typeof dataAsRecord.toNumber === 'function'
+      ) {
+        // We clean this just in case `toNumber` returns something bogus.
+        return cleanDataInner(dataAsRecord.toNumber(), path, pathsChanged);
+      }
+
+      if (isIterable(dataAsRecord)) {
+        const result: CleanedArray = [];
+        let index = 0;
+        pathsChanged.push(path);
+        // `for ... of` is the cleanest way to go through "generic" iterables without
+        //   a helper library.
+        // eslint-disable-next-line no-restricted-syntax
+        for (const value of dataAsRecord) {
+          result.push(
+            cleanDataInner(
+              value,
+              `${path}.<iterator index ${index}>`,
+              pathsChanged
+            )
+          );
+          index += 1;
+        }
+        return result;
+      }
+
+      // We'll still try to clean non-plain objects, but we want to mark that they've
+      //   changed.
+      if (!isPlainObject(data)) {
+        pathsChanged.push(path);
+      }
+
+      const result: CleanedObject = {};
+
+      // Conveniently, `Object.entries` removes symbol keys.
+      Object.entries(dataAsRecord).forEach(([key, value]) => {
+        result[key] = cleanDataInner(value, `${path}.${key}`, pathsChanged);
+      });
+
+      return result;
+    }
+    default: {
+      pathsChanged.push(path);
+      return undefined;
+    }
+  }
+}