Calculate label URL on API

[6543]

fix #8028

[KN4CK3R]

Could the caches contain more then one element?

[codecov-commenter]

Codecov Report

❗ No coverage uploaded for pull request base (main@9a938dc). Click here to learn what that means.
The diff coverage is 75.86%.

@@           Coverage Diff           @@
##             main   #16186   +/-   ##
=======================================
  Coverage        ?   45.20%           
=======================================
  Files           ?      766           
  Lines           ?    86592           
  Branches        ?        0           
=======================================
  Hits            ?    39144           
  Misses          ?    41105           
  Partials        ?     6343           

Impacted Files	Coverage Δ
modules/convert/issue.go	84.50% <66.66%> (ø)
routers/api/v1/repo/issue_label.go	15.38% <66.66%> (ø)
routers/api/v1/org/label.go	56.75% <100.00%> (ø)
routers/api/v1/repo/label.go	56.75% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9a938dc...b13078c. Read the comment docs.

[zeripath]

Could the caches contain more then one element?

Hmm... I don't think they can here.

The labels either belong to the repo or the org. It doesn't appear to make much sense to me that we would provide a cross repo label API.

@6543 is there actually a way that this could happen?

[6543]

Yes the endpoint to get labels of issues can return org & repo labels

[6543]

Could the caches contain more then one element?

yes this are edge cases but they exist

[zeripath]

hmm, let's check these:

• convert/issue.go:21:func ToAPIIssue(issue *models.Issue) *api.Issue {
  • L44: Labels: ToLabelList(issue.Labels, repoCache, nil),
  • All of these labels must be within the same repository and same organization
• convert/issue.go:177: ToLabel(label *models.Label, repoCache map[int64]*models.Repository, orgCache map[int64]*models.User)
  • Called by routers/api/v1/org/label.go 3 times but all must be in the same org.
  • Called by routers/api/v1/repo/label.go 3 times but all must be in the same repo.
  • Called by ToLabelList - see below.
• convert/issue.go:224:ToLabelList
  • Called by ToAPIIssue above. Same Repo/Org
  • Called by routers/api/v1/org/label.go ListLabels - all must be in same org.
  • Called by routers/api/v1/repo/label.go ListLabels - all must be in the same repo.
  • Called by routers/api/v1/repo/issue_label.go 3 times but all must be in the same repo or org.

@6543 where were you thinking this was called where it could be cross-repo or cross-org?

[stale]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs during the next 2 months. Thank you for your contributions.

[delvh]

I think err := issue.Repo.GetOwner(); is misleading, as this implies that err would be the owner.
After what I have seen, this function should most likely be renamed to LoadOwner, because that's what it does.
Returning only an error makes much more sense then.
An alternative for naming it would be InitializeOwner() but that seems to conflict with the current naming scheme.
When renaming it, its function comment should be changed as well as that is simply incorrect by now.
Could it be that that is a leftover from when the owner was initialized eagerly?