Page MenuHomePhabricator
Differential D7649

[native] Add function to upload media to blob service
ClosedPublic
Actions

Authored by bartek on Apr 27 2023, 4:23 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Nov 4, 4:12 PM
Unknown Object (File)
Mon, Nov 4, 4:12 PM
Unknown Object (File)
Mon, Nov 4, 4:12 PM
Unknown Object (File)
Mon, Oct 28, 1:26 AM
Unknown Object (File)
Mon, Oct 28, 1:26 AM
Unknown Object (File)
Mon, Oct 28, 1:26 AM
Unknown Object (File)
Mon, Oct 28, 1:26 AM
Unknown Object (File)
Mon, Oct 28, 1:25 AM
View All Files
Subscribers
ashoat
tomek

Details

Reviewers
atul
marcin
Commits
rCOMMdb9d19e941b8: [native] Add function to upload media to blob service
Summary

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
  3. When the above succeeds, upload blob metadata to keyserver

Depends on D7620, D7648

Test Plan

This function is called and tested in the next diff.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

bartek created this revision.Apr 27 2023, 4:23 AM
bartek held this revision as a draft.
Herald added subscribers: tomek, ashoat. · View Herald TranscriptApr 27 2023, 4:23 AM
bartek added a child revision: D7650: [native] Upload encrypted media to Blob service.Apr 27 2023, 4:29 AM
bartek edited parent revisions, added: D7620: [lib][keyserver] Expose upload_media_metadata endpoint; removed: D7619: [keyserver] Implement endpoint to upload external blob metadata.Apr 27 2023, 4:31 AM
bartek edited the summary of this revision. (Show Details)
Harbormaster completed remote builds in B18844: Diff 25813.Apr 27 2023, 4:41 AM
bartek published this revision for review.Apr 27 2023, 4:54 AM
bartek added inline comments.
native/input/input-state-container.react.js
1097–1100 ↗(On Diff #25813)

Tracked in https://linear.app/comm/issue/ENG-3810/blob-service-sanitize-inputs

bartek mentioned this in D7689: [web] Add function to upload media to Blob service.Apr 29 2023, 9:20 AM
atul requested changes to this revision.May 3 2023, 10:57 AM

Requesting changes to ensure we correctly convert base64 encoding to base64url.

Adding @ashoat as blocking to take a look at new upload boilerplate that closely matches InputStateContainer:uploadBlob(...). Between those and lib/utils/upload-blob/uploadBlob(...) we're going to have three implementations of fairly similar functionality?

native/input/input-state-container.react.js
1097–1100 ↗(On Diff #25813)

So looks like we need base64url encoding instead of base64:

f6cd23.png (876×1 px, 276 KB)

In addition to replacing / with _, we should also replace + with -. While regex approach seems fine, might make sense to use some established library or utility or something to handle the conversion in encoding?

As a start, might make sense to update this, move it to some sort of utils file, and call it base64URLFromBase64 or something?

1098 ↗(On Diff #25813)

Guessing this is a typo? shash => slash?

1122 ↗(On Diff #25813)

Same feedback as https://phab.comm.dev/D7617#inline-50340

1128–1131 ↗(On Diff #25813)

Same feedback as https://phab.comm.dev/D7617#inline-50340

This revision now requires changes to proceed.May 3 2023, 10:57 AM
atul attached a referenced file: F520998: f6cd23.png. (Show Details)May 3 2023, 10:57 AM
ashoat added inline comments.May 3 2023, 12:23 PM
native/input/input-state-container.react.js
1135–1167 ↗(On Diff #25813)

After reviewing step 2 here, it feels like a lot of it is duplicated with the second half of InputStateContainer::uploadBlob (starting on line 1236). The only differences are:

  1. url
  2. parameters
  3. Behavior on resolve (InputStateContainer::uploadBlob does JSON.parse)

The first two can be inputs into a shared async function, and the third can be handled by InputStateContainer::uploadBlob directly after awaiting the shared async function.

@bartek, what do you think of "factoring out" part 2 here?

bartek added inline comments.May 8 2023, 1:50 AM
native/input/input-state-container.react.js
1097–1100 ↗(On Diff #25813)

For blob service, only the / matters, however, base64url here is a great idea.

Found the base64url library which implements it with simple regex replaces. The library is Node-only though (uses Buffer) so I'll create a function somewhere in lib/

1135–1167 ↗(On Diff #25813)

I think it's worth doing, but this is work for a separate diff or two.
My idea is to factor out the whole blobServiceUpload() function - we can create a counterpart for lib/utils/upload-blob.js and follow similar logic as we have in lib/actions/upload-actions with the uploadBlob callback.

Anyway, I'd prefer doing this as a follow-up at the top of the stack - I can create a task for this

bartek added inline comments.May 8 2023, 2:12 AM
native/input/input-state-container.react.js
1135–1167 ↗(On Diff #25813)

Your suggestion is much simpler than my idea, I'll see how much code it saves

Btw, the naming for the existing uploadBlobintroduces some confusion as it refers to "keyserver" upload, while we also have "blob" service uploads

bartek updated this revision to Diff 26180.May 8 2023, 4:30 AM

Created a function to convert base64 to base64url format and called it on the blob hash before uploading.

The de-duplicate refactor will be done in a separate diff.

atul added inline comments.May 8 2023, 4:40 AM
native/input/input-state-container.react.js
1135–1167 ↗(On Diff #25813)

Might be overly verbose, but more clear to have uploadBlobToKeyserver and uploadBlobToBlobService?

Harbormaster completed remote builds in B19126: Diff 26180.May 8 2023, 4:47 AM
ashoat added a comment.May 8 2023, 9:25 AM

The de-duplicate refactor will be done in a separate diff.

Can you create a task to track this before landing?

atul accepted this revision.May 9 2023, 3:29 AM

Looks good, left a couple of minor notes. Would be good to get response on snake_case error messages before landing.

lib/utils/base64.js
6

Nit: I think we usually capitalize URL so this would be toBase64URL

lib/utils/base64.test.js
5–18

Thanks for including these

native/input/input-state-container.react.js
1086–1094

Could we define this type outside of function signature? Maybe just above on line ~1084?

1120

Same feedback as on another diff, but I think we usually tend to have snake_case error names so they can be handled easier by catch blocks. EG:

Screen Shot 2023-05-08 at 6.33.55 PM.png (1×1 px, 361 KB)

But you have more context here, so up to you on what you think would be best.

1183

You could destructure the fields from input above this return, but totally up to you

This revision is now accepted and ready to land.May 9 2023, 3:29 AM
atul attached a referenced file: F527706: Screen Shot 2023-05-08 at 6.33.55 PM.png. (Show Details)May 9 2023, 3:29 AM
bartek added a comment.May 9 2023, 5:00 AM
In D7649#229795, @ashoat wrote:

The de-duplicate refactor will be done 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

lib/utils/base64.js
6

Ah yes, missed that

native/input/input-state-container.react.js
1120

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

bartek added inline comments.May 9 2023, 5:14 AM
native/input/input-state-container.react.js
1086–1094

Type definitions inside class don't work. Will leave it as is, this will be refactored during de-duplication anyway

bartek updated this revision to Diff 26292.May 9 2023, 6:02 AM

Apply review feedback

Harbormaster completed remote builds in B19201: Diff 26292.May 9 2023, 6:23 AM
Closed by commit rCOMMdb9d19e941b8: [native] Add function to upload media to blob service (authored by bartek). · Explain WhyMay 9 2023, 7:03 AM
This revision was automatically updated to reflect the committed changes.
bartek added a commit: rCOMMdb9d19e941b8: [native] Add function to upload media to blob service.
bartek mentioned this in rCOMMc9de9966f13f: [web] Add function to upload media to Blob service.

Revision Contents
Changeset List

PathSize
lib/
utils/
18 lines
18 lines
native/
input/
116 lines

Diff 26180

View Options

lib/utils/base64.js

Loading...
View Options

lib/utils/base64.test.js

Loading...
View Options

native/input/input-state-container.react.js

Loading...