From 8031b34010582f3ff64b57e3fee8951d7ef9b9a6 Mon Sep 17 00:00:00 2001 From: Cory Todd Date: Thu, 18 May 2023 11:31:18 -0700 Subject: [PATCH 1/4] Fix duplicate Reviewed-by trailers Enable deduplication of unofficial reviews. When pull requests are configured to include all approvers, not just official ones, in the default merge messages it was possible to generate duplicated Reviewed-by lines for a single person. fixes #24795 Signed-off-by: Cory Todd --- models/issues/pull.go | 1 + models/issues/review.go | 6 ++++++ 2 files changed, 7 insertions(+) diff --git a/models/issues/pull.go b/models/issues/pull.go index 63bc258d2f39f..07ed6804c25eb 100644 --- a/models/issues/pull.go +++ b/models/issues/pull.go @@ -408,6 +408,7 @@ func (pr *PullRequest) getReviewedByLines(writer io.Writer) error { Type: ReviewTypeApprove, IssueID: pr.IssueID, OfficialOnly: setting.Repository.PullRequest.DefaultMergeMessageOfficialApproversOnly, + Distinct: true, }) if err != nil { log.Error("Unable to FindReviews for PR ID %d: %v", pr.ID, err) diff --git a/models/issues/review.go b/models/issues/review.go index 06cf132a48a3a..36756561518a3 100644 --- a/models/issues/review.go +++ b/models/issues/review.go @@ -243,6 +243,7 @@ type FindReviewOptions struct { IssueID int64 ReviewerID int64 OfficialOnly bool + Distinct bool } func (opts *FindReviewOptions) toCond() builder.Cond { @@ -269,9 +270,14 @@ func FindReviews(ctx context.Context, opts FindReviewOptions) ([]*Review, error) if opts.Page > 0 { sess = db.SetSessionPagination(sess, &opts) } + distinct := "" + if opts.Distinct { + distinct = "reviewer_id" + } return reviews, sess. Asc("created_unix"). Asc("id"). + Distinct(distinct). Find(&reviews) } From 970d85e82654dd16847849439b7b05e5f0ccfe5b Mon Sep 17 00:00:00 2001 From: Cory Todd Date: Sun, 21 May 2023 10:40:09 -0700 Subject: [PATCH 2/4] Add tests for FindReviews and GetApprovers Not all SQL dialects will work with Distinct so use a subquery grouping instead. Add 3 reviews to the test data to simulate the scenario being fixed by this patch. - One reviewer submitting 2 reviews. Only the latest should be returned. - One reviewer submitting 1 review. This is the oldest review and should still be the first approver. Omitting LatestOnly should preserve existing behavior as it is used by the REST API. Signed-off-by: Cory Todd --- models/fixtures/review.yml | 27 +++++++++++++++++++++++++++ models/issues/pull.go | 2 +- models/issues/pull_test.go | 12 ++++++++++++ models/issues/review.go | 11 ++++++----- models/issues/review_test.go | 25 +++++++++++++++++++++++++ 5 files changed, 71 insertions(+), 6 deletions(-) diff --git a/models/fixtures/review.yml b/models/fixtures/review.yml index d44d0cde98dc8..cc2c7e06e73e2 100644 --- a/models/fixtures/review.yml +++ b/models/fixtures/review.yml @@ -105,3 +105,30 @@ official: true updated_unix: 1603196749 created_unix: 1603196749 + +- + id: 13 + type: 1 + reviewer_id: 5 + issue_id: 11 + content: "old review from user5" + updated_unix: 946684820 + created_unix: 946684820 + +- + id: 14 + type: 1 + reviewer_id: 5 + issue_id: 11 + content: "duplicate review from user5 (latest)" + updated_unix: 946684830 + created_unix: 946684830 + +- + id: 15 + type: 1 + reviewer_id: 6 + issue_id: 11 + content: "singular review from user6 and final review for this pr" + updated_unix: 946684831 + created_unix: 946684831 diff --git a/models/issues/pull.go b/models/issues/pull.go index 07ed6804c25eb..a699cbde33927 100644 --- a/models/issues/pull.go +++ b/models/issues/pull.go @@ -408,7 +408,7 @@ func (pr *PullRequest) getReviewedByLines(writer io.Writer) error { Type: ReviewTypeApprove, IssueID: pr.IssueID, OfficialOnly: setting.Repository.PullRequest.DefaultMergeMessageOfficialApproversOnly, - Distinct: true, + LatestOnly: true, }) if err != nil { log.Error("Unable to FindReviews for PR ID %d: %v", pr.ID, err) diff --git a/models/issues/pull_test.go b/models/issues/pull_test.go index 1eb106047cea4..891340493e61f 100644 --- a/models/issues/pull_test.go +++ b/models/issues/pull_test.go @@ -10,6 +10,7 @@ import ( issues_model "code.gitea.io/gitea/models/issues" "code.gitea.io/gitea/models/unittest" user_model "code.gitea.io/gitea/models/user" + "code.gitea.io/gitea/modules/setting" "github.com/stretchr/testify/assert" ) @@ -325,3 +326,14 @@ func TestParseCodeOwnersLine(t *testing.T) { assert.Equal(t, g.Tokens, tokens, "Codeowners tokenizer failed") } } + +func TestGetApprovers(t *testing.T) { + assert.NoError(t, unittest.PrepareTestDatabase()) + pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 5}) + // Official reviews are already deduplicated. Allow unofficial reviews + // to assert that there are no duplicated approvers. + setting.Repository.PullRequest.DefaultMergeMessageOfficialApproversOnly = false + approvers := pr.GetApprovers() + expected := "Reviewed-by: User Five \nReviewed-by: User Six \n" + assert.EqualValues(t, expected, approvers) +} \ No newline at end of file diff --git a/models/issues/review.go b/models/issues/review.go index 36756561518a3..ac561c49ac296 100644 --- a/models/issues/review.go +++ b/models/issues/review.go @@ -243,7 +243,7 @@ type FindReviewOptions struct { IssueID int64 ReviewerID int64 OfficialOnly bool - Distinct bool + LatestOnly bool // Latest per-reviewer } func (opts *FindReviewOptions) toCond() builder.Cond { @@ -270,14 +270,15 @@ func FindReviews(ctx context.Context, opts FindReviewOptions) ([]*Review, error) if opts.Page > 0 { sess = db.SetSessionPagination(sess, &opts) } - distinct := "" - if opts.Distinct { - distinct = "reviewer_id" + if opts.LatestOnly { + sess.In("id", builder. + Select("max ( id ) "). + From("review"). + GroupBy("reviewer_id")) } return reviews, sess. Asc("created_unix"). Asc("id"). - Distinct(distinct). Find(&reviews) } diff --git a/models/issues/review_test.go b/models/issues/review_test.go index 0cb621812c159..b1565f84bc21c 100644 --- a/models/issues/review_test.go +++ b/models/issues/review_test.go @@ -71,6 +71,31 @@ func TestFindReviews(t *testing.T) { assert.Equal(t, "Demo Review", reviews[0].Content) } +func TestFindReviews_NotLatestOnly(t *testing.T) { + assert.NoError(t, unittest.PrepareTestDatabase()) + reviews, err := issues_model.FindReviews(db.DefaultContext, issues_model.FindReviewOptions{ + Type: issues_model.ReviewTypeApprove, + IssueID: 11, + LatestOnly: false, + }) + assert.NoError(t, err) + assert.Len(t, reviews, 3) + assert.Equal(t, "old review from user5", reviews[0].Content) +} + +func TestFindReviews_LatestOnly(t *testing.T) { + assert.NoError(t, unittest.PrepareTestDatabase()) + reviews, err := issues_model.FindReviews(db.DefaultContext, issues_model.FindReviewOptions{ + Type: issues_model.ReviewTypeApprove, + IssueID: 11, + LatestOnly: true, + }) + assert.NoError(t, err) + assert.Len(t, reviews, 2) + assert.Equal(t, "duplicate review from user5 (latest)", reviews[0].Content) + assert.Equal(t, "singular review from user6 and final review for this pr", reviews[1].Content) +} + func TestGetCurrentReview(t *testing.T) { assert.NoError(t, unittest.PrepareTestDatabase()) issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 2}) From bdba85b3492e94ad80daf11ec6db16f51873e070 Mon Sep 17 00:00:00 2001 From: Cory Todd Date: Mon, 5 Jun 2023 16:42:15 -0700 Subject: [PATCH 3/4] Constraint FindReviews with condition On the LatestOnly subquery, re-use the outter condition to avoid always grouping by all reviews in the database. Signed-off-by: Cory Todd --- models/issues/pull_test.go | 2 +- models/issues/review.go | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/models/issues/pull_test.go b/models/issues/pull_test.go index 891340493e61f..5856b5dc58638 100644 --- a/models/issues/pull_test.go +++ b/models/issues/pull_test.go @@ -336,4 +336,4 @@ func TestGetApprovers(t *testing.T) { approvers := pr.GetApprovers() expected := "Reviewed-by: User Five \nReviewed-by: User Six \n" assert.EqualValues(t, expected, approvers) -} \ No newline at end of file +} diff --git a/models/issues/review.go b/models/issues/review.go index ac561c49ac296..843bba3a84141 100644 --- a/models/issues/review.go +++ b/models/issues/review.go @@ -266,7 +266,8 @@ func (opts *FindReviewOptions) toCond() builder.Cond { // FindReviews returns reviews passing FindReviewOptions func FindReviews(ctx context.Context, opts FindReviewOptions) ([]*Review, error) { reviews := make([]*Review, 0, 10) - sess := db.GetEngine(ctx).Where(opts.toCond()) + cond := opts.toCond() + sess := db.GetEngine(ctx).Where(cond) if opts.Page > 0 { sess = db.SetSessionPagination(sess, &opts) } @@ -274,6 +275,7 @@ func FindReviews(ctx context.Context, opts FindReviewOptions) ([]*Review, error) sess.In("id", builder. Select("max ( id ) "). From("review"). + Where(cond). GroupBy("reviewer_id")) } return reviews, sess. From df4a52020651a1ba47d98221915e37733f0d06d6 Mon Sep 17 00:00:00 2001 From: Cory Todd Date: Thu, 8 Jun 2023 12:25:22 -0700 Subject: [PATCH 4/4] Introduce FindLatestReviews Do not create future sources for bugs by introducing the LatestOnly field. Use an explicit function instead. Signed-off-by: Cory Todd --- models/issues/pull.go | 3 +-- models/issues/review.go | 28 ++++++++++++++++++++-------- models/issues/review_test.go | 21 ++++----------------- 3 files changed, 25 insertions(+), 27 deletions(-) diff --git a/models/issues/pull.go b/models/issues/pull.go index a699cbde33927..23b938f95ab76 100644 --- a/models/issues/pull.go +++ b/models/issues/pull.go @@ -404,11 +404,10 @@ func (pr *PullRequest) getReviewedByLines(writer io.Writer) error { defer committer.Close() // Note: This doesn't page as we only expect a very limited number of reviews - reviews, err := FindReviews(ctx, FindReviewOptions{ + reviews, err := FindLatestReviews(ctx, FindReviewOptions{ Type: ReviewTypeApprove, IssueID: pr.IssueID, OfficialOnly: setting.Repository.PullRequest.DefaultMergeMessageOfficialApproversOnly, - LatestOnly: true, }) if err != nil { log.Error("Unable to FindReviews for PR ID %d: %v", pr.ID, err) diff --git a/models/issues/review.go b/models/issues/review.go index 843bba3a84141..3a1ab7468afca 100644 --- a/models/issues/review.go +++ b/models/issues/review.go @@ -243,7 +243,6 @@ type FindReviewOptions struct { IssueID int64 ReviewerID int64 OfficialOnly bool - LatestOnly bool // Latest per-reviewer } func (opts *FindReviewOptions) toCond() builder.Cond { @@ -265,19 +264,32 @@ func (opts *FindReviewOptions) toCond() builder.Cond { // FindReviews returns reviews passing FindReviewOptions func FindReviews(ctx context.Context, opts FindReviewOptions) ([]*Review, error) { + reviews := make([]*Review, 0, 10) + sess := db.GetEngine(ctx).Where(opts.toCond()) + if opts.Page > 0 { + sess = db.SetSessionPagination(sess, &opts) + } + return reviews, sess. + Asc("created_unix"). + Asc("id"). + Find(&reviews) +} + +// FindLatestReviews returns only latest reviews per user, passing FindReviewOptions +func FindLatestReviews(ctx context.Context, opts FindReviewOptions) ([]*Review, error) { reviews := make([]*Review, 0, 10) cond := opts.toCond() sess := db.GetEngine(ctx).Where(cond) if opts.Page > 0 { sess = db.SetSessionPagination(sess, &opts) } - if opts.LatestOnly { - sess.In("id", builder. - Select("max ( id ) "). - From("review"). - Where(cond). - GroupBy("reviewer_id")) - } + + sess.In("id", builder. + Select("max ( id ) "). + From("review"). + Where(cond). + GroupBy("reviewer_id")) + return reviews, sess. Asc("created_unix"). Asc("id"). diff --git a/models/issues/review_test.go b/models/issues/review_test.go index b1565f84bc21c..de7b6b2902897 100644 --- a/models/issues/review_test.go +++ b/models/issues/review_test.go @@ -71,24 +71,11 @@ func TestFindReviews(t *testing.T) { assert.Equal(t, "Demo Review", reviews[0].Content) } -func TestFindReviews_NotLatestOnly(t *testing.T) { +func TestFindLatestReviews(t *testing.T) { assert.NoError(t, unittest.PrepareTestDatabase()) - reviews, err := issues_model.FindReviews(db.DefaultContext, issues_model.FindReviewOptions{ - Type: issues_model.ReviewTypeApprove, - IssueID: 11, - LatestOnly: false, - }) - assert.NoError(t, err) - assert.Len(t, reviews, 3) - assert.Equal(t, "old review from user5", reviews[0].Content) -} - -func TestFindReviews_LatestOnly(t *testing.T) { - assert.NoError(t, unittest.PrepareTestDatabase()) - reviews, err := issues_model.FindReviews(db.DefaultContext, issues_model.FindReviewOptions{ - Type: issues_model.ReviewTypeApprove, - IssueID: 11, - LatestOnly: true, + reviews, err := issues_model.FindLatestReviews(db.DefaultContext, issues_model.FindReviewOptions{ + Type: issues_model.ReviewTypeApprove, + IssueID: 11, }) assert.NoError(t, err) assert.Len(t, reviews, 2)