Page MenuHomePhabricator

[keyserver] Introduce `fetchMediaFromMediaMessageContent(...)`
ClosedPublic

Authored by atul on Sep 8 2022, 11:37 PM.
Tags
None
Referenced Files
F3345422: D5085.diff
Fri, Nov 22, 6:00 AM
Unknown Object (File)
Mon, Nov 11, 2:51 AM
Unknown Object (File)
Fri, Nov 8, 1:37 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:14 PM
Unknown Object (File)
Sat, Nov 2, 1:11 PM
Subscribers

Details

Summary

Given a viewer: Viewer and mediaMessageContents: $ReadOnlyArray<MediaMessageServerDBContent, fetchMediaFromMediaMessageContent(...) will query the serverDB uploads table and return array of "reconstructed" Media.

At a high level...

  1. Go through each MediaMessageServerDBContent and aggregate uploadIDs
  2. Query the uploads table for the id, secret, type, and extra of each upload
  3. Go through each mediaMessageContent and construct corresponding Media by (A) pulling out information from primaryUpload and (B) pulling out information from the thumbnailUpload in the case of videos

The functionality of this function was inspired by the existing fetchMedia(...) and mediaFromRow(...) functionality in upload-fetchers.js... and will replace their usage in the non-legacy multimediaMessageCreationResponder(...)


Depends on D5063

Test Plan

Added the following:

if (request.mediaMessageContents) {
  const retval = await fetchMediaFromMediaMessageContent(
    viewer,
    request.mediaMessageContents,
  );
}

to message-responders: multimediaMessageCreationResponder(...) so fetchMediaFromMediaMessageContent(...) would be hit every time I sent a media message from the native app.

I set breakpoints throughout fetchMediaFromMediaMessageContent(...) and sent media messages with:

  • 1 photo
  • >1 photos
  • 1 video
  • >1 videos
  • 1 photo + 1 video
  • 2 photos + 2 videos

and observed that values at each step were as expected (taking special care to ensure that IDs were strings and numbers where they were supposed to be).

Screen Shot 2022-09-09 at 2.19.36 AM.png (978×2 px, 827 KB)

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

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

We have a convention that we prefer fp versions of lodash functions. I think we can discuss it, as I think that non-fp functions are more readable, but for now it's better to be consistent.

134 ↗(On Diff #16531)

Why do we fetch only the ones with null CONTAINER?

138 ↗(On Diff #16531)

At some point we can consider replacing lodash with vanilla js and use Object.fromEntries instead.

178–179 ↗(On Diff #16531)

Is there a reason behind adding loop prop conditionally instead of adding it always?

This revision is now accepted and ready to land.Sep 13 2022, 10:21 AM
keyserver/src/fetchers/upload-fetchers.js
3 ↗(On Diff #16531)

Makes sense, I'll switch to the fp version.

134 ↗(On Diff #16531)

(Asked the same thing here: https://phab.comm.dev/D2476#58058)

But basically, we don't want to fetch uploads which may have already been assigned to another message.

Additional context: https://linear.app/comm/issue/ENG-236/failure-when-sending-image

138 ↗(On Diff #16531)

That's a good idea... I'd personally much prefer that over lodash/fp

178–179 ↗(On Diff #16531)

I followed the example set by mediaFromRow(row: Object) in the same file to keep things consistent. Not sure if it makes a real difference

address feedback from @tomek: replace lodash/keyby with lodash/fp/keyby

This revision was landed with ongoing or failed builds.Sep 13 2022, 2:59 PM
This revision was automatically updated to reflect the committed changes.
keyserver/src/fetchers/upload-fetchers.js
134 ↗(On Diff #16531)

So maybe it is a good idea to add a code comment?