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

Interface for storing additional data in web storage #726

Open
perotom opened this issue Sep 18, 2024 · 17 comments
Open

Interface for storing additional data in web storage #726

perotom opened this issue Sep 18, 2024 · 17 comments

Comments

@perotom
Copy link

perotom commented Sep 18, 2024

Is your feature request related to a problem? Please describe.
When using services like bunny.net, we need to send additional headers (Authorization, ...) for every request. Those headers are typically generated on the backend as the secret API key is needed. Right now, we use the onBeforeRequest to add those headers after we received the headers in onAfterResponse. Everything works well but not with resumable uploads as the library only saved the upload URL but not the headers needed to successfully upload.

Describe the solution you'd like
Give a way to add additional data to the stored upload URL. One way would be to pass the storage identifier (tus::XXXXXXX:XXXX) to callbacks like onAfterResponse and expose functions like update in WebStorageUrlStorage and give user access to this class.

Describe alternatives you've considered
One way is to use an alternative fingerprint function and do those changes via localStorage without knowledge of the library.

Can you provide help with implementing this feature?
Happy to help.

Additional context

@Acconut
Copy link
Member

Acconut commented Sep 18, 2024

For authorization headers it might be more appropriate to also fetch them from the server before resuming an upload. Then you can ensure that the current user is still allowed to perform this action and that the authorization tokens are still valid. The last point is especially import if a user resumes an upload after a longer duration.

Does that make sense?

@perotom
Copy link
Author

perotom commented Sep 18, 2024

The token expires anyways after a given time so I won't think thats a huge problem. Anyways I see your point.

@perotom perotom closed this as completed Sep 18, 2024
@perotom
Copy link
Author

perotom commented Sep 20, 2024

One thing I noted: To be able to try to fetch the headers again from the server, we would need the fingerprint (better would be the id of the server object) available in PreviousUpload in upload.findPreviousUploads. Is it possible to store this?

Even better would be to give the option to store a custom id to the metadata in storage on onAfterResponse: function (req: tus.HttpRequest, res: tus.HttpResponse).

@perotom perotom reopened this Sep 20, 2024
@Acconut
Copy link
Member

Acconut commented Sep 20, 2024

I'm a bit surprised that you would need the fingerprint for fetching the headers. The fingerprint is a client-side value that has no meaning to the server usually.

In tus, the upload ID is the upload URL, which is part of PreviousUpload (see

this.url = previousUpload.uploadUrl || null
). We forgot to add this value to the documentation, but you can use it. I'll address this shortly :)

@perotom
Copy link
Author

perotom commented Sep 20, 2024

Yes, we use a custom fingerprint method to store it on the server as well to identify the upload object server-side.

Ok, I see, so we would need to search for the given upload URL which needs to be a unique identifier. If every provider fulfils this constraint, this would be a valid approach!

@Acconut
Copy link
Member

Acconut commented Sep 20, 2024

Yes, the upload URL should be unique, but its exact format depends on the used server.

@perotom
Copy link
Author

perotom commented Sep 20, 2024

Ok thank you, then it is a good solution.

@perotom perotom closed this as completed Sep 20, 2024
@perotom
Copy link
Author

perotom commented Sep 30, 2024

I got another question about resumable upload and storing upload URL:
As far as I understood, resumable uploads are stored in web storage. Now we have the following issue:

User uploads file but closes browser at 50%. He tries on another device or browser to upload the other 50%. The resumable information is not stored in web storage. Our API will return the previously used upload URL (as described above). Now tus tries to upload the whole file and the server returns a 409 Conflict status.

Shouldn't tus somehow have a check before starting uploading if the file was already partially uploaded?

@Acconut
Copy link
Member

Acconut commented Oct 2, 2024

We forgot to add this value to the documentation, but you can use it. I'll address this shortly :)

Done in ad68771

User uploads file but closes browser at 50%. He tries on another device or browser to upload the other 50%. The resumable information is not stored in web storage. Our API will return the previously used upload URL (as described above). Now tus tries to upload the whole file and the server returns a 409 Conflict status.

Shouldn't tus somehow have a check before starting uploading if the file was already partially uploaded?

That's not how resuming an upload is intended to work with tus. If a tus client creates a new upload resource, the client expects to start a fresh upload and not resume a previously started uploads. This is why tus-js-client does not send a HEAD request after a POST request.

Most applications do not need to resume uploads across devices, which is why tus-js-client by default stores the resumption information locally on the device. If you want to resume uploads on different devices, I would recommend you to implement an API endpoint on your server to fetch this resumption information whenever a user wants to upload a file. If the server already created an upload for this file, tus-js-client can resume the upload. If not, then tus-js-client can create a new upload. I hope this makes sense.

@perotom
Copy link
Author

perotom commented Oct 3, 2024

Thanks for the information. I thought as the server keeps state, it would be benefitical for the client to make use of this state.

Right now, we had to actually delete the partial data on the server to let the client resume upload (which is exactly the opposit effect of resumable uploads). Especially this affects users who are using privat browsing or use multiple web browsers.

An improvement could be that before the client tries to upload to the given URL, it should check if there is already partial data available and set the corresponding offset header. This would be backward compatible and avoids mentioned problems above. Just a thought.

@Acconut
Copy link
Member

Acconut commented Oct 4, 2024

An improvement could be that before the client tries to upload to the given URL, it should check if there is already partial data available and set the corresponding offset header. This would be backward compatible and avoids mentioned problems above. Just a thought.

This would incur an additional HEAD request for most tus-js-client users, who just create "normal" uploads (i.e. not redirect to an existing resource). Due to the performance penalty, we must avoid such behavior in a standard configuration.

tus-js-client should retry the upload on 409 by sending a HEAD request, so I would expect the upload to finish nevertheless. Is this not the case for you?

We could consider optimizing this scenario even. If tus-js-client gets a 409 with an Upload-Offset header, it can use the new offset without sending a HEAD request first.

@perotom
Copy link
Author

perotom commented Oct 4, 2024

I was getting a 409 conflict response because it started uploading from the start.
I guess it would be a good idea to handle this conflict situation like you described. This would give the mentioned benefits above!

@Acconut
Copy link
Member

Acconut commented Oct 4, 2024

If tus-js-client gets a 409 (and you didn't disable retries), it should resume the upload by sending a HEAD request. Did that not happen for you?

@rizametovd
Copy link

rizametovd commented Dec 3, 2024

I think it would be nice to have the ability to pass extra data to LS as suggested by @perotom .

For example the problem we are facing right now:

  1. User1 uploads some files — everything works fine.
  2. User1 logouts and logins with different account say User2.
  3. LS keeps records for User1.
  4. User2 tries to upload the same files as User1.
  5. HEAD request fails with 500 error.

We clear all LS entries if user clicks on logout button but there are still some ways to logout without clicking logout button (e.g. user manually cleans some entries in LS or cookies). Yes, I know its a bit hacky...

Making extra requests to backend to determine if upload should be resumed or start from scratch looks a little overhead. At the moment we solved this by using metadata (passing userId):

    const previousUploads = await upload.findPreviousUploads();
    const uploadToResume = previousUploads.length > 0 ? previousUploads[0] : null;

    if (uploadToResume && uploadToResume.metadata.userId !== user?.id) {
      clearTusLSEntries.all();
    }

    if (uploadToResume && uploadToResume.metadata.userId === user?.id) {
      upload.resumeFromPreviousUpload(uploadToResume);
    }

    upload.start();

This way we do not need to make any additional requests to backend.

@Acconut
Copy link
Member

Acconut commented Dec 3, 2024

We clear all LS entries if user clicks on logout button but there are still some ways to logout without clicking logout button (e.g. user manually cleans some entries in LS or cookies).

You probably want to clear Local Storage and Session Storage whenever the user signs out to ensure no information is left behind that could leak private data.

I think it would be nice to have the ability to pass extra data to LS as suggested by @perotom .

The upload metadata is persisted in the URL storage, see https://github.com/tus/tus-js-client/blob/main/docs/api.md#tusuploadfindpreviousuploads. You can put the user ID in there and then check if that matches the logged in users. Would that work for you?

@rizametovd
Copy link

rizametovd commented Dec 3, 2024

You can put the user ID in there and then check if that matches the logged in users. Would that work for you?

Yes, it works but in this case metadata will be send to server when creating a new upload?

From docs:

An object with string values used as additional meta data which will be passed along to the server when (and only when) creating a new upload.

Should our backend support metadata or it doesnt matter?

@Acconut
Copy link
Member

Acconut commented Dec 3, 2024

Yes, it works but in this case metadata will be send to server when creating a new upload?

Yes.

Should our backend support metadata or it doesnt matter?

Your backend can ignore the metadata headers. For tus-js-client it does not matter how the server treats metadata. It will persist the metadata in the URL Storage nevertheless.

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

No branches or pull requests

3 participants