fasthttp icon indicating copy to clipboard operation
fasthttp copied to clipboard

NewFastHTTPHandler does not set response status code

Open luisgarciaalanis opened this issue 1 year ago • 7 comments

I recreated the issue because the reopen button is not on the original issue.

The NewFastHTTPHandler() handler does not set the Status code for the response:

https://github.com/valyala/fasthttp/blob/master/fasthttpadaptor/adaptor.go#L59

I have pointed the line of code that needs to be updated to set the status from the ctx response.

The code in line 65 https://github.com/valyala/fasthttp/blob/d3a9c74c92588e83c465057c243e05b46d9118f7/fasthttpadaptor/adaptor.go#L65 Returns a nil value that eventually is changed to 200.

The fix needed is on line 59, the response

w := netHTTPResponseWriter{
	w:   ctx.Response.BodyWriter(),
	ctx: ctx,
}

needs to changed to:

w := netHTTPResponseWriter{
	w:   ctx.Response.BodyWriter(),
	statusCode: ctx.Response.StatusCode(), <---- this is needed
	ctx: ctx,
}

Line 65 needs to be removed since code is set by the caller, ctx already should contain the nil value or a status set by the caller.

luisgarciaalanis avatar Apr 09 '24 00:04 luisgarciaalanis

The handler sets the status code which is captured by netHTTPResponseWriter and then line 65 propagates that to the Response.

Can you show an example of something going wrong?

erikdubbelboer avatar Apr 10 '24 18:04 erikdubbelboer

The handler sets the status code which is captured by netHTTPResponseWriter and then line 65 propagates that to the Response.

Can you show an example of something going wrong?

netHTTPResponseWriter is missing the initialization of statusCode, so when it propagates it propagates a nil value. its not setting the context status that comes from the caller.

luisgarciaalanis avatar Apr 13 '24 20:04 luisgarciaalanis

But that's no problem, cause in net/http not setting a status code means 200 and that's exactly what it is doing here: https://github.com/valyala/fasthttp/blob/d3aa5a15bb0131fe7e58e948cff8ee8ed748112e/fasthttpadaptor/adaptor.go#L98-L100

Or do you mean that you are setting a status code before you call the handler returned by NewFastHTTPHandler and want it to use that?

Like I said, show me an example of this going wrong.

erikdubbelboer avatar Apr 14 '24 08:04 erikdubbelboer

But that's no problem, cause in net/http not setting a status code means 200 and that's exactly what it is doing here:

https://github.com/valyala/fasthttp/blob/d3aa5a15bb0131fe7e58e948cff8ee8ed748112e/fasthttpadaptor/adaptor.go#L98-L100

Or do you mean that you are setting a status code before you call the handler returned by NewFastHTTPHandler and want it to use that?

Like I said, show me an example of this going wrong.

Yes I mean to set the status code before calling the HTTPHandler. It needs to be set on line 59.

// For example imagine a Login handler that wants to set a status
// HTMX handles status to render differently, for example.
func LoginHandler(c *fiber.Ctx) error {
	
	// ........
	if isBadRequest {
		c.Status(http.StatusBadRequest)  <---- this will be converted to 200 always.
		return Render(c, dialog.BadRequest())
	}

	return Render(c, pages.Login())
}

func Render(c *fiber.Ctx, component templ.Component, options ...func(*templ.ComponentHandler)) error {
	componentHandler := templ.Handler(component)
	for _, o := range options {
		o(componentHandler)
	}
	return adaptor.HTTPHandler(componentHandler)(c)
}```

luisgarciaalanis avatar Apr 14 '24 23:04 luisgarciaalanis

Interesting use case. But in that case can't you just turn it around and do this?

	if isBadRequest {
		Render(c, dialog.BadRequest())
		c.Status(http.StatusBadRequest)
	}

erikdubbelboer avatar Apr 15 '24 09:04 erikdubbelboer

You might not know where the Status was set, if the HTTPHandler will reset it you need to get a copy beforehand to set it again afterwards. I found this issue while debugging Templ code using go.fiber, I don't control the adaptor, I just use it on my project. https://github.com/a-h/templ/blob/main/examples/integration-gofiber/main.go

I think it should just respect the Status, the code above I just wrote it as an example.

luisgarciaalanis avatar Apr 16 '24 01:04 luisgarciaalanis

I don't think it happens very often that someone using a http handler will still also want to run fasthttp code. That's why you're the first one to have this issue. I'm afraid I don't have time to fix this now, but a pull request is always welcome.

erikdubbelboer avatar Apr 21 '24 17:04 erikdubbelboer