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

Multipart upload: Changing Tus header "on the fly" doesn't change headers in PATCH requests #3434

Closed
nebomilic opened this issue Jan 13, 2022 · 12 comments
Labels

Comments

@nebomilic
Copy link

nebomilic commented Jan 13, 2022

Hey there 👋

I will just add to what has been written many times before: great work with uppy and tus 👏

Maybe someone here can help me figure out whether I am facing a real issue or it is just my ignorance.

I am using @uppy/core: 1.20.0 with @uppy/tus: 1.9.2 (so last stable version 1).

I have reproduced this with both @uppy/core: 1.20.0 with @uppy/tus: 1.9.2, and @uppy/core: 2.1.4 with @uppy/tus: 2.2.0.

The situation at hand is that I have a multipart upload enabled.
Furthermore, upload is authenticated, so I am sending an access token with the header.

The access token is refreshed every 5 minutes, but upload can take longer than that.
That means that token can change between two chunks of the same file.

To update the header on the fly, whenever the session token changes I am doing:

uppy.getPlugin('Tus').setOptions({ headers });

Nevertheless, PATCH requests have all the same token and for that reason at some point I get an 401.

Any ideas with this?

Thanks a lot 🙏

@nebomilic
Copy link
Author

nebomilic commented Jan 17, 2022

I have observed the same behaviour with latest versions:
@uppy/core: 2.1.4 and @uppy/tus: 2.2.0

I have integrated this comment in the original issue description 👆

@nebomilic
Copy link
Author

When looking into uppy.getPlugin('Tus') , it's opts.header indeed gets updated.

That leads me to the conclusion that the upload is somehow not taking this field into consideration 🤷

@nebomilic
Copy link
Author

I have found a solution that works for me:

 Object.values(uppy.getPlugin('Tus').uploaders).forEach((uploader) => {
    if (uploader) {
      uploader.options = { ...uploader.options, headers};
    }
  });

I would still consider this a bug though.
My expectation is that this can be achieved by calling setOptions() 🤷‍♀️

Of course if you see it differently, you may close this issue.

@Murderlon
Copy link
Member

Sorry for the late response and good to hear you found something that works for you. @arturi @mifi should setOptions do this too?

@BoreBalboa
Copy link

Hello everyone, I also want to thank your team for everything you have done with uppy and tus.

I have the same problem. @nebomilic could you maybe explain in detail how did you fix your problem. I can't solve it the way you solved it. There is a list of "uploaders" from the plugin but they do not have any fields and in your example there seems to be an "uploader.options" field. When I loop through the forEach loop the "uploader" is empty. Could you maybe elaborate more? As @nebomilic said the setOptions doesn't work when you want to set the refreshed token.

One more question for @nebomilic or someone who can answer is how can I stop the upload when there is an upload error? What I want to do is stop the upload when the access token expires, refresh the token, and then continue the upload. I have tried using the event "upload-error" with the prefix "once" but it returns undefined. I have tried many solutions but I was not successful.

@nebomilic
Copy link
Author

Hi @BoreBalboa 👋

I am not sure, but I think uploaders are not null when actual upload is in progress.

The code I posted above is the actual code I am using in my app, the only part missing is that it's being executed inside of an useEffect hook, that has headers in dependency array.

I hope this helps 😊

@BoreBalboa
Copy link

Hi,

Thanks for trying to help. I can't seem to make it work. Do you stop the upload on an error?

@nebomilic
Copy link
Author

In our codebase we do not stop upload programatically, uppy and tus take care of this.
Sharing your code and error message would help.

@BoreBalboa
Copy link

I use the useUppy hook to initialize uppy. In the code below I set the Tus plugin and my token comes from local storage.

.use(Tus, {
                endpoint: `myUrl/V1/upload`,
                headers: {authorization: "Bearer " + token},
                retryDelay: null,
                chunkSize: 52428800,
                limit: 10
            })
    

When I log out "Object.values(uppy2.getPlugin('Tus').uploaders)" I get an array filled with null values. Are you maybe setting the Tus plugin in a different way?

@nebomilic
Copy link
Author

I am indeed not using useUppy hook, but have a custom hook that uses Uppy directly.
However, I don't think that is causing the problem here.

The question here is at which point are you logging uppy2.getPlugin('Tus').uploaders ?

Is it before the upload starts or during the upload.

I would recommend you log uploaders multiple times during your upload.
You can do this simply by using setInterval.

In my case uploaders consist of null values as well, but at some point there are actual TUS uploaders taking their place.

@BoreBalboa
Copy link

@nebomilic Thank you for your help I fixed my problem with your solution. It indeed does work as you said. I also hope in the future setOptions will be fixed and that people won't have this problem.

@Murderlon
Copy link
Member

Hi, I updated the docs in #3720 and the best way to dynamically change headers is in the onBeforeRequest handler.

Example:

import Uppy from '@uppy/core'
import Tus from '@uppy/tus'

new Uppy().use(Tus, { endpoint: '', onBeforeRequest, onShouldRetry, onAfterResponse })

async function onBeforeRequest (req) {
  const token = await getAuthToken()
  req.setHeader('Authorization', `Bearer ${token}`)
}

function onShouldRetry (err, retryAttempt, options, next) {
  if (err?.originalResponse?.getStatus() === 401) {
    return true
  }
  return next(err)
}

async function onAfterResponse (req, res) {
  if (res.getStatus() === 401) {
    await refreshAuthToken()
  }
}

I think this is the way it should be done so closing this. But we can still discuss if necessary.

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