Page MenuHomePhabricator

[keyserver/lib] update deleteFarcasterChannelTag to use communites table
ClosedPublic

Authored by ginsu on May 9 2024, 11:00 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Nov 10, 9:14 PM
Unknown Object (File)
Sun, Nov 10, 7:44 PM
Unknown Object (File)
Sun, Nov 10, 7:43 PM
Unknown Object (File)
Sun, Nov 10, 7:21 PM
Unknown Object (File)
Sun, Nov 10, 1:08 PM
Unknown Object (File)
Sun, Nov 10, 11:42 AM
Unknown Object (File)
Sat, Oct 19, 11:56 AM
Unknown Object (File)
Oct 14 2024, 5:36 PM

Details

Summary

Now that we have a communities table that was introduced in D11903, we can use it to better improve our deleteFarcasterChannelTag method

Depends on D11975

Test Plan

Removed the farcaster channel tag and confirmed that the farcaster channel id and the blob holder were removed in the correct row of the communites table + also confirmed that the blob holding the farcaster channel <> comm community association was deleted

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

keyserver/src/deleters/farcaster-channel-tag-deleters.js
29–34 ↗(On Diff #40030)

Initially tried to use RETURNING blob_holder AS blobHolder as part of the query (just like in deleteInviteLink); however, was getting a You have an error in your SQL syntax; error so created a separate query to get the blob holder and now everything works as expected

lib/types/community-types.js
31 ↗(On Diff #40030)

Initially we were passing the blobHolder as part of the request since the blob holder was temporarily stored in redux; however, now that we can store the blobHolder in the communities table on the keyserver, the blob holder should never be in the client so we should remove it as part of the request

ginsu requested review of this revision.May 9 2024, 11:16 AM
tomek added inline comments.
keyserver/src/deleters/farcaster-channel-tag-deleters.js
29–34 ↗(On Diff #40030)

It seems like MariaDB doesn't support RETURNING with UPDATE statements https://mariadb.com/kb/en/update/.

I can see some problems with the current implementation:

  1. Is it guaranteed that the select will happen before the update?
  2. Are we sure that the rows can't be modified before select and update?

There are at least 3 possible solutions:

  1. We can consider using https://mariadb.com/kb/en/replace/
  2. Maybe transactions can help with this
  3. Maybe it is possible to delete and insert instead of update

The chance for this edge case is really low, so we should decide if we really care about it.

This revision is now accepted and ready to land.May 10 2024, 4:15 AM
keyserver/src/deleters/farcaster-channel-tag-deleters.js
29–34 ↗(On Diff #40030)

Updated the query to use the transaction approach

ginsu requested review of this revision.May 22 2024, 7:23 AM
ginsu added 1 blocking reviewer(s): tomek.

Tested everything end-2-end and everything still works as previously. However, would appreciate a second pair of eyes on the query as a sanity check + since I made some significant changes to query logic.

tomek added inline comments.
keyserver/src/deleters/farcaster-channel-tag-deleters.js
50–56 ↗(On Diff #40546)

We have another problem here: what happens if deleting the blob fails? It might be possible that the keyserver DB will no longer have the holder, but the blob will still exist in the blob service. Should we clear it later, or something?

This revision is now accepted and ready to land.May 27 2024, 4:59 AM
ginsu added inline comments.
keyserver/src/deleters/farcaster-channel-tag-deleters.js
50–56 ↗(On Diff #40546)

Going to land as is, but will create a task to track this concern. At least for now if we run into this edge case we should be able to manually edit/delete database rows in AWS console

cc @bartek

https://linear.app/comm/issue/ENG-8379/improve-handling-failed-delete-farcaster-channel-tag-blob

update + rebase before landing