add comments, remove package-specific test (framework already gets corrected)

modules/web/router_path.go

@@ -55,7 +55,7 @@ func (g *RouterPathGroup) MatchPattern(methods string, pattern *RouterPathGroupP
 
 type routerPathParam struct {
 	name         string
-	patSepEnd    bool
+	pathSepEnd   bool
 	captureGroup int
 }
 
@@ -99,7 +99,7 @@ func (p *routerPathMatcher) matchPath(chiCtx *chi.Context, path string) bool {
 			continue
 		}
 		val := path[pm[groupIdx]:pm[groupIdx+1]]
-		if p.params[i].patSepEnd {
+		if p.params[i].pathSepEnd {
 			val = strings.TrimSuffix(val, "/")
 		}
 		chiCtx.URLParams.Add(p.params[i].name, val)
@@ -154,16 +154,19 @@ func patternRegexp(pattern string, h ...any) *RouterPathGroupPattern {
 		// it is not used so no need to implement it now
 		param := routerPathParam{}
 		if partExp == "*" {
+			// "<part:*>" is a shorthand for optionally matching any string (but not greedy)
 			partExp = ".*?"
 			if lastEnd < len(pattern) && pattern[lastEnd] == '/' {
+				// if this param part ends with path separator "/", then consider it together: "(.*?/)"
 				partExp += "/"
-				param.patSepEnd = true
+				param.pathSepEnd = true
 				lastEnd++
 			}
 			re = append(re, '(')
 			re = append(re, partExp...)
-			re = append(re, ')', '?')
+			re = append(re, ')', '?') // the wildcard matching is optional
 		} else {
+			// the pattern is user-provided regexp, defaults to a path part (separated by "/")
 			partExp = util.IfZero(partExp, "[^/]+")
 			re = append(re, '(')
 			re = append(re, partExp...)

routers/api/packages/api.go

@@ -473,12 +473,6 @@ func CommonRoutes() *web.Router {
 			g.MatchPath("GET", "/<group:*>/repodata/<filename>", rpm.GetRepositoryFile)
 			g.MatchPath("PUT", "/<group:*>/upload", reqPackageAccess(perm.AccessModeWrite), rpm.UploadPackageFile)
 			// this URL pattern is only used internally in the RPM index, it is generated by us, the filename part is not really used (can be anything)
-			// ~~Order is important here: the more specific long route (with filename) must come first.~~
-			// ~~Otherwise, when a package name contains "package",~~
-			// ~~* request path: ".../rpm/package/gitea-package/1.0.2-1/x86_64/any-file-name"~~
-			// ~~* it also matches "/rpm/<group:*>/package/<name>/<version>/<architecture>",~~
-			// TODO: the MatchPath has been fixed, the order is not important anymore
-			// As long as the CI passes, we can remove the comments and the new test case (not needed anymore)
 			g.MatchPath("HEAD,GET", "/<group:*>/package/<name>/<version>/<architecture>/<filename>", rpm.DownloadPackageFile)
 			g.MatchPath("HEAD,GET", "/<group:*>/package/<name>/<version>/<architecture>", rpm.DownloadPackageFile)
 			g.MatchPath("DELETE", "/<group:*>/package/<name>/<version>/<architecture>", reqPackageAccess(perm.AccessModeWrite), rpm.DeletePackageFile)

tests/integration/api_packages_rpm_test.go

@@ -15,7 +15,6 @@ import (
 	"strings"
 	"testing"
 
-	"code.gitea.io/gitea/models/db"
 	"code.gitea.io/gitea/models/packages"
 	"code.gitea.io/gitea/models/unittest"
 	user_model "code.gitea.io/gitea/models/user"
@@ -165,29 +164,6 @@ gpgkey=%sapi/packages/%s/rpm/repository.key`,
 				req = NewRequest(t, "GET", fmt.Sprintf("%s/package/%s/%s/%s/any-file-name", groupURL, packageName, packageVersion, packageArchitecture))
 				resp = MakeRequest(t, req, http.StatusOK)
 				assert.Equal(t, content, resp.Body.Bytes())
-
-				// Test with "package" in the name (reproduces bug if fix not applied)
-				pvs, err := packages.GetVersionsByPackageType(t.Context(), user.ID, packages.TypeRpm)
-				assert.NoError(t, err)
-				require.NotEmpty(t, pvs)
-
-				// Rename package to "gitea-package" in DB to test collision
-				_, err = db.GetEngine(t.Context()).Exec("UPDATE package SET name = ?, lower_name = ? WHERE id = ?", "gitea-package", "gitea-package", pvs[0].PackageID)
-				assert.NoError(t, err)
-
-				// Rename package_file to match new name
-				_, err = db.GetEngine(t.Context()).Exec("UPDATE package_file SET name = ?, lower_name = ? WHERE version_id = ?", "gitea-package-1.0.2-1.x86_64.rpm", "gitea-package-1.0.2-1.x86_64.rpm", pvs[0].ID)
-				assert.NoError(t, err)
-
-				defer func() {
-					// Restore name
-					db.GetEngine(t.Context()).Exec("UPDATE package SET name = ?, lower_name = ? WHERE id = ?", packageName, strings.ToLower(packageName), pvs[0].PackageID)
-					db.GetEngine(t.Context()).Exec("UPDATE package_file SET name = ?, lower_name = ? WHERE version_id = ?", fmt.Sprintf("%s-%s.%s.rpm", packageName, packageVersion, packageArchitecture), strings.ToLower(fmt.Sprintf("%s-%s.%s.rpm", packageName, packageVersion, packageArchitecture)), pvs[0].ID)
-				}()
-
-				req = NewRequest(t, "GET", fmt.Sprintf("%s/package/%s/%s/%s/any-file-name", groupURL, "gitea-package", packageVersion, packageArchitecture))
-				resp = MakeRequest(t, req, http.StatusOK)
-				assert.Equal(t, content, resp.Body.Bytes())
 			})
 
 			t.Run("Repository", func(t *testing.T) {