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

Is there a way to pass variables between the `onUploadCreate` and the `onUploadFinish` functions? #593

Closed sammibajrami closed 5 months ago

sammibajrami commented 5 months ago

I have figured out a way to do it but it's very awkward and also involves changing and adding a few lines in the @tus/server/dist/handlers/PostHandler.js file.

Basically the way that I have it working is that you update the "Upload-Metadata" header of the res object in the onUploadCreate function. This header is then parsed in the PostHandler.js file and the result overwrites the upload.metadata variable, and then the upload object passes the extra metadata to the onUploadFinish function.

Any chance that there is a better way already built in that I'm missing?

Thank you in advance!

Murderlon commented 5 months ago

Hi, this is an XY problem. What is the problem you are trying to solve? Are you only communicating between hooks or does the client also need to know this value?

Maybe you can just do it in server memory with a Map. Or if it needs to survive server restarts, put it in something like Redis?

sammibajrami commented 5 months ago

I'm sorry, here is the problem: I need to pass a variable from the onUploadCreate function to the onUploadFinish function. It does not need to be sent back to the client.

Is there a way to do this?

If not, then I'll look into your other suggestions, thank you!

Murderlon commented 5 months ago

I don't know the requirements and why you are trying to pass that variable. There are many ways to write/read to different things depending on what you are doing, but those all fall into your hands and setup, there is no built-in way of doing this.

sammibajrami commented 5 months ago

I'm sorry for being unclear.

I'm using the onUploadCreate function to validate the upload request, a process that generates a unique ID that I need to get in the onUploadFinish function.

Redis would be a way to do this, but I was hoping that I could simply update the metadata of the upload, but it sounds like that is not an option, correct?

Murderlon commented 5 months ago

If you tell me why you need the ID to be accessible by the onUploadFinish hook, we can see if there are alternative approaches.

You can't change the metadata in onUploadCreate in a way that's persisted in the store too. We may want to support that at some point.

sammibajrami commented 5 months ago

Once the upload is completed I create an entry for it, that includes the unique ID, in a separate database.

I'd be happy to share the code that I wrote to make the changes to the upload object persistent or even make a pull request if you think it'd help. I've got it down to just adding two lines and changing two lines in the tus-node-server/packages/server/src/handlers/PostHandler.ts file.

The main issue with my approach is that the changes do not persist all the way back to the client (which isn't necessary in my case, but still).

Murderlon commented 5 months ago

Once the upload is completed I create an entry for it, that includes the unique ID, in a separate database.

Can't you just validate the upload in onUploadCreate, reject if needed, and only create and store the ID in onUploadFinish without the need to communicate between hooks?

sammibajrami commented 5 months ago

I would, but the ID is created as part of the validation process. I'll just create a fork for myself with those small changes I mentioned, it should be simple to keep up to date with the main repo.

That said, would you like me to create a pull request with those changes?

The way it works is that the onUploadCreate function returns both the res object and the upload object to the PostHandler which then overwrites the upload object in that file same as the res object.

I'm not sufficiently familiar with the whole codebase to be sure that that's the best way to do it, what do you think?

sammibajrami commented 5 months ago

For reference, here is the updated code:

        let upload = new models_1.Upload({
            id,
            size: upload_length ? Number.parseInt(upload_length, 10) : undefined,
            offset: 0,
            metadata,
        });
        if (this.options.onUploadCreate) {
            try {
                const afterUploadCreate = await this.options.onUploadCreate(req, res, upload);
                res = afterUploadCreate[0];
                upload = afterUploadCreate[1];
            }
            catch (error) {
                log(`onUploadCreate error: ${error.body}`);
                throw error;
            }
        }
sammibajrami commented 5 months ago

Here is the fork: https://github.com/sammibajrami/tus-node-server

Murderlon commented 5 months ago

Thanks for sharing. We can't allow to override the entire upload, only metadata should be allowed, and the metadata should be validated as well. It's also important to make it backwards compatible to prevent a breaking change.

I'll take next week probably. Created an issue: https://github.com/tus/tus-node-server/issues/594

sammibajrami commented 5 months ago

No problem, thank you for taking the time to help me!

While I'm sure that there is a smoother way to do this, here is updated code that only overwrites upload.metadata, while also being backwards compatible:

        let upload = new models_1.Upload({
            id,
            size: upload_length ? Number.parseInt(upload_length, 10) : undefined,
            offset: 0,
            metadata,
        });
        if (this.options.onUploadCreate) {
            try {
                const afterUploadCreate = await this.options.onUploadCreate(req, res, upload);
                res = afterUploadCreate[0];
                if (afterUploadCreate[1]) {
                    upload.metadata = afterUploadCreate[1].metadata;
                }
            }
            catch (error) {
                log(`onUploadCreate error: ${error.body}`);
                throw error;
            }
        }