From 551a2e478c0ff5942309020d61dcb8d272d83b48 Mon Sep 17 00:00:00 2001 From: Marko Gacesa Date: Thu, 14 Dec 2023 12:27:47 +0000 Subject: [PATCH] create pr comment in tx (#903) --- app/api/controller/pullreq/comment_create.go | 223 ++++++++++++------- app/api/controller/tx.go | 14 ++ 2 files changed, 157 insertions(+), 80 deletions(-) diff --git a/app/api/controller/pullreq/comment_create.go b/app/api/controller/pullreq/comment_create.go index 7a72e4148..3972b081e 100644 --- a/app/api/controller/pullreq/comment_create.go +++ b/app/api/controller/pullreq/comment_create.go @@ -19,6 +19,7 @@ import ( "fmt" "time" + "github.com/harness/gitness/app/api/controller" "github.com/harness/gitness/app/api/usererror" "github.com/harness/gitness/app/auth" events "github.com/harness/gitness/app/events/pullreq" @@ -82,7 +83,7 @@ func (in *CommentCreateInput) Validate() error { // CommentCreate creates a new pull request comment (pull request activity, type=comment/code-comment). // -//nolint:gocognit,funlen // refactor if needed +//nolint:gocognit // refactor if needed func (c *Controller) CommentCreate( ctx context.Context, session *auth.Session, @@ -95,102 +96,102 @@ func (c *Controller) CommentCreate( return nil, fmt.Errorf("failed to acquire access to repo: %w", err) } - pr, err := c.pullreqStore.FindByNumber(ctx, repo.ID, prNum) - if err != nil { - return nil, fmt.Errorf("failed to find pull request by number: %w", err) - } - if errValidate := in.Validate(); errValidate != nil { return nil, errValidate } - act := getCommentActivity(session, pr, in) + var pr *types.PullReq + var act *types.PullReqActivity - switch { - case in.IsCodeComment(): - cut, err := c.git.DiffCut(ctx, &git.DiffCutParams{ - ReadParams: git.ReadParams{RepoUID: repo.GitUID}, - SourceCommitSHA: in.SourceCommitSHA, - SourceBranch: pr.SourceBranch, - TargetCommitSHA: in.TargetCommitSHA, - TargetBranch: pr.TargetBranch, - Path: in.Path, - LineStart: in.LineStart, - LineStartNew: in.LineStartNew, - LineEnd: in.LineEnd, - LineEndNew: in.LineEndNew, - }) - if errors.AsStatus(err) == errors.StatusNotFound || errors.AsStatus(err) == errors.StatusPathNotFound { - return nil, usererror.BadRequest(errors.Message(err)) - } - if err != nil { - return nil, err - } - - setAsCodeComment(act, cut, in.Path, in.SourceCommitSHA) - _ = act.SetPayload(&types.PullRequestActivityPayloadCodeComment{ - Title: cut.LinesHeader, - Lines: cut.Lines, - LineStartNew: in.LineStartNew, - LineEndNew: in.LineEndNew, - }) - - err = c.writeActivity(ctx, pr, act) - - // Migrate the comment if necessary... Note: we still need to return the code comment as is. - needsNewLineMigrate := in.SourceCommitSHA != cut.LatestSourceSHA - needsOldLineMigrate := pr.MergeBaseSHA != cut.MergeBaseSHA - if err == nil && (needsNewLineMigrate || needsOldLineMigrate) { - comments := []*types.CodeComment{act.AsCodeComment()} - - if needsNewLineMigrate { - c.codeCommentMigrator.MigrateNew(ctx, repo.GitUID, cut.LatestSourceSHA, comments) - } - if needsOldLineMigrate { - c.codeCommentMigrator.MigrateOld(ctx, repo.GitUID, cut.MergeBaseSHA, comments) - } - - if errMigrateUpdate := c.codeCommentView.UpdateAll(ctx, comments); errMigrateUpdate != nil { - // non-critical error - log.Ctx(ctx).Err(errMigrateUpdate). - Msgf("failed to migrate code comment to the latest source/merge-base commit SHA") - } - } - case in.ParentID != 0: - var parentAct *types.PullReqActivity - parentAct, err = c.checkIsReplyable(ctx, pr, in.ParentID) - if err != nil { - return nil, err - } - - act.ParentID = &parentAct.ID - act.Kind = parentAct.Kind - _ = act.SetPayload(types.PullRequestActivityPayloadComment{}) - - err = c.writeReplyActivity(ctx, parentAct, act) - default: - _ = act.SetPayload(types.PullRequestActivityPayloadComment{}) - err = c.writeActivity(ctx, pr, act) - } + pr, err = c.pullreqStore.FindByNumber(ctx, repo.ID, prNum) if err != nil { - return nil, fmt.Errorf("failed to create comment: %w", err) + return nil, fmt.Errorf("failed to find pull request by number: %w", err) } - pr, err = c.pullreqStore.UpdateOptLock(ctx, pr, func(pr *types.PullReq) error { + var cut git.DiffCutOutput + if in.IsCodeComment() { + // fetch code snippet from git for code comments + cut, err = c.fetchDiffCut(ctx, repo, pr, in) + if err != nil { + return nil, err + } + } + + err = controller.TxOptLock(ctx, c.tx, func(ctx context.Context) error { + var err error + + if pr == nil { + // the pull request was fetched before the transaction, we re-fetch it in case of the version conflict error + pr, err = c.pullreqStore.FindByNumber(ctx, repo.ID, prNum) + if err != nil { + return fmt.Errorf("failed to find pull request by number: %w", err) + } + } + + act = getCommentActivity(session, pr, in) + + // In the switch the pull request activity (the code comment) + // is written to the DB (as code comment, a reply, or ordinary comment). + switch { + case in.IsCodeComment(): + setAsCodeComment(act, cut, in.Path, in.SourceCommitSHA) + _ = act.SetPayload(&types.PullRequestActivityPayloadCodeComment{ + Title: cut.LinesHeader, + Lines: cut.Lines, + LineStartNew: in.LineStartNew, + LineEndNew: in.LineEndNew, + }) + + err = c.writeActivity(ctx, pr, act) + + case in.ParentID != 0: + var parentAct *types.PullReqActivity + + parentAct, err = c.checkIsReplyable(ctx, pr, in.ParentID) + if err != nil { + return err + } + + act.ParentID = &parentAct.ID + act.Kind = parentAct.Kind + _ = act.SetPayload(types.PullRequestActivityPayloadComment{}) + + err = c.writeReplyActivity(ctx, parentAct, act) + default: + _ = act.SetPayload(types.PullRequestActivityPayloadComment{}) + err = c.writeActivity(ctx, pr, act) + } + if err != nil { + return fmt.Errorf("failed to write pull request comment: %w", err) + } + pr.CommentCount++ if act.IsBlocking() { pr.UnresolvedCount++ } + + err = c.pullreqStore.Update(ctx, pr) + if err != nil { + return fmt.Errorf("failed to increment pull request comment counters: %w", err) + } + return nil - }) + }, controller.TxOptionResetFunc(func() { + pr = nil // on the version conflict error force re-fetch of the pull request + })) if err != nil { - // non-critical error - log.Ctx(ctx).Err(err).Msgf("failed to increment pull request comment counters") + return nil, err + } + + if in.IsCodeComment() { + // Migrate the comment if necessary... Note: we still need to return the code comment as is. + c.migrateCodeComment(ctx, repo, pr, in, act.AsCodeComment(), cut) } if err = c.sseStreamer.Publish(ctx, repo.ParentID, enum.SSETypePullRequestUpdated, pr); err != nil { log.Ctx(ctx).Warn().Err(err).Msg("failed to publish PR changed event") } + // if it's a regular comment publish a comment create event if !act.IsReply() && act.Type == enum.PullReqActivityTypeComment && act.Kind == enum.PullReqActivityKindComment { c.eventReporter.CommentCreated(ctx, &events.CommentCreatedPayload{ @@ -205,11 +206,15 @@ func (c *Controller) CommentCreate( SourceSHA: pr.SourceSHA, }) } + return act, nil } -func (c *Controller) checkIsReplyable(ctx context.Context, - pr *types.PullReq, parentID int64) (*types.PullReqActivity, error) { +func (c *Controller) checkIsReplyable( + ctx context.Context, + pr *types.PullReq, + parentID int64, +) (*types.PullReqActivity, error) { // make sure the parent comment exists, belongs to the same PR and isn't itself a reply parentAct, err := c.activityStore.Find(ctx, parentID) if errors.Is(err, store.ErrResourceNotFound) || parentAct == nil { @@ -319,3 +324,61 @@ func setAsCodeComment(a *types.PullReqActivity, cut git.DiffCutOutput, path, sou SpanOld: cut.Header.OldSpan, } } + +func (c *Controller) fetchDiffCut( + ctx context.Context, + repo *types.Repository, + pr *types.PullReq, + in *CommentCreateInput, +) (git.DiffCutOutput, error) { + cut, err := c.git.DiffCut(ctx, &git.DiffCutParams{ + ReadParams: git.ReadParams{RepoUID: repo.GitUID}, + SourceCommitSHA: in.SourceCommitSHA, + SourceBranch: pr.SourceBranch, + TargetCommitSHA: in.TargetCommitSHA, + TargetBranch: pr.TargetBranch, + Path: in.Path, + LineStart: in.LineStart, + LineStartNew: in.LineStartNew, + LineEnd: in.LineEnd, + LineEndNew: in.LineEndNew, + }) + if errors.AsStatus(err) == errors.StatusNotFound || errors.AsStatus(err) == errors.StatusPathNotFound { + return git.DiffCutOutput{}, usererror.BadRequest(errors.Message(err)) + } + if err != nil { + return git.DiffCutOutput{}, fmt.Errorf("failed to fetch git diff cut: %w", err) + } + + return cut, nil +} + +func (c *Controller) migrateCodeComment( + ctx context.Context, + repo *types.Repository, + pr *types.PullReq, + in *CommentCreateInput, + cc *types.CodeComment, + cut git.DiffCutOutput, +) { + needsNewLineMigrate := in.SourceCommitSHA != cut.LatestSourceSHA + needsOldLineMigrate := pr.MergeBaseSHA != cut.MergeBaseSHA + if !needsNewLineMigrate && !needsOldLineMigrate { + return + } + + comments := []*types.CodeComment{cc} + + if needsNewLineMigrate { + c.codeCommentMigrator.MigrateNew(ctx, repo.GitUID, cut.LatestSourceSHA, comments) + } + if needsOldLineMigrate { + c.codeCommentMigrator.MigrateOld(ctx, repo.GitUID, cut.MergeBaseSHA, comments) + } + + if errMigrateUpdate := c.codeCommentView.UpdateAll(ctx, comments); errMigrateUpdate != nil { + // non-critical error + log.Ctx(ctx).Err(errMigrateUpdate). + Msgf("failed to migrate code comment to the latest source/merge-base commit SHA") + } +} diff --git a/app/api/controller/tx.go b/app/api/controller/tx.go index 8cbb7767a..7e1dd638a 100644 --- a/app/api/controller/tx.go +++ b/app/api/controller/tx.go @@ -22,8 +22,14 @@ import ( "github.com/harness/gitness/store/database/dbtx" ) +// TxOptionRetryCount transaction option allows setting number of transaction executions reties. +// A transaction started with TxOptLock will be automatically retried in case of version conflict error. type TxOptionRetryCount int +// TxOptionResetFunc transaction provides a function that will be executed before the transaction retry. +// A transaction started with TxOptLock will be automatically retried in case of version conflict error. +type TxOptionResetFunc func() + // TxOptLock runs the provided function inside a database transaction. If optimistic lock error occurs // during the operation, the function will retry the whole transaction again (to the maximum of 5 times, // but this can be overridden by providing an additional TxOptionRetryCount option). @@ -33,10 +39,14 @@ func TxOptLock(ctx context.Context, opts ...interface{}, ) (err error) { tries := 5 + var resetFuncs []func() for _, opt := range opts { if n, ok := opt.(TxOptionRetryCount); ok { tries = int(n) } + if fn, ok := opt.(TxOptionResetFunc); ok { + resetFuncs = append(resetFuncs, fn) + } } for try := 0; try < tries; try++ { @@ -44,6 +54,10 @@ func TxOptLock(ctx context.Context, if !errors.Is(err, store.ErrVersionConflict) { break } + + for _, fn := range resetFuncs { + fn() + } } return