search

tus / tus-node-server

Node.js tus server, standalone or integrable in any framework, with disk, S3, and GGC stores.
https://tus.io/
MIT License
795 stars 197 forks source link

onUploadFinish should be able to alter response body #606

Closed netdown closed 3 months ago

netdown commented 4 months ago

Hi, If I'm not mistaken, currently there is no way to set a response body when the upload succeeds (if I throw with 204 status code, some headers wont be sent), the most I can do is set some custom headers. I think its common that you need to send a detailed json response after processing the file.

In tusd, the pre-finish hook is much more flexible as it expects an object from the hook, i.e.: { "HTTPResponse": { "Body": "{\"message\":\"done\"}", } },

I'm willing to make a PR, but first I'd like to discuss the ideal solution.

I think the onUploadFinish return type could be changed to ServerResponse|{ res: ServerResponse, body?: string|ArrayBuffer, headers?: Record<string, string|number> }. Currently, body is not passed to res.write at all, and header arrays could be easily merged. This would introduce a minimal modification in packages/server/src/handlers/PatchHandler.ts, around line 112.

Murderlon commented 4 months ago

Hi, I'm surprised tusd allows it because a 204 PATCH is not allowed to have a body (cc @Acconut). And you are not allowed to change the status code:

The Server MUST acknowledge successful PATCH requests with the 204 No Content — https://tus.io/protocols/resumable-upload#patch

Regarding headers, have you tried res.setHeader() in your hook?

If we want to change the return type it would have to be backwards compatible as done here: #599. But let's first discuss whether it makes sense at all.

Acconut commented 4 months ago

Hi, I'm surprised tusd allows it because a 204 PATCH is not allowed to have a body (cc @Acconut). And you are not allowed to change the status code:

The Server MUST acknowledge successful PATCH requests with the 204 No Content — https://tus.io/protocols/resumable-upload#patch

In hindsight, enforcing a 204 is a bit too restrictive and we should also allow 200 as most clients will accept both, in my experience. If people want to have custom response bodies for PATCH requests and their client implementations support that, I don't see a reason why tusd should interfere here.

Murderlon commented 4 months ago

Makes sense! But since we're following the current version of the spec now, what are you thoughts on sending a body with 204, which generally isn't allowed? Or do you think it's better to deviate from the spec in this case and allow 200 with a body?

Acconut commented 4 months ago

I think our servers should only generate 204 responses on their own assuming no manual intervention via hooks or events was performed (as tusd and tus-node-server currently do). If users decide to deviate from the spec and change the response code or body they can do so, but at their own risk. But I would not include logic in tus-node-server to automatically use a 200 if the user specified a body. The response code should explicitly be set by the user in such cases.

netdown commented 4 months ago

Regarding 204, MDN web docs states that The protocol allows user agents to vary in how they process such responses. This is observable in persistent connections, where the invalid body may include a distinct response to a subsequent request. Apple Safari rejects any such data. Google Chrome and Microsoft Edge discard up to four invalid bytes preceding a valid response. Firefox tolerates in excess of a kilobyte of invalid data preceding a valid response.

In this case there must be support for using 200. I think its fine that the status code must be changed manually.