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

Returning a body in onUploadFinish is not having any effect in the response #714

Closed
2 tasks done
empz opened this issue Feb 6, 2025 · 12 comments
Closed
2 tasks done
Labels

Comments

@empz
Copy link

empz commented Feb 6, 2025

Initial checklist

  • I understand this is a bug report and questions should be posted in the Community Forum
  • I searched issues and couldn’t find anything (or linked relevant results below)

Steps to reproduce

onUploadFinish: async (req, res, upload) => {
    return {
      res,
      status_code: 204,
      body: `https://${S3_ENDPOINT}/${S3_BUCKET}/${upload.id}`
    }
  }

Context: https://community.transloadit.com/t/how-to-customize-the-url-reported-back-to-the-client-in-tus-server/17457/7

Expected behavior

The response contains the right 204 status code and body.

Actual behavior

No matter what's returned on the onUploadFinish, the response is always 200 with an empty body.

Example response with above code. Should be 204 and body should not be empty.

HTTP/1.1 200 OK
Tus-Resumable: 1.0.0
Access-Control-Allow-Origin: http://localhost:3000
Access-Control-Expose-Headers: Authorization, Content-Type, Location, Tus-Extension, Tus-Max-Size, Tus-Resumable, Tus-Version, Upload-Concat, Upload-Defer-Length, Upload-Length, Upload-Metadata, Upload-Offset, X-HTTP-Method-Override, X-Requested-With, X-Forwarded-Host, X-Forwarded-Proto, Forwarded
Cache-Control: no-store
Upload-Offset: 4483969
Upload-Length: 4483969
Upload-Metadata: relativePath bnVsbA==,name aXRzLW15LWxpZmUubXAz,type YXVkaW8vbXBlZw==,userId M2E0ZjlkNGUtZTg4Mi00MzI4LTg2NDUtYmQyOTNiZTAxNDIy,contentType YXVkaW8vbXBlZw==,filetype YXVkaW8vbXBlZw==,filename aXRzLW15LWxpZmUubXAz
Date: Thu, 06 Feb 2025 08:45:17 GMT
Connection: keep-alive
Keep-Alive: timeout=5

@empz empz added the bug label Feb 6, 2025
@Murderlon
Copy link
Member

We have an e2e test confirming this works:

it('should allow response to be changed in onUploadFinish', (done) => {
const server = new Server({
path: '/test/output',
datastore: new FileStore({directory}),
async onUploadFinish(_, res) {
return {
res,
status_code: 200,
body: '{ fileProcessResult: 12 }',
headers: {'X-TestHeader': '1'},
}
},
})
request(server.listen())
.post(server.options.path)
.set('Tus-Resumable', TUS_RESUMABLE)
.set('Upload-Length', '4')
.then((res) => {
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(200, '{ fileProcessResult: 12 }')
.then((r) => {
assert.equal(r.headers['upload-offset'], '4')
assert.equal(r.headers['x-testheader'], '1')
done()
})
})
})

Are you on the latest version?

@Acconut
Copy link
Member

Acconut commented Feb 6, 2025

Example response with above code. Should be 204 and body should not be empty.

Are you sure this is a response to a PATCH request? Looks like a HEAD response to me (due to the Upload-Metadata header being present). onUploadFinish cannot influence HEAD responses. Your frontend is likely resuming previous uploads, and therefore no new upload is finished and no PATCH request is sent.

@empz
Copy link
Author

empz commented Feb 6, 2025

It's a PATCH request. It's using standard Uppy dashboard so it should just work, right?

Image

20250206-0959-03.3643603.mp4

@empz
Copy link
Author

empz commented Feb 6, 2025

Yes, I'm on latest versions:

"@tus/s3-store": "1.8.0",
"@tus/server": "1.10.1",

@Acconut
Copy link
Member

Acconut commented Feb 6, 2025

Could it be that the use of consola.info instead of console.info in onUploadFinish causes an exception in the handler? If so, the callback cannot influence the response.

@Acconut
Copy link
Member

Acconut commented Feb 6, 2025

If that's the case, the logs should also contain a notice about this:

log(`onUploadFinish: ${error.body}`)

@empz
Copy link
Author

empz commented Feb 6, 2025

If that's the case, the logs should also contain a notice about this:

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

Line 141 in 7db2f17

log(onUploadFinish: ${error.body})

Uhm no, that's not it. The following code has the same issue.

onUploadFinish: async (req, res, upload) => {
    return {
      res,
      status_code: 204,
      body: `https://${S3_ENDPOINT}/${S3_BUCKET}/${upload.id}`
    }
  }

@empz
Copy link
Author

empz commented Feb 6, 2025

Ok, so I think I've found the issue.

For some reason, when I upload a file I've already uploaded, the onUploadFinish doesn't execute. But still, on the browser, the upload-success Uppy event triggers. So while on client it looks like the file was indeed uploaded, the server is ignoring the duplicate.

Is there some kind of cache or something that ignores re-uploads?

My namingFunction is generating a new id for each file so I was assuming uploading the same file multiple times, would result in multiple files. At least, that's how it works when using Transloadit.

  namingFunction(_req, metadata) {
    invariant(metadata?.filename, "filename is required")
    invariant(metadata?.userId, "userId is required")

    const id = nanoid()
    const userId = metadata.userId;
    const filename = metadata.filename;

    const extension = filename.split('.').pop();

    return `${userId}/${id}.${extension}`
  },

@Acconut
Copy link
Member

Acconut commented Feb 12, 2025

For some reason, when I upload a file I've already uploaded, the onUploadFinish doesn't execute. But still, on the browser, the upload-success Uppy event triggers. So while on client it looks like the file was indeed uploaded, the server is ignoring the duplicate.

Is there some kind of cache or something that ignores re-uploads?

tus-js-client and Uppy stores information about started uploads in localStorage to allow users to resume uploads. This also includes completed uploads, meaning that when trying to upload the same file again, tus-js-client recognizes that it transferred this file before, checks with the server whether the file still exists and then does not upload the file again. You can enable tus-js-client's removeFingerprintOnSuccess option to remove completed uploads from localStorage and prevent this from happening, if you don't want this behavior for your app.

@empz
Copy link
Author

empz commented Feb 12, 2025

For some reason, when I upload a file I've already uploaded, the onUploadFinish doesn't execute. But still, on the browser, the upload-success Uppy event triggers. So while on client it looks like the file was indeed uploaded, the server is ignoring the duplicate.
Is there some kind of cache or something that ignores re-uploads?

tus-js-client and Uppy stores information about started uploads in localStorage to allow users to resume uploads. This also includes completed uploads, meaning that when trying to upload the same file again, tus-js-client recognizes that it transferred this file before, checks with the server whether the file still exists and then does not upload the file again. You can enable tus-js-client's removeFingerprintOnSuccess option to remove completed uploads from localStorage and prevent this from happening, if you don't want this behavior for your app.

I understand that, but quoting you "and then does not upload the file again" so then why does the upload-success event fires on the client? If the client knows it has previously uploaded the file, and the server also knows this, the event should not fire. There's no upload involved in this case, so there shouldn't be an upload-success event triggered...

@Acconut
Copy link
Member

Acconut commented Feb 12, 2025

I answered that question in https://community.transloadit.com/t/allow-duplicate-files-whist-keeping-the-resumable-feature/17086/6:

We call it resuming, when the client checks on the upload’s current progress and then transfers any remaining data if necessary. If the upload is already complete, no data is transferred, but we still call the overall process resumption. If you want to give it another name, feel free to do so :slight_smile:

Anyway, if that’s the default, then why is it triggering upload-success if it’s not really uploading anything?

The upload-success event is always triggered when the client notices that the upload is done. Either from finishing a new upload or resuming an existing upload.

@Murderlon
Copy link
Member

Closing this as it's not an issue but how resumable uploads work. If you want to upload duplicates files, you can set removeFingerprintOnSuccess in tus-js-client or Uppy.

@Murderlon Murderlon closed this as not planned Won't fix, can't repro, duplicate, stale Feb 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants