transloadit / uppy-server

[DEPRECATED] 'Uppy Server' was renamed to 'Companion' and lives inside the Uppy repo no
https://github.com/transloadit/uppy/tree/master/packages/%40uppy/companion
MIT License
114 stars 27 forks source link

LocalDisk Provider and Tus Upload handling #62

Closed kingschnulli closed 6 years ago

kingschnulli commented 6 years ago

The docs are listing Local disk as a supported provider - but there is no documentation on this topic anywhre. Also - does uppy-server support direct uploading via Tus or is tus-server needed for this seperatly?

ifedapoolarewaju commented 6 years ago

Also - does uppy-server support direct uploading via Tus or is tus-server needed for this seperatly?

You'd need a tus-server for this one. Uppy-server would simply serve as the tus-client and it would upoad to the tus-server.

The docs are listing Local disk as a supported provider

though the infrastructure for this is mostly in place on the server-end, there's no entirely usable (front to back) public API for this yet. We'll look into this one.

Thank you for reporting 👍

kvz commented 6 years ago

Doesn't local disk just mean the local disk over at the uppy client? Are there plans to source from the uppy server disk? Or am i misunderstanding something here?

ifedapoolarewaju commented 6 years ago

@kvz if it were by your interpretation, then it shouldn't be listed on uppy-server at all. Since files from localdisk on uppy-client would not go through uppy-server.

I interpret this more as uploading to uppy-server's local disk, as opposed to uploading to tus-server, or a multipart server.

kvz commented 6 years ago

I interpret this more as uploading to uppy-server's local disk, as opposed to uploading to tus-server, or a multipart server.

I don't think this is something we wanted to support since that's a) just paving the way to a scaling issue down the road b) we wanted uppy server to remain an oauth/sidecar, and not replicate behavior that exists in apache/nginx/tus already, which add the benefit of maturity and/or resumability

Not sure which docs are saying we're gonna support that, but that does not align with what we had in mind afaik

ifedapoolarewaju commented 6 years ago

just paving the way to a scaling issue down the road

but how though? It would just simply be the same download that uppy-server already does, but with no corresponding upload, and it wouldn't delete the file locally as we already do. However, we can make this optional on the server-side. So you decide whether or not your server should keep files locally.

we wanted uppy server to remain an oauth/sidecar

it still would be, but instead of uploading to another server, you can as well keep it on uppy-server.

kvz commented 6 years ago

but how though? It would just simply be the same download that uppy-server already does, but with no corresponding upload, and it wouldn't delete the file locally as we already do. However, we can make this optional on the server-side. So you decide whether or not your server should keep files locally.

Saving to local disk is just one disk. So if you have some more users and you want to scale an extra uppy server, where will the files then be? 2 disks. Should they sync somehow? Should it be on NFS? These things all have severe limits and each of the next steps has their own set of drawbacks. I feel it's better to encourage people from the get go to save to s3/gcloud/riak cs/something that works beyond a handful users.

And it wouldn't be resumable. If we treat uppy server as a destionation rather than a handler.. I would expect an uppy server to have state of the art upload handling. So we'd have to ship a tus server along with it. And it wouldn't be secure either, not unless folks ad an ~Nginx HTTPS terminator. And there you have it: now you already added a tus/nginx server. Better to use those components directly. Makes it easier to monitor and scale than to duplicate and bundle all the things inside of uppy server imho.

Like, when the next Poodle, Shellshock or Heartbleed happens it's better that just memory of Nginx leaks than all of uppy server's memory cause we handle it straight from Node. Due to the ongoing vulnerabilities found with ssl it also forces you to upgrade your Node.js version every week/month if you terminate it inside the process. If Nginx handles it, you can have ~Ubuntu auto upgrade it without as much BC breaking risk involved.

it still would be, but instead of uploading to another server, you can as well keep it on uppy-server.

So assuming we are not going to bundle a tus server, external storage provider adapters, https, etc. We'd just be offering a mediocre and unsafe (no encryption, no resume) multipart upload receiver, correct?

If we offer it, people will use it, and have issues with it. And then we'd either be slowly forced to duplicate and bundle all this tech into uppy server, or make the recommendation to after all use tusd, an https terminator, etc instead.

And so even when scaling isn't an issue cause people either run an intranet thing or are just confident that they'll never interest the internet enough for their product to the degree that they'll run out of a single server's capacity, I'd estimate that the Venn diagram of:

leaves a very tiny shared surface. And this is the group of people we should be recommending to get a webserver to add and terminate SSL/TLS for them, vs accommodating to remain unsafe.

kvz commented 6 years ago

Sorry for the rant! 🙃 👯

kingschnulli commented 6 years ago

Wow, I didn't mean to trigger such a discussion - wouldn't the easiest thing be to just remove the "Local Disk" bullet point from the docs? It just confused me in the beginning, after getting a better insight on this topic what @kvz writes is totally right.

ifedapoolarewaju commented 6 years ago

yeah, I have removed the doc reference now, thanks!