From 07d0f4c98f3b47db89dba304c63b08db522c04c5 Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Fri, 14 Mar 2025 12:51:55 +0100 Subject: [PATCH 1/8] WIP: fix broken DELETE query for RemoveUserBadges --- models/user/badge.go | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/models/user/badge.go b/models/user/badge.go index 3ff3530a369a5..228323246e4d7 100644 --- a/models/user/badge.go +++ b/models/user/badge.go @@ -86,6 +86,7 @@ func AddUserBadges(ctx context.Context, u *User, badges []*Badge) error { } else if !has { return fmt.Errorf("badge with slug %s doesn't exist", badge.Slug) } + // FIXME check uniqueness if err := db.Insert(ctx, &UserBadge{ BadgeID: badge.ID, UserID: u.ID, @@ -105,13 +106,23 @@ func RemoveUserBadge(ctx context.Context, u *User, badge *Badge) error { // RemoveUserBadges removes badges from a user. func RemoveUserBadges(ctx context.Context, u *User, badges []*Badge) error { return db.WithTx(ctx, func(ctx context.Context) error { + badgeSlugs := make([]string, 0, len(badges)) for _, badge := range badges { - if _, err := db.GetEngine(ctx). - Join("INNER", "badge", "badge.id = `user_badge`.badge_id"). - Where("`user_badge`.user_id=? AND `badge`.slug=?", u.ID, badge.Slug). - Delete(&UserBadge{}); err != nil { + badgeSlugs = append(badgeSlugs, badge.Slug) + } + var userBadges []UserBadge + if err := db.GetEngine(ctx).Table("user_badge"). + Join("INNER", "badge", "badge.id = `user_badge`.badge_id"). + Where("`user_badge`.user_id = ?", u.ID).In("`badge`.slug", badgeSlugs). + Find(&userBadges); err != nil { + return err + } + userBadgeIDs := make([]int64, 0, len(userBadges)) + for _, ub := range userBadges { + userBadgeIDs = append(userBadgeIDs, ub.ID) + } + if _, err := db.GetEngine(ctx).Table("user_badge").In("id", userBadgeIDs).Delete(); err != nil { return err - } } return nil }) From 612488aceb1e49c551a1e8c827d58036470c5446 Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Wed, 19 Mar 2025 16:06:45 +0100 Subject: [PATCH 2/8] User badges: add a test for adding and removing a badge --- models/fixtures/badge.yml | 5 +++++ models/user/badge_test.go | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+) create mode 100644 models/fixtures/badge.yml create mode 100644 models/user/badge_test.go diff --git a/models/fixtures/badge.yml b/models/fixtures/badge.yml new file mode 100644 index 0000000000000..438cd0ca5d4fe --- /dev/null +++ b/models/fixtures/badge.yml @@ -0,0 +1,5 @@ +- + id: 1 + slug: badge1 + description: just a test badge + image_url: badge1.png diff --git a/models/user/badge_test.go b/models/user/badge_test.go new file mode 100644 index 0000000000000..e874809434324 --- /dev/null +++ b/models/user/badge_test.go @@ -0,0 +1,33 @@ +// Copyright 2025 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package user_test + +import ( + "testing" + + "code.gitea.io/gitea/models/db" + "code.gitea.io/gitea/models/unittest" + user_model "code.gitea.io/gitea/models/user" + + "github.com/stretchr/testify/assert" +) + +func TestAddAndRemoveUserBadges(t *testing.T) { + assert.NoError(t, unittest.PrepareTestDatabase()) + badge1 := unittest.AssertExistsAndLoadBean(t, &user_model.Badge{ID: 1}) + user1 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) + + // Add a badge to user and verify that it is returned in the list + assert.NoError(t, user_model.AddUserBadge(db.DefaultContext, user1, badge1)) + badges, count, err := user_model.GetUserBadges(db.DefaultContext, user1) + assert.Equal(t, count, int64(1)) + assert.Equal(t, badges[0].Slug, badge1.Slug) + assert.Nil(t, err) + + // Remove a badge from user and verify that it is no longer in the list + assert.NoError(t, user_model.RemoveUserBadge(db.DefaultContext, user1, badge1)) + badges, count, err = user_model.GetUserBadges(db.DefaultContext, user1) + assert.Equal(t, count, int64(0)) + assert.Nil(t, err) +} From ed17512aa3cab472f549d6b200091c9e7094b726 Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Thu, 20 Mar 2025 15:37:37 +0100 Subject: [PATCH 3/8] make sure that (user_id, badge_id) tuples are unique this adds a db migration, and replaces an index on user_id with a compound unique index on (user_id, badge_id) that should cover the same queries as the old index --- models/migrations/v1_24/v315.go | 62 +++++++++++++++++++++++++++++++++ models/user/badge.go | 13 ++++++- 2 files changed, 74 insertions(+), 1 deletion(-) create mode 100644 models/migrations/v1_24/v315.go diff --git a/models/migrations/v1_24/v315.go b/models/migrations/v1_24/v315.go new file mode 100644 index 0000000000000..2a9fd8d171950 --- /dev/null +++ b/models/migrations/v1_24/v315.go @@ -0,0 +1,62 @@ +// Copyright 2025 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package v1_24 //nolint + +import ( + "fmt" + + "xorm.io/xorm" + "xorm.io/xorm/schemas" +) + +type UserBadge struct { //revive:disable-line:exported + ID int64 `xorm:"pk autoincr"` + BadgeID int64 + UserID int64 +} + +// TableIndices implements xorm's TableIndices interface +func (n *UserBadge) TableIndices() []*schemas.Index { + indices := make([]*schemas.Index, 0, 1) + ubUnique := schemas.NewIndex("unique_user_badge", schemas.UniqueType) + ubUnique.AddColumn("user_id", "badge_id") + indices = append(indices, ubUnique) + return indices +} + +// AddUniqueIndexForUserBadge adds a compound unique indexes for user badge table +// and it replaces an old index on user_id +func AddUniqueIndexForUserBadge(x *xorm.Engine) error { + // remove possible duplicated records in table user_badge + type result struct { + UserID int64 + BadgeID int64 + Cnt int + } + var results []result + if err := x.Select("user_id, badge_id, count(*) as cnt"). + Table("user_badge"). + GroupBy("user_id, badge_id"). + Having("count(*) > 1"). + Find(&results); err != nil { + return err + } + for _, r := range results { + if x.Dialect().URI().DBType == schemas.MSSQL { + if _, err := x.Exec(fmt.Sprintf("delete from user_badge where id in (SELECT top %d id FROM user_badge WHERE user_id = ? and badge_id = ?)", r.Cnt-1), r.UserID, r.BadgeID); err != nil { + return err + } + } else { + var ids []int64 + if err := x.SQL("SELECT id FROM user_badge WHERE user_id = ? and badge_id = ? limit ?", r.UserID, r.BadgeID, r.Cnt-1).Find(&ids); err != nil { + return err + } + if _, err := x.Table("user_badge").In("id", ids).Delete(); err != nil { + return err + } + } + } + + return x.Sync(new(UserBadge)) +} diff --git a/models/user/badge.go b/models/user/badge.go index 228323246e4d7..b6e80b54a8f05 100644 --- a/models/user/badge.go +++ b/models/user/badge.go @@ -8,6 +8,8 @@ import ( "fmt" "code.gitea.io/gitea/models/db" + + "xorm.io/xorm/schemas" ) // Badge represents a user badge @@ -22,7 +24,16 @@ type Badge struct { type UserBadge struct { //nolint:revive ID int64 `xorm:"pk autoincr"` BadgeID int64 - UserID int64 `xorm:"INDEX"` + UserID int64 +} + +// TableIndices implements xorm's TableIndices interface +func (n *UserBadge) TableIndices() []*schemas.Index { + indices := make([]*schemas.Index, 0, 1) + ubUnique := schemas.NewIndex("unique_user_badge", schemas.UniqueType) + ubUnique.AddColumn("user_id", "badge_id") + indices = append(indices, ubUnique) + return indices } func init() { From 41bdefc28577a5c96264c2bc5a91e237fdce148f Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Thu, 20 Mar 2025 15:45:15 +0100 Subject: [PATCH 4/8] Add a test for badge uniqueness --- models/user/badge_test.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/models/user/badge_test.go b/models/user/badge_test.go index e874809434324..4aefbaabdd7be 100644 --- a/models/user/badge_test.go +++ b/models/user/badge_test.go @@ -25,6 +25,14 @@ func TestAddAndRemoveUserBadges(t *testing.T) { assert.Equal(t, badges[0].Slug, badge1.Slug) assert.Nil(t, err) + // Confirm that it is impossible to duplicate the same badge + assert.Error(t, user_model.AddUserBadge(db.DefaultContext, user1, badge1)) + // Nothing happened to the existing badge + badges, count, err = user_model.GetUserBadges(db.DefaultContext, user1) + assert.Equal(t, count, int64(1)) + assert.Equal(t, badges[0].Slug, badge1.Slug) + assert.Nil(t, err) + // Remove a badge from user and verify that it is no longer in the list assert.NoError(t, user_model.RemoveUserBadge(db.DefaultContext, user1, badge1)) badges, count, err = user_model.GetUserBadges(db.DefaultContext, user1) From 79216c9b91ca16952d74425de43142e926c82fda Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Thu, 20 Mar 2025 15:47:38 +0100 Subject: [PATCH 5/8] remove FIXME comment --- models/user/badge.go | 1 - 1 file changed, 1 deletion(-) diff --git a/models/user/badge.go b/models/user/badge.go index b6e80b54a8f05..570b205b2d09b 100644 --- a/models/user/badge.go +++ b/models/user/badge.go @@ -97,7 +97,6 @@ func AddUserBadges(ctx context.Context, u *User, badges []*Badge) error { } else if !has { return fmt.Errorf("badge with slug %s doesn't exist", badge.Slug) } - // FIXME check uniqueness if err := db.Insert(ctx, &UserBadge{ BadgeID: badge.ID, UserID: u.ID, From 14ccbc72f39baf6896e8efd69db0e81e75fe3040 Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Thu, 20 Mar 2025 17:35:39 +0100 Subject: [PATCH 6/8] fix lint warnings --- models/user/badge.go | 2 +- models/user/badge_test.go | 19 ++++++++++--------- 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/models/user/badge.go b/models/user/badge.go index 570b205b2d09b..e40e27597618c 100644 --- a/models/user/badge.go +++ b/models/user/badge.go @@ -132,7 +132,7 @@ func RemoveUserBadges(ctx context.Context, u *User, badges []*Badge) error { userBadgeIDs = append(userBadgeIDs, ub.ID) } if _, err := db.GetEngine(ctx).Table("user_badge").In("id", userBadgeIDs).Delete(); err != nil { - return err + return err } return nil }) diff --git a/models/user/badge_test.go b/models/user/badge_test.go index 4aefbaabdd7be..fba9ea04f9986 100644 --- a/models/user/badge_test.go +++ b/models/user/badge_test.go @@ -21,21 +21,22 @@ func TestAddAndRemoveUserBadges(t *testing.T) { // Add a badge to user and verify that it is returned in the list assert.NoError(t, user_model.AddUserBadge(db.DefaultContext, user1, badge1)) badges, count, err := user_model.GetUserBadges(db.DefaultContext, user1) - assert.Equal(t, count, int64(1)) - assert.Equal(t, badges[0].Slug, badge1.Slug) - assert.Nil(t, err) + assert.Equal(t, int64(1), count) + assert.Equal(t, badge1.Slug, badges[0].Slug) + assert.NoError(t, err) // Confirm that it is impossible to duplicate the same badge assert.Error(t, user_model.AddUserBadge(db.DefaultContext, user1, badge1)) + // Nothing happened to the existing badge badges, count, err = user_model.GetUserBadges(db.DefaultContext, user1) - assert.Equal(t, count, int64(1)) - assert.Equal(t, badges[0].Slug, badge1.Slug) - assert.Nil(t, err) + assert.Equal(t, int64(1), count) + assert.Equal(t, badge1.Slug, badges[0].Slug) + assert.NoError(t, err) // Remove a badge from user and verify that it is no longer in the list assert.NoError(t, user_model.RemoveUserBadge(db.DefaultContext, user1, badge1)) - badges, count, err = user_model.GetUserBadges(db.DefaultContext, user1) - assert.Equal(t, count, int64(0)) - assert.Nil(t, err) + _, count, err = user_model.GetUserBadges(db.DefaultContext, user1) + assert.Equal(t, int64(0), count) + assert.NoError(t, err) } From 605a2dfb111221f259ff3fd7c4f9438ce0cde76a Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Thu, 20 Mar 2025 17:40:46 +0100 Subject: [PATCH 7/8] register new migration --- models/migrations/migrations.go | 1 + 1 file changed, 1 insertion(+) diff --git a/models/migrations/migrations.go b/models/migrations/migrations.go index 5f79a873f14e7..955e9bba15c6a 100644 --- a/models/migrations/migrations.go +++ b/models/migrations/migrations.go @@ -375,6 +375,7 @@ func prepareMigrationTasks() []*migration { newMigration(312, "Add DeleteBranchAfterMerge to AutoMerge", v1_24.AddDeleteBranchAfterMergeForAutoMerge), newMigration(313, "Move PinOrder from issue table to a new table issue_pin", v1_24.MovePinOrderToTableIssuePin), newMigration(314, "Update OwnerID as zero for repository level action tables", v1_24.UpdateOwnerIDOfRepoLevelActionsTables), + newMigration(315, "Add unique index for user_badge table", v1_24.AddUniqueIndexForUserBadge), } return preparedMigrations } From 7f5a9931dd172c30dfe474ccfed297638bd3f112 Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Thu, 20 Mar 2025 17:58:47 +0100 Subject: [PATCH 8/8] prepare for merge --- models/migrations/migrations.go | 2 +- models/migrations/v1_24/{v315.go => v318.go} | 0 2 files changed, 1 insertion(+), 1 deletion(-) rename models/migrations/v1_24/{v315.go => v318.go} (100%) diff --git a/models/migrations/migrations.go b/models/migrations/migrations.go index 955e9bba15c6a..0e570b4079686 100644 --- a/models/migrations/migrations.go +++ b/models/migrations/migrations.go @@ -375,7 +375,7 @@ func prepareMigrationTasks() []*migration { newMigration(312, "Add DeleteBranchAfterMerge to AutoMerge", v1_24.AddDeleteBranchAfterMergeForAutoMerge), newMigration(313, "Move PinOrder from issue table to a new table issue_pin", v1_24.MovePinOrderToTableIssuePin), newMigration(314, "Update OwnerID as zero for repository level action tables", v1_24.UpdateOwnerIDOfRepoLevelActionsTables), - newMigration(315, "Add unique index for user_badge table", v1_24.AddUniqueIndexForUserBadge), + newMigration(318, "Add unique index for user_badge table", v1_24.AddUniqueIndexForUserBadge), } return preparedMigrations } diff --git a/models/migrations/v1_24/v315.go b/models/migrations/v1_24/v318.go similarity index 100% rename from models/migrations/v1_24/v315.go rename to models/migrations/v1_24/v318.go