Improve package API log handling (#35100)

Simplify code and fix log processing logic

routers/api/packages/alpine/alpine.go

@@ -25,9 +25,8 @@ import (
 )
 
 func apiError(ctx *context.Context, status int, obj any) {
-	helper.LogAndProcessError(ctx, status, obj, func(message string) {
-		ctx.PlainText(status, message)
-	})
+	message := helper.ProcessErrorForUser(ctx, status, obj)
+	ctx.PlainText(status, message)
 }
 
 func GetRepositoryKey(ctx *context.Context) {

routers/api/packages/arch/arch.go

@@ -24,9 +24,8 @@ import (
 )
 
 func apiError(ctx *context.Context, status int, obj any) {
-	helper.LogAndProcessError(ctx, status, obj, func(message string) {
-		ctx.PlainText(status, message)
-	})
+	message := helper.ProcessErrorForUser(ctx, status, obj)
+	ctx.PlainText(status, message)
 }
 
 func GetRepositoryKey(ctx *context.Context) {

routers/api/packages/cargo/cargo.go

@@ -37,15 +37,14 @@ type StatusMessage struct {
 }
 
 func apiError(ctx *context.Context, status int, obj any) {
-	helper.LogAndProcessError(ctx, status, obj, func(message string) {
-		ctx.JSON(status, StatusResponse{
-			OK: false,
-			Errors: []StatusMessage{
-				{
-					Message: message,
-				},
+	message := helper.ProcessErrorForUser(ctx, status, obj)
+	ctx.JSON(status, StatusResponse{
+		OK: false,
+		Errors: []StatusMessage{
+			{
+				Message: message,
 			},
-		})
+		},
 	})
 }
 

routers/api/packages/chef/chef.go

@@ -30,10 +30,9 @@ func apiError(ctx *context.Context, status int, obj any) {
 		ErrorMessages []string `json:"error_messages"`
 	}
 
-	helper.LogAndProcessError(ctx, status, obj, func(message string) {
-		ctx.JSON(status, Error{
-			ErrorMessages: []string{message},
-		})
+	message := helper.ProcessErrorForUser(ctx, status, obj)
+	ctx.JSON(status, Error{
+		ErrorMessages: []string{message},
 	})
 }
 

routers/api/packages/composer/composer.go

@@ -28,18 +28,17 @@ import (
 )
 
 func apiError(ctx *context.Context, status int, obj any) {
-	helper.LogAndProcessError(ctx, status, obj, func(message string) {
-		type Error struct {
-			Status  int    `json:"status"`
-			Message string `json:"message"`
-		}
-		ctx.JSON(status, struct {
-			Errors []Error `json:"errors"`
-		}{
-			Errors: []Error{
-				{Status: status, Message: message},
-			},
-		})
+	message := helper.ProcessErrorForUser(ctx, status, obj)
+	type Error struct {
+		Status  int    `json:"status"`
+		Message string `json:"message"`
+	}
+	ctx.JSON(status, struct {
+		Errors []Error `json:"errors"`
+	}{
+		Errors: []Error{
+			{Status: status, Message: message},
+		},
 	})
 }
 

routers/api/packages/conan/conan.go

@@ -61,10 +61,9 @@ func jsonResponse(ctx *context.Context, status int, obj any) {
 }
 
 func apiError(ctx *context.Context, status int, obj any) {
-	helper.LogAndProcessError(ctx, status, obj, func(message string) {
-		jsonResponse(ctx, status, map[string]string{
-			"message": message,
-		})
+	message := helper.ProcessErrorForUser(ctx, status, obj)
+	jsonResponse(ctx, status, map[string]string{
+		"message": message,
 	})
 }
 

routers/api/packages/conda/conda.go

@@ -25,14 +25,13 @@ import (
 )
 
 func apiError(ctx *context.Context, status int, obj any) {
-	helper.LogAndProcessError(ctx, status, obj, func(message string) {
-		ctx.JSON(status, struct {
-			Reason  string `json:"reason"`
-			Message string `json:"message"`
-		}{
-			Reason:  http.StatusText(status),
-			Message: message,
-		})
+	message := helper.ProcessErrorForUser(ctx, status, obj)
+	ctx.JSON(status, struct {
+		Reason  string `json:"reason"`
+		Message string `json:"message"`
+	}{
+		Reason:  http.StatusText(status),
+		Message: message,
 	})
 }
 

routers/api/packages/container/container.go

@@ -93,10 +93,9 @@ func jsonResponse(ctx *context.Context, status int, obj any) {
 }
 
 func apiError(ctx *context.Context, status int, err error) {
-	helper.LogAndProcessError(ctx, status, err, func(message string) {
-		setResponseHeaders(ctx.Resp, &containerHeaders{
-			Status: status,
-		})
+	_ = helper.ProcessErrorForUser(ctx, status, err)
+	setResponseHeaders(ctx.Resp, &containerHeaders{
+		Status: status,
 	})
 }
 

routers/api/packages/cran/cran.go

@@ -22,9 +22,8 @@ import (
 )
 
 func apiError(ctx *context.Context, status int, obj any) {
-	helper.LogAndProcessError(ctx, status, obj, func(message string) {
-		ctx.PlainText(status, message)
-	})
+	message := helper.ProcessErrorForUser(ctx, status, obj)
+	ctx.PlainText(status, message)
 }
 
 func EnumerateSourcePackages(ctx *context.Context) {

routers/api/packages/debian/debian.go

@@ -24,9 +24,8 @@ import (
 )
 
 func apiError(ctx *context.Context, status int, obj any) {
-	helper.LogAndProcessError(ctx, status, obj, func(message string) {
-		ctx.PlainText(status, message)
-	})
+	message := helper.ProcessErrorForUser(ctx, status, obj)
+	ctx.PlainText(status, message)
 }
 
 func GetRepositoryKey(ctx *context.Context) {

routers/api/packages/generic/generic.go

@@ -24,9 +24,8 @@ var (
 )
 
 func apiError(ctx *context.Context, status int, obj any) {
-	helper.LogAndProcessError(ctx, status, obj, func(message string) {
-		ctx.PlainText(status, message)
-	})
+	message := helper.ProcessErrorForUser(ctx, status, obj)
+	ctx.PlainText(status, message)
 }
 
 // DownloadPackageFile serves the specific generic package.

routers/api/packages/goproxy/goproxy.go

@@ -22,9 +22,8 @@ import (
 )
 
 func apiError(ctx *context.Context, status int, obj any) {
-	helper.LogAndProcessError(ctx, status, obj, func(message string) {
-		ctx.PlainText(status, message)
-	})
+	message := helper.ProcessErrorForUser(ctx, status, obj)
+	ctx.PlainText(status, message)
 }
 
 func EnumeratePackageVersions(ctx *context.Context) {

routers/api/packages/helm/helm.go

@@ -28,13 +28,12 @@ import (
 )
 
 func apiError(ctx *context.Context, status int, obj any) {
-	helper.LogAndProcessError(ctx, status, obj, func(message string) {
-		type Error struct {
-			Error string `json:"error"`
-		}
-		ctx.JSON(status, Error{
-			Error: message,
-		})
+	message := helper.ProcessErrorForUser(ctx, status, obj)
+	type Error struct {
+		Error string `json:"error"`
+	}
+	ctx.JSON(status, Error{
+		Error: message,
 	})
 }
 

routers/api/packages/helper/helper.go

@@ -15,31 +15,29 @@ import (
 	"code.gitea.io/gitea/services/context"
 )
 
-// LogAndProcessError logs an error and calls a custom callback with the processed error message.
-// If the error is an InternalServerError the message is stripped if the user is not an admin.
-func LogAndProcessError(ctx *context.Context, status int, obj any, cb func(string)) {
+// ProcessErrorForUser logs the error and returns a user-error message for the end user.
+// If the status is http.StatusInternalServerError, the message is stripped for non-admin users in production.
+func ProcessErrorForUser(ctx *context.Context, status int, errObj any) string {
 	var message string
-	if err, ok := obj.(error); ok {
+	if err, ok := errObj.(error); ok {
 		message = err.Error()
-	} else if obj != nil {
-		message = fmt.Sprintf("%s", obj)
+	} else if errObj != nil {
+		message = fmt.Sprint(errObj)
 	}
+
 	if status == http.StatusInternalServerError {
-		log.ErrorWithSkip(1, message)
-
+		log.Log(2, log.ERROR, "Package registry API internal error: %d %s", status, message)
 		if setting.IsProd && (ctx.Doer == nil || !ctx.Doer.IsAdmin) {
-			message = ""
+			message = "internal server error"
 		}
-	} else {
-		log.Debug(message)
+		return message
 	}
 
-	if cb != nil {
-		cb(message)
-	}
+	log.Log(2, log.DEBUG, "Package registry API user error: %d %s", status, message)
+	return message
 }
 
-// Serves the content of the package file
+// ServePackageFile the content of the package file
 // If the url is set it will redirect the request, otherwise the content is copied to the response.
 func ServePackageFile(ctx *context.Context, s io.ReadSeekCloser, u *url.URL, pf *packages_model.PackageFile, forceOpts ...*context.ServeHeaderOptions) {
 	if u != nil {

routers/api/packages/maven/maven.go

@@ -22,7 +22,6 @@ import (
 	packages_model "code.gitea.io/gitea/models/packages"
 	"code.gitea.io/gitea/modules/globallock"
 	"code.gitea.io/gitea/modules/json"
-	"code.gitea.io/gitea/modules/log"
 	packages_module "code.gitea.io/gitea/modules/packages"
 	maven_module "code.gitea.io/gitea/modules/packages/maven"
 	"code.gitea.io/gitea/modules/util"
@@ -49,14 +48,9 @@ var (
 )
 
 func apiError(ctx *context.Context, status int, obj any) {
-	helper.LogAndProcessError(ctx, status, obj, func(message string) {
-		// The maven client does not present the error message to the user. Log it for users with access to server logs.
-		if status == http.StatusBadRequest || status == http.StatusInternalServerError {
-			log.Error(message)
-		}
-
-		ctx.PlainText(status, message)
-	})
+	message := helper.ProcessErrorForUser(ctx, status, obj)
+	// Maven client doesn't present the error message to end users; site admin can check the server logs that outputted by ProcessErrorForUser
+	ctx.PlainText(status, message)
 }
 
 // DownloadPackageFile serves the content of a package

routers/api/packages/npm/npm.go

@@ -33,10 +33,9 @@ import (
 var errInvalidTagName = errors.New("The tag name is invalid")
 
 func apiError(ctx *context.Context, status int, obj any) {
-	helper.LogAndProcessError(ctx, status, obj, func(message string) {
-		ctx.JSON(status, map[string]string{
-			"error": message,
-		})
+	message := helper.ProcessErrorForUser(ctx, status, obj)
+	ctx.JSON(status, map[string]string{
+		"error": message,
 	})
 }
 

routers/api/packages/nuget/nuget.go

@@ -29,10 +29,9 @@ import (
 )
 
 func apiError(ctx *context.Context, status int, obj any) {
-	helper.LogAndProcessError(ctx, status, obj, func(message string) {
-		ctx.JSON(status, map[string]string{
-			"Message": message,
-		})
+	message := helper.ProcessErrorForUser(ctx, status, obj)
+	ctx.JSON(status, map[string]string{
+		"Message": message,
 	})
 }
 

routers/api/packages/pub/pub.go

@@ -43,13 +43,12 @@ func apiError(ctx *context.Context, status int, obj any) {
 		Error Error `json:"error"`
 	}
 
-	helper.LogAndProcessError(ctx, status, obj, func(message string) {
-		jsonResponse(ctx, status, ErrorWrapper{
-			Error: Error{
-				Code:    http.StatusText(status),
-				Message: message,
-			},
-		})
+	message := helper.ProcessErrorForUser(ctx, status, obj)
+	jsonResponse(ctx, status, ErrorWrapper{
+		Error: Error{
+			Code:    http.StatusText(status),
+			Message: message,
+		},
 	})
 }
 

routers/api/packages/pypi/pypi.go

@@ -40,9 +40,8 @@ var versionMatcher = regexp.MustCompile(`\Av?` +
 	`\z`)
 
 func apiError(ctx *context.Context, status int, obj any) {
-	helper.LogAndProcessError(ctx, status, obj, func(message string) {
-		ctx.PlainText(status, message)
-	})
+	message := helper.ProcessErrorForUser(ctx, status, obj)
+	ctx.PlainText(status, message)
 }
 
 // PackageMetadata returns the metadata for a single package

routers/api/packages/rpm/rpm.go

@@ -26,9 +26,8 @@ import (
 )
 
 func apiError(ctx *context.Context, status int, obj any) {
-	helper.LogAndProcessError(ctx, status, obj, func(message string) {
-		ctx.PlainText(status, message)
-	})
+	message := helper.ProcessErrorForUser(ctx, status, obj)
+	ctx.PlainText(status, message)
 }
 
 // https://dnf.readthedocs.io/en/latest/conf_ref.html

routers/api/packages/rubygems/rubygems.go

@@ -25,9 +25,8 @@ import (
 )
 
 func apiError(ctx *context.Context, status int, obj any) {
-	helper.LogAndProcessError(ctx, status, obj, func(message string) {
-		ctx.PlainText(status, message)
-	})
+	message := helper.ProcessErrorForUser(ctx, status, obj)
+	ctx.PlainText(status, message)
 }
 
 // EnumeratePackages serves the package list

routers/api/packages/swift/swift.go

@@ -77,17 +77,14 @@ func apiError(ctx *context.Context, status int, obj any) {
 		Detail string `json:"detail"`
 	}
 
-	helper.LogAndProcessError(ctx, status, obj, func(message string) {
-		setResponseHeaders(ctx.Resp, &headers{
-			Status:      status,
-			ContentType: "application/problem+json",
-		})
-		if err := json.NewEncoder(ctx.Resp).Encode(Problem{
-			Status: status,
-			Detail: message,
-		}); err != nil {
-			log.Error("JSON encode: %v", err)
-		}
+	message := helper.ProcessErrorForUser(ctx, status, obj)
+	setResponseHeaders(ctx.Resp, &headers{
+		Status:      status,
+		ContentType: "application/problem+json",
+	})
+	_ = json.NewEncoder(ctx.Resp).Encode(Problem{
+		Status: status,
+		Detail: message,
 	})
 }
 

routers/api/packages/vagrant/vagrant.go

@@ -24,14 +24,13 @@ import (
 )
 
 func apiError(ctx *context.Context, status int, obj any) {
-	helper.LogAndProcessError(ctx, status, obj, func(message string) {
-		ctx.JSON(status, struct {
-			Errors []string `json:"errors"`
-		}{
-			Errors: []string{
-				message,
-			},
-		})
+	message := helper.ProcessErrorForUser(ctx, status, obj)
+	ctx.JSON(status, struct {
+		Errors []string `json:"errors"`
+	}{
+		Errors: []string{
+			message,
+		},
 	})
 }