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 <art27@cantab.net>

* Add hack around to show the broken comments

Signed-off-by: Andrew Thornton <art27@cantab.net>

* Add migration and remove template hacks

Signed-off-by: Andrew Thornton <art27@cantab.net>

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

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()
+}

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 {