Refactor arch route handlers (#32972)

modules/web/route.go

@@ -4,14 +4,18 @@
 package web
 
 import (
+	"fmt"
 	"net/http"
 	"net/url"
 	"reflect"
+	"regexp"
 	"strings"
 
+	"code.gitea.io/gitea/modules/container"
 	"code.gitea.io/gitea/modules/htmlutil"
 	"code.gitea.io/gitea/modules/reqctx"
 	"code.gitea.io/gitea/modules/setting"
+	"code.gitea.io/gitea/modules/util"
 	"code.gitea.io/gitea/modules/web/middleware"
 
 	"gitea.com/go-chi/binding"
@@ -181,6 +185,17 @@ func (r *Router) NotFound(h http.HandlerFunc) {
 	r.chiRouter.NotFound(h)
 }
 
+type pathProcessorParam struct {
+	name         string
+	captureGroup int
+}
+
+type PathProcessor struct {
+	methods container.Set[string]
+	re      *regexp.Regexp
+	params  []pathProcessorParam
+}
+
 func (r *Router) normalizeRequestPath(resp http.ResponseWriter, req *http.Request, next http.Handler) {
 	normalized := false
 	normalizedPath := req.URL.EscapedPath()
@@ -238,6 +253,83 @@ func (r *Router) normalizeRequestPath(resp http.ResponseWriter, req *http.Reques
 	next.ServeHTTP(resp, req)
 }
 
+func (p *PathProcessor) ProcessRequestPath(chiCtx *chi.Context, path string) bool {
+	if !p.methods.Contains(chiCtx.RouteMethod) {
+		return false
+	}
+	if !strings.HasPrefix(path, "/") {
+		path = "/" + path
+	}
+	pathMatches := p.re.FindStringSubmatchIndex(path) // Golang regexp match pairs [start, end, start, end, ...]
+	if pathMatches == nil {
+		return false
+	}
+	var paramMatches [][]int
+	for i := 2; i < len(pathMatches); {
+		paramMatches = append(paramMatches, []int{pathMatches[i], pathMatches[i+1]})
+		pmIdx := len(paramMatches) - 1
+		end := pathMatches[i+1]
+		i += 2
+		for ; i < len(pathMatches); i += 2 {
+			if pathMatches[i] >= end {
+				break
+			}
+			paramMatches[pmIdx] = append(paramMatches[pmIdx], pathMatches[i], pathMatches[i+1])
+		}
+	}
+	for i, pm := range paramMatches {
+		groupIdx := p.params[i].captureGroup * 2
+		chiCtx.URLParams.Add(p.params[i].name, path[pm[groupIdx]:pm[groupIdx+1]])
+	}
+	return true
+}
+
+func NewPathProcessor(methods, pattern string) *PathProcessor {
+	p := &PathProcessor{methods: make(container.Set[string])}
+	for _, method := range strings.Split(methods, ",") {
+		p.methods.Add(strings.TrimSpace(method))
+	}
+	re := []byte{'^'}
+	lastEnd := 0
+	for lastEnd < len(pattern) {
+		start := strings.IndexByte(pattern[lastEnd:], '<')
+		if start == -1 {
+			re = append(re, pattern[lastEnd:]...)
+			break
+		}
+		end := strings.IndexByte(pattern[lastEnd+start:], '>')
+		if end == -1 {
+			panic(fmt.Sprintf("invalid pattern: %s", pattern))
+		}
+		re = append(re, pattern[lastEnd:lastEnd+start]...)
+		partName, partExp, _ := strings.Cut(pattern[lastEnd+start+1:lastEnd+start+end], ":")
+		lastEnd += start + end + 1
+
+		// TODO: it could support to specify a "capture group" for the name, for example: "/<name[2]:(\d)-(\d)>"
+		// it is not used so no need to implement it now
+		param := pathProcessorParam{}
+		if partExp == "*" {
+			re = append(re, "(.*?)/?"...)
+			if lastEnd < len(pattern) {
+				if pattern[lastEnd] == '/' {
+					lastEnd++
+				}
+			}
+		} else {
+			partExp = util.IfZero(partExp, "[^/]+")
+			re = append(re, '(')
+			re = append(re, partExp...)
+			re = append(re, ')')
+		}
+		param.name = partName
+		p.params = append(p.params, param)
+	}
+	re = append(re, '$')
+	reStr := string(re)
+	p.re = regexp.MustCompile(reStr)
+	return p
+}
+
 // Combo delegates requests to Combo
 func (r *Router) Combo(pattern string, h ...any) *Combo {
 	return &Combo{r, pattern, h}

modules/web/route_test.go

@@ -7,177 +7,160 @@ import (
 	"bytes"
 	"net/http"
 	"net/http/httptest"
-	"strconv"
+	"strings"
 	"testing"
 
 	"code.gitea.io/gitea/modules/setting"
 	"code.gitea.io/gitea/modules/test"
+	"code.gitea.io/gitea/modules/util"
 
-	chi "github.com/go-chi/chi/v5"
+	"github.com/go-chi/chi/v5"
 	"github.com/stretchr/testify/assert"
 )
 
-func TestRoute1(t *testing.T) {
-	buff := bytes.NewBufferString("")
-	recorder := httptest.NewRecorder()
-	recorder.Body = buff
-
-	r := NewRouter()
-	r.Get("/{username}/{reponame}/{type:issues|pulls}", func(resp http.ResponseWriter, req *http.Request) {
-		username := chi.URLParam(req, "username")
-		assert.EqualValues(t, "gitea", username)
-		reponame := chi.URLParam(req, "reponame")
-		assert.EqualValues(t, "gitea", reponame)
-		tp := chi.URLParam(req, "type")
-		assert.EqualValues(t, "issues", tp)
-	})
+func chiURLParamsToMap(chiCtx *chi.Context) map[string]string {
+	pathParams := chiCtx.URLParams
+	m := make(map[string]string, len(pathParams.Keys))
+	for i, key := range pathParams.Keys {
+		if key == "*" && pathParams.Values[i] == "" {
+			continue // chi router will add an empty "*" key if there is a "Mount"
+		}
+		m[key] = pathParams.Values[i]
+	}
+	return m
+}
 
-	req, err := http.NewRequest("GET", "http://localhost:8000/gitea/gitea/issues", nil)
-	assert.NoError(t, err)
-	r.ServeHTTP(recorder, req)
-	assert.EqualValues(t, http.StatusOK, recorder.Code)
+func TestPathProcessor(t *testing.T) {
+	testProcess := func(pattern, uri string, expectedPathParams map[string]string) {
+		chiCtx := chi.NewRouteContext()
+		chiCtx.RouteMethod = "GET"
+		p := NewPathProcessor("GET", pattern)
+		assert.True(t, p.ProcessRequestPath(chiCtx, uri), "use pattern %s to process uri %s", pattern, uri)
+		assert.Equal(t, expectedPathParams, chiURLParamsToMap(chiCtx), "use pattern %s to process uri %s", pattern, uri)
+	}
+	testProcess("/<p1>/<p2>", "/a/b", map[string]string{"p1": "a", "p2": "b"})
+	testProcess("/<p1:*>", "", map[string]string{"p1": ""}) // this is a special case, because chi router could use empty path
+	testProcess("/<p1:*>", "/", map[string]string{"p1": ""})
+	testProcess("/<p1:*>/<p2>", "/a", map[string]string{"p1": "", "p2": "a"})
+	testProcess("/<p1:*>/<p2>", "/a/b", map[string]string{"p1": "a", "p2": "b"})
+	testProcess("/<p1:*>/<p2>", "/a/b/c", map[string]string{"p1": "a/b", "p2": "c"})
 }
 
-func TestRoute2(t *testing.T) {
+func TestRouter(t *testing.T) {
 	buff := bytes.NewBufferString("")
 	recorder := httptest.NewRecorder()
 	recorder.Body = buff
 
-	hit := -1
+	type resultStruct struct {
+		method      string
+		pathParams  map[string]string
+		handlerMark string
+	}
+	var res resultStruct
+
+	h := func(optMark ...string) func(resp http.ResponseWriter, req *http.Request) {
+		mark := util.OptionalArg(optMark, "")
+		return func(resp http.ResponseWriter, req *http.Request) {
+			res.method = req.Method
+			res.pathParams = chiURLParamsToMap(chi.RouteContext(req.Context()))
+			res.handlerMark = mark
+		}
+	}
 
 	r := NewRouter()
+	r.Get("/{username}/{reponame}/{type:issues|pulls}", h("list-issues-a")) // this one will never be called
 	r.Group("/{username}/{reponame}", func() {
+		r.Get("/{type:issues|pulls}", h("list-issues-b"))
 		r.Group("", func() {
-			r.Get("/{type:issues|pulls}", func(resp http.ResponseWriter, req *http.Request) {
-				username := chi.URLParam(req, "username")
-				assert.EqualValues(t, "gitea", username)
-				reponame := chi.URLParam(req, "reponame")
-				assert.EqualValues(t, "gitea", reponame)
-				tp := chi.URLParam(req, "type")
-				assert.EqualValues(t, "issues", tp)
-				hit = 0
-			})
-
-			r.Get("/{type:issues|pulls}/{index}", func(resp http.ResponseWriter, req *http.Request) {
-				username := chi.URLParam(req, "username")
-				assert.EqualValues(t, "gitea", username)
-				reponame := chi.URLParam(req, "reponame")
-				assert.EqualValues(t, "gitea", reponame)
-				tp := chi.URLParam(req, "type")
-				assert.EqualValues(t, "issues", tp)
-				index := chi.URLParam(req, "index")
-				assert.EqualValues(t, "1", index)
-				hit = 1
-			})
+			r.Get("/{type:issues|pulls}/{index}", h("view-issue"))
 		}, func(resp http.ResponseWriter, req *http.Request) {
-			if stop, err := strconv.Atoi(req.FormValue("stop")); err == nil {
-				hit = stop
+			if stop := req.FormValue("stop"); stop != "" {
+				h(stop)(resp, req)
 				resp.WriteHeader(http.StatusOK)
 			}
 		})
-
 		r.Group("/issues/{index}", func() {
-			r.Get("/view", func(resp http.ResponseWriter, req *http.Request) {
-				username := chi.URLParam(req, "username")
-				assert.EqualValues(t, "gitea", username)
-				reponame := chi.URLParam(req, "reponame")
-				assert.EqualValues(t, "gitea", reponame)
-				index := chi.URLParam(req, "index")
-				assert.EqualValues(t, "1", index)
-				hit = 2
-			})
+			r.Post("/update", h("update-issue"))
 		})
 	})
 
-	req, err := http.NewRequest("GET", "http://localhost:8000/gitea/gitea/issues", nil)
-	assert.NoError(t, err)
-	r.ServeHTTP(recorder, req)
-	assert.EqualValues(t, http.StatusOK, recorder.Code)
-	assert.EqualValues(t, 0, hit)
-
-	req, err = http.NewRequest("GET", "http://localhost:8000/gitea/gitea/issues/1", nil)
-	assert.NoError(t, err)
-	r.ServeHTTP(recorder, req)
-	assert.EqualValues(t, http.StatusOK, recorder.Code)
-	assert.EqualValues(t, 1, hit)
-
-	req, err = http.NewRequest("GET", "http://localhost:8000/gitea/gitea/issues/1?stop=100", nil)
-	assert.NoError(t, err)
-	r.ServeHTTP(recorder, req)
-	assert.EqualValues(t, http.StatusOK, recorder.Code)
-	assert.EqualValues(t, 100, hit)
-
-	req, err = http.NewRequest("GET", "http://localhost:8000/gitea/gitea/issues/1/view", nil)
-	assert.NoError(t, err)
-	r.ServeHTTP(recorder, req)
-	assert.EqualValues(t, http.StatusOK, recorder.Code)
-	assert.EqualValues(t, 2, hit)
-}
-
-func TestRoute3(t *testing.T) {
-	buff := bytes.NewBufferString("")
-	recorder := httptest.NewRecorder()
-	recorder.Body = buff
-
-	hit := -1
-
 	m := NewRouter()
-	r := NewRouter()
 	r.Mount("/api/v1", m)
-
 	m.Group("/repos", func() {
 		m.Group("/{username}/{reponame}", func() {
-			m.Group("/branch_protections", func() {
-				m.Get("", func(resp http.ResponseWriter, req *http.Request) {
-					hit = 0
+			m.Group("/branches", func() {
+				m.Get("", h())
+				m.Post("", h())
+				m.Group("/{name}", func() {
+					m.Get("", h())
+					m.Patch("", h())
+					m.Delete("", h())
+				})
 			})
-				m.Post("", func(resp http.ResponseWriter, req *http.Request) {
-					hit = 1
 		})
-				m.Group("/{name}", func() {
-					m.Get("", func(resp http.ResponseWriter, req *http.Request) {
-						hit = 2
 	})
-					m.Patch("", func(resp http.ResponseWriter, req *http.Request) {
-						hit = 3
+
+	testRoute := func(methodPath string, expected resultStruct) {
+		t.Run(methodPath, func(t *testing.T) {
+			res = resultStruct{}
+			methodPathFields := strings.Fields(methodPath)
+			req, err := http.NewRequest(methodPathFields[0], methodPathFields[1], nil)
+			assert.NoError(t, err)
+			r.ServeHTTP(recorder, req)
+			assert.EqualValues(t, expected, res)
 		})
-					m.Delete("", func(resp http.ResponseWriter, req *http.Request) {
-						hit = 4
+	}
+
+	t.Run("Root Router", func(t *testing.T) {
+		testRoute("GET /the-user/the-repo/other", resultStruct{})
+		testRoute("GET /the-user/the-repo/pulls", resultStruct{
+			method:      "GET",
+			pathParams:  map[string]string{"username": "the-user", "reponame": "the-repo", "type": "pulls"},
+			handlerMark: "list-issues-b",
 		})
+		testRoute("GET /the-user/the-repo/issues/123", resultStruct{
+			method:      "GET",
+			pathParams:  map[string]string{"username": "the-user", "reponame": "the-repo", "type": "issues", "index": "123"},
+			handlerMark: "view-issue",
 		})
+		testRoute("GET /the-user/the-repo/issues/123?stop=hijack", resultStruct{
+			method:      "GET",
+			pathParams:  map[string]string{"username": "the-user", "reponame": "the-repo", "type": "issues", "index": "123"},
+			handlerMark: "hijack",
 		})
+		testRoute("POST /the-user/the-repo/issues/123/update", resultStruct{
+			method:      "POST",
+			pathParams:  map[string]string{"username": "the-user", "reponame": "the-repo", "index": "123"},
+			handlerMark: "update-issue",
 		})
 	})
 
-	req, err := http.NewRequest("GET", "http://localhost:8000/api/v1/repos/gitea/gitea/branch_protections", nil)
-	assert.NoError(t, err)
-	r.ServeHTTP(recorder, req)
-	assert.EqualValues(t, http.StatusOK, recorder.Code)
-	assert.EqualValues(t, 0, hit)
+	t.Run("Sub Router", func(t *testing.T) {
+		testRoute("GET /api/v1/repos/the-user/the-repo/branches", resultStruct{
+			method:     "GET",
+			pathParams: map[string]string{"username": "the-user", "reponame": "the-repo"},
+		})
 
-	req, err = http.NewRequest("POST", "http://localhost:8000/api/v1/repos/gitea/gitea/branch_protections", nil)
-	assert.NoError(t, err)
-	r.ServeHTTP(recorder, req)
-	assert.EqualValues(t, http.StatusOK, recorder.Code, http.StatusOK)
-	assert.EqualValues(t, 1, hit)
+		testRoute("POST /api/v1/repos/the-user/the-repo/branches", resultStruct{
+			method:     "POST",
+			pathParams: map[string]string{"username": "the-user", "reponame": "the-repo"},
+		})
 
-	req, err = http.NewRequest("GET", "http://localhost:8000/api/v1/repos/gitea/gitea/branch_protections/master", nil)
-	assert.NoError(t, err)
-	r.ServeHTTP(recorder, req)
-	assert.EqualValues(t, http.StatusOK, recorder.Code)
-	assert.EqualValues(t, 2, hit)
+		testRoute("GET /api/v1/repos/the-user/the-repo/branches/master", resultStruct{
+			method:     "GET",
+			pathParams: map[string]string{"username": "the-user", "reponame": "the-repo", "name": "master"},
+		})
 
-	req, err = http.NewRequest("PATCH", "http://localhost:8000/api/v1/repos/gitea/gitea/branch_protections/master", nil)
-	assert.NoError(t, err)
-	r.ServeHTTP(recorder, req)
-	assert.EqualValues(t, http.StatusOK, recorder.Code)
-	assert.EqualValues(t, 3, hit)
+		testRoute("PATCH /api/v1/repos/the-user/the-repo/branches/master", resultStruct{
+			method:     "PATCH",
+			pathParams: map[string]string{"username": "the-user", "reponame": "the-repo", "name": "master"},
+		})
 
-	req, err = http.NewRequest("DELETE", "http://localhost:8000/api/v1/repos/gitea/gitea/branch_protections/master", nil)
-	assert.NoError(t, err)
-	r.ServeHTTP(recorder, req)
-	assert.EqualValues(t, http.StatusOK, recorder.Code)
-	assert.EqualValues(t, 4, hit)
+		testRoute("DELETE /api/v1/repos/the-user/the-repo/branches/master", resultStruct{
+			method:     "DELETE",
+			pathParams: map[string]string{"username": "the-user", "reponame": "the-repo", "name": "master"},
+		})
+	})
 }
 
 func TestRouteNormalizePath(t *testing.T) {

routers/api/packages/api.go

@@ -37,6 +37,8 @@ import (
 	"code.gitea.io/gitea/routers/api/packages/vagrant"
 	"code.gitea.io/gitea/services/auth"
 	"code.gitea.io/gitea/services/context"
+
+	"github.com/go-chi/chi/v5"
 )
 
 func reqPackageAccess(accessMode perm.AccessMode) func(ctx *context.Context) {
@@ -139,39 +141,33 @@ func CommonRoutes() *web.Router {
 		r.Group("/arch", func() {
 			r.Methods("HEAD,GET", "/repository.key", arch.GetRepositoryKey)
 
-			r.Methods("HEAD,GET,PUT,DELETE", "*", func(ctx *context.Context) {
-				path := strings.Trim(ctx.PathParam("*"), "/")
+			reqPutRepository := web.NewPathProcessor("PUT", "/<repository:*>")
+			reqGetRepoArchFile := web.NewPathProcessor("HEAD,GET", "/<repository:*>/<architecture>/<filename>")
+			reqDeleteRepoNameVerArch := web.NewPathProcessor("DELETE", "/<repository:*>/<name>/<version>/<architecture>")
 
-				if ctx.Req.Method == "PUT" {
+			r.Any("*", func(ctx *context.Context) {
+				chiCtx := chi.RouteContext(ctx.Req.Context())
+				path := ctx.PathParam("*")
+
+				if reqPutRepository.ProcessRequestPath(chiCtx, path) {
 					reqPackageAccess(perm.AccessModeWrite)(ctx)
 					if ctx.Written() {
 						return
 					}
-					ctx.SetPathParam("repository", path)
 					arch.UploadPackageFile(ctx)
 					return
 				}
 
-				pathFields := strings.Split(path, "/")
-				pathFieldsLen := len(pathFields)
-
-				if (ctx.Req.Method == "HEAD" || ctx.Req.Method == "GET") && pathFieldsLen >= 2 {
-					ctx.SetPathParam("repository", strings.Join(pathFields[:pathFieldsLen-2], "/"))
-					ctx.SetPathParam("architecture", pathFields[pathFieldsLen-2])
-					ctx.SetPathParam("filename", pathFields[pathFieldsLen-1])
+				if reqGetRepoArchFile.ProcessRequestPath(chiCtx, path) {
 					arch.GetPackageOrRepositoryFile(ctx)
 					return
 				}
 
-				if ctx.Req.Method == "DELETE" && pathFieldsLen >= 3 {
+				if reqDeleteRepoNameVerArch.ProcessRequestPath(chiCtx, path) {
 					reqPackageAccess(perm.AccessModeWrite)(ctx)
 					if ctx.Written() {
 						return
 					}
-					ctx.SetPathParam("repository", strings.Join(pathFields[:pathFieldsLen-3], "/"))
-					ctx.SetPathParam("name", pathFields[pathFieldsLen-3])
-					ctx.SetPathParam("version", pathFields[pathFieldsLen-2])
-					ctx.SetPathParam("architecture", pathFields[pathFieldsLen-1])
 					arch.DeletePackageVersion(ctx)
 					return
 				}