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

@tus/server@1.1.0 breaking upload. 404 error #620

Open 1nstinct opened 3 months ago

1nstinct commented 3 months ago

I had installed @tus/server@1.0.0. After I upgrading to 1.1.0 my upload is broken. Getting 404 error for PATCH and HEAD requests.

Screenshot 2024-05-28 at 2 19 15 PM

Interesting fact is that the files are actually uploaded to S3 bucket

image

Environment: node v18.16.1

Murderlon commented 3 months ago

There's too little information to help. Please share your integration code and package versions.

1nstinct commented 3 months ago

The code matches the examples found in the Readme for both versions. The issue is that if I have a nested path, version 1.1.0 ignores it, while version 1.0.0 tolerates it. For example, my upload is under the Express router path /models. The server is configured with the same path: /tus-data.

new Server({
  // PATCH requests containing the data will be sent to this path
  path: '/tus-data',
  datastore: s3Store,
  // Generates the key where file is stored on S3 (only using for octree.bin files atm)
  namingFunction: (request) => `${request.params[modelId].key}/octree.bin`,
  respectForwardedHeaders: true,
});

but when it comes to PATH and HEAD v.1.1.0 goes to a wrong URL under the express routing.

PATCH /tus-data/0eb7f1d1-711a-49bb-8ad4-96e4c00507be%2Foctree.bin

instead of path that is generated by v1.0.0 code

PATCH /models/tus-data/0eb7f1d1-711a-49bb-8ad4-96e4c00507be%2Foctree.bin
Murderlon commented 3 months ago

I see, thanks. I didn't realize a breaking change has occurred there. The PR between those versions that did this is likely https://github.com/tus/tus-node-server/pull/515.

For now, the simple fix is your path should be same as what you set in Express. In the meantime we're already on version 1.6.0 of @tus/server and I highly recommend being on the latest version.

Murderlon commented 3 months ago

I see the issue now, in this PR I removed baseUrl because the server should be framework agnostic and that property only exists in Express. So this never worked in other frameworks (unless they coincidentally set that property too). While the breaking change was unintentional, I think it's better to be consistent.

You should be able to fix this by aligning the path's of Express and tus or use generateUrl and set it yourself manually.

1nstinct commented 3 months ago

I managed to fix the issue by adding and configuring two more functions generateUrl() and getFileIdFromRequest():

new Server({
      // PATCH requests containing the data will be sent to this path
      path: '/tus-data',
      datastore: s3Store,
      // Generates the key where file is stored on S3 (only using for octree.bin files atm)
      namingFunction: (request) => `${request.params[modelId].key}/octree.bin`,
      respectForwardedHeaders: true,
      generateUrl(request, {
        proto, host, path, id,
      }) {
        return `${proto}://${host}/models${path}/${id}`;
      },
      getFileIdFromRequest(request) {
        const parts = decodeURIComponent(request.url).split('/');

        return `${parts[2]}/${parts[3]}`;
      },
    })

tested on v1.6.0

Murderlon commented 3 months ago

I'm surprised the getFileIdFromRequest is needed. The ID should only be the actual ID, not /models/{id}.

Have you tried removing both generateUrl and getFileIdFromRequest and set path to /models/tus-data?

1nstinct commented 3 months ago

I'm surprised the getFileIdFromRequest is needed. The ID should only be the actual ID, not /models/{id}.

Have you tried removing both generateUrl and getFileIdFromRequest and set path to /models/tus-data?

Yes. I am getting 404 error for HEAD and PATCH

image

Murderlon commented 3 months ago

I was confused for a second, but the reason you need all three is not because /models/tus-data didn't work, it's because you create a directory in namingFunction, as per the docs:

Adding a slash means you create a new directory, for which you need to implement all three functions as we need encode the id with base64 into the URL.

My guess is that if you didn't create a directory in namingFunction, path works as expected if you keep it same as the full route under express.

So for you there's no way around implementing all three. We also have an example for this: https://github.com/tus/tus-node-server/tree/main/packages/server#example-store-files-in-custom-nested-directories