@tus/s3-store: add object prefixing

[Murderlon]

Prefixing can be used to create a pseudo-directory structure in the bucket.

tusd supports this with a hardcoded ObjectPrefix option:

https://github.com/tus/tusd/blob/b4ffdf43f034099ee46437271de2ff98c1f52397/pkg/s3store/s3store.go#L987-L994

in @tus/s3-store we have partKey where this could be supported:

tus-node-server/packages/s3-store/index.ts

Lines 144 to 154 in 0af377d

	private partKey(id: string, isIncomplete = false) {
	if (isIncomplete) {
	id += '.part'
	}
	// TODO: introduce ObjectPrefixing for parts and incomplete parts.
	// ObjectPrefix is prepended to the name of each S3 object that is created
	// to store uploaded files. It can be used to create a pseudo-directory
	// structure in the bucket, e.g. "path/to/my/uploads".
	return id
	}

However it might be more flexible have a function getObjectPrefix (upload: Upload) => string so it can be done dynamically based on metadata?

cc @fenos

[fenos]

Hi @Murderlon yes, a function would be better

However, I think that the getObjectPrefix can be used directly in the @tus/server as opposed to each storage adapter, since this would be useful to overwrite the default upload-id coming from the client and simply passed down to the storage adapter to determine the path location

It can be called directly in the getFileIdFromRequest method

In my use case, I'd need to access the request object as well since I'll be prefixing the upload-id with a trusted header example:

new Server({
  getObjectPrefix: (request:Request, upload: Upload) => {
    return request.headers['tenant-id']
  } 
})

The only caveat here is that when we generate the URL on the PostHandler we want to remove the prefix from the generated URL, but that should be easy enough in the generateUrl

As an example, we do exactly this in the Supabase storage-api

overwrite the PostHandler getFileIdFromRequest and remove it on the generateUrl method

[Murderlon]

I'm not sure that would be better. We already have namingFunction which would more or less be conflicting. It also feels a bit hacky to use the ID to create a storage path implicitly, rather than explicitly prepending it with a object prefix for S3, and the fact that we would have to reverse whatever was done to generate the URL.

If it's valuable to have the req for this function, we can make that happen too in getObjectPrefix on the store itself.

cc @Acconut

[fenos]

Yea, I agree that doing it this way seems a bit hacky, somewhat seems like a workaround on the TUS protocol which maps URI 1:1 to the file path

I believe that the use case is quite legitimate to have implicit paths, and it can apply to any sort of Storage adapter.

In our case, we also support the FileStore adapter which we would need the same object prefixing functionality in there, hence I thought that having it at the server level could make sense since you want this prefix shared across all store adapters.

I agree that namingFunction and getObjectPrefix would be quite conflicting, maybe we can simply expose the two methods getFileIdFromRequest and generateUrl on the Server instead and give full freedom to the user:

const server = new Server({
   generateUrl(): string {
      ....
   },
   getFileIdFromRequest(): string {
      ...
   }
})

This way the user can have the freedom of customising the URL to its liking and still be consistent to the underline TUS protocol

[Acconut]

I agree that it would be useful to be able to separate the upload ID (and thus the upload URL) from the storage path. Sometimes you want to customize the upload URL, sometimes the storage path, and sometimes both. It's not the most pressing issue, so the first approach for implementation was to just couple the two together.

We could thing about a general solution where the user is able to fully influence the storage path for all storages, but I think adding an option for object prefix is also fine. That's the easier option for users if they don't want to fiddle around with a custom storage path generation approach.

TUS protocol which maps URI 1:1 to the file path

It's worth noting that the tus protocol itself does not make any assumption about the URL format or the storage path. They can be whatever they want. Only in our implementation (tusd and tus-node-server) we have added the concept of an upload ID that corresponds to the destination because it is a simple implementation that works great in most situations.

[Murderlon]

I like the idea of splitting it into two functions. However it puts quite the burden on on the implementer and would make options.relativeLocation and options.respectForwardedHeaders obsolete when used. People would have to reimplement this logic:

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

Lines 35 to 70 in 0af377d

	generateUrl(req: http.IncomingMessage, id: string) {
	id = encodeURIComponent(id)
	const forwarded = req.headers.forwarded as string | undefined
	const path = this.options.path === '/' ? '' : this.options.path
	// @ts-expect-error baseUrl type doesn't exist?
	const baseUrl = req.baseUrl ?? ''
	let proto
	let host
	if (this.options.relativeLocation) {
	return `${baseUrl}${path}/${id}`
	}
	if (this.options.respectForwardedHeaders) {
	if (forwarded) {
	host ??= reForwardedHost.exec(forwarded)?.[1]
	proto ??= reForwardedProto.exec(forwarded)?.[1]
	}
	const forwardHost = req.headers['x-forwarded-host']
	const forwardProto = req.headers['x-forwarded-proto']
	// @ts-expect-error we can pass undefined
	if (['http', 'https'].includes(forwardProto)) {
	proto ??= forwardProto as string
	}
	host ??= forwardHost
	}
	host ??= req.headers.host
	proto ??= 'http'
	return `${proto}://${host}${baseUrl}${path}/${id}`
	}

Those are complexities I'd rather keep internal. If the problem is specifically about augmenting storage paths, then think we should keep solution tied to the storage solutions rather than make the ID's "magical".

We can do something like this:

const {Server} = require('@tus/server')
const {FileStore} = require('@tus/file-store')
const {S3Store} = require('@tus/s3-store')

const getFilePath = (req, upload) => req.headers['tenant-id']
const s3Store = new S3Store({getFilePath})
const fileStore = new FileStore({getFilePath})
const datastore = useFileStore ? fileStore : s3Store

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

[Acconut]

Alternatively, you could also generate the storage path once before upload is created and then store it in the upload's info object persistently. Whenever, you load an upload, you first look up the info file, extract the storage path and then load the upload from there.

The benefit is that you do not have to derive the storage path from the upload ID for every request. The downside is that this way the info file cannot have a custom path.

[shellscape]

To toss another variant in here: We'd like to store the metadata in a different location/prefix than the resulting file.

[elliotdickison]

@Murderlon @fenos Would you mind giving an example of how to use these functions to set an object prefix in S3? With @tus/server@1.0.0 and @tus/s3-store@1.0.1 I was able to set an object prefix using the namingFunction option and it worked perfectly. After upgrading to 1.2.0 for both packages I am now getting "No file found" 404 errors for the PATCH request that follows upload creation.

I tried moving my prefix logic from namingFunction to getFileIdFromRequest but I still get a 404 for the PATCH request. In fact I get a 404 for the patch request if I use getFileIdFromRequest at all. I moved my prefix logic to generateUrl to customize the URL which worked as far as changing the URL goes, but the file is uploaded to S3 without the prefix present in that URL.

To be honest I'm feeling a little bit confused about the overlap between namingFunction, getFileIdFromRequest, and generateUrl which all seem to have various levels of overlap.

Any ideas? Thanks in advance, and thanks for your work on this. Tus is a life saver.

[Murderlon]

Hi, thanks for the feedback. There definitely should not have been a regression so I'll take a look. I'll see what I can do about the docs as well to clear it up and perhaps we want to think about deprecating namingFunction to only have two functions.

[Murderlon]

@elliotdickison do you have a reproducible example? I created a test and ran it against the latest version and on @tus/server@1.0.0 and they both fail when namingFunction returns an extra path prefix. Both versions succeed if you return something without a /.

    it.only('should use namingFunction correctly', (done) => {
      const server = new Server({
        path: '/test/output',
        datastore: new FileStore({directory: './test/output'}),
        namingFunction() {
          return '1234/5678'
        },
      })
      const length = Buffer.byteLength('test', 'utf8').toString()
      request(server.listen())
        .post(server.options.path)
        .set('Tus-Resumable', TUS_RESUMABLE)
        .set('Upload-Length', length)
        .then((res) => {
          console.log(res.headers.location)
          request(server.listen())
            .patch(removeProtocol(res.headers.location))
            .send('test')
            .set('Tus-Resumable', TUS_RESUMABLE)
            .set('Upload-Offset', '0')
            .set('Content-Type', 'application/offset+octet-stream')
            .expect(204, done)
        })
    })

[elliotdickison]

Thanks for the quick response and updated docs, those did help. Now that I understand the functions I've pinpointed the problem I'm having: namingFunction can no longer return the extra path prefix, and generateUrl only affects the Location header but has no bearing on the final object key in storage.

I used to be able to do this:

// Used to work, no longer
new Server({
  namingFunction () {
    return "this/is/the/object/key.txt"
  }
})

Based on your updated documentation I tried the following, but it doesn't behave as I would hope:

new Server({
  // The object key will be key.txt
  namingFunction () {
    return "key.txt"
  },
  // The Location header will be /this/is/the/object/key.txt but this does not affect
  // the final object key in storage. The extra prefix "/this/is/the/object" will be ignored
  // and the full object key will be "key.txt"
  generateUrl(req, { baseUrl, id }) {
    return `${baseUrl}/file/will/be/stored/${id}`
  }
})

As far as I can tell it is now impossible to include a / in an object key, which means I can no longer take advantage of the pseudo-folder features offered by many object storage providers where / is treated like a directory separator.

[Murderlon]

As fas as I can tell, namingFunction never explicitly supported that use case across all stores. For instance file store doesn't create all directories up to the file in question, is expects it to be in the directory directly:

tus-node-server/packages/file-store/index.ts

Lines 60 to 62 in da1f740

	create(file: Upload): Promise<Upload> {
	return new Promise((resolve, reject) => {
	fs.open(path.join(this.directory, file.id), 'w', async (err, fd) => {

So we would probably have to consistently add and test it in all stores.