Page MenuHomePhabricator

[keyserver] Update `updateThread` to handle image avatars
ClosedPublic

Authored by atul on Apr 13 2023, 10:41 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Jun 29, 8:32 PM
Unknown Object (File)
Sat, Jun 29, 6:59 PM
Unknown Object (File)
Sat, Jun 29, 1:06 PM
Unknown Object (File)
Fri, Jun 28, 2:04 PM
Unknown Object (File)
Fri, Jun 28, 2:04 PM
Unknown Object (File)
Fri, Jun 28, 2:04 PM
Unknown Object (File)
Fri, Jun 28, 2:04 PM
Unknown Object (File)
Fri, Jun 28, 2:02 PM
Subscribers

Details

Summary

We follow a similar approach to D7404. We break apart sqlUpdates into avatarUpdate and nonAvatarUpdates which we handle with separate queries.

We handle nonAvatarUpdates with nonAvatarUpdateQuery, which is largely unchanged from the existing query. I just added some newlines so the formatting was more symmetrical with the newly introduced avatarUpdateQuery.

We introduce avatarUpdateQuery in order to handle updating of avatars. Just as in D7404, we wrap all of the operations in a transaction to ensure that all of these changes occur atomically and we aren't left with a broken state (eg avatar that points to nonexistent upload).

High level overview of avatarUpdateQuery:

  1. We begin transaction.
  2. We set container to NULL for any uploads which previously had their container set to request.threadID. container-less uploads will be swept up by a cron job at a later time.
  3. We set container to request.threadID for the target avatarUploadID specified in the UpdateUserAvatarRequest.
  4. We set avatar to whatever is in avatarUpdate. If avatarUploadID is NOT NULL, we additionally check that the upload specified in the UpdateUserAvatarRequest exists AND was uploaded by the viewer AND has container set to request.threadID AND that thread field is NULL.
  5. We commit the transaction.
Test Plan
  1. Set thread emoji avatar
  2. Make sure it's set correctly
  3. Set image avatar
  4. Make sure it's set correctly
  5. Send a bunch of identical "set image avatar" requests
  6. Make sure updateThread endpoint is idempotent
  7. Clear image avatar
  8. Make sure thread avatar is cleared
  9. etc etc

Basically set/unset thread avatar multiple times to make sure DB/client were as expected. Also tested various non-avatar thread changes to make sure that there are no regressions.

Here's a demo of setting/unsetting image thread avatar:

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

atul requested review of this revision.Apr 13 2023, 10:59 AM
ashoat added inline comments.
keyserver/src/updaters/thread-updaters.js
621–625 ↗(On Diff #25134)

Can we move this definition into the conditional below? It doesn't appear to be used elsewhere

634–676 ↗(On Diff #25134)

Same thing – can you move this (and the avatarUploadID definition) into the conditional?

This revision is now accepted and ready to land.Apr 13 2023, 12:00 PM
This revision was landed with ongoing or failed builds.Apr 13 2023, 1:07 PM
This revision was automatically updated to reflect the committed changes.