Skip to content
This repository has been archived by the owner on Feb 24, 2024. It is now read-only.

meta issue for empty response status issue #2339

Closed
5 tasks done
sio4 opened this issue Oct 8, 2022 · 0 comments · Fixed by #2345
Closed
5 tasks done

meta issue for empty response status issue #2339

sio4 opened this issue Oct 8, 2022 · 0 comments · Fixed by #2345
Assignees
Labels
bug Something isn't working
Milestone

Comments

@sio4
Copy link
Member

sio4 commented Oct 8, 2022

Summary

From the definition of buffalo Handler: https://pkg.go.dev/github.com/gobuffalo/buffalo#Handler

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

However, the responsibility of setting the status code on a response is slightly tricky, since the behavior of the standard ResponseWriter allows developers can omit the status code when it is 200. Even though we can have our own policy, aligning with the standard library could be the best for developers so they can expect the same behavior.

This issue is not just about responsibility but it affects the consistency of request processing. When if the developer omits the status code on their response,

  • The default error middleware doesn't care about the status code if the returned error is nil
  • The default request logger middleware just logs the status code as is, so it will be 0 but the client will get 200
  • The Pop middleware will rollback database transactions when the status code is not set as 2xx or 3xx

I think the direction should be aligned with the standard library, so we need to fix:

  • make the default request logger logs the status code as 200 when the value is still 0.
    • logging the details about auto set could be better but it is optional
  • make the Pop middleware treats zero status code as success (actually informational 1xx should be the same)

Related issues and PRs

Related discussions:

@gobuffalo/core-managers what do you think? any other opinions?
This is a behavior change, but I don't think it will break existing apps widely. I will work on it if you have no concerns.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant