Refactor web package and context package (#25298)

1. The "web" package shouldn't depends on "modules/context" package,
instead, let each "web context" register themselves to the "web"
package.
2. The old Init/Free doesn't make sense, so simplify it
* The ctx in "Init(ctx)" is never used, and shouldn't be used that way
* The "Free" is never called and shouldn't be called because the SSPI
instance is shared

---------

Co-authored-by: Giteabot <[email protected]>

Author: wxiaoguang

cmd/web.go

@@ -191,7 +191,7 @@ func runWeb(ctx *cli.Context) error {
 	}
 
 	// Set up Chi routes
-	c := routers.NormalRoutes(graceful.GetManager().HammerContext())
+	c := routers.NormalRoutes()
 	err := listen(c, true)
 	<-graceful.GetManager().Done()
 	log.Info("PID: %d Gitea Web Finished", os.Getpid())

modules/context/api.go

@@ -20,6 +20,8 @@ import (
 	"code.gitea.io/gitea/modules/httpcache"
 	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/setting"
+	"code.gitea.io/gitea/modules/web"
+	web_types "code.gitea.io/gitea/modules/web/types"
 
 	"gitea.com/go-chi/cache"
 )
@@ -41,6 +43,12 @@ type APIContext struct {
 	Package *Package
 }
 
+func init() {
+	web.RegisterResponseStatusProvider[*APIContext](func(req *http.Request) web_types.ResponseStatusProvider {
+		return req.Context().Value(apiContextKey).(*APIContext)
+	})
+}
+
 // Currently, we have the following common fields in error response:
 // * message: the message for end users (it shouldn't be used for error type detection)
 //            if we need to indicate some errors, we should introduce some new fields like ErrorCode or ErrorType

modules/context/base.go

@@ -96,7 +96,11 @@ func (b *Base) SetTotalCountHeader(total int64) {
 
 // Written returns true if there are something sent to web browser
 func (b *Base) Written() bool {
-	return b.Resp.Status() > 0
+	return b.Resp.WrittenStatus() != 0
+}
+
+func (b *Base) WrittenStatus() int {
+	return b.Resp.WrittenStatus()
 }
 
 // Status writes status code

modules/context/context.go

@@ -21,7 +21,9 @@ import (
 	"code.gitea.io/gitea/modules/setting"
 	"code.gitea.io/gitea/modules/templates"
 	"code.gitea.io/gitea/modules/translation"
+	"code.gitea.io/gitea/modules/web"
 	"code.gitea.io/gitea/modules/web/middleware"
+	web_types "code.gitea.io/gitea/modules/web/types"
 
 	"gitea.com/go-chi/cache"
 	"gitea.com/go-chi/session"
@@ -58,6 +60,12 @@ type Context struct {
 	Package *Package
 }
 
+func init() {
+	web.RegisterResponseStatusProvider[*Context](func(req *http.Request) web_types.ResponseStatusProvider {
+		return req.Context().Value(WebContextKey).(*Context)
+	})
+}
+
 // TrHTMLEscapeArgs runs ".Locale.Tr()" but pre-escapes all arguments with html.EscapeString.
 // This is useful if the locale message is intended to only produce HTML content.
 func (ctx *Context) TrHTMLEscapeArgs(msg string, args ...string) string {

modules/context/private.go

@@ -11,6 +11,8 @@ import (
 
 	"code.gitea.io/gitea/modules/graceful"
 	"code.gitea.io/gitea/modules/process"
+	"code.gitea.io/gitea/modules/web"
+	web_types "code.gitea.io/gitea/modules/web/types"
 )
 
 // PrivateContext represents a context for private routes
@@ -21,6 +23,12 @@ type PrivateContext struct {
 	Repo *Repository
 }
 
+func init() {
+	web.RegisterResponseStatusProvider[*PrivateContext](func(req *http.Request) web_types.ResponseStatusProvider {
+		return req.Context().Value(privateContextKey).(*PrivateContext)
+	})
+}
+
 // Deadline is part of the interface for context.Context and we pass this to the request context
 func (ctx *PrivateContext) Deadline() (deadline time.Time, ok bool) {
 	if ctx.Override != nil {

modules/context/response.go

@@ -5,15 +5,20 @@ package context
 
 import (
 	"net/http"
+
+	web_types "code.gitea.io/gitea/modules/web/types"
 )
 
 // ResponseWriter represents a response writer for HTTP
 type ResponseWriter interface {
 	http.ResponseWriter
 	http.Flusher
-	Status() int
+	web_types.ResponseStatusProvider
+
 	Before(func(ResponseWriter))
-	Size() int // used by access logger template
+
+	Status() int // used by access logger template
+	Size() int   // used by access logger template
 }
 
 var _ ResponseWriter = &Response{}
@@ -46,6 +51,10 @@ func (r *Response) Write(bs []byte) (int, error) {
 	return size, nil
 }
 
+func (r *Response) Status() int {
+	return r.status
+}
+
 func (r *Response) Size() int {
 	return r.written
 }
@@ -71,8 +80,8 @@ func (r *Response) Flush() {
 	}
 }
 
-// Status returned status code written
-func (r *Response) Status() int {
+// WrittenStatus returned status code written
+func (r *Response) WrittenStatus() int {
 	return r.status
 }
 

modules/test/context_tests.go

@@ -9,6 +9,7 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"net/url"
+	"strings"
 	"testing"
 
 	access_model "code.gitea.io/gitea/models/perm/access"
@@ -25,19 +26,26 @@ import (
 	"github.com/stretchr/testify/assert"
 )
 
-// MockContext mock context for unit tests
-// TODO: move this function to other packages, because it depends on "models" package
-func MockContext(t *testing.T, path string) *context.Context {
-	resp := httptest.NewRecorder()
+func mockRequest(t *testing.T, reqPath string) *http.Request {
+	method, path, found := strings.Cut(reqPath, " ")
+	if !found {
+		method = "GET"
+		path = reqPath
+	}
 	requestURL, err := url.Parse(path)
 	assert.NoError(t, err)
-	req := &http.Request{
-		URL:  requestURL,
-		Form: url.Values{},
-	}
+	req := &http.Request{Method: method, URL: requestURL, Form: url.Values{}}
+	req = req.WithContext(middleware.WithContextData(req.Context()))
+	return req
+}
 
+// MockContext mock context for unit tests
+// TODO: move this function to other packages, because it depends on "models" package
+func MockContext(t *testing.T, reqPath string) (*context.Context, *httptest.ResponseRecorder) {
+	resp := httptest.NewRecorder()
+	req := mockRequest(t, reqPath)
 	base, baseCleanUp := context.NewBaseContext(resp, req)
-	base.Data = middleware.ContextData{}
+	base.Data = middleware.GetContextData(req.Context())
 	base.Locale = &translation.MockLocale{}
 	ctx := &context.Context{
 		Base:   base,
@@ -48,29 +56,23 @@ func MockContext(t *testing.T, path string) *context.Context {
 
 	chiCtx := chi.NewRouteContext()
 	ctx.Base.AppendContextValue(chi.RouteCtxKey, chiCtx)
-	return ctx
+	return ctx, resp
 }
 
 // MockAPIContext mock context for unit tests
 // TODO: move this function to other packages, because it depends on "models" package
-func MockAPIContext(t *testing.T, path string) *context.APIContext {
+func MockAPIContext(t *testing.T, reqPath string) (*context.APIContext, *httptest.ResponseRecorder) {
 	resp := httptest.NewRecorder()
-	requestURL, err := url.Parse(path)
-	assert.NoError(t, err)
-	req := &http.Request{
-		URL:  requestURL,
-		Form: url.Values{},
-	}
-
+	req := mockRequest(t, reqPath)
 	base, baseCleanUp := context.NewBaseContext(resp, req)
-	base.Data = middleware.ContextData{}
+	base.Data = middleware.GetContextData(req.Context())
 	base.Locale = &translation.MockLocale{}
 	ctx := &context.APIContext{Base: base}
 	_ = baseCleanUp // during test, it doesn't need to do clean up. TODO: this can be improved later
 
 	chiCtx := chi.NewRouteContext()
 	ctx.Base.AppendContextValue(chi.RouteCtxKey, chiCtx)
-	return ctx
+	return ctx, resp
 }
 
 // LoadRepo load a repo into a test context.

modules/web/handler.go

@@ -9,25 +9,15 @@ import (
 	"net/http"
 	"reflect"
 
-	"code.gitea.io/gitea/modules/context"
 	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/web/routing"
+	"code.gitea.io/gitea/modules/web/types"
 )
 
-// ResponseStatusProvider is an interface to check whether the response has been written by the handler
-type ResponseStatusProvider interface {
-	Written() bool
-}
-
-// TODO: decouple this from the context package, let the context package register these providers
-var argTypeProvider = map[reflect.Type]func(req *http.Request) ResponseStatusProvider{
-	reflect.TypeOf(&context.APIContext{}):     func(req *http.Request) ResponseStatusProvider { return context.GetAPIContext(req) },
-	reflect.TypeOf(&context.Context{}):        func(req *http.Request) ResponseStatusProvider { return context.GetWebContext(req) },
-	reflect.TypeOf(&context.PrivateContext{}): func(req *http.Request) ResponseStatusProvider { return context.GetPrivateContext(req) },
-}
+var responseStatusProviders = map[reflect.Type]func(req *http.Request) types.ResponseStatusProvider{}
 
-func RegisterHandleTypeProvider[T any](fn func(req *http.Request) ResponseStatusProvider) {
-	argTypeProvider[reflect.TypeOf((*T)(nil)).Elem()] = fn
+func RegisterResponseStatusProvider[T any](fn func(req *http.Request) types.ResponseStatusProvider) {
+	responseStatusProviders[reflect.TypeOf((*T)(nil)).Elem()] = fn
 }
 
 // responseWriter is a wrapper of http.ResponseWriter, to check whether the response has been written
@@ -36,10 +26,10 @@ type responseWriter struct {
 	status     int
 }
 
-var _ ResponseStatusProvider = (*responseWriter)(nil)
+var _ types.ResponseStatusProvider = (*responseWriter)(nil)
 
-func (r *responseWriter) Written() bool {
-	return r.status > 0
+func (r *responseWriter) WrittenStatus() int {
+	return r.status
 }
 
 func (r *responseWriter) Header() http.Header {
@@ -68,7 +58,7 @@ var (
 func preCheckHandler(fn reflect.Value, argsIn []reflect.Value) {
 	hasStatusProvider := false
 	for _, argIn := range argsIn {
-		if _, hasStatusProvider = argIn.Interface().(ResponseStatusProvider); hasStatusProvider {
+		if _, hasStatusProvider = argIn.Interface().(types.ResponseStatusProvider); hasStatusProvider {
 			break
 		}
 	}
@@ -101,7 +91,7 @@ func prepareHandleArgsIn(resp http.ResponseWriter, req *http.Request, fn reflect
 		case httpReqType:
 			argsIn[i] = reflect.ValueOf(req)
 		default:
-			if argFn, ok := argTypeProvider[argTyp]; ok {
+			if argFn, ok := responseStatusProviders[argTyp]; ok {
 				if isPreCheck {
 					argsIn[i] = reflect.ValueOf(&responseWriter{})
 				} else {
@@ -129,8 +119,8 @@ func handleResponse(fn reflect.Value, ret []reflect.Value) goctx.CancelFunc {
 
 func hasResponseBeenWritten(argsIn []reflect.Value) bool {
 	for _, argIn := range argsIn {
-		if statusProvider, ok := argIn.Interface().(ResponseStatusProvider); ok {
-			if statusProvider.Written() {
+		if statusProvider, ok := argIn.Interface().(types.ResponseStatusProvider); ok {
+			if statusProvider.WrittenStatus() != 0 {
 				return true
 			}
 		}
@@ -161,7 +151,7 @@ func toHandlerProvider(handler any) func(next http.Handler) http.Handler {
 		return http.HandlerFunc(func(respOrig http.ResponseWriter, req *http.Request) {
 			// wrap the response writer to check whether the response has been written
 			resp := respOrig
-			if _, ok := resp.(ResponseStatusProvider); !ok {
+			if _, ok := resp.(types.ResponseStatusProvider); !ok {
 				resp = &responseWriter{respWriter: resp}
 			}
 

modules/web/middleware/data.go

@@ -17,7 +17,7 @@ type ContextDataStore interface {
 
 type ContextData map[string]any
 
-func (ds ContextData) GetData() map[string]any {
+func (ds ContextData) GetData() ContextData {
 	return ds
 }
 

modules/web/route.go

@@ -7,31 +7,31 @@ import (
 	"net/http"
 	"strings"
 
-	"code.gitea.io/gitea/modules/context"
 	"code.gitea.io/gitea/modules/web/middleware"
 
 	"gitea.com/go-chi/binding"
-	chi "github.com/go-chi/chi/v5"
+	"github.com/go-chi/chi/v5"
 )
 
-// Bind binding an obj to a handler
-func Bind[T any](_ T) any {
-	return func(ctx *context.Context) {
+// Bind binding an obj to a handler's context data
+func Bind[T any](_ T) http.HandlerFunc {
+	return func(resp http.ResponseWriter, req *http.Request) {
 		theObj := new(T) // create a new form obj for every request but not use obj directly
-		binding.Bind(ctx.Req, theObj)
-		SetForm(ctx, theObj)
-		middleware.AssignForm(theObj, ctx.Data)
+		data := middleware.GetContextData(req.Context())
+		binding.Bind(req, theObj)
+		SetForm(data, theObj)
+		middleware.AssignForm(theObj, data)
 	}
 }
 
 // SetForm set the form object
-func SetForm(data middleware.ContextDataStore, obj interface{}) {
-	data.GetData()["__form"] = obj
+func SetForm(dataStore middleware.ContextDataStore, obj interface{}) {
+	dataStore.GetData()["__form"] = obj
 }
 
 // GetForm returns the validate form information
-func GetForm(data middleware.ContextDataStore) interface{} {
-	return data.GetData()["__form"]
+func GetForm(dataStore middleware.ContextDataStore) interface{} {
+	return dataStore.GetData()["__form"]
 }
 
 // Route defines a route based on chi's router

modules/web/routing/logger.go

@@ -8,8 +8,8 @@ import (
 	"strings"
 	"time"
 
-	"code.gitea.io/gitea/modules/context"
 	"code.gitea.io/gitea/modules/log"
+	"code.gitea.io/gitea/modules/web/types"
 )
 
 // NewLoggerHandler is a handler that will log routing to the router log taking account of
@@ -86,8 +86,8 @@ func logPrinter(logger log.Logger) func(trigger Event, record *requestRecord) {
 		}
 
 		var status int
-		if v, ok := record.responseWriter.(context.ResponseWriter); ok {
-			status = v.Status()
+		if v, ok := record.responseWriter.(types.ResponseStatusProvider); ok {
+			status = v.WrittenStatus()
 		}
 		logf := log.Info
 		if strings.HasPrefix(req.RequestURI, "/assets/") {

modules/web/types/response.go

@@ -0,0 +1,10 @@
+// Copyright 2023 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package types
+
+// ResponseStatusProvider is an interface to get the written status in the response
+// Many packages need this interface, so put it in the separate package to avoid import cycle
+type ResponseStatusProvider interface {
+	WrittenStatus() int
+}

routers/api/actions/actions.go

@@ -4,15 +4,14 @@
 package actions
 
 import (
-	"context"
 	"net/http"
 
 	"code.gitea.io/gitea/modules/web"
 	"code.gitea.io/gitea/routers/api/actions/ping"
 	"code.gitea.io/gitea/routers/api/actions/runner"
 )
 
-func Routes(_ context.Context, prefix string) *web.Route {
+func Routes(prefix string) *web.Route {
 	m := web.NewRoute()
 
 	path, handler := ping.NewPingServiceHandler()