[Refactor] CombinedStatus and CommitStatus related functions & structs (#14026)

* RM unused struct

* rename (*CommitStatus) loadRepo() -> loadAttributes()

* move ToCommitStatus into its own file

* use CommitStatusState instead of StatusState

* move CombinedStatus convertion into convert package

* let models.GetLatestCommitStatus use repoID direct and accept ListOptions

* update swagger docs

* fix tests

* Fix swagger docs

* rm page

* fix swagger docs!!!

* return json null

* always return json

* rename api.Status to api.CommitStatus

* fix swagger docs

* sec swagger fix

Author: 6543

integrations/pull_status_test.go

@@ -70,7 +70,7 @@ func TestPullCreate_CommitStatus(t *testing.T) {
 			token := getTokenForLoggedInUser(t, session)
 			req = NewRequestWithJSON(t, "POST", fmt.Sprintf("/api/v1/repos/user1/repo1/statuses/%s?token=%s", commitID, token),
 				api.CreateStatusOption{
-					State:       api.StatusState(status),
+					State:       status,
 					TargetURL:   "http://test.ci/",
 					Description: "",
 					Context:     "testci",

integrations/repo_commits_test.go

@@ -51,7 +51,7 @@ func doTestRepoCommitWithStatus(t *testing.T, state string, classes ...string) {
 	// Call API to add status for commit
 	req = NewRequestWithJSON(t, "POST", "/api/v1/repos/user2/repo1/statuses/"+path.Base(commitURL)+"?token="+token,
 		api.CreateStatusOption{
-			State:       api.StatusState(state),
+			State:       api.CommitStatusState(state),
 			TargetURL:   "http://test.ci/",
 			Description: "",
 			Context:     "testci",
@@ -83,11 +83,11 @@ func doTestRepoCommitWithStatus(t *testing.T, state string, classes ...string) {
 
 func testRepoCommitsWithStatus(t *testing.T, resp *httptest.ResponseRecorder, state string) {
 	decoder := json.NewDecoder(resp.Body)
-	statuses := []*api.Status{}
+	statuses := []*api.CommitStatus{}
 	assert.NoError(t, decoder.Decode(&statuses))
 	assert.Len(t, statuses, 1)
 	for _, s := range statuses {
-		assert.Equal(t, api.StatusState(state), s.State)
+		assert.Equal(t, api.CommitStatusState(state), s.State)
 		assert.Equal(t, setting.AppURL+"api/v1/repos/user2/repo1/statuses/65f1bf27bc3bf70f64657658635e66094edbcb4d", s.URL)
 		assert.Equal(t, "http://test.ci/", s.TargetURL)
 		assert.Equal(t, "", s.Description)

models/commit_status.go

@@ -38,7 +38,7 @@ type CommitStatus struct {
 	UpdatedUnix timeutil.TimeStamp `xorm:"INDEX updated"`
 }
 
-func (status *CommitStatus) loadRepo(e Engine) (err error) {
+func (status *CommitStatus) loadAttributes(e Engine) (err error) {
 	if status.Repo == nil {
 		status.Repo, err = getRepositoryByID(e, status.RepoID)
 		if err != nil {
@@ -56,7 +56,7 @@ func (status *CommitStatus) loadRepo(e Engine) (err error) {
 
 // APIURL returns the absolute APIURL to this commit-status.
 func (status *CommitStatus) APIURL() string {
-	_ = status.loadRepo(x)
+	_ = status.loadAttributes(x)
 	return fmt.Sprintf("%sapi/v1/repos/%s/statuses/%s",
 		setting.AppURL, status.Repo.FullName(), status.SHA)
 }
@@ -139,13 +139,20 @@ func sortCommitStatusesSession(sess *xorm.Session, sortType string) {
 }
 
 // GetLatestCommitStatus returns all statuses with a unique context for a given commit.
-func GetLatestCommitStatus(repo *Repository, sha string, page int) ([]*CommitStatus, error) {
+func GetLatestCommitStatus(repoID int64, sha string, listOptions ListOptions) ([]*CommitStatus, error) {
+	return getLatestCommitStatus(x, repoID, sha, listOptions)
+}
+
+func getLatestCommitStatus(e Engine, repoID int64, sha string, listOptions ListOptions) ([]*CommitStatus, error) {
 	ids := make([]int64, 0, 10)
-	err := x.Limit(10, page*10).
-		Table(&CommitStatus{}).
-		Where("repo_id = ?", repo.ID).And("sha = ?", sha).
+	sess := e.Table(&CommitStatus{}).
+		Where("repo_id = ?", repoID).And("sha = ?", sha).
 		Select("max( id ) as id").
-		GroupBy("context_hash").OrderBy("max( id ) desc").Find(&ids)
+		GroupBy("context_hash").OrderBy("max( id ) desc")
+
+	sess = listOptions.setSessionPagination(sess)
+
+	err := sess.Find(&ids)
 	if err != nil {
 		return nil, err
 	}
@@ -261,7 +268,7 @@ func ParseCommitsWithStatus(oldCommits *list.List, repo *Repository) *list.List
 		commit := SignCommitWithStatuses{
 			SignCommit: &c,
 		}
-		statuses, err := GetLatestCommitStatus(repo, commit.ID.String(), 0)
+		statuses, err := GetLatestCommitStatus(repo.ID, commit.ID.String(), ListOptions{})
 		if err != nil {
 			log.Error("GetLatestCommitStatus: %v", err)
 		} else {

modules/convert/convert.go

@@ -347,27 +347,6 @@ func ToOAuth2Application(app *models.OAuth2Application) *api.OAuth2Application {
 	}
 }
 
-// ToCommitStatus converts models.CommitStatus to api.Status
-func ToCommitStatus(status *models.CommitStatus) *api.Status {
-	apiStatus := &api.Status{
-		Created:     status.CreatedUnix.AsTime(),
-		Updated:     status.CreatedUnix.AsTime(),
-		State:       api.StatusState(status.State),
-		TargetURL:   status.TargetURL,
-		Description: status.Description,
-		ID:          status.Index,
-		URL:         status.APIURL(),
-		Context:     status.Context,
-	}
-
-	if status.CreatorID != 0 {
-		creator, _ := models.GetUserByID(status.CreatorID)
-		apiStatus.Creator = ToUser(creator, false, false)
-	}
-
-	return apiStatus
-}
-
 // ToLFSLock convert a LFSLock to api.LFSLock
 func ToLFSLock(l *models.LFSLock) *api.LFSLock {
 	return &api.LFSLock{

modules/convert/status.go

@@ -0,0 +1,56 @@
+// 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 (
+	"code.gitea.io/gitea/models"
+	api "code.gitea.io/gitea/modules/structs"
+)
+
+// ToCommitStatus converts models.CommitStatus to api.CommitStatus
+func ToCommitStatus(status *models.CommitStatus) *api.CommitStatus {
+	apiStatus := &api.CommitStatus{
+		Created:     status.CreatedUnix.AsTime(),
+		Updated:     status.CreatedUnix.AsTime(),
+		State:       status.State,
+		TargetURL:   status.TargetURL,
+		Description: status.Description,
+		ID:          status.Index,
+		URL:         status.APIURL(),
+		Context:     status.Context,
+	}
+
+	if status.CreatorID != 0 {
+		creator, _ := models.GetUserByID(status.CreatorID)
+		apiStatus.Creator = ToUser(creator, false, false)
+	}
+
+	return apiStatus
+}
+
+// ToCombinedStatus converts List of CommitStatus to a CombinedStatus
+func ToCombinedStatus(statuses []*models.CommitStatus, repo *api.Repository) *api.CombinedStatus {
+
+	if len(statuses) == 0 {
+		return nil
+	}
+
+	retStatus := &api.CombinedStatus{
+		SHA:        statuses[0].SHA,
+		TotalCount: len(statuses),
+		Repository: repo,
+		URL:        "",
+	}
+
+	retStatus.Statuses = make([]*api.CommitStatus, 0, len(statuses))
+	for _, status := range statuses {
+		retStatus.Statuses = append(retStatus.Statuses, ToCommitStatus(status))
+		if status.State.NoBetterThan(retStatus.State) {
+			retStatus.State = status.State
+		}
+	}
+
+	return retStatus
+}

modules/gitgraph/graph_models.go

@@ -113,7 +113,7 @@ func (graph *Graph) LoadAndProcessCommits(repository *models.Repository, gitRepo
 
 		_ = models.CalculateTrustStatus(c.Verification, repository, &keyMap)
 
-		statuses, err := models.GetLatestCommitStatus(repository, c.Commit.ID.String(), 0)
+		statuses, err := models.GetLatestCommitStatus(repository.ID, c.Commit.ID.String(), models.ListOptions{})
 		if err != nil {
 			log.Error("GetLatestCommitStatus: %v", err)
 		} else {

modules/structs/commit_status.go

@@ -4,20 +4,20 @@
 
 package structs
 
-// CommitStatusState holds the state of a Status
+// CommitStatusState holds the state of a CommitStatus
 // It can be "pending", "success", "error", "failure", and "warning"
 type CommitStatusState string
 
 const (
-	// CommitStatusPending is for when the Status is Pending
+	// CommitStatusPending is for when the CommitStatus is Pending
 	CommitStatusPending CommitStatusState = "pending"
-	// CommitStatusSuccess is for when the Status is Success
+	// CommitStatusSuccess is for when the CommitStatus is Success
 	CommitStatusSuccess CommitStatusState = "success"
-	// CommitStatusError is for when the Status is Error
+	// CommitStatusError is for when the CommitStatus is Error
 	CommitStatusError CommitStatusState = "error"
-	// CommitStatusFailure is for when the Status is Failure
+	// CommitStatusFailure is for when the CommitStatus is Failure
 	CommitStatusFailure CommitStatusState = "failure"
-	// CommitStatusWarning is for when the Status is Warning
+	// CommitStatusWarning is for when the CommitStatus is Warning
 	CommitStatusWarning CommitStatusState = "warning"
 )
 

modules/structs/status.go

@@ -8,32 +8,15 @@ import (
 	"time"
 )
 
-// StatusState holds the state of a Status
-// It can be "pending", "success", "error", "failure", and "warning"
-type StatusState string
-
-const (
-	// StatusPending is for when the Status is Pending
-	StatusPending StatusState = "pending"
-	// StatusSuccess is for when the Status is Success
-	StatusSuccess StatusState = "success"
-	// StatusError is for when the Status is Error
-	StatusError StatusState = "error"
-	// StatusFailure is for when the Status is Failure
-	StatusFailure StatusState = "failure"
-	// StatusWarning is for when the Status is Warning
-	StatusWarning StatusState = "warning"
-)
-
-// Status holds a single Status of a single Commit
-type Status struct {
-	ID          int64       `json:"id"`
-	State       StatusState `json:"status"`
-	TargetURL   string      `json:"target_url"`
-	Description string      `json:"description"`
-	URL         string      `json:"url"`
-	Context     string      `json:"context"`
-	Creator     *User       `json:"creator"`
+// CommitStatus holds a single status of a single Commit
+type CommitStatus struct {
+	ID          int64             `json:"id"`
+	State       CommitStatusState `json:"status"`
+	TargetURL   string            `json:"target_url"`
+	Description string            `json:"description"`
+	URL         string            `json:"url"`
+	Context     string            `json:"context"`
+	Creator     *User             `json:"creator"`
 	// swagger:strfmt date-time
 	Created time.Time `json:"created_at"`
 	// swagger:strfmt date-time
@@ -42,24 +25,19 @@ type Status struct {
 
 // CombinedStatus holds the combined state of several statuses for a single commit
 type CombinedStatus struct {
-	State      StatusState `json:"state"`
-	SHA        string      `json:"sha"`
-	TotalCount int         `json:"total_count"`
-	Statuses   []*Status   `json:"statuses"`
-	Repository *Repository `json:"repository"`
-	CommitURL  string      `json:"commit_url"`
-	URL        string      `json:"url"`
+	State      CommitStatusState `json:"state"`
+	SHA        string            `json:"sha"`
+	TotalCount int               `json:"total_count"`
+	Statuses   []*CommitStatus   `json:"statuses"`
+	Repository *Repository       `json:"repository"`
+	CommitURL  string            `json:"commit_url"`
+	URL        string            `json:"url"`
 }
 
-// CreateStatusOption holds the information needed to create a new Status for a Commit
+// CreateStatusOption holds the information needed to create a new CommitStatus for a Commit
 type CreateStatusOption struct {
-	State       StatusState `json:"state"`
-	TargetURL   string      `json:"target_url"`
-	Description string      `json:"description"`
-	Context     string      `json:"context"`
-}
-
-// ListStatusesOption holds pagination information
-type ListStatusesOption struct {
-	Page int
+	State       CommitStatusState `json:"state"`
+	TargetURL   string            `json:"target_url"`
+	Description string            `json:"description"`
+	Context     string            `json:"context"`
 }

routers/api/v1/repo/status.go

@@ -45,7 +45,7 @@ func NewCommitStatus(ctx *context.APIContext, form api.CreateStatusOption) {
 	//     "$ref": "#/definitions/CreateStatusOption"
 	// responses:
 	//   "201":
-	//     "$ref": "#/responses/Status"
+	//     "$ref": "#/responses/CommitStatus"
 	//   "400":
 	//     "$ref": "#/responses/error"
 
@@ -113,7 +113,7 @@ func GetCommitStatuses(ctx *context.APIContext) {
 	//   type: integer
 	// responses:
 	//   "200":
-	//     "$ref": "#/responses/StatusList"
+	//     "$ref": "#/responses/CommitStatusList"
 	//   "400":
 	//     "$ref": "#/responses/error"
 
@@ -165,7 +165,7 @@ func GetCommitStatusesByRef(ctx *context.APIContext) {
 	//   type: integer
 	// responses:
 	//   "200":
-	//     "$ref": "#/responses/StatusList"
+	//     "$ref": "#/responses/CommitStatusList"
 	//   "400":
 	//     "$ref": "#/responses/error"
 
@@ -221,7 +221,7 @@ func getCommitStatuses(ctx *context.APIContext, sha string) {
 		return
 	}
 
-	apiStatuses := make([]*api.Status, 0, len(statuses))
+	apiStatuses := make([]*api.CommitStatus, 0, len(statuses))
 	for _, status := range statuses {
 		apiStatuses = append(apiStatuses, convert.ToCommitStatus(status))
 	}
@@ -233,19 +233,9 @@ func getCommitStatuses(ctx *context.APIContext, sha string) {
 	ctx.JSON(http.StatusOK, apiStatuses)
 }
 
-type combinedCommitStatus struct {
-	State      api.CommitStatusState `json:"state"`
-	SHA        string                `json:"sha"`
-	TotalCount int                   `json:"total_count"`
-	Statuses   []*api.Status         `json:"statuses"`
-	Repo       *api.Repository       `json:"repository"`
-	CommitURL  string                `json:"commit_url"`
-	URL        string                `json:"url"`
-}
-
 // GetCombinedCommitStatusByRef returns the combined status for any given commit hash
 func GetCombinedCommitStatusByRef(ctx *context.APIContext) {
-	// swagger:operation GET /repos/{owner}/{repo}/commits/{ref}/statuses repository repoGetCombinedStatusByRef
+	// swagger:operation GET /repos/{owner}/{repo}/commits/{ref}/status repository repoGetCombinedStatusByRef
 	// ---
 	// summary: Get a commit's combined status, by branch/tag/commit reference
 	// produces:
@@ -268,12 +258,15 @@ func GetCombinedCommitStatusByRef(ctx *context.APIContext) {
 	//   required: true
 	// - name: page
 	//   in: query
-	//   description: page number of results
+	//   description: page number of results to return (1-based)
+	//   type: integer
+	// - name: limit
+	//   in: query
+	//   description: page size of results
 	//   type: integer
-	//   required: false
 	// responses:
 	//   "200":
-	//     "$ref": "#/responses/Status"
+	//     "$ref": "#/responses/CombinedStatus"
 	//   "400":
 	//     "$ref": "#/responses/error"
 
@@ -284,33 +277,18 @@ func GetCombinedCommitStatusByRef(ctx *context.APIContext) {
 	}
 	repo := ctx.Repo.Repository
 
-	page := ctx.QueryInt("page")
-
-	statuses, err := models.GetLatestCommitStatus(repo, sha, page)
+	statuses, err := models.GetLatestCommitStatus(repo.ID, sha, utils.GetListOptions(ctx))
 	if err != nil {
-		ctx.Error(http.StatusInternalServerError, "GetLatestCommitStatus", fmt.Errorf("GetLatestCommitStatus[%s, %s, %d]: %v", repo.FullName(), sha, page, err))
+		ctx.Error(http.StatusInternalServerError, "GetLatestCommitStatus", fmt.Errorf("GetLatestCommitStatus[%s, %s]: %v", repo.FullName(), sha, err))
 		return
 	}
 
 	if len(statuses) == 0 {
-		ctx.Status(http.StatusOK)
+		ctx.JSON(http.StatusOK, &api.CombinedStatus{})
 		return
 	}
 
-	retStatus := &combinedCommitStatus{
-		SHA:        sha,
-		TotalCount: len(statuses),
-		Repo:       convert.ToRepo(repo, ctx.Repo.AccessMode),
-		URL:        "",
-	}
-
-	retStatus.Statuses = make([]*api.Status, 0, len(statuses))
-	for _, status := range statuses {
-		retStatus.Statuses = append(retStatus.Statuses, convert.ToCommitStatus(status))
-		if status.State.NoBetterThan(retStatus.State) {
-			retStatus.State = status.State
-		}
-	}
+	combiStatus := convert.ToCombinedStatus(statuses, convert.ToRepo(repo, ctx.Repo.AccessMode))
 
-	ctx.JSON(http.StatusOK, retStatus)
+	ctx.JSON(http.StatusOK, combiStatus)
 }