[Refactor] move APIFormat() of Issue and Label to convert package (#10423)

* Label: delete .APIFormat() and use convert.ToLabel()

* move issue APIFormat too

* add missing one

* move TEST too

* handle error -> return empty APIIssue

* Apply suggestions from code review

Co-Authored-By: guillep2k <18600385+guillep2k@users.noreply.github.com>

Co-authored-by: zeripath <art27@cantab.net>
Co-authored-by: guillep2k <18600385+guillep2k@users.noreply.github.com>

models/issue.go

@@ -138,6 +138,11 @@ func (issue *Issue) GetPullRequest() (pr *PullRequest, err error) {
 	return
 }
 
+// LoadLabels loads labels
+func (issue *Issue) LoadLabels() error {
+	return issue.loadLabels(x)
+}
+
 func (issue *Issue) loadLabels(e Engine) (err error) {
 	if issue.Labels == nil {
 		issue.Labels, err = getLabelsByIssueID(e, issue.ID)
@@ -364,76 +369,6 @@ func (issue *Issue) State() api.StateType {
 	return api.StateOpen
 }
 
-// APIFormat assumes some fields assigned with values:
-// Required - Poster, Labels,
-// Optional - Milestone, Assignee, PullRequest
-func (issue *Issue) APIFormat() *api.Issue {
-	return issue.apiFormat(x)
-}
-
-func (issue *Issue) apiFormat(e Engine) *api.Issue {
-	issue.loadLabels(e)
-	apiLabels := make([]*api.Label, len(issue.Labels))
-	for i := range issue.Labels {
-		apiLabels[i] = issue.Labels[i].APIFormat()
-	}
-
-	issue.loadPoster(e)
-	issue.loadRepo(e)
-	apiIssue := &api.Issue{
-		ID:       issue.ID,
-		URL:      issue.APIURL(),
-		HTMLURL:  issue.HTMLURL(),
-		Index:    issue.Index,
-		Poster:   issue.Poster.APIFormat(),
-		Title:    issue.Title,
-		Body:     issue.Content,
-		Labels:   apiLabels,
-		State:    issue.State(),
-		Comments: issue.NumComments,
-		Created:  issue.CreatedUnix.AsTime(),
-		Updated:  issue.UpdatedUnix.AsTime(),
-	}
-
-	apiIssue.Repo = &api.RepositoryMeta{
-		ID:       issue.Repo.ID,
-		Name:     issue.Repo.Name,
-		Owner:    issue.Repo.OwnerName,
-		FullName: issue.Repo.FullName(),
-	}
-
-	if issue.ClosedUnix != 0 {
-		apiIssue.Closed = issue.ClosedUnix.AsTimePtr()
-	}
-
-	issue.loadMilestone(e)
-	if issue.Milestone != nil {
-		apiIssue.Milestone = issue.Milestone.APIFormat()
-	}
-
-	issue.loadAssignees(e)
-	if len(issue.Assignees) > 0 {
-		for _, assignee := range issue.Assignees {
-			apiIssue.Assignees = append(apiIssue.Assignees, assignee.APIFormat())
-		}
-		apiIssue.Assignee = issue.Assignees[0].APIFormat() // For compatibility, we're keeping the first assignee as `apiIssue.Assignee`
-	}
-	if issue.IsPull {
-		issue.loadPullRequest(e)
-		apiIssue.PullRequest = &api.PullRequestMeta{
-			HasMerged: issue.PullRequest.HasMerged,
-		}
-		if issue.PullRequest.HasMerged {
-			apiIssue.PullRequest.Merged = issue.PullRequest.MergedUnix.AsTimePtr()
-		}
-	}
-	if issue.DeadlineUnix != 0 {
-		apiIssue.Deadline = issue.DeadlineUnix.AsTimePtr()
-	}
-
-	return apiIssue
-}
-
 // HashTag returns unique hash tag for issue.
 func (issue *Issue) HashTag() string {
 	return "issue-" + com.ToStr(issue.ID)

models/issue_assignees.go

@@ -19,6 +19,11 @@ type IssueAssignees struct {
 	IssueID    int64 `xorm:"INDEX"`
 }
 
+// LoadAssignees load assignees of this issue.
+func (issue *Issue) LoadAssignees() error {
+	return issue.loadAssignees(x)
+}
+
 // This loads all assignees of an issue
 func (issue *Issue) loadAssignees(e Engine) (err error) {
 	// Reset maybe preexisting assignees

models/issue_label.go

@@ -12,8 +12,6 @@ import (
 	"strconv"
 	"strings"
 
-	api "code.gitea.io/gitea/modules/structs"
-
 	"xorm.io/builder"
 	"xorm.io/xorm"
 )
@@ -37,16 +35,6 @@ type Label struct {
 	IsExcluded      bool   `xorm:"-"`
 }
 
-// APIFormat converts a Label to the api.Label format
-func (label *Label) APIFormat() *api.Label {
-	return &api.Label{
-		ID:          label.ID,
-		Name:        label.Name,
-		Color:       strings.TrimLeft(label.Color, "#"),
-		Description: label.Description,
-	}
-}
-
 // GetLabelTemplateFile loads the label template file by given name,
 // then parses and returns a list of name-color pairs and optionally description.
 func GetLabelTemplateFile(name string) ([][3]string, error) {

models/issue_label_test.go

@@ -8,23 +8,11 @@ import (
 	"html/template"
 	"testing"
 
-	api "code.gitea.io/gitea/modules/structs"
-
 	"github.com/stretchr/testify/assert"
 )
 
 // TODO TestGetLabelTemplateFile
 
-func TestLabel_APIFormat(t *testing.T) {
-	assert.NoError(t, PrepareTestDatabase())
-	label := AssertExistsAndLoadBean(t, &Label{ID: 1}).(*Label)
-	assert.Equal(t, api.Label{
-		ID:    label.ID,
-		Name:  label.Name,
-		Color: "abcdef",
-	}, *label.APIFormat())
-}
-
 func TestLabel_CalOpenIssues(t *testing.T) {
 	assert.NoError(t, PrepareTestDatabase())
 	label := AssertExistsAndLoadBean(t, &Label{ID: 1}).(*Label)

modules/convert/issue.go

@@ -5,10 +5,96 @@
 package convert
 
 import (
+	"strings"
+
 	"code.gitea.io/gitea/models"
 	api "code.gitea.io/gitea/modules/structs"
 )
 
+// ToAPIIssue converts an Issue to API format
+// it assumes some fields assigned with values:
+// Required - Poster, Labels,
+// Optional - Milestone, Assignee, PullRequest
+func ToAPIIssue(issue *models.Issue) *api.Issue {
+	if err := issue.LoadLabels(); err != nil {
+		return &api.Issue{}
+	}
+	if err := issue.LoadPoster(); err != nil {
+		return &api.Issue{}
+	}
+	if err := issue.LoadRepo(); err != nil {
+		return &api.Issue{}
+	}
+
+	apiIssue := &api.Issue{
+		ID:       issue.ID,
+		URL:      issue.APIURL(),
+		HTMLURL:  issue.HTMLURL(),
+		Index:    issue.Index,
+		Poster:   issue.Poster.APIFormat(),
+		Title:    issue.Title,
+		Body:     issue.Content,
+		Labels:   ToLabelList(issue.Labels),
+		State:    issue.State(),
+		Comments: issue.NumComments,
+		Created:  issue.CreatedUnix.AsTime(),
+		Updated:  issue.UpdatedUnix.AsTime(),
+	}
+
+	apiIssue.Repo = &api.RepositoryMeta{
+		ID:       issue.Repo.ID,
+		Name:     issue.Repo.Name,
+		Owner:    issue.Repo.OwnerName,
+		FullName: issue.Repo.FullName(),
+	}
+
+	if issue.ClosedUnix != 0 {
+		apiIssue.Closed = issue.ClosedUnix.AsTimePtr()
+	}
+
+	if err := issue.LoadMilestone(); err != nil {
+		return &api.Issue{}
+	}
+	if issue.Milestone != nil {
+		apiIssue.Milestone = issue.Milestone.APIFormat()
+	}
+
+	if err := issue.LoadAssignees(); err != nil {
+		return &api.Issue{}
+	}
+	if len(issue.Assignees) > 0 {
+		for _, assignee := range issue.Assignees {
+			apiIssue.Assignees = append(apiIssue.Assignees, assignee.APIFormat())
+		}
+		apiIssue.Assignee = issue.Assignees[0].APIFormat() // For compatibility, we're keeping the first assignee as `apiIssue.Assignee`
+	}
+	if issue.IsPull {
+		if err := issue.LoadPullRequest(); err != nil {
+			return &api.Issue{}
+		}
+		apiIssue.PullRequest = &api.PullRequestMeta{
+			HasMerged: issue.PullRequest.HasMerged,
+		}
+		if issue.PullRequest.HasMerged {
+			apiIssue.PullRequest.Merged = issue.PullRequest.MergedUnix.AsTimePtr()
+		}
+	}
+	if issue.DeadlineUnix != 0 {
+		apiIssue.Deadline = issue.DeadlineUnix.AsTimePtr()
+	}
+
+	return apiIssue
+}
+
+// ToAPIIssueList converts an IssueList to API format
+func ToAPIIssueList(il models.IssueList) []*api.Issue {
+	result := make([]*api.Issue, len(il))
+	for i := range il {
+		result[i] = ToAPIIssue(il[i])
+	}
+	return result
+}
+
 // ToTrackedTime converts TrackedTime to API format
 func ToTrackedTime(t *models.TrackedTime) (apiT *api.TrackedTime) {
 	apiT = &api.TrackedTime{
@@ -20,7 +106,7 @@ func ToTrackedTime(t *models.TrackedTime) (apiT *api.TrackedTime) {
 		Created:  t.Created,
 	}
 	if t.Issue != nil {
-		apiT.Issue = t.Issue.APIFormat()
+		apiT.Issue = ToAPIIssue(t.Issue)
 	}
 	if t.User != nil {
 		apiT.UserName = t.User.Name
@@ -36,3 +122,22 @@ func ToTrackedTimeList(tl models.TrackedTimeList) api.TrackedTimeList {
 	}
 	return result
 }
+
+// ToLabel converts Label to API format
+func ToLabel(label *models.Label) *api.Label {
+	return &api.Label{
+		ID:          label.ID,
+		Name:        label.Name,
+		Color:       strings.TrimLeft(label.Color, "#"),
+		Description: label.Description,
+	}
+}
+
+// ToLabelList converts list of Label to API format
+func ToLabelList(labels []*models.Label) []*api.Label {
+	result := make([]*api.Label, len(labels))
+	for i := range labels {
+		result[i] = ToLabel(labels[i])
+	}
+	return result
+}

modules/convert/issue_test.go

@@ -0,0 +1,24 @@
+// Copyright 2020 The Gitea Authors. All rights reserved.
+// Use of this source code is governed by a MIT-style
+// license that can be found in the LICENSE file.
+
+package convert
+
+import (
+	"testing"
+
+	"code.gitea.io/gitea/models"
+	api "code.gitea.io/gitea/modules/structs"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestLabel_ToLabel(t *testing.T) {
+	assert.NoError(t, models.PrepareTestDatabase())
+	label := models.AssertExistsAndLoadBean(t, &models.Label{ID: 1}).(*models.Label)
+	assert.Equal(t, &api.Label{
+		ID:    label.ID,
+		Name:  label.Name,
+		Color: "abcdef",
+	}, ToLabel(label))
+}

modules/convert/pull.go

@@ -31,7 +31,7 @@ func ToAPIPullRequest(pr *models.PullRequest) *api.PullRequest {
 		return nil
 	}
 
-	apiIssue := pr.Issue.APIFormat()
+	apiIssue := ToAPIIssue(pr.Issue)
 	if pr.BaseRepo == nil {
 		pr.BaseRepo, err = models.GetRepositoryByID(pr.BaseRepoID)
 		if err != nil {

modules/notification/webhook/webhook.go

@@ -59,7 +59,7 @@ func (m *webhookNotifier) NotifyIssueClearLabels(doer *models.User, issue *model
 		err = webhook_module.PrepareWebhooks(issue.Repo, models.HookEventIssues, &api.IssuePayload{
 			Action:     api.HookIssueLabelCleared,
 			Index:      issue.Index,
-			Issue:      issue.APIFormat(),
+			Issue:      convert.ToAPIIssue(issue),
 			Repository: issue.Repo.APIFormat(mode),
 			Sender:     doer.APIFormat(),
 		})
@@ -155,7 +155,7 @@ func (m *webhookNotifier) NotifyIssueChangeAssignee(doer *models.User, issue *mo
 		mode, _ := models.AccessLevelUnit(doer, issue.Repo, models.UnitTypeIssues)
 		apiIssue := &api.IssuePayload{
 			Index:      issue.Index,
-			Issue:      issue.APIFormat(),
+			Issue:      convert.ToAPIIssue(issue),
 			Repository: issue.Repo.APIFormat(mode),
 			Sender:     doer.APIFormat(),
 		}
@@ -202,7 +202,7 @@ func (m *webhookNotifier) NotifyIssueChangeTitle(doer *models.User, issue *model
 					From: oldTitle,
 				},
 			},
-			Issue:      issue.APIFormat(),
+			Issue:      convert.ToAPIIssue(issue),
 			Repository: issue.Repo.APIFormat(mode),
 			Sender:     issue.Poster.APIFormat(),
 		})
@@ -237,7 +237,7 @@ func (m *webhookNotifier) NotifyIssueChangeStatus(doer *models.User, issue *mode
 	} else {
 		apiIssue := &api.IssuePayload{
 			Index:      issue.Index,
-			Issue:      issue.APIFormat(),
+			Issue:      convert.ToAPIIssue(issue),
 			Repository: issue.Repo.APIFormat(mode),
 			Sender:     doer.APIFormat(),
 		}
@@ -267,7 +267,7 @@ func (m *webhookNotifier) NotifyNewIssue(issue *models.Issue) {
 	if err := webhook_module.PrepareWebhooks(issue.Repo, models.HookEventIssues, &api.IssuePayload{
 		Action:     api.HookIssueOpened,
 		Index:      issue.Index,
-		Issue:      issue.APIFormat(),
+		Issue:      convert.ToAPIIssue(issue),
 		Repository: issue.Repo.APIFormat(mode),
 		Sender:     issue.Poster.APIFormat(),
 	}); err != nil {
@@ -327,7 +327,7 @@ func (m *webhookNotifier) NotifyIssueChangeContent(doer *models.User, issue *mod
 					From: oldContent,
 				},
 			},
-			Issue:      issue.APIFormat(),
+			Issue:      convert.ToAPIIssue(issue),
 			Repository: issue.Repo.APIFormat(mode),
 			Sender:     doer.APIFormat(),
 		})
@@ -355,7 +355,7 @@ func (m *webhookNotifier) NotifyUpdateComment(doer *models.User, c *models.Comme
 	mode, _ := models.AccessLevel(doer, c.Issue.Repo)
 	if err := webhook_module.PrepareWebhooks(c.Issue.Repo, models.HookEventIssueComment, &api.IssueCommentPayload{
 		Action:  api.HookIssueCommentEdited,
-		Issue:   c.Issue.APIFormat(),
+		Issue:   convert.ToAPIIssue(c.Issue),
 		Comment: c.APIFormat(),
 		Changes: &api.ChangesPayload{
 			Body: &api.ChangesFromPayload{
@@ -375,7 +375,7 @@ func (m *webhookNotifier) NotifyCreateIssueComment(doer *models.User, repo *mode
 	mode, _ := models.AccessLevel(doer, repo)
 	if err := webhook_module.PrepareWebhooks(repo, models.HookEventIssueComment, &api.IssueCommentPayload{
 		Action:     api.HookIssueCommentCreated,
-		Issue:      issue.APIFormat(),
+		Issue:      convert.ToAPIIssue(issue),
 		Comment:    comment.APIFormat(),
 		Repository: repo.APIFormat(mode),
 		Sender:     doer.APIFormat(),
@@ -404,7 +404,7 @@ func (m *webhookNotifier) NotifyDeleteComment(doer *models.User, comment *models
 
 	if err := webhook_module.PrepareWebhooks(comment.Issue.Repo, models.HookEventIssueComment, &api.IssueCommentPayload{
 		Action:     api.HookIssueCommentDeleted,
-		Issue:      comment.Issue.APIFormat(),
+		Issue:      convert.ToAPIIssue(comment.Issue),
 		Comment:    comment.APIFormat(),
 		Repository: comment.Issue.Repo.APIFormat(mode),
 		Sender:     doer.APIFormat(),
@@ -449,7 +449,7 @@ func (m *webhookNotifier) NotifyIssueChangeLabels(doer *models.User, issue *mode
 		err = webhook_module.PrepareWebhooks(issue.Repo, models.HookEventIssues, &api.IssuePayload{
 			Action:     api.HookIssueLabelUpdated,
 			Index:      issue.Index,
-			Issue:      issue.APIFormat(),
+			Issue:      convert.ToAPIIssue(issue),
 			Repository: issue.Repo.APIFormat(mode),
 			Sender:     doer.APIFormat(),
 		})
@@ -491,7 +491,7 @@ func (m *webhookNotifier) NotifyIssueChangeMilestone(doer *models.User, issue *m
 		err = webhook_module.PrepareWebhooks(issue.Repo, models.HookEventIssues, &api.IssuePayload{
 			Action:     hookAction,
 			Index:      issue.Index,
-			Issue:      issue.APIFormat(),
+			Issue:      convert.ToAPIIssue(issue),
 			Repository: issue.Repo.APIFormat(mode),
 			Sender:     doer.APIFormat(),
 		})

routers/api/v1/repo/issue.go

@@ -13,6 +13,7 @@ import (
 
 	"code.gitea.io/gitea/models"
 	"code.gitea.io/gitea/modules/context"
+	"code.gitea.io/gitea/modules/convert"
 	issue_indexer "code.gitea.io/gitea/modules/indexer/issues"
 	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/setting"
@@ -168,13 +169,8 @@ func SearchIssues(ctx *context.APIContext) {
 		return
 	}
 
-	apiIssues := make([]*api.Issue, len(issues))
-	for i := range issues {
-		apiIssues[i] = issues[i].APIFormat()
-	}
-
 	ctx.SetLinkHeader(issueCount, setting.UI.IssuePagingNum)
-	ctx.JSON(http.StatusOK, &apiIssues)
+	ctx.JSON(http.StatusOK, convert.ToAPIIssueList(issues))
 }
 
 // ListIssues list the issues of a repository
@@ -287,13 +283,8 @@ func ListIssues(ctx *context.APIContext) {
 		return
 	}
 
-	apiIssues := make([]*api.Issue, len(issues))
-	for i := range issues {
-		apiIssues[i] = issues[i].APIFormat()
-	}
-
 	ctx.SetLinkHeader(ctx.Repo.Repository.NumIssues, listOptions.PageSize)
-	ctx.JSON(http.StatusOK, &apiIssues)
+	ctx.JSON(http.StatusOK, convert.ToAPIIssueList(issues))
 }
 
 // GetIssue get an issue of a repository
@@ -335,7 +326,7 @@ func GetIssue(ctx *context.APIContext) {
 		}
 		return
 	}
-	ctx.JSON(http.StatusOK, issue.APIFormat())
+	ctx.JSON(http.StatusOK, convert.ToAPIIssue(issue))
 }
 
 // CreateIssue create an issue of a repository
@@ -450,7 +441,7 @@ func CreateIssue(ctx *context.APIContext, form api.CreateIssueOption) {
 		ctx.Error(http.StatusInternalServerError, "GetIssueByID", err)
 		return
 	}
-	ctx.JSON(http.StatusCreated, issue.APIFormat())
+	ctx.JSON(http.StatusCreated, convert.ToAPIIssue(issue))
 }
 
 // EditIssue modify an issue of a repository
@@ -596,7 +587,7 @@ func EditIssue(ctx *context.APIContext, form api.EditIssueOption) {
 		ctx.InternalServerError(err)
 		return
 	}
-	ctx.JSON(http.StatusCreated, issue.APIFormat())
+	ctx.JSON(http.StatusCreated, convert.ToAPIIssue(issue))
 }
 
 // UpdateIssueDeadline updates an issue deadline

routers/api/v1/repo/issue_label.go

@@ -10,6 +10,7 @@ import (
 
 	"code.gitea.io/gitea/models"
 	"code.gitea.io/gitea/modules/context"
+	"code.gitea.io/gitea/modules/convert"
 	api "code.gitea.io/gitea/modules/structs"
 	issue_service "code.gitea.io/gitea/services/issue"
 )
@@ -59,11 +60,7 @@ func ListIssueLabels(ctx *context.APIContext) {
 		return
 	}
 
-	apiLabels := make([]*api.Label, len(issue.Labels))
+	ctx.JSON(http.StatusOK, convert.ToLabelList(issue.Labels))
-	for i := range issue.Labels {
-		apiLabels[i] = issue.Labels[i].APIFormat()
-	}
-	ctx.JSON(http.StatusOK, &apiLabels)
 }
 
 // AddIssueLabels add labels for an issue
@@ -118,11 +115,7 @@ func AddIssueLabels(ctx *context.APIContext, form api.IssueLabelsOption) {
 		return
 	}
 
-	apiLabels := make([]*api.Label, len(labels))
+	ctx.JSON(http.StatusOK, convert.ToLabelList(labels))
-	for i := range labels {
-		apiLabels[i] = labels[i].APIFormat()
-	}
-	ctx.JSON(http.StatusOK, &apiLabels)
 }
 
 // DeleteIssueLabel delete a label for an issue
@@ -248,11 +241,7 @@ func ReplaceIssueLabels(ctx *context.APIContext, form api.IssueLabelsOption) {
 		return
 	}
 
-	apiLabels := make([]*api.Label, len(labels))
+	ctx.JSON(http.StatusOK, convert.ToLabelList(labels))
-	for i := range labels {
-		apiLabels[i] = labels[i].APIFormat()
-	}
-	ctx.JSON(http.StatusOK, &apiLabels)
 }
 
 // ClearIssueLabels delete all the labels for an issue

routers/api/v1/repo/label.go

@@ -13,6 +13,7 @@ import (
 
 	"code.gitea.io/gitea/models"
 	"code.gitea.io/gitea/modules/context"
+	"code.gitea.io/gitea/modules/convert"
 	api "code.gitea.io/gitea/modules/structs"
 	"code.gitea.io/gitea/routers/api/v1/utils"
 )
@@ -53,11 +54,7 @@ func ListLabels(ctx *context.APIContext) {
 		return
 	}
 
-	apiLabels := make([]*api.Label, len(labels))
+	ctx.JSON(http.StatusOK, convert.ToLabelList(labels))
-	for i := range labels {
-		apiLabels[i] = labels[i].APIFormat()
-	}
-	ctx.JSON(http.StatusOK, &apiLabels)
 }
 
 // GetLabel get label by repository and label id
@@ -107,7 +104,7 @@ func GetLabel(ctx *context.APIContext) {
 		return
 	}
 
-	ctx.JSON(http.StatusOK, label.APIFormat())
+	ctx.JSON(http.StatusOK, convert.ToLabel(label))
 }
 
 // CreateLabel create a label for a repository
@@ -159,7 +156,7 @@ func CreateLabel(ctx *context.APIContext, form api.CreateLabelOption) {
 		ctx.Error(http.StatusInternalServerError, "NewLabel", err)
 		return
 	}
-	ctx.JSON(http.StatusCreated, label.APIFormat())
+	ctx.JSON(http.StatusCreated, convert.ToLabel(label))
 }
 
 // EditLabel modify a label for a repository
@@ -228,7 +225,7 @@ func EditLabel(ctx *context.APIContext, form api.EditLabelOption) {
 		ctx.ServerError("UpdateLabel", err)
 		return
 	}
-	ctx.JSON(http.StatusOK, label.APIFormat())
+	ctx.JSON(http.StatusOK, convert.ToLabel(label))
 }
 
 // DeleteLabel delete a label for a repository