Skip to content

Commit 7d2700c

Browse files
6543zeripath
6543
and
zeripath
authored
[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 <[email protected]>
1 parent 3f3447a commit 7d2700c

File tree

5 files changed

+103
-48
lines changed

5 files changed

+103
-48
lines changed

‎modules/context/api.go

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -259,3 +259,61 @@ func (ctx *APIContext) NotFound(objs ...interface{}) {
259259
"errors": errors,
260260
})
261261
}
262+
263+
// RepoRefForAPI handles repository reference names when the ref name is not explicitly given
264+
func RepoRefForAPI() macaron.Handler {
265+
return func(ctx *APIContext) {
266+
// Empty repository does not have reference information.
267+
if ctx.Repo.Repository.IsEmpty {
268+
return
269+
}
270+
271+
var err error
272+
273+
if ctx.Repo.GitRepo == nil {
274+
repoPath := models.RepoPath(ctx.Repo.Owner.Name, ctx.Repo.Repository.Name)
275+
ctx.Repo.GitRepo, err = git.OpenRepository(repoPath)
276+
if err != nil {
277+
ctx.InternalServerError(err)
278+
return
279+
}
280+
// We opened it, we should close it
281+
defer func() {
282+
// If it's been set to nil then assume someone else has closed it.
283+
if ctx.Repo.GitRepo != nil {
284+
ctx.Repo.GitRepo.Close()
285+
}
286+
}()
287+
}
288+
289+
refName := getRefName(ctx.Context, RepoRefAny)
290+
291+
if ctx.Repo.GitRepo.IsBranchExist(refName) {
292+
ctx.Repo.Commit, err = ctx.Repo.GitRepo.GetBranchCommit(refName)
293+
if err != nil {
294+
ctx.InternalServerError(err)
295+
return
296+
}
297+
ctx.Repo.CommitID = ctx.Repo.Commit.ID.String()
298+
} else if ctx.Repo.GitRepo.IsTagExist(refName) {
299+
ctx.Repo.Commit, err = ctx.Repo.GitRepo.GetTagCommit(refName)
300+
if err != nil {
301+
ctx.InternalServerError(err)
302+
return
303+
}
304+
ctx.Repo.CommitID = ctx.Repo.Commit.ID.String()
305+
} else if len(refName) == 40 {
306+
ctx.Repo.CommitID = refName
307+
ctx.Repo.Commit, err = ctx.Repo.GitRepo.GetCommit(refName)
308+
if err != nil {
309+
ctx.NotFound("GetCommit", err)
310+
return
311+
}
312+
} else {
313+
ctx.NotFound(fmt.Errorf("not exist: '%s'", ctx.Params("*")))
314+
return
315+
}
316+
317+
ctx.Next()
318+
}
319+
}

‎modules/context/repo.go

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -716,7 +716,6 @@ func RepoRefByType(refType RepoRefType) macaron.Handler {
716716
err error
717717
)
718718

719-
// For API calls.
720719
if ctx.Repo.GitRepo == nil {
721720
repoPath := models.RepoPath(ctx.Repo.Owner.Name, ctx.Repo.Repository.Name)
722721
ctx.Repo.GitRepo, err = git.OpenRepository(repoPath)
@@ -785,7 +784,7 @@ func RepoRefByType(refType RepoRefType) macaron.Handler {
785784

786785
ctx.Repo.Commit, err = ctx.Repo.GitRepo.GetCommit(refName)
787786
if err != nil {
788-
ctx.NotFound("GetCommit", nil)
787+
ctx.NotFound("GetCommit", err)
789788
return
790789
}
791790
} else {

‎routers/api/v1/api.go

Lines changed: 23 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -191,14 +191,14 @@ func reqToken() macaron.Handler {
191191
ctx.RequireCSRF()
192192
return
193193
}
194-
ctx.Context.Error(http.StatusUnauthorized)
194+
ctx.Error(http.StatusUnauthorized, "reqToken", "token is required")
195195
}
196196
}
197197

198198
func reqBasicAuth() macaron.Handler {
199199
return func(ctx *context.APIContext) {
200200
if !ctx.Context.IsBasicAuth {
201-
ctx.Context.Error(http.StatusUnauthorized)
201+
ctx.Error(http.StatusUnauthorized, "reqBasicAuth", "basic auth required")
202202
return
203203
}
204204
ctx.CheckForOTP()
@@ -207,59 +207,59 @@ func reqBasicAuth() macaron.Handler {
207207

208208
// reqSiteAdmin user should be the site admin
209209
func reqSiteAdmin() macaron.Handler {
210-
return func(ctx *context.Context) {
210+
return func(ctx *context.APIContext) {
211211
if !ctx.IsUserSiteAdmin() {
212-
ctx.Error(http.StatusForbidden)
212+
ctx.Error(http.StatusForbidden, "reqSiteAdmin", "user should be the site admin")
213213
return
214214
}
215215
}
216216
}
217217

218218
// reqOwner user should be the owner of the repo or site admin.
219219
func reqOwner() macaron.Handler {
220-
return func(ctx *context.Context) {
220+
return func(ctx *context.APIContext) {
221221
if !ctx.IsUserRepoOwner() && !ctx.IsUserSiteAdmin() {
222-
ctx.Error(http.StatusForbidden)
222+
ctx.Error(http.StatusForbidden, "reqOwner", "user should be the owner of the repo")
223223
return
224224
}
225225
}
226226
}
227227

228228
// reqAdmin user should be an owner or a collaborator with admin write of a repository, or site admin
229229
func reqAdmin() macaron.Handler {
230-
return func(ctx *context.Context) {
230+
return func(ctx *context.APIContext) {
231231
if !ctx.IsUserRepoAdmin() && !ctx.IsUserSiteAdmin() {
232-
ctx.Error(http.StatusForbidden)
232+
ctx.Error(http.StatusForbidden, "reqAdmin", "user should be an owner or a collaborator with admin write of a repository")
233233
return
234234
}
235235
}
236236
}
237237

238238
// reqRepoWriter user should have a permission to write to a repo, or be a site admin
239239
func reqRepoWriter(unitTypes ...models.UnitType) macaron.Handler {
240-
return func(ctx *context.Context) {
240+
return func(ctx *context.APIContext) {
241241
if !ctx.IsUserRepoWriter(unitTypes) && !ctx.IsUserRepoAdmin() && !ctx.IsUserSiteAdmin() {
242-
ctx.Error(http.StatusForbidden)
242+
ctx.Error(http.StatusForbidden, "reqRepoWriter", "user should have a permission to write to a repo")
243243
return
244244
}
245245
}
246246
}
247247

248248
// reqRepoReader user should have specific read permission or be a repo admin or a site admin
249249
func reqRepoReader(unitType models.UnitType) macaron.Handler {
250-
return func(ctx *context.Context) {
250+
return func(ctx *context.APIContext) {
251251
if !ctx.IsUserRepoReaderSpecific(unitType) && !ctx.IsUserRepoAdmin() && !ctx.IsUserSiteAdmin() {
252-
ctx.Error(http.StatusForbidden)
252+
ctx.Error(http.StatusForbidden, "reqRepoReader", "user should have specific read permission or be a repo admin or a site admin")
253253
return
254254
}
255255
}
256256
}
257257

258258
// reqAnyRepoReader user should have any permission to read repository or permissions of site admin
259259
func reqAnyRepoReader() macaron.Handler {
260-
return func(ctx *context.Context) {
260+
return func(ctx *context.APIContext) {
261261
if !ctx.IsUserRepoReaderAny() && !ctx.IsUserSiteAdmin() {
262-
ctx.Error(http.StatusForbidden)
262+
ctx.Error(http.StatusForbidden, "reqAnyRepoReader", "user should have any permission to read repository or permissions of site admin")
263263
return
264264
}
265265
}
@@ -502,7 +502,6 @@ func mustNotBeArchived(ctx *context.APIContext) {
502502
}
503503

504504
// RegisterRoutes registers all v1 APIs routes to web application.
505-
// FIXME: custom form error response
506505
func RegisterRoutes(m *macaron.Macaron) {
507506
bind := binding.Bind
508507

@@ -641,7 +640,7 @@ func RegisterRoutes(m *macaron.Macaron) {
641640
m.Group("/:username/:reponame", func() {
642641
m.Combo("").Get(reqAnyRepoReader(), repo.Get).
643642
Delete(reqToken(), reqOwner(), repo.Delete).
644-
Patch(reqToken(), reqAdmin(), bind(api.EditRepoOption{}), context.RepoRef(), repo.Edit)
643+
Patch(reqToken(), reqAdmin(), bind(api.EditRepoOption{}), context.RepoRefForAPI(), repo.Edit)
645644
m.Post("/transfer", reqOwner(), bind(api.TransferRepoOption{}), repo.Transfer)
646645
m.Combo("/notifications").
647646
Get(reqToken(), notify.ListRepoNotifications).
@@ -653,7 +652,7 @@ func RegisterRoutes(m *macaron.Macaron) {
653652
m.Combo("").Get(repo.GetHook).
654653
Patch(bind(api.EditHookOption{}), repo.EditHook).
655654
Delete(repo.DeleteHook)
656-
m.Post("/tests", context.RepoRef(), repo.TestHook)
655+
m.Post("/tests", context.RepoRefForAPI(), repo.TestHook)
657656
})
658657
m.Group("/git", func() {
659658
m.Combo("").Get(repo.ListGitHooks)
@@ -670,14 +669,14 @@ func RegisterRoutes(m *macaron.Macaron) {
670669
Put(reqAdmin(), bind(api.AddCollaboratorOption{}), repo.AddCollaborator).
671670
Delete(reqAdmin(), repo.DeleteCollaborator)
672671
}, reqToken())
673-
m.Get("/raw/*", context.RepoRefByType(context.RepoRefAny), reqRepoReader(models.UnitTypeCode), repo.GetRawFile)
672+
m.Get("/raw/*", context.RepoRefForAPI(), reqRepoReader(models.UnitTypeCode), repo.GetRawFile)
674673
m.Get("/archive/*", reqRepoReader(models.UnitTypeCode), repo.GetArchive)
675674
m.Combo("/forks").Get(repo.ListForks).
676675
Post(reqToken(), reqRepoReader(models.UnitTypeCode), bind(api.CreateForkOption{}), repo.CreateFork)
677676
m.Group("/branches", func() {
678677
m.Get("", repo.ListBranches)
679-
m.Get("/*", context.RepoRefByType(context.RepoRefBranch), repo.GetBranch)
680-
m.Delete("/*", reqRepoWriter(models.UnitTypeCode), context.RepoRefByType(context.RepoRefBranch), repo.DeleteBranch)
678+
m.Get("/*", repo.GetBranch)
679+
m.Delete("/*", context.ReferencesGitRepo(false), reqRepoWriter(models.UnitTypeCode), repo.DeleteBranch)
681680
m.Post("", reqRepoWriter(models.UnitTypeCode), bind(api.CreateBranchRepoOption{}), repo.CreateBranch)
682681
}, reqRepoReader(models.UnitTypeCode))
683682
m.Group("/branch_protections", func() {
@@ -804,7 +803,7 @@ func RegisterRoutes(m *macaron.Macaron) {
804803
})
805804
}, reqRepoReader(models.UnitTypeReleases))
806805
m.Post("/mirror-sync", reqToken(), reqRepoWriter(models.UnitTypeCode), repo.MirrorSync)
807-
m.Get("/editorconfig/:filename", context.RepoRef(), reqRepoReader(models.UnitTypeCode), repo.GetEditorconfig)
806+
m.Get("/editorconfig/:filename", context.RepoRefForAPI(), reqRepoReader(models.UnitTypeCode), repo.GetEditorconfig)
808807
m.Group("/pulls", func() {
809808
m.Combo("").Get(bind(api.ListPullRequestsOptions{}), repo.ListPullRequests).
810809
Post(reqToken(), mustNotBeArchived, bind(api.CreatePullRequestOption{}), repo.CreatePullRequest)
@@ -851,9 +850,9 @@ func RegisterRoutes(m *macaron.Macaron) {
851850
})
852851
m.Get("/refs", repo.GetGitAllRefs)
853852
m.Get("/refs/*", repo.GetGitRefs)
854-
m.Get("/trees/:sha", context.RepoRef(), repo.GetTree)
855-
m.Get("/blobs/:sha", context.RepoRef(), repo.GetBlob)
856-
m.Get("/tags/:sha", context.RepoRef(), repo.GetTag)
853+
m.Get("/trees/:sha", context.RepoRefForAPI(), repo.GetTree)
854+
m.Get("/blobs/:sha", context.RepoRefForAPI(), repo.GetBlob)
855+
m.Get("/tags/:sha", context.RepoRefForAPI(), repo.GetTag)
857856
}, reqRepoReader(models.UnitTypeCode))
858857
m.Group("/contents", func() {
859858
m.Get("", repo.GetContentsList)

‎routers/api/v1/repo/branch.go

Lines changed: 15 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -46,15 +46,12 @@ func GetBranch(ctx *context.APIContext) {
4646
// responses:
4747
// "200":
4848
// "$ref": "#/responses/Branch"
49+
// "404":
50+
// "$ref": "#/responses/notFound"
4951

50-
if ctx.Repo.TreePath != "" {
51-
// if TreePath != "", then URL contained extra slashes
52-
// (i.e. "master/subbranch" instead of "master"), so branch does
53-
// not exist
54-
ctx.NotFound()
55-
return
56-
}
57-
branch, err := repo_module.GetBranch(ctx.Repo.Repository, ctx.Repo.BranchName)
52+
branchName := ctx.Params("*")
53+
54+
branch, err := repo_module.GetBranch(ctx.Repo.Repository, branchName)
5855
if err != nil {
5956
if git.IsErrBranchNotExist(err) {
6057
ctx.NotFound(err)
@@ -70,7 +67,7 @@ func GetBranch(ctx *context.APIContext) {
7067
return
7168
}
7269

73-
branchProtection, err := ctx.Repo.Repository.GetBranchProtection(ctx.Repo.BranchName)
70+
branchProtection, err := ctx.Repo.Repository.GetBranchProtection(branchName)
7471
if err != nil {
7572
ctx.Error(http.StatusInternalServerError, "GetBranchProtection", err)
7673
return
@@ -113,21 +110,17 @@ func DeleteBranch(ctx *context.APIContext) {
113110
// "$ref": "#/responses/empty"
114111
// "403":
115112
// "$ref": "#/responses/error"
113+
// "404":
114+
// "$ref": "#/responses/notFound"
116115

117-
if ctx.Repo.TreePath != "" {
118-
// if TreePath != "", then URL contained extra slashes
119-
// (i.e. "master/subbranch" instead of "master"), so branch does
120-
// not exist
121-
ctx.NotFound()
122-
return
123-
}
116+
branchName := ctx.Params("*")
124117

125-
if ctx.Repo.Repository.DefaultBranch == ctx.Repo.BranchName {
118+
if ctx.Repo.Repository.DefaultBranch == branchName {
126119
ctx.Error(http.StatusForbidden, "DefaultBranch", fmt.Errorf("can not delete default branch"))
127120
return
128121
}
129122

130-
isProtected, err := ctx.Repo.Repository.IsProtectedBranch(ctx.Repo.BranchName, ctx.User)
123+
isProtected, err := ctx.Repo.Repository.IsProtectedBranch(branchName, ctx.User)
131124
if err != nil {
132125
ctx.InternalServerError(err)
133126
return
@@ -137,7 +130,7 @@ func DeleteBranch(ctx *context.APIContext) {
137130
return
138131
}
139132

140-
branch, err := repo_module.GetBranch(ctx.Repo.Repository, ctx.Repo.BranchName)
133+
branch, err := repo_module.GetBranch(ctx.Repo.Repository, branchName)
141134
if err != nil {
142135
if git.IsErrBranchNotExist(err) {
143136
ctx.NotFound(err)
@@ -153,7 +146,7 @@ func DeleteBranch(ctx *context.APIContext) {
153146
return
154147
}
155148

156-
if err := ctx.Repo.GitRepo.DeleteBranch(ctx.Repo.BranchName, git.DeleteBranchOptions{
149+
if err := ctx.Repo.GitRepo.DeleteBranch(branchName, git.DeleteBranchOptions{
157150
Force: true,
158151
}); err != nil {
159152
ctx.Error(http.StatusInternalServerError, "DeleteBranch", err)
@@ -163,7 +156,7 @@ func DeleteBranch(ctx *context.APIContext) {
163156
// Don't return error below this
164157
if err := repo_service.PushUpdate(
165158
&repo_module.PushUpdateOptions{
166-
RefFullName: git.BranchPrefix + ctx.Repo.BranchName,
159+
RefFullName: git.BranchPrefix + branchName,
167160
OldCommitID: c.ID.String(),
168161
NewCommitID: git.EmptySHA,
169162
PusherID: ctx.User.ID,
@@ -174,7 +167,7 @@ func DeleteBranch(ctx *context.APIContext) {
174167
log.Error("Update: %v", err)
175168
}
176169

177-
if err := ctx.Repo.Repository.AddDeletedBranch(ctx.Repo.BranchName, c.ID.String(), ctx.User.ID); err != nil {
170+
if err := ctx.Repo.Repository.AddDeletedBranch(branchName, c.ID.String(), ctx.User.ID); err != nil {
178171
log.Warn("AddDeletedBranch: %v", err)
179172
}
180173

‎templates/swagger/v1_json.tmpl

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2541,6 +2541,9 @@
25412541
"responses": {
25422542
"200": {
25432543
"$ref": "#/responses/Branch"
2544+
},
2545+
"404": {
2546+
"$ref": "#/responses/notFound"
25442547
}
25452548
}
25462549
},
@@ -2582,6 +2585,9 @@
25822585
},
25832586
"403": {
25842587
"$ref": "#/responses/error"
2588+
},
2589+
"404": {
2590+
"$ref": "#/responses/notFound"
25852591
}
25862592
}
25872593
}

0 commit comments

Comments
 (0)