The Wayback Machine - https://web.archive.org/web/20220212204008/https://github.com/go-gitea/gitea/pull/16186
Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Calculate label URL on API #16186

Merged
merged 12 commits into from Sep 10, 2021
Merged

Calculate label URL on API #16186

merged 12 commits into from Sep 10, 2021

Conversation

@6543
Copy link
Member

@6543 6543 commented Jun 17, 2021

fix #8028
@6543 6543 added this to the 1.15.0 milestone Jun 17, 2021
@KN4CK3R
Copy link
Member

@KN4CK3R KN4CK3R commented Jun 17, 2021

Could the caches contain more then one element?

@zeripath zeripath changed the title [API] lable object calculate URL [API] label object calculate URL Jun 17, 2021
@zeripath zeripath changed the title [API] label object calculate URL [API] Calculate label URL Jun 17, 2021
zeripath
zeripath reviewed Jun 17, 2021
View changes
modules/convert/issue.go Outdated Show resolved Hide resolved
zeripath
zeripath reviewed Jun 17, 2021
View changes
modules/convert/issue.go Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

@codecov-commenter codecov-commenter commented Jun 17, 2021 •
edited

Codecov Report

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

Impacted file tree graph

@@           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
Copy link
Contributor

@zeripath zeripath commented Jun 18, 2021

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
Copy link
Member Author

@6543 6543 commented Jun 18, 2021

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

@6543
Copy link
Member Author

@6543 6543 commented Jun 20, 2021

Could the caches contain more then one element?

yes this are edge cases but they exist

@zeripath
Copy link
Contributor

@zeripath zeripath commented Jun 20, 2021

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?

@techknowlogick techknowlogick removed this from the 1.15.0 milestone Jun 23, 2021
@techknowlogick techknowlogick added this to the 1.16.0 milestone Jun 23, 2021
lunny
lunny reviewed Jun 28, 2021
View changes
modules/convert/issue.go Outdated Show resolved Hide resolved
@stale
Copy link

@stale stale bot commented Aug 28, 2021

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.

@stale stale bot added the stale label Aug 28, 2021
@6543 6543 removed the stale label Aug 29, 2021
@6543 6543 self-assigned this Aug 29, 2021
@6543 6543 removed the status/wip label Aug 29, 2021
lunny
lunny approved these changes Aug 29, 2021
View changes
@zeripath zeripath changed the title [API] Calculate label URL Calculate label URL on API Sep 10, 2021
@6543 6543 merged commit 51578d6 into go-gitea:main Sep 10, 2021
2 checks passed
@6543 6543 deleted the api-lable-fill-url branch Sep 10, 2021
delvh
delvh reviewed Sep 10, 2021
View changes
modules/convert/issue.go
@@ -25,6 +28,9 @@ func ToAPIIssue(issue *models.Issue) *api.Issue {
if err := issue.LoadRepo(); err != nil {
return &api.Issue{}
}
if err := issue.Repo.GetOwner(); err != nil {
Copy link
Contributor

@delvh delvh Sep 10, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@go-gitea go-gitea locked and limited conversation to collaborators Oct 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@delvh delvh

@lunny lunny

@zeripath zeripath

Assignees

@6543 6543

Labels
kind/api kind/bug lgtm/done
Milestone
1.16.0
Linked issues

Successfully merging this pull request may close these issues.

API response for label returns empty 'url' field
8 participants
@6543 @KN4CK3R @codecov-commenter @zeripath @lunny @delvh @techknowlogick @GiteaBot