Paths

Table of Contentst

[web] Migrate existing `ClientDBThreadInfo`s on `web` to be minimally encoded
AbandonedPublic
Actions

Authored by atul on Jan 25 2024, 2:27 PM.

Details

Reviewers

ginsu
tomek
kamil
ashoat

Summary

NOTE: Marking as DRAFT to get feedback from reviewers in case they notice any obvious issues while I continue testing.

This migration basically converts ClientDBThreadInfos in the SQLite threads store to RawThreadInfos and then back to ClientDBThreadInfos.

The function that takes us from ClientDBThreadInfo -> RawThreadInfo handles converting both legacy and minimallyEncoded ClientDBThreadInfo to minimally encoded RawThreadInfos (we decided against splitting ClientDBThreadInfo into separate types since the relevant fields were all string encoded JSON blobs.

convertClientDBThreadInfoToRawThreadInfo was tested thoroughly as part of the original native migration and has "excessive" validation to ensure that we're dealing with correctly formed ClientDBThreadInfos:

Depends on D10818

Test Plan

Did some testing of a previous version of this migration and things worked as expected. That said, made some tweaks (eg using .map instead of for loop, etc) so will go through and test things again.

Will be tested with:

Safari
Chrome
Firefox

Scenario 1: Migration against thread store with all ClientDBThreadInfos currently minimally encoded should be noop

Log into current web client (on master).
Log the value of stores.store.threads at start of migration 11.
Create a temporary migration 12 and log the value of stores.store.threads.
Make sure that the values of stores.store.threads are identical before and after.

Scenario 2: Migration against thread store with all MinimallyEncoded`ClientDBThreadInfos`s currently minimally encoded should be noop

Log into current web client (on master).
Log the value of stores.store.threads at start of migration 11.
Create a temporary migration 12 and log the value of stores.store.threads.
Make sure that the values of stores.store.threads are "legacy" before and "minimally encoded" after.

Diff Detail

Repository

rCOMM Comm

Branch

master

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

atul created this revision.Jan 25 2024, 2:27 PM

atul edited the summary of this revision. (Show Details)Jan 25 2024, 2:32 PM

atul edited the test plan for this revision. (Show Details)Jan 25 2024, 2:38 PM

Harbormaster completed remote builds in B26169: Diff 36146.Jan 25 2024, 2:46 PM

atul requested review of this revision.Jan 25 2024, 2:46 PM

Seems reasonable to me, but would want somebody with more web SQLite experience to take a look

Overall it looks okay.

Unfortunately, the web is not exactly like native, where we had threads in redux-persist, and the process of migrating them to storage is still in progress.

We migrate threads from InitialReduxStateResponder (see here) each time there aren't any thread data in DB. Are threads returned by the keyserver in the correct format or require migration as well?
Right now threads in DB are only for staff. So this migration will operate on empty array for non-staff users.
- We can migrate now, and next, when enabling having threads in DB for all users webapp will operate on the new format of minimally encoded threads without another migration - but I do not have enough context to confirm if this could work, so wondering what's your point of view.
- we can enable threads for all users (I think the last issue was some time ago), it seems to work correctly. (cc. @ashoat)

Right now threads in DB are only for staff. So this migration will operate on empty array for non-staff users.

Does this mean we should also update state.threadStore in this migration? Or does it either come from DB or from keyserver? (keyserver should serve minimally-encoded RawThreadInfos)

We can migrate now, and next, when enabling having threads in DB for all users webapp will operate on the new format of minimally encoded threads without another migration - but I do not have enough context to confirm if this could work, so wondering what's your point of view.

You mean to have the same migration introduced again later for a different version? I don't think it will be necessary, unless we persist web ThreadStore via redux-persist somewhere. If we do, it seems this could be avoided by just updating the redux-persist data in this diff.

we can enable threads for all users (I think the last issue was some time ago), it seems to work correctly. (cc. @ashoat)

Hmm... while it's not directly related to threads, I worry about ENG-6413 – does this become more likely to occur if we launch web-threads-in-SQLite?

Unmarking as DRAFT, think this is good for review.

atul edited the test plan for this revision. (Show Details)Jan 29 2024, 7:37 AM

I guess my initial understanding when I first landed "flip the switch" diff for minimal permissions was correct and we weren't yet persisting threadStore in Redux OR SQLite for non-staff users.

My solution then was going to be to ask staff to log out and log back in on web to get encoding of threadStore consistent in SQLite.

Despite the effort spent on this diff (mostly on testing), it probably makes sense to just send a message in Comm Dev Team asking people to log out and log back in? Even that is probably unnecessary considering the message to log out/log in when @ yiannis was added to the Comm channel.

Just not sure there's much point to this diff?

If you’re sure that it won’t affect production users, then go ahead and abandon it. Let’s talk more in our 1:1… don’t think this process was handled very well

In D10826#312451, @ashoat wrote:

Right now threads in DB are only for staff. So this migration will operate on empty array for non-staff users.

Does this mean we should also update state.threadStore in this migration? Or does it either come from DB or from keyserver? (keyserver should serve minimally-encoded RawThreadInfos)

There is no state.threadStore - we don't persist threads in redux-persist. For staff users threads are from DB, and for other users threads are from the keyserver (getInitialReduxStateResponder).

We can migrate now, and next, when enabling having threads in DB for all users webapp will operate on the new format of minimally encoded threads without another migration - but I do not have enough context to confirm if this could work, so wondering what's your point of view.

You mean to have the same migration introduced again later for a different version? I don't think it will be necessary, unless we persist web ThreadStore via redux-persist somewhere. If we do, it seems this could be avoided by just updating the redux-persist data in this diff.

I made some bad assumptions here - you can ignore this comment. We don't have threads in redux-persist, and since keyserver is serving minimally-encoded RawThreadInfos there is no need for migration in the future. If we enable threads in DB for all users, we will add the correct version or threads from a keyserver like here.

we can enable threads for all users (I think the last issue was some time ago), it seems to work correctly. (cc. @ashoat)

Hmm... while it's not directly related to threads, I worry about ENG-6413 – does this become more likely to occur if we launch web-threads-in-SQLite?

It could be if the issue is some problem with too much memory allocated or used - threadStore is huge.

Despite the effort spent on this diff (mostly on testing), it probably makes sense to just send a message to Comm Dev Team asking people to log out and log back in. Even that is probably unnecessary considering the message to log out/log in when @ yiannis was added to the Comm channel.

Just not sure there's much point to this diff?

I think you can just land this but two things:

make sure the actual migration is only executed when needed - see my inline comment.
make sure this migration will not fail if the user already has minimally-encoded RawThreadInfos (if someone logged out after the keyserver started to serve minimally-encoded it has minimally-encoded RawThreadInfos in DB).

In that case, we can avoid asking to login/logout.

And sorry for the late response here.

web/redux/persist.js
222	`threads` in DB right now are only for staff - you can just early return if `stores.store.threads` is empty - this will skip the migration for production users. (unfortunately, you can't use `canUseDatabaseOnWeb` because at this point you don't have access to `userID`.

Thanks @kamil for clarifying those details. I think it's reasonable to abandon this diff and mark migration task as canceled.

Can send a message in Comm Dev team asking devs to log out/log back in, but not sure that's necessary since they had to do that when @ yiannis was added.

atul mentioned this in D11099: [native] Redux migration to patch in `specialRole` field.Feb 15 2024, 1:54 PM

kamil mentioned this in D11191: DRAFT [lib/native/web] introduce migration to populate community store.Feb 28 2024, 1:13 AM