Use API error helpers and improve response codes (#2366)

woodpecker-ci · Sep 2, 2023 · 3e563ef · 3e563ef
1 parent 3bee51d
commit 3e563ef
Show file tree

Hide file tree

Showing 25 changed files with 117 additions and 156 deletions.
diff --git a/cmd/server/docs/docs.go b/cmd/server/docs/docs.go
@@ -605,7 +605,7 @@ const docTemplate = `{
         },
         "/healthz": {
             "get": {
-                "description": "If everything is fine, just a 200 will be returned, a 500 signals server state is unhealthy.",
+                "description": "If everything is fine, just a 204 will be returned, a 500 signals server state is unhealthy.",
                 "produces": [
                     "text/plain"
                 ],
@@ -614,8 +614,8 @@ const docTemplate = `{
                 ],
                 "summary": "Health information",
                 "responses": {
-                    "200": {
-                        "description": "OK"
+                    "204": {
+                        "description": "No Content"
                     },
                     "500": {
                         "description": "Internal Server Error"
@@ -869,7 +869,7 @@ const docTemplate = `{
             "delete": {
                 "description": "Deletes the given org. Requires admin rights.",
                 "produces": [
-                    "application/json"
+                    "text/plain"
                 ],
                 "tags": [
                     "Orgs"
@@ -894,10 +894,7 @@ const docTemplate = `{
                 ],
                 "responses": {
                     "204": {
-                        "description": "No Content",
-                        "schema": {
-                            "$ref": "#/definitions/Org"
-                        }
+                        "description": "No Content"
                     }
                 }
             }
@@ -1838,8 +1835,8 @@ const docTemplate = `{
                     }
                 ],
                 "responses": {
-                    "200": {
-                        "description": "OK"
+                    "204": {
+                        "description": "No Content"
                     }
                 }
             },
@@ -1928,8 +1925,8 @@ const docTemplate = `{
                     }
                 ],
                 "responses": {
-                    "200": {
-                        "description": "OK"
+                    "204": {
+                        "description": "No Content"
                     }
                 }
             }
@@ -2667,8 +2664,8 @@ const docTemplate = `{
                     }
                 ],
                 "responses": {
-                    "200": {
-                        "description": "OK"
+                    "204": {
+                        "description": "No Content"
                     }
                 }
             },
@@ -3307,7 +3304,7 @@ const docTemplate = `{
                 "tags": [
                     "User"
                 ],
-                "summary": "Return the token of the current user as stringª",
+                "summary": "Return the token of the current user as string",
                 "parameters": [
                     {
                         "type": "string",
@@ -3473,7 +3470,7 @@ const docTemplate = `{
             "delete": {
                 "description": "Deletes the given user. Requires admin rights.",
                 "produces": [
-                    "application/json"
+                    "text/plain"
                 ],
                 "tags": [
                     "Users"
@@ -3497,11 +3494,8 @@ const docTemplate = `{
                     }
                 ],
                 "responses": {
-                    "200": {
-                        "description": "OK",
-                        "schema": {
-                            "$ref": "#/definitions/User"
-                        }
+                    "204": {
+                        "description": "No Content"
                     }
                 }
             },

diff --git a/server/api/agent.go b/server/api/agent.go
@@ -65,7 +65,7 @@ func GetAgent(c *gin.Context) {
 
 	agent, err := store.FromContext(c).AgentFind(agentID)
 	if err != nil {
-		handleDbGetError(c, err)
+		handleDbError(c, err)
 		return
 	}
 	c.JSON(http.StatusOK, agent)
@@ -89,7 +89,7 @@ func GetAgentTasks(c *gin.Context) {
 
 	agent, err := store.FromContext(c).AgentFind(agentID)
 	if err != nil {
-		c.String(http.StatusNotFound, "Cannot find agent. %s", err)
+		handleDbError(c, err)
 		return
 	}
 
@@ -132,7 +132,7 @@ func PatchAgent(c *gin.Context) {
 
 	agent, err := _store.AgentFind(agentID)
 	if err != nil {
-		handleDbGetError(c, err)
+		handleDbError(c, err)
 		return
 	}
 	agent.Name = in.Name
@@ -201,12 +201,12 @@ func DeleteAgent(c *gin.Context) {
 
 	agent, err := _store.AgentFind(agentID)
 	if err != nil {
-		handleDbGetError(c, err)
+		handleDbError(c, err)
 		return
 	}
 	if err = _store.AgentDelete(agent); err != nil {
 		c.String(http.StatusInternalServerError, "Error deleting user. %s", err)
 		return
 	}
-	c.String(http.StatusNoContent, "")
+	c.Status(http.StatusNoContent)
 }
diff --git a/server/api/badge.go b/server/api/badge.go
@@ -62,11 +62,7 @@ func GetBadge(c *gin.Context) {
 	}
 
 	if err != nil {
-		if errors.Is(err, types.RecordNotExist) {
-			c.AbortWithStatus(http.StatusNotFound)
-			return
-		}
-		_ = c.AbortWithError(http.StatusInternalServerError, err)
+		handleDbError(c, err)
 		return
 	}
 
@@ -107,7 +103,7 @@ func GetCC(c *gin.Context) {
 	_store := store.FromContext(c)
 	repo, err := _store.GetRepoName(c.Param("owner") + "/" + c.Param("name"))
 	if err != nil {
-		handleDbGetError(c, err)
+		handleDbError(c, err)
 		return
 	}
 

diff --git a/server/api/cron.go b/server/api/cron.go
@@ -20,6 +20,7 @@ import (
 	"time"
 
 	"github.com/gin-gonic/gin"
+
 	"github.com/woodpecker-ci/woodpecker/server"
 	cronScheduler "github.com/woodpecker-ci/woodpecker/server/cron"
 	"github.com/woodpecker-ci/woodpecker/server/model"
@@ -48,7 +49,7 @@ func GetCron(c *gin.Context) {
 
 	cron, err := store.FromContext(c).CronFind(repo, id)
 	if err != nil {
-		handleDbGetError(c, err)
+		handleDbError(c, err)
 		return
 	}
 	c.JSON(http.StatusOK, cron)
@@ -75,7 +76,7 @@ func RunCron(c *gin.Context) {
 
 	cron, err := _store.CronFind(repo, id)
 	if err != nil {
-		handleDbGetError(c, err)
+		handleDbError(c, err)
 		return
 	}
 
@@ -182,7 +183,7 @@ func PatchCron(c *gin.Context) {
 
 	cron, err := _store.CronFind(repo, id)
 	if err != nil {
-		handleDbGetError(c, err)
+		handleDbError(c, err)
 		return
 	}
 	if in.Branch != "" {
@@ -245,7 +246,7 @@ func GetCronList(c *gin.Context) {
 //	@Summary	Delete a cron job by id
 //	@Router		/repos/{repo_id}/cron/{cron} [delete]
 //	@Produce	plain
-//	@Success	200
+//	@Success	204
 //	@Tags		Repository cron jobs
 //	@Param		Authorization	header	string	true	"Insert your personal access token"	default(Bearer <personal access token>)
 //	@Param		repo_id			path	int		true	"the repository id"
@@ -258,8 +259,8 @@ func DeleteCron(c *gin.Context) {
 		return
 	}
 	if err := store.FromContext(c).CronDelete(repo, id); err != nil {
-		handleDbGetError(c, err)
+		handleDbError(c, err)
 		return
 	}
-	c.String(http.StatusNoContent, "")
+	c.Status(http.StatusNoContent)
 }
diff --git a/server/api/global_secret.go b/server/api/global_secret.go
@@ -18,6 +18,7 @@ import (
 	"net/http"
 
 	"github.com/gin-gonic/gin"
+
 	"github.com/woodpecker-ci/woodpecker/server/router/middleware/session"
 
 	"github.com/woodpecker-ci/woodpecker/server"
@@ -61,7 +62,7 @@ func GetGlobalSecret(c *gin.Context) {
 	name := c.Param("secret")
 	secret, err := server.Config.Services.Secrets.GlobalSecretFind(name)
 	if err != nil {
-		handleDbGetError(c, err)
+		handleDbError(c, err)
 		return
 	}
 	c.JSON(http.StatusOK, secret.Copy())
@@ -122,7 +123,7 @@ func PatchGlobalSecret(c *gin.Context) {
 
 	secret, err := server.Config.Services.Secrets.GlobalSecretFind(name)
 	if err != nil {
-		handleDbGetError(c, err)
+		handleDbError(c, err)
 		return
 	}
 	if in.Value != "" {
@@ -159,8 +160,8 @@ func PatchGlobalSecret(c *gin.Context) {
 func DeleteGlobalSecret(c *gin.Context) {
 	name := c.Param("secret")
 	if err := server.Config.Services.Secrets.GlobalSecretDelete(name); err != nil {
-		handleDbGetError(c, err)
+		handleDbError(c, err)
 		return
 	}
-	c.String(http.StatusNoContent, "")
+	c.Status(http.StatusNoContent)
 }
diff --git a/server/api/helper.go b/server/api/helper.go
@@ -35,13 +35,13 @@ func handlePipelineErr(c *gin.Context, err error) {
 	} else if errors.Is(err, &pipeline.ErrBadRequest{}) {
 		c.String(http.StatusBadRequest, "%s", err)
 	} else if errors.Is(err, &pipeline.ErrFiltered{}) {
-		c.String(http.StatusNoContent, "%s", err)
+		c.Status(http.StatusNoContent)
 	} else {
 		_ = c.AbortWithError(http.StatusInternalServerError, err)
 	}
 }
 
-func handleDbGetError(c *gin.Context, err error) {
+func handleDbError(c *gin.Context, err error) {
 	if errors.Is(err, types.RecordNotExist) {
 		c.AbortWithStatus(http.StatusNotFound)
 		return

diff --git a/server/api/hook.go b/server/api/hook.go
@@ -143,23 +143,20 @@ func PostHook(c *gin.Context) {
 
 	repo, err := _store.GetRepoNameFallback(tmpRepo.ForgeRemoteID, tmpRepo.FullName)
 	if err != nil {
-		msg := fmt.Sprintf("failure to get repo %s from store", tmpRepo.FullName)
-		log.Error().Err(err).Msg(msg)
-		c.String(http.StatusNotFound, msg)
+		log.Error().Err(err).Msgf("failure to get repo %s from store", tmpRepo.FullName)
+		handleDbError(c, err)
 		return
 	}
 	if !repo.IsActive {
-		msg := fmt.Sprintf("ignoring hook: repo %s is inactive", tmpRepo.FullName)
-		log.Debug().Msg(msg)
-		c.String(http.StatusNoContent, msg)
+		log.Debug().Msgf("ignoring hook: repo %s is inactive", tmpRepo.FullName)
+		c.Status(http.StatusNoContent)
 		return
 	}
 	oldFullName := repo.FullName
 
 	if repo.UserID == 0 {
-		msg := fmt.Sprintf("ignoring hook. repo %s has no owner.", repo.FullName)
-		log.Warn().Msg(msg)
-		c.String(http.StatusNoContent, msg)
+		log.Warn().Msgf("ignoring hook. repo %s has no owner.", repo.FullName)
+		c.Status(http.StatusNoContent)
 		return
 	}
 
@@ -220,9 +217,8 @@ func PostHook(c *gin.Context) {
 	//
 
 	if tmpPipeline.Event == model.EventPull && !repo.AllowPull {
-		msg := "ignoring hook: pull requests are disabled for this repo in woodpecker"
-		log.Debug().Str("repo", repo.FullName).Msg(msg)
-		c.String(http.StatusNoContent, msg)
+		log.Debug().Str("repo", repo.FullName).Msg("ignoring hook: pull requests are disabled for this repo in woodpecker")
+		c.Status(http.StatusNoContent)
 		return
 	}
 

diff --git a/server/api/login.go b/server/api/login.go
@@ -210,7 +210,7 @@ func GetLoginToken(c *gin.Context) {
 
 	user, err := _store.GetUserLogin(login)
 	if err != nil {
-		_ = c.AbortWithError(http.StatusNotFound, err)
+		handleDbError(c, err)
 		return
 	}
 

diff --git a/server/api/metrics/prometheus.go b/server/api/metrics/prometheus.go
@@ -43,14 +43,14 @@ func PromHandler() gin.HandlerFunc {
 		header := c.Request.Header.Get("Authorization")
 
 		if header == "" {
-			c.String(401, errInvalidToken.Error())
+			c.String(http.StatusUnauthorized, errInvalidToken.Error())
 			return
 		}
 
 		bearer := fmt.Sprintf("Bearer %s", token)
 
 		if header != bearer {
-			c.String(401, errInvalidToken.Error())
+			c.String(http.StatusForbidden, errInvalidToken.Error())
 			return
 		}