Details

Reviewers

kamil
bartek
rohan
ashoat

Commits

rCOMM5bae368c4381: [keyserver] Add function to fetch reactions/edits/sidebar_source for messages

Summary

issue: https://linear.app/comm/issue/ENG-3689/implement-fetching-edits-sidebar-source-and-reactions-for-message
Usinng the same query as we use everywhere to fetch messages - joining with uploads and memberships to be able to use parseMessageSQLResult to parse the results (as we do in other places).

For edits we need to fetch the newest edit only. This requires a subselect. I used a query from fetchLatestEditMessageContentByIDs to obtain the newest edit.

Test Plan

Artificially added edit messages to my db, checked that the function returns all edits / sidebar_sources / reactions for given messages.

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

inka created this revision.Apr 21 2023, 4:35 AM

Herald added subscribers: atul, tomek, ashoat. · View Herald TranscriptApr 21 2023, 4:35 AM

inka added a parent revision: D7517: [keyserver] Add migration to populate `target_message` for sidebar_source.Apr 21 2023, 4:35 AM

inka added inline comments.

keyserver/src/fetchers/message-fetchers.js
849 ↗	(On Diff #25518)	In the previous diff I populated the `target_message` column for `sidebar_source` messages, so this does return `sidebar_source` messages

Harbormaster completed remote builds in B18628: Diff 25518.Apr 21 2023, 4:52 AM

inka requested review of this revision.Apr 21 2023, 4:52 AM

Rename variables

Harbormaster completed remote builds in B18641: Diff 25532.Apr 21 2023, 7:14 AM

ashoat added inline comments.Apr 21 2023, 8:06 AM

keyserver/src/fetchers/message-fetchers.js
854 ↗	(On Diff #25532)	Can we explain a little bit why we need two queries? Was it difficult to do as one query? Are there tradeoffs?
860 ↗	(On Diff #25532)	What kind of `JOIN` is this? I forget what MySQL defaults to... would be good to be explicit here (is it `INNER JOIN`?)

bartek added inline comments.Apr 21 2023, 8:38 AM

keyserver/src/fetchers/message-fetchers.js
860 ↗	(On Diff #25532)	Join defaults to inner join, but +1 for being explicit

inka added a child revision: D7584: [keyserver] Add function for fetching RawMessageInfo for rows and their related messages.Apr 24 2023, 2:11 AM

use explicit INNER JOIN

keyserver/src/fetchers/message-fetchers.js
854 ↗	(On Diff #25532)	General answer: because such query is hard if not impossible to write. To do this in one query we would have to conditionally pick either elements from m or m2. m is the messages table JOINED to the m2 messages table so that entry from m is the newest edit for entry from m2. So in case of edits we need to look at columns from m. But in case of reactions / sidebar_sources we cannot look at columns from m (the newest edit), because this makes non sense. So we need to get the values from m2. Also the edit query that has a subselect is slower, so I think it's better to have it run the subselect for as few entries as possible.