Page MenuHomePhabricator

[web] Add function to upload media to Blob service
ClosedPublic

Authored by bartek on Apr 29 2023, 9:20 AM.
Tags
None
Referenced Files
F3377301: D7689.diff
Wed, Nov 27, 5:02 AM
Unknown Object (File)
Mon, Nov 25, 8:57 PM
Unknown Object (File)
Mon, Nov 25, 8:57 PM
Unknown Object (File)
Sat, Nov 23, 6:46 PM
Unknown Object (File)
Sun, Nov 10, 8:02 PM
Unknown Object (File)
Sun, Nov 10, 8:01 PM
Unknown Object (File)
Sun, Nov 10, 8:01 PM
Unknown Object (File)
Sun, Nov 10, 1:37 PM
Subscribers

Details

Summary

Web counterpart of D7649. This diff adds a function that calls all necessary endpoints to upload a media file to Blob service. It has 3 stages:

  1. Create a holder and assign it to a blob (identified by blob hash)
  2. If the blob content doesn't exist yet, upload it. Used the XMLHttpRequest call very similiar to the one in lib/utils/upload-blob.js
  3. When the above succeeds, upload blob metadata to keyserver

Depends on D7620, D7687

Test Plan

This function is called and tested in the next diff.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

bartek held this revision as a draft.
bartek published this revision for review.Apr 29 2023, 9:27 AM
atul requested changes to this revision.May 3 2023, 11:25 AM

Requesting changes for base64/base64url encoding feedback.

CC @ashoat on another "implementation" of uploadBlob

web/input/input-state-container.react.js
1017–1020 ↗(On Diff #25938)

Same feedback as https://phab.comm.dev/D7649#inline-50356

TLDR: I think we need to think about this as replacing base64 encoding with base64url encoding. Regex approach may be fine, but we should also handle replacing + with -. While the + might not be causing any issues at the moment, I think it would be a better approach than having a custom base64/base64url encoding that happens to work.

1032 ↗(On Diff #25938)

We don't usually use snake case on the JS side. I'm guessing this corresponds to something on the Rust side where snake case might be the usual convention?

Probably fine, but I think we can "automate" this via some sort of Rust Serde macro(?). Doesn't look like we use this in our codebase, but saw it used in another one I was looking at recently:

#[serde(rename_all = "snake_case")]

But maybe this is too "magical," defer to {O4}

This revision now requires changes to proceed.May 3 2023, 11:25 AM
web/input/input-state-container.react.js
1061 ↗(On Diff #25938)

Similar feedback to what I left in D7649: it looks like substantial portions of step 2 here are copy-pasted from lib/utils/upload-blob.js. Can we try to factor out the shared logic instead?

web/input/input-state-container.react.js
1032 ↗(On Diff #25938)

I don't think we should allow using both camelCase and snake_case in HTTP payloads.
We should either use camelCase and do the serde directive you mentioned or explicitly use snake_case in JS. I followed the latter for consistency with feature-flag service.

1061 ↗(On Diff #25938)

Replied in D7649 - I'd prefer to share code for the whole blobServiceUpload()

Used the toBase64Url for blob hash sanitization the same way as in D7649

The de-duplication refactor will be performed in a separate diff.

The de-duplication refactor will be performed in a separate diff.

Can you create a task to track this before landing?

Looks good, would be good to get response on snake_case error messages before landing.

web/input/input-state-container.react.js
1032 ↗(On Diff #25938)

That makes sense and seems like the best approach for now.

This revision is now accepted and ready to land.May 9 2023, 3:33 AM
In D7689#230216, @atul wrote:

Looks good, would be good to get response on snake_case error messages before landing.

I responded in https://phab.comm.dev/D7617?id=25933#inline-50542 and created a ENG-3841 as this is a more complex task

The de-duplication refactor will be performed in a separate diff.

Can you create a task to track this before landing?

https://linear.app/comm/issue/ENG-3863/de-duplicate-blobserviceupload-code-on-web-and-native