tus / tus-resumable-upload-protocol

Open Protocol for Resumable File Uploads
https://tus.io
MIT License
1.48k stars 103 forks source link

Allow for the server to respond with metadata #164

Open rockwotj opened 3 years ago

rockwotj commented 3 years ago

Hello, I just stumbled across TUS today, and I think it's awesome! Thanks for working on this.

I'd like to use this for my company's uploads, but the server does some post processing after uploading to add some metadata (think things like image dimensions, exif data, video length, etc). I don't see a place in the protocol to allow for responding with extra information upon completion, and the client's don't seem to expose the final server response in order to extract information like this.

Would a maintainer accept an extension to the protocol to allow for such server-based postprocessing?

Acconut commented 3 years ago

Thanks for the kind words!

the server does some post processing after uploading to add some metadata (think things like image dimensions, exif data, video length, etc)

The server can always add additional metadata to the upload that the client can fetch using a HEAD request. Using that workaround that is already possible.

client's don't seem to expose the final server response in order to extract information like this

That depends on the clients, of course. tus-js-client, for example, allows to obtain the responses from the server to inspect them for additional data.

Would a maintainer accept an extension to the protocol to allow for such server-based postprocessing?

We're always open a discussion, yes. You might also want to look into https://github.com/tus/tus-resumable-upload-protocol/pull/159 where the topic of post-processing also appears.

rockwotj commented 3 years ago

The server can always add additional metadata to the upload that the client can fetch using a HEAD request. Using that workaround that is already possible.

This means it has to be a header yes? We have just enough metadata I'm not super comfortable sticking it in a header, because certain proxies have limits on how long headers can be (I believe NGINX is such an example), but I guess we could break it up between lots of different headers, which may work... Or I guess I could return a 200 from a PATCH request (instead of the 204 tusd responds with) and set the metadata there on the final upload.

We're always open a discussion, yes. You might also want to look into #159 where the topic of post-processing also appears.

I'll hop into that discussion and see if I can be helpful.

Acconut commented 3 years ago

This means it has to be a header yes?

Yes, metadata is transported using the Upload-Metadata header.

I'll hop into that discussion and see if I can be helpful.

:+1:

smatsson commented 3 years ago

@Acconut

The server can always add additional metadata to the upload that the client can fetch using a HEAD request. Using that workaround that is already possible.

Should we clarify this in the spec? The only mention of client/server that I can find right now is this which gives the impression that only the client can supply metadata.

The Client MAY supply the Upload-Metadata header to add additional metadata to the upload creation request. The Server MAY decide to ignore or use this information to further process the request or to reject it. If an upload contains additional metadata, responses to HEAD requests MUST include the Upload-Metadata header and its value as specified by the Client during the creation.

Acconut commented 3 years ago

Should we clarify this in the spec? The only mention of client/server that I can find right now is this which gives the impression that only the client can supply metadata.

Good point, yes we should do that!

smatsson commented 3 years ago

After reading the spec a couple of more times, I'm wondering if the server can actually change the metadata? It seems that this is explicitly forbidden in the current spec:

The Client MAY supply the Upload-Metadata header to add additional metadata to the upload creation request. The Server MAY decide to ignore or use this information to further process the request or to reject it. If an upload contains additional metadata, responses to HEAD requests MUST include the Upload-Metadata header and its value as specified by the Client during the creation.

"As specified by the Client during the creation" being the key part. If the Server changes the metadata it is no longer "as specified by the Client" unless the Server's changes are not included in the HEAD response. But then again, the Client can't access it. Any input @Acconut?

RubenGarcia commented 3 years ago

I would have expected that after a successful upload, the server would send a Location header. Then the client could use this Location header to inquire about relevent metadata, possibly at a different endpoint, as this additional metadata looks to me to be outside of the tus protocol. For example: "Location https://service/uploads/path" -> "GET https://service/metadata?location=uploads/path"

Acconut commented 3 years ago

@smatsson: After reading the spec a couple of more times, I'm wondering if the server can actually change the metadata? It seems that this is explicitly forbidden in the current spec

You are right, I forgot about that wording. IIRC, we added this sentence so that servers cannot change metadata keys that were sent by the client. For example, the client sets filename hello.txt and the server later responds with filename hello.TXT. From this perspective my last comments are wrong and the current metadata system cannot be used to send values back to the client.

@RubenGarcia I would have expected that after a successful upload, the server would send a Location header. Then the client could use this Location header to inquire about relevent metadata, possibly at a different endpoint, as this additional metadata looks to me to be outside of the tus protocol.

This would also be a possible approach we have thought about. Maybe you want to take a look at the discussion at https://github.com/tus/tus-resumable-upload-protocol/pull/159 and especially the comment at https://github.com/tus/tus-resumable-upload-protocol/pull/159#issuecomment-693586155.

smatsson commented 3 years ago

@Acconut I think it's a fair trade off so I'll just skip the PR for clarifying the spec and leave it to the upcoming post processing extension.