Add API to manage issue dependencies #17935
Conversation
Codecov Report
@@ Coverage Diff @@
## main #17935 +/- ##
==========================================
- Coverage 47.51% 47.36% -0.15%
==========================================
Files 944 944
Lines 131549 131969 +420
==========================================
+ Hits 62500 62510 +10
- Misses 61541 61950 +409
- Partials 7508 7509 +1
Continue to review full report at Codecov.
|
routers/api/v1/repo/issue.go
Outdated
| issue, err := models.GetIssueWithAttrsByIndex(ctx.Repo.Repository.ID, ctx.ParamsInt64(":index")) | ||
| if err != nil { | ||
| if models.IsErrIssueNotExist(err) { | ||
| ctx.NotFound("IsErrIssueNotExist", err) | ||
| } else { | ||
| ctx.Error(http.StatusInternalServerError, "GetIssueByIndex", err) | ||
| } | ||
| return | ||
| } | ||
|
|
||
| deps, err := issue.BlockedByDependencies() | ||
| if err != nil { | ||
| ctx.Error(http.StatusInternalServerError, "BlockedByDependencies", err) | ||
| return | ||
| } | ||
|
|
||
| page := ctx.FormInt("page") | ||
| if page <= 1 { | ||
| page = 1 | ||
| } | ||
| limit := ctx.FormInt("limit") | ||
| if limit <= 1 { | ||
| limit = setting.API.DefaultPagingNum | ||
| } | ||
|
|
||
| skip := (page - 1) * limit | ||
| max := page * limit | ||
|
|
||
| var issues []*models.Issue | ||
| for i, depMeta := range deps { | ||
| if i < skip || i >= max { | ||
| continue | ||
| } | ||
| depMeta.Issue.Repo = &depMeta.Repository | ||
| issues = append(issues, &depMeta.Issue) | ||
| } | ||
|
|
There was a problem hiding this comment.
Usually when you're writing paging code you're attempting to reduce the load on the server and the DB by not getting everything out of the db at once. Here you're collecting almost all of the information then simply throwing it away. Is there not a way of just getting the right dependencies out?
I'd also argue that it seems like the issue is only used to get the blocking dependencies before being thrown away - in which case it seems like it's not really needed so perhaps we could just get the blocking issues out directly. In particular GetIssueWithAttrsByIndex is loading all of the comments and attachments of the base issue to simply then throw them out! BlockedByDependencies does not need the attributes to be loaded so you don't need that.
In some ways these APIs are unlikely to be called much so the fact that they're not very efficient is not really relevant but it's a point that we should be considering.
There is a serious security problem in this code. Not only can I use these endpoints to check if any repository exists but I can read any issue on any repository using this.
| } | ||
|
|
||
| form := web.GetForm(ctx).(*api.IssueMeta) | ||
| repo, err := repo_model.GetRepositoryByOwnerAndName(form.Owner, form.Name) |
There was a problem hiding this comment.
We probably need to think a bit about permissions here.
When can I add an issue from another repository as a blocker to my issue?
What permissions do I need to have and in which repository?
At present this code can be used to determine not only if a repository exists - but if an issue exists in that repository and then render it out to me!
| repo, err := repo_model.GetRepositoryByOwnerAndName(form.Owner, form.Name) | ||
| if err != nil { | ||
| if repo_model.IsErrRepoNotExist(err) { | ||
| ctx.NotFound("IsErrRepoNotExist", err) | ||
| } else { | ||
| ctx.Error(http.StatusInternalServerError, "GetRepositoryByOwnerAndName", err) | ||
| } | ||
| return | ||
| } |
There was a problem hiding this comment.
We really need to consider permissions here.
| return | ||
| } | ||
|
|
||
| ctx.JSON(http.StatusCreated, convert.ToAPIIssue(dep)) |
There was a problem hiding this comment.
You really need to check if I have permission to read this issue!! Using this endpoint I could read any issue from any repository on the server!
| issues = append(issues, &depMeta.Issue) | ||
| } | ||
|
|
||
| ctx.JSON(http.StatusOK, convert.ToAPIIssueList(issues)) |
There was a problem hiding this comment.
Need to check if I have permission to read these issues!!
| issues = append(issues, &depMeta.Issue) | ||
| } | ||
|
|
||
| ctx.JSON(http.StatusOK, convert.ToAPIIssueList(issues)) |
There was a problem hiding this comment.
Need to check if I have permission to read these issues!!
| return | ||
| } | ||
|
|
||
| ctx.JSON(http.StatusOK, convert.ToAPIIssue(dep)) |
There was a problem hiding this comment.
Need to check if I have permission to read these issues!!
There was a problem hiding this comment.
@zeripath Thanks, I understood it
Adds API endpoints to manage issue/PR dependencies
GET /repos/{owner}/{repo}/issues/{index}/blocksList issues that are blocked by this issuePOST /repos/{owner}/{repo}/issues/{index}/blocksBlock the issue given in the body by the issue in pathDELETE /repos/{owner}/{repo}/issues/{index}/blocksUnblock the issue given in the body by the issue in pathGET /repos/{owner}/{repo}/issues/{index}/dependenciesList an issue's dependenciesPOST /repos/{owner}/{repo}/issues/{index}/dependenciesCreate a new issue dependenciesDELETE /repos/{owner}/{repo}/issues/{index}/dependenciesRemove an issue dependencyCloses #15393