[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>
This commit is contained in:
6543 2020-11-14 17:13:55 +01:00 committed by GitHub
parent 3f3447a1ea
commit 7d2700c8be
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
5 changed files with 103 additions and 48 deletions

View File

@ -259,3 +259,61 @@ func (ctx *APIContext) NotFound(objs ...interface{}) {
"errors": errors, "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()
}
}

View File

@ -716,7 +716,6 @@ func RepoRefByType(refType RepoRefType) macaron.Handler {
err error err error
) )
// For API calls.
if ctx.Repo.GitRepo == nil { if ctx.Repo.GitRepo == nil {
repoPath := models.RepoPath(ctx.Repo.Owner.Name, ctx.Repo.Repository.Name) repoPath := models.RepoPath(ctx.Repo.Owner.Name, ctx.Repo.Repository.Name)
ctx.Repo.GitRepo, err = git.OpenRepository(repoPath) 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) ctx.Repo.Commit, err = ctx.Repo.GitRepo.GetCommit(refName)
if err != nil { if err != nil {
ctx.NotFound("GetCommit", nil) ctx.NotFound("GetCommit", err)
return return
} }
} else { } else {

View File

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

View File

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

View File

@ -2541,6 +2541,9 @@
"responses": { "responses": {
"200": { "200": {
"$ref": "#/responses/Branch" "$ref": "#/responses/Branch"
},
"404": {
"$ref": "#/responses/notFound"
} }
} }
}, },
@ -2582,6 +2585,9 @@
}, },
"403": { "403": {
"$ref": "#/responses/error" "$ref": "#/responses/error"
},
"404": {
"$ref": "#/responses/notFound"
} }
} }
} }