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

Return a custom HTTP error from the datastore #77

Closed
oliverpool opened this issue Dec 5, 2016 · 7 comments
Closed

Return a custom HTTP error from the datastore #77

oliverpool opened this issue Dec 5, 2016 · 7 comments
Assignees

Comments

@oliverpool
Copy link
Contributor

oliverpool commented Dec 5, 2016

How can I return a custom http error code from a custom datastore ?

For instance, if the file extension is not allowed, I would like to send back a http/403 with a reason file extension not allowed.
From the source, if my datastore returns an errors.New("403: File extension not allowed") it will be returned as a 500 error.

Could the unrouted_handler provide an error structure to customize the status code ?
Like return tusd.NewHttpError(403, "File extension not allowed") (if the error implements the HttpError interface, the status code is custom, otherwise it is set to 500).

The unrouted handler could also benefit from this (to remove the ErrStatusCodes map):

ErrUnsupportedVersion  = NewHttpError(http.StatusPreconditionFailed, "unsupported version")
ErrMaxSizeExceeded     = NewHttpError(http.StatusRequestEntityTooLarge, "maximum size exceeded")
...
@Acconut
Copy link
Member

Acconut commented Dec 7, 2016

I do agree that this is a good idea. I will think a bit about how we can implement this is a proper way.

@oliverpool
Copy link
Contributor Author

Maybe this could provide inspiration: https://elithrar.github.io/article/http-handler-error-handling-revisited/

@Acconut Acconut self-assigned this Dec 8, 2016
@oliverpool
Copy link
Contributor Author

One thought: in the documentation, it should be written that the datastores are supposed to work with any client; a custom http status must be appropriately!

  • 1xx: Continuing process
  • 2xx: Processed successfully
  • 3xx: Redirection (no reason to be used, I think)
  • 4xx: Client error (no need for the client to retry "as-is")
  • 5xx: Server error (a later retry may succeed)

@oliverpool
Copy link
Contributor Author

@Acconut : any update on this?

The final implementation of https://elithrar.github.io/article/http-handler-error-handling-revisited/ allows to be fully backward compatible! (thanks to the switch e := err.(type))

@Acconut
Copy link
Member

Acconut commented Jan 24, 2017

I apologize for me belated response. Backwards-compatibility is not an issue I am concerned about but binding the data store implementations tightly to the HTTP protocol. Right now, the data stores have no knowledge that they are accessed using the HTTP protocol and keeping it like that has some advantages to separate the HTTP server accepting requests and the data store which takes care of handling the uploads. I haven't fully decided whether that's a big enough reason to prevent your feature request to be implemented or not but I will give it a try the next few days and then we will see.

@oliverpool
Copy link
Contributor Author

oliverpool commented Jan 25, 2017

I get your point about not only thinking about the http protocol.

Currently the specs only tells about http:

"The protocol provides a mechanism for resumable file uploads via HTTP/1.1 (RFC 7230) and HTTP/2 (RFC 7540)." https://tus.io/protocols/resumable-upload.html#abstract

And I think that if another protocol is supported at some point, it will also need a way to get some status about the transmission. Since the http status codes are well documented, they can be used as a basis to be translated to match the other protocol status codes.

Moreover I see no drawback into providing a custom error interface (StatusError{403, err} for instance) to categorize the errors:

  • if you need the categorization at a later point in time, it's already available
  • if you don't need it, you can still consider them as regular error

@Acconut
Copy link
Member

Acconut commented Jan 26, 2017

I added a corresponding interface to address this issue. However, I am not entirely sure whether to keep the name, HTTPError. Will think about this a bit more :)

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

No branches or pull requests

2 participants