[keyserver] Introduce `assignMessageContainerToMedia(...)` in `upload-updaters`
ClosedPublic
Actions

Authored by atul on Sep 9 2022, 2:25 AM.

Details

Reviewers

• abosh
tomek
marcin

Commits

rCOMMbba6ce98b3e4: [keyserver] Introduce `assignMessageContainerToMedia(...)` in `upload-updaters`

Summary

Pretty much assignMedia(...) but accepts mediaMessageContents: $ReadOnlyArray<MediaMessageServerDBContent> instead of mediaIDs: $ReadOnlyArray<string>.

The intention is to use assignMessageContainerToMedia(...) in the "new"/non-legacy multimediaMessageCreationResponder(...)

Depends on D5086

Test Plan

Later in the stack this function is "consumed" from multimediaMessageCreationResponder(...). I set some breakpoints and confirmed that values were as expected. I also checked the uploads table in my local MySQL DB and found that the container column was assigned as expected:

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

atul created this revision.Sep 9 2022, 2:25 AM

Herald added a subscriber: ashoat. · View Herald TranscriptSep 9 2022, 2:25 AM

atul edited the summary of this revision. (Show Details)Sep 9 2022, 2:34 AM

atul added a parent revision: D5086: [keyserver] Pull `fetchUploadsForMessage(...)` out of `fetchMediaFromMediaMessageContent(...)`.

Harbormaster completed remote builds in B12036: Diff 16533.Sep 9 2022, 2:35 AM

atul requested review of this revision.Sep 9 2022, 2:35 AM

atul added inline comments.Sep 9 2022, 2:37 AM

keyserver/src/updaters/upload-updaters.js
27–32 ↗	(On Diff #16533)	We're using nearly identical logic in `fetchUploadsForMessage(...)`. Don't think it's quite worth extracting out into it's own utility function yet... but it's on my radar as the stack progresses

atul edited the test plan for this revision. (Show Details)Sep 9 2022, 2:44 AM

atul added 1 blocking reviewer(s): tomek.Sep 9 2022, 10:34 AM

atul added 1 blocking reviewer(s): • abosh.Sep 12 2022, 7:46 AM

The code looks ok, but wanted to clarify some things before I can accept.

keyserver/src/updaters/upload-updaters.js
36 ↗	(On Diff #16533)	This function is slightly confusing, because it tells that it's going to assign to a container, but only does that if the container isn't already assigned. Should we remove `IS NULL` condition? Also, what is the purpose of `uploader` check?

This revision now requires changes to proceed.Sep 13 2022, 10:30 AM

atul added inline comments.Sep 13 2022, 3:47 PM

keyserver/src/updaters/upload-updaters.js
36 ↗	(On Diff #16533)	This function is slightly confusing, because it tells that it's going to assign to a container, but only does that if the container isn't already assigned. Should we remove `IS NULL` condition? We don't want to return any uploads that have already been assigned a container. This is an assumption made in `assignMedia(...)` which this function is heavily inspired by. Changing this assumption may (?) break the idempotency of `multimediaMessageCreationResponder(...)`. See the following for context: https://phab.comm.dev/D2476 Also, what is the purpose of `uploader` check? Matches the behavior of the existing `assignMedia(...)`