From 7d2700c8be5da8f2073a576dae209ae07ac6ed22 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Sat, 14 Nov 2020 17:13:55 +0100 Subject: [PATCH] [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 --- modules/context/api.go | 58 ++++++++++++++++++++++++++++++++++ modules/context/repo.go | 3 +- routers/api/v1/api.go | 47 ++++++++++++++------------- routers/api/v1/repo/branch.go | 37 +++++++++------------- templates/swagger/v1_json.tmpl | 6 ++++ 5 files changed, 103 insertions(+), 48 deletions(-) diff --git a/modules/context/api.go b/modules/context/api.go index 9dad588c7..b5f521f63 100644 --- 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 0ef644b52..8a2b99c85 100644 --- 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 42489cd4a..e9f1a395e 100644 --- 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 384225d74..2b20ab048 100644 --- 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 b8f81bb8f..e759a1558 100644 --- 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" } } }