Page MenuHomePhabricator

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

Authored by bartek on Apr 27 2023, 4:23 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Mar 4, 2:33 AM
Unknown Object (File)
Mon, Mar 4, 2:32 AM
Unknown Object (File)
Sun, Mar 3, 4:19 AM
Unknown Object (File)
Feb 26 2024, 4:39 AM
Unknown Object (File)
Feb 23 2024, 8:44 PM
Unknown Object (File)
Feb 23 2024, 1:08 AM
Unknown Object (File)
Feb 22 2024, 10:01 PM
Unknown Object (File)
Feb 18 2024, 5:09 AM
Subscribers

Details

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
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

bartek held this revision as a draft.
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)
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)
1128–1131 ↗(On Diff #25813)
This revision now requires changes to proceed.May 3 2023, 10:57 AM
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?

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

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

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.

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

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

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

Can you create a task to track this before landing?

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 ↗(On Diff #26180)

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

lib/utils/base64.test.js
5–18 ↗(On Diff #26180)

Thanks for including these

native/input/input-state-container.react.js
1086–1094 ↗(On Diff #26180)

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

1120 ↗(On Diff #26180)

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 ↗(On Diff #26180)

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

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 ↗(On Diff #26180)

Ah yes, missed that

native/input/input-state-container.react.js
1120 ↗(On Diff #26180)

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

native/input/input-state-container.react.js
1086–1094 ↗(On Diff #26180)

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