onUploadFinish should be able to alter response body

[netdown]

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]

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]

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]

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]

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]

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.