Page MenuHomePhabricator

[keyserver] Introduce `legacyMultimediaMessageCreationResponder`
ClosedPublic

Authored by atul on Aug 29 2022, 6:14 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Nov 11, 2:51 AM
Unknown Object (File)
Sat, Nov 9, 9:08 AM
Unknown Object (File)
Sat, Nov 9, 6:55 AM
Unknown Object (File)
Sat, Nov 9, 6:54 AM
Unknown Object (File)
Sat, Nov 9, 6:54 AM
Unknown Object (File)
Sat, Nov 9, 6:53 AM
Unknown Object (File)
Sun, Nov 3, 5:32 AM
Unknown Object (File)
Sat, Nov 2, 1:14 PM
Subscribers
None

Details

Summary

The existing multimediaMessageCreationResponder accepts:

  • threadID: string
  • localID: string
  • mediaIDs: string[]

This information is sufficient to create IMAGES messages (type 14), but not MULTIMEDIA messages (type 15). In order to properly represent the relationship between primary media and thumbnail media we need to replace the list of strings with a list of something like the following:

export type MediaMessageContent =
  | {
      +type: 'video',
      +mediaID: string,
      +thumbnailID: string,
    }
  | {
      +type: 'photo',
      +mediaID: string,
    };

We could change the create_multimedia_message endpoint to handle a $ReadOnlyArray<MediaMessageContent>... but that would break existing clients.

We could also create a seperate create_multimedia_message_v2 endpoint, but that would lead to a lot of branching (and changes that need to be made elsewhere eg endpoints.js).

The simplest/cleanest approach in my mind is:

  1. Extract the existing functionality out of multimediaMessageCreationResponder into legacyMultimediaMessageCreationResponder (to support old clients).
  2. Modify sendMultimediaMessageRequestInputValidator to optionally accept a list of MediaMessageContent
  3. If a request includes mediaIDs, send it over to legacyMultimediaMessageCreationResponder
  4. If a request includes mediaMessageContent, it'll be handled within multimediaMessageCreationResponder.
  5. Implement multimediaMessageCreationResponder to handle MULTIMEDIA messages (will likely require some new queries, etc)
  6. Modify native (message-actions: sendMultimediaMessage(...)) to hit the updated multimediaMessageCreationResponder endpoint with mediaMessageContent instead of mediaIDs.

9232dc.png (1×1 px, 704 KB)

  1. Make sure that old clients continue to work with these changes.
  2. etc...

In the long run (outside of the scope of our current video work), I think it makes sense for both photos and videos to use the more flexible MULTIMEDIA: 15 message type.


Depends on D4980

Test Plan
  1. Send image message on native.
  2. Send video message on native.
  3. Set breakpoints in legacyMultimediaMessageCreationResponder and log key values to ensure that things continue to work as expected.

Diff Detail

Repository
rCOMM Comm
Branch
master
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

atul edited the summary of this revision. (Show Details)
atul edited the summary of this revision. (Show Details)
atul requested review of this revision.Aug 29 2022, 6:27 PM

So I assume steps 2-8 will be separate diffs in this stack.

In the long run (outside of the scope of our current video work), I think it makes sense for both photos and videos to use the more flexible MULTIMEDIA: 15 message type.

I also fully agree with this. It would make a lot of the code easier to work with with less branching.

I guess technically it's called "legacy" since it's old but wouldn't just calling it imageMessageCreationResponder make more sense? Since right now it can only handle the format for images, whereas multimedia (according to its current definition of media with a thumbnail, so basically video) can't be handled? Then we can leave a comment or something explaining why we have two separate functions (for step 3/4, where the difference between images and multimedia become more apparent, as opposed to legacy vs. multimedia).

This revision is now accepted and ready to land.Aug 29 2022, 11:03 PM

Personally prefer the legacyMultimediaMessageCreationResponder naming. I think it makes it clear that it's around to support old clients and it kind of "deprecated" and that multimediaMessageCreationResponder(...) is what will handle things going forward.

Even image messages (IMAGES: 14) will go through the new responder once implemented.

This revision now requires review to proceed.Aug 30 2022, 10:14 AM
In D4982#144870, @atul wrote:

Personally prefer the legacyMultimediaMessageCreationResponder naming. I think it makes it clear that it's around to support old clients and it kind of "deprecated" and that multimediaMessageCreationResponder(...) is what will handle things going forward.

Even image messages (IMAGES: 14) will go through the new responder once implemented.

Ok, makes sense

It might make sense to have a hasMinCodeVersion check somewhere so that we expect newer clients to always provide the new payload. Doing so will allow dropping the support for older schema at some point.

This revision is now accepted and ready to land.Aug 31 2022, 4:24 AM
In D4982#145108, @tomek wrote:

It might make sense to have a hasMinCodeVersion check somewhere so that we expect newer clients to always provide the new payload. Doing so will allow dropping the support for older schema at some point.

Good point, will make sure to add that check in a subsequent diff once the functionality of the updated multimediaMessageCreationResponder is fleshed out.

Will also need to think about how we sequence keyserver deployment with native releases so we don't have clients hitting an endpoint that the keyserver doesn't yet support.

rebase around other diffs before landing