[lib] Fix SQLite migrations

Summary: The migrations are broken because we weren't using the right shape of the specification. This diff fixes it and introduces necessary type checks. https://linear.app/comm/issue/ENG-9467/redux-migrations-are-broken Test Plan: Tested this by creating a new migration on the web where a new draft is created. Run the app, and reopened it. Checked if the `SET_CLIENT_DB_STORE` action contains the draft. Reviewers: kamil, ashoat, atul Reviewed By: ashoat Differential Revision: https://phab.comm.dev/D13571
CommE2E · Oct 2, 2024 · 85789c8 · 85789c8
1 parent fb674c2
commit 85789c8
Show file tree

Hide file tree

Showing 5 changed files with 106 additions and 90 deletions.
diff --git a/lib/shared/redux/client-db-utils.js b/lib/shared/redux/client-db-utils.js
@@ -11,8 +11,8 @@ import { messageStoreOpsHandlers } from '../../ops/message-store-ops.js';
 import { reportStoreOpsHandlers } from '../../ops/report-store-ops.js';
 import { syncedMetadataStoreOpsHandlers } from '../../ops/synced-metadata-store-ops.js';
 import { threadActivityStoreOpsHandlers } from '../../ops/thread-activity-store-ops.js';
-import type { ClientDBThreadStoreOperation } from '../../ops/thread-store-ops.js';
 import { threadStoreOpsHandlers } from '../../ops/thread-store-ops.js';
+import type { ThreadStoreOperation } from '../../ops/thread-store-ops.js';
 import { userStoreOpsHandlers } from '../../ops/user-store-ops.js';
 import type {
   ClientDBStoreOperations,
@@ -23,15 +23,12 @@ import type {
   ClientDBThreadInfo,
 } from '../../types/thread-types.js';
 import { values } from '../../utils/objects.js';
-import {
-  convertClientDBThreadInfoToRawThreadInfo,
-  convertRawThreadInfoToClientDBThreadInfo,
-} from '../../utils/thread-ops-utils.js';
+import { convertClientDBThreadInfoToRawThreadInfo } from '../../utils/thread-ops-utils.js';
 
 function createUpdateDBOpsForThreadStoreThreadInfos(
   clientDBThreadInfos: $ReadOnlyArray<ClientDBThreadInfo>,
   migrationFunc: RawThreadInfos => RawThreadInfos,
-): $ReadOnlyArray<ClientDBThreadStoreOperation> {
+): $ReadOnlyArray<ThreadStoreOperation> {
   // 1. Translate `ClientDBThreadInfo`s to `RawThreadInfo`s.
   const rawThreadInfos = clientDBThreadInfos.map(
     convertClientDBThreadInfoToRawThreadInfo,
@@ -46,18 +43,18 @@ function createUpdateDBOpsForThreadStoreThreadInfos(
   // 4. Convert the updated `RawThreadInfos` back into an array.
   const updatedKeyedRawThreadInfosArray = values(updatedKeyedRawThreadInfos);
 
-  // 5. Translate `RawThreadInfo`s back to `ClientDBThreadInfo`s.
-  const updatedClientDBThreadInfos = updatedKeyedRawThreadInfosArray.map(
-    convertRawThreadInfoToClientDBThreadInfo,
+  // 5. Construct `replace` `ThreadStoreOperation`s.
+  const replaceThreadOperations = updatedKeyedRawThreadInfosArray.map(
+    thread => ({
+      type: 'replace',
+      payload: {
+        id: thread.id,
+        threadInfo: thread,
+      },
+    }),
   );
 
-  // 6. Construct `replace` `ClientDBThreadStoreOperation`s.
-  const replaceThreadOperations = updatedClientDBThreadInfos.map(thread => ({
-    type: 'replace',
-    payload: thread,
-  }));
-
-  // 7. Prepend `replaceThreadOperations` with `remove_all` op and return.
+  // 6. Prepend `replaceThreadOperations` with `remove_all` op and return.
   return [{ type: 'remove_all' }, ...replaceThreadOperations];
 }
 

diff --git a/lib/utils/migration-utils.js b/lib/utils/migration-utils.js
@@ -204,18 +204,23 @@ export type StorageMigrationFunction<N: BaseNavInfo, T: BaseAppState<N>> = (
   debug: boolean,
 ) => Promise<?PersistedState<N, T>>;
 
-export type MigrationManifest<N: BaseNavInfo, T: BaseAppState<N>> = {
-  +[number | string]: (PersistedState<N, T>) => Promise<{
-    +state: T,
-    +ops: StoreOperations,
-    +changesSchema?: boolean,
-  }>,
+export type MigrationFunction<
+  N: BaseNavInfo,
+  T: BaseAppState<N>,
+> = T => Promise<{
+  +state: T,
+  +ops: StoreOperations,
+  +changesSchema?: boolean,
+}>;
+
+export type MigrationsManifest<N: BaseNavInfo, T: BaseAppState<N>> = {
+  +[number | string]: MigrationFunction<N, T>,
 };
 
 function createAsyncMigrate<N: BaseNavInfo, T: BaseAppState<N>>(
   legacyMigrations: LegacyMigrationManifest<N, T>,
   config: ConfigType,
-  migrations: MigrationManifest<N, T>,
+  migrations: MigrationsManifest<N, T>,
   handleException: (error: Error, state: T) => T,
   storageMigration: ?StorageMigrationFunction<N, T>,
 ): (
@@ -268,7 +273,7 @@ function createAsyncMigrate<N: BaseNavInfo, T: BaseAppState<N>>(
 
 async function runMigrations<N: BaseNavInfo, T: BaseAppState<N>>(
   legacyMigrations: LegacyMigrationManifest<N, T>,
-  migrations: MigrationManifest<N, T>,
+  migrations: MigrationsManifest<N, T>,
   state: T,
   inboundVersion: number,
   currentVersion: number,

diff --git a/native/redux/client-db-utils.js b/native/redux/client-db-utils.js
@@ -1,6 +1,7 @@
 // @flow
 
 import type { ClientDBMessageStoreOperation } from 'lib/ops/message-store-ops.js';
+import { threadStoreOpsHandlers } from 'lib/ops/thread-store-ops.js';
 import { createUpdateDBOpsForThreadStoreThreadInfos } from 'lib/shared/redux/client-db-utils.js';
 import type {
   RawMessageInfo,
@@ -30,9 +31,11 @@ function updateClientDBThreadStoreThreadInfos(
     clientDBThreadInfos,
     migrationFunc,
   );
+  const dbOperations =
+    threadStoreOpsHandlers.convertOpsToClientDBOps(operations);
 
   try {
-    commCoreModule.processThreadStoreOperationsSync(operations);
+    commCoreModule.processThreadStoreOperationsSync(dbOperations);
   } catch (exception) {
     console.log(exception);
     if (handleMigrationFailure) {

diff --git a/native/redux/persist.js b/native/redux/persist.js
@@ -121,6 +121,10 @@ import {
   convertThreadStoreThreadInfosToNewIDSchema,
   createAsyncMigrate,
 } from 'lib/utils/migration-utils.js';
+import type {
+  MigrationFunction,
+  MigrationsManifest,
+} from 'lib/utils/migration-utils.js';
 import { entries } from 'lib/utils/objects.js';
 import {
   deprecatedConvertClientDBThreadInfoToRawThreadInfo,
@@ -149,6 +153,7 @@ import type { AppState } from './state-types.js';
 import { unshimClientDB } from './unshim-utils.js';
 import { authoritativeKeyserverID } from '../authoritative-keyserver.js';
 import { commCoreModule } from '../native-modules.js';
+import type { NavInfo } from '../navigation/default-state.js';
 import { defaultDeviceCameraInfo } from '../types/camera.js';
 import { isTaskCancelledError } from '../utils/error-handling.js';
 import { defaultURLPrefix } from '../utils/url-utils.js';
@@ -1363,14 +1368,14 @@ const entryStoreTransform: Transform = createTransform(
   { whitelist: ['entryStore'] },
 );
 
-const migrations = {
+const migrations: MigrationsManifest<NavInfo, AppState> = Object.freeze({
   // This migration doesn't change the store but sets a persisted version
   // in the DB
-  [75]: (state: AppState) => ({
+  [75]: (async (state: AppState) => ({
     state,
-    ops: [],
-  }),
-  [76]: (state: AppState) => {
+    ops: {},
+  }): MigrationFunction<NavInfo, AppState>),
+  [76]: (async (state: AppState) => {
     const localMessageInfos = state.messageStore.local;
 
     const replaceOps: $ReadOnlyArray<ReplaceMessageStoreLocalMessageInfoOperation> =
@@ -1394,35 +1399,36 @@ const migrations = {
       operations,
     );
 
-    const dbOperations: $ReadOnlyArray<ClientDBMessageStoreOperation> =
-      messageStoreOpsHandlers.convertOpsToClientDBOps(operations);
-
     return {
       state: {
         ...state,
         messageStore: newMessageStore,
       },
-      ops: dbOperations,
+      ops: {
+        messageStoreOperations: operations,
+      },
     };
-  },
-  [77]: (state: AppState) => ({
+  }: MigrationFunction<NavInfo, AppState>),
+  [77]: (async (state: AppState) => ({
     state,
-    ops: [],
-  }),
-  [78]: (state: AppState) => {
+    ops: {},
+  }): MigrationFunction<NavInfo, AppState>),
+  [78]: (async (state: AppState) => {
     const clientDBThreadInfos = commCoreModule.getAllThreadsSync();
 
-    const dbOperations = createUpdateDBOpsForThreadStoreThreadInfos(
+    const operations = createUpdateDBOpsForThreadStoreThreadInfos(
       clientDBThreadInfos,
       deprecatedUpdateRolesAndPermissions,
     );
 
     return {
       state,
-      ops: dbOperations,
+      ops: {
+        threadStoreOperations: operations,
+      },
     };
-  },
-  [79]: (state: AppState) => {
+  }: MigrationFunction<NavInfo, AppState>),
+  [79]: (async (state: AppState) => {
     return {
       state: {
         ...state,
@@ -1431,10 +1437,10 @@ const migrations = {
           tunnelbrokerToken: null,
         },
       },
-      ops: [],
+      ops: {},
     };
-  },
-  [80]: (state: AppState) => {
+  }: MigrationFunction<NavInfo, AppState>),
+  [80]: (async (state: AppState) => {
     const clientDBThreadInfos = commCoreModule.getAllThreadsSync();
 
     // This isn't actually accurate, but we force this cast here because the
@@ -1444,26 +1450,28 @@ const migrations = {
     const stripMemberPermissions: RawThreadInfos => RawThreadInfos =
       (stripMemberPermissionsFromRawThreadInfos: any);
 
-    const dbOperations = createUpdateDBOpsForThreadStoreThreadInfos(
+    const operations = createUpdateDBOpsForThreadStoreThreadInfos(
       clientDBThreadInfos,
       stripMemberPermissions,
     );
 
     return {
       state,
-      ops: dbOperations,
+      ops: {
+        threadStoreOperations: operations,
+      },
     };
-  },
-  [81]: (state: AppState) => ({
+  }: MigrationFunction<NavInfo, AppState>),
+  [81]: (async (state: any) => ({
     state: {
       ...state,
       queuedDMOperations: {
         operations: {},
       },
     },
-    ops: [],
-  }),
-  [82]: (state: any) => ({
+    ops: {},
+  }): MigrationFunction<NavInfo, AppState>),
+  [82]: (async (state: any) => ({
     state: {
       ...state,
       queuedDMOperations: {
@@ -1473,18 +1481,18 @@ const migrations = {
         membershipQueue: {},
       },
     },
-    ops: [],
-  }),
-  [83]: (state: AppState) => ({
+    ops: {},
+  }): MigrationFunction<NavInfo, AppState>),
+  [83]: (async (state: AppState) => ({
     state: {
       ...state,
       holderStore: {
         storedHolders: {},
       },
     },
-    ops: [],
-  }),
-};
+    ops: {},
+  }): MigrationFunction<NavInfo, AppState>),
+});
 
 // NOTE: renaming this object, and especially the `version` property
 // requires updating `native/native_rust_library/build.rs` to correctly