Page MenuHomePhabricator

[keyserver] Pull `fetchUploadsForMessage(...)` out of `fetchMediaFromMediaMessageContent(...)`
ClosedPublic

Authored by atul on Sep 8 2022, 11:42 PM.
Tags
None
Referenced Files
F3365114: D5086.diff
Mon, Nov 25, 6:37 AM
Unknown Object (File)
Mon, Nov 11, 2:51 AM
Unknown Object (File)
Sat, Nov 2, 1:14 PM
Unknown Object (File)
Sat, Nov 2, 1:14 PM
Unknown Object (File)
Sat, Nov 2, 1:14 PM
Unknown Object (File)
Sat, Nov 2, 1:14 PM
Unknown Object (File)
Sat, Nov 2, 1:11 PM
Unknown Object (File)
Oct 26 2024, 5:12 AM
Subscribers

Details

Summary

Pull the serverDB uploads table query out of fetchMediaFromMediaMessageContent(...) into a separate function to make things a bit shorter and more readable.

This is effectively a noop, just repositioning things.


Depends on D5085

Test Plan

NA, effectively a noop...but sent a few messages and checked values at various breakpoints as a sanity check to ensure things worked as expected.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

atul requested review of this revision.Sep 8 2022, 11:52 PM
tomek added inline comments.
keyserver/src/fetchers/upload-fetchers.js
121 ↗(On Diff #16532)

Is there a way to have more concrete type here?

This revision is now accepted and ready to land.Sep 13 2022, 10:54 AM

rebase after resolving merge conflicts

keyserver/src/fetchers/upload-fetchers.js
121 ↗(On Diff #16532)

The existing fetchMedia(...) and mediaFromRow(...) type rows returned by the DB query as being Object. Noticed that this was a pattern elsewhere in the codebase as well.

Not sure if introducing a new type that contains every column of the returned row makes sense? What do you think the best approach would be here?


Going to land as-is since this is consistent with existing codebase, but happy to address any ideas in a subsequent diff.

keyserver/src/fetchers/upload-fetchers.js
121 ↗(On Diff #16532)

The problem with current approach is that this function can't be used without checking its internals. Also, modifying the returned value would not cause Flow to raise any error. So adding types for a row will make a lot of sense, but we can do this improvement in other places as well.