[API] Only Return Json (#13511)

* Let Branch and Raw Endpoint return json error if not found

* Revert "RM RepoRefByTypeForAPI and move needed parts into GetRawFile directly"

This reverts commit d826d08577b23765cb3c257e7a861191d1aa9a04.

* more similar to RepoRefByType

* dedub-code

* API should just speak JSON

* nice name

Co-authored-by: zeripath <art27@cantab.net>

diff --git a/modules/context/api.go b/modules/context/api.go
index 9dad588c7fefee3feb40f493d72af96c45bf3a9f..b5f521f63c9ab33341860647172d2e964b9a0418 100644 (file)
--- a/modules/context/api.go
+++ b/modules/context/api.go
@@ -259,3 +259,61 @@ func (ctx *APIContext) NotFound(objs ...interface{}) {
                "errors":            errors,
        })
 }
+
+// RepoRefForAPI handles repository reference names when the ref name is not explicitly given
+func RepoRefForAPI() macaron.Handler {
+       return func(ctx *APIContext) {
+               // Empty repository does not have reference information.
+               if ctx.Repo.Repository.IsEmpty {
+                       return
+               }
+
+               var err error
+
+               if ctx.Repo.GitRepo == nil {
+                       repoPath := models.RepoPath(ctx.Repo.Owner.Name, ctx.Repo.Repository.Name)
+                       ctx.Repo.GitRepo, err = git.OpenRepository(repoPath)
+                       if err != nil {
+                               ctx.InternalServerError(err)
+                               return
+                       }
+                       // We opened it, we should close it
+                       defer func() {
+                               // If it's been set to nil then assume someone else has closed it.
+                               if ctx.Repo.GitRepo != nil {
+                                       ctx.Repo.GitRepo.Close()
+                               }
+                       }()
+               }
+
+               refName := getRefName(ctx.Context, RepoRefAny)
+
+               if ctx.Repo.GitRepo.IsBranchExist(refName) {
+                       ctx.Repo.Commit, err = ctx.Repo.GitRepo.GetBranchCommit(refName)
+                       if err != nil {
+                               ctx.InternalServerError(err)
+                               return
+                       }
+                       ctx.Repo.CommitID = ctx.Repo.Commit.ID.String()
+               } else if ctx.Repo.GitRepo.IsTagExist(refName) {
+                       ctx.Repo.Commit, err = ctx.Repo.GitRepo.GetTagCommit(refName)
+                       if err != nil {
+                               ctx.InternalServerError(err)
+                               return
+                       }
+                       ctx.Repo.CommitID = ctx.Repo.Commit.ID.String()
+               } else if len(refName) == 40 {
+                       ctx.Repo.CommitID = refName
+                       ctx.Repo.Commit, err = ctx.Repo.GitRepo.GetCommit(refName)
+                       if err != nil {
+                               ctx.NotFound("GetCommit", err)
+                               return
+                       }
+               } else {
+                       ctx.NotFound(fmt.Errorf("not exist: '%s'", ctx.Params("*")))
+                       return
+               }
+
+               ctx.Next()
+       }
+}

diff --git a/modules/context/repo.go b/modules/context/repo.go
index 0ef644b522b45478eaebf459a30a951abb4c48b5..8a2b99c8542cdf66b1459a289ec0dbc49ed47977 100644 (file)
--- a/modules/context/repo.go
+++ b/modules/context/repo.go
@@ -716,7 +716,6 @@ func RepoRefByType(refType RepoRefType) macaron.Handler {
                        err     error
                )
 
-               // For API calls.
                if ctx.Repo.GitRepo == nil {
                        repoPath := models.RepoPath(ctx.Repo.Owner.Name, ctx.Repo.Repository.Name)
                        ctx.Repo.GitRepo, err = git.OpenRepository(repoPath)
@@ -785,7 +784,7 @@ func RepoRefByType(refType RepoRefType) macaron.Handler {
 
                                ctx.Repo.Commit, err = ctx.Repo.GitRepo.GetCommit(refName)
                                if err != nil {
-                                       ctx.NotFound("GetCommit", nil)
+                                       ctx.NotFound("GetCommit", err)
                                        return
                                }
                        } else {

diff --git a/routers/api/v1/api.go b/routers/api/v1/api.go
index 42489cd4a6c9af084b2f28c9762898da32818478..e9f1a395ed980039e19cfd93cd732b19db070015 100644 (file)
--- a/routers/api/v1/api.go
+++ b/routers/api/v1/api.go
@@ -191,14 +191,14 @@ func reqToken() macaron.Handler {
                        ctx.RequireCSRF()
                        return
                }
-               ctx.Context.Error(http.StatusUnauthorized)
+               ctx.Error(http.StatusUnauthorized, "reqToken", "token is required")
        }
 }
 
 func reqBasicAuth() macaron.Handler {
        return func(ctx *context.APIContext) {
                if !ctx.Context.IsBasicAuth {
-                       ctx.Context.Error(http.StatusUnauthorized)
+                       ctx.Error(http.StatusUnauthorized, "reqBasicAuth", "basic auth required")
                        return
                }
                ctx.CheckForOTP()
@@ -207,9 +207,9 @@ func reqBasicAuth() macaron.Handler {
 
 // reqSiteAdmin user should be the site admin
 func reqSiteAdmin() macaron.Handler {
-       return func(ctx *context.Context) {
+       return func(ctx *context.APIContext) {
                if !ctx.IsUserSiteAdmin() {
-                       ctx.Error(http.StatusForbidden)
+                       ctx.Error(http.StatusForbidden, "reqSiteAdmin", "user should be the site admin")
                        return
                }
        }
@@ -217,9 +217,9 @@ func reqSiteAdmin() macaron.Handler {
 
 // reqOwner user should be the owner of the repo or site admin.
 func reqOwner() macaron.Handler {
-       return func(ctx *context.Context) {
+       return func(ctx *context.APIContext) {
                if !ctx.IsUserRepoOwner() && !ctx.IsUserSiteAdmin() {
-                       ctx.Error(http.StatusForbidden)
+                       ctx.Error(http.StatusForbidden, "reqOwner", "user should be the owner of the repo")
                        return
                }
        }
@@ -227,9 +227,9 @@ func reqOwner() macaron.Handler {
 
 // reqAdmin user should be an owner or a collaborator with admin write of a repository, or site admin
 func reqAdmin() macaron.Handler {
-       return func(ctx *context.Context) {
+       return func(ctx *context.APIContext) {
                if !ctx.IsUserRepoAdmin() && !ctx.IsUserSiteAdmin() {
-                       ctx.Error(http.StatusForbidden)
+                       ctx.Error(http.StatusForbidden, "reqAdmin", "user should be an owner or a collaborator with admin write of a repository")
                        return
                }
        }
@@ -237,9 +237,9 @@ func reqAdmin() macaron.Handler {
 
 // reqRepoWriter user should have a permission to write to a repo, or be a site admin
 func reqRepoWriter(unitTypes ...models.UnitType) macaron.Handler {
-       return func(ctx *context.Context) {
+       return func(ctx *context.APIContext) {
                if !ctx.IsUserRepoWriter(unitTypes) && !ctx.IsUserRepoAdmin() && !ctx.IsUserSiteAdmin() {
-                       ctx.Error(http.StatusForbidden)
+                       ctx.Error(http.StatusForbidden, "reqRepoWriter", "user should have a permission to write to a repo")
                        return
                }
        }
@@ -247,9 +247,9 @@ func reqRepoWriter(unitTypes ...models.UnitType) macaron.Handler {
 
 // reqRepoReader user should have specific read permission or be a repo admin or a site admin
 func reqRepoReader(unitType models.UnitType) macaron.Handler {
-       return func(ctx *context.Context) {
+       return func(ctx *context.APIContext) {
                if !ctx.IsUserRepoReaderSpecific(unitType) && !ctx.IsUserRepoAdmin() && !ctx.IsUserSiteAdmin() {
-                       ctx.Error(http.StatusForbidden)
+                       ctx.Error(http.StatusForbidden, "reqRepoReader", "user should have specific read permission or be a repo admin or a site admin")
                        return
                }
        }
@@ -257,9 +257,9 @@ func reqRepoReader(unitType models.UnitType) macaron.Handler {
 
 // reqAnyRepoReader user should have any permission to read repository or permissions of site admin
 func reqAnyRepoReader() macaron.Handler {
-       return func(ctx *context.Context) {
+       return func(ctx *context.APIContext) {
                if !ctx.IsUserRepoReaderAny() && !ctx.IsUserSiteAdmin() {
-                       ctx.Error(http.StatusForbidden)
+                       ctx.Error(http.StatusForbidden, "reqAnyRepoReader", "user should have any permission to read repository or permissions of site admin")
                        return
                }
        }
@@ -502,7 +502,6 @@ func mustNotBeArchived(ctx *context.APIContext) {
 }
 
 // RegisterRoutes registers all v1 APIs routes to web application.
-// FIXME: custom form error response
 func RegisterRoutes(m *macaron.Macaron) {
        bind := binding.Bind
 
@@ -641,7 +640,7 @@ func RegisterRoutes(m *macaron.Macaron) {
                        m.Group("/:username/:reponame", func() {
                                m.Combo("").Get(reqAnyRepoReader(), repo.Get).
                                        Delete(reqToken(), reqOwner(), repo.Delete).
-                                       Patch(reqToken(), reqAdmin(), bind(api.EditRepoOption{}), context.RepoRef(), repo.Edit)
+                                       Patch(reqToken(), reqAdmin(), bind(api.EditRepoOption{}), context.RepoRefForAPI(), repo.Edit)
                                m.Post("/transfer", reqOwner(), bind(api.TransferRepoOption{}), repo.Transfer)
                                m.Combo("/notifications").
                                        Get(reqToken(), notify.ListRepoNotifications).
@@ -653,7 +652,7 @@ func RegisterRoutes(m *macaron.Macaron) {
                                                m.Combo("").Get(repo.GetHook).
                                                        Patch(bind(api.EditHookOption{}), repo.EditHook).
                                                        Delete(repo.DeleteHook)
-                                               m.Post("/tests", context.RepoRef(), repo.TestHook)
+                                               m.Post("/tests", context.RepoRefForAPI(), repo.TestHook)
                                        })
                                        m.Group("/git", func() {
                                                m.Combo("").Get(repo.ListGitHooks)
@@ -670,14 +669,14 @@ func RegisterRoutes(m *macaron.Macaron) {
                                                Put(reqAdmin(), bind(api.AddCollaboratorOption{}), repo.AddCollaborator).
                                                Delete(reqAdmin(), repo.DeleteCollaborator)
                                }, reqToken())
-                               m.Get("/raw/*", context.RepoRefByType(context.RepoRefAny), reqRepoReader(models.UnitTypeCode), repo.GetRawFile)
+                               m.Get("/raw/*", context.RepoRefForAPI(), reqRepoReader(models.UnitTypeCode), repo.GetRawFile)
                                m.Get("/archive/*", reqRepoReader(models.UnitTypeCode), repo.GetArchive)
                                m.Combo("/forks").Get(repo.ListForks).
                                        Post(reqToken(), reqRepoReader(models.UnitTypeCode), bind(api.CreateForkOption{}), repo.CreateFork)
                                m.Group("/branches", func() {
                                        m.Get("", repo.ListBranches)
-                                       m.Get("/*", context.RepoRefByType(context.RepoRefBranch), repo.GetBranch)
-                                       m.Delete("/*", reqRepoWriter(models.UnitTypeCode), context.RepoRefByType(context.RepoRefBranch), repo.DeleteBranch)
+                                       m.Get("/*", repo.GetBranch)
+                                       m.Delete("/*", context.ReferencesGitRepo(false), reqRepoWriter(models.UnitTypeCode), repo.DeleteBranch)
                                        m.Post("", reqRepoWriter(models.UnitTypeCode), bind(api.CreateBranchRepoOption{}), repo.CreateBranch)
                                }, reqRepoReader(models.UnitTypeCode))
                                m.Group("/branch_protections", func() {
@@ -804,7 +803,7 @@ func RegisterRoutes(m *macaron.Macaron) {
                                        })
                                }, reqRepoReader(models.UnitTypeReleases))
                                m.Post("/mirror-sync", reqToken(), reqRepoWriter(models.UnitTypeCode), repo.MirrorSync)
-                               m.Get("/editorconfig/:filename", context.RepoRef(), reqRepoReader(models.UnitTypeCode), repo.GetEditorconfig)
+                               m.Get("/editorconfig/:filename", context.RepoRefForAPI(), reqRepoReader(models.UnitTypeCode), repo.GetEditorconfig)
                                m.Group("/pulls", func() {
                                        m.Combo("").Get(bind(api.ListPullRequestsOptions{}), repo.ListPullRequests).
                                                Post(reqToken(), mustNotBeArchived, bind(api.CreatePullRequestOption{}), repo.CreatePullRequest)
@@ -851,9 +850,9 @@ func RegisterRoutes(m *macaron.Macaron) {
                                        })
                                        m.Get("/refs", repo.GetGitAllRefs)
                                        m.Get("/refs/*", repo.GetGitRefs)
-                                       m.Get("/trees/:sha", context.RepoRef(), repo.GetTree)
-                                       m.Get("/blobs/:sha", context.RepoRef(), repo.GetBlob)
-                                       m.Get("/tags/:sha", context.RepoRef(), repo.GetTag)
+                                       m.Get("/trees/:sha", context.RepoRefForAPI(), repo.GetTree)
+                                       m.Get("/blobs/:sha", context.RepoRefForAPI(), repo.GetBlob)
+                                       m.Get("/tags/:sha", context.RepoRefForAPI(), repo.GetTag)
                                }, reqRepoReader(models.UnitTypeCode))
                                m.Group("/contents", func() {
                                        m.Get("", repo.GetContentsList)

diff --git a/routers/api/v1/repo/branch.go b/routers/api/v1/repo/branch.go
index 384225d742c99bca40ca6b1185e289d3a8a5b8f8..2b20ab048d0387e8dbf897fa97ca5e5a1494c9a5 100644 (file)
--- a/routers/api/v1/repo/branch.go
+++ b/routers/api/v1/repo/branch.go
@@ -46,15 +46,12 @@ func GetBranch(ctx *context.APIContext) {
        // responses:
        //   "200":
        //     "$ref": "#/responses/Branch"
+       //   "404":
+       //     "$ref": "#/responses/notFound"
 
-       if ctx.Repo.TreePath != "" {
-               // if TreePath != "", then URL contained extra slashes
-               // (i.e. "master/subbranch" instead of "master"), so branch does
-               // not exist
-               ctx.NotFound()
-               return
-       }
-       branch, err := repo_module.GetBranch(ctx.Repo.Repository, ctx.Repo.BranchName)
+       branchName := ctx.Params("*")
+
+       branch, err := repo_module.GetBranch(ctx.Repo.Repository, branchName)
        if err != nil {
                if git.IsErrBranchNotExist(err) {
                        ctx.NotFound(err)
@@ -70,7 +67,7 @@ func GetBranch(ctx *context.APIContext) {
                return
        }
 
-       branchProtection, err := ctx.Repo.Repository.GetBranchProtection(ctx.Repo.BranchName)
+       branchProtection, err := ctx.Repo.Repository.GetBranchProtection(branchName)
        if err != nil {
                ctx.Error(http.StatusInternalServerError, "GetBranchProtection", err)
                return
@@ -113,21 +110,17 @@ func DeleteBranch(ctx *context.APIContext) {
        //     "$ref": "#/responses/empty"
        //   "403":
        //     "$ref": "#/responses/error"
+       //   "404":
+       //     "$ref": "#/responses/notFound"
 
-       if ctx.Repo.TreePath != "" {
-               // if TreePath != "", then URL contained extra slashes
-               // (i.e. "master/subbranch" instead of "master"), so branch does
-               // not exist
-               ctx.NotFound()
-               return
-       }
+       branchName := ctx.Params("*")
 
-       if ctx.Repo.Repository.DefaultBranch == ctx.Repo.BranchName {
+       if ctx.Repo.Repository.DefaultBranch == branchName {
                ctx.Error(http.StatusForbidden, "DefaultBranch", fmt.Errorf("can not delete default branch"))
                return
        }
 
-       isProtected, err := ctx.Repo.Repository.IsProtectedBranch(ctx.Repo.BranchName, ctx.User)
+       isProtected, err := ctx.Repo.Repository.IsProtectedBranch(branchName, ctx.User)
        if err != nil {
                ctx.InternalServerError(err)
                return
@@ -137,7 +130,7 @@ func DeleteBranch(ctx *context.APIContext) {
                return
        }
 
-       branch, err := repo_module.GetBranch(ctx.Repo.Repository, ctx.Repo.BranchName)
+       branch, err := repo_module.GetBranch(ctx.Repo.Repository, branchName)
        if err != nil {
                if git.IsErrBranchNotExist(err) {
                        ctx.NotFound(err)
@@ -153,7 +146,7 @@ func DeleteBranch(ctx *context.APIContext) {
                return
        }
 
-       if err := ctx.Repo.GitRepo.DeleteBranch(ctx.Repo.BranchName, git.DeleteBranchOptions{
+       if err := ctx.Repo.GitRepo.DeleteBranch(branchName, git.DeleteBranchOptions{
                Force: true,
        }); err != nil {
                ctx.Error(http.StatusInternalServerError, "DeleteBranch", err)
@@ -163,7 +156,7 @@ func DeleteBranch(ctx *context.APIContext) {
        // Don't return error below this
        if err := repo_service.PushUpdate(
                &repo_module.PushUpdateOptions{
-                       RefFullName:  git.BranchPrefix + ctx.Repo.BranchName,
+                       RefFullName:  git.BranchPrefix + branchName,
                        OldCommitID:  c.ID.String(),
                        NewCommitID:  git.EmptySHA,
                        PusherID:     ctx.User.ID,
@@ -174,7 +167,7 @@ func DeleteBranch(ctx *context.APIContext) {
                log.Error("Update: %v", err)
        }
 
-       if err := ctx.Repo.Repository.AddDeletedBranch(ctx.Repo.BranchName, c.ID.String(), ctx.User.ID); err != nil {
+       if err := ctx.Repo.Repository.AddDeletedBranch(branchName, c.ID.String(), ctx.User.ID); err != nil {
                log.Warn("AddDeletedBranch: %v", err)
        }
 

diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl
index b8f81bb8f7a0cdee60fd67b2dba1ae05814bce59..e759a1558ce08aaf23ef78c3ada15268556c74c0 100644 (file)
--- a/templates/swagger/v1_json.tmpl
+++ b/templates/swagger/v1_json.tmpl
@@ -2541,6 +2541,9 @@
         "responses": {
           "200": {
             "$ref": "#/responses/Branch"
+          },
+          "404": {
+            "$ref": "#/responses/notFound"
           }
         }
       },
@@ -2582,6 +2585,9 @@
           },
           "403": {
             "$ref": "#/responses/error"
+          },
+          "404": {
+            "$ref": "#/responses/notFound"
           }
         }
       }