Page MenuHomePhabricator

[keyserver/lib] Add a fetchPinnedMessages method to retrieve all pinned messages for a thread
ClosedPublic

Authored by rohan on Mar 20 2023, 3:42 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Nov 25, 1:47 PM
Unknown Object (File)
Sun, Nov 24, 3:05 AM
Unknown Object (File)
Sun, Nov 24, 3:05 AM
Unknown Object (File)
Sun, Nov 24, 3:03 AM
Unknown Object (File)
Sun, Nov 24, 2:59 AM
Unknown Object (File)
Sun, Nov 24, 2:45 AM
Unknown Object (File)
Sun, Nov 24, 2:31 AM
Unknown Object (File)
Sun, Nov 24, 12:53 AM
Subscribers

Details

Summary

For any given thread, we need to allow participants to view the pinned messages. The query checks for messages where m.pinned = 1 and translates the rows into messageInfos using an existing function. I use shimUnsupportedRawMessageInfos to prevent problems if there is a pinned message of a newer message type that an older client does not yet support.

Depends on D6930

Linear: https://linear.app/comm/issue/ENG-3186/add-a-fetchpinnedmessages-method-to-retrieve-all-pinned-messages-for-a

Test Plan

Set three messages in the messages table to be 'pinned' and call this query and log the results. Confirm the results are as expected.

Screenshot 2023-03-20 at 6.11.30 PM.png (740×2 px, 596 KB)

Screenshot 2023-03-20 at 6.11.20 PM.png (738×1 px, 313 KB)

Diff Detail

Repository
rCOMM Comm
Branch
pinned_messages
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

keyserver/src/fetchers/message-fetchers.js
671 ↗(On Diff #23902)

rawMessageInfoFromRows seems to require subthread_permissions. The other queries that call this method use stm.permissions with LEFT JOIN memberships stm ON m.type = ${messageTypes.CREATE_SUB_THREAD} AND stm.thread = m.content AND stm.user = n.user.

I'm not particularly sure why, in this case, I'd need to left join on memberships when m.type = CREATE_SUB_THREAD, so I've just selected subtread_permissions as NULL.

I tried looking at the git history for any queries that do call this method, but they're mostly from several years ago, so if I'm missing something and I do need it, feel free to let me know

rohan requested review of this revision.Mar 20 2023, 3:54 PM

Rename types

keyserver/src/fetchers/message-fetchers.js
671

My comment from a previous revision that explains this: https://phab.comm.dev/D7110?id=23902#inline-46565

ashoat requested changes to this revision.Mar 21 2023, 6:53 AM
ashoat added inline comments.
keyserver/src/fetchers/message-fetchers.js
689

Did you notice that we generally pass an array of messageRows here rather than one? How do we decide what this array contains? Why is it necessary elsewhere in the codebase?

It's critical that you build up your skills at reading/analyzing code. You should fully understand 100% of every single line you submit for code review.

This revision now requires changes to proceed.Mar 21 2023, 6:53 AM
keyserver/src/fetchers/message-fetchers.js
689

I noticed that, taking another look now, I seem to have missed parseMessageSQLResult that seems to be helpful. It looks like in functions that will be fetching multiple messages (fetchCollapsableNotifs, fetchMessageInfos, fetchMessageInfosSince, and fetchDerivedMessages) they each call parseMessageSQLResult and the logic is already there to handle multiple rows. Meanwhile, I'm seeing a warning now when passing the entire messageRows array directly to rawMessageInfoFromRows by assertSingleRow.

That's probably my first sign, now that I'm noticing that, that I'm doing it the wrong way. My new understanding is that in these cases, where we expect to have multiple rows, parseMessageSQLResult creates a map of {id: row}, and iterates over that and calls rawMessageInfoFromRows. I think MULTIMEDIA is a scenario where the key may have multiple rows, but besides that, each key should associate to one row.

My thinking is to update this diff and just call parseMessageSQLResult directly, and return the array of rawMessageInfos from the return type MessageSQLResult.

keyserver/src/fetchers/message-fetchers.js
686–696

Something like this

keyserver/src/fetchers/message-fetchers.js
689

This sounds right!! Yeah, multimedia uploads are the only case where you'll see multiple rows for the same ID. That's because you are LEFT JOINing the uploads table, which can match multiple rows for a single message

Call parseMessageSQLResult instead of rawMessageInfoFromRows

ashoat requested changes to this revision.Mar 22 2023, 8:36 AM
ashoat added inline comments.
keyserver/src/fetchers/message-fetchers.js
669–678 ↗(On Diff #23939)

You're skipping some JOINs (such as the stm join) that are present in other queries. What was your thinking when you decided to omit them?

This revision now requires changes to proceed.Mar 22 2023, 8:36 AM
keyserver/src/fetchers/message-fetchers.js
669–678 ↗(On Diff #23939)

Oh my comment explaining my uncertainty disappeared between revisions: https://phab.comm.dev/D7110?id=23902#inline-46565

keyserver/src/fetchers/message-fetchers.js
669–678 ↗(On Diff #23939)

You definitely missed something. Can you try to do 30-60 minutes of more digging to try to figure out why the stm join exists, and whether we should be using it here?

keyserver/src/fetchers/message-fetchers.js
669–678 ↗(On Diff #23939)

Ok I've got some more understanding now. So when I create a subchannel, a message gets inserted into messages where the content is the thread ID of the new channel was created from. So in my original thread, 84449, when a new subchannel is created (id = 84910), a message is inserted into 84449 with content 84910.

Screenshot 2023-03-23 at 9.42.33 AM.png (182×2 px, 180 KB)

Now when I retrieve all messages and left join for subthread permissions, that message will be the only message with non-null values

Screenshot 2023-03-23 at 9.51.40 AM.png (418×2 px, 144 KB)

The only thing is, I don't think it's relevant here since in this query we query on pinned = 1, and the only messages that can be pinned are 1, 14 15 (text, image, multimedia). So the message type 3 will never be retrieved and the subthread_permissions will remain NULL for all of the rows, with or without the LEFT JOIN. That's why I thought we don't need the additional LEFT JOIN and we can just select NULL as subthread_permissions

keyserver/src/fetchers/message-fetchers.js
669–678 ↗(On Diff #23939)

Oh sorry a typo, the only messages that can be pinned are 0, 14, 15 (text, image, multimedia).

Return an emptyArray of pinnedMessages instead of null if there are none (feedback from D7112)

Planning changes to avoid putting it on reviewer's queue until the discussion in the comments is resolved

ashoat added inline comments.
keyserver/src/fetchers/message-fetchers.js
668 ↗(On Diff #23939)

Do we care to add an ORDER BY to this query? Noticed that fetchMessageInfosSince has ORDER BY m.thread, m.time DESC, m.id DESC

669–678 ↗(On Diff #23939)

Ah, I didn't realize that we were restricting the types of messages that could be pinned that way. In retrospect that makes sense, and it avoids the complex privacy considerations of what "what happens if an admin pins a message about the creation of a secret subchannel". (Privacy checks for messages about secret subchannels are why the stm check exists here.)

Can you add a comment here explaining that we're restricting the message types that can be pinned, so we don't need the privacy checks?

keyserver/src/fetchers/message-fetchers.js
668 ↗(On Diff #23939)

Good point, here we'd probably want to ORDER BY m.pin_time DESC to have the pinned messages from newest to oldest.

669–678 ↗(On Diff #23939)

Sure I'll add a comment

Add a comment explaining why we don't check subthread_permissions here and also ORDER BY m.pin_time DESC to fetch pinned messages from newest to oldest

This revision is now accepted and ready to land.Mar 24 2023, 8:15 AM