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

Forward status code and body from Pre-create http hook #236

Merged
merged 3 commits into from
Feb 12, 2019

Conversation

oliverpool
Copy link
Contributor

Fix #170

@oliverpool
Copy link
Contributor Author

This uses tusd.HTTPError defined in f50f03f

@Acconut
Copy link
Member

Acconut commented Feb 4, 2019

This is looking good, thank you very much! I only have three comments:

  1. This PR is marked as a WIP. Do you still plan on adding more commits to it?
  2. Would you mind updating the documentation (https://github.com/tus/tusd/blob/master/docs/hooks.md#http-hooks) to include a short sentence about this feature?
  3. Could you post the response body of a POST request if the pre-create hook fails, just to see what it looks like?

@oliverpool
Copy link
Contributor Author

oliverpool commented Feb 5, 2019

  1. I marked it WIP, because I wrote it blind, without actually testing it. I just tested it and added a commit
  2. Sure, will update the doc, as soon as we are clear regarding 1. :-)
  3. I just updated my code, to forward the response of the hook. Before that, it returned:
pre-create hook failed: 403 Forbidden
[original reply of the hook]

But if the hook returned some JSON, then it was all messed up (because of the sentence pretended to the response).


To sum up the state of this PR now:

  • if the hook response is OK, then this response is discarded an the upload proceeds
  • if the hook response is not OK, this response is forwarded as-is to the client (keeping the returned status code)
    The tus server logs 2 lines:
event="HookInvocationError" ... error="endpoint returned: {Code} {CodeStatus}" // the body is not logged
event="ResponseOutgoing" ... error="pre-create hook failed: endpoint returned: {Code} {CodeStatus}" // the body is not logged

I choose not to log the body inside tus, because:

  • it might be multiple lines (which srews the logs a bit)
  • I think it is more the responsibility of the hook to know why it failed (for tus the metrics are more interesting I guess)

This is my understanding of this feature, but I'm open to suggestions and critics!

@oliverpool oliverpool changed the title WIP: Forward status code from Pre-create http hook Forward status code and body from Pre-create http hook Feb 5, 2019
brandonkal added a commit to brandonkal/tusd that referenced this pull request Feb 7, 2019
These changes allow the tusd server to properly return the status code from the pre-create hook to the client.
This way client errors (e.g. 401) will not return 500.
This prevents tus from repeating the request 5 times.

These changes are from the first 2 commits of this PR:
tus#236
@Acconut
Copy link
Member

Acconut commented Feb 7, 2019

  • if the hook response is OK, then this response is discarded an the upload proceeds
  • if the hook response is not OK, this response is forwarded as-is to the client (keeping the returned status code)

Ok, sounds good 👍

I choose not to log the body inside tus.

I agree with that decision to keep the log but exclude the hook response body.

The only thing I am a bit concerned about is the extension of the HTTPError interface. Since it's public (on purpose), some people may use it in their applications. But that's probably too few people, so I hope it should be fine.

So the only missing part is documentation, isn't it?

@oliverpool
Copy link
Contributor Author

The only thing I am a bit concerned about is the extension of the HTTPError interface. Since it's public (on purpose), some people may use it in their applications. But that's probably too few people, so I hope it should be fine.

You are right, that this is actually a breaking change.

So the only missing part is documentation, isn't it?

I've just updated it :-)

Copy link
Member

@Acconut Acconut left a comment

Choose a reason for hiding this comment

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

Perfect, amazing! Thank you very much 👍

@Acconut Acconut merged commit 0baa9ea into tus:master Feb 12, 2019
@oliverpool oliverpool deleted the patch-2 branch February 13, 2019 08:14
@oliverpool
Copy link
Contributor Author

Always a pleasure to contribute to this project (thanks to the helpful and reactive maintainers :-)

@Acconut
Copy link
Member

Acconut commented Feb 21, 2019

Ah, that's very nice to hear :) Thank you

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.

2 participants