Handle unset http status

[AtomicNibble]

I have a webhook handler that returns nil when no errors I don't explicilty set a http status code.

func MyWebhook(c buffalo.Context) error {
     // ...
     return nil
}

Which will return a http status code of 200.
But since the status code is zero at the point when the middleware checks the status code, buffalo-pop will rollback the transaction even tho there was no error.

So you end up with a endpoint that returns 200 and no error but silently rollsback the transaction.

Maybe:

if res, ok := c.Response().(*buffalo.Response); ok {
	if res.Status != 0 && (res.Status < 200 || res.Status >= 400) {
		return errNonSuccess
	}
}

Thanks.

[sio4]

Could you please explain some more about your use case with the output of buffalo task middleware for your application if you are still interested in this issue?

I understand there could be some exceptional(?) cases to set the response code outside of the actual handler but I wonder if we can handle the situation in a different way. e.g. register the status handling middleware after the pop middleware so the status should be correctly placed before the pop checking it.

Actually, checking the status code is currently the spec of the function Transaction() so changing this will be a breaking change and we should consider side effects.

[AtomicNibble]

To be clear:

func MyHandler(c buffalo.Context) error {
     return nil
}

Will return http 200 even if you have zero middleware, this is standard behaviour.

I'm not wanting to set the response code outside of the actual handler, but not setting a error code explictly defaults the response code to 200. The problem is the db TX middleware not handling this case so the client will see http 200, but the db transaction got rolledback.

[sio4]

Oh, I see. I don't know why but I was confused that your issue is a matter of a particular condition with your own middleware to set a response code outside the handler. Now I think I got your point, your code relies on the behavior of the ResponseWriter and you don't want to set the status yourself from the handler.

I agree that the current behavior of "200 but rolled back" is indeed problematic, so we need to fix something. One tricky point is that this is middleware for the buffalo and the definition of the buffalo's handler is slightly strict than the standard library (even though it works as the same on this issue):

It is the responsibility of the Handler to handle the request/response correctly.

source: https://pkg.go.dev/github.com/gobuffalo/buffalo#Handler

I think this middleware's current implementation follows this concept, the user's handler should set the correct response status before returns.

I would like to check some more things related to this issue, and will get back.

[AtomicNibble]

Great, thanks.

[sio4]

Related issue: #25

[sio4]

gobuffalo/buffalo#2345 fixed this issue.