Page MenuHomePhabricator

[keyserver] Migration to reset all PRIVATE thread names
ClosedPublic

Authored by ashoat on Apr 15 2023, 11:42 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Dec 9, 10:35 PM
Unknown Object (File)
Sun, Dec 8, 6:36 PM
Unknown Object (File)
Thu, Dec 5, 8:24 PM
Unknown Object (File)
Thu, Dec 5, 8:24 PM
Unknown Object (File)
Thu, Dec 5, 8:24 PM
Unknown Object (File)
Wed, Dec 4, 8:36 PM
Unknown Object (File)
Wed, Dec 4, 8:27 PM
Unknown Object (File)
Wed, Dec 4, 8:23 PM
Subscribers
None

Details

Summary

For all PRIVATE threads where the thread name is the member's username, this will clear out the thread name.

Once D7453 is widely deployed to clients, this will have the effect of allowing ENS resolution for PRIVATE thread names. For older clients without D7453, this would result in the threads being titled "just you".

To reduce the likelihood that a user encounters the "just you" case, I'll wait until we have a build with D7453 live in prod for a bit before landing this diff.

Depends on D7454

Test Plan

Tested the migration in my local environment. Confirmed it unsets the thread names correctly

Diff Detail

Repository
rCOMM Comm
Branch
ashoat/privatename
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

Make sure the role is real so we don't pick up parent admins

keyserver/src/database/migration-config.js
327–333 ↗(On Diff #25191)

I learned how craft an update with a join on MySQL via this article: https://www.mysqltutorial.org/mysql-update-join/

If there are PRIVATE thread with multiple memberships, this will clear the the username if the thread name matches any of the members' usernames. This should never happen since PRIVATE threads should have only one member

ashoat published this revision for review.Apr 15 2023, 12:02 PM

I should either:

  1. Figure out a way to gate this on codeVersion instead, and add a corresponding redux-persist migration to prevent native clients from having to get updated via the state check mechanism
  2. Or generate a bunch of updateTypes.UPDATE_THREAD updates to update the thread names for all clients

Option 2 would probably be easier, and the cost should be low since most users will see at most one PRIVATE thread.

  1. Update to use updateThread utility so that updates are correctly created for all clients
  2. Only reset thread names to the empty string if they are for SIWE users. For other users it won't affect behavior on new clients, but will break old clients

(I am still planning to defer landing this until the client code has been in place for some time.)

tomek added inline comments.
keyserver/src/database/migration-config.js
340–341

Are you sure we need left joins?

This revision is now accepted and ready to land.Apr 17 2023, 11:39 PM
keyserver/src/database/migration-config.js
340–341

As opposed to other kinds of joins, or as opposed to selecting from multiple tables?

As opposed to inner joins, it should make no difference because if either memberships or users tables don't match, then the t.name = u.ethereum_address condition will not be fulfilled.

As opposed to multiple tables, my understanding is that the query engine should resolve this to the same behavior.

keyserver/src/database/migration-config.js
340–341

As opposed to inner joins. Not sure if it affects the performance, but regardless, if we don't want the rows that are matched by left and not by inner, we can be more explicit about it by using inner join - I think it is less confusing. Personally, I think that inner joins should be used by default and left joins only when there's a reason to do so.

keyserver/src/database/migration-config.js
340–341

Okay, that's fair – I'll make the change

Use INNER JOIN instead of LEFT JOIN. I tested the query again to make sure it still works