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

NewFastHTTPHandler does not set response status code #1754

Open
luisgarciaalanis opened this issue Apr 9, 2024 · 7 comments
Open

NewFastHTTPHandler does not set response status code #1754

luisgarciaalanis opened this issue Apr 9, 2024 · 7 comments

Comments

@luisgarciaalanis
Copy link

luisgarciaalanis commented Apr 9, 2024

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

ctx.SetStatusCode(w.StatusCode())

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.

@erikdubbelboer
Copy link
Collaborator

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?

@luisgarciaalanis
Copy link
Author

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.

@erikdubbelboer
Copy link
Collaborator

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:

if w.statusCode == 0 {
return http.StatusOK
}

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.

@luisgarciaalanis
Copy link
Author

luisgarciaalanis commented Apr 14, 2024

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:

if w.statusCode == 0 {
return http.StatusOK
}

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)
}```

@erikdubbelboer
Copy link
Collaborator

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)
	}

@luisgarciaalanis
Copy link
Author

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.

@erikdubbelboer
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants