Page MenuHomePhabricator

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

Authored by atul on Apr 12 2023, 1:40 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Jun 29, 6:18 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
Unknown Object (File)
Thu, Jun 13, 3:11 AM
Unknown Object (File)
Fri, Jun 7, 6:16 AM
Unknown Object (File)
May 28 2024, 11:39 AM
Subscribers

Details

Summary

This transaction will

  1. Unset container for any upload where the viewer.userID matches the container and uploader. This is to make sure any previous image avatar uploads have their container cleared. If the previous avatar was of type emoji or ens, this query is effectively a noop. However, the query should be lightweight since we have an index on container and we only expect <=1 row in the result set.
  2. We set the container for the mediaID in the request to viewer.userID if the upload belongs to the user making the request (uploader = ${viewer.userID}) and the container is NULL. If the update request was of type image, this will ensure the container is correctly set. If the update is of any other type, this is effectively a noop. Again, the query should be lightweight since we have an index on container and we only expect <=1 row in the rsult set... and it should be cached from the previous query.
  3. We update the avatar column of the users table with the newAvatarValue from the request. If mediaID is set (AKA we're dealing with an image avatar), we make sure that an upload with the corresponding ID exists in the uploads table via a SELECT subquery AND that the container is set to the viewer.userID AND that the upload "belongs to the user" (uploader = ${viewer.userID}).

This is all wrapped in a single transaction so we don't end up in a broken state where eg the user avatar is set to an image that doesn't exist in the uploads table (which causes a crash on native as I accidentally discovered during testing).

Test Plan
  1. Set emoji avatar
  2. Make sure it's set correctly
  3. Set image avatar
  4. Make sure it's set correctly
  5. Clear user avatar
  6. Make sure it's set correctly

Basically I set and unset user avatar multiple times, tried setting the image avatar with the same uploadID multiple times to make sure the endpoint is idempotent, etc etc.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

atul published this revision for review.Apr 12 2023, 1:45 PM
atul added inline comments.
keyserver/src/updaters/account-updaters.js
130–159 ↗(On Diff #25060)

We're handling multiple cases in these queries by eg checking ${mediaID} IS NULL

Personally I find this much easier to reason about than having multiple chunks of SQL that are .appended together and having to think through all the possible queries that can be constructed. Also find the switching between SQL and JS to be confusing to parse at times (eg when a SQL statement is passed in as an argument to the function).

That being said, I can break this apart to handle image and non-image cases separately if preferred. Want to emphasize that the performance differences between the two would be microscopic (based on preliminary experimentation in MariaDB query console) and be indistinguishable from noise. It would be solely about readability/style preference.

keyserver/src/updaters/account-updaters.js
133–136 ↗(On Diff #25060)

If there are any existing uploads that have their container set to viewer.userID (a previous image avatar), we want to clear container field.

We want to do this whenever we're updating a user's avatar (whether to emoji/image/ens/etc) to make sure that these uploads are disconnected and swept up by the cron job that removes container-less uploads.

138–142 ↗(On Diff #25060)

If the user is requesting to set an image avatar, we can expect mediaID to be set.

If there is an upload with the mediaID supplied in the request AND it was uploaded by the user AND it has no container, then we want to set the container to the viewerID.

Why do we need the container IS NULL check? Isn't that guaranteed from the first query?

Not quite. The first query will nullify container only of uploads with container set to the viewer.userID. There may still be media with id = ${mediaID and uploader = ${viewer.userID} (eg if the user has ever sent a media message). This additional check ensures that a client (malicious or broken) is unable to re-seat an upload that already belongs to a media message.

150–155 ↗(On Diff #25060)

We want to make sure that the upload provided in the mediaID field of the request exists before we set the avatar field of the users table. Otherwise we can run into situation where the mediaID in an AvatarDBContent points to "nowhere." I ran into this accidentally while testing and the client crashed (as it exists today).

ashoat added inline comments.
keyserver/src/updaters/account-updaters.js
130–159 ↗(On Diff #25060)

This seems reasonable

This revision is now accepted and ready to land.Apr 12 2023, 6:00 PM