From a66c90462da24a916ee62afcb5a1f79d06ed8399 Mon Sep 17 00:00:00 2001 From: Joe Chen Date: Sat, 5 Nov 2022 13:12:53 +0800 Subject: [PATCH] refactor(db): migrate methods off `user.go` and `org.go` (#7219) (#7227) --- internal/db/db.go | 1 + internal/db/org.go | 11 --- internal/db/org_users_test.go | 12 +++ internal/db/orgs.go | 76 +++++++++++++++ internal/db/orgs_test.go | 123 ++++++++++++++++++++++++ internal/db/user.go | 17 ---- internal/db/users.go | 28 +++--- internal/dbutil/logger.go | 2 +- internal/dbutil/string.go | 25 +++++ internal/dbutil/string_test.go | 25 +++++ internal/route/api/v1/org/org.go | 17 +++- internal/route/repo/pull.go | 13 ++- internal/route/user/home.go | 13 ++- templates/user/dashboard/dashboard.tmpl | 2 +- 14 files changed, 307 insertions(+), 58 deletions(-) create mode 100644 internal/db/orgs.go create mode 100644 internal/db/orgs_test.go create mode 100644 internal/dbutil/string.go create mode 100644 internal/dbutil/string_test.go diff --git a/internal/db/db.go b/internal/db/db.go index d291fc498..dfd23d81d 100644 --- a/internal/db/db.go +++ b/internal/db/db.go @@ -124,6 +124,7 @@ func Init(w logger.Writer) (*gorm.DB, error) { Follows = NewFollowsStore(db) LoginSources = &loginSources{DB: db, files: sourceFiles} LFS = &lfs{DB: db} + Orgs = NewOrgsStore(db) OrgUsers = NewOrgUsersStore(db) Perms = &perms{DB: db} Repos = NewReposStore(db) diff --git a/internal/db/org.go b/internal/db/org.go index d3148745c..1232c7ce1 100644 --- a/internal/db/org.go +++ b/internal/db/org.go @@ -297,17 +297,6 @@ func GetOwnedOrgsByUserIDDesc(userID int64, desc string) ([]*User, error) { return getOwnedOrgsByUserID(sess.Desc(desc), userID) } -// GetOrgIDsByUserID returns a list of organization IDs that user belongs to. -// The showPrivate indicates whether to include private memberships. -func GetOrgIDsByUserID(userID int64, showPrivate bool) ([]int64, error) { - orgIDs := make([]int64, 0, 5) - sess := x.Table("org_user").Where("uid = ?", userID) - if !showPrivate { - sess.And("is_public = ?", true) - } - return orgIDs, sess.Distinct("org_id").Find(&orgIDs) -} - func getOrgUsersByOrgID(e Engine, orgID int64, limit int) ([]*OrgUser, error) { orgUsers := make([]*OrgUser, 0, 10) diff --git a/internal/db/org_users_test.go b/internal/db/org_users_test.go index f5c486db6..ff7cd7b26 100644 --- a/internal/db/org_users_test.go +++ b/internal/db/org_users_test.go @@ -5,8 +5,10 @@ package db import ( + "context" "testing" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "gogs.io/gogs/internal/dbtest" @@ -43,9 +45,19 @@ func TestOrgUsers(t *testing.T) { } func orgUsersCountByUser(t *testing.T, db *orgUsers) { + ctx := context.Background() + // TODO: Use OrgUsers.Join to replace SQL hack when the method is available. err := db.Exec(`INSERT INTO org_user (uid, org_id) VALUES (?, ?)`, 1, 1).Error require.NoError(t, err) err = db.Exec(`INSERT INTO org_user (uid, org_id) VALUES (?, ?)`, 2, 1).Error require.NoError(t, err) + + got, err := db.CountByUser(ctx, 1) + require.NoError(t, err) + assert.Equal(t, int64(1), got) + + got, err = db.CountByUser(ctx, 404) + require.NoError(t, err) + assert.Equal(t, int64(0), got) } diff --git a/internal/db/orgs.go b/internal/db/orgs.go new file mode 100644 index 000000000..9fbedf114 --- /dev/null +++ b/internal/db/orgs.go @@ -0,0 +1,76 @@ +// Copyright 2022 The Gogs 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 db + +import ( + "context" + + "github.com/pkg/errors" + "gorm.io/gorm" + + "gogs.io/gogs/internal/dbutil" +) + +// OrgsStore is the persistent interface for organizations. +// +// NOTE: All methods are sorted in alphabetical order. +type OrgsStore interface { + // List returns a list of organizations filtered by options. + List(ctx context.Context, opts ListOrgOptions) ([]*Organization, error) +} + +var Orgs OrgsStore + +var _ OrgsStore = (*orgs)(nil) + +type orgs struct { + *gorm.DB +} + +// NewOrgsStore returns a persistent interface for orgs with given database +// connection. +func NewOrgsStore(db *gorm.DB) OrgsStore { + return &orgs{DB: db} +} + +type ListOrgOptions struct { + // Filter by the membership with the given user ID. + MemberID int64 + // Whether to include private memberships. + IncludePrivateMembers bool +} + +func (db *orgs) List(ctx context.Context, opts ListOrgOptions) ([]*Organization, error) { + if opts.MemberID <= 0 { + return nil, errors.New("MemberID must be greater than 0") + } + + /* + Equivalent SQL for PostgreSQL: + + SELECT * FROM "org" + JOIN org_user ON org_user.org_id = org.id + WHERE + org_user.uid = @memberID + [AND org_user.is_public = @includePrivateMembers] + ORDER BY org.id ASC + */ + tx := db.WithContext(ctx). + Joins(dbutil.Quote("JOIN org_user ON org_user.org_id = %s.id", "user")). + Where("org_user.uid = ?", opts.MemberID). + Order(dbutil.Quote("%s.id ASC", "user")) + if !opts.IncludePrivateMembers { + tx = tx.Where("org_user.is_public = ?", true) + } + + var orgs []*Organization + return orgs, tx.Find(&orgs).Error +} + +type Organization = User + +func (o *Organization) TableName() string { + return "user" +} diff --git a/internal/db/orgs_test.go b/internal/db/orgs_test.go new file mode 100644 index 000000000..bee7444b7 --- /dev/null +++ b/internal/db/orgs_test.go @@ -0,0 +1,123 @@ +// Copyright 2022 The Gogs 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 db + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "gogs.io/gogs/internal/dbtest" + "gogs.io/gogs/internal/dbutil" +) + +func TestOrgs(t *testing.T) { + if testing.Short() { + t.Skip() + } + t.Parallel() + + tables := []interface{}{new(User), new(EmailAddress), new(OrgUser)} + db := &orgs{ + DB: dbtest.NewDB(t, "orgs", tables...), + } + + for _, tc := range []struct { + name string + test func(t *testing.T, db *orgs) + }{ + {"List", orgsList}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Cleanup(func() { + err := clearTables(t, db.DB, tables...) + require.NoError(t, err) + }) + tc.test(t, db) + }) + if t.Failed() { + break + } + } +} + +func orgsList(t *testing.T, db *orgs) { + ctx := context.Background() + + usersStore := NewUsersStore(db.DB) + alice, err := usersStore.Create(ctx, "alice", "alice@example.com", CreateUserOptions{}) + require.NoError(t, err) + bob, err := usersStore.Create(ctx, "bob", "bob@example.com", CreateUserOptions{}) + require.NoError(t, err) + + // TODO: Use Orgs.Create to replace SQL hack when the method is available. + org1, err := usersStore.Create(ctx, "org1", "org1@example.com", CreateUserOptions{}) + require.NoError(t, err) + err = db.Exec( + dbutil.Quote("UPDATE %s SET %s = ? WHERE id = ?", "user", "type"), + UserTypeOrganization, org1.ID, + ).Error + require.NoError(t, err) + org2, err := usersStore.Create(ctx, "org2", "org2@example.com", CreateUserOptions{}) + require.NoError(t, err) + err = db.Exec( + dbutil.Quote("UPDATE %s SET %s = ? WHERE id = ?", "user", "type"), + UserTypeOrganization, org2.ID, + ).Error + require.NoError(t, err) + + // TODO: Use OrgUsers.Join to replace SQL hack when the method is available. + err = db.Exec(`INSERT INTO org_user (uid, org_id, is_public) VALUES (?, ?, ?)`, alice.ID, org1.ID, false).Error + require.NoError(t, err) + err = db.Exec(`INSERT INTO org_user (uid, org_id, is_public) VALUES (?, ?, ?)`, alice.ID, org2.ID, true).Error + require.NoError(t, err) + err = db.Exec(`INSERT INTO org_user (uid, org_id, is_public) VALUES (?, ?, ?)`, bob.ID, org2.ID, true).Error + require.NoError(t, err) + + tests := []struct { + name string + opts ListOrgOptions + wantOrgNames []string + }{ + { + name: "only public memberships for a user", + opts: ListOrgOptions{ + MemberID: alice.ID, + IncludePrivateMembers: false, + }, + wantOrgNames: []string{org2.Name}, + }, + { + name: "all memberships for a user", + opts: ListOrgOptions{ + MemberID: alice.ID, + IncludePrivateMembers: true, + }, + wantOrgNames: []string{org1.Name, org2.Name}, + }, + { + name: "no membership for a non-existent user", + opts: ListOrgOptions{ + MemberID: 404, + IncludePrivateMembers: true, + }, + wantOrgNames: []string{}, + }, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + got, err := db.List(ctx, test.opts) + require.NoError(t, err) + + gotOrgNames := make([]string, len(got)) + for i := range got { + gotOrgNames[i] = got[i].Name + } + assert.Equal(t, test.wantOrgNames, gotOrgNames) + }) + } +} diff --git a/internal/db/user.go b/internal/db/user.go index ea9472242..a5e4d65c2 100644 --- a/internal/db/user.go +++ b/internal/db/user.go @@ -60,23 +60,6 @@ func (u *User) getOrganizationCount(e Engine) (int64, error) { return e.Where("uid=?", u.ID).Count(new(OrgUser)) } -// GetOrganizations returns all organizations that user belongs to. -func (u *User) GetOrganizations(showPrivate bool) error { - orgIDs, err := GetOrgIDsByUserID(u.ID, showPrivate) - if err != nil { - return fmt.Errorf("GetOrgIDsByUserID: %v", err) - } - if len(orgIDs) == 0 { - return nil - } - - u.Orgs = make([]*User, 0, len(orgIDs)) - if err = x.Where("type = ?", UserTypeOrganization).In("id", orgIDs).Find(&u.Orgs); err != nil { - return err - } - return nil -} - // IsUserExist checks if given user name exist, // the user name should be noncased unique. // If uid is presented, then check will rule out that one, diff --git a/internal/db/users.go b/internal/db/users.go index af0db7275..146a65a46 100644 --- a/internal/db/users.go +++ b/internal/db/users.go @@ -20,6 +20,7 @@ import ( "gogs.io/gogs/internal/auth" "gogs.io/gogs/internal/conf" "gogs.io/gogs/internal/cryptoutil" + "gogs.io/gogs/internal/dbutil" "gogs.io/gogs/internal/errutil" "gogs.io/gogs/internal/osutil" "gogs.io/gogs/internal/strutil" @@ -381,16 +382,13 @@ func (db *users) ListFollowers(ctx context.Context, userID int64, page, pageSize LIMIT @limit OFFSET @offset */ users := make([]*User, 0, pageSize) - tx := db.WithContext(ctx). + return users, db.WithContext(ctx). + Joins(dbutil.Quote("LEFT JOIN follow ON follow.user_id = %s.id", "user")). Where("follow.follow_id = ?", userID). Limit(pageSize).Offset((page - 1) * pageSize). - Order("follow.id DESC") - if conf.UsePostgreSQL { - tx.Joins(`LEFT JOIN follow ON follow.user_id = "user".id`) - } else { - tx.Joins(`LEFT JOIN follow ON follow.user_id = user.id`) - } - return users, tx.Find(&users).Error + Order("follow.id DESC"). + Find(&users). + Error } func (db *users) ListFollowings(ctx context.Context, userID int64, page, pageSize int) ([]*User, error) { @@ -404,16 +402,13 @@ func (db *users) ListFollowings(ctx context.Context, userID int64, page, pageSiz LIMIT @limit OFFSET @offset */ users := make([]*User, 0, pageSize) - tx := db.WithContext(ctx). + return users, db.WithContext(ctx). + Joins(dbutil.Quote("LEFT JOIN follow ON follow.follow_id = %s.id", "user")). Where("follow.user_id = ?", userID). Limit(pageSize).Offset((page - 1) * pageSize). - Order("follow.id DESC") - if conf.UsePostgreSQL { - tx.Joins(`LEFT JOIN follow ON follow.follow_id = "user".id`) - } else { - tx.Joins(`LEFT JOIN follow ON follow.follow_id = user.id`) - } - return users, tx.Find(&users).Error + Order("follow.id DESC"). + Find(&users). + Error } func (db *users) UseCustomAvatar(ctx context.Context, userID int64, avatar []byte) error { @@ -452,7 +447,6 @@ type User struct { LoginSource int64 `xorm:"NOT NULL DEFAULT 0" gorm:"not null;default:0"` LoginName string Type UserType - Orgs []*User `xorm:"-" gorm:"-" json:"-"` Location string Website string Rands string `xorm:"VARCHAR(10)" gorm:"type:VARCHAR(10)"` diff --git a/internal/dbutil/logger.go b/internal/dbutil/logger.go index c189949ea..742976357 100644 --- a/internal/dbutil/logger.go +++ b/internal/dbutil/logger.go @@ -15,5 +15,5 @@ type Logger struct { } func (l *Logger) Printf(format string, args ...interface{}) { - fmt.Fprintf(l.Writer, format, args...) + _, _ = fmt.Fprintf(l.Writer, format, args...) } diff --git a/internal/dbutil/string.go b/internal/dbutil/string.go new file mode 100644 index 000000000..4ee3f0125 --- /dev/null +++ b/internal/dbutil/string.go @@ -0,0 +1,25 @@ +// Copyright 2022 The Gogs 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 dbutil + +import ( + "fmt" + + "gogs.io/gogs/internal/conf" +) + +// Quote adds surrounding double quotes for all given arguments before being +// formatted if the current database is UsePostgreSQL. +func Quote(format string, args ...string) string { + anys := make([]any, len(args)) + for i := range args { + if conf.UsePostgreSQL { + anys[i] = `"` + args[i] + `"` + } else { + anys[i] = args[i] + } + } + return fmt.Sprintf(format, anys...) +} diff --git a/internal/dbutil/string_test.go b/internal/dbutil/string_test.go new file mode 100644 index 000000000..ec01f5808 --- /dev/null +++ b/internal/dbutil/string_test.go @@ -0,0 +1,25 @@ +// Copyright 2022 The Gogs 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 dbutil + +import ( + "testing" + + "github.com/stretchr/testify/assert" + + "gogs.io/gogs/internal/conf" +) + +func TestQuote(t *testing.T) { + conf.UsePostgreSQL = true + got := Quote("SELECT * FROM %s", "user") + want := `SELECT * FROM "user"` + assert.Equal(t, want, got) + conf.UsePostgreSQL = false + + got = Quote("SELECT * FROM %s", "user") + want = `SELECT * FROM user` + assert.Equal(t, want, got) +} diff --git a/internal/route/api/v1/org/org.go b/internal/route/api/v1/org/org.go index 4d987dbfc..cefe5cc62 100644 --- a/internal/route/api/v1/org/org.go +++ b/internal/route/api/v1/org/org.go @@ -43,14 +43,21 @@ func CreateOrgForUser(c *context.APIContext, apiForm api.CreateOrgOption, user * } func listUserOrgs(c *context.APIContext, u *db.User, all bool) { - if err := u.GetOrganizations(all); err != nil { - c.Error(err, "get organization") + orgs, err := db.Orgs.List( + c.Req.Context(), + db.ListOrgOptions{ + MemberID: u.ID, + IncludePrivateMembers: all, + }, + ) + if err != nil { + c.Error(err, "list organizations") return } - apiOrgs := make([]*api.Organization, len(u.Orgs)) - for i := range u.Orgs { - apiOrgs[i] = convert.ToOrganization(u.Orgs[i]) + apiOrgs := make([]*api.Organization, len(orgs)) + for i := range orgs { + apiOrgs[i] = convert.ToOrganization(orgs[i]) } c.JSONSuccess(&apiOrgs) } diff --git a/internal/route/repo/pull.go b/internal/route/repo/pull.go index 2745c3369..c203266c2 100644 --- a/internal/route/repo/pull.go +++ b/internal/route/repo/pull.go @@ -69,11 +69,18 @@ func parseBaseRepository(c *context.Context) *db.Repository { } c.Data["ForkFrom"] = baseRepo.Owner.Name + "/" + baseRepo.Name - if err := c.User.GetOrganizations(true); err != nil { - c.Error(err, "get organizations") + orgs, err := db.Orgs.List( + c.Req.Context(), + db.ListOrgOptions{ + MemberID: c.User.ID, + IncludePrivateMembers: true, + }, + ) + if err != nil { + c.Error(err, "list organizations") return nil } - c.Data["Orgs"] = c.User.Orgs + c.Data["Orgs"] = orgs return baseRepo } diff --git a/internal/route/user/home.go b/internal/route/user/home.go index 4d3503981..04b5eb65e 100644 --- a/internal/route/user/home.go +++ b/internal/route/user/home.go @@ -40,11 +40,18 @@ func getDashboardContextUser(c *context.Context) *db.User { } c.Data["ContextUser"] = ctxUser - if err := c.User.GetOrganizations(true); err != nil { - c.Error(err, "get organizations") + orgs, err := db.Orgs.List( + c.Req.Context(), + db.ListOrgOptions{ + MemberID: c.User.ID, + IncludePrivateMembers: true, + }, + ) + if err != nil { + c.Error(err, "list organizations") return nil } - c.Data["Orgs"] = c.User.Orgs + c.Data["Orgs"] = orgs return ctxUser } diff --git a/templates/user/dashboard/dashboard.tmpl b/templates/user/dashboard/dashboard.tmpl index b9a5d3403..14af4a84d 100644 --- a/templates/user/dashboard/dashboard.tmpl +++ b/templates/user/dashboard/dashboard.tmpl @@ -87,7 +87,7 @@