@tus/server: add disableTerminationForFinishedUploads option

[fenos]

This PR allows disabling the termination extension for finished uploads via disableTerminationForFinishedUploads.

Currently, the termination extension will allow deleting files even if the upload was completed.

However, when the upload is completed we might want to use a different custom DELETE endpoint to update other parts of the system and handle the deletion ourselves.

[fenos]

@Acconut @Murderlon thoughts on this?
in my use case i need this functionality since i'm OK with letting users who have upload permission to delete their own incompleted uploads.

However, once the file is completed the user who has uploaded the file might not have permission to delete it.
Anyhow, I handle file deletion in a separate endpoint outside of TUS since lots of things going on in there

BTW sorry for the merge commit cluttering on this PR, I've been doing quite a few git maneuvers with the recent merges and i haven't realised they were on the commit history, hopefully a squash would sort them out 😄

[Acconut]

👍 on adding this. I don't see a reason speaking against it.

[Murderlon]

I understand the problem and the need for something but I'm not sure yet on doing this with inheritance. From my OSS experience, adding booleans for problems like this only solves one version of it and soon people will come that they want more control.

For this reason, we let people create their own get() handlers, so they can compose whatever logic is needed.

Wouldn't doing the same for delete make more sense?

[fenos]

@Murderlon it is a valid point, however, this seems somewhat a very useful feature for the termination extension, which would be nice to have in tusd as well.

As you mentioned I do indeed have my own delete endpoint for finished uploads, because once the upload is finished TUS is out of the equation.

I see TUS as an upload server, terminating finished uploads means it also has file management capabilities which I would personally keep separate from TUS.

The termination extension by spec allows to termination of finished uploads, this PR simply adds the capability to handle deletes of finished uploads in a custom endpoint outside of TUS without overwriting the default behavior which I think is nice.

If we don't provide this flag we will end up overwriting the entire handler:

const server = new Server({... })
server.handlers.DELETE = new MyDeleteHandlerWithCustomTermination()

or as you said completely re-implementing a termination extension within TUS.

[Murderlon]

If we don't provide this flag we will end up overwriting the entire handler:
or as you said completely re-implementing a termination extension within TUS.

It doesn't have to be that complicated nor would you need to override the default handler awkwardly.

This is how get does it:

tus-node-server/packages/server/src/server.ts

Lines 136 to 138 in ba8ef31

	get(path: string, handler: RouteHandler) {
	this.handlers.GET.registerPath(path, handler)
	}

tus-node-server/packages/server/src/handlers/GetHandler.ts

Lines 10 to 27 in ba8ef31

	paths: Map<string, RouteHandler> = new Map()
	registerPath(path: string, handler: RouteHandler): void {
	this.paths.set(path, handler)
	}
	/**
	* Read data from the DataStore and send the stream.
	*/
	async send(
	req: http.IncomingMessage,
	res: http.ServerResponse
	// TODO: always return void or a stream?
	): Promise<stream.Writable | void> {
	if (this.paths.has(req.url as string)) {
	const handler = this.paths.get(req.url as string) as RouteHandler
	return handler(req, res)
	}

Wouldn't you say this is a lot more flexible? I'm just a little allergic for boolean settings after seeing Uppy adding dozens of them and people always come that they want it slightly different.

I see TUS as an upload server, terminating finished uploads means it also has file management capabilities which I would personally keep separate from TUS.

I would also be okay with this, meaning we close this PR and do nothing.

[fenos]

Wouldn't you say this is a lot more flexible? I'm just a little allergic for boolean settings after seeing Uppy adding dozens of them and people always come that they want it slightly different.

are you saying that you'd prefer an approach that goes as follows?

const server = new Server({...})

app.delete('/files', (req, res) => {
   server.deleteOnlyUnfinishedUploads(req, res)
   // or
   server.handlers.DELETE.onlyUnfinishedUploads(req, res)
})

// instead of:

const server = new Server({ disableTerminationForFinishedUploads: true })

app.delete('/files', (req, res) => {
   server.handle(req, res)
})

This is how get does it:

I think it does make sense for the GET handler being this way, since the GET method is not really part of the spec, but the DELETE endpoint (termination extension) is, as of now the termination extension is opinionated by the fact that it manages for both finished and unfinished - we don't give the choice to the user to choose if to use TUS as an upload server only or also as a file management server.

Since the GET implementation is completely up to the user because it lands more on the file-management part of the spectrum, tus-node-server makes it optionally available.

However, in this case we do indeed want to bind the DELETE endpoint and offer termination extension as per spec. We just want to limit its use case to unfinished uploads, which i believe is still part of what an upload server is responsible for

I see TUS as an upload server, terminating finished uploads means it also has file management capabilities which I would personally keep separate from TUS.

I would also be okay with this, meaning we close this PR and do nothing.

if we don't provide a mechanism for this within the framework the only way around will be to re-implement the whole DeleteHandler and add this logic in, for the sole purpose of limiting the capabilities of the termination extension, which in reality i believe that this option is very useful generally on the tus user base.

In terms of the boolean flag or the custom method i'd still go with the boolean flag for the simple reason that if we ever turn this server as a cli executable (similarly to tusd) this option will be indeed a boolean flag.

./tus-node-server -ext-termination-unfinished-uploads-only

Similarly if we feel we have too many boolean flags, probably is a signal that we might want to break down the api surface in a more composable manner. But at this point in time it doesn't seem worth over engineering this simple and useful behaviour over a flag.

In the case where a user wants to add custom logic to the termination extension (and not limit it's default behaviour) then they are free to implement their own DELETE endpoint.

The key part here is to limit existing behaviour not adding custom behaviour.

[fenos]

to be honest i'm also ok going down the path of:

const server = new Server({...})

app.delete('/files', (req, res) => {
   server.deleteOnlyUnfinishedUploads(req, res)
   // or
   server.handlers.DELETE.onlyUnfinishedUploads(req, res)
})

as far as we have this behaviour in 😄

The boolean flag seems to provide a better DX (developer experience) since it's less verbose, but if you think is worth going down the more verbose approach i don't mind it if you think it provides benefits that overweight DX

const server = new Server({...})

app.delete('/files', (req, res) => {
   // My Custom Logic
   server.handlers.DELETE.onlyUnfinishedUploads(req, res)
})

// instead of:

const server = new Server({ disableTerminationForFinishedUploads: true })

app.delete('/files', (req, res) => {
   // My custom logic
   server.handle(req, res)
})

not much of a difference

[Murderlon]

I see your points. Thinking about it more, how about this:

const server = new Server({
  path: '/files',
  datastore: new FileStore({ directory: './files' }),
})

server.delete('/files/:id', async (req, res, context, next) => {
  if (canDelete(req)) {
    return next(req, res, context)
  }
  return server.write(context, req, res, 404, 'Not found\n')
})

• Not adding server.delete means it uses the default delete handler.
• Adding a server.delete runs only this new handler, not the default one
• A new parameter is added, next, which is the default handler. This allows people to compose custom logic without re-implementing everything.
• This would mean this.handlers.DELETE.registerPath(path, handler) would need to be a bit smarter I think, as currently it directly puts the path in there, so :id wouldn't be expanded?
• Optional: make context the first argument everywhere for consistency. This may not be a breaking change as all handlers are internal only?

What do you think about this?

[fenos]

@Murderlon Happy holidays!

I think that the callback function you proposed still doesn't solve the issue, i don't see any difference from doing this:

app.delete('/files', (req, res) => {
   if (canDelete(req)) {
    return res.status(400).send('cant delete')
  }
   server.handle(req, res)
})

or doing it with the callback function.

If we want to implement this functionality only in user space, we need to make it clear that the resource is going to be locked during the callback running, which in some cases might not be ideal.

For example an interface along this lines might allow more flexibility:

server.delete('/files/:id', async (req, res, upload, next) => {
  if (upload.size === upload.offset) {
    return server.write(context, req, res, 404, 'Not found\n')
  }
  return next(req, res)
})

however, I still think is way more involved for an abstraction that we might not be benefitting in the long run.

It would be nice to start with the boolean flag, then if we find more use cases in which users need to access the locked resource on delete we can expand it further

[Murderlon]

Maybe it's best indeed to not overthink it for now and keep it simple 👍