From 82b8679d6fbd1a74ae4d05ec4316286f43afd052 Mon Sep 17 00:00:00 2001 From: Enver Bisevac Date: Mon, 13 Feb 2023 00:47:23 +0100 Subject: [PATCH] [fix] diff-stats api returns total commits and files changed in compare branches (#323) --- gitrpc/diff.go | 67 +++++++++++++++++ gitrpc/interface.go | 1 + gitrpc/rpc/http.pb.go | 1 + gitrpc/rpc/operations.pb.go | 2 + gitrpc/rpc/repo.pb.go | 1 + gitrpc/rpc/shared.pb.go | 1 + internal/api/controller/pullreq/pr_find.go | 87 +++++----------------- internal/api/controller/repo/diff.go | 36 +++++++++ internal/api/handler/repo/diff.go | 23 ++++++ internal/api/openapi/repo.go | 10 +++ internal/router/api.go | 3 + internal/store/database/pullreq.go | 6 +- types/pullreq.go | 13 +++- 13 files changed, 175 insertions(+), 76 deletions(-) diff --git a/gitrpc/diff.go b/gitrpc/diff.go index d8db04465..d7d19d09e 100644 --- a/gitrpc/diff.go +++ b/gitrpc/diff.go @@ -12,6 +12,8 @@ import ( "github.com/harness/gitness/gitrpc/internal/streamio" "github.com/harness/gitness/gitrpc/rpc" + + "golang.org/x/sync/errgroup" ) type DiffParams struct { @@ -85,3 +87,68 @@ func (c *Client) DiffShortStat(ctx context.Context, params *DiffParams) (DiffSho Deletions: int(stat.GetDeletions()), }, nil } + +type DiffStatsOutput struct { + Commits int + FilesChanged int +} + +func (c *Client) DiffStats(ctx context.Context, params *DiffParams) (DiffStatsOutput, error) { + // declare variables which will be used in go routines, + // no need for atomic operations because writing and reading variable + // doesn't happen at the same time + var ( + totalCommits int + totalFiles int + ) + + errGroup, groupCtx := errgroup.WithContext(ctx) + + errGroup.Go(func() error { + // read total commits + + options := &GetCommitDivergencesParams{ + ReadParams: params.ReadParams, + Requests: []CommitDivergenceRequest{ + { + From: params.HeadRef, + To: params.BaseRef, + }, + }, + } + + rpcOutput, err := c.GetCommitDivergences(groupCtx, options) + if err != nil { + return fmt.Errorf("failed to count pull request commits: %w", err) + } + if len(rpcOutput.Divergences) > 0 { + totalCommits = int(rpcOutput.Divergences[0].Ahead) + } + return nil + }) + + errGroup.Go(func() error { + // read short stat + stat, err := c.DiffShortStat(groupCtx, &DiffParams{ + ReadParams: params.ReadParams, + BaseRef: params.BaseRef, + HeadRef: params.HeadRef, + MergeBase: true, // must be true, because commitDivergences use tripple dot notation + }) + if err != nil { + return fmt.Errorf("failed to count pull request file changes: %w", err) + } + totalFiles = stat.Files + return nil + }) + + err := errGroup.Wait() + if err != nil { + return DiffStatsOutput{}, err + } + + return DiffStatsOutput{ + Commits: totalCommits, + FilesChanged: totalFiles, + }, nil +} diff --git a/gitrpc/interface.go b/gitrpc/interface.go index 1915fc1e9..a3d562e8d 100644 --- a/gitrpc/interface.go +++ b/gitrpc/interface.go @@ -47,6 +47,7 @@ type Interface interface { */ RawDiff(ctx context.Context, in *DiffParams, w io.Writer) error DiffShortStat(ctx context.Context, params *DiffParams) (DiffShortStatOutput, error) + DiffStats(ctx context.Context, params *DiffParams) (DiffStatsOutput, error) /* * Merge services diff --git a/gitrpc/rpc/http.pb.go b/gitrpc/rpc/http.pb.go index 9afe821bf..7006fe359 100644 --- a/gitrpc/rpc/http.pb.go +++ b/gitrpc/rpc/http.pb.go @@ -151,6 +151,7 @@ type ServicePackRequest struct { // Depending on the service the matching base type has to be passed // // Types that are assignable to Base: + // // *ServicePackRequest_ReadBase // *ServicePackRequest_WriteBase Base isServicePackRequest_Base `protobuf_oneof:"base"` diff --git a/gitrpc/rpc/operations.pb.go b/gitrpc/rpc/operations.pb.go index 517aeef89..a06443219 100644 --- a/gitrpc/rpc/operations.pb.go +++ b/gitrpc/rpc/operations.pb.go @@ -247,6 +247,7 @@ type CommitFilesAction struct { unknownFields protoimpl.UnknownFields // Types that are assignable to Payload: + // // *CommitFilesAction_Header // *CommitFilesAction_Content Payload isCommitFilesAction_Payload `protobuf_oneof:"payload"` @@ -330,6 +331,7 @@ type CommitFilesRequest struct { unknownFields protoimpl.UnknownFields // Types that are assignable to Payload: + // // *CommitFilesRequest_Header // *CommitFilesRequest_Action Payload isCommitFilesRequest_Payload `protobuf_oneof:"payload"` diff --git a/gitrpc/rpc/repo.pb.go b/gitrpc/rpc/repo.pb.go index fb0ce39f2..8fae9e594 100644 --- a/gitrpc/rpc/repo.pb.go +++ b/gitrpc/rpc/repo.pb.go @@ -130,6 +130,7 @@ type CreateRepositoryRequest struct { unknownFields protoimpl.UnknownFields // Types that are assignable to Data: + // // *CreateRepositoryRequest_Header // *CreateRepositoryRequest_File Data isCreateRepositoryRequest_Data `protobuf_oneof:"data"` diff --git a/gitrpc/rpc/shared.pb.go b/gitrpc/rpc/shared.pb.go index 084d3dfb6..65817ec5b 100644 --- a/gitrpc/rpc/shared.pb.go +++ b/gitrpc/rpc/shared.pb.go @@ -298,6 +298,7 @@ type FileUpload struct { unknownFields protoimpl.UnknownFields // Types that are assignable to Data: + // // *FileUpload_Header // *FileUpload_Chunk Data isFileUpload_Data `protobuf_oneof:"data"` diff --git a/internal/api/controller/pullreq/pr_find.go b/internal/api/controller/pullreq/pr_find.go index 5dee7dc39..b7bdff7ec 100644 --- a/internal/api/controller/pullreq/pr_find.go +++ b/internal/api/controller/pullreq/pr_find.go @@ -14,8 +14,6 @@ import ( "github.com/harness/gitness/internal/auth" "github.com/harness/gitness/types" "github.com/harness/gitness/types/enum" - - "golang.org/x/sync/errgroup" ) // Find returns a pull request from the provided repository. @@ -39,83 +37,32 @@ func (c *Controller) Find( return nil, err } - pr.Stats.Commits, pr.Stats.FilesChanged, err = c.getStats(ctx, repo, pr) + baseRef := pr.TargetBranch + headRef := pr.SourceSHA + if pr.MergeBaseSHA != nil { + baseRef = *pr.MergeBaseSHA + } + if pr.MergeHeadSHA != nil { + headRef = *pr.MergeHeadSHA + } + + output, err := c.gitRPCClient.DiffStats(ctx, &gitrpc.DiffParams{ + ReadParams: gitrpc.CreateRPCReadParams(repo), + BaseRef: baseRef, + HeadRef: headRef, + }) if err != nil { return nil, err } + pr.Stats.DiffStats.Commits = output.Commits + pr.Stats.DiffStats.FilesChanged = output.FilesChanged + updateMergeStatus(pr) return pr, nil } -func (c *Controller) getStats( - ctx context.Context, - repo *types.Repository, - pr *types.PullReq, -) (int, int, error) { - // declare variables which will be used in go routines, - // no need for atomic operations because writing and reading variable - // doesn't happen at the same time - var ( - totalCommits int - totalFiles int - ) - - gitRef := pr.SourceBranch - afterRef := pr.TargetBranch - if pr.State == enum.PullReqStateMerged { - gitRef = *pr.MergeHeadSHA - afterRef = *pr.MergeBaseSHA - } - - errGroup, groupCtx := errgroup.WithContext(ctx) - - errGroup.Go(func() error { - // read total commits - options := &gitrpc.GetCommitDivergencesParams{ - ReadParams: gitrpc.CreateRPCReadParams(repo), - Requests: []gitrpc.CommitDivergenceRequest{ - { - From: gitRef, - To: afterRef, - }, - }, - } - - rpcOutput, err := c.gitRPCClient.GetCommitDivergences(groupCtx, options) - if err != nil { - return fmt.Errorf("failed to count pull request commits: %w", err) - } - if len(rpcOutput.Divergences) > 0 { - totalCommits = int(rpcOutput.Divergences[0].Ahead) - } - return nil - }) - - errGroup.Go(func() error { - // read short stat - stat, err := c.gitRPCClient.DiffShortStat(groupCtx, &gitrpc.DiffParams{ - ReadParams: gitrpc.CreateRPCReadParams(repo), - BaseRef: afterRef, - HeadRef: gitRef, - MergeBase: true, - }) - if err != nil { - return fmt.Errorf("failed to count pull request file changes: %w", err) - } - totalFiles = stat.Files - return nil - }) - - err := errGroup.Wait() - if err != nil { - return 0, 0, err - } - - return totalCommits, totalFiles, nil -} - func updateMergeStatus(pr *types.PullReq) { mc := "" if pr.MergeConflicts != nil { diff --git a/internal/api/controller/repo/diff.go b/internal/api/controller/repo/diff.go index e31dc1594..40ac2fd8c 100644 --- a/internal/api/controller/repo/diff.go +++ b/internal/api/controller/repo/diff.go @@ -13,6 +13,7 @@ import ( apiauth "github.com/harness/gitness/internal/api/auth" "github.com/harness/gitness/internal/api/usererror" "github.com/harness/gitness/internal/auth" + "github.com/harness/gitness/types" "github.com/harness/gitness/types/enum" ) @@ -65,3 +66,38 @@ func parseDiffPath(path string) (CompareInfo, error) { MergeBase: strings.Contains(path, "..."), }, nil } + +func (c *Controller) DiffStats( + ctx context.Context, + session *auth.Session, + repoRef string, + path string, +) (types.DiffStats, error) { + repo, err := c.repoStore.FindByRef(ctx, repoRef) + if err != nil { + return types.DiffStats{}, err + } + + if err = apiauth.CheckRepo(ctx, c.authorizer, session, repo, enum.PermissionRepoView, false); err != nil { + return types.DiffStats{}, err + } + + info, err := parseDiffPath(path) + if err != nil { + return types.DiffStats{}, err + } + + output, err := c.gitRPCClient.DiffStats(ctx, &gitrpc.DiffParams{ + ReadParams: gitrpc.CreateRPCReadParams(repo), + BaseRef: info.BaseRef, + HeadRef: info.HeadRef, + }) + if err != nil { + return types.DiffStats{}, err + } + + return types.DiffStats{ + Commits: output.Commits, + FilesChanged: output.FilesChanged, + }, nil +} diff --git a/internal/api/handler/repo/diff.go b/internal/api/handler/repo/diff.go index f3c4639e6..a9340933a 100644 --- a/internal/api/handler/repo/diff.go +++ b/internal/api/handler/repo/diff.go @@ -31,3 +31,26 @@ func HandleRawDiff(repoCtrl *repo.Controller) http.HandlerFunc { } } } + +// HandleDiffStats how diff statistics of two commits, branches or tags. +func HandleDiffStats(repoCtrl *repo.Controller) http.HandlerFunc { + return func(w http.ResponseWriter, r *http.Request) { + ctx := r.Context() + session, _ := request.AuthSessionFrom(ctx) + repoRef, err := request.GetRepoRefFromPath(r) + if err != nil { + render.TranslatedUserError(w, err) + return + } + + path := request.GetOptionalRemainderFromPath(r) + + output, err := repoCtrl.DiffStats(ctx, session, repoRef, path) + if err != nil { + render.TranslatedUserError(w, err) + return + } + + render.JSON(w, http.StatusOK, output) + } +} diff --git a/internal/api/openapi/repo.go b/internal/api/openapi/repo.go index 96d13a0b6..51697e8ab 100644 --- a/internal/api/openapi/repo.go +++ b/internal/api/openapi/repo.go @@ -506,4 +506,14 @@ func repoOperations(reflector *openapi3.Reflector) { _ = reflector.SetJSONResponse(&opMergeCheck, new(usererror.Error), http.StatusUnauthorized) _ = reflector.SetJSONResponse(&opMergeCheck, new(usererror.Error), http.StatusForbidden) _ = reflector.Spec.AddOperation(http.MethodPost, "/repos/{repo_ref}/merge-check/{range}", opMergeCheck) + + opDiffStats := openapi3.Operation{} + opDiffStats.WithTags("repository") + opDiffStats.WithMapOfAnything(map[string]interface{}{"operationId": "diffStats"}) + _ = reflector.SetRequest(&opDiffStats, new(getRawDiffRequest), http.MethodGet) + _ = reflector.SetJSONResponse(&opDiffStats, new(types.DiffStats), http.StatusOK) + _ = reflector.SetJSONResponse(&opDiffStats, new(usererror.Error), http.StatusInternalServerError) + _ = reflector.SetJSONResponse(&opDiffStats, new(usererror.Error), http.StatusUnauthorized) + _ = reflector.SetJSONResponse(&opDiffStats, new(usererror.Error), http.StatusForbidden) + _ = reflector.Spec.AddOperation(http.MethodGet, "/repos/{repo_ref}/diff-stats/{range}", opDiffStats) } diff --git a/internal/router/api.go b/internal/router/api.go index 2d739969b..009053ca4 100644 --- a/internal/router/api.go +++ b/internal/router/api.go @@ -218,6 +218,9 @@ func setupRepos(r chi.Router, repoCtrl *repo.Controller, pullreqCtrl *pullreq.Co r.Route("/merge-check", func(r chi.Router) { r.Post("/*", handlerrepo.HandleMergeCheck(repoCtrl)) }) + r.Route("/diff-stats", func(r chi.Router) { + r.Get("/*", handlerrepo.HandleDiffStats(repoCtrl)) + }) SetupPullReq(r, pullreqCtrl) diff --git a/internal/store/database/pullreq.go b/internal/store/database/pullreq.go index 4500c9b09..f5feb279a 100644 --- a/internal/store/database/pullreq.go +++ b/internal/store/database/pullreq.go @@ -477,8 +477,10 @@ func mapPullReq(pr *pullReq) *types.PullReq { Merger: nil, Stats: types.PullReqStats{ Conversations: pr.CommentCount, - Commits: 0, - FilesChanged: 0, + DiffStats: types.DiffStats{ + Commits: 0, + FilesChanged: 0, + }, }, } } diff --git a/types/pullreq.go b/types/pullreq.go index 61482fa89..846fedd17 100644 --- a/types/pullreq.go +++ b/types/pullreq.go @@ -50,11 +50,16 @@ type PullReq struct { Stats PullReqStats `json:"stats"` } -// PullReqStats shows total number of conversations, commits and modified files. +// DiffStats shows total number of commits and modified files. +type DiffStats struct { + Commits int `json:"commits"` + FilesChanged int `json:"files_changed"` +} + +// PullReqStats shows Diff statistics and number of conversations. type PullReqStats struct { - Conversations int `json:"conversations"` - Commits int `json:"commits"` - FilesChanged int `json:"files_changed"` + DiffStats + Conversations int `json:"conversations,omitempty"` } // PullReqFilter stores pull request query parameters.