Repository: gobuffalo/buffalo (go)

Issue:
meta issue for empty response status issue
#### 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](https://pkg.go.dev/net/http#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

* [ ] #2300 --> incorrect information
   * [ ] #2334
* [ ] gobuffalo/buffalo-pop#19 --> inconsistent/contradictory response status and database action
   * [ ] gobuffalo/buffalo-pop#23
* [ ] gobuffalo/buffalo-pop#25 --> incorrect information

#### Related discussions:
* https://gophers.slack.com/archives/C3MSAFD40/p1658983223965079

@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.

Pull request:
added assert middleware to assert handler's behavior. (fix #2339)
For the [net/http#ResponseWriter](https://pkg.go.dev/net/http#ResponseWriter), `Write()` will set the response status as "200 OK" if a handler exited without calling `WriteHeader()` but returns `nil`. However, the Buffalo's behavior is slightly different. The Buffalo's actual responding behavior is basically the same since it uses the same, but the buffalo's middlewares (especially `poptx` from `buffalo-pop`) could treat them as a failed action when no response status is found. This inconsistency made some issues and this PR will fix the issue.

See more details in #2339 and other linked issues.

fixes #2339
fixes gobuffalo/buffalo-pop#25
fixes gobuffalo/buffalo-pop#19

related issues/PRs: #2300, #2334, #2335, 

Patch:
diff --git a/middleware.go b/middleware.go
index 1f2ec152..31dc66f5 100644
--- a/middleware.go
+++ b/middleware.go
@@ -1,6 +1,7 @@
 package buffalo
 
 import (
+	"net/http"
 	"reflect"
 	"runtime"
 	"strings"
@@ -123,15 +124,38 @@ func (ms *MiddlewareStack) Replace(mw1 MiddlewareFunc, mw2 MiddlewareFunc) {
 	ms.stack = stack
 }
 
-func (ms *MiddlewareStack) handler(info RouteInfo) Handler {
-	h := info.Handler
-	if len(ms.stack) > 0 {
-		mh := func(_ Handler) Handler {
-			return h
+// assertMiddleware is a hidden middleware that works just befor and after the
+// actual handler runs to make it sure everything is OK with the Handler
+// specification.
+//
+// It writes response header with `http.StatusOK` if the request handler exited
+// without error but the response status is still zero. Setting response is the
+// responsibility of handler but this middleware make it sure the response
+// should be compatible with middleware specification.
+//
+// See also: https://github.com/gobuffalo/buffalo/issues/2339
+func assertMiddleware(handler Handler) Handler {
+	return func(c Context) error {
+		err := handler(c)
+		if err != nil {
+			return err
 		}
 
-		tstack := []MiddlewareFunc{mh}
+		if res, ok := c.Response().(*Response); ok {
+			if res.Status == 0 {
+				res.WriteHeader(http.StatusOK)
+				c.Logger().Debug("warning: handler exited without setting the response status. 200 OK will be used.")
+			}
+		}
+
+		return err
+	}
+}
+
+func (ms *MiddlewareStack) handler(info RouteInfo) Handler {
+	tstack := []MiddlewareFunc{assertMiddleware}
 
+	if len(ms.stack) > 0 {
 		sl := len(ms.stack) - 1
 		for i := sl; i >= 0; i-- {
 			mw := ms.stack[i]
@@ -140,12 +164,13 @@ func (ms *MiddlewareStack) handler(info RouteInfo) Handler {
 				tstack = append(tstack, mw)
 			}
 		}
+	}
 
-		for _, mw := range tstack {
-			h = mw(h)
-		}
-		return h
+	h := info.Handler
+	for _, mw := range tstack {
+		h = mw(h)
 	}
+
 	return h
 }
 


Test patch:
diff --git a/middleware_test.go b/middleware_test.go
index abd7b0e4..822836f4 100644
--- a/middleware_test.go
+++ b/middleware_test.go
@@ -1,6 +1,7 @@
 package buffalo
 
 import (
+	"fmt"
 	"net/http"
 	"testing"
 
@@ -242,3 +243,67 @@ func Test_Middleware_Remove(t *testing.T) {
 	_ = w.HTML("/no_log_autos/1").Get()
 	r.Len(log, 0)
 }
+
+func Test_AssertMiddleware_NilStatus200(t *testing.T) {
+	r := require.New(t)
+	var status int
+
+	a := New(Options{})
+	a.Use(func(h Handler) Handler {
+		return func(c Context) error {
+			err := h(c)
+
+			res, ok := c.Response().(*Response)
+			r.True(ok)
+			status = res.Status
+
+			return err
+		}
+	})
+
+	a.GET("/200", func(c Context) error {
+		c.Response().WriteHeader(http.StatusOK) // explicitly set
+		return nil
+	})
+
+	a.GET("/404", func(c Context) error {
+		c.Response().WriteHeader(http.StatusNotFound) //explicitly set
+		return nil
+	})
+
+	a.GET("/nil", func(c Context) error {
+		return nil // return nil without setting response status. should be OK
+	})
+
+	a.GET("/500", func(c Context) error {
+		return fmt.Errorf("error") // return error
+	})
+
+	a.GET("/502", func(c Context) error {
+		return HTTPError{Status: http.StatusBadGateway} // return HTTPError
+	})
+
+	a.GET("/panic", func(c Context) error {
+		panic("hoy hoy")
+	})
+
+	tests := []struct {
+		path   string
+		code   int
+		status int
+	}{
+		{"/200", http.StatusOK, http.StatusOK}, // when the handler set response code explicitly (e.g. 200, 404)
+		{"/404", http.StatusNotFound, http.StatusNotFound},
+		{"/nil", http.StatusOK, http.StatusOK},        // when the handler returns nil without setting status code
+		{"/502", http.StatusBadGateway, 0},            // set by defaultErrorHandler, when the handler just returns error
+		{"/500", http.StatusInternalServerError, 0},   // set by defaultErrorHandler, when the handler returns HTTPError
+		{"/panic", http.StatusInternalServerError, 0}, // set by PanicHandler
+	}
+	w := httptest.New(a)
+
+	for _, tc := range tests {
+		res := w.HTML(tc.path).Get()
+		r.Equal(tc.status, status)
+		r.Equal(tc.code, res.Code)
+	}
+}