Page MenuHomePhabricator

[native] migrate integrityStore to SQLite
ClosedPublic

Authored by will on Mar 18 2024, 4:38 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Nov 6, 7:54 AM
Unknown Object (File)
Fri, Nov 1, 9:02 PM
Unknown Object (File)
Fri, Nov 1, 8:35 PM
Unknown Object (File)
Thu, Oct 17, 1:54 PM
Unknown Object (File)
Thu, Oct 17, 1:54 PM
Unknown Object (File)
Thu, Oct 17, 1:54 PM
Unknown Object (File)
Thu, Oct 17, 1:54 PM
Unknown Object (File)
Thu, Oct 17, 1:54 PM
Subscribers

Details

Summary

This implements the web/native migration for the integrityStore table to sqlite

Test Plan

integrity table on dev environment didn't have prior integrity store thread hashes, so I tested by migrating raw thread hash data.

  console.log('running migration 64');

  const threadHashes1: ThreadHashes = {
    thread1: 123,
    thread2: 456,
    thread3: 789,
  };
  const threadHashes2: ThreadHashes = {
    thread1: 893,
    thread2: 443,
    thread3: 913,
  };
  const threadHashes3: ThreadHashes = {
    thread4: 593,
  };
  const replaceOp1: ReplaceIntegrityThreadHashesOperation = {
    type: 'replace_integrity_thread_hashes',
    payload: {
      threadHashes: threadHashes1,
    },
  };
  const clearOp: RemoveAllIntegrityThreadHashesOperation = {
    type: 'remove_all_integrity_thread_hashes',
  };
  const replaceOp2: ReplaceIntegrityThreadHashesOperation = {
    type: 'replace_integrity_thread_hashes',
    payload: {
      threadHashes: threadHashes2,
    },
  };
  const replaceOp3: ReplaceIntegrityThreadHashesOperation = {
    type: 'replace_integrity_thread_hashes',
    payload: {
      threadHashes: threadHashes3,
    },
  };

  const dbOperations: $ReadOnlyArray<ClientDBIntegrityStoreOperation> =
    integrityStoreOpsHandlers.convertOpsToClientDBOps([
      clearOp,
      replaceOp1,
      clearOp,
      replaceOp2,
      replaceOp3,
    ]);

  try {
    await commCoreModule.processIntegrityStoreOperations(dbOperations);
    console.log('migration 64 succeeded');
  } catch (exception) {
    console.log('migration 64 failed');
    if (isTaskCancelledError(exception)) {
      return state;
    }
    return handleReduxMigrationFailure(state);
  }
  return state;
},

Successfully wrote test thread hashes to the sqlite database:

Screenshot 2024-03-18 at 7.37.48 PM.png (288×332 px, 49 KB)

Tested web with these steps:

  1. Log out, clear the app data, log in
  2. Run the app from before the store is introduced to the DB. Close the app, serve a new version, and open the app.
  3. Close the app while hashes are being computed (in the old version). Close the app, serve a new version, and open the app.

Used Kamil's diff for downloading database on web to check database contents are correct and reflect latest thread changes

Depends on D11309

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Mar 18 2024, 4:52 PM
Harbormaster failed remote builds in B27568: Diff 38148!

rebase with latest shared worker changes

will requested review of this revision.Mar 18 2024, 9:53 PM
tomek requested changes to this revision.Mar 20 2024, 5:02 AM
tomek added inline comments.
web/redux/persist.js
280 ↗(On Diff #38157)

Have you checked if this store isn't empty? It seems like persistWhitelist doesn't contain integrityStore which means that state.integrityStore.threadHashes should be always empty.

This revision now requires changes to proceed.Mar 20 2024, 5:02 AM
web/redux/persist.js
280 ↗(On Diff #38157)

Realizing now that the user store sqlite migration only migrated native (https://phab.comm.dev/D9912). I mistakenly went off the keyserver migration diffs for this diff. I think we can remove the web migration in this case then.

Or should I be adding integrityStore to the persistWhiteList?

web/redux/persist.js
280 ↗(On Diff #38157)

Or should I be adding integrityStore to the persistWhiteList?

We shouldn't do that because we don't want these to be persisted using redux-persist.

In this case, I think that we can remove this migration, but that needs to be tested thoroughly.

web/redux/persist.js
280 ↗(On Diff #38157)

Could I get some clarification on what you mean by testing thoroughly?

web/redux/persist.js
280 ↗(On Diff #38157)

I think we should test a couple of scenarios:

  1. Log out, clear the app data, log in
  2. Run the app from before the store is introduced to the DB. Close the app, serve a new version, and open the app.
  3. Close the app while hashes are being computed (in the old version). Close the app, serve a new version, and open the app.

In all of these, we should check if the content of the DB is correct.

At some point we should also blacklist integrityStore on native so that it isn't persisted using redux-persist

This revision is now accepted and ready to land.Mar 22 2024, 8:40 AM
will retitled this revision from [web][native] migrate integrityStore to SQLite to [native] migrate integrityStore to SQLite.
This revision was automatically updated to reflect the committed changes.

At some point we should also blacklist integrityStore on native so that it isn't persisted using redux-persist

Was this ever done? It looks like this stack was landed and integrityStore is not in persistBlacklist on native...

At some point we should also blacklist integrityStore on native so that it isn't persisted using redux-persist

Was this ever done? It looks like this stack was landed and integrityStore is not in persistBlacklist on native...

I understood this to be a later follow-up for after we verify sqlite is working for a bit after landing. I should have made a linear issue for this and will do that right now.

Currently, the blacklist includes

'loadingStatuses',
'lifecycleState',
'dimensions',
'draftStore',
'connectivity',
'deviceOrientation',
'frozen',
'threadstore',
'storeloaded',

It seems like we haven't blacklisted other migrations, but I can't find a linear issue for them. Will create a follow up parent task for now

Spoke to @will about this in person and he clarified that we're purposing double-storing this data right for validation. @will will post a Linear issue in a sec that will track:

  1. Removing the data from redux-persist
  2. Adding the field to the blacklist for redux-persist on native (and making sure it's not on the whitelist on web)
  3. Updating the reducer on setClientDBStoreActionType so it actually puts that data into the store
  4. Any other follow-ups necessary

We'll deploy the current state to production for a bit, and let devs monitor to see if the Alert hits. If it doesn't, we'll move forward with the forthcoming task.

Here's the linear issue tracking putting integrityStore on redux persist black list and switching to using sqlite integrityStore:

https://linear.app/comm/issue/ENG-7512/after-verifying-integritystore-sqlite-storing-properly-remove