Page MenuHomePhabricator

[keyserver] Update `fetchUserInfos` to include `avatar`s in `GlobalUserInfo`s
ClosedPublic

Authored by atul on Mar 23 2023, 5:19 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Nov 8, 7:39 AM
Unknown Object (File)
Tue, Nov 5, 4:35 AM
Unknown Object (File)
Tue, Nov 5, 4:34 AM
Unknown Object (File)
Tue, Nov 5, 4:34 AM
Unknown Object (File)
Tue, Nov 5, 4:34 AM
Unknown Object (File)
Tue, Nov 5, 4:34 AM
Unknown Object (File)
Tue, Nov 5, 4:33 AM
Unknown Object (File)
Tue, Nov 5, 2:03 AM
Subscribers

Details

Summary

We're going to include avatars from the users table in the GlobalUserInfos returned by fetchUserInfos.

NOTE: I know it was previously discussed that we want to avoid sending avatars in *UserInfos to older clients. However, the *UserInfo types are used in so many places and included in so many of the "messages" sent between the client and the server that handling every single case with a codeVersion check would be very involved.

If it's possible for older clients to recieve avatars in *UserInfos without issue, then I think we should proceed by including avatars everywhere. My hunch (after reading through the codebase) is that it's completely fine to have superfulous fields in the Redux CurrentUserInfo and in the UserInfo entries. I only took a cursory glance, I will read through thoroughly to confirm this.

If we conclude that it would be catastrophic for old clients to recieve avatar field in *UserInfo, then the best way to handle it would be:

  1. Start from the DB queries and include avatar in all *UserInfos "downstream."
  2. Look at every payload in redux-types to see where *UserInfos from the keyserver are included.
  3. Look at the return types of every responder to see where *UserInfo might be included.
  4. Look at every serverRequest (maybe not necessary?) to see if we ever send *UserInfos as part of a request.
  5. Create some sort of filterOutAvatarsFromUserInfo[s] utility function that would be used RIGHT BEFORE we send data to native clients based on codeVersion

This approach--filtering the avatar field out right before we communicate with client--would be a lot cleaner than drilling Viewer/version check to every single query and having to handle *UserInfos potentially with/without avatar field all throughout keyserver.

This aproach would also avoid complexity with *UserInfo queries that are made by the keyserver rather than any particular client. An example of this is how fetchUserInfos is called by send.js:fetchNotifUserInfos which is called by send.js:fetchInfos which is called by send.js:sendPushNotifs which is called by message-creator:postMessageSend which is called by message-creator.js:createMessages which is called a dozen different places.

TLDR, I think if we can get away with including avatar in *UserInfos for older client, we should seriously consider it. If not, I strongly think that the filtering out of the avatar field should happen where the client and server "interface," NOT at every query that returns *UserInfos OR throughout keyserver where those objects are passed all over the place. I will, however, still consider EG state check mechanism since this approach would necessitate that the *UserInfos would be out of sync.

Test Plan

Set breakpoint and observed that avatar was included in *UserInfos returned by fetchUserInfos:

[screenshot coming shortly]


This won't be landed until the rest of the refactoring is complete AND there's been a LOT more testing.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

atul published this revision for review.Mar 23 2023, 5:29 PM
atul edited the summary of this revision. (Show Details)
ashoat requested changes to this revision.Mar 23 2023, 8:06 PM

That makes sense RE older clients. Thinking about it more, I'm not actually sure why we ever thought it made sense to "hide" the avatar field from older clients... they're going to eventually need the field, and if we hide it from them initially then they will need to rely on the state check mechanism to fix things after they upgrade codeVersions.

Speaking of the state check mechanism, I think a good way to think about this problem space is "how can I prevent the state check mechanism from detecting inconsistency". My main concern here is that you're setting avatar: null, which will immediately render all clients inconsistent with the keyserver for all users. This will have a huge cost in terms of reports created etc.

Let's avoid setting avatar unless it exists, and only set it for a given UserInfo when that user calls the upstream methods to actually set their avatar. (At which point UPDATE_USER updates should help us avoid state inconsistencies.)

This revision now requires changes to proceed.Mar 23 2023, 8:06 PM

Let's avoid setting avatar unless it exists, and only set it for a given UserInfo when that user calls the upstream methods to actually set their avatar. (At which point UPDATE_USER updates should help us avoid state inconsistencies.)

Thanks for that clarification. Will update this diff.

don't include avatar if null (this should also now be landable as is)

keyserver/src/fetchers/user-fetchers.js
52–62 ↗(On Diff #24089)

Can alternatively set userInfos[id] with just id and username and then userInfos[id] = {...userInfos[id], avatar} to include avatar if it's not null if that's preferred over this ternary which is formatted with a lot of indentation

keyserver/src/fetchers/user-fetchers.js
52–62 ↗(On Diff #24089)

Yeah I think that's a better idea, but I don't feel too strongly

53 ↗(On Diff #24089)

I would probably just check avatar... I can see that you never set it to undefined, but I think it's more readable and basically translates as "if avatar is set"

77 ↗(On Diff #24089)

I assume this will get updated in later diffs

208 ↗(On Diff #24089)

As well as this

This revision is now accepted and ready to land.Mar 24 2023, 1:55 PM

rebase before addressing all feedback

change avatar !== null check to avatar truthiness check

keyserver/src/fetchers/user-fetchers.js
77 ↗(On Diff #24089)

yeah

208 ↗(On Diff #24089)

yeah