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: allow customising upload url and upload id extraction #515

Merged

Conversation

fenos
Copy link
Collaborator

@fenos fenos commented Nov 10, 2023

Allow full customization of upload url and upload-id extraction from the URL.
Keeping the default behavior intact.

This PR will allow providing an UploadIdGenerator interface with 2 methods to implement:

  • generateUrl
  • getFileIdFromRequest

This way we can provide users with the possibility of customizing the URL to their liking.
Closes #457

@fenos fenos force-pushed the feat/custom-upload-url-generation-and-extraction branch from a433f73 to 14fe44b Compare November 10, 2023 14:36
Copy link
Member

@Murderlon Murderlon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution. I think it makes sense to offer this feature but I'm not sure about the approach.

  • Conceptually, I'd like to keep "models" for simple interfaces/structs, I don't think we need this as a class there. (from that perspective StreamSplitter and Metadata also don't really belong there but that's another conversation).
  • This puts a bigger implementation burden on the implementer as the entire generateUrl has to be recreated, including the internal options.
  • This makes makes it hard to introduce future changes without breaking custom implementations.

I would go for two simple functions (no class needed I think)

  • getFileIdFromRequest as it is currently
  • generateUrl ideally gets no internal options, instead it receives host and proto which are done by us internally before passing it in. There should be no need to pass relativeLocation because if that's what you want to do in your custom logic, you can just do that. Same for path, this is already known for yourself top level as you pass it into the server. I wonder if this is possible with composition somehow so that we do have those options in scope but we don't expose it to the implementer.

What do you think?

@fenos
Copy link
Collaborator Author

fenos commented Nov 29, 2023

Thanks for this!

Conceptually, I'd like to keep "models" for simple interfaces/structs, I don't think we need this as a class there. (from that perspective StreamSplitter and Metadata also don't really belong there but that's another conversation).

We can refactor this in another PR

This puts a bigger implementation burden on the implementer as the entire generateUrl has to be recreated, including the internal options.
I would go for two simple functions (no class needed I think)

Not necessarily, because if we go with a class such as the above, we can always extend it and get the default behavior + any additional behavior.

However, I'm not against having them as functions, since they can be equally re-used, but they will need to be exported independently and not embedded in the BaseHandler if we want to allow to be re-used

generateUrl ideally gets no internal options, instead it receives host and proto which are done by us internally before passing it in. There should be no need to pass relativeLocation because if that's what you want to do in your custom logic, you can just do that. Same for path, this is already known for yourself top level as you pass it into the server. I wonder if this is possible with composition somehow so that we do have those options in scope but we don't expose it to the implementer.

Ok, this looks like a good compromise!

@fenos
Copy link
Collaborator Author

fenos commented Nov 29, 2023

@Murderlon i've reworked the implementation and now the interface looks as following:

new Server({
  ...
  generateUrl: (req, { proto, host, baseUrl, path, id}) => {
     return 'whatever url composition you want'
  },
  getFileIdFromRequest: (req) => {
     return myCustomExtraction()
  }
})

we can also call the default functions in case we want to just add encoding for example:

import {generateUrl, getFileIdFromRequest} from '@tus/server'

new Server({
  ...
  generateUrl: (req, { proto, host, baseUrl, path, id}) => {
     id = Buffer.from(id, 'utf-8').toString('base64url')
     return generateUrl(id, { proto, host, baseUrl, path })
  },
  getFileIdFromRequest: (req) => {
    const id = getFileIdFromRequest(req.url as string)
    if (!id) return false
    return Buffer.from(id, 'base64url').toString('utf-8')
  }
})

@fenos fenos force-pushed the feat/custom-upload-url-generation-and-extraction branch from 3fe711d to 2ed9f27 Compare November 29, 2023 14:56
Copy link
Member

@Murderlon Murderlon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for applying the feedback! I think we're close to being ready. Some final things:

  • We need docs for this
  • Would be nice to have a simple test in BaseHandler.test.ts
  • I'm inclined not to export the default functions for this. Doesn't add that much value on top of already handling proto and host and we increase the API surface and loose flexibility to change the internal signature.
  • If we don't export, we can move the extractHostAndProto into BaseHandler and inline the generateUrl logic at line 56. Similarly, getFileIdFromRequest can just be inline logic at line 69.

What do you think?

packages/server/src/request.ts Outdated Show resolved Hide resolved
@Murderlon Murderlon changed the title feat: allow customising upload url and upload id extraction @tus/server: allow customising upload url and upload id extraction Nov 30, 2023
@fenos fenos force-pushed the feat/custom-upload-url-generation-and-extraction branch from 2ed9f27 to 76856b0 Compare November 30, 2023 16:17
@fenos
Copy link
Collaborator Author

fenos commented Nov 30, 2023

  • We need docs for this ✅
  • Would be nice to have a simple test in BaseHandler.test.ts ✅
  • I'm inclined not to export the default functions for this. Doesn't add that much value on top of already handling proto and host and we increase the API surface and loose flexibility to change the internal signature.
    If we don't export, we can move the extractHostAndProto into BaseHandler and inline the generateUrl logic at line 56. Similarly, getFileIdFromRequest can just be inline logic at line 69. ✅

All ready!

@fenos fenos force-pushed the feat/custom-upload-url-generation-and-extraction branch 2 times, most recently from bb3643f to 76c9e62 Compare December 1, 2023 09:09
@fenos fenos force-pushed the feat/custom-upload-url-generation-and-extraction branch from 76c9e62 to 3018929 Compare December 1, 2023 09:11
Copy link
Member

@Murderlon Murderlon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect! ✨

packages/server/src/handlers/BaseHandler.ts Outdated Show resolved Hide resolved
@fenos
Copy link
Collaborator Author

fenos commented Dec 1, 2023

@Murderlon ready to merge 🎉

@Murderlon Murderlon merged commit d558ba9 into tus:main Dec 4, 2023
2 checks passed
@fenos fenos deleted the feat/custom-upload-url-generation-and-extraction branch December 5, 2023 08:49
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

Successfully merging this pull request may close these issues.

@tus/s3-store: add object prefixing
2 participants