From 9b32d021b3d72db14ff1132f58f8e45c64da43b8 Mon Sep 17 00:00:00 2001 From: qwerty287 <80460567+qwerty287@users.noreply.github.com> Date: Sat, 15 Apr 2023 15:22:39 +0200 Subject: [PATCH] Consistent status on delete (#1703) Closes #1675 --- server/api/cron.go | 2 +- server/api/global_secret.go | 2 +- server/api/org_secret.go | 2 +- server/api/registry.go | 2 +- server/api/repo_secret.go | 2 +- server/store/datastore/agent.go | 3 +-- server/store/datastore/cron.go | 3 +-- server/store/datastore/helper.go | 11 +++++++++++ server/store/datastore/permission.go | 5 ++--- server/store/datastore/secret.go | 3 +-- server/store/datastore/server_config.go | 3 +-- server/store/datastore/task.go | 3 +-- server/store/datastore/user.go | 2 +- 13 files changed, 24 insertions(+), 19 deletions(-) diff --git a/server/api/cron.go b/server/api/cron.go index bc15389c0..323b73688 100644 --- a/server/api/cron.go +++ b/server/api/cron.go @@ -204,7 +204,7 @@ func DeleteCron(c *gin.Context) { return } if err := store.FromContext(c).CronDelete(repo, id); err != nil { - c.String(http.StatusInternalServerError, "Error deleting cron %d. %s", id, err) + handleDbGetError(c, err) return } c.String(http.StatusNoContent, "") diff --git a/server/api/global_secret.go b/server/api/global_secret.go index 500ffe86f..6822d9b29 100644 --- a/server/api/global_secret.go +++ b/server/api/global_secret.go @@ -118,7 +118,7 @@ func PatchGlobalSecret(c *gin.Context) { func DeleteGlobalSecret(c *gin.Context) { name := c.Param("secret") if err := server.Config.Services.Secrets.GlobalSecretDelete(name); err != nil { - c.String(http.StatusInternalServerError, "Error deleting global secret %q. %s", name, err) + handleDbGetError(c, err) return } c.String(http.StatusNoContent, "") diff --git a/server/api/org_secret.go b/server/api/org_secret.go index 900f5a579..2fe5ae9ae 100644 --- a/server/api/org_secret.go +++ b/server/api/org_secret.go @@ -131,7 +131,7 @@ func DeleteOrgSecret(c *gin.Context) { name = c.Param("secret") ) if err := server.Config.Services.Secrets.OrgSecretDelete(owner, name); err != nil { - c.String(http.StatusInternalServerError, "Error deleting org %q secret %q. %s", owner, name, err) + handleDbGetError(c, err) return } c.String(http.StatusNoContent, "") diff --git a/server/api/registry.go b/server/api/registry.go index 99f901d6d..0a5862ad3 100644 --- a/server/api/registry.go +++ b/server/api/registry.go @@ -135,7 +135,7 @@ func DeleteRegistry(c *gin.Context) { ) err := server.Config.Services.Registries.RegistryDelete(repo, name) if err != nil { - c.String(http.StatusInternalServerError, "Error deleting registry %q. %s", name, err) + handleDbGetError(c, err) return } c.String(http.StatusNoContent, "") diff --git a/server/api/repo_secret.go b/server/api/repo_secret.go index 9c7b88850..612d138cd 100644 --- a/server/api/repo_secret.go +++ b/server/api/repo_secret.go @@ -133,7 +133,7 @@ func DeleteSecret(c *gin.Context) { name = c.Param("secret") ) if err := server.Config.Services.Secrets.SecretDelete(repo, name); err != nil { - c.String(http.StatusInternalServerError, "Error deleting secret %q. %s", name, err) + handleDbGetError(c, err) return } c.String(http.StatusNoContent, "") diff --git a/server/store/datastore/agent.go b/server/store/datastore/agent.go index b299eb883..cd65ef27c 100644 --- a/server/store/datastore/agent.go +++ b/server/store/datastore/agent.go @@ -47,6 +47,5 @@ func (s storage) AgentUpdate(agent *model.Agent) error { } func (s storage) AgentDelete(agent *model.Agent) error { - _, err := s.engine.ID(agent.ID).Delete(new(model.Agent)) - return err + return wrapDelete(s.engine.ID(agent.ID).Delete(new(model.Agent))) } diff --git a/server/store/datastore/cron.go b/server/store/datastore/cron.go index 06dd60b50..edf4be515 100644 --- a/server/store/datastore/cron.go +++ b/server/store/datastore/cron.go @@ -47,8 +47,7 @@ func (s storage) CronUpdate(_ *model.Repo, cron *model.Cron) error { } func (s storage) CronDelete(repo *model.Repo, id int64) error { - _, err := s.engine.ID(id).Where("repo_id = ?", repo.ID).Delete(new(model.Cron)) - return err + return wrapDelete(s.engine.ID(id).Where("repo_id = ?", repo.ID).Delete(new(model.Cron))) } // CronListNextExecute returns limited number of jobs with NextExec being less or equal to the provided unix timestamp diff --git a/server/store/datastore/helper.go b/server/store/datastore/helper.go index b58d49c21..77bcfb390 100644 --- a/server/store/datastore/helper.go +++ b/server/store/datastore/helper.go @@ -28,3 +28,14 @@ func wrapGet(exist bool, err error) error { } return nil } + +// wrapDelete return error if err not nil or if requested entry do not exist +func wrapDelete(c int64, err error) error { + if err != nil { + return err + } + if c == 0 { + return types.RecordNotExist + } + return nil +} diff --git a/server/store/datastore/permission.go b/server/store/datastore/permission.go index 7b8cc8b0c..ce5dfd1f5 100644 --- a/server/store/datastore/permission.go +++ b/server/store/datastore/permission.go @@ -76,10 +76,9 @@ func (s storage) permUpsert(sess *xorm.Session, perm *model.Perm) error { } func (s storage) PermDelete(perm *model.Perm) error { - _, err := s.engine. + return wrapDelete(s.engine. Where("perm_user_id = ? AND perm_repo_id = ?", perm.UserID, perm.RepoID). - Delete(new(model.Perm)) - return err + Delete(new(model.Perm))) } func (s storage) PermFlush(user *model.User, before int64) error { diff --git a/server/store/datastore/secret.go b/server/store/datastore/secret.go index 7a412d1f7..2e54d5c5f 100644 --- a/server/store/datastore/secret.go +++ b/server/store/datastore/secret.go @@ -57,8 +57,7 @@ func (s storage) SecretUpdate(secret *model.Secret) error { } func (s storage) SecretDelete(secret *model.Secret) error { - _, err := s.engine.ID(secret.ID).Delete(new(model.Secret)) - return err + return wrapDelete(s.engine.ID(secret.ID).Delete(new(model.Secret))) } func (s storage) OrgSecretFind(owner, name string) (*model.Secret, error) { diff --git a/server/store/datastore/server_config.go b/server/store/datastore/server_config.go index b5994b57c..e96608420 100644 --- a/server/store/datastore/server_config.go +++ b/server/store/datastore/server_config.go @@ -41,6 +41,5 @@ func (s storage) ServerConfigDelete(key string) error { Key: key, } - _, err := s.engine.Delete(config) - return err + return wrapDelete(s.engine.Delete(config)) } diff --git a/server/store/datastore/task.go b/server/store/datastore/task.go index 1897e47ba..ad5525631 100644 --- a/server/store/datastore/task.go +++ b/server/store/datastore/task.go @@ -30,6 +30,5 @@ func (s storage) TaskInsert(task *model.Task) error { } func (s storage) TaskDelete(id string) error { - _, err := s.engine.Where("task_id = ?", id).Delete(new(model.Task)) - return err + return wrapDelete(s.engine.Where("task_id = ?", id).Delete(new(model.Task))) } diff --git a/server/store/datastore/user.go b/server/store/datastore/user.go index d906f6c51..dc7e98e48 100644 --- a/server/store/datastore/user.go +++ b/server/store/datastore/user.go @@ -55,7 +55,7 @@ func (s storage) DeleteUser(user *model.User) error { return err } - if _, err := sess.ID(user.ID).Delete(new(model.User)); err != nil { + if err := wrapDelete(sess.ID(user.ID).Delete(new(model.User))); err != nil { return err }