Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
1nstinct opened this issue May 28, 2024 · 8 comments
Open

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

1nstinct opened this issue May 28, 2024 · 8 comments

Comments

@1nstinct
Copy link

1nstinct commented May 28, 2024

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

@1nstinct 1nstinct changed the title tus-node-server@1.1.0 breaking upload. 404 error @tus/server@1.1.0 breaking upload. 404 error May 28, 2024
@Murderlon
Copy link
Member

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

@1nstinct
Copy link
Author

1nstinct commented May 30, 2024

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
Copy link
Member

Murderlon commented Jun 3, 2024

I see, thanks. I didn't realize a breaking change has occurred there. The PR between those versions that did this is likely #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
Copy link
Member

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
Copy link
Author

1nstinct commented Jun 4, 2024

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
Copy link
Member

Murderlon commented Jun 5, 2024

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
Copy link
Author

1nstinct commented Jun 6, 2024

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
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants