From 9c9c3348bb5ec3daacb43ab31a9cdf3f2872e46b Mon Sep 17 00:00:00 2001 From: zeripath Date: Fri, 21 Aug 2020 08:53:14 +0100 Subject: [PATCH] Prevent NPE on commenting on lines with invalidated comments (with migration) (#12549) * Prevent NPE on commenting on lines with invalidated comments Only check for a review if we are replying to a previous review. Prevent the NPE in #12239 by assuming that a comment without a Review is non-pending. Fix #12239 Signed-off-by: Andrew Thornton * Add hack around to show the broken comments Signed-off-by: Andrew Thornton * Add migration and remove template hacks Signed-off-by: Andrew Thornton --- models/migrations/migrations.go | 2 + models/migrations/v147.go | 132 ++++++++++++++++++++++++++++++++ services/pull/review.go | 2 +- 3 files changed, 135 insertions(+), 1 deletion(-) create mode 100644 models/migrations/v147.go diff --git a/models/migrations/migrations.go b/models/migrations/migrations.go index b9fdfbfe7..721b045fd 100644 --- a/models/migrations/migrations.go +++ b/models/migrations/migrations.go @@ -226,6 +226,8 @@ var migrations = []Migration{ NewMigration("Increase Language field to 50 in LanguageStats", increaseLanguageField), // v146 -> v147 NewMigration("Add projects info to repository table", addProjectsInfo), + // v147 -> v148 + NewMigration("create review for 0 review id code comments", createReviewsForCodeComments), } // GetCurrentDBVersion returns the current db version diff --git a/models/migrations/v147.go b/models/migrations/v147.go new file mode 100644 index 000000000..9716d6e83 --- /dev/null +++ b/models/migrations/v147.go @@ -0,0 +1,132 @@ +// 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 migrations + +import ( + "code.gitea.io/gitea/modules/timeutil" + + "xorm.io/xorm" +) + +func createReviewsForCodeComments(x *xorm.Engine) error { + // Review + type Review struct { + ID int64 `xorm:"pk autoincr"` + Type int + ReviewerID int64 `xorm:"index"` + OriginalAuthor string + OriginalAuthorID int64 + IssueID int64 `xorm:"index"` + Content string `xorm:"TEXT"` + // Official is a review made by an assigned approver (counts towards approval) + Official bool `xorm:"NOT NULL DEFAULT false"` + CommitID string `xorm:"VARCHAR(40)"` + Stale bool `xorm:"NOT NULL DEFAULT false"` + + CreatedUnix timeutil.TimeStamp `xorm:"INDEX created"` + UpdatedUnix timeutil.TimeStamp `xorm:"INDEX updated"` + } + + const ReviewTypeComment = 2 + + // Comment represents a comment in commit and issue page. + type Comment struct { + ID int64 `xorm:"pk autoincr"` + Type int `xorm:"INDEX"` + PosterID int64 `xorm:"INDEX"` + OriginalAuthor string + OriginalAuthorID int64 + IssueID int64 `xorm:"INDEX"` + LabelID int64 + OldProjectID int64 + ProjectID int64 + OldMilestoneID int64 + MilestoneID int64 + AssigneeID int64 + RemovedAssignee bool + ResolveDoerID int64 + OldTitle string + NewTitle string + OldRef string + NewRef string + DependentIssueID int64 + + CommitID int64 + Line int64 // - previous line / + proposed line + TreePath string + Content string `xorm:"TEXT"` + + // Path represents the 4 lines of code cemented by this comment + PatchQuoted string `xorm:"TEXT patch"` + + CreatedUnix timeutil.TimeStamp `xorm:"INDEX created"` + UpdatedUnix timeutil.TimeStamp `xorm:"INDEX updated"` + + // Reference issue in commit message + CommitSHA string `xorm:"VARCHAR(40)"` + + ReviewID int64 `xorm:"index"` + Invalidated bool + + // Reference an issue or pull from another comment, issue or PR + // All information is about the origin of the reference + RefRepoID int64 `xorm:"index"` // Repo where the referencing + RefIssueID int64 `xorm:"index"` + RefCommentID int64 `xorm:"index"` // 0 if origin is Issue title or content (or PR's) + RefAction int `xorm:"SMALLINT"` // What hapens if RefIssueID resolves + RefIsPull bool + } + + if err := x.Sync2(new(Review), new(Comment)); err != nil { + return err + } + sess := x.NewSession() + defer sess.Close() + if err := sess.Begin(); err != nil { + return err + } + if err := sess.Where("review_id = 0 and type = 21").Iterate(new(Comment), func(idx int, bean interface{}) error { + comment := bean.(*Comment) + + review := &Review{ + Type: ReviewTypeComment, + ReviewerID: comment.PosterID, + IssueID: comment.IssueID, + Official: false, + CommitID: comment.CommitSHA, + Stale: comment.Invalidated, + OriginalAuthor: comment.OriginalAuthor, + OriginalAuthorID: comment.OriginalAuthorID, + CreatedUnix: comment.CreatedUnix, + UpdatedUnix: comment.CreatedUnix, + } + if _, err := sess.NoAutoTime().Insert(review); err != nil { + return err + } + + reviewComment := &Comment{ + Type: 22, + PosterID: comment.PosterID, + Content: "", + IssueID: comment.IssueID, + ReviewID: review.ID, + OriginalAuthor: comment.OriginalAuthor, + OriginalAuthorID: comment.OriginalAuthorID, + CreatedUnix: comment.CreatedUnix, + UpdatedUnix: comment.CreatedUnix, + } + if _, err := sess.NoAutoTime().Insert(reviewComment); err != nil { + return err + } + + comment.ReviewID = review.ID + _, err := sess.ID(comment.ID).Cols("review_id").NoAutoTime().Update(comment) + return err + }); err != nil { + return err + } + + return sess.Commit() +} diff --git a/services/pull/review.go b/services/pull/review.go index 25e0ca858..5a77a4da1 100644 --- a/services/pull/review.go +++ b/services/pull/review.go @@ -29,7 +29,7 @@ func CreateCodeComment(doer *models.User, gitRepo *git.Repository, issue *models // - Comments that are part of a review // - Comments that reply to an existing review - if !isReview { + if !isReview && replyReviewID != 0 { // It's not part of a review; maybe a reply to a review comment or a single comment. // Check if there are reviews for that line already; if there are, this is a reply if existsReview, err = models.ReviewExists(issue, treePath, line); err != nil {