From 0db33abeb1f4fb2022780e2ccd4a0e8291b3afc5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marko=20Ga=C4=87e=C5=A1a?= Date: Tue, 5 Nov 2024 12:02:44 +0000 Subject: [PATCH] feat: [PIPE-22454]: add PR metadata in get/list PR API response (#2912) * add PR metadata in PR API response --- app/api/controller/pullreq/pr_find.go | 12 +- app/api/controller/pullreq/pr_list.go | 4 + app/api/handler/pullreq/pr_find.go | 18 +- app/api/handler/pullreq/pr_metadata.go | 3 +- app/api/openapi/pullreq.go | 7 +- app/api/openapi/space.go | 5 +- app/api/request/pullreq.go | 65 ++++--- app/services/pullreq/service_list.go | 229 +++++++++++++++++++++++-- app/services/pullreq/wire.go | 5 + cmd/gitness/wire_gen.go | 2 +- types/pullreq.go | 14 +- 11 files changed, 316 insertions(+), 48 deletions(-) diff --git a/app/api/controller/pullreq/pr_find.go b/app/api/controller/pullreq/pr_find.go index cbd048e8b..2887aff18 100644 --- a/app/api/controller/pullreq/pr_find.go +++ b/app/api/controller/pullreq/pr_find.go @@ -32,6 +32,7 @@ func (c *Controller) Find( session *auth.Session, repoRef string, pullreqNum int64, + options types.PullReqMetadataOptions, ) (*types.PullReq, error) { if pullreqNum <= 0 { return nil, usererror.BadRequest("A valid pull request number must be provided.") @@ -52,6 +53,10 @@ func (c *Controller) Find( return nil, fmt.Errorf("failed to backfill labels assigned to pull request: %w", err) } + if err := c.pullreqListService.BackfillMetadataForPullReq(ctx, repo, pr, options); err != nil { + 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") } @@ -59,7 +64,7 @@ func (c *Controller) Find( return pr, nil } -// Find returns a pull request from the provided repository. +// FindByBranches returns a pull request from the provided branch pair. func (c *Controller) FindByBranches( ctx context.Context, session *auth.Session, @@ -67,6 +72,7 @@ func (c *Controller) FindByBranches( sourceRepoRef, sourceBranch, targetBranch string, + options types.PullReqMetadataOptions, ) (*types.PullReq, error) { if sourceBranch == "" || targetBranch == "" { return nil, usererror.BadRequest("A valid source/target branch must be provided.") @@ -103,5 +109,9 @@ func (c *Controller) FindByBranches( return nil, usererror.ErrNotFound } + if err := c.pullreqListService.BackfillMetadataForPullReq(ctx, targetRepo, prs[0], options); err != nil { + return nil, fmt.Errorf("failed to backfill pull request metadata: %w", err) + } + return prs[0], nil } diff --git a/app/api/controller/pullreq/pr_list.go b/app/api/controller/pullreq/pr_list.go index 5995378fd..9c5ef33b0 100644 --- a/app/api/controller/pullreq/pr_list.go +++ b/app/api/controller/pullreq/pr_list.go @@ -81,6 +81,10 @@ func (c *Controller) List( return nil, 0, err } + if err := c.pullreqListService.BackfillMetadataForRepo(ctx, repo, list, filter.PullReqMetadataOptions); err != nil { + 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") diff --git a/app/api/handler/pullreq/pr_find.go b/app/api/handler/pullreq/pr_find.go index 84ac37163..d253b74b2 100644 --- a/app/api/handler/pullreq/pr_find.go +++ b/app/api/handler/pullreq/pr_find.go @@ -40,7 +40,13 @@ func HandleFind(pullreqCtrl *pullreq.Controller) http.HandlerFunc { return } - pr, err := pullreqCtrl.Find(ctx, session, repoRef, pullreqNumber) + options, err := request.ParsePullReqMetadataOptions(r) + if err != nil { + render.TranslatedUserError(ctx, w, err) + return + } + + pr, err := pullreqCtrl.Find(ctx, session, repoRef, pullreqNumber, options) if err != nil { render.TranslatedUserError(ctx, w, err) return @@ -50,7 +56,7 @@ func HandleFind(pullreqCtrl *pullreq.Controller) http.HandlerFunc { } } -// HandleFind returns a http.HandlerFunc that finds a pull request. +// HandleFindByBranches returns a http.HandlerFunc that finds a pull request from the provided branch pair. func HandleFindByBranches(pullreqCtrl *pullreq.Controller) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { ctx := r.Context() @@ -76,7 +82,13 @@ func HandleFindByBranches(pullreqCtrl *pullreq.Controller) http.HandlerFunc { return } - pr, err := pullreqCtrl.FindByBranches(ctx, session, repoRef, sourceRepoRef, sourceBranch, targetBranch) + options, err := request.ParsePullReqMetadataOptions(r) + if err != nil { + render.TranslatedUserError(ctx, w, err) + return + } + + pr, err := pullreqCtrl.FindByBranches(ctx, session, repoRef, sourceRepoRef, sourceBranch, targetBranch, options) if err != nil { render.TranslatedUserError(ctx, w, err) return diff --git a/app/api/handler/pullreq/pr_metadata.go b/app/api/handler/pullreq/pr_metadata.go index 9fd7c12a0..1d8637398 100644 --- a/app/api/handler/pullreq/pr_metadata.go +++ b/app/api/handler/pullreq/pr_metadata.go @@ -20,6 +20,7 @@ import ( "github.com/harness/gitness/app/api/controller/pullreq" "github.com/harness/gitness/app/api/render" "github.com/harness/gitness/app/api/request" + "github.com/harness/gitness/types" ) // HandleMetadata returns a http.HandlerFunc that returns PR metadata. @@ -40,7 +41,7 @@ func HandleMetadata(pullreqCtrl *pullreq.Controller) http.HandlerFunc { return } - pr, err := pullreqCtrl.Find(ctx, session, repoRef, pullreqNumber) + pr, err := pullreqCtrl.Find(ctx, session, repoRef, pullreqNumber, types.PullReqMetadataOptions{}) if err != nil { render.TranslatedUserError(ctx, w, err) return diff --git a/app/api/openapi/pullreq.go b/app/api/openapi/pullreq.go index 224ab7269..2a15fbe32 100644 --- a/app/api/openapi/pullreq.go +++ b/app/api/openapi/pullreq.go @@ -504,7 +504,8 @@ func pullReqOperations(reflector *openapi3.Reflector) { QueryParameterPage, QueryParameterLimit, QueryParameterLabelID, QueryParameterValueID, queryParameterAuthorID, queryParameterCommenterID, queryParameterMentionedID, - queryParameterReviewerID, queryParameterReviewDecision) + queryParameterReviewerID, queryParameterReviewDecision, + 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) @@ -516,6 +517,7 @@ func pullReqOperations(reflector *openapi3.Reflector) { getPullReq := openapi3.Operation{} getPullReq.WithTags("pullreq") getPullReq.WithMapOfAnything(map[string]interface{}{"operationId": "getPullReq"}) + getPullReq.WithParameters(queryParameterIncludeChecks, queryParameterIncludeRules) _ = reflector.SetRequest(&getPullReq, new(getPullReqRequest), http.MethodGet) _ = reflector.SetJSONResponse(&getPullReq, new(types.PullReq), http.StatusOK) _ = reflector.SetJSONResponse(&getPullReq, new(usererror.Error), http.StatusBadRequest) @@ -527,7 +529,8 @@ func pullReqOperations(reflector *openapi3.Reflector) { getPullReqByBranches := openapi3.Operation{} getPullReqByBranches.WithTags("pullreq") getPullReqByBranches.WithMapOfAnything(map[string]interface{}{"operationId": "getPullReqByBranches"}) - getPullReqByBranches.WithParameters(queryParameterSourceRepoRefPullRequest) + getPullReqByBranches.WithParameters(queryParameterSourceRepoRefPullRequest, + queryParameterIncludeChecks, queryParameterIncludeRules) _ = reflector.SetRequest(&getPullReqByBranches, new(getPullReqByBranchesRequest), http.MethodGet) _ = reflector.SetJSONResponse(&getPullReqByBranches, new(types.PullReq), http.StatusOK) _ = reflector.SetJSONResponse(&getPullReqByBranches, new(usererror.Error), http.StatusBadRequest) diff --git a/app/api/openapi/space.go b/app/api/openapi/space.go index 8c048d93a..6e99769c2 100644 --- a/app/api/openapi/space.go +++ b/app/api/openapi/space.go @@ -600,9 +600,10 @@ func spaceOperations(reflector *openapi3.Reflector) { QueryParameterLimit, QueryParameterLabelID, QueryParameterValueID, queryParameterAuthorID, queryParameterCommenterID, queryParameterMentionedID, - queryParameterReviewerID, queryParameterReviewDecision) + queryParameterReviewerID, queryParameterReviewDecision, + queryParameterIncludeChecks, queryParameterIncludeRules) _ = reflector.SetRequest(&listPullReq, new(listPullReqRequest), http.MethodGet) - _ = reflector.SetJSONResponse(&listPullReq, new([]types.PullReq), http.StatusOK) + _ = reflector.SetJSONResponse(&listPullReq, new([]types.PullReqRepo), http.StatusOK) _ = reflector.SetJSONResponse(&listPullReq, new(usererror.Error), http.StatusBadRequest) _ = reflector.SetJSONResponse(&listPullReq, new(usererror.Error), http.StatusInternalServerError) _ = reflector.SetJSONResponse(&listPullReq, new(usererror.Error), http.StatusUnauthorized) diff --git a/app/api/request/pullreq.go b/app/api/request/pullreq.go index fd61fda1d..685098846 100644 --- a/app/api/request/pullreq.go +++ b/app/api/request/pullreq.go @@ -109,6 +109,23 @@ func parseReviewDecisions(r *http.Request) []enum.PullReqReviewDecision { return reviewDecisions } +func ParsePullReqMetadataOptions(r *http.Request) (types.PullReqMetadataOptions, error) { + includeChecks, err := GetIncludeChecksFromQueryOrDefault(r, false) + if err != nil { + return types.PullReqMetadataOptions{}, err + } + + includeRules, err := GetIncludeRulesFromQueryOrDefault(r, false) + if err != nil { + return types.PullReqMetadataOptions{}, err + } + + return types.PullReqMetadataOptions{ + IncludeChecks: includeChecks, + IncludeRules: includeRules, + }, nil +} + // ParsePullReqFilter extracts the pull request query parameter from the url. func ParsePullReqFilter(r *http.Request) (*types.PullReqFilter, error) { createdBy, err := QueryParamListAsPositiveInt64(r, QueryParamCreatedBy) @@ -170,28 +187,34 @@ func ParsePullReqFilter(r *http.Request) (*types.PullReqFilter, error) { return nil, fmt.Errorf("encountered error parsing mentioned ID filter: %w", err) } + metadataOptions, err := ParsePullReqMetadataOptions(r) + if err != nil { + return nil, err + } + return &types.PullReqFilter{ - Page: ParsePage(r), - Size: ParseLimit(r), - Query: ParseQuery(r), - CreatedBy: createdBy, - SourceRepoRef: r.URL.Query().Get("source_repo_ref"), - SourceBranch: r.URL.Query().Get("source_branch"), - TargetBranch: r.URL.Query().Get("target_branch"), - States: parsePullReqStates(r), - Sort: ParseSortPullReq(r), - Order: ParseOrder(r), - LabelID: labelID, - ValueID: valueID, - AuthorID: authorID, - CommenterID: commenterID, - ReviewerID: reviewerID, - ReviewDecisions: reviewDecisions, - MentionedID: mentionedID, - ExcludeDescription: excludeDescription, - CreatedFilter: createdFilter, - UpdatedFilter: updatedFilter, - EditedFilter: editedFilter, + Page: ParsePage(r), + Size: ParseLimit(r), + Query: ParseQuery(r), + CreatedBy: createdBy, + SourceRepoRef: r.URL.Query().Get("source_repo_ref"), + SourceBranch: r.URL.Query().Get("source_branch"), + TargetBranch: r.URL.Query().Get("target_branch"), + States: parsePullReqStates(r), + Sort: ParseSortPullReq(r), + Order: ParseOrder(r), + LabelID: labelID, + ValueID: valueID, + AuthorID: authorID, + CommenterID: commenterID, + ReviewerID: reviewerID, + ReviewDecisions: reviewDecisions, + MentionedID: mentionedID, + ExcludeDescription: excludeDescription, + CreatedFilter: createdFilter, + UpdatedFilter: updatedFilter, + EditedFilter: editedFilter, + PullReqMetadataOptions: metadataOptions, }, nil } diff --git a/app/services/pullreq/service_list.go b/app/services/pullreq/service_list.go index 9daccf795..5233532ce 100644 --- a/app/services/pullreq/service_list.go +++ b/app/services/pullreq/service_list.go @@ -22,6 +22,7 @@ import ( "github.com/harness/gitness/app/auth" "github.com/harness/gitness/app/auth/authz" "github.com/harness/gitness/app/services/label" + "github.com/harness/gitness/app/services/protection" "github.com/harness/gitness/app/store" "github.com/harness/gitness/errors" "github.com/harness/gitness/git" @@ -30,18 +31,21 @@ import ( "github.com/harness/gitness/types" "github.com/harness/gitness/types/enum" + "github.com/gotidy/ptr" "github.com/rs/zerolog/log" ) type ListService struct { - tx dbtx.Transactor - git git.Interface - authorizer authz.Authorizer - spaceStore store.SpaceStore - repoStore store.RepoStore - repoGitInfoCache store.RepoGitInfoCache - pullreqStore store.PullReqStore - labelSvc *label.Service + tx dbtx.Transactor + git git.Interface + authorizer authz.Authorizer + spaceStore store.SpaceStore + repoStore store.RepoStore + repoGitInfoCache store.RepoGitInfoCache + pullreqStore store.PullReqStore + checkStore store.CheckStore + labelSvc *label.Service + protectionManager *protection.Manager } func NewListService( @@ -52,17 +56,21 @@ func NewListService( repoStore store.RepoStore, repoGitInfoCache store.RepoGitInfoCache, pullreqStore store.PullReqStore, + checkStore store.CheckStore, labelSvc *label.Service, + protectionManager *protection.Manager, ) *ListService { return &ListService{ - tx: tx, - git: git, - authorizer: authorizer, - spaceStore: spaceStore, - repoStore: repoStore, - repoGitInfoCache: repoGitInfoCache, - pullreqStore: pullreqStore, - labelSvc: labelSvc, + tx: tx, + git: git, + authorizer: authorizer, + spaceStore: spaceStore, + repoStore: repoStore, + repoGitInfoCache: repoGitInfoCache, + pullreqStore: pullreqStore, + checkStore: checkStore, + labelSvc: labelSvc, + protectionManager: protectionManager, } } @@ -166,6 +174,10 @@ func (c *ListService) ListForSpace( } } + if err := c.BackfillMetadata(ctx, response, filter.PullReqMetadataOptions); err != nil { + return nil, fmt.Errorf("failed to backfill metadata: %w", err) + } + return response, nil } @@ -228,3 +240,188 @@ func (c *ListService) BackfillStats(ctx context.Context, pr *types.PullReq) erro return nil } + +// BackfillChecks collects the check metadata for the provided list of pull requests. +func (c *ListService) BackfillChecks( + ctx context.Context, + list []types.PullReqRepo, +) error { + // prepare list of commit SHAs per repository + + repoCommitSHAs := make(map[int64][]string) + for _, entry := range list { + repoID := entry.Repository.ID + commitSHAs := repoCommitSHAs[repoID] + repoCommitSHAs[repoID] = append(commitSHAs, entry.PullRequest.SourceSHA) + } + + // fetch checks for every repository + + type repoSHA struct { + repoID int64 + sha string + } + + repoCheckSummaryMap := make(map[repoSHA]types.CheckCountSummary) + for repoID, commitSHAs := range repoCommitSHAs { + commitCheckSummaryMap, err := c.checkStore.ResultSummary(ctx, repoID, commitSHAs) + if err != nil { + return fmt.Errorf("fail to fetch check summary for commits: %w", err) + } + + for commitSHA, checkSummary := range commitCheckSummaryMap { + repoCheckSummaryMap[repoSHA{repoID: repoID, sha: commitSHA.String()}] = checkSummary + } + } + + // backfill the list with check count summary + + for _, entry := range list { + entry.PullRequest.CheckSummary = + ptr.Of(repoCheckSummaryMap[repoSHA{repoID: entry.Repository.ID, sha: entry.PullRequest.SourceSHA}]) + } + + return nil +} + +// BackfillRules collects the rule metadata for the provided list of pull requests. +func (c *ListService) BackfillRules( + ctx context.Context, + list []types.PullReqRepo, +) error { + // prepare list of branch names per repository + + repoBranchNames := make(map[int64][]string) + repoDefaultBranch := make(map[int64]string) + for _, entry := range list { + repoID := entry.Repository.ID + branchNames := repoBranchNames[repoID] + repoBranchNames[repoID] = append(branchNames, entry.PullRequest.TargetBranch) + repoDefaultBranch[repoID] = entry.Repository.DefaultBranch + } + + // fetch checks for every repository + + type repoBranchName struct { + repoID int64 + branchName string + } + + repoBranchNameMap := make(map[repoBranchName][]types.RuleInfo) + for repoID, branchNames := range repoBranchNames { + repoProtection, err := c.protectionManager.ForRepository(ctx, repoID) + if err != nil { + return fmt.Errorf("fail to fetch protection rules for repository: %w", err) + } + + for _, branchName := range branchNames { + branchRuleInfos, err := protection.GetRuleInfos( + repoProtection, + repoDefaultBranch[repoID], + branchName, + protection.RuleInfoFilterStatusActive, + protection.RuleInfoFilterTypeBranch) + if err != nil { + return fmt.Errorf("fail to get rule infos for branch %s: %w", branchName, err) + } + + repoBranchNameMap[repoBranchName{repoID: repoID, branchName: branchName}] = branchRuleInfos + } + } + + // backfill the list with check count summary + + for _, entry := range list { + key := repoBranchName{repoID: entry.Repository.ID, branchName: entry.PullRequest.TargetBranch} + entry.PullRequest.Rules = repoBranchNameMap[key] + } + + return nil +} + +func (c *ListService) BackfillMetadata( + ctx context.Context, + list []types.PullReqRepo, + options types.PullReqMetadataOptions, +) error { + if options.IsAllFalse() { + return nil + } + + 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 +} + +func (c *ListService) BackfillMetadataForRepo( + ctx context.Context, + repo *types.Repository, + list []*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{ + 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 +} + +func (c *ListService) BackfillMetadataForPullReq( + ctx context.Context, + repo *types.Repository, + 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 +} diff --git a/app/services/pullreq/wire.go b/app/services/pullreq/wire.go index c2d9e6284..6c666e0a0 100644 --- a/app/services/pullreq/wire.go +++ b/app/services/pullreq/wire.go @@ -22,6 +22,7 @@ import ( pullreqevents "github.com/harness/gitness/app/events/pullreq" "github.com/harness/gitness/app/services/codecomments" "github.com/harness/gitness/app/services/label" + "github.com/harness/gitness/app/services/protection" "github.com/harness/gitness/app/sse" "github.com/harness/gitness/app/store" "github.com/harness/gitness/app/url" @@ -85,7 +86,9 @@ func ProvideListService( repoStore store.RepoStore, repoGitInfoCache store.RepoGitInfoCache, pullreqStore store.PullReqStore, + checkStore store.CheckStore, labelSvc *label.Service, + protectionManager *protection.Manager, ) *ListService { return NewListService( tx, @@ -95,6 +98,8 @@ func ProvideListService( repoStore, repoGitInfoCache, pullreqStore, + checkStore, labelSvc, + protectionManager, ) } diff --git a/cmd/gitness/wire_gen.go b/cmd/gitness/wire_gen.go index ea5343023..9985e7ad7 100644 --- a/cmd/gitness/wire_gen.go +++ b/cmd/gitness/wire_gen.go @@ -275,7 +275,7 @@ func initSystem(ctx context.Context, config *types.Config) (*server.System, erro connectorStore := database.ProvideConnectorStore(db, secretStore) repoGitInfoView := database.ProvideRepoGitInfoView(db) repoGitInfoCache := cache.ProvideRepoGitInfoCache(repoGitInfoView) - listService := pullreq.ProvideListService(transactor, gitInterface, authorizer, spaceStore, repoStore, repoGitInfoCache, pullReqStore, labelService) + listService := pullreq.ProvideListService(transactor, gitInterface, authorizer, spaceStore, repoStore, repoGitInfoCache, pullReqStore, checkStore, labelService, protectionManager) exporterRepository, err := exporter.ProvideSpaceExporter(provider, gitInterface, repoStore, jobScheduler, executor, encrypter, streamer) if err != nil { return nil, err diff --git a/types/pullreq.go b/types/pullreq.go index b5b02ef39..5fc577ef8 100644 --- a/types/pullreq.go +++ b/types/pullreq.go @@ -64,7 +64,9 @@ type PullReq struct { Merger *PrincipalInfo `json:"merger"` Stats PullReqStats `json:"stats"` - Labels []*LabelPullReqAssignmentInfo `json:"labels,omitempty"` + Labels []*LabelPullReqAssignmentInfo `json:"labels,omitempty"` + CheckSummary *CheckCountSummary `json:"check_summary,omitempty"` + Rules []RuleInfo `json:"rules,omitempty"` } func (pr *PullReq) UpdateMergeOutcome(method enum.MergeMethod, conflictFiles []string) { @@ -167,12 +169,22 @@ type PullReqFilter struct { CreatedFilter UpdatedFilter EditedFilter + PullReqMetadataOptions // internal use only SpaceIDs []int64 RepoIDBlacklist []int64 } +type PullReqMetadataOptions struct { + IncludeChecks bool `json:"include_checks"` + IncludeRules bool `json:"include_rules"` +} + +func (options PullReqMetadataOptions) IsAllFalse() bool { + return !options.IncludeChecks && !options.IncludeRules +} + // PullReqReview holds pull request review. type PullReqReview struct { ID int64 `json:"id"`