Skip to content

Commit 08f571c

Browse files
eternal-flame-ADeternal-flame-AD
committed
enhance: serialize user update methods
1 parent 14bfc25 commit 08f571c

8 files changed

Lines changed: 92 additions & 69 deletions

File tree

‎api/user.go‎

Lines changed: 57 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -12,15 +12,17 @@ import (
1212
"github.com/gotify/server/v3/model"
1313
)
1414

15+
var errCannotDeleteLastAdmin = errors.New("cannot delete last admin")
16+
1517
// The UserDatabase interface for encapsulating database access.
16-
type UserDatabase interface {
17-
GetUsers() ([]*model.User, error)
18+
type UserDatabase[T UserDatabase[T]] interface {
19+
Txn(fn func(txdb T) error) error
20+
GetUsers(condition ...any) ([]*model.User, error)
1821
GetUserByID(id uint) (*model.User, error)
1922
GetUserByName(name string) (*model.User, error)
2023
DeleteUserByID(id uint) error
2124
UpdateUser(user *model.User) error
2225
CreateUser(user *model.User) error
23-
CountUser(condition ...any) (int64, error)
2426
}
2527

2628
// UserChangeNotifier notifies listeners for user changes.
@@ -58,8 +60,8 @@ func (c *UserChangeNotifier) fireUserAdded(uid uint) error {
5860
}
5961

6062
// The UserAPI provides handlers for managing users.
61-
type UserAPI struct {
62-
DB UserDatabase
63+
type UserAPI[T UserDatabase[T]] struct {
64+
DB T
6365
PasswordStrength int
6466
UserChangeNotifier *UserChangeNotifier
6567
Registration bool
@@ -90,7 +92,7 @@ type UserAPI struct {
9092
// description: Forbidden
9193
// schema:
9294
// $ref: "#/definitions/Error"
93-
func (a *UserAPI) GetUsers(ctx *gin.Context) {
95+
func (a *UserAPI[T]) GetUsers(ctx *gin.Context) {
9496
users, err := a.DB.GetUsers()
9597
if success := successOrAbort(ctx, 500, err); !success {
9698
return
@@ -126,7 +128,7 @@ func (a *UserAPI) GetUsers(ctx *gin.Context) {
126128
// description: Forbidden
127129
// schema:
128130
// $ref: "#/definitions/Error"
129-
func (a *UserAPI) GetCurrentUser(ctx *gin.Context) {
131+
func (a *UserAPI[T]) GetCurrentUser(ctx *gin.Context) {
130132
user, err := a.DB.GetUserByID(auth.GetUserID(ctx))
131133
if success := successOrAbort(ctx, 500, err); !success {
132134
return
@@ -185,7 +187,7 @@ func (a *UserAPI) GetCurrentUser(ctx *gin.Context) {
185187
// description: Forbidden
186188
// schema:
187189
// $ref: "#/definitions/Error"
188-
func (a *UserAPI) CreateUser(ctx *gin.Context) {
190+
func (a *UserAPI[T]) CreateUser(ctx *gin.Context) {
189191
user := model.CreateUserExternal{}
190192
if err := ctx.Bind(&user); err == nil {
191193
if err := password.ValidateNewPassword(user.Pass); err != nil {
@@ -286,7 +288,7 @@ func (a *UserAPI) CreateUser(ctx *gin.Context) {
286288
// description: Not Found
287289
// schema:
288290
// $ref: "#/definitions/Error"
289-
func (a *UserAPI) GetUserByID(ctx *gin.Context) {
291+
func (a *UserAPI[T]) GetUserByID(ctx *gin.Context) {
290292
withID(ctx, "id", func(id uint) {
291293
user, err := a.DB.GetUserByID(id)
292294
if success := successOrAbort(ctx, 500, err); !success {
@@ -336,26 +338,33 @@ func (a *UserAPI) GetUserByID(ctx *gin.Context) {
336338
// description: Not Found
337339
// schema:
338340
// $ref: "#/definitions/Error"
339-
func (a *UserAPI) DeleteUserByID(ctx *gin.Context) {
341+
func (a *UserAPI[T]) DeleteUserByID(ctx *gin.Context) {
340342
withID(ctx, "id", func(id uint) {
341343
user, err := a.DB.GetUserByID(id)
342344
if success := successOrAbort(ctx, 500, err); !success {
343345
return
344346
}
345347
if user != nil {
346-
adminCount, err := a.DB.CountUser(&model.User{Admin: true})
347-
if success := successOrAbort(ctx, 500, err); !success {
348-
return
349-
}
350-
if user.Admin && adminCount == 1 {
351-
ctx.AbortWithError(400, errors.New("cannot delete last admin"))
352-
return
353-
}
354-
if err := a.UserChangeNotifier.fireUserDeleted(id); err != nil {
355-
ctx.AbortWithError(500, err)
356-
return
348+
for range 3 {
349+
err = a.DB.Txn(func(txdb T) error {
350+
if err := txdb.DeleteUserByID(id); err != nil {
351+
return err
352+
}
353+
anotherAdmin, err := txdb.GetUsers(&model.User{Admin: true})
354+
if err != nil {
355+
return err
356+
}
357+
if user.Admin && len(anotherAdmin) == 0 {
358+
ctx.AbortWithError(400, errCannotDeleteLastAdmin)
359+
return errCannotDeleteLastAdmin
360+
}
361+
return a.UserChangeNotifier.fireUserDeleted(id)
362+
})
363+
if err == nil || ctx.IsAborted() {
364+
return
365+
}
357366
}
358-
successOrAbort(ctx, 500, a.DB.DeleteUserByID(id))
367+
successOrAbort(ctx, 500, err)
359368
} else {
360369
ctx.AbortWithError(404, errors.New("user does not exist"))
361370
}
@@ -395,7 +404,7 @@ func (a *UserAPI) DeleteUserByID(ctx *gin.Context) {
395404
// description: Forbidden
396405
// schema:
397406
// $ref: "#/definitions/Error"
398-
func (a *UserAPI) ChangePassword(ctx *gin.Context) {
407+
func (a *UserAPI[T]) ChangePassword(ctx *gin.Context) {
399408
pw := model.UserExternalPass{}
400409
if err := ctx.Bind(&pw); err == nil {
401410
if err := password.ValidateNewPassword(pw.Pass); err != nil {
@@ -461,7 +470,7 @@ func (a *UserAPI) ChangePassword(ctx *gin.Context) {
461470
// description: Not Found
462471
// schema:
463472
// $ref: "#/definitions/Error"
464-
func (a *UserAPI) UpdateUserByID(ctx *gin.Context) {
473+
func (a *UserAPI[T]) UpdateUserByID(ctx *gin.Context) {
465474
withID(ctx, "id", func(id uint) {
466475
var updatedUser *model.UpdateUserExternal
467476
if err := ctx.Bind(&updatedUser); err == nil {
@@ -470,15 +479,7 @@ func (a *UserAPI) UpdateUserByID(ctx *gin.Context) {
470479
return
471480
}
472481
if dbUser != nil {
473-
adminCount, err := a.DB.CountUser(&model.User{Admin: true})
474-
if success := successOrAbort(ctx, 500, err); !success {
475-
return
476-
}
477-
if !updatedUser.Admin && dbUser.Admin && adminCount == 1 {
478-
ctx.AbortWithError(400, errors.New("cannot delete last admin"))
479-
return
480-
}
481-
482+
dbUserWasAdmin := dbUser.Admin
482483
dbUser.Name = updatedUser.Name
483484
dbUser.Admin = updatedUser.Admin
484485

@@ -494,10 +495,30 @@ func (a *UserAPI) UpdateUserByID(ctx *gin.Context) {
494495
}
495496
dbUser.Pass = pw
496497
}
497-
if success := successOrAbort(ctx, 500, a.DB.UpdateUser(dbUser)); !success {
498-
return
498+
499+
for range 3 {
500+
err = a.DB.Txn(func(txdb T) error {
501+
if err := txdb.UpdateUser(dbUser); err != nil {
502+
return err
503+
}
504+
505+
anotherAdmin, err := txdb.GetUsers(&model.User{Admin: true})
506+
if err != nil {
507+
return err
508+
}
509+
if !updatedUser.Admin && dbUserWasAdmin && len(anotherAdmin) == 0 {
510+
ctx.AbortWithError(400, errCannotDeleteLastAdmin)
511+
return errCannotDeleteLastAdmin
512+
}
513+
514+
return nil
515+
})
516+
517+
if err == nil || ctx.IsAborted() {
518+
return
519+
}
499520
}
500-
ctx.JSON(200, toExternalUser(dbUser))
521+
successOrAbort(ctx, 500, err)
501522
} else {
502523
ctx.AbortWithError(404, errors.New("user does not exist"))
503524
}

‎api/user_test.go‎

Lines changed: 2 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ func TestUserSuite(t *testing.T) {
2525
type UserSuite struct {
2626
suite.Suite
2727
db *testdb.Database
28-
a *UserAPI
28+
a *UserAPI[*testdb.Database]
2929
ctx *gin.Context
3030
recorder *httptest.ResponseRecorder
3131
notifiedAdd bool
@@ -49,7 +49,7 @@ func (s *UserSuite) BeforeTest(suiteName, testName string) {
4949
s.notifiedAdd = true
5050
return nil
5151
})
52-
s.a = &UserAPI{DB: s.db, UserChangeNotifier: s.notifier}
52+
s.a = &UserAPI[*testdb.Database]{DB: s.db, UserChangeNotifier: s.notifier}
5353
}
5454

5555
func (s *UserSuite) AfterTest(suiteName, testName string) {
@@ -350,17 +350,6 @@ func (s *UserSuite) Test_UpdateUserByID_InvalidID() {
350350
assert.Equal(s.T(), 400, s.recorder.Code)
351351
}
352352

353-
func (s *UserSuite) Test_UpdateUserByID_EmptyPassword_Expect400() {
354-
s.loginAdmin()
355-
356-
s.ctx.Params = gin.Params{{Key: "id", Value: "1"}}
357-
358-
s.ctx.Request = httptest.NewRequest("POST", "/user/1", strings.NewReader(`{"name": "admin", "pass": "", "admin": false}`))
359-
s.ctx.Request.Header.Set("Content-Type", "application/json")
360-
s.a.UpdateUserByID(s.ctx)
361-
assert.Equal(s.T(), 400, s.recorder.Code)
362-
}
363-
364353
func (s *UserSuite) Test_UpdateUserByID_TooLongPassword_Expect400() {
365354
s.loginAdmin()
366355

‎database/database.go‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -172,14 +172,24 @@ func createDirectoryIfSqlite(dialect, connection string) {
172172

173173
// GormDatabase is a wrapper for the gorm framework.
174174
type GormDatabase struct {
175-
DB *gorm.DB
175+
DB *gorm.DB
176+
Nested bool
176177
}
177178

178179
// Close closes the gorm database connection.
179180
func (d *GormDatabase) Close() {
181+
if d.Nested {
182+
return
183+
}
180184
sqldb, err := d.DB.DB()
181185
if err != nil {
182186
return
183187
}
184188
sqldb.Close()
185189
}
190+
191+
func (d *GormDatabase) Txn(fn func(txdb *GormDatabase) error) error {
192+
return d.DB.Transaction(func(tx *gorm.DB) error {
193+
return fn(&GormDatabase{DB: tx, Nested: true})
194+
}, &sql.TxOptions{Isolation: sql.LevelSerializable})
195+
}

‎database/user.go‎

Lines changed: 7 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -44,23 +44,19 @@ func (d *GormDatabase) GetUserByID(id uint) (*model.User, error) {
4444
return nil, err
4545
}
4646

47-
// CountUser returns the user count which satisfies the given condition.
48-
func (d *GormDatabase) CountUser(condition ...any) (int64, error) {
49-
c := int64(-1)
47+
// GetUsers returns the users which satisfy the given condition.
48+
func (d *GormDatabase) GetUsers(condition ...any) ([]*model.User, error) {
49+
users := make([]*model.User, 0)
5050
handle := d.DB.Model(new(model.User))
5151
if len(condition) == 1 {
5252
handle = handle.Where(condition[0])
5353
} else if len(condition) > 1 {
5454
handle = handle.Where(condition[0], condition[1:]...)
5555
}
56-
err := handle.Count(&c).Error
57-
return c, err
58-
}
59-
60-
// GetUsers returns all users.
61-
func (d *GormDatabase) GetUsers() ([]*model.User, error) {
62-
var users []*model.User
63-
err := d.DB.Find(&users).Error
56+
err := handle.Find(&users).Error
57+
if err == gorm.ErrRecordNotFound {
58+
return nil, nil
59+
}
6460
return users, err
6561
}
6662

‎database/user_test.go‎

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -19,9 +19,10 @@ func (s *DatabaseSuite) TestUser() {
1919
require.NoError(s.T(), err)
2020
assert.NotNil(s.T(), jmattheis, "on bootup the first user should be automatically created")
2121

22-
adminCount, err := s.db.CountUser("admin = ?", true)
22+
admins, err := s.db.GetUsers("admin = ?", true)
2323
require.NoError(s.T(), err)
24-
assert.Equal(s.T(), int64(1), adminCount, "there is initially one admin")
24+
assert.Len(s.T(), admins, 1)
25+
assert.True(s.T(), admins[0].Admin, "the admin user should be an admin")
2526

2627
users, err := s.db.GetUsers()
2728
require.NoError(s.T(), err)
@@ -31,9 +32,9 @@ func (s *DatabaseSuite) TestUser() {
3132
nicories := &model.User{Name: "nicories", Pass: []byte{1, 2, 3, 4}, Admin: false}
3233
s.db.CreateUser(nicories)
3334
assert.NotEqual(s.T(), 0, nicories.ID, "on create user a new id should be assigned")
34-
userCount, err := s.db.CountUser()
35+
users, err = s.db.GetUsers()
3536
require.NoError(s.T(), err)
36-
assert.Equal(s.T(), int64(2), userCount, "two users should exist")
37+
assert.Len(s.T(), users, 2, "two users should exist")
3738

3839
user, err = s.db.GetUserByName("nicories")
3940
require.NoError(s.T(), err)
@@ -58,9 +59,9 @@ func (s *DatabaseSuite) TestUser() {
5859
require.NoError(s.T(), err)
5960
assert.Len(s.T(), users, 2)
6061

61-
adminCount, err = s.db.CountUser(&model.User{Admin: true})
62+
admins, err = s.db.GetUsers(&model.User{Admin: true})
6263
require.NoError(s.T(), err)
63-
assert.Equal(s.T(), int64(2), adminCount, "two admins exist")
64+
assert.Len(s.T(), admins, 2, "two admins exist")
6465

6566
require.NoError(s.T(), s.db.DeleteUserByID(tom.ID))
6667
users, err = s.db.GetUsers()

‎plugin/manager.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ import (
2323

2424
// The Database interface for encapsulating database access.
2525
type Database interface {
26-
GetUsers() ([]*model.User, error)
26+
GetUsers(condition ...any) ([]*model.User, error)
2727
GetPluginConfByUserAndPath(userid uint, path string) (*model.PluginConf, error)
2828
CreatePluginConf(p *model.PluginConf) error
2929
GetPluginConfByApplicationID(appid uint) (*model.PluginConf, error)

‎router/router.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -104,7 +104,7 @@ func Create(db *database.GormDatabase, vInfo *model.VersionInfo, conf *config.Co
104104
}
105105
sessionHandler := api.SessionAPI{DB: db, NotifyDeleted: streamHandler.NotifyDeletedClient, SecureCookie: conf.Server.SecureCookie, LocalAuthEnabled: conf.LocalAuthEnabled}
106106
userChangeNotifier := new(api.UserChangeNotifier)
107-
userHandler := api.UserAPI{DB: db, PasswordStrength: conf.PassStrength, UserChangeNotifier: userChangeNotifier, Registration: conf.Registration}
107+
userHandler := api.UserAPI[*database.GormDatabase]{DB: db, PasswordStrength: conf.PassStrength, UserChangeNotifier: userChangeNotifier, Registration: conf.Registration}
108108

109109
pluginManager, err := plugin.NewManager(db, conf.PluginsDir, g.Group("/plugin/:id/custom/"), streamHandler)
110110
if err != nil {

‎test/testdb/database.go‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,12 @@ type Database struct {
2020
t *testing.T
2121
}
2222

23+
func (d *Database) Txn(fn func(txdb *Database) error) error {
24+
return d.GormDatabase.Txn(func(txdb *database.GormDatabase) error {
25+
return fn(&Database{GormDatabase: txdb, t: d.t})
26+
})
27+
}
28+
2329
// AppClientBuilder has helper methods to create applications and clients.
2430
type AppClientBuilder struct {
2531
userID uint

0 commit comments

Comments
 (0)