Fix PR nil-dereference bug (#2195)

* Fix PR nil-dereference bug

* Revert to original error format

Author: ethantkoenig

models/issue.go

@@ -148,6 +148,19 @@ func (issue *Issue) loadAssignee(e Engine) (err error) {
 	return
 }
 
+func (issue *Issue) loadPullRequest(e Engine) (err error) {
+	if issue.IsPull && issue.PullRequest == nil {
+		issue.PullRequest, err = getPullRequestByIssueID(e, issue.ID)
+		if err != nil {
+			if IsErrPullRequestNotExist(err) {
+				return err
+			}
+			return fmt.Errorf("getPullRequestByIssueID [%d]: %v", issue.ID, err)
+		}
+	}
+	return nil
+}
+
 func (issue *Issue) loadAttributes(e Engine) (err error) {
 	if err = issue.loadRepo(e); err != nil {
 		return
@@ -172,12 +185,9 @@ func (issue *Issue) loadAttributes(e Engine) (err error) {
 		return
 	}
 
-	if issue.IsPull && issue.PullRequest == nil {
+	if err = issue.loadPullRequest(e); err != nil && !IsErrPullRequestNotExist(err) {
 		// It is possible pull request is not yet created.
-		issue.PullRequest, err = getPullRequestByIssueID(e, issue.ID)
-		if err != nil && !IsErrPullRequestNotExist(err) {
-			return fmt.Errorf("getPullRequestByIssueID [%d]: %v", issue.ID, err)
-		}
+		return err
 	}
 
 	if issue.Attachments == nil {
@@ -321,8 +331,15 @@ func (issue *Issue) HasLabel(labelID int64) bool {
 func (issue *Issue) sendLabelUpdatedWebhook(doer *User) {
 	var err error
 	if issue.IsPull {
-		err = issue.PullRequest.LoadIssue()
-		if err != nil {
+		if err = issue.loadRepo(x); err != nil {
+			log.Error(4, "loadRepo: %v", err)
+			return
+		}
+		if err = issue.loadPullRequest(x); err != nil {
+			log.Error(4, "loadPullRequest: %v", err)
+			return
+		}
+		if err = issue.PullRequest.LoadIssue(); err != nil {
 			log.Error(4, "LoadIssue: %v", err)
 			return
 		}
@@ -430,6 +447,8 @@ func (issue *Issue) ClearLabels(doer *User) (err error) {
 
 	if err := issue.loadRepo(sess); err != nil {
 		return err
+	} else if err = issue.loadPullRequest(sess); err != nil {
+		return err
 	}
 
 	if has, err := hasAccess(sess, doer.ID, issue.Repo, AccessModeWrite); err != nil {

models/issue_test.go

@@ -81,3 +81,68 @@ func TestGetParticipantsByIssueID(t *testing.T) {
 	// Users 3 and 5 made actual comments (see fixtures/comment.yml)
 	checkParticipants(1, []int{3, 5})
 }
+
+func TestIssue_AddLabel(t *testing.T) {
+	var tests = []struct {
+		issueID int64
+		labelID int64
+		doerID  int64
+	}{
+		{1, 2, 2}, // non-pull-request, not-already-added label
+		{1, 1, 2}, // non-pull-request, already-added label
+		{2, 2, 2}, // pull-request, not-already-added label
+		{2, 1, 2}, // pull-request, already-added label
+	}
+	for _, test := range tests {
+		assert.NoError(t, PrepareTestDatabase())
+		issue := AssertExistsAndLoadBean(t, &Issue{ID: test.issueID}).(*Issue)
+		label := AssertExistsAndLoadBean(t, &Label{ID: test.labelID}).(*Label)
+		doer := AssertExistsAndLoadBean(t, &User{ID: test.doerID}).(*User)
+		assert.NoError(t, issue.AddLabel(doer, label))
+		AssertExistsAndLoadBean(t, &IssueLabel{IssueID: test.issueID, LabelID: test.labelID})
+	}
+}
+
+func TestIssue_AddLabels(t *testing.T) {
+	var tests = []struct {
+		issueID  int64
+		labelIDs []int64
+		doerID   int64
+	}{
+		{1, []int64{1, 2}, 2}, // non-pull-request
+		{1, []int64{}, 2},     // non-pull-request, empty
+		{2, []int64{1, 2}, 2}, // pull-request
+		{2, []int64{}, 1},     // pull-request, empty
+	}
+	for _, test := range tests {
+		assert.NoError(t, PrepareTestDatabase())
+		issue := AssertExistsAndLoadBean(t, &Issue{ID: test.issueID}).(*Issue)
+		labels := make([]*Label, len(test.labelIDs))
+		for i, labelID := range test.labelIDs {
+			labels[i] = AssertExistsAndLoadBean(t, &Label{ID: labelID}).(*Label)
+		}
+		doer := AssertExistsAndLoadBean(t, &User{ID: test.doerID}).(*User)
+		assert.NoError(t, issue.AddLabels(doer, labels))
+		for _, labelID := range test.labelIDs {
+			AssertExistsAndLoadBean(t, &IssueLabel{IssueID: test.issueID, LabelID: labelID})
+		}
+	}
+}
+
+func TestIssue_ClearLabels(t *testing.T) {
+	var tests = []struct {
+		issueID int64
+		doerID  int64
+	}{
+		{1, 2}, // non-pull-request, has labels
+		{2, 2}, // pull-request, has labels
+		{3, 2}, // pull-request, has no labels
+	}
+	for _, test := range tests {
+		assert.NoError(t, PrepareTestDatabase())
+		issue := AssertExistsAndLoadBean(t, &Issue{ID: test.issueID}).(*Issue)
+		doer := AssertExistsAndLoadBean(t, &User{ID: test.doerID}).(*User)
+		assert.NoError(t, issue.ClearLabels(doer))
+		AssertNotExistsBean(t, &IssueLabel{IssueID: test.issueID})
+	}
+}