Various fixes and improvements (#1643)

- allow repo names to be case-insensitive - improve backend error handling on DB get errors (record not found -> 404, else -> 500) - replace magic numbers of http response codes - unify the look and feel of cancel / save buttons on forms and view them in one line --------- Co-authored-by: Lauris BH <lauris@nix.lv>
woodpecker-ci · Mar 19, 2023 · ade8e6d · ade8e6d
1 parent 42a115e
commit ade8e6d
Show file tree

Hide file tree

Showing 27 changed files with 230 additions and 179 deletions.
diff --git a/server/api/agent.go b/server/api/agent.go
@@ -21,7 +21,6 @@ import (
 
 	"github.com/gin-gonic/gin"
 	"github.com/gorilla/securecookie"
-
 	"github.com/woodpecker-ci/woodpecker/server/model"
 	"github.com/woodpecker-ci/woodpecker/server/router/middleware/session"
 	"github.com/woodpecker-ci/woodpecker/server/store"
@@ -45,7 +44,7 @@ func GetAgent(c *gin.Context) {
 
 	agent, err := store.FromContext(c).AgentFind(agentID)
 	if err != nil {
-		c.String(http.StatusNotFound, "Cannot find agent. %s", err)
+		handleDbGetError(c, err)
 		return
 	}
 	c.JSON(http.StatusOK, agent)
@@ -69,7 +68,7 @@ func PatchAgent(c *gin.Context) {
 
 	agent, err := _store.AgentFind(agentID)
 	if err != nil {
-		c.AbortWithStatus(http.StatusNotFound)
+		handleDbGetError(c, err)
 		return
 	}
 	agent.Name = in.Name
@@ -121,12 +120,12 @@ func DeleteAgent(c *gin.Context) {
 
 	agent, err := _store.AgentFind(agentID)
 	if err != nil {
-		c.String(http.StatusNotFound, "Cannot find user. %s", err)
+		handleDbGetError(c, err)
 		return
 	}
 	if err = _store.AgentDelete(agent); err != nil {
 		c.String(http.StatusInternalServerError, "Error deleting user. %s", err)
 		return
 	}
-	c.String(http.StatusOK, "")
+	c.String(http.StatusNoContent, "")
 }
diff --git a/server/api/badge.go b/server/api/badge.go
@@ -19,10 +19,12 @@
 package api
 
 import (
+	"errors"
 	"fmt"
 	"net/http"
 
 	"github.com/rs/zerolog/log"
+	"github.com/woodpecker-ci/woodpecker/server/store/types"
 
 	"github.com/gin-gonic/gin"
 
@@ -36,7 +38,11 @@ func GetBadge(c *gin.Context) {
 	_store := store.FromContext(c)
 	repo, err := _store.GetRepoName(c.Param("owner") + "/" + c.Param("name"))
 	if err != nil || !repo.IsActive {
-		c.AbortWithStatus(404)
+		if err == nil || errors.Is(err, types.RecordNotExist) {
+			c.AbortWithStatus(http.StatusNotFound)
+			return
+		}
+		_ = c.AbortWithError(http.StatusInternalServerError, err)
 		return
 	}
 
@@ -63,17 +69,17 @@ func GetCC(c *gin.Context) {
 	_store := store.FromContext(c)
 	repo, err := _store.GetRepoName(c.Param("owner") + "/" + c.Param("name"))
 	if err != nil {
-		c.AbortWithStatus(404)
+		handleDbGetError(c, err)
 		return
 	}
 
 	pipelines, err := _store.GetPipelineList(repo, 1)
 	if err != nil || len(pipelines) == 0 {
-		c.AbortWithStatus(404)
+		c.AbortWithStatus(http.StatusNotFound)
 		return
 	}
 
 	url := fmt.Sprintf("%s/%s/%d", server.Config.Server.Host, repo.FullName, pipelines[0].Number)
 	cc := ccmenu.New(repo, pipelines[0], url)
-	c.XML(200, cc)
+	c.XML(http.StatusOK, cc)
 }
diff --git a/server/api/cron.go b/server/api/cron.go
@@ -34,16 +34,16 @@ func GetCron(c *gin.Context) {
 	repo := session.Repo(c)
 	id, err := strconv.ParseInt(c.Param("cron"), 10, 64)
 	if err != nil {
-		c.String(400, "Error parsing cron id. %s", err)
+		c.String(http.StatusBadRequest, "Error parsing cron id. %s", err)
 		return
 	}
 
 	cron, err := store.FromContext(c).CronFind(repo, id)
 	if err != nil {
-		c.String(404, "Error getting cron %q. %s", id, err)
+		handleDbGetError(c, err)
 		return
 	}
-	c.JSON(200, cron)
+	c.JSON(http.StatusOK, cron)
 }
 
 // RunCron starts a cron job now.
@@ -52,13 +52,13 @@ func RunCron(c *gin.Context) {
 	_store := store.FromContext(c)
 	id, err := strconv.ParseInt(c.Param("cron"), 10, 64)
 	if err != nil {
-		c.String(400, "Error parsing cron id. %s", err)
+		c.String(http.StatusBadRequest, "Error parsing cron id. %s", err)
 		return
 	}
 
 	cron, err := _store.CronFind(repo, id)
 	if err != nil {
-		c.String(http.StatusNotFound, "Error getting cron %q. %s", id, err)
+		handleDbGetError(c, err)
 		return
 	}
 
@@ -74,14 +74,14 @@ func RunCron(c *gin.Context) {
 		return
 	}
 
-	c.JSON(200, pl)
+	c.JSON(http.StatusOK, pl)
 }
 
 // PostCron persists the cron job to the database.
 func PostCron(c *gin.Context) {
 	repo := session.Repo(c)
 	user := session.User(c)
-	store := store.FromContext(c)
+	_store := store.FromContext(c)
 	forge := server.Config.Services.Forge
 
 	in := new(model.Cron)
@@ -97,13 +97,13 @@ func PostCron(c *gin.Context) {
 		Branch:    in.Branch,
 	}
 	if err := cron.Validate(); err != nil {
-		c.String(400, "Error inserting cron. validate failed: %s", err)
+		c.String(http.StatusUnprocessableEntity, "Error inserting cron. validate failed: %s", err)
 		return
 	}
 
 	nextExec, err := cronScheduler.CalcNewNext(in.Schedule, time.Now())
 	if err != nil {
-		c.String(400, "Error inserting cron. schedule could not parsed: %s", err)
+		c.String(http.StatusBadRequest, "Error inserting cron. schedule could not parsed: %s", err)
 		return
 	}
 	cron.NextExec = nextExec.Unix()
@@ -112,28 +112,28 @@ func PostCron(c *gin.Context) {
 		// check if branch exists on forge
 		_, err := forge.BranchHead(c, user, repo, in.Branch)
 		if err != nil {
-			c.String(400, "Error inserting cron. branch not resolved: %s", err)
+			c.String(http.StatusBadRequest, "Error inserting cron. branch not resolved: %s", err)
 			return
 		}
 	}
 
-	if err := store.CronCreate(cron); err != nil {
-		c.String(500, "Error inserting cron %q. %s", in.Name, err)
+	if err := _store.CronCreate(cron); err != nil {
+		c.String(http.StatusInternalServerError, "Error inserting cron %q. %s", in.Name, err)
 		return
 	}
-	c.JSON(200, cron)
+	c.JSON(http.StatusOK, cron)
 }
 
 // PatchCron updates the cron job in the database.
 func PatchCron(c *gin.Context) {
 	repo := session.Repo(c)
 	user := session.User(c)
-	store := store.FromContext(c)
+	_store := store.FromContext(c)
 	forge := server.Config.Services.Forge
 
 	id, err := strconv.ParseInt(c.Param("cron"), 10, 64)
 	if err != nil {
-		c.String(400, "Error parsing cron id. %s", err)
+		c.String(http.StatusBadRequest, "Error parsing cron id. %s", err)
 		return
 	}
 
@@ -144,16 +144,16 @@ func PatchCron(c *gin.Context) {
 		return
 	}
 
-	cron, err := store.CronFind(repo, id)
+	cron, err := _store.CronFind(repo, id)
 	if err != nil {
-		c.String(404, "Error getting cron %d. %s", id, err)
+		handleDbGetError(c, err)
 		return
 	}
 	if in.Branch != "" {
 		// check if branch exists on forge
 		_, err := forge.BranchHead(c, user, repo, in.Branch)
 		if err != nil {
-			c.String(400, "Error inserting cron. branch not resolved: %s", err)
+			c.String(http.StatusBadRequest, "Error inserting cron. branch not resolved: %s", err)
 			return
 		}
 		cron.Branch = in.Branch
@@ -162,7 +162,7 @@ func PatchCron(c *gin.Context) {
 		cron.Schedule = in.Schedule
 		nextExec, err := cronScheduler.CalcNewNext(in.Schedule, time.Now())
 		if err != nil {
-			c.String(400, "Error inserting cron. schedule could not parsed: %s", err)
+			c.String(http.StatusBadRequest, "Error inserting cron. schedule could not parsed: %s", err)
 			return
 		}
 		cron.NextExec = nextExec.Unix()
@@ -173,14 +173,14 @@ func PatchCron(c *gin.Context) {
 	cron.CreatorID = user.ID
 
 	if err := cron.Validate(); err != nil {
-		c.String(400, "Error inserting cron. validate failed: %s", err)
+		c.String(http.StatusUnprocessableEntity, "Error inserting cron. validate failed: %s", err)
 		return
 	}
-	if err := store.CronUpdate(repo, cron); err != nil {
-		c.String(500, "Error updating cron %q. %s", in.Name, err)
+	if err := _store.CronUpdate(repo, cron); err != nil {
+		c.String(http.StatusInternalServerError, "Error updating cron %q. %s", in.Name, err)
 		return
 	}
-	c.JSON(200, cron)
+	c.JSON(http.StatusOK, cron)
 }
 
 // GetCronList gets the cron job list from the database and writes
@@ -189,23 +189,23 @@ func GetCronList(c *gin.Context) {
 	repo := session.Repo(c)
 	list, err := store.FromContext(c).CronList(repo)
 	if err != nil {
-		c.String(500, "Error getting cron list. %s", err)
+		c.String(http.StatusInternalServerError, "Error getting cron list. %s", err)
 		return
 	}
-	c.JSON(200, list)
+	c.JSON(http.StatusOK, list)
 }
 
 // DeleteCron deletes the named cron job from the database.
 func DeleteCron(c *gin.Context) {
 	repo := session.Repo(c)
 	id, err := strconv.ParseInt(c.Param("cron"), 10, 64)
 	if err != nil {
-		c.String(400, "Error parsing cron id. %s", err)
+		c.String(http.StatusBadRequest, "Error parsing cron id. %s", err)
 		return
 	}
 	if err := store.FromContext(c).CronDelete(repo, id); err != nil {
-		c.String(500, "Error deleting cron %d. %s", id, err)
+		c.String(http.StatusInternalServerError, "Error deleting cron %d. %s", id, err)
 		return
 	}
-	c.String(204, "")
+	c.String(http.StatusNoContent, "")
 }
diff --git a/server/api/file.go b/server/api/file.go
@@ -23,7 +23,6 @@ import (
 
 	"github.com/gin-gonic/gin"
 	"github.com/rs/zerolog/log"
-
 	"github.com/woodpecker-ci/woodpecker/server/router/middleware/session"
 	"github.com/woodpecker-ci/woodpecker/server/store"
 )
@@ -40,7 +39,7 @@ func FileList(c *gin.Context) {
 	repo := session.Repo(c)
 	pipeline, err := _store.GetPipelineNumber(repo, num)
 	if err != nil {
-		_ = c.AbortWithError(http.StatusInternalServerError, err)
+		handleDbGetError(c, err)
 		return
 	}
 
@@ -79,7 +78,7 @@ func FileGet(c *gin.Context) {
 
 	pipeline, err := _store.GetPipelineNumber(repo, num)
 	if err != nil {
-		_ = c.AbortWithError(http.StatusInternalServerError, err)
+		handleDbGetError(c, err)
 		return
 	}
 
@@ -91,18 +90,18 @@ func FileGet(c *gin.Context) {
 
 	file, err := _store.FileFind(step, name)
 	if err != nil {
-		c.String(404, "Error getting file %q. %s", name, err)
+		c.String(http.StatusNotFound, "Error getting file %q. %s", name, err)
 		return
 	}
 
 	if !raw {
-		c.JSON(200, file)
+		c.JSON(http.StatusOK, file)
 		return
 	}
 
 	rc, err := _store.FileRead(step, file.Name)
 	if err != nil {
-		c.String(404, "Error getting file stream %q. %s", name, err)
+		c.String(http.StatusNotFound, "Error getting file stream %q. %s", name, err)
 		return
 	}
 	defer rc.Close()

diff --git a/server/api/global_secret.go b/server/api/global_secret.go
@@ -17,10 +17,9 @@ package api
 import (
 	"net/http"
 
+	"github.com/gin-gonic/gin"
 	"github.com/woodpecker-ci/woodpecker/server"
 	"github.com/woodpecker-ci/woodpecker/server/model"
-
-	"github.com/gin-gonic/gin"
 )
 
 // GetGlobalSecretList gets the global secret list from
@@ -45,10 +44,10 @@ func GetGlobalSecret(c *gin.Context) {
 	name := c.Param("secret")
 	secret, err := server.Config.Services.Secrets.GlobalSecretFind(name)
 	if err != nil {
-		c.String(404, "Error getting global secret %q. %s", name, err)
+		handleDbGetError(c, err)
 		return
 	}
-	c.JSON(200, secret.Copy())
+	c.JSON(http.StatusOK, secret.Copy())
 }
 
 // PostGlobalSecret persists a global secret to the database.
@@ -66,14 +65,14 @@ func PostGlobalSecret(c *gin.Context) {
 		PluginsOnly: in.PluginsOnly,
 	}
 	if err := secret.Validate(); err != nil {
-		c.String(400, "Error inserting global secret. %s", err)
+		c.String(http.StatusBadRequest, "Error inserting global secret. %s", err)
 		return
 	}
 	if err := server.Config.Services.Secrets.GlobalSecretCreate(secret); err != nil {
-		c.String(500, "Error inserting global secret %q. %s", in.Name, err)
+		c.String(http.StatusInternalServerError, "Error inserting global secret %q. %s", in.Name, err)
 		return
 	}
-	c.JSON(200, secret.Copy())
+	c.JSON(http.StatusOK, secret.Copy())
 }
 
 // PatchGlobalSecret updates a global secret in the database.
@@ -89,7 +88,7 @@ func PatchGlobalSecret(c *gin.Context) {
 
 	secret, err := server.Config.Services.Secrets.GlobalSecretFind(name)
 	if err != nil {
-		c.String(404, "Error getting global secret %q. %s", name, err)
+		handleDbGetError(c, err)
 		return
 	}
 	if in.Value != "" {
@@ -104,22 +103,22 @@ func PatchGlobalSecret(c *gin.Context) {
 	secret.PluginsOnly = in.PluginsOnly
 
 	if err := secret.Validate(); err != nil {
-		c.String(400, "Error updating global secret. %s", err)
+		c.String(http.StatusBadRequest, "Error updating global secret. %s", err)
 		return
 	}
 	if err := server.Config.Services.Secrets.GlobalSecretUpdate(secret); err != nil {
-		c.String(500, "Error updating global secret %q. %s", in.Name, err)
+		c.String(http.StatusInternalServerError, "Error updating global secret %q. %s", in.Name, err)
 		return
 	}
-	c.JSON(200, secret.Copy())
+	c.JSON(http.StatusOK, secret.Copy())
 }
 
 // DeleteGlobalSecret deletes the named global secret from the database.
 func DeleteGlobalSecret(c *gin.Context) {
 	name := c.Param("secret")
 	if err := server.Config.Services.Secrets.GlobalSecretDelete(name); err != nil {
-		c.String(500, "Error deleting global secret %q. %s", name, err)
+		c.String(http.StatusInternalServerError, "Error deleting global secret %q. %s", name, err)
 		return
 	}
-	c.String(204, "")
+	c.String(http.StatusNoContent, "")
 }