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

Pre-create (http) hook should be able to set a specific status code #170

Closed
ke4 opened this issue Feb 12, 2018 · 10 comments
Closed

Pre-create (http) hook should be able to set a specific status code #170

ke4 opened this issue Feb 12, 2018 · 10 comments

Comments

@ke4
Copy link

ke4 commented Feb 12, 2018

When the http hook returns an error code (4xx-5xx), then there is currently no mechanism to return a specific status code from tusd server other then 500 Internal Server Error. Quote from the documentation: "a non-zero exit code tells tusd to reject the request with a 500 Internal Server Error response containing the process' output from stderr."
I think, it would be better to use the response code and payload (message) from the http hook and forward is as a response to the user/client.
That way if there was a user error (4xx codes - bad request), then the user would know what to fix in the request and could send it again. Sending the original message would direct the user what and how to fix in the request.
Or the user would know that there is something wrong with the server (5xx codes).

@Acconut
Copy link
Member

Acconut commented Feb 12, 2018

I absolutely agree with you that this would be a nice feature to have. However, I am unsure how this could be implemented. The hooks are always invoked as child processes, so communication is limited. Using the exit code is possible but also limited in a certain way since exit code must be in the range of 0-255 where many number have a special meaning. Do you have any concrete ideas?

@kvz
Copy link
Member

kvz commented Feb 13, 2018

Perhaps http status code == 399 + exit code? but maybe, this is just silly :) Is the stdout already used for something? If not, we could dump json with more details maybe.

@Acconut
Copy link
Member

Acconut commented Feb 16, 2018

I just realized that @ke4 is not talking about file hooks but the HTTP ones instead, sorry for the mistake. In this case, exit codes are of not matter and, as he said, we can use the response code of the HTTP hooks. I don't think that this should be a problem.

@sunel
Copy link

sunel commented Apr 6, 2018

Instead of exit code, we can force the hooks to emit a pre-formatted response or code which can be sent back. The same can be implemented for the HTTP too.

@Acconut
Copy link
Member

Acconut commented Apr 9, 2018

we can force the hooks to emit a pre-formatted response or code which can be sent back

Good idea, but I am unsure how we would be able to implement this in a simple yet backwards compatible way to ensure that existing hooks preserve their functionality. Do you have a more concrete thought on how this could look?

@sunel
Copy link

sunel commented Apr 9, 2018

backwards compatible way to ensure that existing hooks preserve their functionality

I think this wont break backwards compatible, in spite of returning a response the exit code will be same, so existing system just works the same.

New approach will send a response which is given by the hooks.

@Acconut
Copy link
Member

Acconut commented Apr 12, 2018

That would be possible, yes. I will keep this on my list

@ke4
Copy link
Author

ke4 commented Jan 14, 2019

Could anybody update me if there is any progress with this issue, please?
Many thanks

@Acconut
Copy link
Member

Acconut commented Jan 16, 2019

No, there has not been any progress yet. But I am happy to assist if you want to give the implementation a try.

@oliverpool
Copy link
Contributor

oliverpool commented Feb 2, 2019

Thanks to f50f03f , it should be quite straightforward:

  1. Replace https://github.com/tus/tusd/blob/master/cmd/tusd/cli/hooks.go#L147 with
	if resp.StatusCode >= http.StatusBadRequest {
		return body, tusd.NewHTTPError(fmt.Errorf("endpoint returned: %s\n%s", resp.Status, body), resp.Status)
	}
  1. Replace https://github.com/tus/tusd/blob/master/cmd/tusd/cli/hooks.go#L35 with
	if output, err := invokeHookSync(HookPreCreate, info, true); err != nil {
		hookErr := fmt.Errorf("pre-create hook failed: %s\n%s", err, string(output))
		if statusErr, ok := err.(HTTPError); ok {
			return "", tusd.NewHTTPError(hookErr, statusErr.StatusCode())
		}
		return "", hookErr
	}

oliverpool added a commit to oliverpool/tusd that referenced this issue Feb 2, 2019
Acconut pushed a commit that referenced this issue Feb 12, 2019
* Forward status code from Pre-create http hook

Fix #170

* Allow the returned body to difer from the logged error

* Update HTTP Hooks documentation
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

5 participants