Migrate User.Update method to sync by default (#10942)

app/bot.go

@@ -44,8 +44,8 @@ func (a *App) PatchBot(botUserId string, botPatch *model.BotPatch) (*model.Bot,
 	user.Username = patchedUser.Username
 	user.Email = patchedUser.Email
 	user.FirstName = patchedUser.FirstName
-	if result := <-a.Srv.Store.User().Update(user, true); result.Err != nil {
-		return nil, result.Err
+	if _, err := a.Srv.Store.User().Update(user, true); err != nil {
+		return nil, err
 	}
 
 	result := <-a.Srv.Store.Bot().Update(bot)

app/user.go

@@ -989,11 +989,11 @@ func (a *App) UpdateActive(user *model.User, active bool) (*model.User, *model.A
 		user.DeleteAt = model.GetMillis()
 	}
 
-	result := <-a.Srv.Store.User().Update(user, true)
-	if result.Err != nil {
-		return nil, result.Err
+	userUpdate, err := a.Srv.Store.User().Update(user, true)
+	if err != nil {
+		return nil, err
 	}
-	ruser := result.Data.([2]*model.User)[0]
+	ruser := userUpdate.New
 
 	if !active {
 		if err := a.userDeactivated(ruser); err != nil {
@@ -1110,7 +1110,7 @@ func (a *App) UpdateUser(user *model.User, sendNotifications bool) (*model.User,
 	if *a.Config().EmailSettings.RequireEmailVerification && prev.Email != user.Email {
 		newEmail = user.Email
 
-		_, err := a.GetUserByEmail(newEmail)
+		_, err = a.GetUserByEmail(newEmail)
 		if err == nil {
 			return nil, model.NewAppError("UpdateUser", "store.sql_user.update.email_taken.app_error", nil, "user_id="+user.Id, http.StatusBadRequest)
 		}
@@ -1118,32 +1118,31 @@ func (a *App) UpdateUser(user *model.User, sendNotifications bool) (*model.User,
 		user.Email = prev.Email
 	}
 
-	result := <-a.Srv.Store.User().Update(user, false)
-	if result.Err != nil {
-		return nil, result.Err
+	userUpdate, err := a.Srv.Store.User().Update(user, false)
+	if err != nil {
+		return nil, err
 	}
-	rusers := result.Data.([2]*model.User)
 
 	if sendNotifications {
-		if rusers[0].Email != rusers[1].Email || newEmail != "" {
+		if userUpdate.New.Email != userUpdate.Old.Email || newEmail != "" {
 			if *a.Config().EmailSettings.RequireEmailVerification {
 				a.Srv.Go(func() {
-					if err := a.SendEmailVerification(rusers[0], newEmail); err != nil {
+					if err := a.SendEmailVerification(userUpdate.New, newEmail); err != nil {
 						mlog.Error(err.Error())
 					}
 				})
 			} else {
 				a.Srv.Go(func() {
-					if err := a.SendEmailChangeEmail(rusers[1].Email, rusers[0].Email, rusers[0].Locale, a.GetSiteURL()); err != nil {
+					if err := a.SendEmailChangeEmail(userUpdate.Old.Email, userUpdate.New.Email, userUpdate.New.Locale, a.GetSiteURL()); err != nil {
 						mlog.Error(err.Error())
 					}
 				})
 			}
 		}
 
-		if rusers[0].Username != rusers[1].Username {
+		if userUpdate.New.Username != userUpdate.Old.Username {
 			a.Srv.Go(func() {
-				if err := a.SendChangeUsernameEmail(rusers[1].Username, rusers[0].Username, rusers[0].Email, rusers[0].Locale, a.GetSiteURL()); err != nil {
+				if err := a.SendChangeUsernameEmail(userUpdate.Old.Username, userUpdate.New.Username, userUpdate.New.Email, userUpdate.New.Locale, a.GetSiteURL()); err != nil {
 					mlog.Error(err.Error())
 				}
 			})
@@ -1161,7 +1160,7 @@ func (a *App) UpdateUser(user *model.User, sendNotifications bool) (*model.User,
 		})
 	}
 
-	return rusers[0], nil
+	return userUpdate.New, nil
 }
 
 func (a *App) UpdateUserActive(userId string, active bool) *model.AppError {
@@ -1375,14 +1374,19 @@ func (a *App) UpdateUserRoles(userId string, newRoles string, sendWebSocketEvent
 	}
 
 	user.Roles = newRoles
-	uchan := a.Srv.Store.User().Update(user, true)
+	uchan := make(chan store.StoreResult, 1)
+	go func() {
+		userUpdate, err := a.Srv.Store.User().Update(user, true)
+		uchan <- store.StoreResult{Data: userUpdate, Err: err}
+		close(uchan)
+	}()
 	schan := a.Srv.Store.Session().UpdateRoles(user.Id, newRoles)
 
 	result := <-uchan
 	if result.Err != nil {
 		return nil, result.Err
 	}
-	ruser := result.Data.([2]*model.User)[0]
+	ruser := result.Data.(*model.UserUpdate).New
 
 	if result := <-schan; result.Err != nil {
 		// soft error since the user roles were still updated
@@ -1886,12 +1890,12 @@ func (a *App) UpdateOAuthUserAttrs(userData io.Reader, user *model.User, provide
 	}
 
 	if userAttrsChanged {
-		result := <-a.Srv.Store.User().Update(user, true)
-		if result.Err != nil {
-			return result.Err
+		users, err := a.Srv.Store.User().Update(user, true)
+		if err != nil {
+			return err
 		}
 
-		user = result.Data.([2]*model.User)[0]
+		user = users.New
 		a.InvalidateCacheForUser(user.Id)
 
 		esInterface := a.Elasticsearch

model/user.go

@@ -87,6 +87,11 @@ type User struct {
 	TermsOfServiceCreateAt int64     `db:"-" json:"terms_of_service_create_at,omitempty"`
 }
 
+type UserUpdate struct {
+	Old *User
+	New *User
+}
+
 type UserPatch struct {
 	Username    *string   `json:"username"`
 	Password    *string   `json:"password,omitempty"`

store/sqlstore/user_store.go

@@ -142,71 +142,72 @@ func (us SqlUserStore) Save(user *model.User) store.StoreChannel {
 	})
 }
 
-func (us SqlUserStore) Update(user *model.User, trustedUpdateData bool) store.StoreChannel {
-	return store.Do(func(result *store.StoreResult) {
-		user.PreUpdate()
+func (us SqlUserStore) Update(user *model.User, trustedUpdateData bool) (*model.UserUpdate, *model.AppError) {
+	user.PreUpdate()
 
-		if result.Err = user.IsValid(); result.Err != nil {
-			return
+	if err := user.IsValid(); err != nil {
+		return nil, err
+	}
+
+	oldUserResult, err := us.GetMaster().Get(model.User{}, user.Id)
+	if err != nil {
+		return nil, model.NewAppError("SqlUserStore.Update", "store.sql_user.update.finding.app_error", nil, "user_id="+user.Id+", "+err.Error(), http.StatusInternalServerError)
+	}
+
+	if oldUserResult == nil {
+		return nil, model.NewAppError("SqlUserStore.Update", "store.sql_user.update.find.app_error", nil, "user_id="+user.Id, http.StatusBadRequest)
+	}
+
+	oldUser := oldUserResult.(*model.User)
+	user.CreateAt = oldUser.CreateAt
+	user.AuthData = oldUser.AuthData
+	user.AuthService = oldUser.AuthService
+	user.Password = oldUser.Password
+	user.LastPasswordUpdate = oldUser.LastPasswordUpdate
+	user.LastPictureUpdate = oldUser.LastPictureUpdate
+	user.EmailVerified = oldUser.EmailVerified
+	user.FailedAttempts = oldUser.FailedAttempts
+	user.MfaSecret = oldUser.MfaSecret
+	user.MfaActive = oldUser.MfaActive
+
+	if !trustedUpdateData {
+		user.Roles = oldUser.Roles
+		user.DeleteAt = oldUser.DeleteAt
+	}
+
+	if user.IsOAuthUser() {
+		if !trustedUpdateData {
+			user.Email = oldUser.Email
 		}
-
-		if oldUserResult, err := us.GetMaster().Get(model.User{}, user.Id); err != nil {
-			result.Err = model.NewAppError("SqlUserStore.Update", "store.sql_user.update.finding.app_error", nil, "user_id="+user.Id+", "+err.Error(), http.StatusInternalServerError)
-		} else if oldUserResult == nil {
-			result.Err = model.NewAppError("SqlUserStore.Update", "store.sql_user.update.find.app_error", nil, "user_id="+user.Id, http.StatusBadRequest)
-		} else {
-			oldUser := oldUserResult.(*model.User)
-			user.CreateAt = oldUser.CreateAt
-			user.AuthData = oldUser.AuthData
-			user.AuthService = oldUser.AuthService
-			user.Password = oldUser.Password
-			user.LastPasswordUpdate = oldUser.LastPasswordUpdate
-			user.LastPictureUpdate = oldUser.LastPictureUpdate
-			user.EmailVerified = oldUser.EmailVerified
-			user.FailedAttempts = oldUser.FailedAttempts
-			user.MfaSecret = oldUser.MfaSecret
-			user.MfaActive = oldUser.MfaActive
-
-			if !trustedUpdateData {
-				user.Roles = oldUser.Roles
-				user.DeleteAt = oldUser.DeleteAt
-			}
-
-			if user.IsOAuthUser() {
-				if !trustedUpdateData {
-					user.Email = oldUser.Email
-				}
-			} else if user.IsLDAPUser() && !trustedUpdateData {
-				if user.Username != oldUser.Username ||
-					user.Email != oldUser.Email {
-					result.Err = model.NewAppError("SqlUserStore.Update", "store.sql_user.update.can_not_change_ldap.app_error", nil, "user_id="+user.Id, http.StatusBadRequest)
-					return
-				}
-			} else if user.Email != oldUser.Email {
-				user.EmailVerified = false
-			}
-
-			if user.Username != oldUser.Username {
-				user.UpdateMentionKeysFromUsername(oldUser.Username)
-			}
-
-			if count, err := us.GetMaster().Update(user); err != nil {
-				if IsUniqueConstraintError(err, []string{"Email", "users_email_key", "idx_users_email_unique"}) {
-					result.Err = model.NewAppError("SqlUserStore.Update", "store.sql_user.update.email_taken.app_error", nil, "user_id="+user.Id+", "+err.Error(), http.StatusBadRequest)
-				} else if IsUniqueConstraintError(err, []string{"Username", "users_username_key", "idx_users_username_unique"}) {
-					result.Err = model.NewAppError("SqlUserStore.Update", "store.sql_user.update.username_taken.app_error", nil, "user_id="+user.Id+", "+err.Error(), http.StatusBadRequest)
-				} else {
-					result.Err = model.NewAppError("SqlUserStore.Update", "store.sql_user.update.updating.app_error", nil, "user_id="+user.Id+", "+err.Error(), http.StatusInternalServerError)
-				}
-			} else if count != 1 {
-				result.Err = model.NewAppError("SqlUserStore.Update", "store.sql_user.update.app_error", nil, fmt.Sprintf("user_id=%v, count=%v", user.Id, count), http.StatusInternalServerError)
-			} else {
-				user.Sanitize(map[string]bool{})
-				oldUser.Sanitize(map[string]bool{})
-				result.Data = [2]*model.User{user, oldUser}
-			}
+	} else if user.IsLDAPUser() && !trustedUpdateData {
+		if user.Username != oldUser.Username || user.Email != oldUser.Email {
+			return nil, model.NewAppError("SqlUserStore.Update", "store.sql_user.update.can_not_change_ldap.app_error", nil, "user_id="+user.Id, http.StatusBadRequest)
 		}
-	})
+	} else if user.Email != oldUser.Email {
+		user.EmailVerified = false
+	}
+
+	if user.Username != oldUser.Username {
+		user.UpdateMentionKeysFromUsername(oldUser.Username)
+	}
+
+	count, err := us.GetMaster().Update(user)
+	if err != nil {
+		if IsUniqueConstraintError(err, []string{"Email", "users_email_key", "idx_users_email_unique"}) {
+			return nil, model.NewAppError("SqlUserStore.Update", "store.sql_user.update.email_taken.app_error", nil, "user_id="+user.Id+", "+err.Error(), http.StatusBadRequest)
+		} else if IsUniqueConstraintError(err, []string{"Username", "users_username_key", "idx_users_username_unique"}) {
+			return nil, model.NewAppError("SqlUserStore.Update", "store.sql_user.update.username_taken.app_error", nil, "user_id="+user.Id+", "+err.Error(), http.StatusBadRequest)
+		}
+		return nil, model.NewAppError("SqlUserStore.Update", "store.sql_user.update.updating.app_error", nil, "user_id="+user.Id+", "+err.Error(), http.StatusInternalServerError)
+	}
+
+	if count != 1 {
+		return nil, model.NewAppError("SqlUserStore.Update", "store.sql_user.update.app_error", nil, fmt.Sprintf("user_id=%v, count=%v", user.Id, count), http.StatusInternalServerError)
+	}
+
+	user.Sanitize(map[string]bool{})
+	oldUser.Sanitize(map[string]bool{})
+	return &model.UserUpdate{New: user, Old: oldUser}, nil
 }
 
 func (us SqlUserStore) UpdateLastPictureUpdate(userId string) store.StoreChannel {

store/store.go

@@ -245,7 +245,7 @@ type PostStore interface {
 
 type UserStore interface {
 	Save(user *model.User) StoreChannel
-	Update(user *model.User, allowRoleUpdate bool) StoreChannel
+	Update(user *model.User, allowRoleUpdate bool) (*model.UserUpdate, *model.AppError)
 	UpdateLastPictureUpdate(userId string) StoreChannel
 	ResetLastPictureUpdate(userId string) StoreChannel
 	UpdateUpdateAt(userId string) StoreChannel

store/storetest/bot_store.go

@@ -175,7 +175,7 @@ func testBotStoreGetAll(t *testing.T, ss store.Store) {
 		t.Fatal("couldn't save user", err)
 	}
 	deletedUser.DeleteAt = model.GetMillis()
-	if err := (<-ss.User().Update(&deletedUser, true)).Err; err != nil {
+	if _, err := ss.User().Update(&deletedUser, true); err != nil {
 		t.Fatal("couldn't delete user", err)
 	}
 	defer func() { require.Nil(t, ss.User().PermanentDelete(deletedUser.Id)) }()

store/storetest/channel_store.go

@@ -3433,8 +3433,10 @@ func testChannelStoreRemoveAllDeactivatedMembers(t *testing.T, ss store.Store) {
 	// Deactivate users 1 & 2.
 	u1.DeleteAt = model.GetMillis()
 	u2.DeleteAt = model.GetMillis()
-	require.Nil(t, (<-ss.User().Update(&u1, true)).Err)
-	require.Nil(t, (<-ss.User().Update(&u2, true)).Err)
+	_, err = ss.User().Update(&u1, true)
+	require.Nil(t, err)
+	_, err = ss.User().Update(&u2, true)
+	require.Nil(t, err)
 
 	// Remove all deactivated users from the channel.
 	assert.Nil(t, (<-ss.Channel().RemoveAllDeactivatedMembers(c1.Id)).Err)

store/storetest/mocks/UserStore.go

@@ -747,19 +747,28 @@ func (_m *UserStore) SearchWithoutTeam(term string, options *model.UserSearchOpt
 }
 
 // Update provides a mock function with given fields: user, allowRoleUpdate
-func (_m *UserStore) Update(user *model.User, allowRoleUpdate bool) store.StoreChannel {
+func (_m *UserStore) Update(user *model.User, allowRoleUpdate bool) (*model.UserUpdate, *model.AppError) {
 	ret := _m.Called(user, allowRoleUpdate)
 
-	var r0 store.StoreChannel
-	if rf, ok := ret.Get(0).(func(*model.User, bool) store.StoreChannel); ok {
+	var r0 *model.UserUpdate
+	if rf, ok := ret.Get(0).(func(*model.User, bool) *model.UserUpdate); ok {
 		r0 = rf(user, allowRoleUpdate)
 	} else {
 		if ret.Get(0) != nil {
-			r0 = ret.Get(0).(store.StoreChannel)
+			r0 = ret.Get(0).(*model.UserUpdate)
 		}
 	}
 
-	return r0
+	var r1 *model.AppError
+	if rf, ok := ret.Get(1).(func(*model.User, bool) *model.AppError); ok {
+		r1 = rf(user, allowRoleUpdate)
+	} else {
+		if ret.Get(1) != nil {
+			r1 = ret.Get(1).(*model.AppError)
+		}
+	}
+
+	return r0, r1
 }
 
 // UpdateAuthData provides a mock function with given fields: userId, service, authData, email, resetMfa

store/storetest/post_store.go

@@ -2167,7 +2167,8 @@ func testPostStoreGetRepliesForExport(t *testing.T, ss store.Store) {
 
 	// Checking whether replies by deleted user are exported
 	u1.DeleteAt = 1002
-	store.Must(ss.User().Update(&u1, false))
+	_, err = ss.User().Update(&u1, false)
+	require.Nil(t, err)
 
 	r1 = <-ss.Post().GetRepliesForExport(p1.Id)
 	assert.Nil(t, r1.Err)

store/storetest/team_store.go

@@ -1026,7 +1026,8 @@ func testSaveTeamMemberMaxMembers(t *testing.T, ss store.Store) {
 	user2, err := ss.User().Get(userIds[1])
 	require.Nil(t, err)
 	user2.DeleteAt = 1234
-	store.Must(ss.User().Update(user2, true))
+	_, err = ss.User().Update(user2, true)
+	require.Nil(t, err)
 
 	newUserId2 := store.Must(ss.User().Save(&model.User{
 		Username: model.NewId(),

store/storetest/user_store.go

@@ -152,24 +152,24 @@ func testUserStoreUpdate(t *testing.T, ss store.Store) {
 
 	time.Sleep(100 * time.Millisecond)
 
-	if err := (<-ss.User().Update(u1, false)).Err; err != nil {
+	if _, err := ss.User().Update(u1, false); err != nil {
 		t.Fatal(err)
 	}
 
 	missing := &model.User{}
-	if err := (<-ss.User().Update(missing, false)).Err; err == nil {
+	if _, err := ss.User().Update(missing, false); err == nil {
 		t.Fatal("Update should have failed because of missing key")
 	}
 
 	newId := &model.User{
 		Id: model.NewId(),
 	}
-	if err := (<-ss.User().Update(newId, false)).Err; err == nil {
+	if _, err := ss.User().Update(newId, false); err == nil {
 		t.Fatal("Update should have failed because id change")
 	}
 
 	u2.Email = MakeEmail()
-	if err := (<-ss.User().Update(u2, false)).Err; err == nil {
+	if _, err := ss.User().Update(u2, false); err == nil {
 		t.Fatal("Update should have failed because you can't modify AD/LDAP fields")
 	}
 
@@ -183,20 +183,20 @@ func testUserStoreUpdate(t *testing.T, ss store.Store) {
 	store.Must(ss.Team().SaveMember(&model.TeamMember{TeamId: model.NewId(), UserId: u3.Id}, -1))
 
 	u3.Email = MakeEmail()
-	if result := <-ss.User().Update(u3, false); result.Err != nil {
+	if userUpdate, err := ss.User().Update(u3, false); err != nil {
 		t.Fatal("Update should not have failed")
 	} else {
-		newUser := result.Data.([2]*model.User)[0]
+		newUser := userUpdate.New
 		if newUser.Email != oldEmail {
 			t.Fatal("Email should not have been updated as the update is not trusted")
 		}
 	}
 
 	u3.Email = MakeEmail()
-	if result := <-ss.User().Update(u3, true); result.Err != nil {
+	if userUpdate, err := ss.User().Update(u3, true); err != nil {
 		t.Fatal("Update should not have failed")
 	} else {
-		newUser := result.Data.([2]*model.User)[0]
+		newUser := userUpdate.New
 		if newUser.Email == oldEmail {
 			t.Fatal("Email should have been updated as the update is trusted")
 		}