Skip to content

Only update needed columns when update user #2296

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Aug 12, 2017
Merged

Only update needed columns when update user #2296

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1ed48e5
only update needed columns when update user
lunny Aug 11, 2017
adf019a
fix missing update_unix column
lunny Aug 12, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cmd/admin.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ func runChangePassword(c *cli.Context) error {
return fmt.Errorf("%v", err)
}
user.EncodePasswd()
if err := models.UpdateUser(user); err != nil {
if err := models.UpdateUserCols(user, "passwd", "salt"); err != nil {
return fmt.Errorf("%v", err)
}

Expand Down
33 changes: 29 additions & 4 deletions models/user.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,7 @@ func (u *User) SetLastLogin() {
// UpdateDiffViewStyle updates the users diff view style
func (u *User) UpdateDiffViewStyle(style string) error {
u.DiffViewStyle = style
return UpdateUser(u)
return UpdateUserCols(u, "diff_view_style")
}

// AfterSet is invoked from XORM after setting the value of a field of this object.
Expand Down Expand Up @@ -860,7 +860,9 @@ func updateUser(e Engine, u *User) error {
if len(u.AvatarEmail) == 0 {
u.AvatarEmail = u.Email
}
u.Avatar = base.HashEmail(u.AvatarEmail)
if len(u.AvatarEmail) > 0 {
u.Avatar = base.HashEmail(u.AvatarEmail)
}
}

u.LowerName = strings.ToLower(u.Name)
Expand All @@ -877,6 +879,29 @@ func UpdateUser(u *User) error {
return updateUser(x, u)
}

// UpdateUserCols update user according special columns
func UpdateUserCols(u *User, cols ...string) error {
// Organization does not need email
u.Email = strings.ToLower(u.Email)
Copy link
Member

@sapk sapk Aug 11, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we iterate over cols array and with a switch only do needed operation when matching col ?

Example : https://play.golang.org/p/4pRMeP89Re

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't get what you mean.

Copy link
Member

@sapk sapk Aug 12, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it is overkill

// UpdateUserCols update user according special columns
func UpdateUserCols(u *User, cols ...string) error {
	//Format only when needed
	for _, c := range cols {
		switch c {
		case "email":
			// Organization does not need email
			u.Email = strings.ToLower(u.Email)
			if !u.IsOrganization() {
				if len(u.AvatarEmail) == 0 {
					u.AvatarEmail = u.Email
				}
			}
		case "name":
			u.LowerName = strings.ToLower(u.Name)
		case "location":
			u.Location = base.TruncateString(u.Location, 255)
		case "website":
			u.Website = base.TruncateString(u.Website, 255)
		case "description":
			u.Description = base.TruncateString(u.Description, 255)
		}
	}
	if !u.IsOrganization() {
		if len(u.AvatarEmail) > 0 {
			u.Avatar = base.HashEmail(u.AvatarEmail)
		}
	}
	_, err := x.Id(u.ID).Cols(cols...).Update(u)
	return err
}

if !u.IsOrganization() {
if len(u.AvatarEmail) == 0 {
u.AvatarEmail = u.Email
}
if len(u.AvatarEmail) > 0 {
u.Avatar = base.HashEmail(u.AvatarEmail)
}
}

u.LowerName = strings.ToLower(u.Name)
u.Location = base.TruncateString(u.Location, 255)
u.Website = base.TruncateString(u.Website, 255)
u.Description = base.TruncateString(u.Description, 255)

cols = append(cols, "updated_unix")
_, err := x.Id(u.ID).Cols(cols...).Update(u)
return err
}

// UpdateUserSetting updates user's settings.
func UpdateUserSetting(u *User) error {
if !u.IsOrganization() {
Expand Down Expand Up @@ -1418,7 +1443,7 @@ func SyncExternalUsers() {
}
usr.IsActive = true

err = UpdateUser(usr)
err = UpdateUserCols(usr, "full_name", "email", "is_admin", "is_active")
if err != nil {
log.Error(4, "SyncExternalUsers[%s]: Error updating user %s: %v", s.Name, usr.Name, err)
}
Expand All @@ -1440,7 +1465,7 @@ func SyncExternalUsers() {
log.Trace("SyncExternalUsers[%s]: Deactivating user %s", s.Name, usr.Name)

usr.IsActive = false
err = UpdateUser(&usr)
err = UpdateUserCols(&usr, "is_active")
if err != nil {
log.Error(4, "SyncExternalUsers[%s]: Error deactivating user %s: %v", s.Name, usr.Name, err)
}
Expand Down
2 changes: 1 addition & 1 deletion routers/api/v1/org/org.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ func Edit(ctx *context.APIContext, form api.EditOrgOption) {
org.Description = form.Description
org.Website = form.Website
org.Location = form.Location
if err := models.UpdateUser(org); err != nil {
if err := models.UpdateUserCols(org, "full_name", "description", "website", "location"); err != nil {
ctx.Error(500, "UpdateUser", err)
return
}
Expand Down
18 changes: 10 additions & 8 deletions routers/user/auth.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -340,8 +340,8 @@ func handleSignInFull(ctx *context.Context, u *models.User, remember bool, obeyR

// Register last login
u.SetLastLogin()
if err := models.UpdateUser(u); err != nil {
ctx.Handle(500, "UpdateUser", err)
if err := models.UpdateUserCols(u, "last_login_unix"); err != nil {
ctx.Handle(500, "UpdateUserCols", err)
return
}

Expand Down Expand Up @@ -430,8 +430,8 @@ func handleOAuth2SignIn(u *models.User, gothUser goth.User, ctx *context.Context

// Register last login
u.SetLastLogin()
if err := models.UpdateUser(u); err != nil {
ctx.Handle(500, "UpdateUser", err)
if err := models.UpdateUserCols(u, "last_login_unix"); err != nil {
ctx.Handle(500, "UpdateUserCols", err)
return
}

Expand Down Expand Up @@ -666,7 +666,8 @@ func LinkAccountPostRegister(ctx *context.Context, cpt *captcha.Captcha, form au
if models.CountUsers() == 1 {
u.IsAdmin = true
u.IsActive = true
if err := models.UpdateUser(u); err != nil {
u.SetLastLogin()
if err := models.UpdateUserCols(u, "is_admin", "is_active", "last_login_unix"); err != nil {
ctx.Handle(500, "UpdateUser", err)
return
}
Expand Down Expand Up @@ -781,7 +782,8 @@ func SignUpPost(ctx *context.Context, cpt *captcha.Captcha, form auth.RegisterFo
if models.CountUsers() == 1 {
u.IsAdmin = true
u.IsActive = true
if err := models.UpdateUser(u); err != nil {
u.SetLastLogin()
if err := models.UpdateUserCols(u, "is_admin", "is_active", "last_login_unix"); err != nil {
ctx.Handle(500, "UpdateUser", err)
return
}
Expand Down Expand Up @@ -840,7 +842,7 @@ func Activate(ctx *context.Context) {
ctx.Handle(500, "UpdateUser", err)
return
}
if err := models.UpdateUser(user); err != nil {
if err := models.UpdateUserCols(user, "is_active", "rands"); err != nil {
if models.IsErrUserNotExist(err) {
ctx.Error(404)
} else {
Expand Down Expand Up @@ -991,7 +993,7 @@ func ResetPasswdPost(ctx *context.Context) {
return
}
u.EncodePasswd()
if err := models.UpdateUser(u); err != nil {
if err := models.UpdateUserCols(u, "passwd", "rands", "salt"); err != nil {
ctx.Handle(500, "UpdateUser", err)
return
}
Expand Down
3 changes: 2 additions & 1 deletion routers/user/auth_openid.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -404,7 +404,8 @@ func RegisterOpenIDPost(ctx *context.Context, cpt *captcha.Captcha, form auth.Si
if models.CountUsers() == 1 {
u.IsAdmin = true
u.IsActive = true
if err := models.UpdateUser(u); err != nil {
u.SetLastLogin()
if err := models.UpdateUserCols(u, "is_admin", "is_active", "last_login_unix"); err != nil {
ctx.Handle(500, "UpdateUser", err)
return
}
Expand Down
4 changes: 2 additions & 2 deletions routers/user/setting.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ func UpdateAvatarSetting(ctx *context.Context, form auth.AvatarForm, ctxUser *mo
}
}

if err := models.UpdateUser(ctxUser); err != nil {
if err := models.UpdateUserCols(ctxUser, "avatar", "avatar_email", "use_custom_avatar"); err != nil {
return fmt.Errorf("UpdateUser: %v", err)
}

Expand Down Expand Up @@ -221,7 +221,7 @@ func SettingsPasswordPost(ctx *context.Context, form auth.ChangePasswordForm) {
return
}
ctx.User.EncodePasswd()
if err := models.UpdateUser(ctx.User); err != nil {
if err := models.UpdateUserCols(ctx.User, "salt", "passwd"); err != nil {
ctx.Handle(500, "UpdateUser", err)
return
}
Expand Down