From bd3f72254d81c4b32fd3f5adfd4ba3961c6ee51a Mon Sep 17 00:00:00 2001 From: Shobhit Gupta Date: Thu, 20 Jun 2019 05:37:06 -0700 Subject: [PATCH] Migrate Channel.MigrateChannelMembers to Sync by default (#11277) --- migrations/advanced_permissions_phase_2.go | 7 +- store/sqlstore/channel_store.go | 108 ++++++++++----------- store/store.go | 2 +- store/storetest/channel_store.go | 7 +- store/storetest/mocks/ChannelStore.go | 19 +++- 5 files changed, 72 insertions(+), 71 deletions(-) diff --git a/migrations/advanced_permissions_phase_2.go b/migrations/advanced_permissions_phase_2.go index 55b1876c46..e4a72b0bc0 100644 --- a/migrations/advanced_permissions_phase_2.go +++ b/migrations/advanced_permissions_phase_2.go @@ -87,16 +87,15 @@ func (worker *Worker) runAdvancedPermissionsPhase2Migration(lastDone string) (bo } } else if progress.CurrentTable == "ChannelMembers" { // Run a ChannelMembers migration batch. - if result := <-worker.app.Srv.Store.Channel().MigrateChannelMembers(progress.LastChannelId, progress.LastUserId); result.Err != nil { - return false, progress.ToJson(), result.Err + if data, err := worker.app.Srv.Store.Channel().MigrateChannelMembers(progress.LastChannelId, progress.LastUserId); err != nil { + return false, progress.ToJson(), err } else { - if result.Data == nil { + if data == nil { // We haven't progressed. That means we've reached the end of this final stage of the migration. return true, progress.ToJson(), nil } - data := result.Data.(map[string]string) progress.LastChannelId = data["ChannelId"] progress.LastUserId = data["UserId"] } diff --git a/store/sqlstore/channel_store.go b/store/sqlstore/channel_store.go index 867466bbff..e25a0aa089 100644 --- a/store/sqlstore/channel_store.go +++ b/store/sqlstore/channel_store.go @@ -2292,70 +2292,64 @@ func (s SqlChannelStore) GetChannelsByScheme(schemeId string, offset int, limit // in batches as a single transaction per batch to ensure consistency but to also minimise execution time to avoid // causing unnecessary table locks. **THIS FUNCTION SHOULD NOT BE USED FOR ANY OTHER PURPOSE.** Executing this function // *after* the new Schemes functionality has been used on an installation will have unintended consequences. -func (s SqlChannelStore) MigrateChannelMembers(fromChannelId string, fromUserId string) store.StoreChannel { - return store.Do(func(result *store.StoreResult) { - var transaction *gorp.Transaction - var err error +func (s SqlChannelStore) MigrateChannelMembers(fromChannelId string, fromUserId string) (map[string]string, *model.AppError) { + var transaction *gorp.Transaction + var err error - if transaction, err = s.GetMaster().Begin(); err != nil { - result.Err = model.NewAppError("SqlChannelStore.MigrateChannelMembers", "store.sql_channel.migrate_channel_members.open_transaction.app_error", nil, err.Error(), http.StatusInternalServerError) - return + if transaction, err = s.GetMaster().Begin(); err != nil { + return nil, model.NewAppError("SqlChannelStore.MigrateChannelMembers", "store.sql_channel.migrate_channel_members.open_transaction.app_error", nil, err.Error(), http.StatusInternalServerError) + } + defer finalizeTransaction(transaction) + + var channelMembers []channelMember + if _, err := transaction.Select(&channelMembers, "SELECT * from ChannelMembers WHERE (ChannelId, UserId) > (:FromChannelId, :FromUserId) ORDER BY ChannelId, UserId LIMIT 100", map[string]interface{}{"FromChannelId": fromChannelId, "FromUserId": fromUserId}); err != nil { + return nil, model.NewAppError("SqlChannelStore.MigrateChannelMembers", "store.sql_channel.migrate_channel_members.select.app_error", nil, err.Error(), http.StatusInternalServerError) + } + + if len(channelMembers) == 0 { + // No more channel members in query result means that the migration has finished. + return nil, nil + } + + for _, member := range channelMembers { + roles := strings.Fields(member.Roles) + var newRoles []string + if !member.SchemeAdmin.Valid { + member.SchemeAdmin = sql.NullBool{Bool: false, Valid: true} } - defer finalizeTransaction(transaction) + if !member.SchemeUser.Valid { + member.SchemeUser = sql.NullBool{Bool: false, Valid: true} + } + if !member.SchemeGuest.Valid { + member.SchemeGuest = sql.NullBool{Bool: false, Valid: true} + } + for _, role := range roles { + if role == model.CHANNEL_ADMIN_ROLE_ID { + member.SchemeAdmin = sql.NullBool{Bool: true, Valid: true} + } else if role == model.CHANNEL_USER_ROLE_ID { + member.SchemeUser = sql.NullBool{Bool: true, Valid: true} + } else if role == model.CHANNEL_GUEST_ROLE_ID { + member.SchemeGuest = sql.NullBool{Bool: true, Valid: true} + } else { + newRoles = append(newRoles, role) + } + } + member.Roles = strings.Join(newRoles, " ") - var channelMembers []channelMember - if _, err := transaction.Select(&channelMembers, "SELECT * from ChannelMembers WHERE (ChannelId, UserId) > (:FromChannelId, :FromUserId) ORDER BY ChannelId, UserId LIMIT 100", map[string]interface{}{"FromChannelId": fromChannelId, "FromUserId": fromUserId}); err != nil { - result.Err = model.NewAppError("SqlChannelStore.MigrateChannelMembers", "store.sql_channel.migrate_channel_members.select.app_error", nil, err.Error(), http.StatusInternalServerError) - return + if _, err := transaction.Update(&member); err != nil { + return nil, model.NewAppError("SqlChannelStore.MigrateChannelMembers", "store.sql_channel.migrate_channel_members.update.app_error", nil, err.Error(), http.StatusInternalServerError) } - if len(channelMembers) == 0 { - // No more channel members in query result means that the migration has finished. - return - } + } - for _, member := range channelMembers { - roles := strings.Fields(member.Roles) - var newRoles []string - if !member.SchemeAdmin.Valid { - member.SchemeAdmin = sql.NullBool{Bool: false, Valid: true} - } - if !member.SchemeUser.Valid { - member.SchemeUser = sql.NullBool{Bool: false, Valid: true} - } - if !member.SchemeGuest.Valid { - member.SchemeGuest = sql.NullBool{Bool: false, Valid: true} - } - for _, role := range roles { - if role == model.CHANNEL_ADMIN_ROLE_ID { - member.SchemeAdmin = sql.NullBool{Bool: true, Valid: true} - } else if role == model.CHANNEL_USER_ROLE_ID { - member.SchemeUser = sql.NullBool{Bool: true, Valid: true} - } else if role == model.CHANNEL_GUEST_ROLE_ID { - member.SchemeGuest = sql.NullBool{Bool: true, Valid: true} - } else { - newRoles = append(newRoles, role) - } - } - member.Roles = strings.Join(newRoles, " ") + if err := transaction.Commit(); err != nil { + return nil, model.NewAppError("SqlChannelStore.MigrateChannelMembers", "store.sql_channel.migrate_channel_members.commit_transaction.app_error", nil, err.Error(), http.StatusInternalServerError) + } - if _, err := transaction.Update(&member); err != nil { - result.Err = model.NewAppError("SqlChannelStore.MigrateChannelMembers", "store.sql_channel.migrate_channel_members.update.app_error", nil, err.Error(), http.StatusInternalServerError) - return - } - - } - - if err := transaction.Commit(); err != nil { - result.Err = model.NewAppError("SqlChannelStore.MigrateChannelMembers", "store.sql_channel.migrate_channel_members.commit_transaction.app_error", nil, err.Error(), http.StatusInternalServerError) - return - } - - data := make(map[string]string) - data["ChannelId"] = channelMembers[len(channelMembers)-1].ChannelId - data["UserId"] = channelMembers[len(channelMembers)-1].UserId - result.Data = data - }) + data := make(map[string]string) + data["ChannelId"] = channelMembers[len(channelMembers)-1].ChannelId + data["UserId"] = channelMembers[len(channelMembers)-1].UserId + return data, nil } func (s SqlChannelStore) ResetAllChannelSchemes() store.StoreChannel { diff --git a/store/store.go b/store/store.go index 4b52224682..af12a25b47 100644 --- a/store/store.go +++ b/store/store.go @@ -190,7 +190,7 @@ type ChannelStore interface { GetChannelUnread(channelId, userId string) (*model.ChannelUnread, *model.AppError) ClearCaches() GetChannelsByScheme(schemeId string, offset int, limit int) StoreChannel - MigrateChannelMembers(fromChannelId string, fromUserId string) StoreChannel + MigrateChannelMembers(fromChannelId string, fromUserId string) (map[string]string, *model.AppError) ResetAllChannelSchemes() StoreChannel ClearAllCustomRoleAssignments() *model.AppError MigratePublicChannels() error diff --git a/store/storetest/channel_store.go b/store/storetest/channel_store.go index e00a1f2fc9..e9f89c572c 100644 --- a/store/storetest/channel_store.go +++ b/store/storetest/channel_store.go @@ -2966,12 +2966,11 @@ func testChannelStoreMigrateChannelMembers(t *testing.T, ss store.Store) { lastDoneUserId := strings.Repeat("0", 26) for { - res := <-ss.Channel().MigrateChannelMembers(lastDoneChannelId, lastDoneUserId) - if assert.Nil(t, res.Err) { - if res.Data == nil { + data, err := ss.Channel().MigrateChannelMembers(lastDoneChannelId, lastDoneUserId) + if assert.Nil(t, err) { + if data == nil { break } - data := res.Data.(map[string]string) lastDoneChannelId = data["ChannelId"] lastDoneUserId = data["UserId"] } diff --git a/store/storetest/mocks/ChannelStore.go b/store/storetest/mocks/ChannelStore.go index 524b25fc71..9b012911a2 100644 --- a/store/storetest/mocks/ChannelStore.go +++ b/store/storetest/mocks/ChannelStore.go @@ -983,19 +983,28 @@ func (_m *ChannelStore) IsUserInChannelUseCache(userId string, channelId string) } // MigrateChannelMembers provides a mock function with given fields: fromChannelId, fromUserId -func (_m *ChannelStore) MigrateChannelMembers(fromChannelId string, fromUserId string) store.StoreChannel { +func (_m *ChannelStore) MigrateChannelMembers(fromChannelId string, fromUserId string) (map[string]string, *model.AppError) { ret := _m.Called(fromChannelId, fromUserId) - var r0 store.StoreChannel - if rf, ok := ret.Get(0).(func(string, string) store.StoreChannel); ok { + var r0 map[string]string + if rf, ok := ret.Get(0).(func(string, string) map[string]string); ok { r0 = rf(fromChannelId, fromUserId) } else { if ret.Get(0) != nil { - r0 = ret.Get(0).(store.StoreChannel) + r0 = ret.Get(0).(map[string]string) } } - return r0 + var r1 *model.AppError + if rf, ok := ret.Get(1).(func(string, string) *model.AppError); ok { + r1 = rf(fromChannelId, fromUserId) + } else { + if ret.Get(1) != nil { + r1 = ret.Get(1).(*model.AppError) + } + } + + return r0, r1 } // MigratePublicChannels provides a mock function with given fields: