feat: [CODE-3030]: introduce include_git_stats param for list PRs APIs (

#3284) * introduce include_git_stats param for list PRs APIs
harness · Jan 17, 2025 · f016ac9 · f016ac9
1 parent 57a071c
commit f016ac9
Show file tree

Hide file tree

Showing 11 changed files with 112 additions and 108 deletions.
diff --git a/app/api/controller/pullreq/pr_find.go b/app/api/controller/pullreq/pr_find.go
@@ -22,8 +22,6 @@ import (
 	"github.com/harness/gitness/app/auth"
 	"github.com/harness/gitness/types"
 	"github.com/harness/gitness/types/enum"
-
-	"github.com/rs/zerolog/log"
 )
 
 // Find returns a pull request from the provided repository.
@@ -57,10 +55,6 @@ func (c *Controller) Find(
 		return nil, fmt.Errorf("failed to backfill pull request metadata: %w", err)
 	}
 
-	if err := c.pullreqListService.BackfillStats(ctx, pr); err != nil {
-		log.Ctx(ctx).Warn().Err(err).Msg("failed to backfill PR stats")
-	}
-
 	return pr, nil
 }
 

diff --git a/app/api/controller/pullreq/pr_list.go b/app/api/controller/pullreq/pr_list.go
@@ -22,8 +22,6 @@ import (
 	"github.com/harness/gitness/store/database/dbtx"
 	"github.com/harness/gitness/types"
 	"github.com/harness/gitness/types/enum"
-
-	"github.com/rs/zerolog/log"
 )
 
 // List returns a list of pull requests from the provided repository.
@@ -85,11 +83,5 @@ func (c *Controller) List(
 		return nil, 0, fmt.Errorf("failed to backfill metadata for pull requests: %w", err)
 	}
 
-	for _, pr := range list {
-		if err := c.pullreqListService.BackfillStats(ctx, pr); err != nil {
-			log.Ctx(ctx).Warn().Err(err).Msg("failed to backfill PR stats")
-		}
-	}
-
 	return list, count, nil
 }
diff --git a/app/api/handler/pullreq/pr_find.go b/app/api/handler/pullreq/pr_find.go
@@ -46,6 +46,8 @@ func HandleFind(pullreqCtrl *pullreq.Controller) http.HandlerFunc {
 			return
 		}
 
+		options.IncludeGitStats = true // always backfill PR git stats when fetching one PR.
+
 		pr, err := pullreqCtrl.Find(ctx, session, repoRef, pullreqNumber, options)
 		if err != nil {
 			render.TranslatedUserError(ctx, w, err)
@@ -88,6 +90,8 @@ func HandleFindByBranches(pullreqCtrl *pullreq.Controller) http.HandlerFunc {
 			return
 		}
 
+		options.IncludeGitStats = true // always backfill PR git stats when fetching one PR.
+
 		pr, err := pullreqCtrl.FindByBranches(ctx, session, repoRef, sourceRepoRef, sourceBranch, targetBranch, options)
 		if err != nil {
 			render.TranslatedUserError(ctx, w, err)

diff --git a/app/api/handler/pullreq/pr_metadata.go b/app/api/handler/pullreq/pr_metadata.go
@@ -41,7 +41,11 @@ func HandleMetadata(pullreqCtrl *pullreq.Controller) http.HandlerFunc {
 			return
 		}
 
-		pr, err := pullreqCtrl.Find(ctx, session, repoRef, pullreqNumber, types.PullReqMetadataOptions{})
+		options := types.PullReqMetadataOptions{
+			IncludeGitStats: true,
+		}
+
+		pr, err := pullreqCtrl.Find(ctx, session, repoRef, pullreqNumber, options)
 		if err != nil {
 			render.TranslatedUserError(ctx, w, err)
 			return

diff --git a/app/api/openapi/pullreq.go b/app/api/openapi/pullreq.go
@@ -505,7 +505,7 @@ func pullReqOperations(reflector *openapi3.Reflector) {
 		QueryParameterLabelID, QueryParameterValueID,
 		queryParameterAuthorID, queryParameterCommenterID, queryParameterMentionedID,
 		queryParameterReviewerID, queryParameterReviewDecision,
-		queryParameterIncludeChecks, queryParameterIncludeRules)
+		queryParamIncludeGitStats, queryParameterIncludeChecks, queryParameterIncludeRules)
 	_ = reflector.SetRequest(&listPullReq, new(listPullReqRequest), http.MethodGet)
 	_ = reflector.SetJSONResponse(&listPullReq, new([]types.PullReq), http.StatusOK)
 	_ = reflector.SetJSONResponse(&listPullReq, new(usererror.Error), http.StatusBadRequest)

diff --git a/app/api/openapi/repo.go b/app/api/openapi/repo.go
@@ -301,6 +301,22 @@ var queryParameterIncludeCommit = openapi3.ParameterOrRef{
 	},
 }
 
+var queryParamIncludeGitStats = openapi3.ParameterOrRef{
+	Parameter: &openapi3.Parameter{
+		Name: request.QueryParamIncludeStats,
+		In:   openapi3.ParameterInQuery,
+		Description: ptr.String(
+			"If true, the git diff stats would be included in the response."),
+		Required: ptr.Bool(false),
+		Schema: &openapi3.SchemaOrRef{
+			Schema: &openapi3.Schema{
+				Type:    ptrSchemaType(openapi3.SchemaTypeBoolean),
+				Default: ptrptr(false),
+			},
+		},
+	},
+}
+
 var queryParameterIncludeChecks = openapi3.ParameterOrRef{
 	Parameter: &openapi3.Parameter{
 		Name: request.QueryParamIncludeChecks,

diff --git a/app/api/openapi/space.go b/app/api/openapi/space.go
@@ -671,7 +671,7 @@ func spaceOperations(reflector *openapi3.Reflector) {
 		QueryParameterLabelID, QueryParameterValueID,
 		queryParameterAuthorID, queryParameterCommenterID, queryParameterMentionedID,
 		queryParameterReviewerID, queryParameterReviewDecision,
-		queryParameterIncludeChecks, queryParameterIncludeRules)
+		queryParamIncludeGitStats, queryParameterIncludeChecks, queryParameterIncludeRules)
 	_ = reflector.SetRequest(&listPullReq, new(listPullReqRequest), http.MethodGet)
 	_ = reflector.SetJSONResponse(&listPullReq, new([]types.PullReqRepo), http.StatusOK)
 	_ = reflector.SetJSONResponse(&listPullReq, new(usererror.Error), http.StatusBadRequest)

diff --git a/app/api/request/git.go b/app/api/request/git.go
@@ -50,6 +50,7 @@ const (
 	QueryParamService      = "service"
 	QueryParamCommitSHA    = "commit_sha"
 
+	QueryParamIncludeGitStats = "include_git_stats"
 	QueryParamIncludeChecks   = "include_checks"
 	QueryParamIncludeRules    = "include_rules"
 	QueryParamIncludePullReqs = "include_pullreqs"
@@ -64,6 +65,14 @@ func GetIncludeCommitFromQueryOrDefault(r *http.Request, deflt bool) (bool, erro
 	return QueryParamAsBoolOrDefault(r, QueryParamIncludeCommit, deflt)
 }
 
+func GetIncludeStatsFromQueryOrDefault(r *http.Request, deflt bool) (bool, error) {
+	return QueryParamAsBoolOrDefault(r, QueryParamIncludeStats, deflt)
+}
+
+func GetIncludeGitStatsFromQueryOrDefault(r *http.Request, deflt bool) (bool, error) {
+	return QueryParamAsBoolOrDefault(r, QueryParamIncludeGitStats, deflt)
+}
+
 func GetIncludeChecksFromQueryOrDefault(r *http.Request, deflt bool) (bool, error) {
 	return QueryParamAsBoolOrDefault(r, QueryParamIncludeChecks, deflt)
 }
@@ -181,7 +190,7 @@ func ParseCommitFilter(r *http.Request) (*types.CommitFilter, error) {
 	if err != nil {
 		return nil, err
 	}
-	includeStats, err := QueryParamAsBoolOrDefault(r, QueryParamIncludeStats, false)
+	includeStats, err := GetIncludeStatsFromQueryOrDefault(r, false)
 	if err != nil {
 		return nil, err
 	}

diff --git a/app/api/request/pullreq.go b/app/api/request/pullreq.go
@@ -111,6 +111,15 @@ func parseReviewDecisions(r *http.Request) []enum.PullReqReviewDecision {
 }
 
 func ParsePullReqMetadataOptions(r *http.Request) (types.PullReqMetadataOptions, error) {
+	// TODO: Remove the "includeGitStats := true" line and uncomment the following code block.
+	// Because introduction of "include_git_stats" parameter is a breaking API change,
+	// we should remove this line only after other teams who need PR stats include the include_stats=true in API calls.
+	includeGitStats := true
+	/*includeGitStats, err := GetIncludeGitStatsFromQueryOrDefault(r, false)
+	if err != nil {
+		return types.PullReqMetadataOptions{}, err
+	}*/
+
 	includeChecks, err := GetIncludeChecksFromQueryOrDefault(r, false)
 	if err != nil {
 		return types.PullReqMetadataOptions{}, err
@@ -122,8 +131,9 @@ func ParsePullReqMetadataOptions(r *http.Request) (types.PullReqMetadataOptions,
 	}
 
 	return types.PullReqMetadataOptions{
-		IncludeChecks: includeChecks,
-		IncludeRules:  includeRules,
+		IncludeGitStats: includeGitStats,
+		IncludeChecks:   includeChecks,
+		IncludeRules:    includeRules,
 	}, nil
 }
 

diff --git a/app/services/pullreq/service_list.go b/app/services/pullreq/service_list.go
@@ -160,12 +160,6 @@ func (c *ListService) ListForSpace(
 		return nil, fmt.Errorf("failed to backfill labels assigned to pull requests: %w", err)
 	}
 
-	for _, pr := range list {
-		if err := c.BackfillStats(ctx, pr); err != nil {
-			log.Ctx(ctx).Warn().Err(err).Msg("failed to backfill PR stats")
-		}
-	}
-
 	response := make([]types.PullReqRepo, len(list))
 	for i := range list {
 		response[i] = types.PullReqRepo{
@@ -216,33 +210,46 @@ func (c *ListService) streamPullReqs(
 	return pullReqs, repoUnchecked, nil
 }
 
-func (c *ListService) BackfillStats(ctx context.Context, pr *types.PullReq) error {
-	s := pr.Stats.DiffStats
-	if s.Commits != nil && s.FilesChanged != nil && s.Additions != nil && s.Deletions != nil {
-		return nil
+func clearStats(list []types.PullReqRepo) {
+	for _, entry := range list {
+		entry.PullRequest.Stats.DiffStats = types.DiffStats{}
 	}
+}
 
-	repoGitInfo, err := c.repoGitInfoCache.Get(ctx, pr.TargetRepoID)
-	if err != nil {
-		return fmt.Errorf("failed get repo git info to fetch diff stats: %w", err)
-	}
+func (c *ListService) backfillStats(
+	ctx context.Context,
+	list []types.PullReqRepo,
+) error {
+	for _, entry := range list {
+		pr := entry.PullRequest
 
-	output, err := c.git.DiffStats(ctx, &git.DiffParams{
-		ReadParams: git.CreateReadParams(repoGitInfo),
-		BaseRef:    pr.MergeBaseSHA,
-		HeadRef:    pr.SourceSHA,
-	})
-	if err != nil {
-		return fmt.Errorf("failed get diff stats: %w", err)
-	}
+		s := pr.Stats.DiffStats
+		if s.Commits != nil && s.FilesChanged != nil && s.Additions != nil && s.Deletions != nil {
+			return nil
+		}
 
-	pr.Stats.DiffStats = types.NewDiffStats(output.Commits, output.FilesChanged, output.Additions, output.Deletions)
+		repoGitInfo, err := c.repoGitInfoCache.Get(ctx, pr.TargetRepoID)
+		if err != nil {
+			return fmt.Errorf("failed get repo git info to fetch diff stats: %w", err)
+		}
+
+		output, err := c.git.DiffStats(ctx, &git.DiffParams{
+			ReadParams: git.CreateReadParams(repoGitInfo),
+			BaseRef:    pr.MergeBaseSHA,
+			HeadRef:    pr.SourceSHA,
+		})
+		if err != nil {
+			return fmt.Errorf("failed get diff stats: %w", err)
+		}
+
+		pr.Stats.DiffStats = types.NewDiffStats(output.Commits, output.FilesChanged, output.Additions, output.Deletions)
+	}
 
 	return nil
 }
 
-// BackfillChecks collects the check metadata for the provided list of pull requests.
-func (c *ListService) BackfillChecks(
+// backfillChecks collects the check metadata for the provided list of pull requests.
+func (c *ListService) backfillChecks(
 	ctx context.Context,
 	list []types.PullReqRepo,
 ) error {
@@ -284,8 +291,8 @@ func (c *ListService) BackfillChecks(
 	return nil
 }
 
-// BackfillRules collects the rule metadata for the provided list of pull requests.
-func (c *ListService) BackfillRules(
+// backfillRules collects the rule metadata for the provided list of pull requests.
+func (c *ListService) backfillRules(
 	ctx context.Context,
 	list []types.PullReqRepo,
 ) error {
@@ -344,56 +351,44 @@ func (c *ListService) BackfillMetadata(
 	list []types.PullReqRepo,
 	options types.PullReqMetadataOptions,
 ) error {
-	if options.IsAllFalse() {
-		return nil
-	}
-
 	if options.IncludeChecks {
-		if err := c.BackfillChecks(ctx, list); err != nil {
+		if err := c.backfillChecks(ctx, list); err != nil {
 			return fmt.Errorf("failed to backfill checks")
 		}
 	}
 
 	if options.IncludeRules {
-		if err := c.BackfillRules(ctx, list); err != nil {
+		if err := c.backfillRules(ctx, list); err != nil {
 			return fmt.Errorf("failed to backfill rules")
 		}
 	}
 
+	if options.IncludeGitStats {
+		if err := c.backfillStats(ctx, list); err != nil {
+			log.Ctx(ctx).Warn().Err(err).Msg("failed to backfill PR stats")
+		}
+	} else {
+		clearStats(list)
+	}
+
 	return nil
 }
 
 func (c *ListService) BackfillMetadataForRepo(
 	ctx context.Context,
 	repo *types.Repository,
-	list []*types.PullReq,
+	pullReqs []*types.PullReq,
 	options types.PullReqMetadataOptions,
 ) error {
-	if options.IsAllFalse() {
-		return nil
-	}
-
-	listPullReqRepo := make([]types.PullReqRepo, len(list))
-	for i, pr := range list {
-		listPullReqRepo[i] = types.PullReqRepo{
+	list := make([]types.PullReqRepo, len(pullReqs))
+	for i, pr := range pullReqs {
+		list[i] = types.PullReqRepo{
 			PullRequest: pr,
 			Repository:  repo,
 		}
 	}
 
-	if options.IncludeChecks {
-		if err := c.BackfillChecks(ctx, listPullReqRepo); err != nil {
-			return fmt.Errorf("failed to backfill checks")
-		}
-	}
-
-	if options.IncludeRules {
-		if err := c.BackfillRules(ctx, listPullReqRepo); err != nil {
-			return fmt.Errorf("failed to backfill rules")
-		}
-	}
-
-	return nil
+	return c.BackfillMetadata(ctx, list, options)
 }
 
 func (c *ListService) BackfillMetadataForPullReq(
@@ -402,26 +397,10 @@ func (c *ListService) BackfillMetadataForPullReq(
 	pr *types.PullReq,
 	options types.PullReqMetadataOptions,
 ) error {
-	if options.IsAllFalse() {
-		return nil
-	}
-
 	list := []types.PullReqRepo{{
 		PullRequest: pr,
 		Repository:  repo,
 	}}
 
-	if options.IncludeChecks {
-		if err := c.BackfillChecks(ctx, list); err != nil {
-			return fmt.Errorf("failed to backfill checks")
-		}
-	}
-
-	if options.IncludeRules {
-		if err := c.BackfillRules(ctx, list); err != nil {
-			return fmt.Errorf("failed to backfill rules")
-		}
-	}
-
-	return nil
+	return c.BackfillMetadata(ctx, list, options)
 }