From 69c1cd3f381b19b988a6af51a8e38303f216b001 Mon Sep 17 00:00:00 2001 From: Unknwon Date: Sat, 1 Dec 2018 21:41:30 -0500 Subject: [PATCH] routes/api: change status handle to new style Also fixed one bug that did not catch team not found error. --- models/errors/org.go | 21 ++++++++++++++++++++ models/org.go | 3 +-- models/org_team.go | 7 ++++--- pkg/context/api.go | 24 ++++++++++++++++++++++- routes/api/v1/api.go | 46 +++++++++++++++----------------------------- 5 files changed, 64 insertions(+), 37 deletions(-) create mode 100644 models/errors/org.go diff --git a/models/errors/org.go b/models/errors/org.go new file mode 100644 index 000000000..565327464 --- /dev/null +++ b/models/errors/org.go @@ -0,0 +1,21 @@ +// Copyright 2018 The Gogs Authors. All rights reserved. +// Use of this source code is governed by a MIT-style +// license that can be found in the LICENSE file. + +package errors + +import "fmt" + +type TeamNotExist struct { + TeamID int64 + Name string +} + +func IsTeamNotExist(err error) bool { + _, ok := err.(TeamNotExist) + return ok +} + +func (err TeamNotExist) Error() string { + return fmt.Sprintf("team does not exist [team_id: %d, name: %s]", err.TeamID, err.Name) +} diff --git a/models/org.go b/models/org.go index f5967cea4..e3744c8a2 100644 --- a/models/org.go +++ b/models/org.go @@ -15,8 +15,7 @@ import ( ) var ( - ErrOrgNotExist = errors.New("Organization does not exist") - ErrTeamNotExist = errors.New("Team does not exist") + ErrOrgNotExist = errors.New("Organization does not exist") ) // IsOwnedBy returns true if given user is in the owner team. diff --git a/models/org_team.go b/models/org_team.go index fc06d6601..c3aba0e12 100644 --- a/models/org_team.go +++ b/models/org_team.go @@ -5,11 +5,12 @@ package models import ( - "errors" "fmt" "strings" "github.com/go-xorm/xorm" + + "github.com/gogs/gogs/models/errors" ) const OWNER_TEAM = "Owners" @@ -274,7 +275,7 @@ func getTeamOfOrgByName(e Engine, orgID int64, name string) (*Team, error) { if err != nil { return nil, err } else if !has { - return nil, ErrTeamNotExist + return nil, errors.TeamNotExist{0, name} } return t, nil } @@ -290,7 +291,7 @@ func getTeamByID(e Engine, teamID int64) (*Team, error) { if err != nil { return nil, err } else if !has { - return nil, ErrTeamNotExist + return nil, errors.TeamNotExist{teamID, ""} } return t, nil } diff --git a/pkg/context/api.go b/pkg/context/api.go index fab660241..7df5b23aa 100644 --- a/pkg/context/api.go +++ b/pkg/context/api.go @@ -6,6 +6,7 @@ package context import ( "fmt" + "net/http" "strings" "github.com/Unknwon/paginater" @@ -33,7 +34,7 @@ func (c *APIContext) Error(status int, title string, obj interface{}) { message = obj.(string) } - if status == 500 { + if status == http.StatusInternalServerError { log.Error(3, "%s: %s", title, message) } @@ -43,6 +44,27 @@ func (c *APIContext) Error(status int, title string, obj interface{}) { }) } +// NotFound renders the 404 response. +func (c *APIContext) NotFound() { + c.Status(http.StatusNotFound) +} + +// ServerError renders the 500 response. +func (c *APIContext) ServerError(title string, err error) { + c.Error(http.StatusInternalServerError, title, err) +} + +// NotFoundOrServerError use error check function to determine if the error +// is about not found. It responses with 404 status code for not found error, +// or error context description for logging purpose of 500 server error. +func (c *APIContext) NotFoundOrServerError(title string, errck func(error) bool, err error) { + if errck(err) { + c.NotFound() + return + } + c.ServerError(title, err) +} + // SetLinkHeader sets pagination link header by given total number and page size. func (c *APIContext) SetLinkHeader(total, pageSize int) { page := paginater.New(total, pageSize, c.QueryInt("page"), 0) diff --git a/routes/api/v1/api.go b/routes/api/v1/api.go index 2b4644d4d..3ac5ef98c 100644 --- a/routes/api/v1/api.go +++ b/routes/api/v1/api.go @@ -5,6 +5,7 @@ package v1 import ( + "net/http" "strings" "github.com/go-macaron/binding" @@ -39,43 +40,34 @@ func repoAssignment() macaron.Handler { } else { owner, err = models.GetUserByName(userName) if err != nil { - if errors.IsUserNotExist(err) { - c.Status(404) - } else { - c.Error(500, "GetUserByName", err) - } + c.NotFoundOrServerError("GetUserByName", errors.IsUserNotExist, err) return } } c.Repo.Owner = owner - // Get repository. repo, err := models.GetRepositoryByName(owner.ID, repoName) if err != nil { - if errors.IsRepoNotExist(err) { - c.Status(404) - } else { - c.Error(500, "GetRepositoryByName", err) - } + c.NotFoundOrServerError("GetRepositoryByName", errors.IsRepoNotExist, err) return } else if err = repo.GetOwner(); err != nil { - c.Error(500, "GetOwner", err) + c.ServerError("GetOwner", err) return } if c.IsTokenAuth && c.User.IsAdmin { c.Repo.AccessMode = models.ACCESS_MODE_OWNER } else { - mode, err := models.AccessLevel(c.User.ID, repo) + mode, err := models.AccessLevel(c.UserID(), repo) if err != nil { - c.Error(500, "AccessLevel", err) + c.ServerError("AccessLevel", err) return } c.Repo.AccessMode = mode } if !c.Repo.HasAccess() { - c.Status(404) + c.NotFound() return } @@ -87,7 +79,7 @@ func repoAssignment() macaron.Handler { func reqToken() macaron.Handler { return func(c *context.Context) { if !c.IsTokenAuth { - c.Error(401) + c.Error(http.StatusUnauthorized) return } } @@ -96,7 +88,7 @@ func reqToken() macaron.Handler { func reqBasicAuth() macaron.Handler { return func(c *context.Context) { if !c.IsBasicAuth { - c.Error(401) + c.Error(http.StatusUnauthorized) return } } @@ -105,7 +97,7 @@ func reqBasicAuth() macaron.Handler { func reqAdmin() macaron.Handler { return func(c *context.Context) { if !c.IsLogged || !c.User.IsAdmin { - c.Error(403) + c.Error(http.StatusForbidden) return } } @@ -114,7 +106,7 @@ func reqAdmin() macaron.Handler { func reqRepoWriter() macaron.Handler { return func(c *context.Context) { if !c.Repo.IsWriter() { - c.Error(403) + c.Error(http.StatusForbidden) return } } @@ -138,11 +130,7 @@ func orgAssignment(args ...bool) macaron.Handler { if assignOrg { c.Org.Organization, err = models.GetUserByName(c.Params(":orgname")) if err != nil { - if errors.IsUserNotExist(err) { - c.Status(404) - } else { - c.Error(500, "GetUserByName", err) - } + c.NotFoundOrServerError("GetUserByName", errors.IsUserNotExist, err) return } } @@ -150,11 +138,7 @@ func orgAssignment(args ...bool) macaron.Handler { if assignTeam { c.Org.Team, err = models.GetTeamByID(c.ParamsInt64(":teamid")) if err != nil { - if errors.IsUserNotExist(err) { - c.Status(404) - } else { - c.Error(500, "GetTeamById", err) - } + c.NotFoundOrServerError("GetTeamByID", errors.IsTeamNotExist, err) return } } @@ -163,7 +147,7 @@ func orgAssignment(args ...bool) macaron.Handler { func mustEnableIssues(c *context.APIContext) { if !c.Repo.Repository.EnableIssues || c.Repo.Repository.EnableExternalTracker { - c.Status(404) + c.NotFound() return } } @@ -326,7 +310,7 @@ func RegisterRoutes(m *macaron.Macaron) { }, orgAssignment(true)) m.Any("/*", func(c *context.Context) { - c.Error(404) + c.NotFound() }) m.Group("/admin", func() {