Merge pull request 'Do not update PRs based on events that happened before they existed' (#2932) from earl-warren/forgejo:wip-superfluous into forgejo

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/2932
Reviewed-by: oliverpool <oliverpool@noreply.codeberg.org>

models/fixtures/TestGetUnmergedPullRequestsByHeadInfoMax/issue.yml

@@ -0,0 +1,12 @@
+-
+  id: 1001
+  repo_id: 1
+  index: 1001
+  poster_id: 1
+  name: issue1
+  content: content for the first issue
+  is_pull: true
+  created: 111111111
+  created_unix: 946684800
+  updated_unix: 978307200
+  is_closed: false

models/fixtures/TestGetUnmergedPullRequestsByHeadInfoMax/pull_request.yml

@@ -0,0 +1,13 @@
+-
+  id: 1001
+  type: 0 # pull request
+  status: 2 # mergable
+  issue_id: 1001
+  index: 1001
+  head_repo_id: 1
+  base_repo_id: 1
+  head_branch: branchmax
+  base_branch: master
+  merge_base: 4a357436d925b5c974181ff12a994538ddc5a269
+  has_merged: false
+  flow: 0

models/forgejo_migrations/migrate.go

@@ -59,6 +59,8 @@ var migrations = []*Migration{
 	// v9 -> v10
 	NewMigration("Add pronouns to user", forgejo_v1_22.AddPronounsToUser),
 	// v11 -> v12
+	NewMigration("Add the `created` column to the `issue` table", forgejo_v1_22.AddCreatedToIssue),
+	// v12 -> v13
 	NewMigration("Add repo_archive_download_count table", forgejo_v1_22.AddRepoArchiveDownloadCount),
 }
 

models/forgejo_migrations/v1_22/v11.go

@@ -3,16 +3,17 @@
 
 package v1_22 //nolint
 
-import "xorm.io/xorm"
+import (
+	"code.gitea.io/gitea/modules/timeutil"
 
-func AddRepoArchiveDownloadCount(x *xorm.Engine) error {
-	type RepoArchiveDownloadCount struct {
-		ID        int64 `xorm:"pk autoincr"`
-		RepoID    int64 `xorm:"index unique(s)"`
-		ReleaseID int64 `xorm:"index unique(s)"`
-		Type      int   `xorm:"unique(s)"`
-		Count     int64
+	"xorm.io/xorm"
+)
+
+func AddCreatedToIssue(x *xorm.Engine) error {
+	type Issue struct {
+		ID      int64 `xorm:"pk autoincr"`
+		Created timeutil.TimeStampNano
 	}
 
-	return x.Sync(&RepoArchiveDownloadCount{})
+	return x.Sync(&Issue{})
 }

models/forgejo_migrations/v1_22/v12.go

@@ -0,0 +1,18 @@
+// Copyright 2024 The Forgejo Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package v1_22 //nolint
+
+import "xorm.io/xorm"
+
+func AddRepoArchiveDownloadCount(x *xorm.Engine) error {
+	type RepoArchiveDownloadCount struct {
+		ID        int64 `xorm:"pk autoincr"`
+		RepoID    int64 `xorm:"index unique(s)"`
+		ReleaseID int64 `xorm:"index unique(s)"`
+		Type      int   `xorm:"unique(s)"`
+		Count     int64
+	}
+
+	return x.Sync(&RepoArchiveDownloadCount{})
+}

models/issues/issue.go

@@ -124,6 +124,8 @@ type Issue struct {
 
 	DeadlineUnix timeutil.TimeStamp `xorm:"INDEX"`
 
+	Created timeutil.TimeStampNano
+
 	CreatedUnix timeutil.TimeStamp `xorm:"INDEX created"`
 	UpdatedUnix timeutil.TimeStamp `xorm:"INDEX updated"`
 	ClosedUnix  timeutil.TimeStamp `xorm:"INDEX"`

models/issues/issue_index.go

@@ -9,6 +9,14 @@ import (
 	"code.gitea.io/gitea/models/db"
 )
 
+func GetMaxIssueIndexForRepo(ctx context.Context, repoID int64) (int64, error) {
+	var max int64
+	if _, err := db.GetEngine(ctx).Select("MAX(`index`)").Table("issue").Where("repo_id=?", repoID).Get(&max); err != nil {
+		return 0, err
+	}
+	return max, nil
+}
+
 // RecalculateIssueIndexForRepo create issue_index for repo if not exist and
 // update it based on highest index of existing issues assigned to a repo
 func RecalculateIssueIndexForRepo(ctx context.Context, repoID int64) error {
@@ -18,8 +26,8 @@ func RecalculateIssueIndexForRepo(ctx context.Context, repoID int64) error {
 	}
 	defer committer.Close()
 
-	var max int64
-	if _, err = db.GetEngine(ctx).Select(" MAX(`index`)").Table("issue").Where("repo_id=?", repoID).Get(&max); err != nil {
+	max, err := GetMaxIssueIndexForRepo(ctx, repoID)
+	if err != nil {
 		return err
 	}
 

models/issues/issue_index_test.go

@@ -0,0 +1,38 @@
+// Copyright 2024 The Forgejo Authors
+// SPDX-License-Identifier: MIT
+
+package issues_test
+
+import (
+	"testing"
+
+	"code.gitea.io/gitea/models/db"
+	issues_model "code.gitea.io/gitea/models/issues"
+	repo_model "code.gitea.io/gitea/models/repo"
+	"code.gitea.io/gitea/models/unittest"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestGetMaxIssueIndexForRepo(t *testing.T) {
+	assert.NoError(t, unittest.PrepareTestDatabase())
+
+	repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1})
+
+	maxPR, err := issues_model.GetMaxIssueIndexForRepo(db.DefaultContext, repo.ID)
+	assert.NoError(t, err)
+
+	issue := testCreateIssue(t, repo.ID, repo.OwnerID, "title1", "content1", false)
+	assert.Greater(t, issue.Index, maxPR)
+
+	maxPR, err = issues_model.GetMaxIssueIndexForRepo(db.DefaultContext, repo.ID)
+	assert.NoError(t, err)
+
+	pull := testCreateIssue(t, repo.ID, repo.OwnerID, "title2", "content2", true)
+	assert.Greater(t, pull.Index, maxPR)
+
+	maxPR, err = issues_model.GetMaxIssueIndexForRepo(db.DefaultContext, repo.ID)
+	assert.NoError(t, err)
+
+	assert.Equal(t, maxPR, pull.Index)
+}

models/issues/issue_update.go

@@ -325,6 +325,8 @@ func NewIssueWithIndex(ctx context.Context, doer *user_model.User, opts NewIssue
 		return fmt.Errorf("issue exist")
 	}
 
+	opts.Issue.Created = timeutil.TimeStampNanoNow()
+
 	if _, err := e.Insert(opts.Issue); err != nil {
 		return err
 	}

models/issues/pull_list.go

@@ -47,6 +47,14 @@ func listPullRequestStatement(ctx context.Context, baseRepoID int64, opts *PullR
 	return sess, nil
 }
 
+func GetUnmergedPullRequestsByHeadInfoMax(ctx context.Context, repoID, olderThan int64, branch string) ([]*PullRequest, error) {
+	prs := make([]*PullRequest, 0, 2)
+	sess := db.GetEngine(ctx).
+		Join("INNER", "issue", "issue.id = `pull_request`.issue_id").
+		Where("`pull_request`.head_repo_id = ? AND `pull_request`.head_branch = ? AND `pull_request`.has_merged = ? AND `issue`.is_closed = ? AND `pull_request`.flow = ? AND (`issue`.`created` IS NULL OR `issue`.`created` <= ?)", repoID, branch, false, false, PullRequestFlowGithub, olderThan)
+	return prs, sess.Find(&prs)
+}
+
 // GetUnmergedPullRequestsByHeadInfo returns all pull requests that are open and has not been merged
 func GetUnmergedPullRequestsByHeadInfo(ctx context.Context, repoID int64, branch string) ([]*PullRequest, error) {
 	prs := make([]*PullRequest, 0, 2)

models/issues/pull_test.go

@@ -4,7 +4,9 @@
 package issues_test
 
 import (
+	"fmt"
 	"testing"
+	"time"
 
 	"code.gitea.io/gitea/models/db"
 	issues_model "code.gitea.io/gitea/models/issues"
@@ -12,6 +14,7 @@ import (
 	"code.gitea.io/gitea/models/unittest"
 	user_model "code.gitea.io/gitea/models/user"
 	"code.gitea.io/gitea/modules/setting"
+	"code.gitea.io/gitea/tests"
 
 	"github.com/stretchr/testify/assert"
 )
@@ -156,6 +159,100 @@ func TestGetUnmergedPullRequestsByHeadInfo(t *testing.T) {
 	}
 }
 
+func TestGetUnmergedPullRequestsByHeadInfoMax(t *testing.T) {
+	defer tests.AddFixtures("models/fixtures/TestGetUnmergedPullRequestsByHeadInfoMax/")()
+	assert.NoError(t, unittest.PrepareTestDatabase())
+
+	repoID := int64(1)
+	olderThan := int64(0)
+
+	// for NULL created field the olderThan condition is ignored
+	prs, err := issues_model.GetUnmergedPullRequestsByHeadInfoMax(db.DefaultContext, repoID, olderThan, "branch2")
+	assert.NoError(t, err)
+	assert.Equal(t, int64(1), prs[0].HeadRepoID)
+
+	// test for when the created field is set
+	branch := "branchmax"
+	prs, err = issues_model.GetUnmergedPullRequestsByHeadInfoMax(db.DefaultContext, repoID, olderThan, branch)
+	assert.NoError(t, err)
+	assert.Len(t, prs, 0)
+	olderThan = time.Now().UnixNano()
+	assert.NoError(t, err)
+	prs, err = issues_model.GetUnmergedPullRequestsByHeadInfoMax(db.DefaultContext, repoID, olderThan, branch)
+	assert.NoError(t, err)
+	assert.Len(t, prs, 1)
+	for _, pr := range prs {
+		assert.Equal(t, int64(1), pr.HeadRepoID)
+		assert.Equal(t, branch, pr.HeadBranch)
+	}
+	pr := prs[0]
+
+	for _, testCase := range []struct {
+		table   string
+		field   string
+		id      int64
+		match   any
+		nomatch any
+	}{
+		{
+			table:   "issue",
+			field:   "is_closed",
+			id:      pr.IssueID,
+			match:   false,
+			nomatch: true,
+		},
+		{
+			table:   "pull_request",
+			field:   "flow",
+			id:      pr.ID,
+			match:   issues_model.PullRequestFlowGithub,
+			nomatch: issues_model.PullRequestFlowAGit,
+		},
+		{
+			table:   "pull_request",
+			field:   "head_repo_id",
+			id:      pr.ID,
+			match:   pr.HeadRepoID,
+			nomatch: 0,
+		},
+		{
+			table:   "pull_request",
+			field:   "head_branch",
+			id:      pr.ID,
+			match:   pr.HeadBranch,
+			nomatch: "something else",
+		},
+		{
+			table:   "pull_request",
+			field:   "has_merged",
+			id:      pr.ID,
+			match:   false,
+			nomatch: true,
+		},
+	} {
+		t.Run(testCase.field, func(t *testing.T) {
+			update := fmt.Sprintf("UPDATE `%s` SET `%s` = ? WHERE `id` = ?", testCase.table, testCase.field)
+
+			// expect no match
+			_, err = db.GetEngine(db.DefaultContext).Exec(update, testCase.nomatch, testCase.id)
+			assert.NoError(t, err)
+			prs, err = issues_model.GetUnmergedPullRequestsByHeadInfoMax(db.DefaultContext, repoID, olderThan, branch)
+			assert.NoError(t, err)
+			assert.Len(t, prs, 0)
+
+			// expect one match
+			_, err = db.GetEngine(db.DefaultContext).Exec(update, testCase.match, testCase.id)
+			assert.NoError(t, err)
+			prs, err = issues_model.GetUnmergedPullRequestsByHeadInfoMax(db.DefaultContext, repoID, olderThan, branch)
+			assert.NoError(t, err)
+			assert.Len(t, prs, 1)
+
+			// identical to the known PR
+			assert.Equal(t, pr.ID, prs[0].ID)
+		})
+	}
+}
+
 func TestGetUnmergedPullRequestsByBaseInfo(t *testing.T) {
 	assert.NoError(t, unittest.PrepareTestDatabase())
 	prs, err := issues_model.GetUnmergedPullRequestsByBaseInfo(db.DefaultContext, 1, "master")