companion: set upload filename from metadata during uploads

[ifedapoolarewaju]

fixes #1409

[goto-bus-stop]

hah ok, so we did already do it here!

[arturi]

What about adding this filename and filetype conversion to uploadRemote on @uppy/tus client instead? Then it’s consistent — tusd expects metadata this way, it doesn’t convert name to filename for you, and so will Companion. We have that conversion for direct uploads, and we’ll do the same in the same place for remote uploads.

https://github.com/transloadit/uppy/blob/master/packages/%40uppy/tus/src/index.js#L180-L184

[goto-bus-stop]

I guess we have to keep it in companion until 2.0 :(

[arturi]

That’s right, forgot about that, good catch! But what about adding it on the client anyway, then we can add a 2.0 todo to remove on Companion and forget about it.

[ifedapoolarewaju]

What about adding this filename and filetype conversion to uploadRemote on @uppy/tus client instead? Then it’s consistent

@arturi but then we'd have to change it for XHR-Uploads too. And as you can see from the code, the way companion sets filetype for multipart is quite different and prone to change depending on the request library used. So I don't think it matters if we change it from the client. So long as companion knows what to expect, it can change it accordingly

[arturi]

The filepath property overrides filename and may contain a relative path. This is typically used when uploading multiple files from a directory.
— https://github.com/form-data/form-data#alternative-submission-methods

Do we want this too? We get the path when a folder with files is dropped in uppy.

#1509 the property is relativePath and it will be null if there was no folder.

[arturi]

We discussed in Slack that meta like filetype and filename should be set in the actual upload handler, specific to each protocol — tus/multipart/s3. This is because tusd uses filetype, but request uses contentType, for example.

[ifedapoolarewaju]

Do we want this too? We get the path when a folder with files is dropped in uppy.

@arturi I'll rather try to avoid this now, as I am not sure of the security implications

[arturi]

Do we want this too? We get the path when a folder with files is dropped in uppy.
I'll rather try to avoid this now, as I am not sure of the security implications

Discussed with Ife. Because the filepath overrides the filename, we’ve decided against using it for now, since we are sending relativePath anyway, that can already be used on the server side to mimic the user’s file structure.

[arturi]

Maybe a comment/link here too, as to why filename and contentType? Stating that it’s conventional and linking to https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Disposition#Directives and https://github.com/request/request?

[ifedapoolarewaju]

I think it might be redundant because Content-Disposition is pretty common and that's what the browser also uses, and the library used is request so linking to request is unnecessary because this exact example is in the readme.

[arturi]

Ok, I’ll leave a link here then: https://github.com/request/request#multipartform-data-multipart-form-uploads

[arturi]

Nitpicking, but is it defined by the protocol, or just the tusd implementation? I didn’t find it in the protocol, so maybe linking here: https://github.com/tus/tusd/blob/5b376141903c1fd64480c06dde3dfe61d191e53d/unrouted_handler.go#L614-L646?

[ifedapoolarewaju]

filename is mentioned a few times in the protocol https://github.com/tus/tus-resumable-upload-protocol/blob/master/protocol.md#protocol-extensions

[ifedapoolarewaju]

since filetype is not documented, I have added a comment to the URL you shared

[arturi]

Tested again locally with latest changes, works 👌

[arturi]

I think it’s good to go! Nice work!

[goto-bus-stop]

Changes look solid!

In xhr-upload client uploads, we use the original file name and type instead of the metadata file name and type. So there will be a difference between client and remote uploads still with this PR. I think using metadata is probably better though.

Maybe we can still do this and then change the name/type for xhr-upload client uploads in a minor release? It would be a breaking change but on an extreme technicality so it may be fine. The only problem is that changing the type may be harder for client uploads. You can create a new Blob but I don't know if that will affect memory usage/performance.

[goto-bus-stop]

To recap from the call: We'll merge this first, then change the xhr-upload client uploads to send metadata name and type instead, by putting file.meta.name in formData.append and by slicing the file data Blob + using the contentType option.