Page MenuHomePhabricator

[keyserver] Add function for fetching messages matching search query
ClosedPublic

Authored by inka on Apr 24 2023, 3:24 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Dec 16, 7:16 AM
Unknown Object (File)
Sun, Dec 15, 7:55 PM
Unknown Object (File)
Sun, Dec 15, 7:55 PM
Unknown Object (File)
Sun, Dec 15, 7:55 PM
Unknown Object (File)
Sun, Dec 15, 7:55 PM
Unknown Object (File)
Sun, Dec 15, 7:55 PM
Unknown Object (File)
Sun, Dec 15, 7:55 PM
Unknown Object (File)
Sun, Dec 15, 7:55 PM
Subscribers

Details

Summary

issue: https://linear.app/comm/issue/ENG-3317/implement-fetching-messages-based-on-the-query
I'm using the same cursor pattern as we use in fetchMessageInfos(fetchMessagesBeforeCursor), fetchErrorReportInfos
Joining with messages m2 is used to fetch the message a sidebar_source is pointing to.

Test Plan

Called this function from createMessages and checked that it returns messages matching the provided pattern, from the given thread. Checked that it returns correct related messages, especially for the
message that is the first message in a sidebar.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Apr 24 2023, 3:38 AM
Harbormaster failed remote builds in B18689: Diff 25595!
inka requested review of this revision.Apr 25 2023, 4:17 AM
keyserver/src/database/search-utils.js
137 ↗(On Diff #25652)

Can you clarify if it's really helpful to split based on this versus just splitting on a single space character? My impression was that segmentAndStem converts extraneous whitespace to a single space, but I may be wrong

144 ↗(On Diff #25652)

I feel like this would be more readable

keyserver/src/fetchers/message-fetchers.js
923 ↗(On Diff #25652)

Can you clarify in what scenarios viewer won't be set?

925 ↗(On Diff #25652)

It doesn't make sense to have a required parameter after optional ones.

We can make the optional parameters required (but with an optional type), eg. cursor: ?string instead of cursor?: string. But perhaps we should just reorder so that the required parameter comes first?

Address review

keyserver/src/database/search-utils.js
137 ↗(On Diff #25652)

segmentAndStem does remove additional whitespaces. This is not incorrect, but unnecessarily broad.

keyserver/src/fetchers/message-fetchers.js
923 ↗(On Diff #25652)

My thinking was that since the function doesn't need the viewer to be defined, I shouldn't artificially create this constraint in case someone needs to use it without a user for some reason.
This for example makes testing easier

ashoat added inline comments.
keyserver/src/database/search-utils.js
145 ↗(On Diff #25676)

Isn't this the same as using a string? Curious why you're using a RegExp here

keyserver/src/database/search-utils.test.js
1 ↗(On Diff #25676)

Nit: can you add a newline after this declaration?

This revision is now accepted and ready to land.Apr 25 2023, 10:19 AM

Looks good, please apply @ashoat's nits ;)

Address review, remove + < > ~ from query to avoid sql errors

inka requested review of this revision.Apr 26 2023, 8:48 AM

Requesting review because after a discussion in ENG-3744 we decided that for now we only want to exclude + < > ~ signs from the query. Those are signs that are both not removed by segmentAndStem, and at the same time are operands of the MATCH AGAINST query, so keeping them in the query as words results in errors.

This revision is now accepted and ready to land.Apr 26 2023, 9:02 AM

Add searchMessagesPageSize

keyserver/src/fetchers/message-fetchers.js
911 ↗(On Diff #26152)

The limit has been increased by 1 in the most recent diff update. It was my impression that we weren't planning on doing the "fetch 1 extra" strategy anymore, though I don't remember why that decision was made.

@inka, can you clarify if we're using the "fetch 1 extra" strategy still, and why the change was made?

keyserver/src/fetchers/message-fetchers.js
911 ↗(On Diff #26152)

Looks like we're using the "fetch 1 extra" strategy in D7651 now. So that answers the first question... just curious about the second question. Basically, I recall that we were initially doing this, and then stopped doing it, and now looks like we're doing it again... curious for context on that

keyserver/src/fetchers/message-fetchers.js
911 ↗(On Diff #26152)

Initially I used it somewhere else - in D7117, when I was fetching messages to be processed entirely on the server side. It didn't make sense there because the server had to do the same amount of work either way (run the query the same number of times).
Here I am calling this endpoint from the client, who benefits from knowing if the results have ended - they don't have to call the server again. The server runs the same number of queries as if it didn't fetch and ignore the additional entry, but the client doesn't have to call the server and get an empty result.
So this is a different situation than before. It makes the code on the client side more readable as well.

Here is what you wrote on that diff, that explains it:

We do something similar in message-fetchers.js (assume that if the result count is the same as the LIMIT, then there is more to fetch), but the difference there is that we have another variable we pass to the client that indicates if we think the result count is "exhaustive" or not. If we weren't passing that variable to the client, then there would be no point

keyserver/src/fetchers/message-fetchers.js
911 ↗(On Diff #26152)

Thanks for explaining!